1355031 - Make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

VideoOnlySeekBegin and VideoOnlySeekCompleted are used to notify the beginning and completion of the video-only seek operation. However, a seek operation could be canceled and we didn't dispatch VideoOnlySeekCompleted in that case. In this bug, we make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1355031 P1 - adjust the event order;

(EnterVideoSuspend/ExitVideoSuspend) event pair and (VideoOnlySeekBegin/VideoOnlySeekCompleted) event pair should not interleave each other.

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

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1355031 P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair;

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

Comment 3

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df6adc06643ccefe9da1cf29ef0ec358426c0c74
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b7e122e7be4c8d129363c0173e4912aa469967

Comment 4

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

Comment on attachment 8856421 [details]
Bug 1355031 P1 - adjust the event order;

https://reviewboard.mozilla.org/r/128370/#review131180

Comment 5

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

Comment on attachment 8856422 [details]
Bug 1355031 P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair;

https://reviewboard.mozilla.org/r/128372/#review131184

::: dom/media/MediaDecoderStateMachine.cpp:1041
(Diff revision 1)
>      return SeekingState::Enter(Move(aSeekJob), aVisibility);
>    }
>  
>    void Exit() override
>    {
> +    if (mSeekJob.Exists() && mSeekJob.mTarget->IsVideoOnly()) {

Remove the |mSeekJob.Exists()| check, so you have only one place to dispatch event.

https://reviewboard-hg.mozilla.org/gecko/file/64fa714092c9/dom/media/MediaDecoderStateMachine.cpp#l2438

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8856421 [details]
Bug 1355031 P1 - adjust the event order;

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

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8856422 [details]
Bug 1355031 P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair;

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

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8856422 [details]
Bug 1355031 P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair;

https://reviewboard.mozilla.org/r/128372/#review131184

> Remove the |mSeekJob.Exists()| check, so you have only one place to dispatch event.
> 
> https://reviewboard-hg.mozilla.org/gecko/file/64fa714092c9/dom/media/MediaDecoderStateMachine.cpp#l2438

I found that we cannot do it, at least for now, because that, in SeekingState::SeekComplete(), we call `mSeekJob.Resolve()` which reset the `mSeekJob.mTarget` (http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/dom/media/MediaDecoderStateMachine.cpp#2393-2398).

So, we need to dispatch the VideoOnlySeekCompleted event in both SeekingState::SeekComplete() and AccurateSeekingState::Exit(). Reasonable?

I think we should land this bug as it is and have another bug to refactor the way we store SeekTarget and the SeekPromise if we want.

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8856422 [details]
Bug 1355031 P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair;

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

Comment 10

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f01b3c5d7d3717bd2c2e7263e6940ba6a79f2a6c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d276ac04fb1e79426f9c028cbbaa1f29d34e8245

Comment 11

[Tzuhao Kuo [:kaku]]

Try looks good, let's check in, thanks for the review.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9c4cea9d957
P1 - adjust the event order; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2df7d0984161
P2 - make sure that VideoOnlySeekBegin and VideoOnlySeekCompleted are always dispatched in pair; r=jwwang

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/d9c4cea9d957
https://hg.mozilla.org/mozilla-central/rev/2df7d0984161