Use multiple MediaStreamGraph in the same process

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

50 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
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

Updated

3 years ago
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Assignee

Comment 2

3 years ago
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+
Assignee

Updated

2 years ago
Blocks: 1341555
Assignee

Updated

2 years ago
Depends on: 1371719
Assignee

Updated

2 years ago
Depends on: 1372247

Comment 8

2 years ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de53323f3ad
Create new MSGs for each nsPIDOMWindow. r=jesup

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5de53323f3ad
Status: ASSIGNED → RESOLVED
Closed: 2 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)
Assignee

Comment 11

2 years ago
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
See Also: → 1406027
Depends on: 1406027
See Also: 1406027
Depends on: 1407088
You need to log in before you can comment on or make changes to this bug.