Move MDSM::mQueuedSeek into state objects

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

a year ago
mQueuedSeek is needed only for states that can't handle seek immediately.
(Assignee)

Updated

a year ago
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
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)
(Assignee)

Updated

a year ago
Blocks: 1311872

Comment 7

a year ago
mozreview-review
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 8

a year ago
mozreview-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 9

a year ago
mozreview-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
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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 11

a year ago
mozreview-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/#review86862
Attachment #8803234 - Flags: review?(kikuo) → review+

Comment 12

a year ago
mozreview-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 13

a year ago
mozreview-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 14

a year ago
mozreview-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+
(Assignee)

Comment 15

a year ago
Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
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

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03952d792757
https://hg.mozilla.org/mozilla-central/rev/736157befbd0
https://hg.mozilla.org/mozilla-central/rev/3bd04c577eed
https://hg.mozilla.org/mozilla-central/rev/5020b5928284
https://hg.mozilla.org/mozilla-central/rev/e72768d9d633
https://hg.mozilla.org/mozilla-central/rev/67e846eb085b
Status: NEW → RESOLVED
Last Resolved: a year 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.