1330360 - Use multiple MediaStreamGraph in the same process

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

This is a matter of changing what is the key in the `gGraphs` hash table. This will allow lowering the load of each `MediaStreamGraph`, and is going to be better for Quantum DOM for their event tagging thing.

For now, this uses `nsPIDOMWindowInner` because it's readily available. We can change that as needed, and use DocGroups, for example.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Use multiple MSG per process, based on the document. WIP DONT LAND

As said above, work in progress, this seem to work fine.

Eitan, we're trying to have an MSG per document. What should I do with WebSpeech
? For now, all WebSpeech stuff will share the same MSG, but I can do something
better if needed.

What needs to be done is to test this better, and to measure the performance
delta between what we have now and this.

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8825862 [details] [diff] [review]
Use multiple MSG per process, based on the document. WIP DONT LAND

jesup, if you want to have a look...

Comment 3

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

Comment on attachment 8825862 [details] [diff] [review]
Use multiple MSG per process, based on the document. WIP DONT LAND

Review of attachment 8825862 [details] [diff] [review]:
-----------------------------------------------------------------

f+ for most of it.

This gives us one graph per inner document, not per doc-group (unless doc-group == innerwindow, which I didn't think it does, but I haven't touched docgroups in a while).  This might be ok, if the graphs in a docgroup have to stay totally independent.

I also note we'll have one callback with cubeb registered per graph, instead of one OS callback farmed out to N graphs, right?  (admittedly the simplest solution, and if perf is ok on all platforms on low-end machines, ok.)
Also I worry this will interact poorly with the GraphDriver code for audio input when multiple tabs have input open.  Right now they all filter down to a single full-duplex channel, but with this there will be N full-duplex channels, which could cause confusion with the output data fed back to the AEC, and insert multiple copies of the input data.  I think we'd need to ensure that all input channels funnel through one MSG --- and then we'd have to handle the one MSG with the input channel being destroyed - switch a different MSG to full-duplex.

So we really should consider having all the MSGs share an output driver, which makes the evilness here all melt away.  We might lose possible parallelism in running the graphs IF the OS would invoke multiple graphs in parallel on different OS threads, which it may not do.  This might also more cleanly map to future audio sandboxing work.

::: dom/media/MediaStreamGraph.cpp
@@ +3427,4 @@
>  MediaStreamGraph*
>  MediaStreamGraph::GetInstance(MediaStreamGraph::GraphDriverType aGraphDriverRequested,
> +                              dom::AudioChannel aChannel,
> +                              nsPIDOMWindowInner* aWindow)

*aWindow is const, no?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Paul Adenot (:padenot) from comment #1)
> Created attachment 8825862 [details] [diff] [review]
> Use multiple MSG per process, based on the document. WIP DONT LAND
> 
> As said above, work in progress, this seem to work fine.
> 
> Eitan, we're trying to have an MSG per document. What should I do with
> WebSpeech
> ? For now, all WebSpeech stuff will share the same MSG, but I can do
> something
> better if needed.
> 
> What needs to be done is to test this better, and to measure the performance
> delta between what we have now and this.

WebSpeech's current use of MSG is only in B2G (nsPicoService). I think we should remove that service and stop using MSG altogether. Other platforms send the audio to the device from the speech service, so we don't have a use for this.

Comment 5

[Eitan Isaacson [:eeejay]]

I guess two followup bugs would be:

a) remove nsPicoService.
b) remove "direct audio" speech service support (ie. remove all code that uses MSG).

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 8825862 [details] [diff] [review]
Use multiple MSG per process, based on the document. WIP DONT LAND

Thumbs up from speech end (I think we need to stop using MSG directly altogether).

Comment 7

[Eitan Isaacson [:eeejay]]

Opened a bug for that.. bug 1331696.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de53323f3ad
Create new MSGs for each nsPIDOMWindow. r=jesup

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5de53323f3ad

Comment 10

[Karl Tomlinson (:karlt)]

Paul, 5de53323f3ad looks similar to the patch here.
Did that land accidentally, or with the wrong bug number?

Comment 11

[Paul Adenot (:padenot)]

Ha this is confusing indeed. I landed this (rebased and slightly modified iirc) after talking with randell during the SF work week, and doing a bunch of work in other bugs (1341555, 1371719, 1372247).

I should have reflected this in the flags.

Comment 12

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

We need to resolve the regression from this -- bug 1381638