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?

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?

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.

I seemed to hit this on YouTube when I switched from 720p to 1080p on some GoPro Hero videos.

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

Removing from beta blockers because it is not caused by MSE and therefore would still occur in regular video elements on YouTube.

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.

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.

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?

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.

https://hg.mozilla.org/releases/mozilla-aurora/rev/5752d3a428d4 https://hg.mozilla.org/releases/mozilla-beta/rev/f82a118e1064