MediaStreamGraph::GetInstance() is broken

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(scheng)
Flags: needinfo?(roc)
(Assignee)

Updated

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

Comment 2

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

Comment 4

3 years ago
It means we have to remove SetMozAudioChannelType. But yes. we can.

Comment 5

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #4)
> It means we have to remove SetMozAudioChannelType. But yes. we can.

Let's do that!
(Assignee)

Comment 6

3 years ago
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.
Attachment #8496783 - Flags: review?(roc)
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=486cc913829b
Attachment #8496783 - Flags: review?(roc) → review+
(Assignee)

Comment 8

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

Updated

3 years ago
Blocks: 1074757
(Assignee)

Updated

3 years ago
Blocks: 1074760
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f95edf23a4
https://hg.mozilla.org/mozilla-central/rev/11f95edf23a4
Status: NEW → RESOLVED
Last Resolved: 3 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.
(Assignee)

Comment 13

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

Updated

3 years ago
Flags: needinfo?(scheng)
(Assignee)

Comment 16

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a8651f9a26e

If green, we can land it.
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago3 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://hg.mozilla.org/mozilla-central/rev/dccf8c0eb2b7
(Assignee)

Comment 21

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

Updated

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

Comment 23

3 years ago
Created attachment 8523827 [details] [diff] [review]
bug.patch
Attachment #8496783 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc3209d766a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acc3209d766a
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1223670
Depends on: 1228484
Depends on: 1107534
Depends on: 1179484
No longer depends on: 1228484
You need to log in before you can comment on or make changes to this bug.