Closed Bug 1378085 Opened 7 years ago Closed 7 years ago

Decouple video-only seek into a subclass of AccurateSeekingState.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(9 files, 1 obsolete file)

Bug 1378085 P1 - create VideoOnlySeekingState;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 P2 - override Enter();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 P3 - override Exit();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 p4 - override HandleAudioDecoded();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 p6 - override HandleAudioWaited();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 p7 - override DoSeek();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1378085 P0 - update playback position only if event visibility is observable;
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
Spawn from bug 1375922 comment 24.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Depends on: 1375922
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 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();
Attachment #8883249 - Flags: review?(jwwang) → review-
Comment on attachment 8883248 [details]
Bug 1378085 P1 - create VideoOnlySeekingState;

https://reviewboard.mozilla.org/r/154176/#review159436
Attachment #8883248 - Flags: review?(jwwang) → review+
Comment on attachment 8883250 [details]
Bug 1378085 P3 - override Exit();

https://reviewboard.mozilla.org/r/154180/#review159450
Attachment #8883250 - Flags: review?(jwwang) → review+
Comment on attachment 8883251 [details]
Bug 1378085 p4 - override HandleAudioDecoded();

https://reviewboard.mozilla.org/r/154182/#review159452
Attachment #8883251 - Flags: review?(jwwang) → review+
Comment on attachment 8883252 [details]
Bug 1378085 p5 - override HandleWaitingForAudio(), HandleAudioCanceled(), and HandleEndOfAudio();

https://reviewboard.mozilla.org/r/154184/#review159454
Attachment #8883252 - Flags: review?(jwwang) → review+
Comment on attachment 8883253 [details]
Bug 1378085 p6 - override HandleAudioWaited();

https://reviewboard.mozilla.org/r/154186/#review159456
Attachment #8883253 - Flags: review?(jwwang) → review+
Comment on attachment 8883254 [details]
Bug 1378085 p7 - override DoSeek();

https://reviewboard.mozilla.org/r/154188/#review159458
Attachment #8883254 - Flags: review?(jwwang) → review+
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.
Attachment #8883255 - Flags: review?(jwwang) → review-
Comment on attachment 8883256 [details]
Bug 1378085 P8 - always dispatch VideoOnlySeekCompleted at VideoOnlySeekingState::Exit();

https://reviewboard.mozilla.org/r/154192/#review159462
Attachment #8883256 - Flags: review?(jwwang) → review+
Attachment #8883255 - Attachment is obsolete: true
Comment on attachment 8883818 [details]
Bug 1378085 P0 - update playback position only if event visibility is observable;

https://reviewboard.mozilla.org/r/154772/#review159804
Attachment #8883818 - Flags: review?(jwwang) → review+
Comment on attachment 8883249 [details]
Bug 1378085 P2 - override Enter();

https://reviewboard.mozilla.org/r/154178/#review159806
Attachment #8883249 - Flags: review?(jwwang) → review+
Try looks pretty good, thanks for the review!
Pushed by tkuo@mozilla.com:
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
Blocks: 1378691
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: