1284399 - Remove SeekTask::mSeekJob

Status: RESOLVED FIXED

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

Description

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

SeekJob is the connection between MDSM and MediaDecoder which should be stored in MDSM instead of SeekTask. In fact SeekTask should know nothing about MediaDecoder which is MDSM's concern.

Comment 1

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

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

Comment 2

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

Attachment: Bug 1284399. Part 1 - add GetSeekTarget() to remove direct access to mSeekJob.

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

Comment 3

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

Attachment: Bug 1284399. Part 2 - add MDSM::mCurrentSeek to replace SeekTask::mSeekJob.

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

Comment 4

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

Attachment: Bug 1284399. Part 3 - remove SeekTask::Exists().

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

Comment 5

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

Attachment: Bug 1284399. Part 4 - move |mSeekJob.RejectIfExists(__func__)| out of SeekTask::Discard().

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

Comment 6

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

Attachment: Bug 1284399. Part 5 - remove SeekTask::mSeekJob.

Drop{Audio,Video}UpToSeekTarget() is always called before Discard() so
mSeekJob.Exists() is guaranteed to be true.

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

Comment 7

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

Attachment: Bug 1284399. Part 6 - store a copy of SeekTarget instead of its reference in SeekTask so it's life cycle can be independent from the client.

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

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770355 [details]
Bug 1284399. Part 1 - add GetSeekTarget() to remove direct access to mSeekJob.

https://reviewboard.mozilla.org/r/63810/#review61456

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770356 [details]
Bug 1284399. Part 2 - add MDSM::mCurrentSeek to replace SeekTask::mSeekJob.

https://reviewboard.mozilla.org/r/63812/#review61458

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770357 [details]
Bug 1284399. Part 3 - remove SeekTask::Exists().

https://reviewboard.mozilla.org/r/63814/#review61452

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770358 [details]
Bug 1284399. Part 4 - move |mSeekJob.RejectIfExists(__func__)| out of SeekTask::Discard().

https://reviewboard.mozilla.org/r/63816/#review61460

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770359 [details]
Bug 1284399. Part 5 - remove SeekTask::mSeekJob.

https://reviewboard.mozilla.org/r/63818/#review61462

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8770360 [details]
Bug 1284399. Part 6 - store a copy of SeekTarget instead of its reference in SeekTask so it's life cycle can be independent from the client.

https://reviewboard.mozilla.org/r/63820/#review61464

::: dom/media/SeekTask.h:59
(Diff revision 1)
>  
>    virtual RefPtr<SeekTaskPromise> Seek(const media::TimeUnit& aDuration) = 0;
>  
>    virtual bool NeedToResetMDSM() const = 0;
>  
> -  SeekTarget& GetSeekTarget();
> +  const SeekTarget& GetSeekTarget();

I think we can also constify this method not only its result.

Comment 14

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

https://reviewboard.mozilla.org/r/63820/#review61464

> I think we can also constify this method not only its result.

The member can't be const because we might change it in AdjustFastSeekIfNeeded().

Comment 15

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

Thanks for the review!

Comment 16

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

(In reply to JW Wang [:jwwang] from comment #14)
> https://reviewboard.mozilla.org/r/63820/#review61464
> 
> > I think we can also constify this method not only its result.
> 
> The member can't be const because we might change it in
> AdjustFastSeekIfNeeded().

Sorry that I misread it. The GetSeekTarget() function can be a const member in fact.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb07471f5f37
Part 1 - add GetSeekTarget() to remove direct access to mSeekJob. r=kaku
https://hg.mozilla.org/integration/autoland/rev/2c43a873bb1c
Part 2 - add MDSM::mCurrentSeek to replace SeekTask::mSeekJob. r=kaku
https://hg.mozilla.org/integration/autoland/rev/762d46fe61f0
Part 3 - remove SeekTask::Exists(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/60b332af3201
Part 4 - move |mSeekJob.RejectIfExists(__func__)| out of SeekTask::Discard(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/4101bf6e8ff1
Part 5 - remove SeekTask::mSeekJob. r=kaku
https://hg.mozilla.org/integration/autoland/rev/3dbc9bb0a070
Part 6 - store a copy of SeekTarget instead of its reference in SeekTask so it's life cycle can be independent from the client. r=kaku

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb07471f5f37
https://hg.mozilla.org/mozilla-central/rev/2c43a873bb1c
https://hg.mozilla.org/mozilla-central/rev/762d46fe61f0
https://hg.mozilla.org/mozilla-central/rev/60b332af3201
https://hg.mozilla.org/mozilla-central/rev/4101bf6e8ff1
https://hg.mozilla.org/mozilla-central/rev/3dbc9bb0a070