diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index d79e199e8c..9eda2a076e 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -154,7 +154,6 @@ RTCPSender::RTCPSender( transport_(outgoing_transport), using_nack_(false), sending_(false), - remb_enabled_(false), next_time_to_send_rtcp_(0), timestamp_offset_(0), last_rtp_timestamp_(0), @@ -237,33 +236,23 @@ int32_t RTCPSender::SetSendingStatus(const FeedbackState& feedback_state, return 0; } -bool RTCPSender::REMB() const { - rtc::CritScope lock(&critical_section_rtcp_sender_); - return remb_enabled_; -} - -void RTCPSender::SetREMBStatus(bool enable) { - rtc::CritScope lock(&critical_section_rtcp_sender_); - remb_enabled_ = enable; - if (!enable) { - // Stop sending remb each report until it is reenabled and remb data set. - ConsumeFlag(kRtcpRemb, true); - } -} - -void RTCPSender::SetREMBData(uint32_t bitrate, - const std::vector& ssrcs) { +void RTCPSender::SetRemb(uint32_t bitrate, const std::vector& ssrcs) { rtc::CritScope lock(&critical_section_rtcp_sender_); remb_bitrate_ = bitrate; remb_ssrcs_ = ssrcs; - if (remb_enabled_) - SetFlag(kRtcpRemb, false); + SetFlag(kRtcpRemb, /*is_volatile=*/false); // Send a REMB immediately if we have a new REMB. The frequency of REMBs is // throttled by the caller. next_time_to_send_rtcp_ = clock_->TimeInMilliseconds(); } +void RTCPSender::UnsetRemb() { + rtc::CritScope lock(&critical_section_rtcp_sender_); + // Stop sending REMB each report until it is reenabled and REMB data set. + ConsumeFlag(kRtcpRemb, /*forced=*/true); +} + bool RTCPSender::TMMBR() const { rtc::CritScope lock(&critical_section_rtcp_sender_); return IsFlagPresent(RTCPPacketType::kRtcpTmmbr); diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 69a0de5f00..3896559219 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -120,11 +120,9 @@ class RTCPSender { int32_t nackSize = 0, const uint16_t* nackList = 0); - bool REMB() const; + void SetRemb(uint32_t bitrate, const std::vector& ssrcs); - void SetREMBStatus(bool enable); - - void SetREMBData(uint32_t bitrate, const std::vector& ssrcs); + void UnsetRemb(); bool TMMBR() const; @@ -199,7 +197,6 @@ class RTCPSender { rtc::CriticalSection critical_section_rtcp_sender_; bool using_nack_ RTC_GUARDED_BY(critical_section_rtcp_sender_); bool sending_ RTC_GUARDED_BY(critical_section_rtcp_sender_); - bool remb_enabled_ RTC_GUARDED_BY(critical_section_rtcp_sender_); int64_t next_time_to_send_rtcp_ RTC_GUARDED_BY(critical_section_rtcp_sender_); diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 589db34565..9ba7689425 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -497,44 +497,39 @@ TEST_F(RtcpSenderTest, SendNack) { EXPECT_THAT(parser()->nack()->packet_ids(), ElementsAre(0, 1, 16)); } -TEST_F(RtcpSenderTest, RembStatus) { +TEST_F(RtcpSenderTest, RembNotIncludedBeforeSet) { + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + + rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr); + + ASSERT_EQ(1, parser()->receiver_report()->num_packets()); + EXPECT_EQ(0, parser()->remb()->num_packets()); +} + +TEST_F(RtcpSenderTest, RembNotIncludedAfterUnset) { const uint64_t kBitrate = 261011; const std::vector kSsrcs = {kRemoteSsrc, kRemoteSsrc + 1}; rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); - - EXPECT_FALSE(rtcp_sender_->REMB()); + rtcp_sender_->SetRemb(kBitrate, kSsrcs); rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr); ASSERT_EQ(1, parser()->receiver_report()->num_packets()); - EXPECT_EQ(0, parser()->remb()->num_packets()); + EXPECT_EQ(1, parser()->remb()->num_packets()); - rtcp_sender_->SetREMBStatus(true); - EXPECT_TRUE(rtcp_sender_->REMB()); - rtcp_sender_->SetREMBData(kBitrate, kSsrcs); + // Turn off REMB. rtcp_sender no longer should send it. + rtcp_sender_->UnsetRemb(); rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr); ASSERT_EQ(2, parser()->receiver_report()->num_packets()); EXPECT_EQ(1, parser()->remb()->num_packets()); - - // Sending another report sends remb again, even if no new remb data was set. - rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr); - ASSERT_EQ(3, parser()->receiver_report()->num_packets()); - EXPECT_EQ(2, parser()->remb()->num_packets()); - - // Turn off remb. rtcp_sender no longer should send it. - rtcp_sender_->SetREMBStatus(false); - EXPECT_FALSE(rtcp_sender_->REMB()); - rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr); - ASSERT_EQ(4, parser()->receiver_report()->num_packets()); - EXPECT_EQ(2, parser()->remb()->num_packets()); } TEST_F(RtcpSenderTest, SendRemb) { const uint64_t kBitrate = 261011; - std::vector ssrcs; - ssrcs.push_back(kRemoteSsrc); - ssrcs.push_back(kRemoteSsrc + 1); + const std::vector kSsrcs = {kRemoteSsrc, kRemoteSsrc + 1}; rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); - rtcp_sender_->SetREMBData(kBitrate, ssrcs); - EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpRemb)); + rtcp_sender_->SetRemb(kBitrate, kSsrcs); + + rtcp_sender_->SendRTCP(feedback_state(), kRtcpRemb); + EXPECT_EQ(1, parser()->remb()->num_packets()); EXPECT_EQ(kSenderSsrc, parser()->remb()->sender_ssrc()); EXPECT_EQ(kBitrate, parser()->remb()->bitrate_bps()); @@ -542,32 +537,19 @@ TEST_F(RtcpSenderTest, SendRemb) { ElementsAre(kRemoteSsrc, kRemoteSsrc + 1)); } -TEST_F(RtcpSenderTest, RembIncludedInCompoundPacketIfEnabled) { +TEST_F(RtcpSenderTest, RembIncludedInEachCompoundPacketAfterSet) { const int kBitrate = 261011; - std::vector ssrcs; - ssrcs.push_back(kRemoteSsrc); + const std::vector kSsrcs = {kRemoteSsrc, kRemoteSsrc + 1}; rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); - rtcp_sender_->SetREMBStatus(true); - EXPECT_TRUE(rtcp_sender_->REMB()); - rtcp_sender_->SetREMBData(kBitrate, ssrcs); - EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); + rtcp_sender_->SetRemb(kBitrate, kSsrcs); + + rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport); EXPECT_EQ(1, parser()->remb()->num_packets()); // REMB should be included in each compound packet. - EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); + rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport); EXPECT_EQ(2, parser()->remb()->num_packets()); } -TEST_F(RtcpSenderTest, RembNotIncludedInCompoundPacketIfNotEnabled) { - const int kBitrate = 261011; - std::vector ssrcs; - ssrcs.push_back(kRemoteSsrc); - rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); - rtcp_sender_->SetREMBData(kBitrate, ssrcs); - EXPECT_FALSE(rtcp_sender_->REMB()); - EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); - EXPECT_EQ(0, parser()->remb()->num_packets()); -} - TEST_F(RtcpSenderTest, SendXrWithVoipMetric) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); RTCPVoIPMetric metric; @@ -757,7 +739,7 @@ TEST_F(RtcpSenderTest, SendCompoundPliRemb) { std::vector ssrcs; ssrcs.push_back(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); - rtcp_sender_->SetREMBData(kBitrate, ssrcs); + rtcp_sender_->SetRemb(kBitrate, ssrcs); std::set packet_types; packet_types.insert(kRtcpRemb); packet_types.insert(kRtcpPli); @@ -766,7 +748,6 @@ TEST_F(RtcpSenderTest, SendCompoundPliRemb) { EXPECT_EQ(1, parser()->pli()->num_packets()); } - // This test is written to verify that BYE is always the last packet // type in a RTCP compoud packet. The rtcp_sender_ is recreated with // mock_transport, which is used to check for whether BYE at the end diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index b1c91095ff..cfd994acbf 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -631,12 +631,11 @@ int32_t ModuleRtpRtcpImpl::RemoteRTCPStat( // (REMB) Receiver Estimated Max Bitrate. void ModuleRtpRtcpImpl::SetRemb(uint32_t bitrate_bps, const std::vector& ssrcs) { - rtcp_sender_.SetREMBStatus(true); - rtcp_sender_.SetREMBData(bitrate_bps, ssrcs); + rtcp_sender_.SetRemb(bitrate_bps, ssrcs); } void ModuleRtpRtcpImpl::UnsetRemb() { - rtcp_sender_.SetREMBStatus(false); + rtcp_sender_.UnsetRemb(); } int32_t ModuleRtpRtcpImpl::RegisterSendRtpHeaderExtension( diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index e21bc068cd..91675be7c7 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -1232,8 +1232,7 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]); if (remb_value > 0) { - rtcp_sender.SetREMBStatus(true); - rtcp_sender.SetREMBData(remb_value, std::vector()); + rtcp_sender.SetRemb(remb_value, std::vector()); } RTCPSender::FeedbackState feedback_state; EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));