From 494947bbcfdc5980835fbf523d2003ef246230c1 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Tue, 16 Apr 2019 14:50:08 +0200 Subject: [PATCH] Remove direct use of FieldTrials from modules/remote_bitrate_estimator Instead use WebRtcKeyValueConfig and FieldTrialBasedConfig BUG=webrtc:10335 Change-Id: Ie148cb466f86d8fa1ded5c7f125fbcccf6e7dbe3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132714 Commit-Queue: Per Kjellander Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#27642} --- .../goog_cc/delay_based_bwe.cc | 1 + modules/remote_bitrate_estimator/BUILD.gn | 3 ++ modules/remote_bitrate_estimator/DEPS | 2 ++ .../aimd_rate_control.cc | 28 ++++++++++++------- .../aimd_rate_control.h | 3 +- .../aimd_rate_control_unittest.cc | 4 ++- .../overuse_detector.cc | 25 +++++++++-------- .../overuse_detector.h | 8 ++++-- .../overuse_detector_unittest.cc | 11 ++++++-- .../remote_bitrate_estimator_abs_send_time.cc | 6 ++-- .../remote_bitrate_estimator_abs_send_time.h | 2 ++ .../remote_bitrate_estimator_single_stream.cc | 14 ++++++---- .../remote_bitrate_estimator_single_stream.h | 2 ++ 13 files changed, 73 insertions(+), 36 deletions(-) diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 450449f61d..665c11dc2b 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -90,6 +90,7 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, delay_detector_(), last_seen_packet_(Timestamp::MinusInfinity()), uma_recorded_(false), + rate_control_(key_value_config), trendline_window_size_( key_value_config->Lookup(kBweWindowSizeInPacketsExperiment) .find("Enabled") == 0 diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn index 7ce7cbaaff..20977b2619 100644 --- a/modules/remote_bitrate_estimator/BUILD.gn +++ b/modules/remote_bitrate_estimator/BUILD.gn @@ -43,7 +43,9 @@ rtc_static_library("remote_bitrate_estimator") { "../..:webrtc_common", "../../api:network_state_predictor_api", "../../api:rtp_headers", + "../../api/transport:field_trial_based_config", "../../api/transport:network_control", + "../../api/transport:webrtc_key_value_config", "../../api/units:data_rate", "../../api/units:timestamp", "../../modules:module_api", @@ -208,6 +210,7 @@ if (rtc_include_tests) { ":remote_bitrate_estimator", "..:module_api_public", "../..:webrtc_common", + "../../api/transport:field_trial_based_config", "../../rtc_base", "../../rtc_base:checks", "../../rtc_base:rtc_base_approved", diff --git a/modules/remote_bitrate_estimator/DEPS b/modules/remote_bitrate_estimator/DEPS index 8499c25613..66a3201bd0 100644 --- a/modules/remote_bitrate_estimator/DEPS +++ b/modules/remote_bitrate_estimator/DEPS @@ -1,4 +1,6 @@ include_rules = [ "+logging/rtc_event_log", "+system_wrappers", + # Avoid directly using field_trial. Instead use WebRtcKeyValueConfig. + "-system_wrappers/include/field_trial.h", ] diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc index 688b3b8b4e..90cda33e35 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -25,17 +25,23 @@ #include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_minmax.h" -#include "system_wrappers/include/field_trial.h" namespace webrtc { +namespace { + constexpr TimeDelta kDefaultRtt = TimeDelta::Millis<200>(); constexpr double kDefaultBackoffFactor = 0.85; const char kBweBackOffFactorExperiment[] = "WebRTC-BweBackOffFactor"; -double ReadBackoffFactor() { +bool IsEnabled(const WebRtcKeyValueConfig& field_trials, + absl::string_view key) { + return field_trials.Lookup(key).find("Enabled") == 0; +} + +double ReadBackoffFactor(const WebRtcKeyValueConfig& key_value_config) { std::string experiment_string = - webrtc::field_trial::FindFullName(kBweBackOffFactorExperiment); + key_value_config.Lookup(kBweBackOffFactorExperiment); double backoff_factor; int parsed_values = sscanf(experiment_string.c_str(), "Enabled-%lf", &backoff_factor); @@ -53,7 +59,9 @@ double ReadBackoffFactor() { return kDefaultBackoffFactor; } -AimdRateControl::AimdRateControl() +} // namespace + +AimdRateControl::AimdRateControl(const WebRtcKeyValueConfig* key_value_config) : min_configured_bitrate_(congestion_controller::GetMinBitrate()), max_configured_bitrate_(DataRate::kbps(30000)), current_bitrate_(max_configured_bitrate_), @@ -64,13 +72,13 @@ AimdRateControl::AimdRateControl() time_last_bitrate_decrease_(Timestamp::MinusInfinity()), time_first_throughput_estimate_(Timestamp::MinusInfinity()), bitrate_is_initialized_(false), - beta_(webrtc::field_trial::IsEnabled(kBweBackOffFactorExperiment) - ? ReadBackoffFactor() + beta_(IsEnabled(*key_value_config, kBweBackOffFactorExperiment) + ? ReadBackoffFactor(*key_value_config) : kDefaultBackoffFactor), rtt_(kDefaultRtt), - in_experiment_(!AdaptiveThresholdExperimentIsDisabled()), + in_experiment_(!AdaptiveThresholdExperimentIsDisabled(*key_value_config)), smoothing_experiment_( - webrtc::field_trial::IsEnabled("WebRTC-Audio-BandwidthSmoothing")), + IsEnabled(*key_value_config, "WebRTC-Audio-BandwidthSmoothing")), initial_backoff_interval_("initial_backoff_interval"), low_throughput_threshold_("low_throughput", DataRate::Zero()), capacity_deviation_ratio_threshold_("cap_thr", 0.2), @@ -80,7 +88,7 @@ AimdRateControl::AimdRateControl() // WebRTC-BweAimdRateControlConfig/initial_backoff_interval:100ms, // low_throughput:50kbps/ ParseFieldTrial({&initial_backoff_interval_, &low_throughput_threshold_}, - field_trial::FindFullName("WebRTC-BweAimdRateControlConfig")); + key_value_config->Lookup("WebRTC-BweAimdRateControlConfig")); if (initial_backoff_interval_) { RTC_LOG(LS_INFO) << "Using aimd rate control with initial back-off interval" << " " << ToString(*initial_backoff_interval_) << "."; @@ -89,7 +97,7 @@ AimdRateControl::AimdRateControl() ParseFieldTrial( {&capacity_deviation_ratio_threshold_, &cross_traffic_factor_, &capacity_limit_deviation_factor_}, - field_trial::FindFullName("WebRTC-Bwe-AimdRateControl-NetworkState")); + key_value_config->Lookup("WebRTC-Bwe-AimdRateControl-NetworkState")); } AimdRateControl::~AimdRateControl() {} diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.h b/modules/remote_bitrate_estimator/aimd_rate_control.h index dfefc485bf..4b51c9462d 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -15,6 +15,7 @@ #include "absl/types/optional.h" #include "api/transport/network_types.h" +#include "api/transport/webrtc_key_value_config.h" #include "api/units/data_rate.h" #include "api/units/timestamp.h" #include "modules/congestion_controller/goog_cc/link_capacity_estimator.h" @@ -29,7 +30,7 @@ namespace webrtc { // multiplicatively. class AimdRateControl { public: - AimdRateControl(); + explicit AimdRateControl(const WebRtcKeyValueConfig* key_value_config); ~AimdRateControl(); // Returns true if the target bitrate has been initialized. This happens diff --git a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc index 4bec9e8ff5..0c93ca86c4 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc @@ -9,6 +9,7 @@ */ #include +#include "api/transport/field_trial_based_config.h" #include "modules/remote_bitrate_estimator/aimd_rate_control.h" #include "system_wrappers/include/clock.h" #include "test/field_trial.h" @@ -32,11 +33,12 @@ constexpr double kFractionAfterOveruse = 0.85; struct AimdRateControlStates { std::unique_ptr aimd_rate_control; std::unique_ptr simulated_clock; + FieldTrialBasedConfig field_trials; }; AimdRateControlStates CreateAimdRateControlStates() { AimdRateControlStates states; - states.aimd_rate_control.reset(new AimdRateControl()); + states.aimd_rate_control.reset(new AimdRateControl(&states.field_trials)); states.simulated_clock.reset(new SimulatedClock(kClockInitialTime)); return states; } diff --git a/modules/remote_bitrate_estimator/overuse_detector.cc b/modules/remote_bitrate_estimator/overuse_detector.cc index ec4b77285e..6698c55632 100644 --- a/modules/remote_bitrate_estimator/overuse_detector.cc +++ b/modules/remote_bitrate_estimator/overuse_detector.cc @@ -19,7 +19,6 @@ #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h" #include "rtc_base/checks.h" #include "rtc_base/numerics/safe_minmax.h" -#include "system_wrappers/include/field_trial.h" namespace webrtc { @@ -33,9 +32,10 @@ const double kMaxAdaptOffsetMs = 15.0; const double kOverUsingTimeThreshold = 10; const int kMaxNumDeltas = 60; -bool AdaptiveThresholdExperimentIsDisabled() { +bool AdaptiveThresholdExperimentIsDisabled( + const WebRtcKeyValueConfig& key_value_config) { std::string experiment_string = - webrtc::field_trial::FindFullName(kAdaptiveThresholdExperiment); + key_value_config.Lookup(kAdaptiveThresholdExperiment); const size_t kMinExperimentLength = kDisabledPrefixLength; if (experiment_string.length() < kMinExperimentLength) return false; @@ -44,9 +44,11 @@ bool AdaptiveThresholdExperimentIsDisabled() { // Gets thresholds from the experiment name following the format // "WebRTC-AdaptiveBweThreshold/Enabled-0.5,0.002/". -bool ReadExperimentConstants(double* k_up, double* k_down) { +bool ReadExperimentConstants(const WebRtcKeyValueConfig& key_value_config, + double* k_up, + double* k_down) { std::string experiment_string = - webrtc::field_trial::FindFullName(kAdaptiveThresholdExperiment); + key_value_config.Lookup(kAdaptiveThresholdExperiment); const size_t kMinExperimentLength = kEnabledPrefixLength + 3; if (experiment_string.length() < kMinExperimentLength || experiment_string.substr(0, kEnabledPrefixLength) != kEnabledPrefix) @@ -55,10 +57,10 @@ bool ReadExperimentConstants(double* k_up, double* k_down) { "%lf,%lf", k_up, k_down) == 2; } -OveruseDetector::OveruseDetector() +OveruseDetector::OveruseDetector(const WebRtcKeyValueConfig* key_value_config) // Experiment is on by default, but can be disabled with finch by setting // the field trial string to "WebRTC-AdaptiveBweThreshold/Disabled/". - : in_experiment_(!AdaptiveThresholdExperimentIsDisabled()), + : in_experiment_(!AdaptiveThresholdExperimentIsDisabled(*key_value_config)), k_up_(0.0087), k_down_(0.039), overusing_time_threshold_(100), @@ -68,8 +70,8 @@ OveruseDetector::OveruseDetector() time_over_using_(-1), overuse_counter_(0), hypothesis_(BandwidthUsage::kBwNormal) { - if (!AdaptiveThresholdExperimentIsDisabled()) - InitializeExperiment(); + if (!AdaptiveThresholdExperimentIsDisabled(*key_value_config)) + InitializeExperiment(*key_value_config); } OveruseDetector::~OveruseDetector() {} @@ -144,12 +146,13 @@ void OveruseDetector::UpdateThreshold(double modified_offset, int64_t now_ms) { last_update_ms_ = now_ms; } -void OveruseDetector::InitializeExperiment() { +void OveruseDetector::InitializeExperiment( + const WebRtcKeyValueConfig& key_value_config) { RTC_DCHECK(in_experiment_); double k_up = 0.0; double k_down = 0.0; overusing_time_threshold_ = kOverUsingTimeThreshold; - if (ReadExperimentConstants(&k_up, &k_down)) { + if (ReadExperimentConstants(key_value_config, &k_up, &k_down)) { k_up_ = k_up; k_down_ = k_down; } diff --git a/modules/remote_bitrate_estimator/overuse_detector.h b/modules/remote_bitrate_estimator/overuse_detector.h index a72881b714..1df6cab786 100644 --- a/modules/remote_bitrate_estimator/overuse_detector.h +++ b/modules/remote_bitrate_estimator/overuse_detector.h @@ -12,16 +12,18 @@ #include +#include "api/transport/webrtc_key_value_config.h" #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "rtc_base/constructor_magic.h" namespace webrtc { -bool AdaptiveThresholdExperimentIsDisabled(); +bool AdaptiveThresholdExperimentIsDisabled( + const WebRtcKeyValueConfig& key_value_config); class OveruseDetector { public: - OveruseDetector(); + explicit OveruseDetector(const WebRtcKeyValueConfig* key_value_config); virtual ~OveruseDetector(); // Update the detection state based on the estimated inter-arrival time delta @@ -40,7 +42,7 @@ class OveruseDetector { private: void UpdateThreshold(double modified_offset, int64_t now_ms); - void InitializeExperiment(); + void InitializeExperiment(const WebRtcKeyValueConfig& key_value_config); bool in_experiment_; double k_up_; diff --git a/modules/remote_bitrate_estimator/overuse_detector_unittest.cc b/modules/remote_bitrate_estimator/overuse_detector_unittest.cc index 7fefa5c80b..91f9609c0a 100644 --- a/modules/remote_bitrate_estimator/overuse_detector_unittest.cc +++ b/modules/remote_bitrate_estimator/overuse_detector_unittest.cc @@ -14,6 +14,7 @@ #include #include +#include "api/transport/field_trial_based_config.h" #include "modules/remote_bitrate_estimator/inter_arrival.h" #include "modules/remote_bitrate_estimator/overuse_detector.h" #include "modules/remote_bitrate_estimator/overuse_estimator.h" @@ -38,7 +39,9 @@ class OveruseDetectorTest : public ::testing::Test { random_(123456789) {} protected: - void SetUp() override { overuse_detector_.reset(new OveruseDetector()); } + void SetUp() override { + overuse_detector_.reset(new OveruseDetector(&field_trials_)); + } int Run100000Samples(int packets_per_frame, size_t packet_size, @@ -107,6 +110,7 @@ class OveruseDetectorTest : public ::testing::Test { } } + const FieldTrialBasedConfig field_trials_; int64_t now_ms_; int64_t receive_time_ms_; uint32_t rtp_timestamp_; @@ -671,9 +675,12 @@ class OveruseDetectorExperimentTest : public OveruseDetectorTest { "WebRTC-AdaptiveBweThreshold/Enabled-0.01,0.00018/") {} protected: - void SetUp() override { overuse_detector_.reset(new OveruseDetector()); } + void SetUp() override { + overuse_detector_.reset(new OveruseDetector(&field_trials_)); + } test::ScopedFieldTrials override_field_trials_; + const FieldTrialBasedConfig field_trials_; }; TEST_F(OveruseDetectorExperimentTest, ThresholdAdapts) { diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index cb3a01ce0f..ed2d061c3a 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -14,6 +14,7 @@ #include +#include "api/transport/field_trial_based_config.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/checks.h" #include "rtc_base/constructor_magic.h" @@ -95,13 +96,14 @@ RemoteBitrateEstimatorAbsSendTime::RemoteBitrateEstimatorAbsSendTime( observer_(observer), inter_arrival_(), estimator_(), - detector_(), + detector_(&field_trials_), incoming_bitrate_(kBitrateWindowMs, 8000), incoming_bitrate_initialized_(false), total_probes_received_(0), first_packet_time_ms_(-1), last_update_ms_(-1), - uma_recorded_(false) { + uma_recorded_(false), + remote_rate_(&field_trials_) { RTC_DCHECK(clock_); RTC_DCHECK(observer_); RTC_LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating."; diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h index 5403fca336..02225a5c23 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h @@ -19,6 +19,7 @@ #include #include "api/rtp_headers.h" +#include "api/transport/field_trial_based_config.h" #include "modules/remote_bitrate_estimator/aimd_rate_control.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/remote_bitrate_estimator/inter_arrival.h" @@ -121,6 +122,7 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { rtc::RaceChecker network_race_; Clock* const clock_; + const FieldTrialBasedConfig field_trials_; RemoteBitrateObserver* const observer_; std::unique_ptr inter_arrival_; std::unique_ptr estimator_; diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index e5e25c97d9..aabf122c61 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -43,13 +43,14 @@ static const double kTimestampToMs = 1.0 / 90.0; struct RemoteBitrateEstimatorSingleStream::Detector { explicit Detector(int64_t last_packet_time_ms, const OverUseDetectorOptions& options, - bool enable_burst_grouping) + bool enable_burst_grouping, + const WebRtcKeyValueConfig* key_value_config) : last_packet_time_ms(last_packet_time_ms), inter_arrival(90 * kTimestampGroupLengthMs, kTimestampToMs, enable_burst_grouping), estimator(options), - detector() {} + detector(key_value_config) {} int64_t last_packet_time_ms; InterArrival inter_arrival; OveruseEstimator estimator; @@ -62,7 +63,7 @@ RemoteBitrateEstimatorSingleStream::RemoteBitrateEstimatorSingleStream( : clock_(clock), incoming_bitrate_(kBitrateWindowMs, 8000), last_valid_incoming_bitrate_(0), - remote_rate_(new AimdRateControl()), + remote_rate_(new AimdRateControl(&field_trials_)), observer_(observer), last_process_time_(-1), process_interval_ms_(kProcessIntervalMs), @@ -103,8 +104,9 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( // automatically cleaned up when we have one RemoteBitrateEstimator per REMB // group. std::pair insert_result = - overuse_detectors_.insert(std::make_pair( - ssrc, new Detector(now_ms, OverUseDetectorOptions(), true))); + overuse_detectors_.insert( + std::make_pair(ssrc, new Detector(now_ms, OverUseDetectorOptions(), + true, &field_trials_))); it = insert_result.first; } Detector* estimator = it->second; @@ -255,7 +257,7 @@ void RemoteBitrateEstimatorSingleStream::GetSsrcs( AimdRateControl* RemoteBitrateEstimatorSingleStream::GetRemoteRate() { if (!remote_rate_) - remote_rate_.reset(new AimdRateControl()); + remote_rate_.reset(new AimdRateControl(&field_trials_)); return remote_rate_.get(); } diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h index 3f05c45972..80129cec45 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h @@ -17,6 +17,7 @@ #include #include +#include "api/transport/field_trial_based_config.h" #include "modules/remote_bitrate_estimator/aimd_rate_control.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/constructor_magic.h" @@ -63,6 +64,7 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { AimdRateControl* GetRemoteRate() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); Clock* const clock_; + const FieldTrialBasedConfig field_trials_; SsrcOveruseEstimatorMap overuse_detectors_ RTC_GUARDED_BY(crit_sect_); RateStatistics incoming_bitrate_ RTC_GUARDED_BY(crit_sect_); uint32_t last_valid_incoming_bitrate_ RTC_GUARDED_BY(crit_sect_);