Closed Bug 1341555 Opened 3 years ago Closed 3 years ago

Label runnables in dom/media/MediaStreamGraph.cpp

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jwwang, Assigned: padenot)

References

Details

(Whiteboard: [QDL][BACKLOG][MEDIA])

Attachments

(2 files)

See https://wiki.mozilla.org/Quantum/DOM for the motivation.
Blocks: 1341539
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.
Rank: 19
Priority: -- → P1
Hi Paul
Can you find someone to take this bug? Thanks!
Flags: needinfo?(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.
Assignee: nobody → padenot
Flags: needinfo?(padenot)
Whiteboard: [QDL][BACKLOG][MEDIA]
How's this going, Paul? MediaStreamGraphStableStateRunnable is really common, so it would be nice to get this fixed.
Flags: needinfo?(padenot)
I'll start now. Sorry about the delay, was on PTO.
Flags: needinfo?(padenot)
Depends on: 1330360
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.
Attachment #8882005 - Flags: review?(apehrson) → review+
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.
Attachment #8882006 - Flags: review?(apehrson) → review+
Pushed by paul@paul.cx:
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.
You need to log in before you can comment on or make changes to this bug.