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 <perkj@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27617}diff --git a/modules/congestion_controller/goog_cc/BUILD.gn b/modules/congestion_controller/goog_cc/BUILD.gn
index d9749bc..e21e1a1 100644
--- a/modules/congestion_controller/goog_cc/BUILD.gn
+++ b/modules/congestion_controller/goog_cc/BUILD.gn
@@ -89,6 +89,7 @@
"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 3009de4..2bcea3e 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<AlrExperimentSettings> 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 c30ba1d..b58c926 100644
--- a/modules/congestion_controller/goog_cc/alr_detector.h
+++ b/modules/congestion_controller/goog_cc/alr_detector.h
@@ -15,6 +15,7 @@
#include <stdint.h>
#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 @@
// 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 2c8232c..c1f96a1 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 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 @@
"WebRTC-ProbingScreenshareBwe/Control/");
absl::optional<AlrExperimentSettings> parsed_params =
AlrExperimentSettings::CreateFromFieldTrial(
- "WebRTC-ProbingScreenshareBwe");
+ FieldTrialBasedConfig(), "WebRTC-ProbingScreenshareBwe");
EXPECT_FALSE(static_cast<bool>(parsed_params));
}
@@ -162,7 +165,7 @@
"WebRTC-ProbingScreenshareBwe/1.1,2875,85,20,-20,1/");
absl::optional<AlrExperimentSettings> parsed_params =
AlrExperimentSettings::CreateFromFieldTrial(
- "WebRTC-ProbingScreenshareBwe");
+ FieldTrialBasedConfig(), "WebRTC-ProbingScreenshareBwe");
ASSERT_TRUE(static_cast<bool>(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 06d94fe..489f6a0 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 @@
: nullptr),
bandwidth_estimation_(
absl::make_unique<SendSideBandwidthEstimation>(event_log_)),
- alr_detector_(absl::make_unique<AlrDetector>()),
+ alr_detector_(absl::make_unique<AlrDetector>(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 e6331f2..71302f1 100644
--- a/modules/pacing/paced_sender.cc
+++ b/modules/pacing/paced_sender.cc
@@ -46,7 +46,6 @@
}
} // namespace
-
const int64_t PacedSender::kMaxQueueLengthMs = 2000;
const float PacedSender::kDefaultPaceMultiplier = 2.5f;
@@ -54,31 +53,23 @@
PacketSender* packet_sender,
RtcEventLog* event_log,
const WebRtcKeyValueConfig* field_trials)
- : PacedSender(clock,
- packet_sender,
- event_log,
- field_trials
- ? *field_trials
- : static_cast<const webrtc::WebRtcKeyValueConfig&>(
- 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<FieldTrialBasedConfig>() : 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 @@
"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 @@
std::max(min_send_bitrate_kbps_, estimated_bitrate_bps_ / 1000) *
pacing_factor_;
if (!alr_detector_)
- alr_detector_ = absl::make_unique<AlrDetector>(nullptr /*event_log*/);
+ alr_detector_ = absl::make_unique<AlrDetector>(field_trials_);
alr_detector_->SetEstimatedBitrate(bitrate_bps);
}
@@ -248,7 +239,7 @@
absl::optional<int64_t> PacedSender::GetApplicationLimitedRegionStartTime() {
rtc::CritScope cs(&critsect_);
if (!alr_detector_)
- alr_detector_ = absl::make_unique<AlrDetector>(nullptr /*event_log*/);
+ alr_detector_ = absl::make_unique<AlrDetector>(field_trials_);
return alr_detector_->GetApplicationLimitedRegionStartTime();
}
diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h
index a6aad24..8f9f413 100644
--- a/modules/pacing/paced_sender.h
+++ b/modules/pacing/paced_sender.h
@@ -150,11 +150,6 @@
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 @@
Clock* const clock_;
PacketSender* const packet_sender_;
+ const std::unique_ptr<FieldTrialBasedConfig> fallback_field_trials_;
+ const WebRtcKeyValueConfig* field_trials_;
std::unique_ptr<AlrDetector> 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 70dcb5b..fc9673a 100644
--- a/rtc_base/experiments/BUILD.gn
+++ b/rtc_base/experiments/BUILD.gn
@@ -15,7 +15,8 @@
]
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 15a3438..7bbca5c 100644
--- a/rtc_base/experiments/alr_experiment.cc
+++ b/rtc_base/experiments/alr_experiment.cc
@@ -14,8 +14,8 @@
#include <stdio.h>
#include <string>
+#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 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>
AlrExperimentSettings::CreateFromFieldTrial(const char* experiment_name) {
+ return AlrExperimentSettings::CreateFromFieldTrial(FieldTrialBasedConfig(),
+ experiment_name);
+}
+
+absl::optional<AlrExperimentSettings>
+AlrExperimentSettings::CreateFromFieldTrial(
+ const WebRtcKeyValueConfig& key_value_config,
+ const char* experiment_name) {
absl::optional<AlrExperimentSettings> 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 876bd02..5b0661c 100644
--- a/rtc_base/experiments/alr_experiment.h
+++ b/rtc_base/experiments/alr_experiment.h
@@ -14,6 +14,7 @@
#include <stdint.h>
#include "absl/types/optional.h"
+#include "api/transport/webrtc_key_value_config.h"
namespace webrtc {
struct AlrExperimentSettings {
@@ -32,7 +33,12 @@
static const char kStrictPacingAndProbingExperimentName[];
static absl::optional<AlrExperimentSettings> CreateFromFieldTrial(
const char* experiment_name);
+ static absl::optional<AlrExperimentSettings> CreateFromFieldTrial(
+ const WebRtcKeyValueConfig& key_value_config,
+ const char* experiment_name);
static bool MaxOneFieldTrialEnabled();
+ static bool MaxOneFieldTrialEnabled(
+ const WebRtcKeyValueConfig& key_value_config);
private:
AlrExperimentSettings() = default;
