Remove MediaDecoderStateMachine::mPendingSeek

RESOLVED FIXED in Firefox 48

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This somewhat simplifies our seeking pipeline for MDSM. It is often confusing to me to figure out each role of mCurrentSeek, mPendingSeek and mQueuedSeek played in the seeking process.

By calling InitiateSeek() immediately after changing mState to DECODER_STATE_SEEKING without waiting for the next state machine cycle, we can eliminate the need for mPendingSeek which is used to store the seek target and transferred to mCurrentSeek in the next state machine cycle.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1253184
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60356c9f11c9
(Assignee)

Comment 2

2 years ago
Created attachment 8728206 [details]
MozReview Request: Bug 1252767 - Remove MediaDecoderStateMachine::mPendingSeek. r=cpearce.

Review commit: https://reviewboard.mozilla.org/r/38865/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38865/
Attachment #8728206 - Flags: review?(cpearce)
Comment on attachment 8728206 [details]
MozReview Request: Bug 1252767 - Remove MediaDecoderStateMachine::mPendingSeek. r=cpearce.

https://reviewboard.mozilla.org/r/38865/#review35687

::: dom/media/MediaDecoderStateMachine.h:531
(Diff revision 1)
> +    void Steal(SeekJob& aOther)

I know you've just moved this code, but maybe we can use a move constructor/operation here instead of a Steal() function?

::: dom/media/MediaDecoderStateMachine.cpp:957
(Diff revision 1)
>            // Non-precise seek; or a pending seek exists ; we can stop the seek

Does it still make sense for the comment to say "or a pending seek exists"?

::: dom/media/MediaDecoderStateMachine.cpp:2094
(Diff revision 1)
>    // Change state to DECODING or COMPLETED now. SeekingStopped will

This comment refers to SeekingStopped, which appears to no longer exist.
Attachment #8728206 - Flags: review?(cpearce) → review+
(Assignee)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/38865/#review35687

> I know you've just moved this code, but maybe we can use a move constructor/operation here instead of a Steal() function?

Sure. The move semantics is more universal and easier to understand. I will address that in next patch.

> Does it still make sense for the comment to say "or a pending seek exists"?

Will remove the comment in next patch.

> This comment refers to SeekingStopped, which appears to no longer exist.

Will remove the comment in next patch.
(Assignee)

Comment 5

2 years ago
In fact, I will address the move constructor in next bug which is quite orthogonal to this bug.
(Assignee)

Updated

2 years ago
Blocks: 1255268

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e950da72985

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e950da72985
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.