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)

Assignee

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → jwwang
Blocks: 1300711, 1295892
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8792372 - Flags: review?(cpearce)

Comment 2

3 years ago
mozreview-review
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+
Assignee

Comment 3

3 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Assignee

Comment 5

3 years ago
Thanks for the review!

Comment 6

3 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2af2eaac5b48
don't enter buffering while prerolling. r=cpearce

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2af2eaac5b48
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.