Label runnables in dom/media/MediaStreamGraph.cpp

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
Rank:
19
RESOLVED FIXED
2 years ago
a year 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

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

Updated

a year ago
Depends on: 1330360
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year 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

a year 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

a year 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.

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0c4f2ca44b4
https://hg.mozilla.org/mozilla-central/rev/5c2fea87df16
https://hg.mozilla.org/mozilla-central/rev/29ac12c81bcd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.