Closed Bug 1308147 Opened 3 years ago Closed 3 years ago

Move MDSM::mQueuedSeek into state objects

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

mQueuedSeek is needed only for states that can't handle seek immediately.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8803232 - Flags: review?(kikuo)
Attachment #8803233 - Flags: review?(kikuo)
Attachment #8803234 - Flags: review?(kikuo)
Attachment #8803235 - Flags: review?(kikuo)
Attachment #8803236 - Flags: review?(kikuo)
Attachment #8803237 - Flags: review?(kikuo)
Blocks: 1311872
Comment on attachment 8803232 [details]
Bug 1308147. Part 1 - add WaitForCDMState::mPendingSeek to store a pending seek job.

https://reviewboard.mozilla.org/r/87544/#review86546
Attachment #8803232 - Flags: review?(kikuo) → review+
Comment on attachment 8803233 [details]
Bug 1308147. Part 2 - add DormantState::mPendingSeek to store a pending seek job.

https://reviewboard.mozilla.org/r/87546/#review86838
Attachment #8803233 - Flags: review?(kikuo) → review+
Comment on attachment 8803234 [details]
Bug 1308147. Part 3 - add DecodingFirstFrameState::mPendingSeek to store a pending seek job.

https://reviewboard.mozilla.org/r/87548/#review86846

::: dom/media/MediaDecoderStateMachine.cpp:1299
(Diff revision 1)
>  DormantState::HandleDormant(bool aDormant)
>  {
>    if (!aDormant) {
>      MOZ_ASSERT(!Info().IsEncrypted() || mMaster->mCDMProxy);
> -    SetState<DecodingFirstFrameState>();
> +    SeekJob seekJob = Move(mPendingSeek);
> +    SetState<DecodingFirstFrameState>(Move(seekJob));

Hi JW,
Could this two lines be merged into SetState<DecodingFirstFrameState>(Move(mPendingSeek));  ?

Since the move happened in line 262[1] and you use deathgrip to ensure the destruction will happen after this function return.

Any special reason for seperated into two lines?
[1]
http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/dom/media/MediaDecoderStateMachine.cpp#258,262
Comment on attachment 8803234 [details]
Bug 1308147. Part 3 - add DecodingFirstFrameState::mPendingSeek to store a pending seek job.

https://reviewboard.mozilla.org/r/87548/#review86846

> Hi JW,
> Could this two lines be merged into SetState<DecodingFirstFrameState>(Move(mPendingSeek));  ?
> 
> Since the move happened in line 262[1] and you use deathgrip to ensure the destruction will happen after this function return.
> 
> Any special reason for seperated into two lines?
> [1]
> http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/dom/media/MediaDecoderStateMachine.cpp#258,262

No, we can't. See bug 1312321 for the detail.
Comment on attachment 8803234 [details]
Bug 1308147. Part 3 - add DecodingFirstFrameState::mPendingSeek to store a pending seek job.

https://reviewboard.mozilla.org/r/87548/#review86862
Attachment #8803234 - Flags: review?(kikuo) → review+
Comment on attachment 8803235 [details]
Bug 1308147. Part 4 - reject mPendingSeek in WaitForCDMState::Exit().

https://reviewboard.mozilla.org/r/87550/#review86866
Attachment #8803235 - Flags: review?(kikuo) → review+
Comment on attachment 8803236 [details]
Bug 1308147. Part 5 - reject mPendingSeek in DormantState::Exit().

https://reviewboard.mozilla.org/r/87552/#review86868
Attachment #8803236 - Flags: review?(kikuo) → review+
Comment on attachment 8803237 [details]
Bug 1308147. Part 6 - remove unused MDSM::mQueuedSeek for there is no code to modify it.

https://reviewboard.mozilla.org/r/87554/#review86914

Cool stuff !
Attachment #8803237 - Flags: review?(kikuo) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03952d792757
Part 1 - add WaitForCDMState::mPendingSeek to store a pending seek job. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/736157befbd0
Part 2 - add DormantState::mPendingSeek to store a pending seek job. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/3bd04c577eed
Part 3 - add DecodingFirstFrameState::mPendingSeek to store a pending seek job. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/5020b5928284
Part 4 - reject mPendingSeek in WaitForCDMState::Exit(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/e72768d9d633
Part 5 - reject mPendingSeek in DormantState::Exit(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/67e846eb085b
Part 6 - remove unused MDSM::mQueuedSeek for there is no code to modify it. r=kikuo
You need to log in before you can comment on or make changes to this bug.