Closed Bug 1330360 Opened 4 years ago Closed 3 years ago

Use multiple MediaStreamGraph in the same process

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(1 file)

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.
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.
Attachment #8825862 - Flags: feedback?(eitan)
Assignee: nobody → padenot
Status: NEW → ASSIGNED
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...
Attachment #8825862 - Flags: feedback?(rjesup)
Rank: 25
Priority: -- → P2
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?
Attachment #8825862 - Flags: feedback?(rjesup) → feedback-
(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.
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 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).
Attachment #8825862 - Flags: feedback?(eitan) → feedback+
Opened a bug for that.. bug 1331696.
Blocks: 1341555
Depends on: 1371719
Depends on: 1372247
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de53323f3ad
Create new MSGs for each nsPIDOMWindow. r=jesup
https://hg.mozilla.org/mozilla-central/rev/5de53323f3ad
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1387454
Paul, 5de53323f3ad looks similar to the patch here.
Did that land accidentally, or with the wrong bug number?
Flags: needinfo?(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.
Flags: needinfo?(padenot)
We need to resolve the regression from this -- bug 1381638
Depends on: 1407088
You need to log in before you can comment on or make changes to this bug.