Label runnables in dom/media/MediaStreamGraph.cpp

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
Rank:
19
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: padenot)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(2 attachments)

Reporter

Description

2 years ago
See https://wiki.mozilla.org/Quantum/DOM for the motivation.
Reporter

Updated

2 years ago
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
Reporter

Comment 2

2 years ago
Hi Paul
Can you find someone to take this bug? Thanks!
Flags: needinfo?(padenot)
Assignee

Comment 3

2 years ago
(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)

Updated

2 years ago
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)
Assignee

Comment 5

2 years ago
I'll start now. Sorry about the delay, was on PTO.
Flags: needinfo?(padenot)
Assignee

Updated

2 years ago
Depends on: 1330360
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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 9

2 years ago
mozreview-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+

Comment 10

2 years ago
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.