1125970 - Remove or make safe MediaTaskQueue flushing capability

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

In bug 1125472 we got bitten by flushing the MDSM's decode task queue causing an event that set critical state to be dropped, causing a playback problem.

We could prevent such problems in future by either not allowing task queue's to be flushed, or by otherwise making the flushing "safe".

We flush the task queues in pretty much every MediaDataDecoder implementation, in order to shut down their task queues faster, so we'd either need to Not Do That, or find another way to achieve that. Possibly this is an unnecessary optimization.

Comment 1

[Bobby Holley (:bholley)]

Picking up chris' patches from bug 1125472. Let's see where they stand on try now that the dust has settled:

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

Comment 2

[Bobby Holley (:bholley)]

Ok, so we still have timeouts on dom/media/test/test_bug493187.html. Let's try with my programmatic logging setup:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f035eb7b904

Comment 3

[Bobby Holley (:bholley)]

Found the bug, or at least one of them. Pushing to try to confirm:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5487e41c4590

Comment 4

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> Found the bug, or at least one of them. Pushing to try to confirm:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5487e41c4590

Looking good so far. Did a full try push with an additional patch to make this stuff safe:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=931e8f5d4ae5

Comment 5

[Bobby Holley (:bholley)]

(My type-system based solution involves a lot of mechanical changes the code that I can't build locally, so I'm just iterating a bit on try)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=192a25d2f0cc

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Cancel tasks to resume decode in ResetDecode(). r=mattwoodrow

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1.5 - Reject video promise in RequestVideoWithSkipTask::Cancel. v1

This fixes the android/b2g hang in dom/media/test/test_bug493187.html. I will
fold it into Part 1 before landing.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't flush decode task queue in MediaDecoderStateMachine::FlushDecoding(). r=bholley

Comment 9

[Bobby Holley (:bholley)]

I'm going to hold off flagging for review on the last patch until comment 5 comes back green.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make flushing an opt-in mechanism, and use it only for the PDM task queues. v1

Comment 11

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

Comment on attachment 8564812 [details] [diff] [review]
Part 3 - Make flushing an opt-in mechanism, and use it only for the PDM task queues. v1

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

The name "FlushableMediaTaskQueue" is a bit of an eye sore, but I'll consider that as motivation to change our PDMs to not flush task queues.

Comment 12

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

Comment on attachment 8564806 [details] [diff] [review]
Part 1.5 - Reject video promise in RequestVideoWithSkipTask::Cancel. v1

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

::: dom/media/MediaDecoderReader.cpp
@@ +197,2 @@
>      mIsCanceled = true;
> +    mReader->mBaseVideoPromise.Reject(MediaDecoderReader::CANCELED, __func__);

Do you also need to reject mReader->mBaseAudioPromise? We can still return from MediaDecoderReader::RequestAudioData() without resolving mBaseAudioPromise without resolving the promise...

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 1 - Reject promises in MediaDecoderReader::ResetDecode and don't re-request audio and video when the promises have been rejected. v1

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7f3ee2c169d

All good modulo static analysis failure for implicit constructor. Verifying fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5feb52634342

Comment 16

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4873be7b96a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59eb02479c38
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/581ed2f292ff

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4873be7b96a1
https://hg.mozilla.org/mozilla-central/rev/59eb02479c38
https://hg.mozilla.org/mozilla-central/rev/581ed2f292ff

Comment 18

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

Bobby - What do you think about uplifting this bug to 37 given that this bug fixed bug 1130253, which is an MSE P1, and that we're planning to ship MSE in 37? If you think that this can be uplifted, this bug contains quite a sizable code change touching many files. Can the fix be verified on Nightly first?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> Bobby - What do you think about uplifting this bug to 37 given that this bug
> fixed bug 1130253, which is an MSE P1, and that we're planning to ship MSE
> in 37? If you think that this can be uplifted, this bug contains quite a
> sizable code change touching many files. Can the fix be verified on Nightly
> first?

We should absolutely uplift this to 37. It fixes lots of crashes and hangs by ceasing to do something that is very unsafe. The only verification that needs to be done is to ensure that we're not getting any new kinds of crashes and hangs (which we could, since this is a significant change in behavior).

The sizeable code change (part 3) doesn't change behavior - it just makes things more type-safe (i.e. prevents people from flushing the main task queue, which causes lots of crashes).  Part 2 is the big one, even though it's only 2 lines.

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 8564807 [details] [diff] [review]
Part 2 - Don't flush decode task queue in MediaDecoderStateMachine::FlushDecoding(). r=bholley

Requesting 37 uplift of all patches on this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Playback stalls an crashes playing video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects all video playback, so it's not protected by our ability to pref off MSE. On the other hand, this should be more robust than what we have for non-MSE playback too. Risk is acceptable, I think, and if not it's better to find out soon.
[String/UUID change made/needed]: None.

NB the third patch doesn't apply cleanly because of eme changes which haven't been uplifted. Per comment #19 it should be safe to just omit those parts of the patch. If we want to reduce risk, we can skip or delay uplifting the third patch, but I'd really like to have it for consistency in the MSE code.

Comment 21

[Ralph Giles (:rillian)]

Pushed to aurora with pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/0804aec59497
https://hg.mozilla.org/releases/mozilla-aurora/rev/74c5dedf63e9
https://hg.mozilla.org/releases/mozilla-aurora/rev/a48f841801e0

Comment 22

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

Comment on attachment 8564829 [details] [diff] [review]
Part 1 - Reject promises in MediaDecoderReader::ResetDecode and don't re-request audio and video when the promises have been rejected. v1

As noted, pre-approval was granted for a collection of MSE changes. Adding approval to the bug.