WebAudio can't be sent over peerConnection. From Bug 1070127 comment 16: MediaPipeLineTransmit listens to TrackUnionStream for data. There appears to be a bug in TrackUnionStream in that it fails to call NotifyQueuedTrackChanges on listeners when data comes in from being the mOutputStream of an AudioNodeStream rather than through the regular CopyTrackData calls. From the existence of FilterAudioNodeStreamTrack it looks like at least two ways of propagating data are in play here. STR: 1. Run URL test-script with patches in Bug 1070127 and hit [Start] button. 2. Breakpoint on the last TrackUnionStream::ProcessInput and notice how it doesn't call CopyTrackData to pull data from sources and therefore never calls NotifyQueuedTrackChanges on listeners either. Instead webaudio sound seems to come in through MediaStreamDestinationEngine::ProcessBlock which pushes audio segments to mOutputStream which is also the TrackUnionStream, but not notification of listeners seems to take place in this case, so nothing alerts MediaPipeline. 3. No sound on the remote end.

Renamed to differentiate this from Bug 1070127.

What would it take to fix this? Is this something Pehrson could work on?

No idea, I haven't looked into this in some time. This is probably a good thing for him to look at. I'm happy to give a quick explanation on how Web Audio/the MSG works, to save him a couple hours of code reading.

Hi Andreas -- Welcome back. :-) Do you think you could take this one as your next bug? It is the last bug that prevents PeerConnections from accepting a Web Audio source, and we want to get it landed in Fx39 (the cycle that just started). Ideally this would land in the next 2-3 weeks. Do you think you'll have the time to do this? Thanks!

Sure, I'll take a look.

(In reply to Paul Adenot (:padenot) from comment #3) > No idea, I haven't looked into this in some time. This is probably a good > thing for him to look at. > > I'm happy to give a quick explanation on how Web Audio/the MSG works, to > save him a couple hours of code reading. I'd like to think I understand the MSG fairly well by now, but if things look hairy I'll ask you for guidance :)

For the MediaStreamAudioDestinationNode we have mStream and mDOMStream (wrapping the TrackUnionStream used for output). mStream's inputs are processed In MediaStreamAudioDestinationNode::ProcessBlock, where we're also processing its output (the input for the output stream). This seems backwards; can't we just let the TrackUnionStream under mDOMStream have its inputs processed by the graph like a normal stream? There's also a track id filter in place on the output stream to avoid that we're processing mStream's track 1 as an input to the output stream during ProcessInput(). I don't quite follow the rationale behind this, is it just to avoid ending up with two tracks in the output stream in case it would find and process its input (mStream)? Paul, do you know why the filter is/was needed? (https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/MediaStreamAudioDestinationNode.cpp#60)

Here's assuming the track id filter is for nothing more than I wondered about in my previous comment. Instead MediaStreamAudioDestinationNode::ProcessBlock handling the copying of data to the output stream, this patch lets the output stream do it per the standard TrackUnionStream::ProcessInput. This also allows us to scrap |TrackIDFilterCallback| and |MediaStreamDestinationEngine| to become quite a bit cleaner. Works fine with the provided test page (thanks jib). Mochitest to come.

Hints were creating a track with a different ID than the one coming from the webaudio node chain of streams. I just changed the patch to not set any hints on the DOMMediaStream used for output; instead we create a track on the DOMMediaStream with the right ID. This is similar to what we do with hints removed in bug 1129263 and will simplify rebasing on it (or this, whichever lands first).

Note that this test passes before the fix as well. I guess we don't have a way to check that audio (or the right audio, perhaps) is actually flowing over the connection? We'll need a mochitest for a supported feature like this anyhow.

Comment on attachment 8571266 [details] [diff] [review] Add mochitest for sending WebAudio over PeerConnection Review of attachment 8571266 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/mochitest.ini @@ +177,5 @@ > [test_peerConnection_addDataChannel.html] > skip-if = toolkit == 'gonk' # b2g (Bug 1059867) > [test_peerConnection_addDataChannelNoBundle.html] > skip-if = toolkit == 'gonk' # b2g (Bug 1059867) > +[test_peerConnection_webAudio.html] Since we should have tests for hooking webaudio to the input and output of peerconnections, let's do both here, or make two separate tests and rename this one. I'd be fine with one test that checks both (though the other direction could be done in an followon bug)

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10) > Created attachment 8571266 [details] [diff] [review] > Add mochitest for sending WebAudio over PeerConnection > > Note that this test passes before the fix as well. I guess we don't have a > way to check that audio (or the right audio, perhaps) is actually flowing > over the connection? > > We'll need a mochitest for a supported feature like this anyhow. Well you can connect the other side to an AudioContext and do a correlation check or something. I don't know how our WebRTC mochitest work, though, so maybe I'm saying something stupid.

Comment on attachment 8571264 [details] [diff] [review] Let the output stream itself process input data from MediaStreamAudioNodeStream Review of attachment 8571264 [details] [diff] [review]: ----------------------------------------------------------------- roc, I think this is fine, am I missing something? I think I don't have enough context here, I tried to read the old bug, but quite a lot have changed since then.

Comment on attachment 8571264 [details] [diff] [review] Let the output stream itself process input data from MediaStreamAudioNodeStream Review of attachment 8571264 [details] [diff] [review]: ----------------------------------------------------------------- I can't remember why I thought mStream would have two tracks...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > Comment on attachment 8571264 [details] [diff] [review] > Let the output stream itself process input data from > MediaStreamAudioNodeStream > > Review of attachment 8571264 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can't remember why I thought mStream would have two tracks... Source of this change: https://bugzilla.mozilla.org/show_bug.cgi?id=865257#c34 Is the reasoning from the original review/landing now obsolete?

(In reply to Paul Adenot (:padenot) from comment #12) > Well you can connect the other side to an AudioContext and do a correlation > check or something. I don't know how our WebRTC mochitest work, though, so > maybe I'm saying something stupid. Not at all, worked out great. This test also fails without the fix \o/ r? jesup for the WebRTC test, padenot for WebAudio usage.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f27969be4b Looks OK, but sanity algorithm will need some tuning for Android. E.g., one case looks like this: 00:24:29 INFO - 1668 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 29/1024, input=51, output=102 - expected PASS 00:24:29 INFO - 1669 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 30/1024, input=128, output=137 00:24:29 INFO - 1670 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 31/1024, input=224, output=224 00:24:29 INFO - 1671 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 32/1024, input=255, output=255 00:24:29 INFO - 1672 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 33/1024, input=255, output=255 00:24:29 INFO - 1673 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 34/1024, input=225, output=226 00:24:29 INFO - 1674 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 35/1024, input=130, output=138 00:24:29 INFO - 1675 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 36/1024, input=51, output=114 - expected PASS 00:24:29 INFO - 1676 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_webAudio.html | Sane audio frequency values at index 37/1024, input=47, output=104 - expected PASS

(In reply to Randell Jesup [:jesup] from comment #15) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > > Comment on attachment 8571264 [details] [diff] [review] > > Let the output stream itself process input data from > > MediaStreamAudioNodeStream > > > > Review of attachment 8571264 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I can't remember why I thought mStream would have two tracks... > > Source of this change: > https://bugzilla.mozilla.org/show_bug.cgi?id=865257#c34 Yeah, I read that already, and I still don't remember why I said that. :-)

Comment on attachment 8571777 [details] [diff] [review] Add mochitest for piping WebAudio in and out of PeerConnection Review of attachment 8571777 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > (In reply to Randell Jesup [:jesup] from comment #15) > > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > > > Comment on attachment 8571264 [details] [diff] [review] > > > Let the output stream itself process input data from > > > MediaStreamAudioNodeStream > > > > > > Review of attachment 8571264 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > I can't remember why I thought mStream would have two tracks... > > > > Source of this change: > > https://bugzilla.mozilla.org/show_bug.cgi?id=865257#c34 > > Yeah, I read that already, and I still don't remember why I said that. :-) From the patches, ehsan was adding a second track with a different trackid for output, and there are assertions to make sure that and what now is called AUDIO_TRACK don't use the same id - implying strongly they're in the same stream. I haven't read (or grokked) the entirety of MediaStreamAudioDestinationNode or AudioNodeStream, so I'm unclear on the validity of this concern or the impact.

ehsan - can you comment about the WebAudio trackid stuff you wrote initially? (see comments above)

This is very weird, I can't think of why we ever had this extra track type either. Reading the code right now, it indeed seems unnecessary, as evidenced by removing it fixing this bug. :-)

(In reply to Randell Jesup [:jesup] from comment #20) > From the patches, ehsan was adding a second track with a different trackid > for output, and there are assertions to make sure that and what now is > called AUDIO_TRACK don't use the same id - implying strongly they're in the > same stream. I haven't read (or grokked) the entirety of > MediaStreamAudioDestinationNode or AudioNodeStream, so I'm unclear on the > validity of this concern or the impact. That's also the only reason I could come up with when working on this bug - that the tracks were in the same stream. But they weren't, and they didn't appear to have been either. Much confusion :)

Fixing the commit msg. Carries r=roc.

Per comment 17, it's now checking frequencies where input or output have a value of 200 or more (of 255). Carrying forward r=jesup,padenot

Try push for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ad4782c35b2 Some failures, so far unrelated to this bug.

Try is green as far as this bug goes. Please land the test last.

Needs rebasing.

Rebased. Compiled and passing the test locally.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/796e84a25f16 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6d897fc65c

Sorry, I had to back out for crashtests failures on (at least) Linux x64 and Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/22a0f682dfd8 Example of a failure: https://treeherder.mozilla.org/logviewer.html#?job_id=7203663&repo=mozilla-inbound

So since TrackUnionStream is processing input data now, instead of MediaStreamAudioDestinationNode (which processed its output data); we've exposed something existing I think. Simple to reproduce though, so that's promising.

This appears to happen when InMutedCycle() returns true. There's basically no input data but the input is not blocked so TrackUnionStream tries to copy. Since InMutedCycle() is a method of ProcessedMediaStream, I'd say TrackUnionStream needs to know how to handle it.

This handles InMutedCycle() in TrackUnionStream. Same way as AudioNodeStream does it. Try is happy now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01a564026f14

Comment on attachment 8573060 [details] [diff] [review] Handle InMutedCycle() in TrackUnionStream::ProcessInput Review of attachment 8573060 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/322d60fc413e https://hg.mozilla.org/integration/mozilla-inbound/rev/ff063b9a1ea2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b78fd38002f5

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/803e783be389 for frequent mochitest-e10s failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7337088&repo=mozilla-inbound

Also: https://treeherder.mozilla.org/logviewer.html#?job_id=7339538&repo=mozilla-inbound /test_peerConnection_webAudio.html | Sane audio frequency values at index 31/1024, input=224, output=47 - expected PASS /test_peerConnection_webAudio.html | Sane audio frequency values at index 32/1024, input=255, output=82 - expected PASS /test_peerConnection_webAudio.html | Sane audio frequency values at index 33/1024, input=255, output=82 - expected PASS /test_peerConnection_webAudio.html | Sane audio frequency values at index 34/1024, input=225, output=48 - expected PASS https://treeherder.mozilla.org/logviewer.html#?job_id=7340944&repo=mozilla-inbound | Sane audio frequency values at index 31/1024, input=224, output=95 - expected PASS | Sane audio frequency values at index 32/1024, input=255, output=130 - expected PASS | Sane audio frequency values at index 33/1024, input=255, output=130 - expected PASS | Sane audio frequency values at index 34/1024, input=225, output=96 - expected PASS

This might be caused by the fact that the AnalyserNode applies a Blackman windows on its input, and that there might be latency between the input and output. Can we try not window (analyser.smoothingTimeConstant = 0) ? We might as well dump the frequency data and check what's up? Or maybe there is the time-stretcher-on-under-run thing kicking in on the receiving side that messes up the data? That would not surprise me, since this is try on a probably overloaded machine.

(In reply to Paul Adenot (:padenot) from comment #39) > Or maybe there is the time-stretcher-on-under-run thing kicking in on the > receiving side that messes up the data? That would not surprise me, since > this is try on a probably overloaded machine. Quite possible, and impossible to avoid. A longer run should show some good data I imagine, but this is inherently timing/load sensitive.

Thanks for the suggestions guys. I did some testing on this yesterday after much the same conclusions. Here's a fine looking run with a 2 second setTimeout before comparing input and output, just to verify we're probably dealing with something overloaded: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c61cca524c63 Here's one without the blackman window (fails): https://treeherder.mozilla.org/#/jobs?repo=try&revision=043a21f5e611 Next I think I'll try one that waits for the output element to progress a bit (using "timeupdate") after all previous steps have finished (and load has decreased from the initial setup of things). Hopefully works without the intermittents, and at least gets rid of the setTimeout I had before.

Like so: https://treeherder.mozilla.org/#/jobs?repo=try&revision=819a0a54928f

Much better now, but still seeing a failure on that try run :| Randell, Paul, any ideas?

I had a brief discussion with Paul on IRC and will try to compare the global maxima in the frequency domain for both input and output. As long as they both occur around the same frequency we should be good.

Let's see how it holds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a028130fbdee

Is there any workaround for this? Any ETA? When will it hit Nightly?

We're in the process of landing this now. (We've tried twice, but we keep getting backed out for test bustage.) We expect to land this within the next day or so -- which means it should be in Nightly by the end of this week.

Now checking that the global maximum of both input's and output's frequency domain is at roughly the same place. It's also waiting for the output stream to progress 0.5 seconds before doing the check since we saw complete silence on the output a few times in the earlier runs. Here's a diff from the previous patch: https://hg.mozilla.org/try/rev/a9d7fb2e81ad

https://hg.mozilla.org/integration/mozilla-inbound/rev/87bd80f421e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ebf1da748d https://hg.mozilla.org/integration/mozilla-inbound/rev/05d937916cc8

https://hg.mozilla.org/mozilla-central/rev/87bd80f421e7 https://hg.mozilla.org/mozilla-central/rev/b9ebf1da748d https://hg.mozilla.org/mozilla-central/rev/05d937916cc8