MDSM might enter buffering state while still prerolling

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.