diff --git a/DEPS b/DEPS index e518fcd41a..455d628915 100644 --- a/DEPS +++ b/DEPS @@ -2612,6 +2612,7 @@ include_rules = [ "+absl/base/const_init.h", "+absl/base/macros.h", "+absl/container/inlined_vector.h", + "+absl/functional/bind_front.h", "+absl/memory/memory.h", "+absl/meta/type_traits.h", "+absl/strings/ascii.h", diff --git a/abseil-in-webrtc.md b/abseil-in-webrtc.md index 79b1031ffd..7bed46cd97 100644 --- a/abseil-in-webrtc.md +++ b/abseil-in-webrtc.md @@ -22,6 +22,7 @@ will generate a shared library. ## **Allowed** +* `absl::bind_front` * `absl::InlinedVector` * `absl::WrapUnique` * `absl::optional` and related stuff from `absl/types/optional.h`. diff --git a/call/BUILD.gn b/call/BUILD.gn index b7cac1ca81..9f0cd037f3 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -73,7 +73,10 @@ rtc_library("call_interfaces") { "../rtc_base:rtc_base_approved", "../rtc_base/network:sent_packet", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/functional:bind_front", + "//third_party/abseil-cpp/absl/types:optional", + ] } rtc_source_set("audio_sender_interface") { @@ -304,7 +307,10 @@ rtc_library("call") { "../video", "adaptation:resource_adaptation", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/functional:bind_front", + "//third_party/abseil-cpp/absl/types:optional", + ] } rtc_library("video_stream_api") { diff --git a/call/call.cc b/call/call.cc index e6cf1c31dd..a9ae07b60a 100644 --- a/call/call.cc +++ b/call/call.cc @@ -19,6 +19,7 @@ #include #include +#include "absl/functional/bind_front.h" #include "absl/types/optional.h" #include "api/rtc_event_log/rtc_event_log.h" #include "api/sequence_checker.h" @@ -658,7 +659,12 @@ Call::Call(Clock* clock, configured_max_padding_bitrate_bps_(0), estimated_send_bitrate_kbps_counter_(clock_, nullptr, true), pacer_bitrate_kbps_counter_(clock_, nullptr, true), - receive_side_cc_(clock_, transport_send->packet_router()), + receive_side_cc_(clock, + absl::bind_front(&PacketRouter::SendCombinedRtcpPacket, + transport_send->packet_router()), + absl::bind_front(&PacketRouter::SendRemb, + transport_send->packet_router()), + /*network_state_estimator=*/nullptr), receive_time_calculator_(ReceiveTimeCalculator::CreateFromFieldTrial()), video_send_delay_stats_(new SendDelayStats(clock_)), start_ms_(clock_->TimeInMilliseconds()), diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index b46cd8d7fc..84661c05b7 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -35,14 +35,6 @@ class RemoteBitrateObserver; class ReceiveSideCongestionController : public CallStatsObserver, public Module { public: - // TODO(bugs.webrtc.org/12693): Deprecate - ReceiveSideCongestionController(Clock* clock, PacketRouter* packet_router); - // TODO(bugs.webrtc.org/12693): Deprecate - ReceiveSideCongestionController( - Clock* clock, - PacketRouter* packet_router, - NetworkStateEstimator* network_state_estimator); - ReceiveSideCongestionController( Clock* clock, RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index e4e6cc9698..61a126fbe3 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -119,26 +119,6 @@ void ReceiveSideCongestionController::WrappingBitrateEstimator:: rbe_->SetMinBitrate(min_bitrate_bps_); } -ReceiveSideCongestionController::ReceiveSideCongestionController( - Clock* clock, - PacketRouter* packet_router) - : ReceiveSideCongestionController(clock, packet_router, nullptr) {} - -ReceiveSideCongestionController::ReceiveSideCongestionController( - Clock* clock, - PacketRouter* packet_router, - NetworkStateEstimator* network_state_estimator) - : remb_throttler_([](auto...) {}, clock), - remote_bitrate_estimator_(packet_router, clock), - remote_estimator_proxy_( - clock, - [packet_router]( - std::vector> packets) { - packet_router->SendCombinedRtcpPacket(std::move(packets)); - }, - &field_trial_config_, - network_state_estimator) {} - ReceiveSideCongestionController::ReceiveSideCongestionController( Clock* clock, RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 5317f510c9..3b1278e504 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -27,20 +27,11 @@ #include "rtc_base/trace_event.h" namespace webrtc { -namespace { - -constexpr int kRembSendIntervalMs = 200; - -} // namespace PacketRouter::PacketRouter() : PacketRouter(0) {} PacketRouter::PacketRouter(uint16_t start_transport_seq) : last_send_module_(nullptr), - last_remb_time_ms_(rtc::TimeMillis()), - last_send_bitrate_bps_(0), - bitrate_bps_(0), - max_bitrate_bps_(std::numeric_limits::max()), active_remb_module_(nullptr), transport_seq_(start_transport_seq) {} @@ -235,77 +226,19 @@ uint16_t PacketRouter::CurrentTransportSequenceNumber() const { return transport_seq_ & 0xFFFF; } -void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, - uint32_t bitrate_bps) { - // % threshold for if we should send a new REMB asap. - const int64_t kSendThresholdPercent = 97; - // TODO(danilchap): Remove receive_bitrate_bps variable and the cast - // when OnReceiveBitrateChanged takes bitrate as int64_t. - int64_t receive_bitrate_bps = static_cast(bitrate_bps); - - int64_t now_ms = rtc::TimeMillis(); - { - MutexLock lock(&remb_mutex_); - - // If we already have an estimate, check if the new total estimate is below - // kSendThresholdPercent of the previous estimate. - if (last_send_bitrate_bps_ > 0) { - int64_t new_remb_bitrate_bps = - last_send_bitrate_bps_ - bitrate_bps_ + receive_bitrate_bps; - - if (new_remb_bitrate_bps < - kSendThresholdPercent * last_send_bitrate_bps_ / 100) { - // The new bitrate estimate is less than kSendThresholdPercent % of the - // last report. Send a REMB asap. - last_remb_time_ms_ = now_ms - kRembSendIntervalMs; - } - } - bitrate_bps_ = receive_bitrate_bps; - - if (now_ms - last_remb_time_ms_ < kRembSendIntervalMs) { - return; - } - // NOTE: Updated if we intend to send the data; we might not have - // a module to actually send it. - last_remb_time_ms_ = now_ms; - last_send_bitrate_bps_ = receive_bitrate_bps; - // Cap the value to send in remb with configured value. - receive_bitrate_bps = std::min(receive_bitrate_bps, max_bitrate_bps_); - } - SendRemb(receive_bitrate_bps, ssrcs); -} - -void PacketRouter::SetMaxDesiredReceiveBitrate(int64_t bitrate_bps) { - RTC_DCHECK_GE(bitrate_bps, 0); - { - MutexLock lock(&remb_mutex_); - max_bitrate_bps_ = bitrate_bps; - if (rtc::TimeMillis() - last_remb_time_ms_ < kRembSendIntervalMs && - last_send_bitrate_bps_ > 0 && - last_send_bitrate_bps_ <= max_bitrate_bps_) { - // Recent measured bitrate is already below the cap. - return; - } - } - SendRemb(bitrate_bps, /*ssrcs=*/{}); -} - -bool PacketRouter::SendRemb(int64_t bitrate_bps, - const std::vector& ssrcs) { +void PacketRouter::SendRemb(int64_t bitrate_bps, std::vector ssrcs) { MutexLock lock(&modules_mutex_); if (!active_remb_module_) { - return false; + return; } // The Add* and Remove* methods above ensure that REMB is disabled on all // other modules, because otherwise, they will send REMB with stale info. - active_remb_module_->SetRemb(bitrate_bps, ssrcs); - - return true; + active_remb_module_->SetRemb(bitrate_bps, std::move(ssrcs)); } -bool PacketRouter::SendCombinedRtcpPacket( +void PacketRouter::SendCombinedRtcpPacket( std::vector> packets) { MutexLock lock(&modules_mutex_); @@ -315,15 +248,14 @@ bool PacketRouter::SendCombinedRtcpPacket( continue; } rtp_module->SendCombinedRtcpPacket(std::move(packets)); - return true; + return; } if (rtcp_feedback_senders_.empty()) { - return false; + return; } auto* rtcp_sender = rtcp_feedback_senders_[0]; rtcp_sender->SendCombinedRtcpPacket(std::move(packets)); - return true; } void PacketRouter::AddRembModuleCandidate( diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 2fa104b4cd..7a6e24d7ea 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -39,9 +39,7 @@ class RtpRtcpInterface; // module if possible (sender report), otherwise on receive module // (receiver report). For the latter case, we also keep track of the // receive modules. -class PacketRouter : public RemoteBitrateObserver, - public TransportFeedbackSenderInterface, - public PacingController::PacketSender { +class PacketRouter : public PacingController::PacketSender { public: PacketRouter(); explicit PacketRouter(uint16_t start_transport_seq); @@ -62,24 +60,12 @@ class PacketRouter : public RemoteBitrateObserver, uint16_t CurrentTransportSequenceNumber() const; - // Called every time there is a new bitrate estimate for a receive channel - // group. This call will trigger a new RTCP REMB packet if the bitrate - // estimate has decreased or if no RTCP REMB packet has been sent for - // a certain time interval. - // Implements RtpReceiveBitrateUpdate. - void OnReceiveBitrateChanged(const std::vector& ssrcs, - uint32_t bitrate_bps) override; - - // Ensures remote party notified of the receive bitrate limit no larger than - // |bitrate_bps|. - void SetMaxDesiredReceiveBitrate(int64_t bitrate_bps); - // Send REMB feedback. - bool SendRemb(int64_t bitrate_bps, const std::vector& ssrcs); + void SendRemb(int64_t bitrate_bps, std::vector ssrcs); // Sends |packets| in one or more IP packets. - bool SendCombinedRtcpPacket( - std::vector> packets) override; + void SendCombinedRtcpPacket( + std::vector> packets); private: void AddRembModuleCandidate(RtcpFeedbackSenderInterface* candidate_module, @@ -107,16 +93,6 @@ class PacketRouter : public RemoteBitrateObserver, std::vector rtcp_feedback_senders_ RTC_GUARDED_BY(modules_mutex_); - // TODO(eladalon): remb_mutex_ only ever held from one function, and it's not - // clear if that function can actually be called from more than one thread. - Mutex remb_mutex_; - // The last time a REMB was sent. - int64_t last_remb_time_ms_ RTC_GUARDED_BY(remb_mutex_); - int64_t last_send_bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); - // The last bitrate update. - int64_t bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); - int64_t max_bitrate_bps_ RTC_GUARDED_BY(remb_mutex_); - // Candidates for the REMB module can be RTP sender/receiver modules, with // the sender modules taking precedence. std::vector sender_remb_candidates_ diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 10cf98b3dd..77fe5f9f8d 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -74,25 +74,19 @@ TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_GeneratePadding) { EXPECT_TRUE(packet_router_.GeneratePadding(bytes).empty()); } -TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_OnReceiveBitrateChanged) { - const std::vector ssrcs = {1, 2, 3}; - constexpr uint32_t bitrate_bps = 10000; - - packet_router_.OnReceiveBitrateChanged(ssrcs, bitrate_bps); -} TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendRemb) { const std::vector ssrcs = {1, 2, 3}; constexpr uint32_t bitrate_bps = 10000; - - EXPECT_FALSE(packet_router_.SendRemb(bitrate_bps, ssrcs)); + // Expect not to crash + packet_router_.SendRemb(bitrate_bps, ssrcs); } TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) { std::vector> feedback; feedback.push_back(std::make_unique()); - - EXPECT_FALSE(packet_router_.SendCombinedRtcpPacket(std::move(feedback))); + // Expect not to crash + packet_router_.SendCombinedRtcpPacket(std::move(feedback)); } TEST_F(PacketRouterTest, GeneratePaddingPrioritizesRtx) { @@ -327,10 +321,10 @@ TEST_F(PacketRouterTest, SendTransportFeedback) { std::vector> feedback; feedback.push_back(std::make_unique()); - EXPECT_CALL(rtp_1, SendCombinedRtcpPacket).Times(1); + EXPECT_CALL(rtp_1, SendCombinedRtcpPacket); packet_router_.SendCombinedRtcpPacket(std::move(feedback)); packet_router_.RemoveSendRtpModule(&rtp_1); - EXPECT_CALL(rtp_2, SendCombinedRtcpPacket).Times(1); + EXPECT_CALL(rtp_2, SendCombinedRtcpPacket); std::vector> new_feedback; new_feedback.push_back(std::make_unique()); packet_router_.SendCombinedRtcpPacket(std::move(new_feedback)); @@ -442,86 +436,7 @@ TEST_F(PacketRouterDeathTest, RemovalOfNeverAddedReceiveModuleDisallowed) { } #endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST(PacketRouterRembTest, LowerEstimateToSendRemb) { - rtc::ScopedFakeClock clock; - NiceMock rtp; - PacketRouter packet_router; - - packet_router.AddSendRtpModule(&rtp, true); - - uint32_t bitrate_estimate = 456; - const std::vector ssrcs = {1234}; - - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - clock.AdvanceTime(TimeDelta::Millis(1000)); - EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Lower the estimate with more than 3% to trigger a call to SetRemb right - // away. - bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - packet_router.RemoveSendRtpModule(&rtp); -} - -TEST(PacketRouterRembTest, VerifyIncreasingAndDecreasing) { - rtc::ScopedFakeClock clock; - NiceMock rtp; - PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp, true); - - uint32_t bitrate_estimate[] = {456, 789}; - std::vector ssrcs = {1234, 5678}; - - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - EXPECT_CALL(rtp, SetRemb(bitrate_estimate[0], ssrcs)).Times(1); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]); - - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1] + 100); - - // Lower the estimate to trigger a callback. - EXPECT_CALL(rtp, SetRemb(bitrate_estimate[1], ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1]); - - packet_router.RemoveSendRtpModule(&rtp); -} - -TEST(PacketRouterRembTest, NoRembForIncreasedBitrate) { - rtc::ScopedFakeClock clock; - NiceMock rtp; - PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp, true); - - uint32_t bitrate_estimate = 456; - std::vector ssrcs = {1234, 5678}; - - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)).Times(1); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Increased estimate shouldn't trigger a callback right away. - EXPECT_CALL(rtp, SetRemb(_, _)).Times(0); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate + 1); - - // Decreasing the estimate less than 3% shouldn't trigger a new callback. - EXPECT_CALL(rtp, SetRemb(_, _)).Times(0); - int lower_estimate = bitrate_estimate * 98 / 100; - packet_router.OnReceiveBitrateChanged(ssrcs, lower_estimate); - - packet_router.RemoveSendRtpModule(&rtp); -} - -TEST(PacketRouterRembTest, ChangeSendRtpModule) { +TEST(PacketRouterRembTest, ChangeSendRtpModuleChangeRembSender) { rtc::ScopedFakeClock clock; NiceMock rtp_send; NiceMock rtp_recv; @@ -532,191 +447,18 @@ TEST(PacketRouterRembTest, ChangeSendRtpModule) { uint32_t bitrate_estimate = 456; std::vector ssrcs = {1234, 5678}; - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - clock.AdvanceTime(TimeDelta::Millis(1000)); - EXPECT_CALL(rtp_send, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Decrease estimate to trigger a REMB. - bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp_send, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(rtp_send, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Remove the sending module -> should get remb on the second module. packet_router.RemoveSendRtpModule(&rtp_send); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp_recv, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(rtp_recv, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); packet_router.RemoveReceiveRtpModule(&rtp_recv); } -TEST(PacketRouterRembTest, OnlyOneRembForRepeatedOnReceiveBitrateChanged) { - rtc::ScopedFakeClock clock; - NiceMock rtp; - PacketRouter packet_router; - packet_router.AddSendRtpModule(&rtp, true); - - uint32_t bitrate_estimate = 456; - const std::vector ssrcs = {1234}; - - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - clock.AdvanceTime(TimeDelta::Millis(1000)); - EXPECT_CALL(rtp, SetRemb(_, _)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Lower the estimate, should trigger a call to SetRemb right away. - bitrate_estimate = bitrate_estimate - 100; - EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged again, this should not trigger a new callback. - EXPECT_CALL(rtp, SetRemb(_, _)).Times(0); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - packet_router.RemoveSendRtpModule(&rtp); -} - -TEST(PacketRouterRembTest, SetMaxDesiredReceiveBitrateLimitsSetRemb) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - const int64_t cap_bitrate = 100000; - EXPECT_CALL(remb_sender, SetRemb(Le(cap_bitrate), _)).Times(AtLeast(1)); - EXPECT_CALL(remb_sender, SetRemb(Gt(cap_bitrate), _)).Times(0); - - const std::vector ssrcs = {1234}; - packet_router.SetMaxDesiredReceiveBitrate(cap_bitrate); - packet_router.OnReceiveBitrateChanged(ssrcs, cap_bitrate + 5000); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, cap_bitrate - 5000); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - -TEST(PacketRouterRembTest, - SetMaxDesiredReceiveBitrateTriggersRembWhenMoreRestrictive) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - const int64_t measured_bitrate_bps = 150000; - const int64_t cap_bitrate_bps = measured_bitrate_bps - 5000; - const std::vector ssrcs = {1234}; - EXPECT_CALL(remb_sender, SetRemb(measured_bitrate_bps, _)); - packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps); - - EXPECT_CALL(remb_sender, SetRemb(cap_bitrate_bps, _)); - packet_router.SetMaxDesiredReceiveBitrate(cap_bitrate_bps); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - -TEST(PacketRouterRembTest, - SetMaxDesiredReceiveBitrateDoesNotTriggerRembWhenAsRestrictive) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - const uint32_t measured_bitrate_bps = 150000; - const uint32_t cap_bitrate_bps = measured_bitrate_bps; - const std::vector ssrcs = {1234}; - EXPECT_CALL(remb_sender, SetRemb(measured_bitrate_bps, _)); - packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps); - - EXPECT_CALL(remb_sender, SetRemb(_, _)).Times(0); - packet_router.SetMaxDesiredReceiveBitrate(cap_bitrate_bps); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - -TEST(PacketRouterRembTest, - SetMaxDesiredReceiveBitrateDoesNotTriggerRembWhenLessRestrictive) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - const uint32_t measured_bitrate_bps = 150000; - const uint32_t cap_bitrate_bps = measured_bitrate_bps + 500; - const std::vector ssrcs = {1234}; - EXPECT_CALL(remb_sender, SetRemb(measured_bitrate_bps, _)); - packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps); - - EXPECT_CALL(remb_sender, SetRemb(_, _)).Times(0); - packet_router.SetMaxDesiredReceiveBitrate(cap_bitrate_bps); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - -TEST(PacketRouterRembTest, - SetMaxDesiredReceiveBitrateTriggersRembWhenNoRecentMeasure) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - const uint32_t measured_bitrate_bps = 150000; - const uint32_t cap_bitrate_bps = measured_bitrate_bps + 5000; - const std::vector ssrcs = {1234}; - EXPECT_CALL(remb_sender, SetRemb(measured_bitrate_bps, _)); - packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps); - clock.AdvanceTime(TimeDelta::Millis(1000)); - - EXPECT_CALL(remb_sender, SetRemb(cap_bitrate_bps, _)); - packet_router.SetMaxDesiredReceiveBitrate(cap_bitrate_bps); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - -TEST(PacketRouterRembTest, - SetMaxDesiredReceiveBitrateTriggersRembWhenNoMeasures) { - rtc::ScopedFakeClock clock; - PacketRouter packet_router; - clock.AdvanceTime(TimeDelta::Millis(1000)); - NiceMock remb_sender; - constexpr bool remb_candidate = true; - packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - - // Set cap. - EXPECT_CALL(remb_sender, SetRemb(100000, _)).Times(1); - packet_router.SetMaxDesiredReceiveBitrate(100000); - // Increase cap. - EXPECT_CALL(remb_sender, SetRemb(200000, _)).Times(1); - packet_router.SetMaxDesiredReceiveBitrate(200000); - // Decrease cap. - EXPECT_CALL(remb_sender, SetRemb(150000, _)).Times(1); - packet_router.SetMaxDesiredReceiveBitrate(150000); - - // Test tear-down. - packet_router.RemoveSendRtpModule(&remb_sender); -} - // Only register receiving modules and make sure we fallback to trigger a REMB // packet on this one. TEST(PacketRouterRembTest, NoSendingRtpModule) { @@ -729,18 +471,14 @@ TEST(PacketRouterRembTest, NoSendingRtpModule) { uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); - - // Call OnReceiveBitrateChanged twice to get a first estimate. - clock.AdvanceTime(TimeDelta::Millis(1000)); - EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Lower the estimate to trigger a new packet REMB packet. - EXPECT_CALL(rtp, SetRemb(bitrate_estimate - 100, ssrcs)).Times(1); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100); + EXPECT_CALL(rtp, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); - EXPECT_CALL(rtp, UnsetRemb()).Times(1); + EXPECT_CALL(rtp, UnsetRemb()); packet_router.RemoveReceiveRtpModule(&rtp); } @@ -756,8 +494,7 @@ TEST(PacketRouterRembTest, NonCandidateSendRtpModuleNotUsedForRemb) { constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; EXPECT_CALL(module, SetRemb(_, _)).Times(0); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveSendRtpModule(&module); @@ -774,9 +511,8 @@ TEST(PacketRouterRembTest, CandidateSendRtpModuleUsedForRemb) { constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - EXPECT_CALL(module, SetRemb(bitrate_estimate, ssrcs)).Times(1); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(module, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveSendRtpModule(&module); @@ -794,8 +530,7 @@ TEST(PacketRouterRembTest, NonCandidateReceiveRtpModuleNotUsedForRemb) { constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; EXPECT_CALL(module, SetRemb(_, _)).Times(0); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveReceiveRtpModule(&module); @@ -812,9 +547,8 @@ TEST(PacketRouterRembTest, CandidateReceiveRtpModuleUsedForRemb) { constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - EXPECT_CALL(module, SetRemb(bitrate_estimate, ssrcs)).Times(1); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(module, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveReceiveRtpModule(&module); @@ -837,11 +571,10 @@ TEST(PacketRouterRembTest, constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - EXPECT_CALL(send_module, SetRemb(bitrate_estimate, ssrcs)).Times(1); + EXPECT_CALL(send_module, SetRemb(bitrate_estimate, ssrcs)); EXPECT_CALL(receive_module, SetRemb(_, _)).Times(0); - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveReceiveRtpModule(&receive_module); @@ -865,11 +598,11 @@ TEST(PacketRouterRembTest, constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; - EXPECT_CALL(send_module, SetRemb(bitrate_estimate, ssrcs)).Times(1); + EXPECT_CALL(send_module, SetRemb(bitrate_estimate, ssrcs)); EXPECT_CALL(receive_module, SetRemb(_, _)).Times(0); clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveReceiveRtpModule(&receive_module); @@ -893,10 +626,8 @@ TEST(PacketRouterRembTest, ReceiveModuleTakesOverWhenLastSendModuleRemoved) { constexpr uint32_t bitrate_estimate = 456; const std::vector ssrcs = {1234}; EXPECT_CALL(send_module, SetRemb(_, _)).Times(0); - EXPECT_CALL(receive_module, SetRemb(bitrate_estimate, ssrcs)).Times(1); - - clock.AdvanceTime(TimeDelta::Millis(1000)); - packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate); + EXPECT_CALL(receive_module, SetRemb(bitrate_estimate, ssrcs)); + packet_router.SendRemb(bitrate_estimate, ssrcs); // Test tear-down packet_router.RemoveReceiveRtpModule(&receive_module); diff --git a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h index a9edfb3e1b..ac937bbfe0 100644 --- a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h +++ b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h @@ -38,15 +38,6 @@ class RemoteBitrateObserver { virtual ~RemoteBitrateObserver() {} }; -// TODO(bugs.webrtc.org/12693): Deprecate -class TransportFeedbackSenderInterface { - public: - virtual ~TransportFeedbackSenderInterface() = default; - - virtual bool SendCombinedRtcpPacket( - std::vector> packets) = 0; -}; - // TODO(holmer): Remove when all implementations have been updated. struct ReceiveBandwidthEstimatorStats {}; diff --git a/rtc_tools/BUILD.gn b/rtc_tools/BUILD.gn index 202095789c..fb8bbe9718 100644 --- a/rtc_tools/BUILD.gn +++ b/rtc_tools/BUILD.gn @@ -402,6 +402,7 @@ if (!build_with_chromium) { absl_deps = [ "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/base:core_headers", + "//third_party/abseil-cpp/absl/functional:bind_front", "//third_party/abseil-cpp/absl/strings", "//third_party/abseil-cpp/absl/types:optional", ] diff --git a/rtc_tools/rtc_event_log_visualizer/analyzer.cc b/rtc_tools/rtc_event_log_visualizer/analyzer.cc index d6acda9ec3..7690d7d6ea 100644 --- a/rtc_tools/rtc_event_log_visualizer/analyzer.cc +++ b/rtc_tools/rtc_event_log_visualizer/analyzer.cc @@ -19,6 +19,7 @@ #include #include "absl/algorithm/container.h" +#include "absl/functional/bind_front.h" #include "absl/strings/string_view.h" #include "api/function_view.h" #include "api/network_state_predictor.h" @@ -1367,13 +1368,11 @@ void EventLogAnalyzer::CreateSendSideBweSimulationGraph(Plot* plot) { void EventLogAnalyzer::CreateReceiveSideBweSimulationGraph(Plot* plot) { using RtpPacketType = LoggedRtpPacketIncoming; - class RembInterceptingPacketRouter : public PacketRouter { + class RembInterceptor { public: - void OnReceiveBitrateChanged(const std::vector& ssrcs, - uint32_t bitrate_bps) override { + void SendRemb(uint32_t bitrate_bps, std::vector ssrcs) { last_bitrate_bps_ = bitrate_bps; bitrate_updated_ = true; - PacketRouter::OnReceiveBitrateChanged(ssrcs, bitrate_bps); } uint32_t last_bitrate_bps() const { return last_bitrate_bps_; } bool GetAndResetBitrateUpdated() { @@ -1400,10 +1399,10 @@ void EventLogAnalyzer::CreateReceiveSideBweSimulationGraph(Plot* plot) { } SimulatedClock clock(0); - RembInterceptingPacketRouter packet_router; - // TODO(terelius): The PacketRouter is used as the RemoteBitrateObserver. - // Is this intentional? - ReceiveSideCongestionController rscc(&clock, &packet_router); + RembInterceptor remb_interceptor; + ReceiveSideCongestionController rscc( + &clock, [](auto...) {}, + absl::bind_front(&RembInterceptor::SendRemb, &remb_interceptor), nullptr); // TODO(holmer): Log the call config and use that here instead. // static const uint32_t kDefaultStartBitrateBps = 300000; // rscc.SetBweBitrates(0, kDefaultStartBitrateBps, -1); @@ -1428,9 +1427,9 @@ void EventLogAnalyzer::CreateReceiveSideBweSimulationGraph(Plot* plot) { float x = config_.GetCallTimeSec(clock.TimeInMicroseconds()); acked_time_series.points.emplace_back(x, y); } - if (packet_router.GetAndResetBitrateUpdated() || + if (remb_interceptor.GetAndResetBitrateUpdated() || clock.TimeInMicroseconds() - last_update_us >= 1e6) { - uint32_t y = packet_router.last_bitrate_bps() / 1000; + uint32_t y = remb_interceptor.last_bitrate_bps() / 1000; float x = config_.GetCallTimeSec(clock.TimeInMicroseconds()); time_series.points.emplace_back(x, y); last_update_us = clock.TimeInMicroseconds(); diff --git a/test/fuzzers/BUILD.gn b/test/fuzzers/BUILD.gn index 741c510cf5..d7afb65215 100644 --- a/test/fuzzers/BUILD.gn +++ b/test/fuzzers/BUILD.gn @@ -245,6 +245,7 @@ webrtc_fuzzer_test("congestion_controller_feedback_fuzzer") { "../../modules/remote_bitrate_estimator", "../../modules/rtp_rtcp:rtp_rtcp_format", ] + absl_deps = [ "//third_party/abseil-cpp/absl/functional:bind_front" ] } rtc_library("audio_decoder_fuzzer") { diff --git a/test/fuzzers/congestion_controller_feedback_fuzzer.cc b/test/fuzzers/congestion_controller_feedback_fuzzer.cc index 084c8c300a..06a73b0434 100644 --- a/test/fuzzers/congestion_controller_feedback_fuzzer.cc +++ b/test/fuzzers/congestion_controller_feedback_fuzzer.cc @@ -8,6 +8,7 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include "absl/functional/bind_front.h" #include "modules/congestion_controller/include/receive_side_congestion_controller.h" #include "modules/pacing/packet_router.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" @@ -21,7 +22,10 @@ void FuzzOneInput(const uint8_t* data, size_t size) { return; SimulatedClock clock(data[i++]); PacketRouter packet_router; - ReceiveSideCongestionController cc(&clock, &packet_router); + ReceiveSideCongestionController cc( + &clock, + absl::bind_front(&PacketRouter::SendCombinedRtcpPacket, &packet_router), + absl::bind_front(&PacketRouter::SendRemb, &packet_router), nullptr); RemoteBitrateEstimator* rbe = cc.GetRemoteBitrateEstimator(true); RTPHeader header; header.ssrc = ByteReader::ReadBigEndian(&data[i]);