Bypass skipToKeyframe logic in MediaDecoderStateMachine when we have a fully async decoder

RESOLVED FIXED in Firefox 36

Status

()

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

29 Branch
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
This code exists to stop video decoding from blocking audio, but this shouldn't be an issue with a fully async decoder.

Chris, do you think we want to remove it entirely, or just make it much more lenient?
(Reporter)

Comment 1

4 years ago
Created attachment 8537601 [details] [diff] [review]
Avoid skipToKeyframe for async decoders
Attachment #8537601 - Flags: review?(cpearce)
(Reporter)

Updated

4 years ago
Blocks: 778617
(Assignee)

Comment 2

4 years ago
Comment on attachment 8537601 [details] [diff] [review]
Avoid skipToKeyframe for async decoders

Review of attachment 8537601 [details] [diff] [review]:
-----------------------------------------------------------------

We can't bypass skip-to-next-keyframe entirely, as that broke A/V sync when I tested on low end hardware; the video decode never caught up once it fell behind.

I think we should instead make it more lenient when the reader is async.

Would ignoring the audio when calculating whether we should enter skip-to-next-keyframe for async readers work, i.e.:

    if (mState == DECODER_STATE_DECODING &&
        IsVideoDecoding() &&
        ((!mReader->IsAsync() && mIsAudioPrerolling && IsAudioDecoding() &&
          GetDecodedAudioDuration() < mLowAudioThresholdUsecs * mPlaybackRate) ||
          (!mIsVideoPrerolling && IsVideoDecoding() &&
           // don't skip frame when |clock time| <= |mVideoFrameEndTime| for
           // we are still in the safe range without underrunning video frames
           GetClock() > mVideoFrameEndTime &&
          (static_cast<uint32_t>(VideoQueue().GetSize())
            < LOW_VIDEO_FRAMES * mPlaybackRate))) &&
        !HasLowUndecodedData())

That may not work. We may need to come up with something else.

Why is the skip-to-next-keyframe logic triggering with MSE? Is the amount of buffered or decoded data running low when you switch decoders?
Attachment #8537601 - Flags: review?(cpearce) → review-
See Also: → bug 1091992
(Reporter)

Comment 3

4 years ago
Unfortunately I can't reproduce this happening any more, so I'm not sure. We're certainly not running out of decoded frames now, but that might not have been the case when I saw this previously.
Priority: -- → P1
(Assignee)

Comment 4

4 years ago
I seemed to hit this on YouTube when I switched from 720p to 1080p on some GoPro Hero videos.
(Assignee)

Comment 5

4 years ago
It might be worth testing the patch in Bug 1091992 to see if that makes things better here.

That patch checks how far ahead of the current playback position we've decoded the video, rather than just assuming that when the video queue is empty that we should skip. We could try making that video skip test more lenient too.
The issues is the core cause for bug 1107737
Blocks: 1107737

Updated

4 years ago
Blocks: 1118945
Removing from beta blockers because it is not caused by MSE and therefore would still occur in regular video elements on YouTube.
No longer blocks: 1118945
(Assignee)

Updated

4 years ago
Assignee: matt.woodrow → cpearce
(Assignee)

Comment 8

4 years ago
Created attachment 8548006 [details] [diff] [review]
Patch: don't consider audio in NeedToSkipToNextKeyframe() for async readers

Don't consider audio when we determine whether we should skip-to-next-keyframe when we're using an async reader. Also don't increase the low audio threshold; since for async readers the video and audio decode happens on different task queues, there's nothing to be gained by increasing the threshold when we do engage skip-to-next-keyframe.
Attachment #8537601 - Attachment is obsolete: true
Attachment #8548006 - Flags: review?(matt.woodrow)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8548006 [details] [diff] [review]
Patch: don't consider audio in NeedToSkipToNextKeyframe() for async readers

Review of attachment 8548006 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/MediaSourceReader.h
@@ +130,5 @@
>  #endif
>  
> +  virtual bool IsAsync() const MOZ_OVERRIDE {
> +    return (!mAudioReader || mAudioReader->IsAsync()) &&
> +           (!mVideoReader || mVideoReader->IsAsync());

I'm pretty sure we require that the readers are of the same type, so you could probably just assert that they return matching results instead of checking both.
Attachment #8548006 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 10

4 years ago
Comment on attachment 8548006 [details] [diff] [review]
Patch: don't consider audio in NeedToSkipToNextKeyframe() for async readers

Review of attachment 8548006 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/MediaSourceReader.h
@@ +130,5 @@
>  #endif
>  
> +  virtual bool IsAsync() const MOZ_OVERRIDE {
> +    return (!mAudioReader || mAudioReader->IsAsync()) &&
> +           (!mVideoReader || mVideoReader->IsAsync());

Once WebM MSE is enabled, couldn't a vorbis sourcebuffer be peered with an h264 sourcebuffer, and since the WebMReader is stil synchronous, we'd then be mixing async and sync modes?
https://hg.mozilla.org/mozilla-central/rev/6ceb11cd7fc1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8548006 [details] [diff] [review]
Patch: don't consider audio in NeedToSkipToNextKeyframe() for async readers

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Some videos stutter or freeze during playback. [Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This changes how we manage playback resources for all video playback, but is a straightforward change and isolated change. I'd say risk is low.
[String/UUID change made/needed]: None.
Attachment #8548006 - Flags: approval-mozilla-beta?
Attachment #8548006 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8548006 - Flags: approval-mozilla-beta?
Attachment #8548006 - Flags: approval-mozilla-beta+
Attachment #8548006 - Flags: approval-mozilla-aurora?
Attachment #8548006 - Flags: approval-mozilla-aurora+
status-firefox36: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5752d3a428d4
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed

Updated

4 years ago
Depends on: 1178063
You need to log in before you can comment on or make changes to this bug.