1104964 - Make MediaDecoderReader own the task queue

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

In bug 1097823, I ran into tricky teardown races getting rid of AudioDecoderRendezvous, and punted on the issue temporarily by just running webaudio buffer decodes directly on the decoder thread pool. I plan to fix this by giving MediaBufferDecoder a proper MediaTaskQueue.

The big wrinkle is that MediaTaskQueue shutdown is blocks, which is not something we want to do on the main thread (where MediaBufferDecoder gets torn down). So it needs an asynchronous shutdown mechanism, and we need to coordinate that with promises such that it can automatically reject the promise if the queue is being torn down. This may involve requiring some sort of unified base enum type for all MediaPromise rejection values.

This could get somewhat involved, so I'm leaving it as a followup.

Comment 1

[Bobby Holley (:bholley)]

I ended up needing to sort all this stuff out for bug 1097823 to fix shutdown leaks, so I'm morphing this bug and inverting the dependency.

The basic solution I came up with was to have MediaDecoderReader own the task queue (rather than MediaDecoderStateMachine) so that it can carefully reject its outstanding promises before shutting down the task queue.

Comment 2

[Bobby Holley (:bholley)]

(This change ends up being cleaner for various other reasons as well).

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e632ae081a9b

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Handle ShutdownPoolsEvent race. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make sure that MediaDecoderReader::Shutdown is always called. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Split shutdown initiatation and queue-drain-waiting into separate pieces. v1

This patch shouldn't change any behavior. The upcoming patch takes advantage of
these separate pieces.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Rename the task queue for intel video decoding to something more obvious. v1

Otherwise it will shadow the mTaskQueue that we're about to introduce on
MediaDecoderReader. It's not clear to me why we need a separate decode task
queue here, but that's kind of orthogonal.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make MediaDecoderReader own the task queue. v1

Comment 9

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

Comment on attachment 8530665 [details] [diff] [review]
Part 4 - Rename the task queue for intel video decoding to something more obvious. v1

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

::: dom/media/webm/WebMReader.h
@@ +186,5 @@
>    nsIntSize GetInitialFrame();
>    uint64_t GetLastVideoFrameTime();
>    void SetLastVideoFrameTime(uint64_t aFrameTime);
>    layers::LayersBackend GetLayersBackendType() { return mLayersBackendType; }
> +  MediaTaskQueue* GetIntelVP8TaskQueue() { return mIntelVP8TaskQueue; }

Please call this VideoTaskQueue, or somesuch, since we are also going to use this path for HW decoding VP8 on B2G, eventually... And we will may get VP9 decoding on desktop too.

Comment 10

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

Comment on attachment 8530666 [details] [diff] [review]
Part 5 - Make MediaDecoderReader own the task queue. v1

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

::: dom/media/MediaDecoderReader.cpp
@@ +253,2 @@
>  {
> +  if (!mTaskQueue) {

Assert here that the task queue is not borrowed? You don't want to create a task queue when one's borrowed, right?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #10)
> Assert here that the task queue is not borrowed? You don't want to create a
> task queue when one's borrowed, right?

Yes, but in that case mTaskQueue is non-null by definition. Happy to assert against it anyway though.

Comment 12

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/11a0c78d319d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd847aca8a71
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c1c3b79b82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cebbf49492d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/786ff9a62a39

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11a0c78d319d
https://hg.mozilla.org/mozilla-central/rev/dd847aca8a71
https://hg.mozilla.org/mozilla-central/rev/f8c1c3b79b82
https://hg.mozilla.org/mozilla-central/rev/cebbf49492d3
https://hg.mozilla.org/mozilla-central/rev/786ff9a62a39

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8530662 [details] [diff] [review]
Part 1 - Handle ShutdownPoolsEvent race. v1

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

We met this problem in bug 1002804 comment 33 where we tried to write gtests for MediaTaskQueue and SharedThreadPool. I proposed to ref-count sMonitor and sPools to make it easier to manage their life cycles. I wonder if bug 1002804 will go on.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8530663 [details] [diff] [review]
Part 2 - Make sure that MediaDecoderReader::Shutdown is always called. v1

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

I don't know how this is relatd to bug 1104421 comment 0 where it has try push problems in forcing Shutdown() is called. Maybe kinetik knows the detail.

::: dom/media/MediaDecoderReader.cpp
@@ +71,5 @@
>  }
>  
>  MediaDecoderReader::~MediaDecoderReader()
>  {
> +  MOZ_ASSERT(mShutdown);

We have a memory consistency problem is the destructor is not called on the decoding thread where mShutdown is modified.

@@ +266,5 @@
>  void
>  MediaDecoderReader::Shutdown()
>  {
>    MOZ_ASSERT(mDecoder->OnDecodeThread());
> +  mShutdown = true;

Should we check Shutdown() is called only exactly once?

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Hi kinetik,
Can you comment comment 15? Thanks.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8530666 [details] [diff] [review]
Part 5 - Make MediaDecoderReader own the task queue. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3196,5 @@
>  }
>  
>  bool MediaDecoderStateMachine::OnDecodeThread() const
>  {
> +  return !DecodeTaskQueue() || DecodeTaskQueue()->IsCurrentThreadIn();

Can we gurantee DecodetaskQueue() will always be non-null for mReader->EnsureTaskQueue() is called in Init()?

Comment 18

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #16)
> Hi kinetik,
> Can you comment comment 15? Thanks.

I didn't spend any time on it beyond an initial test as I was trying to fix a different bug at the time.  It looks like this patch does what bug 1104421 wanted, so I'll close it.

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8530662 [details] [diff] [review]
Part 1 - Handle ShutdownPoolsEvent race. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Moderate; these patches touch code used by video playback in general.
[String/UUID change made/needed]: None

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 8530663 [details] [diff] [review]
Part 2 - Make sure that MediaDecoderReader::Shutdown is always called. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Moderate; these patches touch code used by video playback in general.
[String/UUID change made/needed]: None

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8530664 [details] [diff] [review]
Part 3 - Split shutdown initiatation and queue-drain-waiting into separate pieces. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Moderate; these patches touch code used by video playback in general.
[String/UUID change made/needed]: None

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8530665 [details] [diff] [review]
Part 4 - Rename the task queue for intel video decoding to something more obvious. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Moderate; these patches touch code used by video playback in general.
[String/UUID change made/needed]: None

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8530666 [details] [diff] [review]
Part 5 - Make MediaDecoderReader own the task queue. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent playback support, video sites more
likely to use flash.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Moderate; these patches touch code used by video playback in general.
[String/UUID change made/needed]: None

Comment 24

[Bobby Holley (:bholley)]

Raph, (In reply to Ralph Giles (:rillian) from comment #19)
> Comment on attachment 8530662 [details] [diff] [review]
> Part 1 - Handle ShutdownPoolsEvent race. v1
> 
> Approval Request Comment
> [Feature/regressing bug #]: MSE
> [User impact if declined]: Less consistent playback support, video sites more
> likely to use flash.
> [Describe test coverage new/current, TBPL]: Landed on m-c.
> [Risks and why]: Moderate; these patches touch code used by video playback
> in general.
> [String/UUID change made/needed]: None

Thanks for doing this Ralph. I think this process is a bit suboptimal though - IMO we should clear the plan with the release drivers, and then just have someone uplift all media patches that landed on Nightly directly to aurora without approval (probably waiting 3 days or so for any stability issues to shake out). Does that sound reasonable?

Comment 25

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c755ca7a6484
https://hg.mozilla.org/releases/mozilla-aurora/rev/78b691bfdee3
https://hg.mozilla.org/releases/mozilla-aurora/rev/a52f49bac58b
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f823d350912
https://hg.mozilla.org/releases/mozilla-aurora/rev/874ce9bce91a

Comment 26

[Ralph Giles (:rillian)]

(In reply to Bobby Holley (:bholley) from comment #24)

> - IMO we should clear the plan with the release drivers, and then just have
> someone uplift all media patches that landed on Nightly directly to aurora
> without approval (probably waiting 3 days or so for any stability issues to
> shake out). Does that sound reasonable?

It sounds reasonable to me, but when I proposed this, Sylvestre asked for bug-by-bug approval requests. I've been doing patch-by-patch instead, since that's the only way I know to set the flags.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Ralph Giles (:rillian) from comment #26)
> It sounds reasonable to me, but when I proposed this, Sylvestre asked for
> bug-by-bug approval requests. I've been doing patch-by-patch instead, since
> that's the only way I know to set the flags.

Known papercut in bugzilla - just flag one of the patches in the series and say "this request applies to all the patches in this bug". Also, make sure to uplift from the actual changes landed on Nightly, rather than the patches in the bugs.

Comment 28

[Ralph Giles (:rillian)]

(In reply to Bobby Holley (:bholley) from comment #27)

> Known papercut in bugzilla - just flag one of the patches in the series and
> say "this request applies to all the patches in this bug".

Ok, thanks. That will save a lot of time.

> Also, make sure
> to uplift from the actual changes landed on Nightly, rather than the patches
> in the bugs.

I'd guessed the in-bug patches weren't reliable. Ryan, do your check-in scripts do this or do you rely on the bug attachments being up-to-date?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I normally export the changeset(s) that landed on m-c unless there's an explicitly-marked branch patch attached.