Disable direct audio listeners for RTCPeerConnection with full duplex

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: MediaStreamGraph
P2
normal
Rank:
25
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Rank: 25
Priority: -- → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
I did a quick test of this (unoptimized) and it was fine. I'll do some more testing on my other machines at home when try has put up a couple of builds.
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Did some stress testing and observed delay buildups on the receiving end of a call :/
What was the setup ?
Flags: needinfo?(pehrson)
(Assignee)

Comment 5

a year ago
An appear.in room with two clients with this patch and six other clients just for cpu load.

Those two clients were on dual core macbook airs and the six others on a macbook pro. I was listening to audio from one of the airs to the other and first noticed some dropouts and then there was a huge delay.

I'll do some more testing in the same setup both with and without this patch.
Flags: needinfo?(pehrson)
(Assignee)

Comment 6

a year ago
So we concluded that there is latency locally, according to padenot in the cubeb resampler. I'm not 100% sure there's no extra latency in the peer connection but that would have to be tested.

On linux there's another issue where audio capture stops completely. achronop is looking into that.
(Assignee)

Comment 7

a year ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #6)
> So we concluded that there is latency locally, according to padenot in the
> cubeb resampler. I'm not 100% sure there's no extra latency in the peer
> connection but that would have to be tested.

I did some testing and I didn't notice any extra latency in the peer connection whatsoever.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #7)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #6)
> > So we concluded that there is latency locally, according to padenot in the
> > cubeb resampler. I'm not 100% sure there's no extra latency in the peer
> > connection but that would have to be tested.
> 
> I did some testing and I didn't notice any extra latency in the peer
> connection whatsoever.

I'm not worried about a few samples of latency in the resampler.

Paul: do you believe that audio data inserted into the graph by AppendToTrack() (with this patch) will be processed through the graph and get to the NotifyXxxxx() call that feeds the PeerConnection (MediaPipeline) in the same MSG iteration, assuming no fun-and-games with WebAudio/etc nodes?
Flags: needinfo?(padenot)
(Assignee)

Comment 9

a year ago
(In reply to Randell Jesup [:jesup] from comment #8)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #7)
> > (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #6)
> > > So we concluded that there is latency locally, according to padenot in the
> > > cubeb resampler. I'm not 100% sure there's no extra latency in the peer
> > > connection but that would have to be tested.
> > 
> > I did some testing and I didn't notice any extra latency in the peer
> > connection whatsoever.
> 
> I'm not worried about a few samples of latency in the resampler.

I'm not talking a few samples here. This went up to a couple of seconds and then reset and was snappy again. Paul told me it was the resampler but I don't know. It was on MacOS, with a Macbook Pro's internal microphone and TRS connected headphones.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> (In reply to Randell Jesup [:jesup] from comment #8)

> > I'm not worried about a few samples of latency in the resampler.
> 
> I'm not talking a few samples here. This went up to a couple of seconds and
> then reset and was snappy again. Paul told me it was the resampler but I
> don't know. It was on MacOS, with a Macbook Pro's internal microphone and
> TRS connected headphones.

Until full-duplex doesn't have drift problems, we can't disable direct listeners.  *Maybe* we can disable it for Linux since we haven't seen any indication of input/output drift on Linux.

Comment 11

a year ago
mozreview-review
Comment on attachment 8813294 [details]
Bug 1319445 - Don't use direct listener for audio in PeerConnection with full duplex.

https://reviewboard.mozilla.org/r/94742/#review97708

clearing review for now
Attachment #8813294 - Flags: review?(rjesup)
(Assignee)

Comment 12

a year ago
You mean that the delay I saw is not observed when using direct listeners?
Flags: needinfo?(rjesup)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #12)
> You mean that the delay I saw is not observed when using direct listeners?

Actually, that's a good point (I was tired when I wrote that) - if this is repeatable, we can check, but I suspect thinking about it that the delay isn't affected by directlisteners (or rather, directlisteners no longer do the primary job they were added for when full-duplex is in play.  

If this problem is repeatable, we could test with and without this patch.  However, this is the right way to go long-term, so in retrospect we probably should land this and then fix anything it breaks.
Flags: needinfo?(rjesup)

Comment 14

a year ago
mozreview-review
Comment on attachment 8813294 [details]
Bug 1319445 - Don't use direct listener for audio in PeerConnection with full duplex.

https://reviewboard.mozilla.org/r/94742/#review97746
Attachment #8813294 - Flags: review+
(Assignee)

Comment 15

11 months ago
I tried with full duplex and direct listeners - no improvement there.

Comment 16

11 months ago
mozreview-review
Comment on attachment 8813294 [details]
Bug 1319445 - Don't use direct listener for audio in PeerConnection with full duplex.

https://reviewboard.mozilla.org/r/94742/#review103876

Easy enough to back out if something crazy comes up.
Attachment #8813294 - Flags: review?(padenot) → review+

Comment 17

11 months ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3c51f1d45483
Don't use direct listener for audio in PeerConnection with full duplex. r=jesup,padenot
Backed out for timing out while checking for video stats in mda tests:

https://hg.mozilla.org/integration/autoland/rev/5bdd39969d244c0639f746350091f9bfeaba1ed7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3c51f1d45483bdb0e962ca7b6201a590e70c42f9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=67279666&repo=autoland

[task 2017-01-09T16:23:13.425306Z] 16:23:13     INFO - Should have RTP stats for track {63aa10cc-f6ed-4c6b-b827-e077586d6af4}
[task 2017-01-09T16:23:13.426015Z] 16:23:13     INFO - RTP stats: {"id":"outbound_rtp_video_1","timestamp":1483978992963.96,"type":"outboundrtp","bitrateMean":0,"bitrateStdDev":0,"framerateMean":0,"framerateStdDev":0,"isRemote":false,"mediaType":"video","remoteId":"outbound_rtcp_video_1","ssrc":"1076270288","bytesSent":0,"droppedFrames":0,"packetsSent":0}
[task 2017-01-09T16:23:13.428511Z] 16:23:13     INFO - Track {63aa10cc-f6ed-4c6b-b827-e077586d6af4} has 0 outboundrtp RTP packets.
[task 2017-01-09T16:23:13.429605Z] 16:23:13     INFO - Timeout checking for stats for track {63aa10cc-f6ed-4c6b-b827-e077586d6af4} after 30000ms
[task 2017-01-09T16:23:13.429683Z] 16:23:13     INFO - Buffered messages finished
[task 2017-01-09T16:23:13.430334Z] 16:23:13     INFO - TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html | Error in test execution: Timeout checking for stats for video track {63aa10cc-f6ed-4c6b-b827-e077586d6af4} after 30000ms 
[task 2017-01-09T16:23:13.430942Z] 16:23:13     INFO -     execute/<@dom/media/tests/mochitest/head.js:750:14
[task 2017-01-09T16:23:13.431050Z] 16:23:13     INFO -     Async*execute@dom/media/tests/mochitest/head.js:739:12
[task 2017-01-09T16:23:13.431672Z] 16:23:13     INFO -     promise callback*runNetworkTest/</<@dom/media/tests/mochitest/pc.js:1922:7
[task 2017-01-09T16:23:13.431754Z] 16:23:13     INFO -     runTestWhenReady/<@dom/media/tests/mochitest/head.js:370:41
[task 2017-01-09T16:23:13.432439Z] 16:23:13     INFO -     promise callback*runTestWhenReady@dom/media/tests/mochitest/head.js:370:10
[task 2017-01-09T16:23:13.433037Z] 16:23:13     INFO -     runNetworkTest/<@dom/media/tests/mochitest/pc.js:1921:5
[task 2017-01-09T16:23:13.433132Z] 16:23:13     INFO -     promise callback*runNetworkTest@dom/media/tests/mochitest/pc.js:1920:10
[task 2017-01-09T16:23:13.433758Z] 16:23:13     INFO -     @dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html:15:3
[task 2017-01-09T16:23:13.433832Z] 16:23:13     INFO - closeDataChannels called with index: 0
Flags: needinfo?(pehrson)
(Assignee)

Comment 19

11 months ago
I caused this issue by landing bug 1305949 in between...

New patch coming up to fix it.
Flags: needinfo?(pehrson)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

11 months ago
mozreview-review
Comment on attachment 8825672 [details]
Bug 1319445 - Switch to Add/RemoveVideoOutput for MediaPipelineTransmit with video.

https://reviewboard.mozilla.org/r/103774/#review104588

Glad you caught the FakeMediaStream bit ... want to see that *die* sometime...

::: media/webrtc/signaling/test/FakeMediaStreams.h:413
(Diff revision 1)
> +    return mIsVideo? this : nullptr;
> +  }
> +  Fake_MediaStreamTrack* AsAudioStreamTrack()
> +  {
> +    return mIsVideo? nullptr : this;

minor, minor nit: space before ?
Attachment #8825672 - Flags: review?(rjesup) → review+
(Assignee)

Comment 23

10 months ago
(In reply to Randell Jesup [:jesup] from comment #22)
> Comment on attachment 8825672 [details]
> Bug 1319445 - Switch to Add/RemoveVideoOutput for MediaPipelineTransmit with
> video.
> 
> https://reviewboard.mozilla.org/r/103774/#review104588
> 
> Glad you caught the FakeMediaStream bit ... want to see that *die*
> sometime...

Me too, and it won't compile everything in a `./mach build` without it :(
Comment hidden (mozreview-request)

Comment 25

10 months ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e7e9bc8280d2
Don't use direct listener for audio in PeerConnection with full duplex. r=jesup,padenot
https://hg.mozilla.org/integration/autoland/rev/0faa85ca0fcf
Switch to Add/RemoveVideoOutput for MediaPipelineTransmit with video. r=jesup

Comment 26

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7e9bc8280d2
https://hg.mozilla.org/mozilla-central/rev/0faa85ca0fcf
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Clearing NI.
Flags: needinfo?(padenot)
(Assignee)

Updated

a month ago
Depends on: 1406027
You need to log in before you can comment on or make changes to this bug.