962385 - Refactor MP4Reader to run its decoders async

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

MP4Reader can utilize multiple cores on CPUs better if it runs async. If we use thread pools (where necessary) we can also improve scalability and reduce resource consumption.

I'll start by making MP4Reader still expose the synchronous API to MediaDecoderStateMachine, but run asynchronously "under the hood". Once I have MP4Reader working this way, I will change MediaDecoderStateMachine to run asynchronously (in another bug), and then MP4Reader can use that infrastructure.

Comment 1

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

Attachment: Patch 1: MediaTaskQueue

Add a MediaTaskQueue abstraction, that enables Runnables to be dispatched to a thread pool and run in synchronized (i.e. in order, non-concurrent) fashion. This means we can run multiple MediaTaskQueues on the same SharedThreadPool, and it's opaque to the thing using the MediaTaskQueue that it's events are running on (possibly different) threads that are being shared by other things.

I use a monitor to enforce non concurrency, and to ensure we have a memory barrier to make this safe.

Comment 2

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

Attachment: Patch 2: Make MediaDataDecoder asynchronous

Make the MediaDataDecoder interface asynchronous.

Actually it's not entirely asynchronous. We still only send input while we're in MP4Reader::Decode*Data() waiting for output. In future we'll send input when the MediaQueue's aren't "full". This is a step towards that.

Comment 3

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

Attachment: Patch 3: Make WMF PlatformDecoderModule async

Makes WMF's MediaDataDecoders conform to the async interface.

Comment 4

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

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

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8367062 [details] [diff] [review]
Patch 1: MediaTaskQueue

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

Looks good, just a few questions:

::: content/media/MediaTaskQueue.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */

Minor, but the mode lines differ from the coding style versions:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */

@@ +37,5 @@
> +  mTasks.push(aRunnable);
> +  if (mIsRunning) {
> +    return NS_OK;
> +  }
> +  mIsRunning = true;

Set mIsRunning to true after Dispatch succeeds?  Then move the assert until after mQueueMonitor is held?

@@ +53,5 @@
> +  AwaitIdle(mon);
> +}
> +
> +void
> +MediaTaskQueue::AwaitIdle(MonitorAutoLock& aMonitor)

I prefer the AwaitIdle/AwaitIdleUnlocked pattern.  You can't pass any monitor into AwaitIdle as it's making assumptions about what data is protected by the monitor, so it seems better to just make this unlocked and assert that mQueueMonitor is already held before waiting on it.

@@ +105,5 @@
> +  MOZ_ASSERT(event);
> +  {
> +    // Take a lock. This ensures we have a memory fence enforced, so that
> +    // if the object we're calling wasn't designed to be threadsafe, it will
> +    // be.

Doesn't the fact that mQueueMonitor has been taken and dropped provide the same guarantee?

Comment 6

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

(In reply to Matthew Gregan [:kinetik] from comment #5)
> Comment on attachment 8367062 [details] [diff] [review]
> Patch 1: MediaTaskQueue
> 
> Review of attachment 8367062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just a few questions:
> 
> ::: content/media/MediaTaskQueue.cpp
> @@ +1,2 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim:set ts=2 sw=2 sts=2 et cindent: */
> 
> Minor, but the mode lines differ from the coding style versions:

Oops, copy-paste fail.

> @@ +37,5 @@
> > +  mTasks.push(aRunnable);
> > +  if (mIsRunning) {
> > +    return NS_OK;
> > +  }
> > +  mIsRunning = true;
> 
> Set mIsRunning to true after Dispatch succeeds?  Then move the assert until
> after mQueueMonitor is held?

Ah yes. Good idea.


> @@ +53,5 @@
> > +  AwaitIdle(mon);
> > +}
> > +
> > +void
> > +MediaTaskQueue::AwaitIdle(MonitorAutoLock& aMonitor)
> 
> I prefer the AwaitIdle/AwaitIdleUnlocked pattern.  You can't pass any
> monitor into AwaitIdle as it's making assumptions about what data is
> protected by the monitor, so it seems better to just make this unlocked and
> assert that mQueueMonitor is already held before waiting on it.

Fair point. I'll change the private AwaitIdle to AwaitIdleLocked() and assert the monitor is held.


> @@ +105,5 @@
> > +  MOZ_ASSERT(event);
> > +  {
> > +    // Take a lock. This ensures we have a memory fence enforced, so that
> > +    // if the object we're calling wasn't designed to be threadsafe, it will
> > +    // be.
> 
> Doesn't the fact that mQueueMonitor has been taken and dropped provide the
> same guarantee?

Yeah, provided we cycle the lock before and after running the task we should be good. I'll add comments noting that. Thanks!

Comment 7

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

Most of content/media has incorrect mode lines:    :(

cpearce@MIDAS ~/src/mozilla/purple
$ grep -r "Mode: C++; tab-width:" content/media/* | grep "tab-width: 8" | wc -l
16

cpearce@MIDAS ~/src/mozilla/purple
$ grep -r "Mode: C++; tab-width:" content/media/* | grep "tab-width: 2" | wc -l
338

cpearce@MIDAS ~/src/mozilla/purple
$ grep -r "Mode: C++; tab-width:" content/media/* | wc -l
357

Comment 8

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

Attachment: Patch 1v2: MediaTaskQueue

Mode lines fixed, removed task monitor, switch to AwaitIdleLocked(), and set mIsRunning to true only after Dispatch succeeds.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8367109 [details] [diff] [review]
Patch 1v2: MediaTaskQueue

+    MOZ_ASSERT(mQueue->mIsRunning);
+    MonitorAutoLock mon(mQueue->mQueueMonitor);

Need to swap the order of these since making the mIsRunning change.

Comment 10

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

Whoops, thanks!

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8367066 [details] [diff] [review]
Patch 2: Make MediaDataDecoder asynchronous

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

::: content/media/fmp4/BlankDecoderModule.cpp
@@ +66,2 @@
>    {
> +    // We must delete the same when we're finished with it.

same = sample?  Comment is slightly confusing anyway, as the ownership (and thus responsibility for destructor) is held by the OutputEvent.

::: content/media/fmp4/MP4Reader.cpp
@@ +84,5 @@
>  MP4Reader::MP4Reader(AbstractMediaDecoder* aDecoder)
> +  : MediaDecoderReader(aDecoder)
> +  , mLayersBackendType(layers::LayersBackend::LAYERS_NONE)
> +  , mAudio("MP4 audio decoder data", Preferences::GetUint("media.mp4-audio-decode-ahead", 2))
> +  , mVideo("MP4 video decoder data", Preferences::GetUint("media.mp4-audio-decode-ahead", 2))

mp4-video-decode-ahead?

@@ +304,5 @@
> +// * Cache the value of mNumFramesOutput, as prevFramesOutput.
> +// * While we've not output data (mNumFramesOutput != prevNumFramesOutput)
> +//   and while we still require input, we demux and input data in the reader.
> +//   We assume we require input if
> +//   ((mNumFrameInput - mNumFramesOutput) < sDecodeAheadMargin) or

I can't find mNumFrameInput (or mNumFramesInput) anywhere.  Same with mNumFramesOut.  s/Frames/Samples/?

@@ +322,2 @@
>  
> +  data.mMonitor.Lock();

I guess doing this manually works out tidier than using MonitorAutoLock/MonitorAutoUnlock with extra nesting?

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8367067 [details] [diff] [review]
Patch 3: Make WMF PlatformDecoderModule async

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

::: content/media/fmp4/wmf/WMFDecoderModule.cpp
@@ +76,5 @@
> +  return new WMFMediaDataDecoder(new WMFVideoOutputSource(aLayersBackend,
> +                                                          aImageContainer,
> +                                                          sDXVAEnabled),
> +                             aVideoTaskQueue,
> +                             aCallback);

Nit: indent weirdness.

Comment 13

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

Thanks Matthew and Paul.

(In reply to Matthew Gregan [:kinetik] from comment #11)
> Comment on attachment 8367066 [details] [diff] [review]
> Patch 2: Make MediaDataDecoder asynchronous

> @@ +322,2 @@
> >  
> > +  data.mMonitor.Lock();
> 
> I guess doing this manually works out tidier than using
> MonitorAutoLock/MonitorAutoUnlock with extra nesting?

Basically yes. And doing this avoids needing to retake the lock after dropping it to call something, just so that the top level MonitorAutoLock can drop it again on exit. i.e. we save one lock cycle.

Comment 14

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

Landed, with review comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/519a6a48fb8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3c4d6edf9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/144021ba1c5c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/519a6a48fb8e
https://hg.mozilla.org/mozilla-central/rev/8b3c4d6edf9a
https://hg.mozilla.org/mozilla-central/rev/144021ba1c5c

Comment 16

[Jacek Caban]

I pushed a trivial fixup for GCC build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/30831efd61f0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30831efd61f0