From 5b69873cb5db4f91e428ff91ee332501fe6b4cf4 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Mon, 15 Apr 2019 12:36:33 +0200 Subject: [PATCH] Remove direct use of FieldTrials from AlrDetector Instead use WebRtcKeyValueConfig and FieldTrialBasedConfig. The purpose is to allow a user of GoogCC to use different settings on different instances. BUG=webrtc:10335 Change-Id: I2f837688c9fdd341eecb44484cc784b1c80da1a9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132791 Commit-Queue: Per Kjellander Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#27617} --- .../congestion_controller/goog_cc/BUILD.gn | 1 + .../goog_cc/alr_detector.cc | 11 +++++-- .../goog_cc/alr_detector.h | 6 ++-- .../goog_cc/alr_detector_unittest.cc | 7 +++-- .../goog_cc/goog_cc_network_control.cc | 2 +- modules/pacing/paced_sender.cc | 31 +++++++------------ modules/pacing/paced_sender.h | 7 ++--- rtc_base/experiments/BUILD.gn | 3 +- rtc_base/experiments/alr_experiment.cc | 23 +++++++++++--- rtc_base/experiments/alr_experiment.h | 6 ++++ 10 files changed, 58 insertions(+), 39 deletions(-) diff --git a/modules/congestion_controller/goog_cc/BUILD.gn b/modules/congestion_controller/goog_cc/BUILD.gn index d9749bc5d1..e21e1a112d 100644 --- a/modules/congestion_controller/goog_cc/BUILD.gn +++ b/modules/congestion_controller/goog_cc/BUILD.gn @@ -89,6 +89,7 @@ rtc_source_set("alr_detector") { "alr_detector.h", ] deps = [ + "../../../api/transport:field_trial_based_config", "../../../api/transport:webrtc_key_value_config", "../../../logging:rtc_event_log_api", "../../../logging:rtc_event_pacing", diff --git a/modules/congestion_controller/goog_cc/alr_detector.cc b/modules/congestion_controller/goog_cc/alr_detector.cc index 3009de44fd..2bcea3e3a6 100644 --- a/modules/congestion_controller/goog_cc/alr_detector.cc +++ b/modules/congestion_controller/goog_cc/alr_detector.cc @@ -23,20 +23,25 @@ #include "rtc_base/time_utils.h" namespace webrtc { -AlrDetector::AlrDetector() : AlrDetector(nullptr) {} -AlrDetector::AlrDetector(RtcEventLog* event_log) +AlrDetector::AlrDetector(const WebRtcKeyValueConfig* key_value_config) + : AlrDetector(key_value_config, nullptr) {} + +AlrDetector::AlrDetector(const WebRtcKeyValueConfig* key_value_config, + RtcEventLog* event_log) : bandwidth_usage_percent_(kDefaultAlrBandwidthUsagePercent), alr_start_budget_level_percent_(kDefaultAlrStartBudgetLevelPercent), alr_stop_budget_level_percent_(kDefaultAlrStopBudgetLevelPercent), alr_budget_(0, true), event_log_(event_log) { - RTC_CHECK(AlrExperimentSettings::MaxOneFieldTrialEnabled()); + RTC_CHECK(AlrExperimentSettings::MaxOneFieldTrialEnabled(*key_value_config)); absl::optional experiment_settings = AlrExperimentSettings::CreateFromFieldTrial( + *key_value_config, AlrExperimentSettings::kScreenshareProbingBweExperimentName); if (!experiment_settings) { experiment_settings = AlrExperimentSettings::CreateFromFieldTrial( + *key_value_config, AlrExperimentSettings::kStrictPacingAndProbingExperimentName); } if (experiment_settings) { diff --git a/modules/congestion_controller/goog_cc/alr_detector.h b/modules/congestion_controller/goog_cc/alr_detector.h index c30ba1d093..b58c92682b 100644 --- a/modules/congestion_controller/goog_cc/alr_detector.h +++ b/modules/congestion_controller/goog_cc/alr_detector.h @@ -15,6 +15,7 @@ #include #include "absl/types/optional.h" +#include "api/transport/webrtc_key_value_config.h" #include "modules/pacing/interval_budget.h" namespace webrtc { @@ -30,8 +31,9 @@ class RtcEventLog; // Note: This class is not thread-safe. class AlrDetector { public: - AlrDetector(); - explicit AlrDetector(RtcEventLog* event_log); + explicit AlrDetector(const WebRtcKeyValueConfig* key_value_config); + AlrDetector(const WebRtcKeyValueConfig* key_value_config, + RtcEventLog* event_log); ~AlrDetector(); void OnBytesSent(size_t bytes_sent, int64_t send_time_ms); diff --git a/modules/congestion_controller/goog_cc/alr_detector_unittest.cc b/modules/congestion_controller/goog_cc/alr_detector_unittest.cc index 2c8232c092..c1f96a1f8b 100644 --- a/modules/congestion_controller/goog_cc/alr_detector_unittest.cc +++ b/modules/congestion_controller/goog_cc/alr_detector_unittest.cc @@ -10,6 +10,7 @@ #include "modules/congestion_controller/goog_cc/alr_detector.h" +#include "api/transport/field_trial_based_config.h" #include "rtc_base/checks.h" #include "rtc_base/experiments/alr_experiment.h" #include "test/field_trial.h" @@ -71,11 +72,13 @@ class SimulateOutgoingTrafficIn { class AlrDetectorTest : public ::testing::Test { public: + AlrDetectorTest() : alr_detector_(&field_trials_) {} void SetUp() override { alr_detector_.SetEstimatedBitrate(kEstimatedBitrateBps); } protected: + FieldTrialBasedConfig field_trials_; AlrDetector alr_detector_; int64_t timestamp_ms_ = 1000; }; @@ -153,7 +156,7 @@ TEST_F(AlrDetectorTest, ParseControlFieldTrial) { "WebRTC-ProbingScreenshareBwe/Control/"); absl::optional parsed_params = AlrExperimentSettings::CreateFromFieldTrial( - "WebRTC-ProbingScreenshareBwe"); + FieldTrialBasedConfig(), "WebRTC-ProbingScreenshareBwe"); EXPECT_FALSE(static_cast(parsed_params)); } @@ -162,7 +165,7 @@ TEST_F(AlrDetectorTest, ParseActiveFieldTrial) { "WebRTC-ProbingScreenshareBwe/1.1,2875,85,20,-20,1/"); absl::optional parsed_params = AlrExperimentSettings::CreateFromFieldTrial( - "WebRTC-ProbingScreenshareBwe"); + FieldTrialBasedConfig(), "WebRTC-ProbingScreenshareBwe"); ASSERT_TRUE(static_cast(parsed_params)); EXPECT_EQ(1.1f, parsed_params->pacing_factor); EXPECT_EQ(2875, parsed_params->max_paced_queue_time); diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc index 06d94fe0d1..489f6a0f19 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -105,7 +105,7 @@ GoogCcNetworkController::GoogCcNetworkController( : nullptr), bandwidth_estimation_( absl::make_unique(event_log_)), - alr_detector_(absl::make_unique()), + alr_detector_(absl::make_unique(key_value_config_)), probe_bitrate_estimator_(new ProbeBitrateEstimator(event_log)), delay_based_bwe_(new DelayBasedBwe(key_value_config_, event_log_, diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index e6331f20f8..71302f1108 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -46,7 +46,6 @@ bool IsEnabled(const WebRtcKeyValueConfig& field_trials, } } // namespace - const int64_t PacedSender::kMaxQueueLengthMs = 2000; const float PacedSender::kDefaultPaceMultiplier = 2.5f; @@ -54,31 +53,23 @@ PacedSender::PacedSender(Clock* clock, PacketSender* packet_sender, RtcEventLog* event_log, const WebRtcKeyValueConfig* field_trials) - : PacedSender(clock, - packet_sender, - event_log, - field_trials - ? *field_trials - : static_cast( - FieldTrialBasedConfig())) {} - -PacedSender::PacedSender(Clock* clock, - PacketSender* packet_sender, - RtcEventLog* event_log, - const WebRtcKeyValueConfig& field_trials) : clock_(clock), packet_sender_(packet_sender), + fallback_field_trials_( + !field_trials ? absl::make_unique() : nullptr), + field_trials_(field_trials ? field_trials : fallback_field_trials_.get()), alr_detector_(), - drain_large_queues_(!IsDisabled(field_trials, "WebRTC-Pacer-DrainQueue")), + drain_large_queues_( + !IsDisabled(*field_trials_, "WebRTC-Pacer-DrainQueue")), send_padding_if_silent_( - IsEnabled(field_trials, "WebRTC-Pacer-PadInSilence")), - pace_audio_(!IsDisabled(field_trials, "WebRTC-Pacer-BlockAudio")), + IsEnabled(*field_trials_, "WebRTC-Pacer-PadInSilence")), + pace_audio_(!IsDisabled(*field_trials_, "WebRTC-Pacer-BlockAudio")), min_packet_limit_ms_("", kDefaultMinPacketLimitMs), last_timestamp_ms_(clock_->TimeInMilliseconds()), paused_(false), media_budget_(0), padding_budget_(0), - prober_(field_trials), + prober_(*field_trials_), probing_send_failure_(false), estimated_bitrate_bps_(0), min_send_bitrate_kbps_(0u), @@ -97,7 +88,7 @@ PacedSender::PacedSender(Clock* clock, "pushback experiment must be enabled."; } ParseFieldTrial({&min_packet_limit_ms_}, - field_trials.Lookup("WebRTC-Pacer-MinPacketLimitMs")); + field_trials_->Lookup("WebRTC-Pacer-MinPacketLimitMs")); UpdateBudgetWithElapsedTime(min_packet_limit_ms_); } @@ -184,7 +175,7 @@ void PacedSender::SetEstimatedBitrate(uint32_t bitrate_bps) { std::max(min_send_bitrate_kbps_, estimated_bitrate_bps_ / 1000) * pacing_factor_; if (!alr_detector_) - alr_detector_ = absl::make_unique(nullptr /*event_log*/); + alr_detector_ = absl::make_unique(field_trials_); alr_detector_->SetEstimatedBitrate(bitrate_bps); } @@ -248,7 +239,7 @@ int64_t PacedSender::ExpectedQueueTimeMs() const { absl::optional PacedSender::GetApplicationLimitedRegionStartTime() { rtc::CritScope cs(&critsect_); if (!alr_detector_) - alr_detector_ = absl::make_unique(nullptr /*event_log*/); + alr_detector_ = absl::make_unique(field_trials_); return alr_detector_->GetApplicationLimitedRegionStartTime(); } diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index a6aad24c11..8f9f4138ef 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -150,11 +150,6 @@ class PacedSender : public Pacer { void SetQueueTimeLimit(int limit_ms); private: - PacedSender(Clock* clock, - PacketSender* packet_sender, - RtcEventLog* event_log, - const WebRtcKeyValueConfig& field_trials); - int64_t UpdateTimeAndGetElapsedMs(int64_t now_us) RTC_EXCLUSIVE_LOCKS_REQUIRED(critsect_); bool ShouldSendKeepalive(int64_t at_time_us) const @@ -179,6 +174,8 @@ class PacedSender : public Pacer { Clock* const clock_; PacketSender* const packet_sender_; + const std::unique_ptr fallback_field_trials_; + const WebRtcKeyValueConfig* field_trials_; std::unique_ptr alr_detector_ RTC_PT_GUARDED_BY(critsect_); const bool drain_large_queues_; diff --git a/rtc_base/experiments/BUILD.gn b/rtc_base/experiments/BUILD.gn index 70dcb5bf36..fc9673a651 100644 --- a/rtc_base/experiments/BUILD.gn +++ b/rtc_base/experiments/BUILD.gn @@ -15,7 +15,8 @@ rtc_static_library("alr_experiment") { ] deps = [ "../:rtc_base_approved", - "../../system_wrappers:field_trial", + "../../api/transport:field_trial_based_config", + "../../api/transport:webrtc_key_value_config", "//third_party/abseil-cpp/absl/types:optional", ] } diff --git a/rtc_base/experiments/alr_experiment.cc b/rtc_base/experiments/alr_experiment.cc index 15a3438361..7bbca5c88b 100644 --- a/rtc_base/experiments/alr_experiment.cc +++ b/rtc_base/experiments/alr_experiment.cc @@ -14,8 +14,8 @@ #include #include +#include "api/transport/field_trial_based_config.h" #include "rtc_base/logging.h" -#include "system_wrappers/include/field_trial.h" namespace webrtc { @@ -26,16 +26,29 @@ const char AlrExperimentSettings::kStrictPacingAndProbingExperimentName[] = const char kDefaultProbingScreenshareBweSettings[] = "1.0,2875,80,40,-60,3"; bool AlrExperimentSettings::MaxOneFieldTrialEnabled() { - return field_trial::FindFullName(kStrictPacingAndProbingExperimentName) + return AlrExperimentSettings::MaxOneFieldTrialEnabled( + FieldTrialBasedConfig()); +} + +bool AlrExperimentSettings::MaxOneFieldTrialEnabled( + const WebRtcKeyValueConfig& key_value_config) { + return key_value_config.Lookup(kStrictPacingAndProbingExperimentName) .empty() || - field_trial::FindFullName(kScreenshareProbingBweExperimentName) - .empty(); + key_value_config.Lookup(kScreenshareProbingBweExperimentName).empty(); } absl::optional AlrExperimentSettings::CreateFromFieldTrial(const char* experiment_name) { + return AlrExperimentSettings::CreateFromFieldTrial(FieldTrialBasedConfig(), + experiment_name); +} + +absl::optional +AlrExperimentSettings::CreateFromFieldTrial( + const WebRtcKeyValueConfig& key_value_config, + const char* experiment_name) { absl::optional ret; - std::string group_name = field_trial::FindFullName(experiment_name); + std::string group_name = key_value_config.Lookup(experiment_name); const std::string kIgnoredSuffix = "_Dogfood"; std::string::size_type suffix_pos = group_name.rfind(kIgnoredSuffix); diff --git a/rtc_base/experiments/alr_experiment.h b/rtc_base/experiments/alr_experiment.h index 876bd02517..5b0661c5b4 100644 --- a/rtc_base/experiments/alr_experiment.h +++ b/rtc_base/experiments/alr_experiment.h @@ -14,6 +14,7 @@ #include #include "absl/types/optional.h" +#include "api/transport/webrtc_key_value_config.h" namespace webrtc { struct AlrExperimentSettings { @@ -32,7 +33,12 @@ struct AlrExperimentSettings { static const char kStrictPacingAndProbingExperimentName[]; static absl::optional CreateFromFieldTrial( const char* experiment_name); + static absl::optional CreateFromFieldTrial( + const WebRtcKeyValueConfig& key_value_config, + const char* experiment_name); static bool MaxOneFieldTrialEnabled(); + static bool MaxOneFieldTrialEnabled( + const WebRtcKeyValueConfig& key_value_config); private: AlrExperimentSettings() = default;