From 03e6cccc28d76f28c926ce7cadaeba2c6a6cacf4 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 15 Sep 2022 11:11:20 +0000 Subject: [PATCH] Revert "rtp sender: don't send BYE on deactivating streams" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit a22c2a0c581cbe3f612f7a7d9fb9840186cc1e06. Reason for revert: breaks upstream project Original change's description: > rtp sender: don't send BYE on deactivating streams > > as this breaks RTCP assumptions about SSRCs being no longer > active as defined in > https://www.rfc-editor.org/rfc/rfc3550#section-6.6 > > This should not be sent in reaction to temporarily disabling > a stream via RTCRtpParameters.active as this does not mean that > the participant is leaving the session as defined in > https://www.rfc-editor.org/rfc/rfc3550#section-6.3.7 > and does not indicate end of participation as defined in > https://www.rfc-editor.org/rfc/rfc3550#section-6.1 > which stipulates BYE should be the last packet sent from this SSRC. > > BUG=webrtc:11082 > > Change-Id: Ia5144857f85303643146b0759184f0f3f50b66e4 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/273348 > Reviewed-by: Harald Alvestrand > Commit-Queue: Philipp Hancke > Cr-Commit-Position: refs/heads/main@{#38059} Bug: webrtc:11082 Change-Id: Iaaff0c0d7bb857fe9ce78ebcc716f3c6f1bc5c4a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275640 Reviewed-by: Henrik Boström Reviewed-by: Tomas Gunnarsson Commit-Queue: Philipp Hancke Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#38097} --- call/rtp_video_sender.cc | 1 + modules/rtp_rtcp/source/rtcp_sender.cc | 19 +++++++++++++++++-- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 5 +++-- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 1 + modules/rtp_rtcp/source/rtp_rtcp_interface.h | 2 +- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 8e582ea942..0e4f8d8059 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -513,6 +513,7 @@ void RtpVideoSender::SetActiveModulesLocked( const bool was_active = rtp_module.Sending(); const bool should_be_active = active_modules[i]; + // Sends a kRtcpByeCode when going from true to false. rtp_module.SetSendingStatus(active_modules[i]); if (was_active && !should_be_active) { diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index 626c430a5d..7983371097 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -213,8 +213,23 @@ bool RTCPSender::Sending() const { void RTCPSender::SetSendingStatus(const FeedbackState& feedback_state, bool sending) { - MutexLock lock(&mutex_rtcp_sender_); - sending_ = sending; + bool sendRTCPBye = false; + { + MutexLock lock(&mutex_rtcp_sender_); + + if (method_ != RtcpMode::kOff) { + if (sending == false && sending_ == true) { + // Trigger RTCP bye + sendRTCPBye = true; + } + } + sending_ = sending; + } + if (sendRTCPBye) { + if (SendRTCP(feedback_state, kRtcpBye) != 0) { + RTC_LOG(LS_WARNING) << "Failed to send RTCP BYE"; + } + } } void RTCPSender::SetNonSenderRttMeasurement(bool enabled) { diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index f88aacb0f6..ae59dc5b0c 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -329,12 +329,13 @@ TEST_F(RtcpSenderTest, SendBye) { EXPECT_EQ(kSenderSsrc, parser()->bye()->sender_ssrc()); } -TEST_F(RtcpSenderTest, StopSendingDoesNotTriggersBye) { +TEST_F(RtcpSenderTest, StopSendingTriggersBye) { auto rtcp_sender = CreateRtcpSender(GetDefaultConfig()); rtcp_sender->SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender->SetSendingStatus(feedback_state(), true); rtcp_sender->SetSendingStatus(feedback_state(), false); - EXPECT_EQ(0, parser()->bye()->num_packets()); + EXPECT_EQ(1, parser()->bye()->num_packets()); + EXPECT_EQ(kSenderSsrc, parser()->bye()->sender_ssrc()); } TEST_F(RtcpSenderTest, SendFir) { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 191a2aa6ae..a3662f19d9 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -310,6 +310,7 @@ RTCPSender::FeedbackState ModuleRtpRtcpImpl::GetFeedbackState() { int32_t ModuleRtpRtcpImpl::SetSendingStatus(const bool sending) { if (rtcp_sender_.Sending() != sending) { + // Sends RTCP BYE when going from true to false rtcp_sender_.SetSendingStatus(GetFeedbackState(), sending); } return 0; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 8381b58d5f..8b1d11aa45 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -281,7 +281,7 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { // Returns the FlexFEC SSRC, if there is one. virtual absl::optional FlexfecSsrc() const = 0; - // Sets sending status. + // Sets sending status. Sends kRtcpByeCode when going from true to false. // Returns -1 on failure else 0. virtual int32_t SetSendingStatus(bool sending) = 0;