Simplify setting/unsetting REMB in RtcpSender
follow up of https://webrtc-review.googlesource.com/c/src/+/7983
Bug: None
Change-Id: I408c21408478d801a769e2e9d5f2eb9408430a4b
Reviewed-on: https://webrtc-review.googlesource.com/12520
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Reviewed-by: Elad Alon <eladalon@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20359}diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc
index d79e199..9eda2a0 100644
--- a/modules/rtp_rtcp/source/rtcp_sender.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender.cc
@@ -154,7 +154,6 @@
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 @@
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<uint32_t>& ssrcs) {
+void RTCPSender::SetRemb(uint32_t bitrate, const std::vector<uint32_t>& 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 69a0de5..3896559 100644
--- a/modules/rtp_rtcp/source/rtcp_sender.h
+++ b/modules/rtp_rtcp/source/rtcp_sender.h
@@ -120,11 +120,9 @@
int32_t nackSize = 0,
const uint16_t* nackList = 0);
- bool REMB() const;
+ void SetRemb(uint32_t bitrate, const std::vector<uint32_t>& ssrcs);
- void SetREMBStatus(bool enable);
-
- void SetREMBData(uint32_t bitrate, const std::vector<uint32_t>& ssrcs);
+ void UnsetRemb();
bool TMMBR() const;
@@ -199,7 +197,6 @@
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 589db34..9ba7689 100644
--- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -497,44 +497,39 @@
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<uint32_t> 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<uint32_t> ssrcs;
- ssrcs.push_back(kRemoteSsrc);
- ssrcs.push_back(kRemoteSsrc + 1);
+ const std::vector<uint32_t> 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 @@
ElementsAre(kRemoteSsrc, kRemoteSsrc + 1));
}
-TEST_F(RtcpSenderTest, RembIncludedInCompoundPacketIfEnabled) {
+TEST_F(RtcpSenderTest, RembIncludedInEachCompoundPacketAfterSet) {
const int kBitrate = 261011;
- std::vector<uint32_t> ssrcs;
- ssrcs.push_back(kRemoteSsrc);
+ const std::vector<uint32_t> 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<uint32_t> 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 @@
std::vector<uint32_t> ssrcs;
ssrcs.push_back(kRemoteSsrc);
rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);
- rtcp_sender_->SetREMBData(kBitrate, ssrcs);
+ rtcp_sender_->SetRemb(kBitrate, ssrcs);
std::set<RTCPPacketType> packet_types;
packet_types.insert(kRtcpRemb);
packet_types.insert(kRtcpPli);
@@ -766,7 +748,6 @@
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 b1c9109..cfd994a 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -631,12 +631,11 @@
// (REMB) Receiver Estimated Max Bitrate.
void ModuleRtpRtcpImpl::SetRemb(uint32_t bitrate_bps,
const std::vector<uint32_t>& 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 e21bc06..91675be 100644
--- a/video/video_send_stream_tests.cc
+++ b/video/video_send_stream_tests.cc
@@ -1232,8 +1232,7 @@
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<uint32_t>());
+ rtcp_sender.SetRemb(remb_value, std::vector<uint32_t>());
}
RTCPSender::FeedbackState feedback_state;
EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
