Closed Bug 1104964 Opened 9 years ago Closed 9 years ago

Make MediaDecoderReader own the task queue

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

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.
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.
Blocks: 1097823
No longer depends on: 1097823
Summary: Improve relationship between MediaTaskQueue teardown and Promise resolution/rejection, and use MediaTaskQueue for webaudio's MediaBufferDecoder → Make MediaDecoderReader own the task queue
(This change ends up being cleaner for various other reasons as well).
This patch shouldn't change any behavior. The upcoming patch takes advantage of
these separate pieces.
Attachment #8530664 - Flags: review?(cpearce)
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.
Attachment #8530665 - Flags: review?(cpearce)
Attachment #8530662 - Flags: review?(cpearce) → review+
Attachment #8530663 - Flags: review?(cpearce) → review+
Attachment #8530664 - Flags: review?(cpearce) → review+
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.
Attachment #8530665 - Flags: review?(cpearce) → review+
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?
Attachment #8530666 - Flags: review?(cpearce) → review+
(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 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 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?
Hi kinetik,
Can you comment comment 15? Thanks.
Flags: needinfo?(kinetik)
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()?
(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.
Flags: needinfo?(kinetik)
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
Attachment #8530662 - Flags: approval-mozilla-aurora?
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
Attachment #8530663 - Flags: approval-mozilla-aurora?
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
Attachment #8530664 - Flags: approval-mozilla-aurora?
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
Attachment #8530665 - Flags: approval-mozilla-aurora?
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
Attachment #8530666 - Flags: approval-mozilla-aurora?
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?
Attachment #8530666 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8530665 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8530664 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8530663 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8530662 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
(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.
(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?
Flags: needinfo?(ryanvm)
I normally export the changeset(s) that landed on m-c unless there's an explicitly-marked branch patch attached.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.