From 2ee83c17844b302a1b54a27c6340adea723a4ed9 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 6 May 2024 19:51:23 +0200 Subject: [PATCH] Provide Environment for ReceiveSideConfestionController construction Environment includes propagated field trials that can be later passed to RemoteBitrateEstimators member, and would allow not to rely on the global field trial string Bug: webrtc:42220378 Change-Id: Icf75a433c20352b2c22829c2148c92f69a2517aa Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349645 Commit-Queue: Danil Chapovalov Reviewed-by: Mirko Bonadei Cr-Commit-Position: refs/heads/main@{#42242} --- call/call.cc | 2 +- modules/congestion_controller/BUILD.gn | 3 +++ .../receive_side_congestion_controller.h | 10 +++++++++- .../receive_side_congestion_controller.cc | 17 +++++++++++++++++ ...ceive_side_congestion_controller_unittest.cc | 17 +++++++++-------- rtc_tools/BUILD.gn | 3 +++ rtc_tools/rtc_event_log_visualizer/analyzer.cc | 3 ++- test/fuzzers/BUILD.gn | 1 + ...receive_side_congestion_controller_fuzzer.cc | 3 ++- 9 files changed, 47 insertions(+), 12 deletions(-) diff --git a/call/call.cc b/call/call.cc index cca344a5a8..a6385a5170 100644 --- a/call/call.cc +++ b/call/call.cc @@ -666,7 +666,7 @@ Call::Call(const CallConfig& config, aggregate_network_up_(false), receive_stats_(&env_.clock()), send_stats_(&env_.clock()), - receive_side_cc_(&env_.clock(), + receive_side_cc_(env_, absl::bind_front(&PacketRouter::SendCombinedRtcpPacket, transport_send->packet_router()), absl::bind_front(&PacketRouter::SendRemb, diff --git a/modules/congestion_controller/BUILD.gn b/modules/congestion_controller/BUILD.gn index 9845754566..a3ff002c55 100644 --- a/modules/congestion_controller/BUILD.gn +++ b/modules/congestion_controller/BUILD.gn @@ -28,6 +28,7 @@ rtc_library("congestion_controller") { deps = [ "../../api:rtp_parameters", + "../../api/environment", "../../api/transport:network_control", "../../api/units:data_rate", "../../api/units:time_delta", @@ -39,6 +40,7 @@ rtc_library("congestion_controller") { "../remote_bitrate_estimator", "../rtp_rtcp:rtp_rtcp_format", ] + absl_deps = [ "//third_party/abseil-cpp/absl/base:nullability" ] } if (rtc_include_tests && !build_with_chromium) { @@ -51,6 +53,7 @@ if (rtc_include_tests && !build_with_chromium) { ] deps = [ ":congestion_controller", + "../../api/environment:environment_factory", "../../api/test/network_emulation", "../../api/test/network_emulation:create_cross_traffic", "../../api/units:data_rate", diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index 8d81ccbe69..b4cbfb6947 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -14,6 +14,8 @@ #include #include +#include "absl/base/nullability.h" +#include "api/environment/environment.h" #include "api/transport/network_control.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" @@ -35,12 +37,18 @@ class RemoteBitrateEstimator; class ReceiveSideCongestionController : public CallStatsObserver { public: ReceiveSideCongestionController( + const Environment& env, + RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, + RembThrottler::RembSender remb_sender, + absl::Nullable network_state_estimator); + + [[deprecated]] ReceiveSideCongestionController( Clock* clock, RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, RembThrottler::RembSender remb_sender, NetworkStateEstimator* network_state_estimator); - ~ReceiveSideCongestionController() override {} + ~ReceiveSideCongestionController() override = default; void OnReceivedPacket(const RtpPacketReceived& packet, MediaType media_type); diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index e042678897..3bcc82f973 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -10,6 +10,8 @@ #include "modules/congestion_controller/include/receive_side_congestion_controller.h" +#include "absl/base/nullability.h" +#include "api/environment/environment.h" #include "api/media_types.h" #include "api/units/data_rate.h" #include "modules/pacing/packet_router.h" @@ -68,6 +70,21 @@ void ReceiveSideCongestionController::PickEstimator( } } +ReceiveSideCongestionController::ReceiveSideCongestionController( + const Environment& env, + RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, + RembThrottler::RembSender remb_sender, + absl::Nullable network_state_estimator) + : clock_(env.clock()), + remb_throttler_(std::move(remb_sender), &clock_), + remote_estimator_proxy_(std::move(feedback_sender), + network_state_estimator), + rbe_( + std::make_unique(&remb_throttler_, + &clock_)), + using_absolute_send_time_(false), + packets_since_absolute_send_time_(0) {} + ReceiveSideCongestionController::ReceiveSideCongestionController( Clock* clock, RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, diff --git a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc index a0658476ca..5788898439 100644 --- a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc +++ b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc @@ -10,6 +10,7 @@ #include "modules/congestion_controller/include/receive_side_congestion_controller.h" +#include "api/environment/environment_factory.h" #include "api/test/network_emulation/create_cross_traffic.h" #include "api/test/network_emulation/cross_traffic.h" #include "api/units/data_rate.h" @@ -41,11 +42,11 @@ TEST(ReceiveSideCongestionControllerTest, SendsRembWithAbsSendTime) { MockFunction>)> feedback_sender; MockFunction)> remb_sender; - SimulatedClock clock_(123456); + SimulatedClock clock(123456); ReceiveSideCongestionController controller( - &clock_, feedback_sender.AsStdFunction(), remb_sender.AsStdFunction(), - nullptr); + CreateEnvironment(&clock), feedback_sender.AsStdFunction(), + remb_sender.AsStdFunction(), nullptr); RtpHeaderExtensionMap extensions; extensions.Register(1); @@ -58,8 +59,8 @@ TEST(ReceiveSideCongestionControllerTest, SendsRembWithAbsSendTime) { .Times(AtLeast(1)); for (int i = 0; i < 10; ++i) { - clock_.AdvanceTime(kPayloadSize / kInitialBitrate); - Timestamp now = clock_.CurrentTime(); + clock.AdvanceTime(kPayloadSize / kInitialBitrate); + Timestamp now = clock.CurrentTime(); packet.SetExtension(AbsoluteSendTime::To24Bits(now)); packet.set_arrival_time(now); controller.OnReceivedPacket(packet, MediaType::VIDEO); @@ -71,11 +72,11 @@ TEST(ReceiveSideCongestionControllerTest, MockFunction>)> feedback_sender; MockFunction)> remb_sender; - SimulatedClock clock_(123456); + SimulatedClock clock(123456); ReceiveSideCongestionController controller( - &clock_, feedback_sender.AsStdFunction(), remb_sender.AsStdFunction(), - nullptr); + CreateEnvironment(&clock), feedback_sender.AsStdFunction(), + remb_sender.AsStdFunction(), nullptr); EXPECT_CALL(remb_sender, Call(123, _)); controller.SetMaxDesiredReceiveBitrate(DataRate::BitsPerSec(123)); } diff --git a/rtc_tools/BUILD.gn b/rtc_tools/BUILD.gn index 35f36be7b7..521412a317 100644 --- a/rtc_tools/BUILD.gn +++ b/rtc_tools/BUILD.gn @@ -359,6 +359,7 @@ if (!build_with_chromium) { rtc_library("event_log_visualizer_utils") { visibility = [ "*" ] + allow_poison = [ "environment_construction" ] sources = [ "rtc_event_log_visualizer/alerts.cc", "rtc_event_log_visualizer/alerts.h", @@ -384,6 +385,7 @@ if (!build_with_chromium) { "../api:scoped_refptr", "../api/audio_codecs:audio_codecs_api", # TODO(kwiberg): Remove this # dependency. + "../api/environment:environment_factory", "../api/neteq:neteq_api", "../api/rtc_event_log:rtc_event_log", "../api/transport:field_trial_based_config", @@ -437,6 +439,7 @@ if (!build_with_chromium) { rtc_library("event_log_visualizer_bindings") { visibility = [ "*" ] + allow_poison = [ "environment_construction" ] sources = [ "rtc_event_log_visualizer/analyzer_bindings.cc", "rtc_event_log_visualizer/analyzer_bindings.h", diff --git a/rtc_tools/rtc_event_log_visualizer/analyzer.cc b/rtc_tools/rtc_event_log_visualizer/analyzer.cc index 899cc5b7ac..5ed4783082 100644 --- a/rtc_tools/rtc_event_log_visualizer/analyzer.cc +++ b/rtc_tools/rtc_event_log_visualizer/analyzer.cc @@ -28,6 +28,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "api/dtls_transport_interface.h" +#include "api/environment/environment_factory.h" #include "api/function_view.h" #include "api/media_types.h" #include "api/network_state_predictor.h" @@ -1851,7 +1852,7 @@ void EventLogAnalyzer::CreateReceiveSideBweSimulationGraph(Plot* plot) { SimulatedClock clock(0); RembInterceptor remb_interceptor; ReceiveSideCongestionController rscc( - &clock, [](auto...) {}, + CreateEnvironment(&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; diff --git a/test/fuzzers/BUILD.gn b/test/fuzzers/BUILD.gn index c667b64bed..d857d9cd21 100644 --- a/test/fuzzers/BUILD.gn +++ b/test/fuzzers/BUILD.gn @@ -278,6 +278,7 @@ webrtc_fuzzer_test("receive_side_congestion_controller_fuzzer") { sources = [ "receive_side_congestion_controller_fuzzer.cc" ] deps = [ "../../api:array_view", + "../../api/environment:environment_factory", "../../api/units:time_delta", "../../api/units:timestamp", "../../modules/congestion_controller", diff --git a/test/fuzzers/receive_side_congestion_controller_fuzzer.cc b/test/fuzzers/receive_side_congestion_controller_fuzzer.cc index 8f548c2b90..ff2c6b1c53 100644 --- a/test/fuzzers/receive_side_congestion_controller_fuzzer.cc +++ b/test/fuzzers/receive_side_congestion_controller_fuzzer.cc @@ -13,6 +13,7 @@ #include #include "api/array_view.h" +#include "api/environment/environment_factory.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "modules/congestion_controller/include/receive_side_congestion_controller.h" @@ -28,7 +29,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { Timestamp arrival_time = Timestamp::Micros(123'456'789); SimulatedClock clock(arrival_time); ReceiveSideCongestionController cc( - &clock, + CreateEnvironment(&clock), /*feedback_sender=*/[](auto...) {}, /*remb_sender=*/[](auto...) {}, /*network_state_estimator=*/nullptr);