Closed Bug 1133167 Opened 5 years ago Closed 5 years ago

Add CancelSeek call to FlushDecoding()

Categories

(Core :: Audio/Video, defect)

x86_64
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1128357 +++

During working for Bug 1128357, I faced the crash like Bug 1128357 comment 18. It seems to be caused by lacking seek during FlushDecoding().
Blocks: 1128357
No longer blocks: MSE, 1050031, 1123535
No longer depends on: 1128357
Blocks: 1050031
Assignee: nobody → sotaro.ikeda.g
Priority: P1 → --
Attachment #8564472 - Flags: review?(cpearce)
Comment on attachment 8564472 [details] [diff] [review]
Patch - Add CancelSeek call to FlushDecoding()

Review of attachment 8564472 [details] [diff] [review]:
-----------------------------------------------------------------

I think this matches what we already do in MediaDecoderStateMachine::DecodeSeek(), but I want bholley to also check that this is OK.

I think until we stop using MediaTaskQueue::FlushAndDispatch() in MediaDecoderStateMachine::FlushDecoding(), this is about the best we can do.
Attachment #8564472 - Flags: review?(cpearce)
Attachment #8564472 - Flags: review?(bobbyholley)
Attachment #8564472 - Flags: review+
(In reply to Chris Pearce (:cpearce) from comment #3)
> I think until we stop using MediaTaskQueue::FlushAndDispatch() in
> MediaDecoderStateMachine::FlushDecoding(), this is about the best we can do.

I'm about to land that change in bug 1125970. Is there still a reason to do this once we're no longer flushing the task queue?

It seems like there still might be, given that at least in the MSE case, seeking can take arbitrarily long.
Flags: needinfo?(cpearce)
7(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #4)
> (In reply to Chris Pearce (:cpearce) from comment #3)
> > I think until we stop using MediaTaskQueue::FlushAndDispatch() in
> > MediaDecoderStateMachine::FlushDecoding(), this is about the best we can do.
> 
> I'm about to land that change in bug 1125970. Is there still a reason to do
> this once we're no longer flushing the task queue?

We could still be hit by this if we don't do something here after the landing of bug 1125970, right?

The alternative to this patch would be to dispatch a task to call Reader::CancelSeek() before we dispatch the task to call ResetDecode(), but the state may have changed by the time that task runs.
 
> It seems like there still might be, given that at least in the MSE case,
> seeking can take arbitrarily long.

Probably the safest thing to do for now is run with this patch until we get a chance to clean the state machine up.
Flags: needinfo?(cpearce)
Comment on attachment 8564472 [details] [diff] [review]
Patch - Add CancelSeek call to FlushDecoding()

Review of attachment 8564472 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2954,5 @@
> +    mReader->CancelSeek();
> +    mCancelingSeek = true;
> +  }
> +
> +  mReader->ResetDecode();

Should we scope the monitor outside this call? That would avoid changing the behavior (whereby we're now invoking ResetDecode with the monitor where we weren't before).

I'm fine either way - up to you, really.
Attachment #8564472 - Flags: review?(bobbyholley) → review+
Apply the comment. Carry "r=cpearce,bholley".
Attachment #8564472 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c6e50e83f7cb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1135304
No longer blocks: 1135304
Comment on attachment 8565451 [details] [diff] [review]
Patch - Add CancelSeek call to FlushDecoding()

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes playing youtube video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects all media playback and is straightforward and easy to revert. Risk is moderately low.
[String/UUID change made/needed]: None.
Attachment #8565451 - Flags: approval-mozilla-aurora?
Comment on attachment 8565451 [details] [diff] [review]
Patch - Add CancelSeek call to FlushDecoding()

As stated, this bug was pre-approved to land with a set of changes for MSE. Marking the approval after the fact.
Attachment #8565451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1166937
You need to log in before you can comment on or make changes to this bug.