-
Notifications
You must be signed in to change notification settings - Fork 213
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
Ensure that the done
event is triggered even if the segment does not contain audio/video data
#224
base: master
Are you sure you want to change the base?
Conversation
Woohoo! Thank you! |
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.
Looks good, thanks @mjneil . We may just want to add a test or two for this case.
@@ -356,7 +356,7 @@ <h3>footer</h3> | |||
prepareSourceBuffer(combined, outputType, function () { | |||
console.log('appending...'); | |||
window.vjsBuffer.appendBuffer(bytes); | |||
video.play(); | |||
// video.play(); |
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.
Were these meant to be commented out?
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.
Oops. Autoplay on an airplane with sound turned on. I can revert this, though I think its a bit annoying to keep the autoplay
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 video.volume = 0
, no? :D
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.
All of our automated tests have to be run with video.muted = true;
to avoid autoplay restrictions breaking our tests. YMMV.
lib/mp4/transmuxer.js
Outdated
} | ||
if (flushSource !== 'VideoSegmentStream' && flushSource !== 'AudioSegmentStream') { | ||
// Return because we haven't received a flush from a data-generating | ||
// portion of the segment (meaning that we have only recieved meta-data |
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.
metadata
@mjneil can you find a spare minute and add a test? |
The issue seems to still exist - was this PR abandoned or just postponed? |
Fixes #194
This PR also makes the output
type
of thedata
events more consistent. Ifremux === true
and both audio and video are specified as tracks in the PMT, the output type will always becombined
even if the particular segment is missing audio or video data.This change primarily makes sure that the transmuxer doesn't get stuck in a state waiting for content that will never come. It doesn't necessarily guarantee the outputted fmp4 will play in the browser without issue because the init segment will say that the fragment will contain both tracks even if one has no data.
e.g. chrome://media-internals may have an error similar to