Closed Bug 1284399 Opened 6 years ago Closed 5 years ago

Remove SeekTask::mSeekJob

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

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.
Assignee: nobody → jwwang
Blocks: 1283751
Review commit: https://reviewboard.mozilla.org/r/63810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63810/
Attachment #8770355 - Flags: review?(kaku)
Attachment #8770356 - Flags: review?(kaku)
Attachment #8770357 - Flags: review?(kaku)
Attachment #8770358 - Flags: review?(kaku)
Attachment #8770359 - Flags: review?(kaku)
Attachment #8770360 - Flags: review?(kaku)
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 on attachment 8770355 [details]
Bug 1284399. Part 1 - add GetSeekTarget() to remove direct access to mSeekJob.

https://reviewboard.mozilla.org/r/63810/#review61456
Attachment #8770355 - Flags: review?(kaku) → review+
Comment on attachment 8770356 [details]
Bug 1284399. Part 2 - add MDSM::mCurrentSeek to replace SeekTask::mSeekJob.

https://reviewboard.mozilla.org/r/63812/#review61458
Attachment #8770356 - Flags: review?(kaku) → review+
Comment on attachment 8770357 [details]
Bug 1284399. Part 3 - remove SeekTask::Exists().

https://reviewboard.mozilla.org/r/63814/#review61452
Attachment #8770357 - Flags: review?(kaku) → review+
Comment on attachment 8770358 [details]
Bug 1284399. Part 4 - move |mSeekJob.RejectIfExists(__func__)| out of SeekTask::Discard().

https://reviewboard.mozilla.org/r/63816/#review61460
Attachment #8770358 - Flags: review?(kaku) → review+
Comment on attachment 8770359 [details]
Bug 1284399. Part 5 - remove SeekTask::mSeekJob.

https://reviewboard.mozilla.org/r/63818/#review61462
Attachment #8770359 - Flags: review?(kaku) → review+
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.
Attachment #8770360 - Flags: review?(kaku) → review+
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().
Thanks for the review!
(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.
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.