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

fix: detect demuxed video underflow gaps #948

Merged
merged 4 commits into from
Sep 23, 2020
Merged

Conversation

brandonocasey
Copy link
Contributor

Description

We are seeing a video underflow for demuxed fmp4 content. I think the main issue is timing related, but I have yet to narrow down the cause. While going through our code I noticed that we already detect video underflows for muxed content. I added code to detect demuxed video underflows so that we can fix those as well.

@gkatsev gkatsev added this to the 2.2 milestone Sep 10, 2020
let gap;

if (videoBuffered.length && audioBuffered.length) {
const lastVideoRange = Ranges.findRange(videoBuffered, currentTime - 3);
Copy link
Member

Choose a reason for hiding this comment

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

why the -3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gapFromVideoUnderflow implies that audio will keep playing for ~3 seconds during a video underflow and that is what I have seen as well. I will add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a fudge to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that we shouldn't as 3 is already a "fudge" here.

// Even if there is no available next range, there is still a possibility we are
// stuck in a gap due to video underflow.
const gap = this.gapFromVideoUnderflow_(buffered, currentTime);
Copy link
Member

Choose a reason for hiding this comment

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

and this generic buffered gap check isn't necessary because we still check for this gap above which may not be a video underflow? Or is this equivalent to checking against videoBuffered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generic buffered only worked when the it was referring to muxed content because we will go off of the "video buffer", which is really the muxed content, in that case. Now that we have a case that checks for demuxed content we can just use videoBuffered for the original muxed check and both buffers for the demuxed check.

acc += val;

if (val.stack) {
acc += '\n' + val.stack;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually print out log error stack traces when we assert.

src/playback-watcher.js Outdated Show resolved Hide resolved
src/playback-watcher.js Outdated Show resolved Hide resolved
let gap;

if (videoBuffered.length && audioBuffered.length) {
const lastVideoRange = Ranges.findRange(videoBuffered, currentTime - 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a fudge to it?

test/playback-watcher.test.js Outdated Show resolved Hide resolved
@gkatsev gkatsev merged commit d0ef298 into main Sep 23, 2020
@gkatsev gkatsev deleted the fix/demuxed-underflow branch September 23, 2020 17:31
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.

3 participants