1239135 - Allow sending MediaStreamGraph control messages from off-mainthread

Status: NEW

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

Description

[Randell Jesup [:jesup] (needinfo me)]

The MSG AppendMessage() is MainThread only, so to interact with MediaStreamGraph you have to often send a runnable to MainThread (and deal with UI/JS/etc jank).  This is both annoying and hurts quality by delaying realtime actions.

It should be possible to send control messages from any thread.

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

RunMessageAfterProcessing() is available from the graph thread, but not from other threads.
https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/dom/media/MediaStreamGraphImpl.h#254

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Right - I wanted to send it messages from the MediaManager thread (getUserMedia/etc)

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

A short-term solution is to make MediaStreamGraphImpl::AppendMessage doing DispatchToMainThread instead of "MOZ_ASSERT(NS_IsMainThread(), "main thread only")";
Is that good enough?

Comment 4

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

I think the reason for the main thread limitation is that it provides stable state which allows changes to MSG happen in an indivisible fashion.

Doing DispatchToMainThread internally will not provide this batch effect.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Well, my personal thought is that my doing DispatchToMainThread is not really different than someone else doing it.  However, if you want to submit multiple messages one could do that from MainThread and they'd be batched up for MSG to process in one iteration (presuming this batching is handled via RunInStableState).  So code that cares about batching would need to not run from other threads.  Code that *doesn't* care about batching can run anywhere.

There is another way to address this, which is to be explicit about what commands need to be synchronized - much as we have a 'queued' option to AddTrack and FinishAddTrack(). For commands, we could have a StartMessageGroup()/EndMessageGroup() option - but then we have to be able to reliably scope the grouping, which may be a pain.

We could decide that grouping only applies to MainThread calls.  Or provide a clearly-delineated off-main-thread-allowed call, but the problem is that the API generally hides the AppendMessage calls from the calling code, just exposing "AddTrack" etc.  We could add an ForwardMessages() call to tell it forwarding is allowed, or default to forwarding off mainthread, with explicit StartMessageGroup/EndMessageGroup for use off-main-thread (for the rare case where we care)

Alternatively, we could look at revising the entire way this works to try to reduce the reliance on MainThread as a synchronizing aspect except where it comes to syncing with JS state (in both directions).   Perhaps my last proposal in the previous paragraph is best (allow, and allow off-main-thread code to explicitly group).

Comment 6

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

AbstractThread employs a tail dispatcher to provide run-to-completion which doesn't require explicit BeginTransaction/EndTransaction pairs to group messages. This might be something to consider about.

Comment 7

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.