Closed Bug 1319445 Opened 8 years ago Closed 7 years ago

Disable direct audio listeners for RTCPeerConnection with full duplex

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(2 files)

Bug 1319445 - Don't use direct listener for audio in PeerConnection with full duplex.
58 bytes, text/x-review-board-request
padenot
: review+
jesup
: review+
Details
Bug 1319445 - Switch to Add/RemoveVideoOutput for MediaPipelineTransmit with video.
59 bytes, text/x-review-board-request
jesup
: review+
Details 
      No description provided.
Rank: 25
Priority: -- → P2
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
Did some stress testing and observed delay buildups on the receiving end of a call :/
What was the setup ?
Flags: needinfo?(pehrson)
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)
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.
(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)
(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 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)
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 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+
I tried with full duplex and direct listeners - no improvement there.
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+
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)
I caused this issue by landing bug 1305949 in between...

New patch coming up to fix it.
Flags: needinfo?(pehrson)
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+
(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 :(
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
https://hg.mozilla.org/mozilla-central/rev/e7e9bc8280d2
https://hg.mozilla.org/mozilla-central/rev/0faa85ca0fcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Clearing NI.
Flags: needinfo?(padenot)
Depends on: 1406027
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: