1302040 - MDSM might enter buffering state while still prerolling

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1302040 - don't enter buffering while prerolling.

Review commit: https://reviewboard.mozilla.org/r/79436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79436/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8792372 [details]
Bug 1302040 - don't enter buffering while prerolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79436/diff/1-2/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2af2eaac5b48
don't enter buffering while prerolling. r=cpearce

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2af2eaac5b48