1608118 - sender.replaceTrack() horribly broken, flickers back and forth continuously

Status: VERIFIED FIXED

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

Description

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

sender.replaceTrack appears broken like out of a bad movie.

STRs:

1. Open https://jsfiddle.net/jib1/yukbn0ep/ and allow camera
2. Check ☑ scramble

Expected result:

• Video switches from camera (track 1) to consistent white noise (track 2)

Actual result:

• Tracks (Video and white noise) alternate rapidly, flickering back and forth forever. scramble switch stops working.

Tested on Mac and Windows.

Regression range:

8:34.06 INFO: Narrowed inbound regression window from [f02f44b9, 313e9635] (3 builds) to [7c1213a7, 313e9635] (2 builds) (~1 steps left)
8:34.06 INFO: No more inbound revisions, bisection finished.
8:34.06 INFO: Last good revision: 7c1213a74d2844a2da97228e4660ffd8711b9346
8:34.06 INFO: First bad revision: 313e96351327ad2a57182dc37743cedb1d8efa78
8:34.06 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7c1213a74d2844a2da97228e4660ffd8711b9346&tochange=313e96351327ad2a57182dc37743cedb1d8efa78

Since bug 1601034 got uplifted, this just went to release. 😞

Comment 1

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

[Tracking Requested - why for this release]: Video flickers during rudimentary WebRTC call that relies on sender.replaceTrack. Seems rudimentary, but haven't had time to find and observe a regression in the wild yet.

Oddly, https://whereby.com, for which bug 1601034 was landed to solve a problem, does not appear afflicted, so it is yet unclear how widely WebRTC sites will be affected by this.

There may perhaps be some combination of factors involved, like whether the tracks are enabled or disabled at the time of the replace perhaps?

Comment 2

[Byron Campen [:bwc]]

This is likely related to (and maybe even a dupe of) bug 1606875.

Comment 3

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

Note that neither track is disabled in this case.

Comment 4

[Mike Taylor [:miketaylr]]

Do we think someone has the cycles to fix this for a future 72 dot release?

Comment 6

[Byron Campen [:bwc]]

I could try, although this may be a problem in MediaTrackGraph, which I'm only passingly familiar with.

Comment 7

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

I've verified the problem also happens with two camera tracks https://jsfiddle.net/jib1/h05w2kgr/ (I thought maybe it was limited to cam vs. non-cam, given some of the commit messages in bug 1601034, but not so).

Comment 8

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

It's caused by https://phabricator.services.mozilla.com/D56619. Dan, Paul, any idea?

Comment 9

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

The commit of the patch in comment 8 only mentions audio tracks, but the code changes setTracks for both video and audio.

One option might be to try to restore the old code path for video, and leave audio as is, since I see no mention of testing video—tests probably pass because they look for new color, not absence of old color (guessing).

But solving it that way might be a bit messy from looking at it. Might be simpler if we can find what's wrong with the new path for video. Clearly, something is still attached to the old dom track, as it's basically sending both old and new track at once it seems. Hoping others more familiar with MTG can spot it.

Comment 10

[Alex Chronopoulos [:achronop]]

Checking the offending patch I believe the problem is that we do not remove the listeners from the old mDomTrack before we replace it. I did a quick patch to do that and it seems to work. I need to investigate further to be sure this is the right fix.

Comment 11

[Dan Minor [:dminor]]

Thanks for having a look Alex!

Comment 12

[Alex Chronopoulos [:achronop]]

I think I found the cause. The MediaPipelineTransmit::mListener is added in mDomTrack indirectly here. This is because it goes through 1, 2, 4 and finally 5.

The ForwardedInputTrack::SetInput() get all the direct listeners from itself (mSendTrack) and add them to the source (mDomeTrack::mTrack) when the MediaInput is set. On the other hand on ForwardedInputTrack::RemoveInput() those listeners are not removed. Thus, later on, when the track is replaced and the mSendPort is destroyed the mListener is not removed from the old track and it continues pushing data to the transmitter pipeline.

Removing the direct listeners in ForwardedInputTrack::RemoveInput(), in the same way that they are added fixes the problem. I'll push on try and test it more on Monday.

BTW this is a video only bug because it involves direct listeners which are not used in audio.

Comment 13

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c74357b61e1ce2a76eeb2625b9be39ee5f72d911

Comment 14

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

Thanks Alex! Test looks green and from rudimentary testing locally it seems to fix the problem. I'll put it up for review to Paul so we can get it on Nightly asap, so we can consider it for a dot release.

I do notice ForwardedInputTrack appears used in a couple other places. Are we confident this won't break any of those places? The first fix you showed me Friday in MediaPipeline.cpp seemed smaller and also seemed to work:

--- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -992,16 +992,17 @@ nsresult MediaPipelineTransmit::SetTrack
         gMediaPipelineLog, LogLevel::Debug,
         ("Reattaching pipeline to track %p track %s conduit type: %s",
          &aDomTrack, track_id.c_str(),
          (mConduit->type() == MediaSessionConduit::AUDIO ? "audio" : "video")));
   }

   if (mDomTrack) {
     mDomTrack->RemoveConsumer(mTrackConsumer);
+    mDomTrack->RemoveDirectListener(mListener);
   }

Which is the minimal and best fix for a dot-release do we think? I don't have a preference or great knowledge of this code, just wanted to raise the question. cc Paul

Comment 15

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

Attachment: Bug 1608118 - Remove direct listeners from the source in ForwardedInputTrack when MediaInput is removed r=padenot

Comment 16

[Alex Chronopoulos [:achronop]]

Thank you jib. I was planning to push for review once my testing was finished.

Comment 17

[Alex Chronopoulos [:achronop]]

I am done with my testing, the patch is already r+ed, I'll go on and push it.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96ec3a9ac5b8
Remove direct listeners from the source in ForwardedInputTrack when MediaInput is removed r=padenot

Comment 19

[Paul Adenot (:padenot)]

(clearing ni)

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/96ec3a9ac5b8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 22

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

Comment on attachment 9121718 [details]
Bug 1608118 - Remove direct listeners from the source in ForwardedInputTrack when MediaInput is removed r=padenot

Beta/Release Uplift Approval Request

• User impact if declined: Remote viewers in WebRTC calls may experience seizure-inducing flickering (between two video sources) that won't stop, when a website attempts to replace a video track currently being transmitted with another different video track (without renegotiating) e.g. when a user attempts to switch to another camera or to sharing their screen in place of their camera stream.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STRs in comment 0 and comment 7 (see comment 9 on limitations of our automated test).
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk since patch is small, well-contained and adds code to remove an input that should have been removed, but wasn't for video like it was for audio, because video uses "direct" listeners, a second class of listener plumbing. Now the code removes these "direct" listeners as well, not just the regular ones. These "direct" listeners are and were cleaned up everywhere else, just not in this recently introduced replaceTrack code optimization.
• String changes made/needed:

Comment 23

[Alex Chronopoulos [:achronop]]

Clearing NI

Comment 24

[Alin Ilea, Desktop QA]

Reproduced the initial issue using a Beta build 73.0b7 (Build id: 20200118195856).
Verified - Fixed in latest Nightly 74.0a1 (Build id: 20200121215203) using Windows 10 and Mac OS 10.15. Verified also with two camera tracks (comment 7).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121718 [details]
Bug 1608118 - Remove direct listeners from the source in ForwardedInputTrack when MediaInput is removed r=padenot

Fixes a very ugly WebRTC regression causing flickering video playback. Approved for 73.0b9.

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/677842171b39

Comment 27

[Alin Ilea, Desktop QA]

Verified - Fixed in latest Beta 73.0b9 (Build id: 20200122212931) using Windows 10 and Mac OS 10.15

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 9121718 [details]
Bug 1608118 - Remove direct listeners from the source in ForwardedInputTrack when MediaInput is removed r=padenot

Not planning another dot release for 72, so this will ship in 73.