1607579 - RTCPeerConnection mutes remote track when it shouldn't during renegotiation

Status: VERIFIED FIXED

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

Description

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

STRs:

1. Open https://jsfiddle.net/jib1/08ogn17j/

Expected results (Chrome, if it didn't fire unmute too fast, an unrelated bug):

stable sendrecv
unmute
Renegotiating:
Calling pc2.SLD
Calling pc1.SRD
stable sendrecv

Actual results (Firefox):

stable sendrecv
unmute
Renegotiating:
Calling pc2.SLD
mute
Calling pc1.SRD
stable sendrecv
unmute

A one second delay between methods in the script shows the mute on pc1 comes from pc2.SLD.

Comment 1

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

[Tracking Requested - why for this release]: Unfortunate regression in behavior (firing the mute + unmute events on tracks when it shouldn't).

This is caused by old bug 1232234, but is escalated by bug 1595479 exposing RTCP BYE to JS.

Let's keep the issues separate until we've decided how to resolve the behavior regression: probably by disabling (with a pref) the new bug 1595479 behavior until bug 1232234 can be fixed.

Comment 2

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

May even be observable as video flicker or audio stutter when renegotiation happens.

Comment 3

[Dan Minor [:dminor]]

I think it makes sense to disable this with a pref for now.

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1607579 - Add pref to disable mute on RTCP bye or timeout; r=bwc!

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1607579 - Enable mute on RTCP bye for RTCPeerConnection-remote-track-mute.https.html.ini; r=bwc!

Enabling the pref here should help us catch regressions to the RTCP bye / timeout
mute code even though we leave it disabled overall.

Depends on D59163

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1607579 - Revert changes to test expectations in RTCRtpTransceiver.https.html; r=bwc!

Rather than change the expectations here I should have marked Firefox as failing
the checkMute tests as the behaviour is at least partly incorrect.

Depends on D59164

Comment 7

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d041f57900e63cc49997493cd4809cbdbef7fcd

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ce10136f9d8
Add pref to disable mute on RTCP bye or timeout; r=bwc
https://hg.mozilla.org/integration/autoland/rev/ed545193c066
Enable mute on RTCP bye for RTCPeerConnection-remote-track-mute.https.html.ini; r=bwc
https://hg.mozilla.org/integration/autoland/rev/47a473757e80
Revert changes to test expectations in RTCRtpTransceiver.https.html; r=bwc

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21094 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/21094
* Community-TC (pull_request) (https://community-tc.services.mozilla.com/tasks/groups/b5V9TMqtSumtpnL7Su8Rpg)

Comment 11

[Dan Minor [:dminor]]

Looks like it is intermittently failing in "chao_mode_flags=3":

:17.02 INFO | Test | Subtest | Results | Messages |
3:17.02 INFO |----------------------------------------|-------------|----------------------------|------------------------------------------------------------------------------|
3:17.02 INFO | /webrtc/RTCRtpTransceiver.https.html | checkMute | FAIL: 6/10, PASS: 4/10 | assert_equals: Got 1 mute event for pc2's audio track expected 2 but got 1 |
3:17.02 INFO
3:17.02 INFO ::: Running tests in a loop 10 times : PASS
3:17.02 INFO ::: Running tests in a loop with restarts 5 times : PASS
3:17.02 INFO ::: Running tests in a loop 10 times with flags chaos_mode_flags=3 : FAIL
3:17.02 INFO :::
3:17.02 ERROR ::: Test verification FAIL
3:17.02 INFO :::
[taskcluster 2020-01-08 20:26:44.471Z] === Task Finished ===
[taskcluster 2020-01-08 20:26:44.627Z] Unsuccessful task run with exit code: 1 completed in 275.005 seconds

I'll have a look, but I wonder if this is the first time this test has gone through verification in "chaos mode".

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3ce10136f9d8
https://hg.mozilla.org/mozilla-central/rev/ed545193c066
https://hg.mozilla.org/mozilla-central/rev/47a473757e80

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 14

[Daniel Bodea [:danibodea]]

This issue was reproduced in Beta v73.0b2 and the fix was verified on Nightly v74.0a1 from 2020-01-09 on Windows 10 and Mac OS 10.15.

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 9119475 [details]
Bug 1607579 - Add pref to disable mute on RTCP bye or timeout; r=bwc!

Beta/Release Uplift Approval Request

• User impact if declined: Potentially causes video flickering or audio stutter on calls during renegotiation.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very low, this just prefs off a recent change to restore previous behaviour.
• String changes made/needed: None

Comment 16

[Dan Minor [:dminor]]

To follow up on Comment 11, the problem with the upstream wpt merge was in https://hg.mozilla.org/mozilla-central/rev/47a473757e80 which was an attempt to revert the tests back to the way they were before Bug 1595479. With the QA verification in Comment 14 and my local testing we're not generating extra BYEs, so I think the problem was latent in the test and not with anything in these patches. I don't think it's worth fighting with the upstream bot to try and get the changes here merged upstream.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119475 [details]
Bug 1607579 - Add pref to disable mute on RTCP bye or timeout; r=bwc!

Prefs off a recent change to avoid video flickering or audio stutter on calls during renegotiation. Approved for 73.0b3.

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e5b7a328de9
https://hg.mozilla.org/releases/mozilla-beta/rev/bb99637f5d7e
https://hg.mozilla.org/releases/mozilla-beta/rev/1008d06118f1

Comment 19

[Daniel Bodea [:danibodea]]

This fix has also been verified on Beta v73.0b3 on Windows 10 and Ubuntu 18.

Comment 20

[James Graham [:jgraham]]

Just ignoring the PR and allowing gecko's wpt and upstream to diverge isn't a good idea :) Unless the changes here are reverted in some other bug it sounds like the correct option is for me to admin-merge the PR? I didn't fully understand Comment #16

Comment 21

[Dan Minor [:dminor]]

If you can admin-merge, that would be great, the intention was to revert the changes I had made in another bug and so should not be making anything worse than they were before. The changes I made in the other bug loosened some test expectations, the revert tightened them back up. I had thought that the changes I made here would just be overwritten the next time we merged from upstream, which would also be acceptable. No divergence intended :) Thanks!

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham

Comment 23

[Dan Minor [:dminor]]

Thanks James!