Closed Bug 1155089 Opened 5 years ago Closed 5 years ago

RTPSender.replaceTrack()ing a gUM audio track with a WebAudio track yields silence

Categories

(Core :: WebRTC: Audio/Video, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

()

Details

Attachments

(3 files, 1 obsolete file)

Steps to reproduce:

1. Open the jsfiddle in the URL field.
2. Click Start and approve the prompt, you should now hear your own microphone.
3. Click Flip.

Expected:
A 800Hz square tone through WebAudio should be played out.

Actual:
Silence.
I have so far tracked it to a TrackID mismatch in MediaPipelineTransmit::PipelineListener, see [0], but since flipping back to playing the gUM track also doesn't work, I suspect there might be other issues as well.

[0] https://hg.mozilla.org/mozilla-central/file/7c2a1eaed3ac/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#l901
/r/7279 - Bug 1155089 - Part 1. Reset |TrackID| for MediaPipelineTransmit::PipelineListener on replaceTrack(). r=bwc
/r/7281 - Bug 1155089 - Part 2. Break out WebAudio input/output checker to PeerConnectionTest. r=mt
/r/7283 - Bug 1155089 - Part 3. Test replacing with WebAudio track in track_peerConnection_replaceTrack.html. r=jib

Pull down these commits:

hg pull -r 51a590f76eed1e4b1da339e562c75da574392a3f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594636 - Flags: review?(martin.thomson)
Attachment #8594636 - Flags: review?(jib)
Attachment #8594636 - Flags: review?(docfaraday)
The solution to the silence in Part 1 is to dispatch a reset of the MediaStreamListener's registered track id. When replacing we first remove the listener from the old stream and then add it to the new stream. We need to reset the listener's track id in between (it keeps a track id in case there are multiple tracks of the same kind on the stream). Both RemoveListener() and AddListener() are async though so we have to reset the TrackID on the media graph thread.

Then, to make the test fail without the fix, I wired up the WebAudio checker from test_peerConnection_webAudio.html. I also broke that part out to PeerConnectionTest - and avoided some code duplication.
https://reviewboard.mozilla.org/r/7279/#review6047

There's probably some simplification we can do here, but it looks sane to me. I'm sad that this track id manipulation isn't done at all in unit-tests; if this can be done easily I think it would be worthwhile.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
(Diff revision 1)
> +    virtual void Run()

Should mark this override.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
(Diff revision 1)
> +  class Message : public ControlMessage {

If we're only ever going to set the track id to TRACK_INVALID, we can simplify this some, and rename to something like DispatchUnsetTrackId.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
(Diff revision 1)
> +  listener_->DispatchSetTrackId(stream_->GraphImpl(), TRACK_INVALID);

Would probably be useful to comment that setting to TRACK_INVALID causes the listener to set its track id based on the next NewData callback.
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

https://reviewboard.mozilla.org/r/7277/#review6067

::: dom/media/tests/mochitest/pc.js:1527
(Diff revision 1)
> +      return Promise.reject(new Error("No audio track on " + side + " side."));

`throw new Error(...)` is sufficient.

::: dom/media/tests/mochitest/pc.js:1521
(Diff revision 1)
> +    } else {
> +      return Promise.reject(new Error("Invalid first argument '" + side +
> +                                      "' to getByteFrequencyData(). " +
> +                                      "Expected 'local' or 'remote'."));
> +    }

I wouldn't bother with an else clause here.

::: dom/media/tests/mochitest/pc.js:1542
(Diff revision 1)
> +    return Promise.resolve(audioStream.data);

This is a purely synchronous function, don't use `Promise.resolve` and `Promise.reject`, just return and throw.

::: dom/media/tests/mochitest/pc.js:1535
(Diff revision 1)
> +    if (!audioStream.analyser) {
> +      audioStream.sourceNode = test.audioContext.createMediaStreamSource(audioStream);
> +      audioStream.analyser = test.audioContext.createAnalyser();
> +      audioStream.sourceNode.connect(audioStream.analyser);
> +      audioStream.data = new Uint8Array(audioStream.analyser.frequencyBinCount);
> +    }
> +    audioStream.analyser.getByteFrequencyData(audioStream.data);
> +    return Promise.resolve(audioStream.data);

Please create a class for this data rather than patching over the DOM object in this way.

::: dom/media/tests/mochitest/pc.js:1514
(Diff revision 1)
> +      streams = this._pc.getLocalStreams();
> +      audioTrack = this._pc.getSenders().map(s => s.track).find(isAudioTrack);
> +    } else if (side === 'remote') {
> +      streams = this._pc.getRemoteStreams();
> +      // getReceivers() is not implemented yet.
> +      var audioStream = streams.find(s => s.getTracks().some(isAudioTrack));
> +      audioTrack = audioStream.getTracks().find(isAudioTrack);

This is strangely asymmetric.  It also has a name shadowing problem on `audioStream`, which could be a problem.

I think that you should use the same logic for both sides, namely:

```
streams = this._pc.getXxxStreams();
audioStream = streams.find(hasAudioTrack);
audioTrack = audioStream.getTracks().find(isAudioTrack);
```

::: dom/media/tests/mochitest/pc.js:1560
(Diff revision 1)
> +      from.getByteFrequencyData('local')
> +        .then(data => inputMax = data.reduce(maxWithIndex, initial))
> +        .then(() => this.getByteFrequencyData('remote'))
> +        .then(data => outputMax = data.reduce(maxWithIndex, initial))
> +        .then(() => {

You've written this as asynchronous code, but it's purely synchronous.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:15
(Diff revision 1)
> -  function isSenderOfTrack(sender) {
> -    return sender.track == this;
> +  function isSenderOfKind(sender) {
> +    return sender.track.kind == this;
>    }

Please don't use `this` in a function without context.  Use explicit arguments.

I know that this is an established pattern, but it's a bad one.  It abuses the `thisArg` parameter of the array iterator functions in a non-obvious fashion.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:141
(Diff revision 1)
> +    ]);
> +    test.chain.append([

Remove these two lines: test.chain.append takes an array of things to add.

Replace with `], [`

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:146
(Diff revision 1)
> +    ]);
>      test.chain.append([

], [

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:859
(Diff revision 1)
> +#ifndef USE_FAKE_MEDIA_STREAMS

Can't you just use `if (graph) {` here?
Attachment #8594636 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/7283/#review6065

lgtm, mostly style nits and code-shortening ideas.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:19
(Diff revision 1)
> -  runNetworkTest(function () {
> -    test = new PeerConnectionTest();
> -    test.setMediaConstraints([{video: true}], [{video: true}]);
> +  function isTrackOfKind(track) {
> +    return track.kind == this;
> +  }

unused

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:32
(Diff revision 1)
> +    return pc.getLocalStreams()
> +      .every(stream => stream.getTracks()       // Every local stream,
> +      .some(track => pc.getSenders()            // should have some track,
> +      .some(sender => sender.track === track))) // that's being sent over |pc|.

Nit: Brackets accumulate on each line, so I'd expect each line to indent more than the previous or to the open-bracket, as some IDEs do.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:82
(Diff revision 1)
> +    var replacetestLocal = [ function PC_LOCAL_REPLACE_VIDEOTRACK(test) {
> +      return replacetest(test.pcLocal);
> +    } ];
> +    var replacetestRemote = [ function PC_REMOTE_REPLACE_VIDEOTRACK(test) {
> +      return replacetest(test.pcRemote);
>      } ];

I would inline these I think, and maybe number them when reused?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:100
(Diff revision 1)
> -        var oldstream = test.pcLocal._pc.getLocalStreams()[0];
> -        var oldtrack = oldstream.getVideoTracks()[0];
> -        var sender = test.pcLocal._pc.getSenders().find(isSenderOfTrack, oldtrack);
> +        var pc = test.pcLocal._pc;
> +        var sender = pc.getSenders().find(isSenderOfKind, "video");
> +        ok(sender, "should still have a sender of video");
> +        var oldstream = pc.getLocalStreams().find(isStreamOfSender, sender);
> +        var oldtrack = oldstream.getTracks().find(isTrackOfSender, sender);
>          return sender.replaceTrack(oldtrack) // same track

In hindsight I think all this can be replaced with:

    return sender.replaceTrack(sender.track)

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:40
(Diff revision 1)
> +    var sender = pc.getSenders().find(isSenderOfKind, "video");

inline

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:45
(Diff revision 1)
> +    var oldstream = pc.getLocalStreams().find(isStreamOfSender, sender);
> +    var oldtrack = oldstream.getTracks().find(isTrackOfSender, sender);

Inline in spite of reuse.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:62
(Diff revision 1)
> -          var stream = test.pcLocal._pc.getLocalStreams()[0];
> -          var track = stream.getVideoTracks()[0];
> +        var stream = pc.getLocalStreams().find(isStreamOfSender, sender);
> +        var track = stream.getTracks().find(isTrackOfSender, sender);

inline

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:113
(Diff revision 1)
> +        var sender = pc.getSenders().find(isSenderOfKind, "audio");

inline

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:116
(Diff revision 1)
> +        var oldStream = pc.getLocalStreams().find(isStreamOfSender, sender);
> +        var oldTrack = oldStream.getTracks().find(isTrackOfSender, sender);

inline

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:127
(Diff revision 1)
> +        var newstream = destNode.stream;
> +        var newtrack = newstream.getAudioTracks()[0];

inline unused newstream

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:34
(Diff revision 1)
> +      .some(track => pc.getSenders()            // should have some track,

Any reason this is not

    .every(track => pc.getSenders()  // should have all tracks,

?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:42
(Diff revision 1)
> +    ok(allLocalStreamsHaveSender(pc),
> +       "Shouldn't have any streams without a corresponding sender");

Note that get(Local|Remote)Streams() is deprecated and will be less relevant over time, so while it still serves as a good double-check for things, we should stop treating it as the holder of truth, to make sure we have a fair number of tests that stand independently of it when it goes away. For now I think this is a fine double-check.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:15
(Diff revision 1)
> -  function isSenderOfTrack(sender) {
> -    return sender.track == this;
> +  function isSenderOfKind(sender) {
> +    return sender.track.kind == this;
>    }

I wrote this bit-function before being hip to arrow functions, and it hardly seems worth its linespace anymore. Lets inline it along with all the new callbacks modeled on it.

JS > English.

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:76
(Diff revision 1)
> +    test.chain.removeAfter("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
> +    var flowtestRemote = test.chain.remove("PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT");
> +    var flowtestLocal = test.chain.remove("PC_LOCAL_CHECK_MEDIA_FLOW_PRESENT");

I assume PC_LOCAL_CHECK_MEDIA_FLOW_PRESENT comes before PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT or flowtestLocal will be undefined or null here? Maybe reorder these assignments to rely a tiny bit less on order?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:110
(Diff revision 1)
> +    test.chain.append([
> +      function PC_LOCAL_REPLACE_AUDIOTRACK_WEBAUDIO(test) {

We replace video twice because we had bugs replacing a replaced track. Do we want to do the same for webaudio, or do we think that's redundant?

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:113
(Diff revision 1)
> +        var sender = pc.getSenders().find(isSenderOfKind, "audio");
> +        ok(sender, "track has a sender");
> +        var oldSenderCount = pc.getSenders().length;
> +        var oldStream = pc.getLocalStreams().find(isStreamOfSender, sender);
> +        var oldTrack = oldStream.getTracks().find(isTrackOfSender, sender);

Any reason not to shorten this to:
    var oldTrack = sender.track;
?
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

Reviewboard!
Attachment #8594636 - Flags: review?(jib) → review+
https://reviewboard.mozilla.org/r/7279/#review6097

> If we're only ever going to set the track id to TRACK_INVALID, we can simplify this some, and rename to something like DispatchUnsetTrackId.

Sure, I assume it won't be very many other use cases.
https://reviewboard.mozilla.org/r/7279/#review6099

I'll try to put something in.

And click OK on the comment before publishing ;-)
https://reviewboard.mozilla.org/r/7283/#review6101

> Any reason this is not
> 
>     .every(track => pc.getSenders()  // should have all tracks,
> 
> ?

Yeah, imagine you're sending (and getting from getLocalStreams()) stream A [audio1, video1]. You now have two senders, say, senderAudio1, senderVideo1.

Then you get stream B [audio2] from somewhere, and do senderAudio1.replaceTrack(audio2), resulting in:
getLocalStreams() -> [stream A [audio1, video1], stream B [audio2]]
getSenders() -> [senderAudio2, senderVideo1]

Hence, there is no sender for audio1.

This test is to make sure that when we have replaced all tracks in a stream, that stream is no longer returned from getLocalStreams(). E.g., when we replace the video track on the remote peer (since it is only sending video, like the test was before my changes).

> Note that get(Local|Remote)Streams() is deprecated and will be less relevant over time, so while it still serves as a good double-check for things, we should stop treating it as the holder of truth, to make sure we have a fair number of tests that stand independently of it when it goes away. For now I think this is a fine double-check.

Ah, good to know!

I also noted that getReceivers() as of now only gives you an empty list back. Is there any other alternative to getRemoteStreams() truth-wise?

> I would inline these I think, and maybe number them when reused?

Sounds good, but then I'd really like to do the same thing for the flowtests as well. I'm assuming the templates for the flowtests won't be changing much so this should be fine. They're both oneliners.

> We replace video twice because we had bugs replacing a replaced track. Do we want to do the same for webaudio, or do we think that's redundant?

I'd say it's redundant. We have the same MediaPipelineTransmit class for both audio and video (as opposed to MediaPipelineReceive{Audio|Video}).

> Inline in spite of reuse.

I simplified this quite a bit. oldTrack is really sender.track (we're checking that it's in local streams after each replaceTrack()). There was also a check right after these lines that was redundant as it is also checked after replaceTrack().

> Any reason not to shorten this to:
>     var oldTrack = sender.track;
> ?

I didn't see the forest for all the trees, probably :)
https://reviewboard.mozilla.org/r/7283/#review6103

> I assume PC_LOCAL_CHECK_MEDIA_FLOW_PRESENT comes before PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT or flowtestLocal will be undefined or null here? Maybe reorder these assignments to rely a tiny bit less on order?

I should perhaps mention that these flow checks are not really doing anything but returning right away. They are just checking that the video elements had some flow at some point, not that they're still having it. I'd like to address that as a follow-up. As part of bug 1054706 perhaps.
https://reviewboard.mozilla.org/r/7277/#review6105

> This is strangely asymmetric.  It also has a name shadowing problem on `audioStream`, which could be a problem.
> 
> I think that you should use the same logic for both sides, namely:
> 
> ```
> streams = this._pc.getXxxStreams();
> audioStream = streams.find(hasAudioTrack);
> audioTrack = audioStream.getTracks().find(isAudioTrack);
> ```

It's not as easy as it sounds :-)

As jib mentioned in his review of part 3, getXxxStreams() are deprecated and getSenders()/getReceivers() are carrying the truth. It's not that getXxxStreams() are not carrying the truth any longer, but sometimes they carry a bit more than just the truth, like:

We run into problems with your proposal in case we e.g., have a stream with audio and video and replace only one of its tracks. getLocalStreams() will then return two streams, the original (containing the track we didn't replace and are still sending) and the new (containing the track we replaced the old one with, and are also sending). If we'd look up the first audio track in getLocalStreams() we'd get the old one which is no longer used.

I'd happily make it symmetric for the remote side by using getReceivers() but in its current state it always returns an empty list, so I can't.

Anyhow, this got refactored away when I moved things into a class. Still using getSenders() and getRemoteStreams() but at least it's more explicit now.

> Can't you just use `if (graph) {` here?

Not without including MediaStreamGraph.h (and with MSG comes the world, more or less). It's already macroed out in the same manner at the top of this file.

So, no :-)
(In reply to Byron Campen [:bwc] from comment #4)
> I'm sad that this track id manipulation isn't done at all in unit-tests;
> if this can be done easily I think it would be worthwhile.

I was going to take a look at the unit-tests but when I got them to run (after some 'otool' and 'install_name_tool' magic to tell them where libnss3.dylib, libmozglue.dylib and XUL lives - I'm on Mac) I got this error:
> [----------] 4 tests from MediaPipelineTest
> [ RUN      ] MediaPipelineTest.TestAudioSendNoMux
> 2015-04-21 07:48:45.034291 UTC - 240676864[10a31d5d0]: Assertion failure: mod != NULL, at pk11slot.c:1799
> Assertion failure: mod != NULL, at pk11slot.c:1799
> Redirecting call to abort() to mozalloc_abort
> 
> Hit MOZ_CRASH() at /Users/pehrsons/Comoyo/gecko-dev/memory/mozalloc/mozalloc_abort.cpp:33
> Segmentation fault: 11

Any idea what's up with that?
Flags: needinfo?(docfaraday)
This is the stack trace.

* thread #22: tid = 0x5fc9eb, 0x0000000109ee9301 libmozglue.dylib`mozalloc_abort(msg=<unavailable>) + 81 at mozalloc_abort.cpp:33, name = 'Socket Thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000109ee9301 libmozglue.dylib`mozalloc_abort(msg=<unavailable>) + 81 at mozalloc_abort.cpp:33
    frame #1: 0x0000000109ee9330 libmozglue.dylib`abort + 16 at mozalloc_abort.cpp:71
    frame #2: 0x00000001008b772d libnss3.dylib`PR_Assert(s=<unavailable>, file=<unavailable>, ln=<unavailable>) + 93 at prlog.c:559
    frame #3: 0x00000001006296bc libnss3.dylib`PK11_GetInternalSlot + 60 at pk11slot.c:1799
    frame #4: 0x00000001002d919a mediapipeline_unittest`mozilla::DtlsIdentity::Generate() + 42 at dtlsidentity.cpp:31
    frame #5: 0x00000001000059c4 mediapipeline_unittest`(anonymous namespace)::TransportInfo::Init(this=0x000000010a21e160, client=true) + 1364 at mediapipeline_unittest.cpp:81
    frame #6: 0x0000000100005196 mediapipeline_unittest`(anonymous namespace)::TestAgent::ConnectRtp((anonymous namespace)::TestAgent*, (anonymous namespace)::TestAgent*) [inlined] (anonymous namespace)::TransportInfo::InitAndConnect(client=0x000000010a21e160, server=0x000000010a21e0d0)::TransportInfo&, (anonymous namespace)::TransportInfo&) + 8 at mediapipeline_unittest.cpp:57
    frame #7: 0x000000010000518e mediapipeline_unittest`(anonymous namespace)::TestAgent::ConnectRtp(client=0x000000010a21e120, server=0x000000010a21e090)::TestAgent*, (anonymous namespace)::TestAgent*) + 30 at mediapipeline_unittest.cpp:141
    frame #8: 0x0000000100006212 mediapipeline_unittest`mozilla::runnable_args_nm_2<void (*)((anonymous namespace)::TestAgent*, (anonymous namespace)::TestAgent*), (anonymous namespace)::TestAgentReceive*, (anonymous namespace)::TestAgentSend*>::Run(this=<unavailable>) + 18 at runnable_utils_generated.h:161
    frame #9: 0x0000000100013429 mediapipeline_unittest`mozilla::SyncRunnable::Run(this=0x000000010a256f90) + 25 at SyncRunnable.h:75
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

/r/7279 - Bug 1155089 - Part 1. Reset |TrackID| for MediaPipelineTransmit::PipelineListener on replaceTrack(). r=bwc
/r/7281 - Bug 1155089 - Part 2. Break out WebAudio input/output checker to PeerConnectionTest. r=mt
/r/7283 - Bug 1155089 - Part 3. Test replacing with WebAudio track in track_peerConnection_replaceTrack.html. r=jib

Pull down these commits:

hg pull -r 62c49359ec5a0af3e03e133e4043c006797aaf2e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594636 - Flags: review?(martin.thomson)
Attachment #8594636 - Flags: review?(jib)
Attachment #8594636 - Flags: review+
https://reviewboard.mozilla.org/r/7283/#review6107

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:25
(Diff revisions 1 - 2)
> +    var sender = pc.getSenders().find(sn => sn.track.kind === "video");

Mozilla style guide [1] says to prefer == over ===, which is established in this file, so we should adhere to that (applies throughout). Advocates for === need to change the style guide first.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
Attachment #8594636 - Flags: review?(jib) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #13)
> (In reply to Byron Campen [:bwc] from comment #4)
> > I'm sad that this track id manipulation isn't done at all in unit-tests;
> > if this can be done easily I think it would be worthwhile.
> 
> I was going to take a look at the unit-tests but when I got them to run
> (after some 'otool' and 'install_name_tool' magic to tell them where
> libnss3.dylib, libmozglue.dylib and XUL lives - I'm on Mac) I got this error:
> > [----------] 4 tests from MediaPipelineTest
> > [ RUN      ] MediaPipelineTest.TestAudioSendNoMux
> > 2015-04-21 07:48:45.034291 UTC - 240676864[10a31d5d0]: Assertion failure: mod != NULL, at pk11slot.c:1799
> > Assertion failure: mod != NULL, at pk11slot.c:1799
> > Redirecting call to abort() to mozalloc_abort
> > 
> > Hit MOZ_CRASH() at /Users/pehrsons/Comoyo/gecko-dev/memory/mozalloc/mozalloc_abort.cpp:33
> > Segmentation fault: 11
> 
> Any idea what's up with that?

   You're probably running into MOZILLA_INTERNAL_API problems. If you're having to fiddle with the linker outside of the build you're gonna have a bad time.
Flags: needinfo?(docfaraday)
Attachment #8594636 - Flags: review?(docfaraday) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> I also noted that getReceivers() as of now only gives you an empty list
> back. Is there any other alternative to getRemoteStreams() truth-wise?

Not yet unfortunately. Need to add RtpReceiver.
https://reviewboard.mozilla.org/r/7281/#review6127

::: dom/media/tests/mochitest/pc.js:734
(Diff revision 2)
> +  this.audioContext = audioContext || new AudioContext();

This shouldn't be necessary in a constructor.

::: dom/media/tests/mochitest/pc.js:1548
(Diff revision 2)
> +    var inputAudioStream = from._pc.getLocalStreams()
> +      .find(s => s.getAudioTracks().some(t => from._pc.getSenders()
> +                                              .map(sn => sn.track).includes(t)));

I think that I'd extract `from._pc.getSenders().map(sn => sn.track)` into a temporary.  Two reasons: it's faster that way, and it's easier to read.

::: dom/media/tests/mochitest/pc.js:1562
(Diff revision 2)
> +    return new Promise((resolve, reject) => inputElem.ontimeupdate = () => {

Would it be clearer if the `ontimeupdate` event registration were handled by `AudioStreamAnalyser`?

::: dom/media/tests/mochitest/pc.js:1574
(Diff revision 2)
> +      // When the input and output maxima are within reasonable distance
> +      // from each other, we can be sure that the input tone has made it
> +      // through the peer connection.
> +      if (Math.abs(inputMax.index - outputMax.index) < 10) {

This continues to bother me.  This test assumes that there is no delay, or that it is less than 10ms.  I don't know what sort of timing information we expose, so this is probably unfixable, but I think that I would at least make this number larger, and maybe add a correction quantity within the Math.abs call.
https://reviewboard.mozilla.org/r/7283/#review6129

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:86
(Diff revision 2)
> +    ]);
> +
> +    test.chain.append([

], [

::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:112
(Diff revision 2)
> +        // We need a frequency not too close to the fake audio track.

What frequency is that?
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

reviewboard !
Attachment #8594636 - Flags: review?(martin.thomson) → review+
https://reviewboard.mozilla.org/r/7281/#review6187

> I think that I'd extract `from._pc.getSenders().map(sn => sn.track)` into a temporary.  Two reasons: it's faster that way, and it's easier to read.

Done.

> Would it be clearer if the `ontimeupdate` event registration were handled by `AudioStreamAnalyser`?

I like the separation of concerns when the `AudioStreamAnalyser` doesn't know about other `AudioStreamAnalyser`s or `MediaElement`s.

> This shouldn't be necessary in a constructor.

I'll make it mandatory.

> This continues to bother me.  This test assumes that there is no delay, or that it is less than 10ms.  I don't know what sort of timing information we expose, so this is probably unfixable, but I think that I would at least make this number larger, and maybe add a correction quantity within the Math.abs call.

I'm not sure I follow you here. Where does the delay come into play?

We're comparing the indexes with the largest value in the frequency domain repeatedly until they match. In an ideal case (i.e., on the local side, where the tone was generated) there would be one index with value 255 and 1023 indexes with value 0. This eventually (can take a couple of "timeupdate" iterations on slow devices) propagates to the output and the test passes. The frequency on the output might not always be peaking at the same index as the input due to codecs and such, but it's been well within the 10 indexes tolerance for the cases I've seen (it was one off on Android iirc).

I renamed `checkReceivingAudioFrom` to `checkReceivingToneFrom` to make it more clear that the intended use is with pure tones.
https://reviewboard.mozilla.org/r/7281/#review6189

> I'll make it mandatory.

Published too soon. I changed my mind and made the constructor create an `AudioContext`. This means there will be _some_ contexts all alive at the same time, but that shouldn't have too much impact.
https://reviewboard.mozilla.org/r/7283/#review6191

> Mozilla style guide [1] says to prefer == over ===, which is established in this file, so we should adhere to that (applies throughout). Advocates for === need to change the style guide first.
> 
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

Right, that's just me not having read the style guide well enough. Thanks!
https://reviewboard.mozilla.org/r/7283/#review6193

> ], [

I'd like to keep them apart to make it clear where the comment above the previous block stops applying.

> What frequency is that?

1kHz, see https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineDefault.cpp#282

I added it to the comment to make it clear what my intention was in case anything changes.
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

/r/7279 - Bug 1155089 - Part 1. Reset |TrackID| for MediaPipelineTransmit::PipelineListener on replaceTrack(). r=bwc
/r/7281 - Bug 1155089 - Part 2. Break out WebAudio input/output checker to PeerConnectionTest. r=mt
/r/7283 - Bug 1155089 - Part 3. Test replacing with WebAudio track in track_peerConnection_replaceTrack.html. r=jib

Pull down these commits:

hg pull -r 8bfccf3d5ee8f2616505f0a564e460abc4433b1a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594636 - Flags: review+
Comment on attachment 8594636 [details]
MozReview Request: bz://1155089/pehrsons

Nits addressed. Carrying forward r=bwc for Part 1, r=mt for part 2, r=jib for part 3.
Attachment #8594636 - Flags: review+
Depends on: 1157701
Attachment #8594636 - Attachment is obsolete: true
Attachment #8620058 - Flags: review+
Attachment #8620059 - Flags: review+
Attachment #8620060 - Flags: review+
You need to log in before you can comment on or make changes to this bug.