Implement firing mute events when a RTCP BYE is received, or an RTCP timeout happens
Categories
(Core :: WebRTC: Audio/Video, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: dminor, Assigned: dminor)
References
(Depends on 1 open bug)
Details
Attachments
(9 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
+++ This bug was initially created as a clone of Bug #1545855 +++
A separate bug to implement firing the mute events.
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Hi Jan-Ivar, with these changes, I'm seeing one extra mute and unmute event for pc2's audio and video tracks in this test: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webrtc/RTCRtpTransceiver.https.html#1092. Is that expected behaviour with these changes or does that indicate a bug in my implementation? Thanks!
Assignee | ||
Comment 3•4 years ago
|
||
Just to clarify, I'm wondering if by adding a mute when an RTCP bye is received we'd expect to see an extra mute and unmute in the test I mentioned above. I had problems convincing myself one way or the other. Maybe Byron knows?
Comment 4•4 years ago
|
||
So, there may be a pre-existing bug you're tripping over here that I'm fixing as part of bug 1591199. (https://hg.mozilla.org/try/rev/d136056a174e32c100c9e88e4e68123707c2ce8e)
When exactly are you seeing the extra events?
Comment 5•4 years ago
|
||
I don't see transceiver.stop()
called or port 0 anywhere in that test, so I wouldn't expect an RTCP BYE, except on pc.close()
.
Assignee | ||
Comment 6•4 years ago
|
||
Thank you for having a look.
Part of my problem was that I was focusing on receiving the RTCP bye events. As soon as I looked at when we send an RTCP bye, things became much clearer. Basically, on the audio side, any call to SetLocalDescription or SetRemoteDescription can cause us to close a connection, the stacks look like:
#0 0x00007fffe973357c in webrtc::RTCPSender::SetSendingStatus(webrtc::RTCPSender::FeedbackState const&, bool) (this=0x7fffcd438018, feedback_state=..., sending=false)
at /home/dminor/src/firefox/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:238
#1 0x00007fffe9747975 in webrtc::ModuleRtpRtcpImpl::SetSendingStatus(bool) (this=0x7fffcd438000, sending=false)
at /home/dminor/src/firefox/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:406
#2 0x00007fffe97e2e6f in webrtc::voe::Channel::StopSend() (this=0x7fffd49c4800)
at /home/dminor/src/firefox/media/webrtc/trunk/webrtc/voice_engine/channel.cc:1155
#3 0x00007fffe97ed097 in webrtc::VoEBaseImpl::StopSend(int) (this=0x7fffd4910110, channel=<optimized out>)
at /home/dminor/src/firefox/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:285
#4 0x00007fffe95f9963 in webrtc::internal::AudioSendStream::Stop() (this=0x7fffcd705000)
at /home/dminor/src/firefox/media/webrtc/trunk/webrtc/audio/audio_send_stream.cc:259
#5 0x00007fffe666a89c in mozilla::WebrtcAudioConduit::StopTransmittingLocked() (this=0x7fffd49d9000)
at /home/dminor/src/firefox/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:797
#6 0x00007fffe666a7bd in mozilla::WebrtcAudioConduit::StopTransmitting() (this=0x7fffd49d9000)
at /home/dminor/src/firefox/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:765
#7 0x00007fffe668e99b in mozilla::MediaPipelineTransmit::Stop() (this=<optimized out>) at /home/dminor/src/firefox/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:862
#8 0x00007fffe66b8314 in mozilla::TransceiverImpl::UpdateConduit() (this=0x7fffce9da580)
at /home/dminor/src/firefox/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:193
#9 0x00007fffe66b3217 in mozilla::PeerConnectionMedia::UpdateMediaPipelines() (this=<optimized out>)
at /home/dminor/src/firefox/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:275
#10 0x00007fffe66b3a04 in mozilla::PeerConnectionImpl::SetSignalingState_m(mozilla::dom::RTCSignalingState, bool) (this=0x7fffce9ab000, aSignalingState=<optimized out>, rollback=false)
at /home/dminor/src/firefox/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2250
#11 0x00007fffe66b078d in mozilla::PeerConnectionImpl::SetRemoteDescription(int, char c
onst*) (this=<optimized out>, action=<optimized out>, aSDP=<optimized out>)
On the audio side we also end up stopping the stream when configuring the rtp extensions. On the video side, the stop occurs as a task on a task queue, but the code path from PeerConnectionImpl::SetRemoteDescription up to VideoSendStream::Stop() appears identical.
From this, it does appear that the extra mute/unmutes are expected with our current implementation because the tests call SetLocalDescription and SetRemoteDescription as part of their execution, and these will trigger RTCP byes to be sent.
I did try Byron's patch, and it definitely changed the behaviour, but I couldn't really say whether it was better or worse.
Part of the problem with this test is that it ends up being a bit racy with the addition of the new mutes. I either need to extend the current timeout from 100ms to something like 1000ms to give enough time for all the mutes and unmutes to come in, or keep the timeout and change the expectations to allow for >= rather than ==.
Looking at https://w3c.github.io/webrtc-pc/#mediastreamtrack, it does say that calling SetRemoteDescription can cause the track to be muted, so I think that part is ok. I'm not sure about calling SetLocalDescription.
Comment 7•4 years ago
|
||
Ok, that sounds broken. RTCP BYE should only be sent when the transceiver is stopped, not when a renegotiation happens (even if we transition to recvonly or inactive).
Does Chrome do this too?
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #7)
Ok, that sounds broken. RTCP BYE should only be sent when the transceiver is stopped, not when a renegotiation happens (even if we transition to recvonly or inactive).
Does Chrome do this too?
So comparing behaviour on just the checkMute subtest of RTCRtpTransceiver.https.html (with a few modifications to get the test running on Chrome) I see 8 RTCP byes generated by Chrome vs. 12 RTCP byes generated by Firefox. With the debugger attached, it appears that Chrome also generates byes in response to SetLocalDescription and SetRemoteDescription. Two of the extra byes generated by Firefox are due to the fact that we create an audio send stream and then tear it down when we register RTP extensions. That should be easy to eliminate. The other two may take some extra digging, I was getting harness failures when attempting to run a wpt test with the debugger attached today, so I can't say for certain what is causing them or how easy they would be to eliminate.
Assignee | ||
Comment 9•4 years ago
|
||
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37fdf94eb9c06202db06169920cd89142cc322f5
I've managed to get the number of byes on checkMute from 12 to 10, and overall on RTCRtpTransceiver.https.html the number of byes went from 80 something to 50 something with the patches here, so I think our behaviour is now roughly comparable to Chrome on this. That said, with the changes here, we're still going to expose some unpleasant behaviour that Chrome hides. The webrtc.org APIs are set up to recreate a stream if properties change rather than reconfigure it, so I'm not sure how easy it will be to improve things further. I think improving that should be part of a separate bug. I'm open to whether that is something that needs to be fixed prior to landing the stuff here, or if it is something that can be filed as a follow up.
Assignee | ||
Comment 10•4 years ago
|
||
Assignee | ||
Comment 11•4 years ago
|
||
Depends on D57858
Assignee | ||
Comment 12•4 years ago
|
||
Depends on D57859
Assignee | ||
Comment 13•4 years ago
|
||
This consolidates the shutdown code and ensures that an RTCP bye is sent
as part of tearing down a PeerConnection.
Depends on D57860
Assignee | ||
Comment 14•4 years ago
|
||
Depends on D57861
Assignee | ||
Comment 15•4 years ago
|
||
We don't need to start transmitting until the transceiver asks us to. Doing
this early leads to creating and tearing down an audio send stream
unnecessarily, generating extra RTCP byes. These changes make the behaviour
consistent with the VideoConduit version.
Depends on D57863
Assignee | ||
Comment 16•4 years ago
|
||
The mute event listener needs to be added to the chain earlier as the track
will already have muted after renegotiation occurs. We could remove the
check completely, but this will help ensure we don't regress firing mute events
when the track is stopped.
Depends on D57864
Assignee | ||
Comment 17•4 years ago
|
||
Depends on D57865
Assignee | ||
Comment 18•4 years ago
|
||
With the change to trigger mute events when a RTCP bye is received we expect
to see more mutes and unmutes in checkMute. However, Chrome and Firefox do not
send the same number of RTCP byes when running this code, and whether or not
a bye is received prior to the test finishing is racy even in local testing.
This changes some of the tests to use greater than equal to allow but not
require receiving extra byes.
Depends on D57866
Comment 19•4 years ago
|
||
Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e3a9c3ae15d4 Add RtcpEventObserver to RtcpReceiver; r=ng https://hg.mozilla.org/integration/autoland/rev/d39d39da251e Plumb RtcpEventObserver through Channel and VideoReceiveStream; r=ng https://hg.mozilla.org/integration/autoland/rev/f38f6323c5a1 Add RtcpEventObserver to Conduits and Transceiver; r=ng https://hg.mozilla.org/integration/autoland/rev/be270c85dc3f Call Stop as part of Transceiver shutdown; r=ng https://hg.mozilla.org/integration/autoland/rev/581724fb90b3 Remove unnecessary cast from UpdateConduitRtpExtmap; r=ng https://hg.mozilla.org/integration/autoland/rev/ea2af0037f39 Remove unnecessary StartTransmitting from ConfigureSendMediaCodec; r=ng https://hg.mozilla.org/integration/autoland/rev/808c92d7fdc1 Update remove then add track mochitests to handle earlier mute events; r=ng https://hg.mozilla.org/integration/autoland/rev/8c62d1b30028 Update RTCPeerConnection-remote-track-mute expectations; r=ng https://hg.mozilla.org/integration/autoland/rev/04ae5aa635e9 Use greater than equal in checkMute in RTCRtpTransceiver.https.html; r=ng
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20896 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
Comment 23•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3a9c3ae15d4
https://hg.mozilla.org/mozilla-central/rev/d39d39da251e
https://hg.mozilla.org/mozilla-central/rev/f38f6323c5a1
https://hg.mozilla.org/mozilla-central/rev/be270c85dc3f
https://hg.mozilla.org/mozilla-central/rev/581724fb90b3
https://hg.mozilla.org/mozilla-central/rev/ea2af0037f39
https://hg.mozilla.org/mozilla-central/rev/808c92d7fdc1
https://hg.mozilla.org/mozilla-central/rev/8c62d1b30028
https://hg.mozilla.org/mozilla-central/rev/04ae5aa635e9
Description
•