-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(VideoAsset): handle unplayable videos [WPB-11667] #18200
feat(VideoAsset): handle unplayable videos [WPB-11667] #18200
Conversation
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #18200 +/- ##
=======================================
Coverage 46.39% 46.40%
=======================================
Files 797 800 +3
Lines 25798 25827 +29
Branches 5886 5888 +2
=======================================
+ Hits 11970 11984 +14
- Misses 12326 12337 +11
- Partials 1502 1506 +4 |
We want to display big placeholder for unsupported video? :) |
@przemvs It's the size of the video player, hmm I wouldn't shrink that down. Imagine you click the play button, and then, because of the error, the player disappears and some smaller error message shows up 😕 Screen.Recording.2024-10-24.at.09.26.15.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Added few comments, please have a look.
// It's not 100% reliable (e.g. doesn't check codecs), but it's synchorous, which is helpful for initial rendering. | ||
const isVideoMimeTypeSupported = (mimeType: string): boolean => { | ||
const video = document.createElement('video'); | ||
const canPlay = video.canPlayType(mimeType) !== ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, if we create an enum for the return types it would be more readable:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/canPlayType#return_value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's necessary because we don't use it directly - we just check that it's not an empty string.
@@ -153,6 +205,7 @@ const VideoAsset: React.FC<VideoAssetProps> = ({ | |||
poster={videoPreview?.url} | |||
onError={event => { | |||
setVideoPlaybackError(true); | |||
amplify.publish(WebAppEvents.ANALYTICS.EVENT, EventName.MESSAGES.VIDEO.PLAY_FAILED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 4 place where we logged PLAY_FAILED
event.
Just curious if it will only log once per error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and it shouldn't fire a few at once.
- fires when mime type is not supported (the simple check)
- fires when a video is not playable (the advanced check)
- fires when something fails in the
try{}
block insideonPlayButtonClicked
(e.g.getAssetUrl
function) - fires on the video error callback - I guess now it's not gonna be a common case (because we handled unplayable videos before that), but in case something fails, then we can log it
I wanted to add logs in many places, so that we can later analyse how many videos were played successfully, how many ended with "unsupported mime type", and how many failed due to unsupported codecs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explanation 🚀
Description
Handles unsupported videos:
I added logs to Countly, let's measure how big the problem is.
Checklist