1307677 - Move some code from MDSM::InitiateSeek to state objects

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1307677. Part 1 - move some code from Seek() to various state objects.

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

Comment 2

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

Attachment: Bug 1307677. Part 2 - add some assertion to DecodingFirstFrameState::HandleSeek().

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

Comment 3

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

Attachment: Bug 1307677. Part 3 - move |mCurrentSeek.mPromise.Ensure| out of InitiateSeek(). This makes it easier to move more code into SeekingState.

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

Comment 4

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

Attachment: Bug 1307677. Part 4 - prepare to move most code of InitiateSeek() into the entry action of SeekingState.

Note we can't call SetState() because we might need to seek again in the SEEKING state.

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

Comment 5

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

Attachment: Bug 1307677. Part 5 - move most code of InitiateSeek() into the entry action of SeekingState.

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

Comment 6

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

Attachment: Bug 1307677. Part 6 - don't access mMaster after InitiateSeek() which will delete the current state object.

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

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797905 [details]
Bug 1307677. Part 1 - move some code from Seek() to various state objects.

https://reviewboard.mozilla.org/r/83498/#review82104

::: dom/media/MediaDecoderStateMachine.cpp:487
(Diff revision 1)
>      return true;
>    }
>  
> +  RefPtr<MediaDecoder::SeekPromise> HandleSeek(SeekTarget aTarget) override
> +  {
> +    // Should've transitioned to DECODING in Enter().

Could we make this comment more explicit, it took me a while to understand..., such as "If mMaster->mSentFirstFrameLoadedEvent is true, then we should have transitioned to DECODING sate in Enter()"?

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797906 [details]
Bug 1307677. Part 2 - add some assertion to DecodingFirstFrameState::HandleSeek().

https://reviewboard.mozilla.org/r/83500/#review82130

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797907 [details]
Bug 1307677. Part 3 - move |mCurrentSeek.mPromise.Ensure| out of InitiateSeek(). This makes it easier to move more code into SeekingState.

https://reviewboard.mozilla.org/r/83502/#review82132

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797908 [details]
Bug 1307677. Part 4 - prepare to move most code of InitiateSeek() into the entry action of SeekingState.

https://reviewboard.mozilla.org/r/83504/#review82108

::: dom/media/MediaDecoderStateMachine.cpp:2114
(Diff revision 1)
>  void
>  MediaDecoderStateMachine::InitiateSeek(SeekJob aSeekJob)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>  
> -  SetState(DECODER_STATE_SEEKING);
> +  mStateObj->Exit();

I think it's worthy having the comment "Note we can't call SetState() because we might need to seek again in the SEEKING state." here.

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797909 [details]
Bug 1307677. Part 5 - move most code of InitiateSeek() into the entry action of SeekingState.

https://reviewboard.mozilla.org/r/83506/#review82134

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8797910 [details]
Bug 1307677. Part 6 - don't access mMaster after InitiateSeek() which will delete the current state object.

https://reviewboard.mozilla.org/r/83508/#review82136

Comment 13

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

Thanks for the review!

Comment 14

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

Comment on attachment 8797905 [details]
Bug 1307677. Part 1 - move some code from Seek() to various state objects.

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

Comment 15

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

Comment on attachment 8797906 [details]
Bug 1307677. Part 2 - add some assertion to DecodingFirstFrameState::HandleSeek().

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

Comment 16

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

Comment on attachment 8797907 [details]
Bug 1307677. Part 3 - move |mCurrentSeek.mPromise.Ensure| out of InitiateSeek(). This makes it easier to move more code into SeekingState.

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

Comment 17

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

Comment on attachment 8797908 [details]
Bug 1307677. Part 4 - prepare to move most code of InitiateSeek() into the entry action of SeekingState.

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

Comment 18

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

Comment on attachment 8797909 [details]
Bug 1307677. Part 5 - move most code of InitiateSeek() into the entry action of SeekingState.

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

Comment 19

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

Comment on attachment 8797910 [details]
Bug 1307677. Part 6 - don't access mMaster after InitiateSeek() which will delete the current state object.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aad3743b683e
Part 1 - move some code from Seek() to various state objects. r=kaku
https://hg.mozilla.org/integration/autoland/rev/c873b164dd0b
Part 2 - add some assertion to DecodingFirstFrameState::HandleSeek(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/f7fa85291b36
Part 3 - move |mCurrentSeek.mPromise.Ensure| out of InitiateSeek(). This makes it easier to move more code into SeekingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/fa6fa05f85fd
Part 4 - prepare to move most code of InitiateSeek() into the entry action of SeekingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/d8b4fa10a177
Part 5 - move most code of InitiateSeek() into the entry action of SeekingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/2108248c4021
Part 6 - don't access mMaster after InitiateSeek() which will delete the current state object. r=kaku

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aad3743b683e
https://hg.mozilla.org/mozilla-central/rev/c873b164dd0b
https://hg.mozilla.org/mozilla-central/rev/f7fa85291b36
https://hg.mozilla.org/mozilla-central/rev/fa6fa05f85fd
https://hg.mozilla.org/mozilla-central/rev/d8b4fa10a177
https://hg.mozilla.org/mozilla-central/rev/2108248c4021