Closed Bug 1595479 Opened 2 years ago Closed 2 years ago

Implement firing mute events when a RTCP BYE is received, or an RTCP timeout happens

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Depends on 1 open bug)

Details

Attachments

(9 files)

+++ This bug was initially created as a clone of Bug #1545855 +++

A separate bug to implement firing the mute events.

Duplicate of this bug: 1545855
Type: defect → task

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!

Flags: needinfo?(jib)

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?

Flags: needinfo?(docfaraday)

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?

Flags: needinfo?(docfaraday)

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().

Flags: needinfo?(jib)

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.

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?

(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.

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.

Status: NEW → ASSIGNED

This consolidates the shutdown code and ensures that an RTCP bye is sent
as part of tearing down a PeerConnection.

Depends on D57860

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

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

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

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
Regressions: 1607205
You need to log in before you can comment on or make changes to this bug.