1601034 - whereby.com results in audio video drift if you update the settings

Status: VERIFIED FIXED

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

Description

[Alex Chronopoulos [:achronop]]

STR:
Connect to a room in the new whereby.com (mozilla.whereby.com).
Press the settings.
Change the microphone (assuming you have more than one)
Press "Apply Changes"
On the new gUM prompt press "Allow"
The remote party reports A/V delay, between 1 and 2 seconds.

I am testing on Linux but I don't know if that affects

Comment 1

[Andreas Pehrson [:pehrsons]]

Does the remote party report audio delay or video delay?

Comment 2

[Andreas Pehrson [:pehrsons]]

I have a hypothesis on what's happening, and if it holds it means that there is a video delay, and that this delay is roughly proportional to the time the user takes to approve the new gUM prompt.

Comment 3

[Andreas Pehrson [:pehrsons]]

We've observed both audio delays and video delays. Unclear exactly how this happens. But it smells to me like all tracks are stopped, new tracks get acquired through gUM (after user approval, can take a loooong time!), then tracks get replaced with sender.replaceTrack() and sending continues.

After this has finished it looks like the remote side's jitter buffers are very confused.

Could it be because we keep sending a video frame every second whereas we don't send audio at all, while blocked on the gUM prompt?

Byron, do you know how we handle/should handle A/V sync when something like this happens?

Comment 4

[Alex Chronopoulos [:achronop]]

I have been experimenting with this fiddle. It reproduces the error, which is sporadic. Also, the delay varies between tries.

The time to approve the second gUM affects but in an odd way. If the approve happens fast the problem occurs. If the approve happens after some time (10 secs) there is no error, the receiving stream is fine and A/V is in sync.

Comment 5

[Byron Campen [:bwc]]

No, that stuff all happens down in the webrtc.org code.

Comment 6

[Dan Minor [:dminor]]

Sorry, I'm a bit behind. I'll have a look at this later today or early next week.

Comment 7

[Andreas Pehrson [:pehrsons]]

I'm not entirely clear how Whereby do their switches yet, but assuming that they replaceTrack() an ended track with a live track I think I can fix this.

We can make each active RTCRtpSender hold a ForwardedInputTrack that doesn't end until the sender goes away for sure. Thus if the input track ends the ForwardedInputTrack is still producing silence for the peer connection's rtp stream.

Same concept as we did for DecodedStream in bug 1172394, and similar to bug 1586370 in that we run the graph even though no audio is being processed by it.

I have some WIP patches that seem to result in a massive improvement. Just as the new devices start flowing to the remote side, there's some video glitches visible there, but they recover fast. This is probably an artifact of the video jitter buffer, since we didn't send anything between stopping the old and acquiring the new camera. If whereby wouldn't stop the old camera until they acquire the new, this shouldn't be an issue.

Comment 8

[Andreas Pehrson [:pehrsons]]

I forked Alex' fiddle for a corner case I found. If we just stop and replace audio, without touching video, that seems to result in silence on Nightly. I've fixed this.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1601034 - Add a ProcessedMediaTrack layer in MediaPipelineTransmit to handle replaceTrack of ended tracks. r?dminor!,padenot!

Before this patch, if a send audio MediaStreamTrack ended, we ended up not
sending anything over the network. If replaceTrack() at that point replaced the
ended track with a live one, we'd start sending data again, but the rtp stream
would continue from where the previous track ended.

Having a gap in audio like that would confuse a receiver's video jitter
buffer, because it's trying to sync to an audio track that just had a massive
amount of "jitter" (it can't tell the difference).

This patch fixes this by adding a track layer in MediaPipelineTransmit that
remains active for as long as the MediaPipeline is active. Thus if the send
audio MediaStreamTrack ends, we continue sending silence over the network, which
the receiver can understand. If later replaced, the receiver sees real audio
instead of silence and continues gracefully.

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1601034 - Only update the conduit in replaceTrack() if the track source changed between camera and non-camera. r?bwc

Depends on D56619

Comment 11

[Andreas Pehrson [:pehrsons]]

I'm also testing with this fiddle that does sender.replaceTrack(null). Seems to reproduce in release 71 as well.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e17753103d27
Add a ProcessedMediaTrack layer in MediaPipelineTransmit to handle replaceTrack of ended tracks. r=dminor,padenot
https://hg.mozilla.org/integration/autoland/rev/6a429cfa6440
Only update the conduit in replaceTrack() if the track source changed between camera and non-camera. r=bwc

Comment 13

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1601034) for causing gtest mediapipeline_unittest.cpp failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=280900020&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&searchStr=linux%2Cx64%2Cdebug%2Ctest-linux64%2Fdebug-gtest-1proc%2C%28gtest%29&revision=6a429cfa6440712c2cc878bcdd80e93ad3032444

backout: https://hg.mozilla.org/integration/autoland/rev/454e0c260c29fe241f5bbabf4e5e0b4a55e56890

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1601034 - Set the MediaPipelineTransmit send track in a dedicated method to enable unit tests. r?bwc

Depends on D56620

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1601034 - Do not block main thread in mediapipeline_unittest.cpp. r?bwc

The STS thread dispatches SyncRunnables to main thread. There are two cases of
blocking affected here:

• SyncRunnables from main to STS:
  Can cause a deadlock.
• PR_Sleep on the main thread:
  Effectively sleeps STS too, if STS dispatches a SyncRunnable to main during
  main's sleep.

This patch gets rid of both of these.

Depends on D57001

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98212026404b
Add a ProcessedMediaTrack layer in MediaPipelineTransmit to handle replaceTrack of ended tracks. r=dminor,padenot
https://hg.mozilla.org/integration/autoland/rev/902cf0de82c3
Only update the conduit in replaceTrack() if the track source changed between camera and non-camera. r=bwc
https://hg.mozilla.org/integration/autoland/rev/715477743439
Set the MediaPipelineTransmit send track in a dedicated method to enable unit tests. r=bwc
https://hg.mozilla.org/integration/autoland/rev/313e96351327
Do not block main thread in mediapipeline_unittest.cpp. r=bwc

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/98212026404b
https://hg.mozilla.org/mozilla-central/rev/902cf0de82c3
https://hg.mozilla.org/mozilla-central/rev/715477743439
https://hg.mozilla.org/mozilla-central/rev/313e96351327

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9114985 [details]
Bug 1601034 - Add a ProcessedMediaTrack layer in MediaPipelineTransmit to handle replaceTrack of ended tracks. r?dminor!,padenot!

Beta/Release Uplift Approval Request

• User impact if declined: Users using webrtc services may appear with video a few seconds behind audio, on the other participants' screen.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0. Any whereby room should work now as they've rolled the new interface out everywhere (no mozilla subdomain needed).
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This mainly shuffles some things on main thread around. The biggest risk is missing some event on the newly introduced MediaTrack that was only signaled on the old one, but I can't think of any more than the one (enabled/disabled) fixed in this patch.
• String changes made/needed:

Comment 19

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

I can't confirm the fix on my end - on Ubuntu 18.04 and Nightly 73.0a1 (20191216214733). I see a delay (audio and video), right after the microphone setting is done, but with no noticeable difference between Firefox 72 beta 7 and the latest Nightly 73.0a1 (also, after a short period of time video and audio get in sync)

Alex, please can you confirm whether this issue is still reproducible on the latest Nightly 73.0a1?

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 9114985 [details]
Bug 1601034 - Add a ProcessedMediaTrack layer in MediaPipelineTransmit to handle replaceTrack of ended tracks. r?dminor!,padenot!

webrtc fix, approved for 72.0b8

Comment 21

[Andreas Pehrson [:pehrsons]]

Ok I just did some testing myself and I think bug 1586370 is playing some tricks on us (same symptoms, and it kicks in with the STR here).

Try this STR instead, it should rule out bug 1586370 by keeping the mic we are changing to on when the switch happens:

1. Pick a whereby room to use, from here on called "the room"
2. Make sure you have two microphones on your system, from here on called Mic A and Mic B
3. Join the room with a separate machine, (or another browser instance on the same machine) -- mute camera and mic
4. Go to https://mozilla.github.io/webrtc-landing/gum_test.html, click "Microphone", choose Mic A, mute the audio element
5. Go to the room, make sure to select Mic B and any camera
6. Continue the original STR:
   Press the settings.
   Change the microphone (assuming you have more than one)
   Press "Apply Changes"
   On the new gUM prompt press "Allow"
   The remote party reports A/V delay, between 1 and 2 seconds.

Expected: Audio and video stays in sync, even after waiting for some time

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/31b6c779b3f1
https://hg.mozilla.org/releases/mozilla-beta/rev/7d968d4de8f8
https://hg.mozilla.org/releases/mozilla-beta/rev/6233ac57b4d1
https://hg.mozilla.org/releases/mozilla-beta/rev/50a4683d2a68

Comment 23

[Simona Badau, Desktop QA]

Thanks Andreas! With the provided steps, I was able to reproduce the initial issue. I can now confirm that audio and video stay in sync on the latest Nightly 73.0a1 and on Firefox 72 beta 8 - verified as fixed on Windows 10 x64 and Ubuntu 18.04.

Comment 24

[Alex Chronopoulos [:achronop]]

This is now verified. Also, I have verified the fiddle from comment 4. I am clearing the NI.