If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove SeekJob::mTransition

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 months ago
Spawn from bug 1366362 comment 32.
(Assignee)

Updated

4 months ago
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

4 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed26a395e08998d01d43a712a329b061167c067

Comment 6

4 months ago
mozreview-review
Comment on attachment 8871215 [details]
Bug 1367701 P1 - add a virtual Transit() method;

https://reviewboard.mozilla.org/r/142704/#review146366

::: dom/media/MediaDecoderStateMachine.cpp:1028
(Diff revision 1)
>  
>  protected:
>    SeekJob mSeekJob;
>  
>    virtual void DoSeek() = 0;
> +  virtual void Transit() { SetState<DecodingState>(); }

NextState sounds more appropriate to me

Comment 7

4 months ago
mozreview-review
Comment on attachment 8871216 [details]
Bug 1367701 P1 - add a virtual Transit() method and remove SeekJob:mTransition;

https://reviewboard.mozilla.org/r/142706/#review146370

P2 and P1 should be merged together. As with just P1 NextFrameSeek is broken once again.

::: dom/media/MediaDecoderStateMachine.cpp:1763
(Diff revision 1)
>  private:
>    MozPromiseRequestHolder<MediaDecoder::SeekPromise> mAccurateSeekRequest;
>    SeekJob mFutureSeekJob;
>  
> -  void Transit() override { // No transition after seek completed. }
> +  // We don't want to transition to DecodingState once this seek completes.
> +  void Transit() override { }

this member shouldn't be made private. it's a bad habit I've seen ised in this code

Comment 8

4 months ago
mozreview-review
Comment on attachment 8871215 [details]
Bug 1367701 P1 - add a virtual Transit() method;

https://reviewboard.mozilla.org/r/142704/#review146372

::: dom/media/MediaDecoderStateMachine.cpp:1028
(Diff revision 1)
>  
>  protected:
>    SeekJob mSeekJob;
>  
>    virtual void DoSeek() = 0;
> +  virtual void Transit() { SetState<DecodingState>(); }

I will say:

// Transition to the next state (defined by the subclass) when seek is completed.
virtual void GoToNextState();
Attachment #8871215 - Flags: review?(jwwang) → review+

Comment 9

4 months ago
mozreview-review
Comment on attachment 8871217 [details]
Bug 1367701 P2 - transit to NextFrameSeekingState in Transit();

https://reviewboard.mozilla.org/r/142708/#review146374

::: dom/media/MediaDecoderStateMachine.cpp:1739
(Diff revision 1)
>    {
>      mFutureSeekJob = Move(aFutureSeekJob);
>  
>      AccurateSeekingState::Enter(Move(aCurrentSeekJob),
>                                  EventVisibility::Suppressed)
>        ->Then(OwnerThread(),

i think the entire MozPromiseRequestHolder can be removed, we no longer need thr Then.

either the promise is resolved and the it will go to the next state, or Exit will be called early which will reject the promise.

we no longer need to care about interrupting the workflow

Comment 10

4 months ago
mozreview-review
Comment on attachment 8871216 [details]
Bug 1367701 P1 - add a virtual Transit() method and remove SeekJob:mTransition;

https://reviewboard.mozilla.org/r/142706/#review146376

::: dom/media/MediaDecoderStateMachine.cpp:1763
(Diff revision 1)
>  private:
>    MozPromiseRequestHolder<MediaDecoder::SeekPromise> mAccurateSeekRequest;
>    SeekJob mFutureSeekJob;
>  
> -  void Transit() override { // No transition after seek completed. }
> +  // We don't want to transition to DecodingState once this seek completes.
> +  void Transit() override { }

It can be when it is not part of the public interface which is commonly used in Template Method pattern.

We can discuss that somewhere else. Let's just keep the visibility consistent in this change.
Attachment #8871216 - Flags: review?(jwwang) → review+

Comment 11

4 months ago
mozreview-review
Comment on attachment 8871217 [details]
Bug 1367701 P2 - transit to NextFrameSeekingState in Transit();

https://reviewboard.mozilla.org/r/142708/#review146380

::: dom/media/MediaDecoderStateMachine.cpp:1739
(Diff revision 1)
>    {
>      mFutureSeekJob = Move(aFutureSeekJob);
>  
>      AccurateSeekingState::Enter(Move(aCurrentSeekJob),
>                                  EventVisibility::Suppressed)
>        ->Then(OwnerThread(),

SGTM.
Attachment #8871217 - Flags: review?(jwwang) → review+

Comment 12

4 months ago
mozreview-review
Comment on attachment 8871218 [details]
Bug 1367701 P3 - modify comments;

https://reviewboard.mozilla.org/r/142710/#review146382
Attachment #8871218 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8871215 - Attachment is obsolete: true
(Assignee)

Comment 16

4 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bc1af4a17431bb9e8f43c602c56989632a6f258
(Assignee)

Comment 17

4 months ago
Try looks good, thanks for the reveiw!

Comment 18

4 months ago
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b504b3373572
P1 - add a virtual Transit() method and remove SeekJob:mTransition; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c057839d5db2
P2 - transit to NextFrameSeekingState in Transit(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5850e5d245fb
P3 - modify comments; r=jwwang

Comment 19

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b504b3373572
https://hg.mozilla.org/mozilla-central/rev/c057839d5db2
https://hg.mozilla.org/mozilla-central/rev/5850e5d245fb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.