From fe2063ebc711db718b24254ff0d11a6c9385f766 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Wed, 12 May 2021 09:02:43 +0200 Subject: [PATCH] Remove REMB throttling funcionality from PacketRouter This removes PacketRouter inheritance from RemoteBitrateObserver and TransportFeedbackSenderInterface. Call binds methods for sending REMB and transport feedback messages from RemoteCongestionController to PacketRouter. This is needed until the RTCPTranseiver is used instead of the RTP modules. Bug: webrtc:12693 Change-Id: I7088de497cd6d1e15c98788ff3e6b0a2c8897ea8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215965 Reviewed-by: Harald Alvestrand Reviewed-by: Mirko Bonadei Reviewed-by: Danil Chapovalov Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/master@{#33993} --- DEPS | 1 + abseil-in-webrtc.md | 1 + call/BUILD.gn | 10 +- call/call.cc | 8 +- .../receive_side_congestion_controller.h | 8 - .../receive_side_congestion_controller.cc | 20 -- modules/pacing/packet_router.cc | 80 +---- modules/pacing/packet_router.h | 32 +- modules/pacing/packet_router_unittest.cc | 325 ++---------------- .../include/remote_bitrate_estimator.h | 9 - rtc_tools/BUILD.gn | 1 + .../rtc_event_log_visualizer/analyzer.cc | 19 +- test/fuzzers/BUILD.gn | 1 + .../congestion_controller_feedback_fuzzer.cc | 6 +- 14 files changed, 71 insertions(+), 450 deletions(-) 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]);