1341555 - Label runnables in dom/media/MediaStreamGraph.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

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

See https://wiki.mozilla.org/Quantum/DOM for the motivation.

Comment 1

[Karl Tomlinson (:karlt)]

The MSG currently uses a single main thread runnable for updates to all doc
groups.

Perhaps we want a separate MSG for each doc group.

Comment 2

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

Hi Paul
Can you find someone to take this bug? Thanks!

Comment 3

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (:karlt) from comment #1)
> The MSG currently uses a single main thread runnable for updates to all doc
> groups.
> 
> Perhaps we want a separate MSG for each doc group.

I've written the start of a patch for this in bug 1330360. The one problem we face is that for now, to feed the reverse stream to the echo canceller, we take advantage of the fact that the MSG is mixing the audio for all the real-time audio streams of a Firefox process. With multi e10s, this will not be true anymore.

I'm working on getting us a real reverse-stream (using OS facilities, such as pulse monitoring devices, and loopback devices on Windows), so this issue will be gone. My patch works fine, is simple, and brings us a bunch of benefits: load/failure isolation between MSGs, better chance that things will work when refreshing a document, and it simply makes sense if we end up having more than one main thread at some point.

Also, yes, I'm taking this.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How's this going, Paul? MediaStreamGraphStableStateRunnable is really common, so it would be nice to get this fixed.

Comment 5

[Paul Adenot (:padenot)]

I'll start now. Sorry about the delay, was on PTO.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1341555 - Label MSG runnables.

Review commit: https://reviewboard.mozilla.org/r/153086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153086/

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1341555 - Consolidate use of the MSG's AbstractMainThread, and stop having AbstractMainThread on MediaStreams.

Review commit: https://reviewboard.mozilla.org/r/153088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153088/

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8882005 [details]
Bug 1341555 - Label MSG runnables.

https://reviewboard.mozilla.org/r/153086/#review158306

::: dom/media/MediaStreamGraph.h:1281
(Diff revision 1)
>    // Main thread only
>    static MediaStreamGraph* GetInstance(GraphDriverType aGraphDriverRequested,
>                                         dom::AudioChannel aChannel,
>                                         nsPIDOMWindowInner* aWindow);
> -  static MediaStreamGraph* CreateNonRealtimeInstance(TrackRate aSampleRate);
> +  static MediaStreamGraph* CreateNonRealtimeInstance(TrackRate aSampleRate,
> +                                                     nsPIDOMWindowInner* aWindowId);

Looking at the class it feels like this should be `aWindow`.

::: dom/media/MediaStreamGraph.cpp:1015
(Diff revision 1)
> -    NS_DispatchToMainThread(WrapRunnable(this,
> +    RefPtr<nsIRunnable> runnable = WrapRunnable(this,
> -                                         &MediaStreamGraphImpl::OpenAudioInput,
> +                                  &MediaStreamGraphImpl::OpenAudioInput,
> -                                         aID, RefPtr<AudioDataListener>(aListener)));
> +                                  aID, RefPtr<AudioDataListener>(aListener));

Indentation

::: dom/media/MediaStreamGraph.cpp:1018
(Diff revision 1)
>    // So, so, so annoying.  Can't AppendMessage except on Mainthread
>    if (!NS_IsMainThread()) {
> -    NS_DispatchToMainThread(WrapRunnable(this,
> +    RefPtr<nsIRunnable> runnable = WrapRunnable(this,
> -                                         &MediaStreamGraphImpl::OpenAudioInput,
> +                                  &MediaStreamGraphImpl::OpenAudioInput,
> -                                         aID, RefPtr<AudioDataListener>(aListener)));
> +                                  aID, RefPtr<AudioDataListener>(aListener));
> +    mAbstractMainThread->Dispatch(runnable.forget());

Since you implemented `MediaStreamGraphImpl::Dispatch`, should we use that everywhere? For consistency.

And if you're changing all of them anyway, you could just as well add `do_AddRef` where appropriate.

::: dom/media/MediaStreamGraph.cpp:1888
(Diff revision 1)
>    EnsureRunInStableState();
>  }
>  
> +void MediaStreamGraphImpl::Dispatch(already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  mAbstractMainThread->Dispatch(std::move(aRunnable));

We want mfbt's `Move` over std::move IIRC.

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8882006 [details]
Bug 1341555 - Consolidate use of the MSG's AbstractMainThread, and stop having AbstractMainThread on MediaStreams.

https://reviewboard.mozilla.org/r/153088/#review158314

::: dom/media/MediaStreamGraph.h
(Diff revision 1)
>   *
>   * When the graph is changed, we may need to throw out buffered data and
>   * reprocess it. This is triggered automatically by the MediaStreamGraph.
>   */
>  
> -class AbstractThread;

Are you sure this is OK for non-unified builds?

::: dom/media/MediaStreamGraph.h:269
(Diff revision 1)
>    /**
> +   * Return the right main thread for this MediaStream.
> +   */
> +  AbstractThread* AbstractMainThread();

I don't see this used anywhere.

::: dom/media/MediaStreamGraph.cpp:1889
(Diff revision 1)
> -void MediaStreamGraphImpl::Dispatch(already_AddRefed<nsIRunnable>&& aRunnable)
> +void
> +MediaStreamGraphImpl::Dispatch(already_AddRefed<nsIRunnable>&& aRunnable)

Put this in the other patch.

::: dom/media/MediaStreamGraph.cpp:4192
(Diff revision 1)
> -    aMainThread->CreateDirectTaskDrainer(Move(aRunnable));
> +    static_cast<MediaStreamGraphImpl*>(this)
> +      ->AbstractMainThread()

Use `MediaStreamGraph::AbstractMainThread()` instead.

::: dom/media/MediaStreamGraphImpl.h:823
(Diff revision 1)
>  
>    dom::AudioChannel AudioChannel() const { return mAudioChannel; }
>  
>    // used to limit graph shutdown time
>    nsCOMPtr<nsITimer> mShutdownTimer;
> +  const RefPtr<AbstractThread> mAbstractMainThread;

This should be in the other patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c4f2ca44b4
Label MSG runnables. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2fea87df16
Consolidate use of the MSG's AbstractMainThread, and stop having AbstractMainThread on MediaStreams. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/29ac12c81bcd
Make static analysis happier.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0c4f2ca44b4
https://hg.mozilla.org/mozilla-central/rev/5c2fea87df16
https://hg.mozilla.org/mozilla-central/rev/29ac12c81bcd