From f20ed3e8ad6fa5868878790367590c29396996f7 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 8 May 2024 12:49:40 +0200 Subject: [PATCH] Add option to provide Environment for CongestionConroller construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This would allow network controllers, GoogCcNetworkController in particular to have access to Environment at construction and thus it can rely on propagated field trials and won't need to fallback to the global field trial string Bug: webrtc:42220378 Change-Id: I36099522e3866a89a8c8d6303da03f7d5b1cad8e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/350201 Reviewed-by: Björn Terelius Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#42260} --- api/transport/BUILD.gn | 1 + api/transport/network_control.h | 7 ++++ call/rtp_transport_controller_send.cc | 3 +- .../congestion_controller/goog_cc/BUILD.gn | 2 ++ .../goog_cc_network_control_unittest.cc | 6 ++-- modules/congestion_controller/pcc/BUILD.gn | 1 + .../pcc/pcc_network_controller_unittest.cc | 33 +++++-------------- .../rtc_event_log_visualizer/analyzer.cc | 3 +- .../log_simulation.cc | 4 +-- 9 files changed, 28 insertions(+), 32 deletions(-) diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 9f89df5d80..756e1cc16f 100644 --- a/api/transport/BUILD.gn +++ b/api/transport/BUILD.gn @@ -39,6 +39,7 @@ rtc_library("network_control") { deps = [ "../../api:field_trials_view", + "../environment", "../rtc_event_log", "../units:data_rate", "../units:data_size", diff --git a/api/transport/network_control.h b/api/transport/network_control.h index 862322443d..3b76dff155 100644 --- a/api/transport/network_control.h +++ b/api/transport/network_control.h @@ -15,6 +15,7 @@ #include #include "absl/base/attributes.h" +#include "api/environment/environment.h" #include "api/field_trials_view.h" #include "api/rtc_event_log/rtc_event_log.h" #include "api/transport/network_types.h" @@ -35,6 +36,12 @@ class TargetTransferRateObserver { // Configuration sent to factory create function. The parameters here are // optional to use for a network controller implementation. struct NetworkControllerConfig { + // TODO: bugs.webrtc.org/42220378 - Delete the default constructor and + // thus make Environment (including field trials) a required parameter. + [[deprecated]] NetworkControllerConfig() = default; + explicit NetworkControllerConfig(const Environment& env) + : key_value_config(&env.field_trials()), event_log(&env.event_log()) {} + // The initial constraints to start with, these can be changed at any later // time by calls to OnTargetRateConstraints. Note that the starting rate // has to be set initially to provide a starting state for the network diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 6a168de924..0104d3d013 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -90,6 +90,7 @@ RtpTransportControllerSend::RtpTransportControllerSend( process_interval_(controller_factory_fallback_->GetProcessInterval()), last_report_block_time_( Timestamp::Millis(env_.clock().TimeInMilliseconds())), + initial_config_(env_), reset_feedback_on_route_change_( !env_.field_trials().IsEnabled("WebRTC-Bwe-NoFeedbackReset")), add_pacing_to_cwin_(env_.field_trials().IsEnabled( @@ -105,8 +106,6 @@ RtpTransportControllerSend::RtpTransportControllerSend( env_.field_trials().Lookup("WebRTC-Bwe-NetworkRouteConstraints")); initial_config_.constraints = ConvertConstraints(config.bitrate_config, &env_.clock()); - initial_config_.event_log = &env_.event_log(); - initial_config_.key_value_config = &env_.field_trials(); RTC_DCHECK(config.bitrate_config.start_bitrate_bps > 0); pacer_.SetPacingRates( diff --git a/modules/congestion_controller/goog_cc/BUILD.gn b/modules/congestion_controller/goog_cc/BUILD.gn index c017d39f5d..c6660cb409 100644 --- a/modules/congestion_controller/goog_cc/BUILD.gn +++ b/modules/congestion_controller/goog_cc/BUILD.gn @@ -339,6 +339,8 @@ if (rtc_include_tests) { ":send_side_bwe", "../../../api:field_trials_view", "../../../api:network_state_predictor_api", + "../../../api/environment", + "../../../api/environment:environment_factory", "../../../api/rtc_event_log", "../../../api/test/network_emulation", "../../../api/test/network_emulation:create_cross_traffic", diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc index c651f1376a..41aa3e50dd 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc @@ -19,6 +19,8 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "api/environment/environment.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/transport/goog_cc_factory.h" @@ -274,7 +276,7 @@ class NetworkControllerTestFixture { int starting_bandwidth_kbps = kInitialBitrateKbps, int min_data_rate_kbps = 0, int max_data_rate_kbps = 5 * kInitialBitrateKbps) { - NetworkControllerConfig config; + NetworkControllerConfig config(env_); config.constraints.at_time = Timestamp::Zero(); config.constraints.min_data_rate = DataRate::KilobitsPerSec(min_data_rate_kbps); @@ -282,11 +284,11 @@ class NetworkControllerTestFixture { DataRate::KilobitsPerSec(max_data_rate_kbps); config.constraints.starting_rate = DataRate::KilobitsPerSec(starting_bandwidth_kbps); - config.event_log = &event_log_; return config; } NiceMock event_log_; + const Environment env_ = CreateEnvironment(&event_log_); GoogCcNetworkControllerFactory factory_; }; diff --git a/modules/congestion_controller/pcc/BUILD.gn b/modules/congestion_controller/pcc/BUILD.gn index 85b12b3771..51e3b4fab1 100644 --- a/modules/congestion_controller/pcc/BUILD.gn +++ b/modules/congestion_controller/pcc/BUILD.gn @@ -111,6 +111,7 @@ if (rtc_include_tests && !build_with_chromium) { ":pcc_controller", ":rtt_tracker", ":utility_function", + "../../../api/environment:environment_factory", "../../../api/transport:network_control", "../../../api/units:data_rate", "../../../api/units:data_size", diff --git a/modules/congestion_controller/pcc/pcc_network_controller_unittest.cc b/modules/congestion_controller/pcc/pcc_network_controller_unittest.cc index f7cf2c45c9..cf4a884c85 100644 --- a/modules/congestion_controller/pcc/pcc_network_controller_unittest.cc +++ b/modules/congestion_controller/pcc/pcc_network_controller_unittest.cc @@ -12,6 +12,7 @@ #include +#include "api/environment/environment_factory.h" #include "modules/congestion_controller/pcc/pcc_factory.h" #include "test/gmock.h" #include "test/gtest.h" @@ -41,35 +42,19 @@ inline Matcher TargetRateCloseTo(DataRate rate) { AllOf(Ge(min_data_rate), Le(max_data_rate))); } -NetworkControllerConfig InitialConfig( - int starting_bandwidth_kbps = kInitialBitrate.kbps(), - int min_data_rate_kbps = 0, - int max_data_rate_kbps = 5 * kInitialBitrate.kbps()) { - NetworkControllerConfig config; - config.constraints.at_time = kDefaultStartTime; - config.constraints.min_data_rate = - DataRate::KilobitsPerSec(min_data_rate_kbps); - config.constraints.max_data_rate = - DataRate::KilobitsPerSec(max_data_rate_kbps); - config.constraints.starting_rate = - DataRate::KilobitsPerSec(starting_bandwidth_kbps); - return config; -} - -ProcessInterval InitialProcessInterval() { - ProcessInterval process_interval; - process_interval.at_time = kDefaultStartTime; - return process_interval; -} - } // namespace TEST(PccNetworkControllerTest, SendsConfigurationOnFirstProcess) { - std::unique_ptr controller_; - controller_.reset(new pcc::PccNetworkController(InitialConfig())); + Environment env = CreateEnvironment(); + NetworkControllerConfig config(env); + config.constraints.at_time = kDefaultStartTime; + config.constraints.min_data_rate = DataRate::Zero(); + config.constraints.max_data_rate = 5 * kInitialBitrate; + config.constraints.starting_rate = kInitialBitrate; + pcc::PccNetworkController controller(config); NetworkControlUpdate update = - controller_->OnProcessInterval(InitialProcessInterval()); + controller.OnProcessInterval({.at_time = kDefaultStartTime}); EXPECT_THAT(*update.target_rate, TargetRateCloseTo(kInitialBitrate)); EXPECT_THAT(*update.pacer_config, Property(&PacerConfig::data_rate, Ge(kInitialBitrate))); diff --git a/rtc_tools/rtc_event_log_visualizer/analyzer.cc b/rtc_tools/rtc_event_log_visualizer/analyzer.cc index 5ed4783082..527b86944d 100644 --- a/rtc_tools/rtc_event_log_visualizer/analyzer.cc +++ b/rtc_tools/rtc_event_log_visualizer/analyzer.cc @@ -1651,11 +1651,10 @@ void EventLogAnalyzer::CreateSendSideBweSimulationGraph(Plot* plot) { TimeDelta process_interval = factory.GetProcessInterval(); // TODO(holmer): Log the call config and use that here instead. static const uint32_t kDefaultStartBitrateBps = 300000; - NetworkControllerConfig cc_config; + NetworkControllerConfig cc_config(CreateEnvironment(&null_event_log)); cc_config.constraints.at_time = Timestamp::Micros(clock.TimeInMicroseconds()); cc_config.constraints.starting_rate = DataRate::BitsPerSec(kDefaultStartBitrateBps); - cc_config.event_log = &null_event_log; auto goog_cc = factory.Create(cc_config); TimeSeries time_series("Delay-based estimate", LineStyle::kStep, diff --git a/rtc_tools/rtc_event_log_visualizer/log_simulation.cc b/rtc_tools/rtc_event_log_visualizer/log_simulation.cc index a32c800b70..bfa0b4b51f 100644 --- a/rtc_tools/rtc_event_log_visualizer/log_simulation.cc +++ b/rtc_tools/rtc_event_log_visualizer/log_simulation.cc @@ -15,6 +15,7 @@ #include #include +#include "api/environment/environment_factory.h" #include "api/transport/network_control.h" #include "api/transport/network_types.h" #include "api/units/data_rate.h" @@ -46,11 +47,10 @@ void LogBasedNetworkControllerSimulation::HandleStateUpdate( void LogBasedNetworkControllerSimulation::ProcessUntil(Timestamp to_time) { if (last_process_.IsInfinite()) { - NetworkControllerConfig config; + NetworkControllerConfig config(CreateEnvironment(&null_event_log_)); config.constraints.at_time = to_time; config.constraints.min_data_rate = DataRate::KilobitsPerSec(30); config.constraints.starting_rate = DataRate::KilobitsPerSec(300); - config.event_log = &null_event_log_; controller_ = factory_->Create(config); } if (last_process_.IsInfinite() ||