Closed Bug 1329554 Opened 5 years ago Closed 5 years ago

Move the call to DispatchDecodeTasksIfNeeded() from PlayStateChanged() into DecodingState

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/media/MediaDecoderStateMachine.cpp#2870

DecodingState is in charge of decoding and should be the only state which cares about the change in mMinimizePreroll.
Assignee: nobody → jwwang
Blocks: 1324999
Priority: -- → P3
Attachment #8824854 - Flags: review?(kikuo)
Attachment #8824855 - Flags: review?(kikuo)
Comment on attachment 8824854 [details]
Bug 1329554. Part 1 - let DecodingState decide whether to dispatch decoding tasks when mMinimizePreroll changes.

https://reviewboard.mozilla.org/r/103154/#review103738

::: dom/media/MediaDecoderStateMachine.cpp:635
(Diff revision 1)
>    void HandlePlayStateChanged(MediaDecoder::PlayState aPlayState) override
>    {
>      if (aPlayState == MediaDecoder::PLAY_STATE_PLAYING) {
>        // Schedule Step() to check if we can start playback.
>        mMaster->ScheduleStateMachine();
> +      // Try to dispatch decoding tasks for mMinimizePreroll might be reset.

It seems grammatically incorrect to me.

How about this ?
"... for mMiniziePreroll *which* might be reset."
Attachment #8824854 - Flags: review?(kikuo) → review+
Comment on attachment 8824855 [details]
Bug 1329554. Part 2 - remove |mMaster->mMinimizePreroll| checks from BufferingState::Step().

https://reviewboard.mozilla.org/r/103156/#review103740
Attachment #8824855 - Flags: review?(kikuo) → review+
Comment on attachment 8824854 [details]
Bug 1329554. Part 1 - let DecodingState decide whether to dispatch decoding tasks when mMinimizePreroll changes.

https://reviewboard.mozilla.org/r/103154/#review103738

> It seems grammatically incorrect to me.
> 
> How about this ?
> "... for mMiniziePreroll *which* might be reset."

It means "Try to dispatch decoding tasks _because_ mMinimizePreroll might be reset."
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70bec73165a5
Part 1 - let DecodingState decide whether to dispatch decoding tasks when mMinimizePreroll changes. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/7ec0bc99f5cf
Part 2 - remove |mMaster->mMinimizePreroll| checks from BufferingState::Step(). r=kikuo
https://hg.mozilla.org/mozilla-central/rev/70bec73165a5
https://hg.mozilla.org/mozilla-central/rev/7ec0bc99f5cf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.