RTCPeerConnection mutes remote track when it shouldn't during renegotiation
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox72 | --- | unaffected |
firefox73 | + | verified |
firefox74 | + | verified |
People
(Reporter: jib, Assigned: dminor)
References
(Regression)
Details
(Keywords: regression)
Attachments
(3 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
Bug 1607579 - Enable mute on RTCP bye for RTCPeerConnection-remote-track-mute.https.html.ini; r=bwc!
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
STRs:
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.
Reporter | ||
Comment 1•5 years ago
•
|
||
[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.
Reporter | ||
Comment 2•5 years ago
|
||
May even be observable as video flicker or audio stutter when renegotiation happens.
Assignee | ||
Comment 3•5 years ago
|
||
I think it makes sense to disable this with a pref for now.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
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
Assignee | ||
Comment 6•5 years ago
|
||
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
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Pushed by dminor@mozilla.com: 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21094 for changes under testing/web-platform/tests
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)
Assignee | ||
Comment 11•5 years ago
|
||
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ce10136f9d8
https://hg.mozilla.org/mozilla-central/rev/ed545193c066
https://hg.mozilla.org/mozilla-central/rev/47a473757e80
Upstream PR was closed without merging
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 17•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 18•5 years ago
|
||
bugherder uplift |
Comment 19•5 years ago
|
||
This fix has also been verified on Beta v73.0b3 on Windows 10 and Ubuntu 18.
Comment 20•5 years ago
|
||
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
Assignee | ||
Comment 21•5 years ago
|
||
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!
Upstream PR merged by jgraham
Updated•5 years ago
|
Assignee | ||
Comment 23•5 years ago
|
||
Thanks James!
Updated•5 years ago
|
Description
•