Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mjneil
Copy link
Contributor

@mjneil mjneil commented Oct 16, 2018

Fixes #194

This PR also makes the output type of the data events more consistent. If remux === true and both audio and video are specified as tracks in the PMT, the output type will always be combined 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

Media segment did not contain any coded frames for track 257, mismatching initialization segment. Therefore, MSE coded frame processing may not interoperably detect discontinuities in appended media.

@joeyparrish
Copy link
Contributor

Woohoo! Thank you!

Copy link
Contributor

@gesinger gesinger left a 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();
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor

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.

}
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metadata

@gkatsev
Copy link
Member

gkatsev commented Mar 22, 2019

@mjneil can you find a spare minute and add a test?

@perqa
Copy link

perqa commented Oct 7, 2022

The issue seems to still exist - was this PR abandoned or just postponed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants