1133167 - Add CancelSeek call to FlushDecoding()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ 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().

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: Patch - Add CancelSeek call to FlushDecoding()

Comment 2

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6336e8c07776

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: Patch - Add CancelSeek call to FlushDecoding()

Apply the comment. Carry "r=cpearce,bholley".

Comment 8

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e50e83f7cb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6e50e83f7cb

Comment 10

[Ralph Giles (:rillian)]

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.

Comment 11

[Ralph Giles (:rillian)]

Pushed to aurora with pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fb98996305c

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

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.