1294615 - Simplify the code of MediaDecoderStateMachine::SetDormant()

Status: RESOLVED FIXED

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

Description

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

We will prove mQueuedSeek.Exists() is always false and mCurrentSeek.Exists() is always true when seek is is action. This allows us to remove some checks from SetDormant() to simplify the code.

Comment 1

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

Attachment: Bug 1294615. Part 1 - Assert mQueuedSeek.Exists() is false in InitiateSeek().

InitiateSeek() is called from several places where we can prove mQueuedSeek.Exists() is false:
1. MaybeFinishDecodeFirstFrame(): mQueuedSeek is moved when calling InitiateSeek().
2. Seek() rejects mQueuedSeek before calling InitiateSeek().
3. StartDecoding(): mQueuedSeek is moved when calling InitiateSeek().
4. VisibilityChanged(): it doesn't call InitiateSeek() when mQueuedSeek.Exists() is true.

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

Comment 2

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

Attachment: Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

We've proven that when seek is in action, mQueuedSeek.Exists() is always false and mCurrentSeek.Exists() is always true.

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

Comment 3

[Tzuhao Kuo [:kaku]]

Comment on attachment 8780389 [details]
Bug 1294615. Part 1 - Assert mQueuedSeek.Exists() is false in InitiateSeek().

https://reviewboard.mozilla.org/r/71114/#review68618

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

https://reviewboard.mozilla.org/r/71116/#review68624

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
> -        // are handled.
> +      // are handled.
> -        if (mCurrentSeek.mTarget.IsVideoOnly()) {
> +      if (mCurrentSeek.mTarget.IsVideoOnly()) {
> -          mCurrentSeek.mTarget.SetType(SeekTarget::Accurate);
> +        mCurrentSeek.mTarget.SetType(SeekTarget::Accurate);
> -        }
> +      }
> -        mQueuedSeek = Move(mCurrentSeek);
> +      mQueuedSeek = Move(mCurrentSeek);
> -        mSeekTaskRequest.DisconnectIfExists();

Shouldn't we still disconnect mSeekRequest? Otherwise we will hit the assertion of |mState == DECODER_STATE_SEEKING| in OnSeekTaskResolved()/OnSeekTaskRejected().

Comment 5

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

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

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

Comment 6

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

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

https://reviewboard.mozilla.org/r/71116/#review68624

> Shouldn't we still disconnect mSeekRequest? Otherwise we will hit the assertion of |mState == DECODER_STATE_SEEKING| in OnSeekTaskResolved()/OnSeekTaskRejected().

It is already done in Reset().

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

https://reviewboard.mozilla.org/r/71116/#review68636

Comment 8

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

Thanks!

Comment 9

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

Comment on attachment 8780389 [details]
Bug 1294615. Part 1 - Assert mQueuedSeek.Exists() is false in InitiateSeek().

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

Comment 10

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

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

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

Comment 11

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

Comment on attachment 8780389 [details]
Bug 1294615. Part 1 - Assert mQueuedSeek.Exists() is false in InitiateSeek().

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

Comment 12

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

Comment on attachment 8780390 [details]
Bug 1294615. Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71116/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da868fdc53aa
Part 1 - Assert mQueuedSeek.Exists() is false in InitiateSeek(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/d4ac66433eeb
Part 2 - Refactor MDSM::SetDormant() to remove unnecessary checks for mQueuedSeek.Exists() and mCurrentSeek.Exists(). r=kaku

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da868fdc53aa
https://hg.mozilla.org/mozilla-central/rev/d4ac66433eeb