1081819 - WebAudio data isn't transmitting over established peerConnection

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: pc_test_webaudio.html - test case

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.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Renamed to differentiate this from Bug 1070127.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 3

[Paul Adenot (:padenot)]

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.

Comment 4

[Maire Reavy [:mreavy]]

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!

Comment 5

[Andreas Pehrson [:pehrsons]]

Sure, I'll take a look.

Comment 6

[Andreas Pehrson [:pehrsons]]

(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 :)

Comment 7

[Andreas Pehrson [:pehrsons]]

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)

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Let the output stream itself process input data from MediaStreamAudioNodeStream

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.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Let the output stream itself process input data from MediaStreamAudioNodeStream

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).

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: 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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 12

[Paul Adenot (:padenot)]

(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 13

[Paul Adenot (:padenot)]

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 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

(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?

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for piping WebAudio in and out of PeerConnection

(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.

Comment 17

[Andreas Pehrson [:pehrsons]]

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

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 19

[Paul Adenot (:padenot)]

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!

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 22

[(no longer active)]

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.  :-)

Comment 23

[Andreas Pehrson [:pehrsons]]

(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 :)

Comment 24

[Andreas Pehrson [:pehrsons]]

Attachment: Let the output stream itself process input data from MediaStreamAudioDestinationNode::mStream (carries r=roc)

Fixing the commit msg. Carries r=roc.

Comment 25

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for piping WebAudio in and out of PeerConnection (carries r=jesup,padenot)

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

Comment 26

[Andreas Pehrson [:pehrsons]]

Try push for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ad4782c35b2

Some failures, so far unrelated to this bug.

Comment 27

[Andreas Pehrson [:pehrsons]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 29

[Andreas Pehrson [:pehrsons]]

Attachment: Let the output stream itself process input data from MediaStreamAudioDestinationNode::mStream (carries r=roc)

Rebased. Compiled and passing the test locally.

Comment 30

[Paul Adenot (:padenot)]

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

Comment 31

[Paul Adenot (:padenot)]

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

Comment 32

[Andreas Pehrson [:pehrsons]]

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.

Comment 33

[Andreas Pehrson [:pehrsons]]

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.

Comment 34

[Andreas Pehrson [:pehrsons]]

Attachment: Handle InMutedCycle() in TrackUnionStream::ProcessInput

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 35

[Paul Adenot (:padenot)]

Comment on attachment 8573060 [details] [diff] [review]
Handle InMutedCycle() in TrackUnionStream::ProcessInput

Review of attachment 8573060 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, yes.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 37

[Phil Ringnalda (:philor)]

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

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 39

[Paul Adenot (:padenot)]

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.

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

(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.

Comment 41

[Andreas Pehrson [:pehrsons]]

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.

Comment 42

[Andreas Pehrson [:pehrsons]]

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

Comment 43

[Andreas Pehrson [:pehrsons]]

Much better now, but still seeing a failure on that try run :|

Randell, Paul, any ideas?

Comment 44

[Andreas Pehrson [:pehrsons]]

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.

Comment 45

[Andreas Pehrson [:pehrsons]]

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

Comment 46

[manolo]

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

Comment 47

[Maire Reavy [:mreavy]]

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.

Comment 48

[Andreas Pehrson [:pehrsons]]

Attachment: Add mochitest for piping WebAudio in and out of PeerConnection

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

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 50

[Carsten Book [:Tomcat]]

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