Closed Bug 1073615 Opened 5 years ago Closed 5 years ago

MediaStreamGraph::GetInstance() is broken

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

https://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#2781

This code is very buggy because it creates a gGraph the first time using the 2 parameters (aHint and the audiochannel). But the second time it runs, it ignores the parameters and returns the previous gGraph object.

That has to be fixed and here the proposal:

1. GetInstance() should not take parameters. Instead "Normal" audio channel must be used and no hint should be taken.

2. MediaStream should store the audiochannel.

3. the audio backend should be created lazily when it's needed: it means when MediaStream has something to do.

4. At this point we create the cubeb backend if we don't already have it something for that particular channel.

5. If the MediaStream changes AudioChannel, we can try to change the cubeb backend.

Feedbacks?
Flags: needinfo?(scheng)
Flags: needinfo?(roc)
Blocks: 1043762
An MSG instance is driven by a libcubeb callback. So there can only be one cubeb instance per MSG. This means we have to have one MSG per AudioChannel. That means you can't connect a MediaStream to two different AudioChannel outputs, but I hope we can live with that at least for now. (It's fixable later, at the cost of introducing some latency for such MediaStreams.)

That means we need to change gGraph into a collection indexed by AudioChannel.
Flags: needinfo?(roc)
The problem I see is this: AudioContext() creates an AudioDestinationNode() and that get an instance of a MediaStreamGraph object using a particular AudioChannel.

In AudioContext() we can change the AudioChannel using 'setMozAudioChannelType(foobar)'. That doesn't really touch the MediaStreamGraph backend but it just changes an attribute in MediaStream.

Can we connect a MediaStream to a different backend when the AudioChannel changes?
Flags: needinfo?(roc)
(In reply to Andrea Marchesini (:baku) from comment #2)
> The problem I see is this: AudioContext() creates an AudioDestinationNode()
> and that get an instance of a MediaStreamGraph object using a particular
> AudioChannel.
> 
> In AudioContext() we can change the AudioChannel using
> 'setMozAudioChannelType(foobar)'. That doesn't really touch the
> MediaStreamGraph backend but it just changes an attribute in MediaStream.
> 
> Can we connect a MediaStream to a different backend when the AudioChannel
> changes?

No. Can we take away the ability to change the audio channel type?
Flags: needinfo?(roc)
It means we have to remove SetMozAudioChannelType. But yes. we can.
(In reply to Andrea Marchesini (:baku) from comment #4)
> It means we have to remove SetMozAudioChannelType. But yes. we can.

Let's do that!
Attached patch bug.patch (obsolete) — Splinter Review
If we like this patch, we have to inform the gaia team about this readonly mozAudioChannelType attribute because I guess they are using it a lot for ffos.
Attachment #8496783 - Flags: review?(roc)
https://tbpl.mozilla.org/?tree=Try&rev=87a58f9715fc

Waiting for a full green result on try, I write an email to gaia-dev mailing list.
Blocks: 1074757
Blocks: 1074760
https://hg.mozilla.org/mozilla-central/rev/11f95edf23a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Backed out under the strong suspicion of causing bug 1075345, which is extremely frequent on OSX.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3de2f8ec3d
Status: RESOLVED → REOPENED
Depends on: 1075345
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
Depends on: 1076394
(In reply to Andrea Marchesini (:baku) from comment #6)
> Created attachment 8496783 [details] [diff] [review]
> bug.patch
> 
> If we like this patch, we have to inform the gaia team about this readonly
> mozAudioChannelType attribute because I guess they are using it a lot for
> ffos.

This broke a lot of stuff in gaia, can we communicate changes like this before this happens again in the future? We would also need some time to adopt our code and land at the same time that gecko does if it's a breaking change like this.
It seems that this patch breaks some mochitest on OSX (bug 1075345).
Wondering if you have some idea why... Of course, I cannot reproduce this issue locally (on linux).
Flags: needinfo?(roc)
Also caused a big spike on Android (bug 1061675).
(In reply to Andrea Marchesini (:baku) from comment #13)
> It seems that this patch breaks some mochitest on OSX (bug 1075345).
> Wondering if you have some idea why... Of course, I cannot reproduce this
> issue locally (on linux).

I don't know. Also, I'm on vacation. I suggest you add some info()s to the test and to try pushes on Mac to figure out at which point during the test we get stuck.
Flags: needinfo?(roc)
Flags: needinfo?(scheng)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a476e673470

Will be watching carefully for any orange spikes.
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5a476e673470
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1097721
Intermittents are back (see deps). Backed out. On the bright side, no reports of any Gaia issues this time around.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccf8c0eb2b7
Status: RESOLVED → REOPENED
Depends on: 1061675
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e14cdddbcf5f
it seems that the dep issue is a timeout problem. Increasing the timeout looks fully green so far.
Keywords: checkin-needed
Hi, seems the patch didn't apply cleanly:

unable to find 'content/media/MediaStreamGraph.cpp' for patching
6 out of 6 hunks FAILED -- saving rejects to file content/media/MediaStreamGraph.cpp.rej
unable to find 'content/media/MediaStreamGraphImpl.h' for patching
2 out of 2 hunks FAILED -- saving rejects to file content/media/MediaStreamGraphImpl.h.rej
unable to find 'content/media/webaudio/AudioContext.cpp' for patching
1 out of 1 hunks FAILED -- saving rejects to file content/media/webaudio/AudioContext.cpp.rej
unable to find 'content/media/webaudio/AudioContext.h' for patching
1 out of 1 hunks FAILED -- saving rejects to file content/media/webaudio/AudioContext.h.rej
unable to find 'content/media/webaudio/test/test_mozaudiochannel.html' for patching
2 out of 2 hunks FAILED -- saving rejects to file content/media/webaudio/test/test_mozaudiochannel.html.rej
patch failed, unable to continue (try -v)
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Attached patch bug.patchSplinter Review
Attachment #8496783 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acc3209d766a
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1223670
Depends on: 1228484
Depends on: 1107534
Depends on: CVE-2015-4477
No longer depends on: 1228484
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.