Closed
Bug 1302040
Opened 7 years ago
Closed 7 years ago
MDSM might enter buffering state while still prerolling
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
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 | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8792372 -
Flags: review?(cpearce)
Comment 2•7 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•7 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•7 years ago
|
||
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2af2eaac5b48
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•