1378085 - Decouple video-only seek into a subclass of AccurateSeekingState.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

Spawn from bug 1375922 comment 24.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P1 - create VideoOnlySeekingState;

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

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P2 - override Enter();

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

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P3 - override Exit();

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

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 p4 - override HandleAudioDecoded();

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

Comment 5

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();

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

Comment 6

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 p6 - override HandleAudioWaited();

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

Comment 7

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 p7 - override DoSeek();

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

Comment 8

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P8 - don't update playback position in video-only seeking;

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

Comment 9

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();

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

Comment 10

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51afcc026035f2662d27730472bf9a991af6fe64

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883248 [details]
Bug 1378085 P1 - create VideoOnlySeekingState;

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

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

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

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883250 [details]
Bug 1378085 P3 - override Exit();

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

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883251 [details]
Bug 1378085 p4 - override HandleAudioDecoded();

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

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883252 [details]
Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();

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

Comment 16

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883253 [details]
Bug 1378085 p6 - override HandleAudioWaited();

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

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883254 [details]
Bug 1378085 p7 - override DoSeek();

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

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883255 [details]
Bug 1378085 P8 - don't update playback position in video-only seeking;

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

Comment 19

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883256 [details]
Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();

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

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

https://reviewboard.mozilla.org/r/154178/#review159426

::: dom/media/MediaDecoderStateMachine.cpp:974
(Diff revision 2)
> -    // Dispatch a mozvideoonlyseekbegin event to indicate UI for corresponding
> +    NotifyVideoOnlySeekBegin();
> -    // changes.
> -    if (mSeekJob.mTarget->IsVideoOnly()) {
> -      mMaster->mOnPlaybackEvent.Notify(MediaEventType::VideoOnlySeekBegin);
> -    }

Having a port here for a specific subclass is not ideal, but we need to maintain the event order of ExitVideoSuspend and VideoOnlySeekBegin (as we introduced in bug 1355031). Any suggestion?

Comment 21

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be0c90fdf9b62de45a98b7cf7f6edcc8c7a32596

Comment 22

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

Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

https://reviewboard.mozilla.org/r/154178/#review159434

::: dom/media/MediaDecoderStateMachine.cpp:974
(Diff revision 2)
> -    // Dispatch a mozvideoonlyseekbegin event to indicate UI for corresponding
> +    NotifyVideoOnlySeekBegin();
> -    // changes.
> -    if (mSeekJob.mTarget->IsVideoOnly()) {
> -      mMaster->mOnPlaybackEvent.Notify(MediaEventType::VideoOnlySeekBegin);
> -    }

It is confusing when you have many pieces of code that could be overridden by the sub-classes in a function.

::: dom/media/MediaDecoderStateMachine.cpp:1780
(Diff revision 2)
> +                                          EventVisibility aVisibility)
> +  {
> +    MOZ_ASSERT(aSeekJob.mTarget->IsVideoOnly());
> +    MOZ_ASSERT(aVisibility == EventVisibility::Suppressed);
> +
> +    return AccurateSeekingState::Enter(Move(aSeekJob), aVisibility);

You can change the order of the statements and do:

// Stop playback.
RefPtr<MediaDecoder::SeekPromise> p = AccurateSeekingState::Enter(...);
// Notify the event.
return p.forget();

Comment 23

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

Comment on attachment 8883248 [details]
Bug 1378085 P1 - create VideoOnlySeekingState;

https://reviewboard.mozilla.org/r/154176/#review159436

Comment 24

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

Comment on attachment 8883250 [details]
Bug 1378085 P3 - override Exit();

https://reviewboard.mozilla.org/r/154180/#review159450

Comment 25

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

Comment on attachment 8883251 [details]
Bug 1378085 p4 - override HandleAudioDecoded();

https://reviewboard.mozilla.org/r/154182/#review159452

Comment 26

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

Comment on attachment 8883252 [details]
Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();

https://reviewboard.mozilla.org/r/154184/#review159454

Comment 27

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

Comment on attachment 8883253 [details]
Bug 1378085 p6 - override HandleAudioWaited();

https://reviewboard.mozilla.org/r/154186/#review159456

Comment 28

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

Comment on attachment 8883254 [details]
Bug 1378085 p7 - override DoSeek();

https://reviewboard.mozilla.org/r/154188/#review159458

Comment 29

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

Comment on attachment 8883255 [details]
Bug 1378085 P8 - don't update playback position in video-only seeking;

https://reviewboard.mozilla.org/r/154190/#review159460

The same problem as P2.

Comment 30

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

Comment on attachment 8883256 [details]
Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();

https://reviewboard.mozilla.org/r/154192/#review159462

Comment 31

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1378085 P0 - update playback position only if event visibility is observable;

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

Comment 32

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883248 [details]
Bug 1378085 P1 - create VideoOnlySeekingState;

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

Comment 33

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

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

Comment 34

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883250 [details]
Bug 1378085 P3 - override Exit();

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

Comment 35

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883251 [details]
Bug 1378085 p4 - override HandleAudioDecoded();

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

Comment 36

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883252 [details]
Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();

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

Comment 37

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883253 [details]
Bug 1378085 p6 - override HandleAudioWaited();

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

Comment 38

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883254 [details]
Bug 1378085 p7 - override DoSeek();

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

Comment 39

[Tzuhao Kuo [:kaku]]

Comment on attachment 8883256 [details]
Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();

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

Comment 40

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74f6012dc8616691ca9a6ca05a80b8d8f568f382
try for P0 only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af98dcfc8816f335bae96c10b199c2219450b093

Comment 41

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

Comment on attachment 8883818 [details]
Bug 1378085 P0 - update playback position only if event visibility is observable;

https://reviewboard.mozilla.org/r/154772/#review159804

Comment 42

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

Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

https://reviewboard.mozilla.org/r/154178/#review159806

Comment 43

[Tzuhao Kuo [:kaku]]

Try looks pretty good, thanks for the review!

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad71152dc8a5
P0 - update playback position only if event visibility is observable; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/8ea49c9fe3b8
P1 - create VideoOnlySeekingState; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e3e65c829660
P2 - override Enter(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/1395148684c3
P3 - override Exit(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/17c791afc45b
p4 - override HandleAudioDecoded(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/cdee15b41196
p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fc04749e3534
p6 - override HandleAudioWaited(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ac3d88e3f67e
p7 - override DoSeek(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d890f9fc0b5c
P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit(); r=jwwang

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/ad71152dc8a5
https://hg.mozilla.org/mozilla-central/rev/8ea49c9fe3b8
https://hg.mozilla.org/mozilla-central/rev/e3e65c829660
https://hg.mozilla.org/mozilla-central/rev/1395148684c3
https://hg.mozilla.org/mozilla-central/rev/17c791afc45b
https://hg.mozilla.org/mozilla-central/rev/cdee15b41196
https://hg.mozilla.org/mozilla-central/rev/fc04749e3534
https://hg.mozilla.org/mozilla-central/rev/ac3d88e3f67e
https://hg.mozilla.org/mozilla-central/rev/d890f9fc0b5c