Refactor MP4Reader to run its decoders async

RESOLVED FIXED in mozilla30

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug)

29 Branch
mozilla30
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Blocks: 941296
Blocks: 941302, 941301, 941298
Created attachment 8367062 [details] [diff] [review]
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.
Attachment #8367062 - Flags: review?(kinetik)
Created attachment 8367066 [details] [diff] [review]
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.
Attachment #8367066 - Flags: review?(kinetik)
Created attachment 8367067 [details] [diff] [review]
Patch 3: Make WMF PlatformDecoderModule async

Makes WMF's MediaDataDecoders conform to the async interface.
Attachment #8367067 - Flags: review?(paul)
https://tbpl.mozilla.org/?tree=Try&rev=53f01bedc9a3
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?
(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!
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
Created attachment 8367109 [details] [diff] [review]
Patch 1v2: MediaTaskQueue

Mode lines fixed, removed task monitor, switch to AwaitIdleLocked(), and set mIsRunning to true only after Dispatch succeeds.
Attachment #8367062 - Attachment is obsolete: true
Attachment #8367062 - Flags: review?(kinetik)
Attachment #8367109 - Flags: review?(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.
Attachment #8367109 - Flags: review?(kinetik) → review+
Whoops, thanks!
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?
Attachment #8367066 - Flags: review?(kinetik) → review+
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.
Attachment #8367067 - Flags: review?(paul) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/519a6a48fb8e
https://hg.mozilla.org/mozilla-central/rev/8b3c4d6edf9a
https://hg.mozilla.org/mozilla-central/rev/144021ba1c5c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Comment 16

4 years ago
I pushed a trivial fixup for GCC build:

https://hg.mozilla.org/integration/mozilla-inbound/rev/30831efd61f0
https://hg.mozilla.org/mozilla-central/rev/30831efd61f0
No longer blocks: 941302
Blocks: 941302
Blocks: 979104
No longer blocks: 979104
Depends on: 1143887
You need to log in before you can comment on or make changes to this bug.