diff --git a/webrtc/modules/pacing/packet_router.cc b/webrtc/modules/pacing/packet_router.cc index 1cee90a441..64e1aeacc1 100644 --- a/webrtc/modules/pacing/packet_router.cc +++ b/webrtc/modules/pacing/packet_router.cc @@ -22,73 +22,63 @@ namespace webrtc { PacketRouter::PacketRouter() : last_remb_time_ms_(rtc::TimeMillis()), last_send_bitrate_bps_(0), + active_remb_module_(nullptr), transport_seq_(0) {} PacketRouter::~PacketRouter() { RTC_DCHECK(rtp_send_modules_.empty()); RTC_DCHECK(rtp_receive_modules_.empty()); + RTC_DCHECK(sender_remb_candidates_.empty()); + RTC_DCHECK(receiver_remb_candidates_.empty()); + RTC_DCHECK(active_remb_module_ == nullptr); } -void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module) { +void PacketRouter::AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate) { rtc::CritScope cs(&modules_crit_); RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module) == rtp_send_modules_.end()); - if (rtp_send_modules_.empty() && !rtp_receive_modules_.empty()) { - rtp_receive_modules_.front()->SetREMBStatus(false); - } - // Put modules which can use regular payload packets (over rtx) instead of // padding first as it's less of a waste if ((rtp_module->RtxSendStatus() & kRtxRedundantPayloads) > 0) { - if (!rtp_send_modules_.empty()) { - rtp_send_modules_.front()->SetREMBStatus(false); - } rtp_send_modules_.push_front(rtp_module); - rtp_module->SetREMBStatus(true); } else { - if (rtp_send_modules_.empty()) { - rtp_module->SetREMBStatus(true); - } - rtp_send_modules_.push_back(rtp_module); } + + if (remb_candidate) { + AddRembModuleCandidate(rtp_module, true); + } } void PacketRouter::RemoveSendRtpModule(RtpRtcp* rtp_module) { rtc::CritScope cs(&modules_crit_); - RTC_DCHECK(std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), - rtp_module) != rtp_send_modules_.end()); - rtp_send_modules_.remove(rtp_module); - rtp_module->SetREMBStatus(false); - if (!rtp_send_modules_.empty()) { - rtp_send_modules_.front()->SetREMBStatus(true); - } else if (!rtp_receive_modules_.empty()) { - rtp_receive_modules_.front()->SetREMBStatus(true); - } + MaybeRemoveRembModuleCandidate(rtp_module, /* sender = */ true); + auto it = + std::find(rtp_send_modules_.begin(), rtp_send_modules_.end(), rtp_module); + RTC_DCHECK(it != rtp_send_modules_.end()); + rtp_send_modules_.erase(it); } -void PacketRouter::AddReceiveRtpModule(RtpRtcp* rtp_module) { +void PacketRouter::AddReceiveRtpModule(RtpRtcp* rtp_module, + bool remb_candidate) { rtc::CritScope cs(&modules_crit_); RTC_DCHECK(std::find(rtp_receive_modules_.begin(), rtp_receive_modules_.end(), rtp_module) == rtp_receive_modules_.end()); - if (rtp_send_modules_.empty() && rtp_receive_modules_.empty()) { - rtp_module->SetREMBStatus(true); - } + rtp_receive_modules_.push_back(rtp_module); + + if (remb_candidate) { + AddRembModuleCandidate(rtp_module, false); + } } void PacketRouter::RemoveReceiveRtpModule(RtpRtcp* rtp_module) { rtc::CritScope cs(&modules_crit_); + MaybeRemoveRembModuleCandidate(rtp_module, /* sender = */ false); const auto& it = std::find(rtp_receive_modules_.begin(), rtp_receive_modules_.end(), rtp_module); RTC_DCHECK(it != rtp_receive_modules_.end()); rtp_receive_modules_.erase(it); - if (rtp_send_modules_.empty()) { - rtp_module->SetREMBStatus(false); - if (!rtp_receive_modules_.empty()) { - rtp_receive_modules_.front()->SetREMBStatus(true); - } - } } bool PacketRouter::TimeToSendPacket(uint32_t ssrc, @@ -190,18 +180,17 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, bool PacketRouter::SendRemb(uint32_t bitrate_bps, const std::vector& ssrcs) { rtc::CritScope lock(&modules_crit_); - RtpRtcp* remb_module; - if (!rtp_send_modules_.empty()) - remb_module = rtp_send_modules_.front(); - else if (!rtp_receive_modules_.empty()) - remb_module = rtp_receive_modules_.front(); - else + + if (!active_remb_module_) { return false; + } + // The Add* and Remove* methods above ensure that this (and only this) module // has REMB enabled. REMB should be disabled on all other modules, because // otherwise, they will send REMB with stale info. - RTC_DCHECK(remb_module->REMB()); - remb_module->SetREMBData(bitrate_bps, ssrcs); + RTC_DCHECK(active_remb_module_->REMB()); + active_remb_module_->SetREMBData(bitrate_bps, ssrcs); + return true; } @@ -222,4 +211,69 @@ bool PacketRouter::SendTransportFeedback(rtcp::TransportFeedback* packet) { return false; } +void PacketRouter::AddRembModuleCandidate(RtpRtcp* candidate_module, + bool sender) { + RTC_DCHECK(candidate_module); + std::vector& candidates = + sender ? sender_remb_candidates_ : receiver_remb_candidates_; + RTC_DCHECK(std::find(candidates.cbegin(), candidates.cend(), + candidate_module) == candidates.cend()); + candidates.push_back(candidate_module); + DetermineActiveRembModule(); +} + +void PacketRouter::MaybeRemoveRembModuleCandidate(RtpRtcp* candidate_module, + bool sender) { + RTC_DCHECK(candidate_module); + std::vector& candidates = + sender ? sender_remb_candidates_ : receiver_remb_candidates_; + auto it = std::find(candidates.begin(), candidates.end(), candidate_module); + + if (it == candidates.end()) { + return; // Function called due to removal of non-REMB-candidate module. + } + + if (*it == active_remb_module_) { + UnsetActiveRembModule(); + } + candidates.erase(it); + DetermineActiveRembModule(); +} + +void PacketRouter::UnsetActiveRembModule() { + RTC_CHECK(active_remb_module_); + RTC_DCHECK(active_remb_module_->REMB()); + active_remb_module_->SetREMBStatus(false); + active_remb_module_ = nullptr; +} + +void PacketRouter::DetermineActiveRembModule() { + // Sender modules take precedence over receiver modules, because SRs (sender + // reports) are sent more frequently than RR (receiver reports). + // When adding the first sender module, we should change the active REMB + // module to be that. Otherwise, we remain with the current active module. + + RtpRtcp* new_active_remb_module_; + + if (!sender_remb_candidates_.empty()) { + new_active_remb_module_ = sender_remb_candidates_.front(); + } else if (!receiver_remb_candidates_.empty()) { + new_active_remb_module_ = receiver_remb_candidates_.front(); + } else { + new_active_remb_module_ = nullptr; + } + + if (new_active_remb_module_ != active_remb_module_) { + if (active_remb_module_) { + UnsetActiveRembModule(); + } + if (new_active_remb_module_) { + RTC_DCHECK(!new_active_remb_module_->REMB()); + new_active_remb_module_->SetREMBStatus(true); + } + } + + active_remb_module_ = new_active_remb_module_; +} + } // namespace webrtc diff --git a/webrtc/modules/pacing/packet_router.h b/webrtc/modules/pacing/packet_router.h index 12d726a2aa..e5abdeb2ab 100644 --- a/webrtc/modules/pacing/packet_router.h +++ b/webrtc/modules/pacing/packet_router.h @@ -49,11 +49,18 @@ class PacketRouter : public PacedSender::PacketSender, RTC_DEPRECATED void RemoveRtpModule(RtpRtcp* rtp_module) { RemoveReceiveRtpModule(rtp_module); } - void AddSendRtpModule(RtpRtcp* rtp_module); - void RemoveSendRtpModule(RtpRtcp* rtp_module); - void AddReceiveRtpModule(RtpRtcp* rtp_module); + void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate); + void RemoveSendRtpModule(RtpRtcp* rtp_module); + RTC_DEPRECATED void AddSendRtpModule(RtpRtcp* rtp_module) { + AddSendRtpModule(rtp_module, true); + } + + void AddReceiveRtpModule(RtpRtcp* rtp_module, bool remb_candidate); void RemoveReceiveRtpModule(RtpRtcp* rtp_module); + RTC_DEPRECATED void AddReceiveRtpModule(RtpRtcp* rtp_module) { + AddReceiveRtpModule(rtp_module, true); + } // Implements PacedSender::Callback. bool TimeToSendPacket(uint32_t ssrc, @@ -84,11 +91,20 @@ class PacketRouter : public PacedSender::PacketSender, virtual bool SendTransportFeedback(rtcp::TransportFeedback* packet); private: + void AddRembModuleCandidate(RtpRtcp* candidate_module, bool sender) + EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void MaybeRemoveRembModuleCandidate(RtpRtcp* candidate_module, bool sender) + EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void UnsetActiveRembModule() EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + void DetermineActiveRembModule() EXCLUSIVE_LOCKS_REQUIRED(modules_crit_); + rtc::RaceChecker pacer_race_; rtc::CriticalSection modules_crit_; std::list rtp_send_modules_ GUARDED_BY(modules_crit_); std::vector rtp_receive_modules_ GUARDED_BY(modules_crit_); + // TODO(eladalon): remb_crit_ only ever held from one function, and it's not + // clear if that function can actually be called from more than one thread. rtc::CriticalSection remb_crit_; // The last time a REMB was sent. int64_t last_remb_time_ms_ GUARDED_BY(remb_crit_); @@ -96,6 +112,12 @@ class PacketRouter : public PacedSender::PacketSender, // The last bitrate update. uint32_t bitrate_bps_ GUARDED_BY(remb_crit_); + // Candidates for the REMB module can be RTP sender/receiver modules, with + // the sender modules taking precedence. + std::vector sender_remb_candidates_ GUARDED_BY(modules_crit_); + std::vector receiver_remb_candidates_ GUARDED_BY(modules_crit_); + RtpRtcp* active_remb_module_ GUARDED_BY(modules_crit_); + volatile int transport_seq_; RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter); diff --git a/webrtc/modules/pacing/packet_router_unittest.cc b/webrtc/modules/pacing/packet_router_unittest.cc index e7f08ca9b5..6ef1b0df76 100644 --- a/webrtc/modules/pacing/packet_router_unittest.cc +++ b/webrtc/modules/pacing/packet_router_unittest.cc @@ -25,23 +25,41 @@ using ::testing::AnyNumber; using ::testing::Field; using ::testing::NiceMock; using ::testing::Return; +using ::testing::ReturnPointee; +using ::testing::SaveArg; namespace webrtc { +// TODO(eladalon): Restructure and/or replace the existing monolithic tests +// (only some of the test are monolithic) according to the new +// guidelines - small tests for one thing at a time. +// (I'm not removing any tests during CL, so as to demonstrate no regressions.) + +class MockRtpRtcpWithRembTracking : public MockRtpRtcp { + public: + MockRtpRtcpWithRembTracking() { + ON_CALL(*this, SetREMBStatus(_)).WillByDefault(SaveArg<0>(&remb_)); + ON_CALL(*this, REMB()).WillByDefault(ReturnPointee(&remb_)); + } + + private: + bool remb_ = false; +}; + class PacketRouterTest : public ::testing::Test { public: PacketRouterTest() : packet_router_(new PacketRouter()) {} protected: - static const int kProbeMinProbes = 5; - static const int kProbeMinBytes = 1000; + static constexpr int kProbeMinProbes = 5; + static constexpr int kProbeMinBytes = 1000; const std::unique_ptr packet_router_; }; TEST_F(PacketRouterTest, TimeToSendPacket) { NiceMock rtp_1; NiceMock rtp_2; - packet_router_->AddSendRtpModule(&rtp_1); - packet_router_->AddSendRtpModule(&rtp_2); + packet_router_->AddSendRtpModule(&rtp_1, false); + packet_router_->AddSendRtpModule(&rtp_2, false); const uint16_t kSsrc1 = 1234; uint16_t sequence_number = 17; @@ -125,8 +143,8 @@ TEST_F(PacketRouterTest, TimeToSendPadding) { // rtp_2 will be prioritized for padding. EXPECT_CALL(rtp_2, RtxSendStatus()).WillOnce(Return(kRtxRedundantPayloads)); EXPECT_CALL(rtp_2, SSRC()).WillRepeatedly(Return(kSsrc2)); - packet_router_->AddSendRtpModule(&rtp_1); - packet_router_->AddSendRtpModule(&rtp_2); + packet_router_->AddSendRtpModule(&rtp_1, false); + packet_router_->AddSendRtpModule(&rtp_2, false); // Default configuration, sending padding on all modules sending media, // ordered by priority (based on rtx mode). @@ -210,7 +228,7 @@ TEST_F(PacketRouterTest, TimeToSendPadding) { TEST_F(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) { NiceMock rtp; - packet_router_->AddSendRtpModule(&rtp); + packet_router_->AddSendRtpModule(&rtp, false); static const uint16_t kSsrc = 1234; EXPECT_CALL(rtp, SSRC()).WillRepeatedly(Return(kSsrc)); EXPECT_CALL(rtp, SendingMedia()).WillRepeatedly(Return(false)); @@ -246,8 +264,8 @@ TEST_F(PacketRouterTest, AllocateSequenceNumbers) { TEST_F(PacketRouterTest, SendTransportFeedback) { NiceMock rtp_1; NiceMock rtp_2; - packet_router_->AddSendRtpModule(&rtp_1); - packet_router_->AddReceiveRtpModule(&rtp_2); + packet_router_->AddSendRtpModule(&rtp_1, false); + packet_router_->AddReceiveRtpModule(&rtp_2, false); rtcp::TransportFeedback feedback; EXPECT_CALL(rtp_1, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true)); @@ -258,19 +276,60 @@ TEST_F(PacketRouterTest, SendTransportFeedback) { packet_router_->RemoveReceiveRtpModule(&rtp_2); } +#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) +TEST_F(PacketRouterTest, DoubleRegistrationOfSendModuleDisallowed) { + NiceMock module; + + constexpr bool remb_candidate = false; // Value irrelevant. + packet_router_->AddSendRtpModule(&module, remb_candidate); + EXPECT_DEATH(packet_router_->AddSendRtpModule(&module, remb_candidate), ""); + + // Test tear-down + packet_router_->RemoveSendRtpModule(&module); +} + +TEST_F(PacketRouterTest, DoubleRegistrationOfReceiveModuleDisallowed) { + NiceMock module; + + constexpr bool remb_candidate = false; // Value irrelevant. + packet_router_->AddReceiveRtpModule(&module, remb_candidate); + EXPECT_DEATH(packet_router_->AddReceiveRtpModule(&module, remb_candidate), + ""); + + // Test tear-down + packet_router_->RemoveReceiveRtpModule(&module); +} + +TEST_F(PacketRouterTest, RemovalOfNeverAddedSendModuleDisallowed) { + NiceMock module; + + EXPECT_DEATH(packet_router_->RemoveSendRtpModule(&module), ""); +} + +TEST_F(PacketRouterTest, RemovalOfNeverAddedReceiveModuleDisallowed) { + NiceMock module; + + EXPECT_DEATH(packet_router_->RemoveReceiveRtpModule(&module), ""); +} +#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) + +// TODO(eladalon): Remove this test; it should be covered by: +// 1. SendCandidatePreferredOverReceiveCandidate_SendModuleAddedFirst +// 2. SendCandidatePreferredOverReceiveCandidate_ReceiveModuleAddedFirst +// 3. LowerEstimateToSendRemb +// (Not removing in this CL to prove it doesn't break this test.) TEST(PacketRouterRembTest, PreferSendModuleOverReceiveModule) { rtc::ScopedFakeClock clock; - NiceMock rtp_recv; - NiceMock rtp_send; + NiceMock rtp_recv; + NiceMock rtp_send; PacketRouter packet_router; - EXPECT_CALL(rtp_recv, SetREMBStatus(true)).Times(1); - packet_router.AddReceiveRtpModule(&rtp_recv); + packet_router.AddReceiveRtpModule(&rtp_recv, true); + ASSERT_TRUE(rtp_recv.REMB()); const uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - ON_CALL(rtp_recv, REMB()).WillByDefault(Return(true)); packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. @@ -279,35 +338,32 @@ TEST(PacketRouterRembTest, PreferSendModuleOverReceiveModule) { packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Add a send module, which should be preferred over the receive module. - EXPECT_CALL(rtp_recv, SetREMBStatus(false)).Times(1); - EXPECT_CALL(rtp_send, SetREMBStatus(true)).Times(1); - packet_router.AddSendRtpModule(&rtp_send); - ON_CALL(rtp_recv, REMB()).WillByDefault(Return(false)); - ON_CALL(rtp_send, REMB()).WillByDefault(Return(true)); + packet_router.AddSendRtpModule(&rtp_send, true); + EXPECT_FALSE(rtp_recv.REMB()); + EXPECT_TRUE(rtp_send.REMB()); // Lower bitrate to send another REMB packet. EXPECT_CALL(rtp_send, SetREMBData(bitrate_estimate - 100, ssrcs)).Times(1); packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100); - EXPECT_CALL(rtp_send, SetREMBStatus(false)).Times(1); - EXPECT_CALL(rtp_recv, SetREMBStatus(true)).Times(1); packet_router.RemoveSendRtpModule(&rtp_send); - EXPECT_CALL(rtp_recv, SetREMBStatus(false)).Times(1); + EXPECT_TRUE(rtp_recv.REMB()); + EXPECT_FALSE(rtp_send.REMB()); + packet_router.RemoveReceiveRtpModule(&rtp_recv); } TEST(PacketRouterRembTest, LowerEstimateToSendRemb) { rtc::ScopedFakeClock clock; - NiceMock rtp; + NiceMock rtp; PacketRouter packet_router; - EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1); - packet_router.AddSendRtpModule(&rtp); + packet_router.AddSendRtpModule(&rtp, true); + EXPECT_TRUE(rtp.REMB()); uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - ON_CALL(rtp, REMB()).WillByDefault(Return(true)); packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); // Call OnReceiveBitrateChanged twice to get a first estimate. @@ -321,15 +377,15 @@ TEST(PacketRouterRembTest, LowerEstimateToSendRemb) { EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)).Times(1); packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - EXPECT_CALL(rtp, SetREMBStatus(false)).Times(1); packet_router.RemoveSendRtpModule(&rtp); + EXPECT_FALSE(rtp.REMB()); } TEST(PacketRouterRembTest, VerifyIncreasingAndDecreasing) { rtc::ScopedFakeClock clock; NiceMock rtp; PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp); + packet_router.AddSendRtpModule(&rtp, true); uint32_t bitrate_estimate[] = {456, 789}; std::vector ssrcs = {1234, 5678}; @@ -355,7 +411,7 @@ TEST(PacketRouterRembTest, NoRembForIncreasedBitrate) { rtc::ScopedFakeClock clock; NiceMock rtp; PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp); + packet_router.AddSendRtpModule(&rtp, true); uint32_t bitrate_estimate = 456; std::vector ssrcs = {1234, 5678}; @@ -385,8 +441,8 @@ TEST(PacketRouterRembTest, ChangeSendRtpModule) { NiceMock rtp_send; NiceMock rtp_recv; PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp_send); - packet_router.AddReceiveRtpModule(&rtp_recv); + packet_router.AddSendRtpModule(&rtp_send, true); + packet_router.AddReceiveRtpModule(&rtp_recv, true); uint32_t bitrate_estimate = 456; std::vector ssrcs = {1234, 5678}; @@ -423,7 +479,7 @@ TEST(PacketRouterRembTest, OnlyOneRembForRepeatedOnReceiveBitrateChanged) { rtc::ScopedFakeClock clock; NiceMock rtp; PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp); + packet_router.AddSendRtpModule(&rtp, true); uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; @@ -455,7 +511,7 @@ TEST(PacketRouterRembTest, NoSendingRtpModule) { PacketRouter packet_router; EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1); - packet_router.AddReceiveRtpModule(&rtp); + packet_router.AddReceiveRtpModule(&rtp, true); uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; @@ -476,4 +532,176 @@ TEST(PacketRouterRembTest, NoSendingRtpModule) { packet_router.RemoveReceiveRtpModule(&rtp); } +TEST(PacketRouterRembTest, NonCandidateSendRtpModuleNotUsedForRemb) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock module; + + constexpr bool remb_candidate = false; + + packet_router.AddSendRtpModule(&module, remb_candidate); + EXPECT_FALSE(module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(module, SetREMBData(_, _)).Times(0); + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveSendRtpModule(&module); +} + +TEST(PacketRouterRembTest, CandidateSendRtpModuleUsedForRemb) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock module; + + constexpr bool remb_candidate = true; + + packet_router.AddSendRtpModule(&module, remb_candidate); + EXPECT_TRUE(module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(module, SetREMBData(bitrate_estimate, ssrcs)).Times(1); + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveSendRtpModule(&module); +} + +TEST(PacketRouterRembTest, NonCandidateReceiveRtpModuleNotUsedForRemb) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock module; + + constexpr bool remb_candidate = false; + + packet_router.AddReceiveRtpModule(&module, remb_candidate); + ASSERT_FALSE(module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(module, SetREMBData(_, _)).Times(0); + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveReceiveRtpModule(&module); +} + +TEST(PacketRouterRembTest, CandidateReceiveRtpModuleUsedForRemb) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock module; + + constexpr bool remb_candidate = true; + + packet_router.AddReceiveRtpModule(&module, remb_candidate); + EXPECT_TRUE(module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(module, SetREMBData(bitrate_estimate, ssrcs)).Times(1); + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveReceiveRtpModule(&module); +} + +TEST(PacketRouterRembTest, + SendCandidatePreferredOverReceiveCandidate_SendModuleAddedFirst) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock send_module; + NiceMock receive_module; + + constexpr bool remb_candidate = true; + + // Send module added - activated. + packet_router.AddSendRtpModule(&send_module, remb_candidate); + ASSERT_TRUE(send_module.REMB()); + + // Receive module added - the send module remains the active one. + packet_router.AddReceiveRtpModule(&receive_module, remb_candidate); + EXPECT_TRUE(send_module.REMB()); + EXPECT_FALSE(receive_module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(send_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1); + EXPECT_CALL(receive_module, SetREMBData(_, _)).Times(0); + + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveReceiveRtpModule(&receive_module); + packet_router.RemoveSendRtpModule(&send_module); +} + +TEST(PacketRouterRembTest, + SendCandidatePreferredOverReceiveCandidate_ReceiveModuleAddedFirst) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock send_module; + NiceMock receive_module; + + constexpr bool remb_candidate = true; + + // Receive module added - activated. + packet_router.AddReceiveRtpModule(&receive_module, remb_candidate); + ASSERT_TRUE(receive_module.REMB()); + + // Send module added - replaces receive module as active. + packet_router.AddSendRtpModule(&send_module, remb_candidate); + EXPECT_FALSE(receive_module.REMB()); + EXPECT_TRUE(send_module.REMB()); + + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(send_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1); + EXPECT_CALL(receive_module, SetREMBData(_, _)).Times(0); + + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveReceiveRtpModule(&receive_module); + packet_router.RemoveSendRtpModule(&send_module); +} + +TEST(PacketRouterRembTest, ReceiveModuleTakesOverWhenLastSendModuleRemoved) { + rtc::ScopedFakeClock clock; + PacketRouter packet_router; + NiceMock send_module; + NiceMock receive_module; + + constexpr bool remb_candidate = true; + + // Send module active, receive module inactive. + packet_router.AddSendRtpModule(&send_module, remb_candidate); + packet_router.AddReceiveRtpModule(&receive_module, remb_candidate); + ASSERT_TRUE(send_module.REMB()); + ASSERT_FALSE(receive_module.REMB()); + + // Send module removed - receive module becomes active. + packet_router.RemoveSendRtpModule(&send_module); + EXPECT_FALSE(send_module.REMB()); + EXPECT_TRUE(receive_module.REMB()); + constexpr uint32_t bitrate_estimate = 456; + const std::vector ssrcs = {1234}; + EXPECT_CALL(send_module, SetREMBData(_, _)).Times(0); + EXPECT_CALL(receive_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1); + + clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000)); + packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + + // Test tear-down + packet_router.RemoveReceiveRtpModule(&receive_module); +} + } // namespace webrtc diff --git a/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc index 4204670c7e..ae19d90e1a 100644 --- a/webrtc/video/rtp_video_stream_receiver.cc +++ b/webrtc/video/rtp_video_stream_receiver.cc @@ -113,7 +113,8 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( keyframe_request_sender_(keyframe_request_sender), timing_(timing), has_received_frame_(false) { - packet_router_->AddReceiveRtpModule(rtp_rtcp_.get()); + constexpr bool remb_candidate = true; + packet_router_->AddReceiveRtpModule(rtp_rtcp_.get(), remb_candidate); rtp_receive_statistics_->RegisterRtpStatisticsCallback(receive_stats_proxy); rtp_receive_statistics_->RegisterRtcpStatisticsCallback(receive_stats_proxy); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 5509553e25..2c3a5c495c 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -846,8 +846,10 @@ VideoSendStreamImpl::VideoSendStreamImpl( // We add the highest spatial layer first to ensure it'll be prioritized // when sending padding, with the hope that the packet rate will be smaller, // and that it's more important to protect than the lower layers. - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) - transport->packet_router()->AddSendRtpModule(rtp_rtcp); + for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { + constexpr bool remb_candidate = true; + transport->packet_router()->AddSendRtpModule(rtp_rtcp, remb_candidate); + } for (size_t i = 0; i < config_->rtp.extensions.size(); ++i) { const std::string& extension = config_->rtp.extensions[i].uri; diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 5c2525cf2b..047f3e19dd 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -2508,14 +2508,16 @@ void Channel::RegisterSenderCongestionControlObjects( seq_num_allocator_proxy_->SetSequenceNumberAllocator(packet_router); rtp_packet_sender_proxy_->SetPacketSender(rtp_packet_sender); _rtpRtcpModule->SetStorePacketsStatus(true, 600); - packet_router->AddSendRtpModule(_rtpRtcpModule.get()); + constexpr bool remb_candidate = false; + packet_router->AddSendRtpModule(_rtpRtcpModule.get(), remb_candidate); packet_router_ = packet_router; } void Channel::RegisterReceiverCongestionControlObjects( PacketRouter* packet_router) { RTC_DCHECK(packet_router && !packet_router_); - packet_router->AddReceiveRtpModule(_rtpRtcpModule.get()); + constexpr bool remb_candidate = false; + packet_router->AddReceiveRtpModule(_rtpRtcpModule.get(), remb_candidate); packet_router_ = packet_router; }