Closed Bug 1302040 Opened 5 years ago Closed 5 years ago

MDSM might enter buffering state while still prerolling

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/dom/media/MediaDecoderStateMachine.cpp#1184

It is possible to enter buffering when mPlayState is MediaDecoder::PLAY_STATE_PLAYING and IsPlaying() is false which means we are still prerolling.

We certainly don't want to enter buffering state while prerolling.
Assignee: nobody → jwwang
Blocks: 1300711, 1295892
Priority: -- → P3
Attachment #8792372 - Flags: review?(cpearce)
Comment on attachment 8792372 [details]
Bug 1302040 - don't enter buffering while prerolling.

https://reviewboard.mozilla.org/r/79436/#review78308

::: dom/media/MediaDecoderStateMachine.cpp:1376
(Diff revision 1)
> -  if (mPlayState == MediaDecoder::PLAY_STATE_PLAYING &&
> -      mResource->IsExpectingMoreData()) {
> +  // Don't enter buffering when MediaDecoder is not playing.
> +  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING) {
> +    return;
> +  }
> +
> +  // We are still prerolling. Don't enter buffering.

I suggest you have this comment also say something to the effect of "Don't enter buffering, so that the decoder has a chance to enqueue some decoded data before we give up and start buffering."


You might want to (statically?) assert (somewhere) that the threshold of decoded data that after which we no longer consider ourselves prerolling is sufficiently larger than the low data threshold, so that we don't end up jumping back to buffering immediately once we exit prerolling state.
Attachment #8792372 - Flags: review?(cpearce) → review+
Comment on attachment 8792372 [details]
Bug 1302040 - don't enter buffering while prerolling.

https://reviewboard.mozilla.org/r/79436/#review78308

> I suggest you have this comment also say something to the effect of "Don't enter buffering, so that the decoder has a chance to enqueue some decoded data before we give up and start buffering."
> 
> 
> You might want to (statically?) assert (somewhere) that the threshold of decoded data that after which we no longer consider ourselves prerolling is sufficiently larger than the low data threshold, so that we don't end up jumping back to buffering immediately once we exit prerolling state.

http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/dom/media/MediaDecoderStateMachine.cpp#2236
HasLowDecodedData() doesn't check mPlaybackRate as DonePrerolling{Audio,Video} does. I am sure it will fail the assertion when mPlaybackRate != 1. I will file a new bug to fix that and add the assertion as you suggested.
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2af2eaac5b48
don't enter buffering while prerolling. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/2af2eaac5b48
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.