Cleanup usage of FieldTrialsView by RateControlSettings and CongestionWindowPushbackController
Replace factory that takes optional FieldTrialView with a constructor that takes non-optional reference to the same interface - all callers already guarantee it is not nullptr
Replace several local IsEnabled/IsDisabled helpers with the same helpers in FieldTrialView
In CongestionWindowPushbackController tests pass field trials bypassing global field trial string
Bug: webrtc:42220378
Change-Id: Ic49ad78919d834a5e3b9b69545d3b39088023a75
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349900
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42270}
diff --git a/media/engine/simulcast_encoder_adapter.cc b/media/engine/simulcast_encoder_adapter.cc
index ab2947f..b4a6486 100644
--- a/media/engine/simulcast_encoder_adapter.cc
+++ b/media/engine/simulcast_encoder_adapter.cc
@@ -247,8 +247,7 @@
bypass_mode_(false),
encoded_complete_callback_(nullptr),
boost_base_layer_quality_(
- RateControlSettings::ParseFromKeyValueConfig(&env_.field_trials())
- .Vp8BoostBaseLayerQuality()),
+ RateControlSettings(env_.field_trials()).Vp8BoostBaseLayerQuality()),
prefer_temporal_support_on_base_layer_(env_.field_trials().IsEnabled(
"WebRTC-Video-PreferTemporalSupportOnBaseLayer")),
per_layer_pli_(SupportsPerLayerPictureLossIndication(format.parameters)),
diff --git a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc
index 8a5bf93..d3b791d 100644
--- a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc
+++ b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.cc
@@ -13,7 +13,6 @@
#include <algorithm>
#include <cstdint>
-#include "absl/strings/match.h"
#include "api/field_trials_view.h"
#include "api/units/data_size.h"
#include "rtc_base/experiments/rate_control_settings.h"
@@ -21,17 +20,14 @@
namespace webrtc {
CongestionWindowPushbackController::CongestionWindowPushbackController(
- const FieldTrialsView* key_value_config)
- : add_pacing_(
- absl::StartsWith(key_value_config->Lookup(
- "WebRTC-AddPacingToCongestionWindowPushback"),
- "Enabled")),
+ const FieldTrialsView& key_value_config)
+ : add_pacing_(key_value_config.IsEnabled(
+ "WebRTC-AddPacingToCongestionWindowPushback")),
min_pushback_target_bitrate_bps_(
- RateControlSettings::ParseFromKeyValueConfig(key_value_config)
+ RateControlSettings(key_value_config)
.CongestionWindowMinPushbackTargetBitrateBps()),
- current_data_window_(
- RateControlSettings::ParseFromKeyValueConfig(key_value_config)
- .CongestionWindowInitialDataWindow()) {}
+ current_data_window_(RateControlSettings(key_value_config)
+ .CongestionWindowInitialDataWindow()) {}
void CongestionWindowPushbackController::UpdateOutstandingData(
int64_t outstanding_bytes) {
diff --git a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h
index c18d9c6..a344fa1 100644
--- a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h
+++ b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller.h
@@ -27,7 +27,7 @@
class CongestionWindowPushbackController {
public:
explicit CongestionWindowPushbackController(
- const FieldTrialsView* key_value_config);
+ const FieldTrialsView& key_value_config);
void UpdateOutstandingData(int64_t outstanding_bytes);
void UpdatePacingQueue(int64_t pacing_bytes);
uint32_t UpdateTargetBitrate(uint32_t bitrate_bps);
diff --git a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller_unittest.cc b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller_unittest.cc
index c584c05..4299e05 100644
--- a/modules/congestion_controller/goog_cc/congestion_window_pushback_controller_unittest.cc
+++ b/modules/congestion_controller/goog_cc/congestion_window_pushback_controller_unittest.cc
@@ -13,93 +13,91 @@
#include <cstdint>
#include <memory>
-#include "api/transport/field_trial_based_config.h"
#include "api/units/data_size.h"
-#include "test/field_trial.h"
+#include "test/explicit_key_value_config.h"
#include "test/gmock.h"
#include "test/gtest.h"
-using ::testing::_;
-
namespace webrtc {
namespace test {
-class CongestionWindowPushbackControllerTest : public ::testing::Test {
- public:
- CongestionWindowPushbackControllerTest() {
- cwnd_controller_.reset(
- new CongestionWindowPushbackController(&field_trial_config_));
- }
+using ::testing::_;
- protected:
- FieldTrialBasedConfig field_trial_config_;
+TEST(CongestionWindowPushbackControllerTest, FullCongestionWindow) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig(""));
- std::unique_ptr<CongestionWindowPushbackController> cwnd_controller_;
-};
-
-TEST_F(CongestionWindowPushbackControllerTest, FullCongestionWindow) {
- cwnd_controller_->UpdateOutstandingData(100000);
- cwnd_controller_->SetDataWindow(DataSize::Bytes(50000));
+ cwnd_controller.UpdateOutstandingData(100000);
+ cwnd_controller.SetDataWindow(DataSize::Bytes(50000));
uint32_t bitrate_bps = 80000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(72000u, bitrate_bps);
- cwnd_controller_->SetDataWindow(DataSize::Bytes(50000));
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ cwnd_controller.SetDataWindow(DataSize::Bytes(50000));
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(static_cast<uint32_t>(72000 * 0.9 * 0.9), bitrate_bps);
}
-TEST_F(CongestionWindowPushbackControllerTest, NormalCongestionWindow) {
- cwnd_controller_->UpdateOutstandingData(199999);
- cwnd_controller_->SetDataWindow(DataSize::Bytes(200000));
+TEST(CongestionWindowPushbackControllerTest, NormalCongestionWindow) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig(""));
+
+ cwnd_controller.UpdateOutstandingData(199999);
+ cwnd_controller.SetDataWindow(DataSize::Bytes(200000));
uint32_t bitrate_bps = 80000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(80000u, bitrate_bps);
}
-TEST_F(CongestionWindowPushbackControllerTest, LowBitrate) {
- cwnd_controller_->UpdateOutstandingData(100000);
- cwnd_controller_->SetDataWindow(DataSize::Bytes(50000));
+TEST(CongestionWindowPushbackControllerTest, LowBitrate) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig(""));
+
+ cwnd_controller.UpdateOutstandingData(100000);
+ cwnd_controller.SetDataWindow(DataSize::Bytes(50000));
uint32_t bitrate_bps = 35000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(static_cast<uint32_t>(35000 * 0.9), bitrate_bps);
- cwnd_controller_->SetDataWindow(DataSize::Bytes(20000));
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ cwnd_controller.SetDataWindow(DataSize::Bytes(20000));
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(30000u, bitrate_bps);
}
-TEST_F(CongestionWindowPushbackControllerTest, NoPushbackOnDataWindowUnset) {
- cwnd_controller_->UpdateOutstandingData(1e8); // Large number
+TEST(CongestionWindowPushbackControllerTest, NoPushbackOnDataWindowUnset) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig(""));
+
+ cwnd_controller.UpdateOutstandingData(1e8); // Large number
uint32_t bitrate_bps = 80000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_EQ(80000u, bitrate_bps);
}
-TEST_F(CongestionWindowPushbackControllerTest, PushbackOnInititialDataWindow) {
- test::ScopedFieldTrials trials("WebRTC-CongestionWindow/InitWin:100000/");
- cwnd_controller_.reset(
- new CongestionWindowPushbackController(&field_trial_config_));
- cwnd_controller_->UpdateOutstandingData(1e8); // Large number
+TEST(CongestionWindowPushbackControllerTest, PushbackOnInititialDataWindow) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig("WebRTC-CongestionWindow/InitWin:100000/"));
+
+ cwnd_controller.UpdateOutstandingData(1e8); // Large number
uint32_t bitrate_bps = 80000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_GT(80000u, bitrate_bps);
}
-TEST_F(CongestionWindowPushbackControllerTest, PushbackDropFrame) {
- test::ScopedFieldTrials trials("WebRTC-CongestionWindow/DropFrame:true/");
- cwnd_controller_.reset(
- new CongestionWindowPushbackController(&field_trial_config_));
- cwnd_controller_->UpdateOutstandingData(1e8); // Large number
- cwnd_controller_->SetDataWindow(DataSize::Bytes(50000));
+TEST(CongestionWindowPushbackControllerTest, PushbackDropFrame) {
+ CongestionWindowPushbackController cwnd_controller(
+ ExplicitKeyValueConfig("WebRTC-CongestionWindow/DropFrame:true/"));
+
+ cwnd_controller.UpdateOutstandingData(1e8); // Large number
+ cwnd_controller.SetDataWindow(DataSize::Bytes(50000));
uint32_t bitrate_bps = 80000;
- bitrate_bps = cwnd_controller_->UpdateTargetBitrate(bitrate_bps);
+ bitrate_bps = cwnd_controller.UpdateTargetBitrate(bitrate_bps);
EXPECT_GT(80000u, bitrate_bps);
}
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 e485f73..7f958c9 100644
--- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
+++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
@@ -65,14 +65,6 @@
// below the current throughput estimate to drain the network queues.
constexpr double kProbeDropThroughputFraction = 0.85;
-bool IsEnabled(const FieldTrialsView* config, absl::string_view key) {
- return absl::StartsWith(config->Lookup(key), "Enabled");
-}
-
-bool IsNotDisabled(const FieldTrialsView* config, absl::string_view key) {
- return !absl::StartsWith(config->Lookup(key), "Disabled");
-}
-
BandwidthLimitedCause GetBandwidthLimitedCause(LossBasedState loss_based_state,
bool is_rtt_above_limit,
BandwidthUsage bandwidth_usage) {
@@ -108,27 +100,24 @@
safe_reset_on_route_change_("Enabled"),
safe_reset_acknowledged_rate_("ack"),
use_min_allocatable_as_lower_bound_(
- IsNotDisabled(key_value_config_, "WebRTC-Bwe-MinAllocAsLowerBound")),
- ignore_probes_lower_than_network_estimate_(IsNotDisabled(
- key_value_config_,
+ !key_value_config_->IsDisabled("WebRTC-Bwe-MinAllocAsLowerBound")),
+ ignore_probes_lower_than_network_estimate_(!key_value_config_->IsDisabled(
"WebRTC-Bwe-IgnoreProbesLowerThanNetworkStateEstimate")),
limit_probes_lower_than_throughput_estimate_(
- IsNotDisabled(key_value_config_,
- "WebRTC-Bwe-LimitProbesLowerThanThroughputEstimate")),
- rate_control_settings_(
- RateControlSettings::ParseFromKeyValueConfig(key_value_config_)),
- pace_at_max_of_bwe_and_lower_link_capacity_(
- IsEnabled(key_value_config_,
- "WebRTC-Bwe-PaceAtMaxOfBweAndLowerLinkCapacity")),
+ !key_value_config_->IsDisabled(
+ "WebRTC-Bwe-LimitProbesLowerThanThroughputEstimate")),
+ rate_control_settings_(*key_value_config_),
+ pace_at_max_of_bwe_and_lower_link_capacity_(key_value_config_->IsEnabled(
+ "WebRTC-Bwe-PaceAtMaxOfBweAndLowerLinkCapacity")),
limit_pacingfactor_by_upper_link_capacity_estimate_(
- IsEnabled(key_value_config_,
- "WebRTC-Bwe-LimitPacingFactorByUpperLinkCapacityEstimate")),
+ key_value_config_->IsEnabled(
+ "WebRTC-Bwe-LimitPacingFactorByUpperLinkCapacityEstimate")),
probe_controller_(
new ProbeController(key_value_config_, config.event_log)),
congestion_window_pushback_controller_(
rate_control_settings_.UseCongestionWindowPushback()
? std::make_unique<CongestionWindowPushbackController>(
- key_value_config_)
+ *key_value_config_)
: nullptr),
bandwidth_estimation_(
std::make_unique<SendSideBandwidthEstimation>(key_value_config_,
diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
index be3ccc3..a5036cf 100644
--- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
+++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
@@ -309,8 +309,7 @@
std::unique_ptr<LibvpxInterface> interface)
: env_(env),
libvpx_(std::move(interface)),
- rate_control_settings_(
- RateControlSettings::ParseFromKeyValueConfig(&env_.field_trials())),
+ rate_control_settings_(env_.field_trials()),
resolution_bitrate_limits_(std::move(settings.resolution_bitrate_limits)),
key_frame_request_(kMaxSimulcastStreams, false),
last_encoder_output_time_(kMaxSimulcastStreams,
diff --git a/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc b/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc
index 5a6d465..21d4602 100644
--- a/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc
+++ b/modules/video_coding/codecs/vp9/libvpx_vp9_encoder.cc
@@ -250,9 +250,8 @@
is_svc_(false),
inter_layer_pred_(InterLayerPredMode::kOn),
external_ref_control_(false), // Set in InitEncode because of tests.
- trusted_rate_controller_(
- RateControlSettings::ParseFromKeyValueConfig(&env.field_trials())
- .LibvpxVp9TrustedRateController()),
+ trusted_rate_controller_(RateControlSettings(env.field_trials())
+ .LibvpxVp9TrustedRateController()),
first_frame_in_picture_(true),
ss_info_needed_(false),
force_all_active_layers_(false),
diff --git a/rtc_base/experiments/rate_control_settings.cc b/rtc_base/experiments/rate_control_settings.cc
index 84e7b1b..b81432d8 100644
--- a/rtc_base/experiments/rate_control_settings.cc
+++ b/rtc_base/experiments/rate_control_settings.cc
@@ -34,11 +34,6 @@
const char kUseBaseHeavyVp8Tl3RateAllocationFieldTrialName[] =
"WebRTC-UseBaseHeavyVP8TL3RateAllocation";
-bool IsEnabled(const FieldTrialsView* const key_value_config,
- absl::string_view key) {
- return absl::StartsWith(key_value_config->Lookup(key), "Enabled");
-}
-
} // namespace
constexpr char CongestionWindowConfig::kKey[];
@@ -75,32 +70,25 @@
}
RateControlSettings::RateControlSettings(
- const FieldTrialsView* const key_value_config) {
+ const FieldTrialsView& key_value_config) {
std::string congestion_window_config =
- key_value_config->Lookup(CongestionWindowConfig::kKey).empty()
- ? kCongestionWindowDefaultFieldTrialString
- : key_value_config->Lookup(CongestionWindowConfig::kKey);
+ key_value_config.Lookup(CongestionWindowConfig::kKey);
+ if (congestion_window_config.empty()) {
+ congestion_window_config = kCongestionWindowDefaultFieldTrialString;
+ }
congestion_window_config_ =
CongestionWindowConfig::Parse(congestion_window_config);
- video_config_.vp8_base_heavy_tl3_alloc = IsEnabled(
- key_value_config, kUseBaseHeavyVp8Tl3RateAllocationFieldTrialName);
+ video_config_.vp8_base_heavy_tl3_alloc = key_value_config.IsEnabled(
+ kUseBaseHeavyVp8Tl3RateAllocationFieldTrialName);
video_config_.Parser()->Parse(
- key_value_config->Lookup(VideoRateControlConfig::kKey));
+ key_value_config.Lookup(VideoRateControlConfig::kKey));
}
RateControlSettings::~RateControlSettings() = default;
RateControlSettings::RateControlSettings(RateControlSettings&&) = default;
RateControlSettings RateControlSettings::ParseFromFieldTrials() {
- FieldTrialBasedConfig field_trial_config;
- return RateControlSettings(&field_trial_config);
-}
-
-RateControlSettings RateControlSettings::ParseFromKeyValueConfig(
- const FieldTrialsView* const key_value_config) {
- FieldTrialBasedConfig field_trial_config;
- return RateControlSettings(key_value_config ? key_value_config
- : &field_trial_config);
+ return RateControlSettings(FieldTrialBasedConfig());
}
bool RateControlSettings::UseCongestionWindow() const {
diff --git a/rtc_base/experiments/rate_control_settings.h b/rtc_base/experiments/rate_control_settings.h
index 05e942d..ab9e1ed 100644
--- a/rtc_base/experiments/rate_control_settings.h
+++ b/rtc_base/experiments/rate_control_settings.h
@@ -48,12 +48,11 @@
class RateControlSettings final {
public:
- ~RateControlSettings();
+ explicit RateControlSettings(const FieldTrialsView& key_value_config);
RateControlSettings(RateControlSettings&&);
+ ~RateControlSettings();
static RateControlSettings ParseFromFieldTrials();
- static RateControlSettings ParseFromKeyValueConfig(
- const FieldTrialsView* const key_value_config);
// When CongestionWindowPushback is enabled, the pacer is oblivious to
// the congestion window. The relation between outstanding data and
@@ -82,8 +81,6 @@
bool BitrateAdjusterCanUseNetworkHeadroom() const;
private:
- explicit RateControlSettings(const FieldTrialsView* const key_value_config);
-
CongestionWindowConfig congestion_window_config_;
VideoRateControlConfig video_config_;
};
diff --git a/video/config/simulcast.cc b/video/config/simulcast.cc
index 42d644e..8373341 100644
--- a/video/config/simulcast.cc
+++ b/video/config/simulcast.cc
@@ -381,8 +381,7 @@
RTC_DCHECK(max_layers > 1 || is_screenshare_with_conference_mode);
const bool base_heavy_tl3_rate_alloc =
- webrtc::RateControlSettings::ParseFromKeyValueConfig(&trials)
- .Vp8BaseHeavyTl3RateAllocation();
+ webrtc::RateControlSettings(trials).Vp8BaseHeavyTl3RateAllocation();
if (is_screenshare_with_conference_mode) {
return GetScreenshareLayers(max_layers, width, height, bitrate_priority,
max_qp, temporal_layers_supported,
diff --git a/video/encoder_bitrate_adjuster.cc b/video/encoder_bitrate_adjuster.cc
index 02468cb..0932607 100644
--- a/video/encoder_bitrate_adjuster.cc
+++ b/video/encoder_bitrate_adjuster.cc
@@ -47,9 +47,8 @@
EncoderBitrateAdjuster::EncoderBitrateAdjuster(
const VideoCodec& codec_settings,
const FieldTrialsView& field_trials)
- : utilize_bandwidth_headroom_(
- RateControlSettings::ParseFromKeyValueConfig(&field_trials)
- .BitrateAdjusterCanUseNetworkHeadroom()),
+ : utilize_bandwidth_headroom_(RateControlSettings(field_trials)
+ .BitrateAdjusterCanUseNetworkHeadroom()),
frames_since_layout_change_(0),
min_bitrates_bps_{},
frame_size_pixels_{},
diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc
index 142827d..e4ee2e5 100644
--- a/video/video_send_stream_impl.cc
+++ b/video/video_send_stream_impl.cc
@@ -221,10 +221,9 @@
if (alr_settings)
return alr_settings->pacing_factor;
- RateControlSettings rate_control_settings =
- RateControlSettings::ParseFromKeyValueConfig(&field_trials);
- return rate_control_settings.GetPacingFactor().value_or(
- default_pacing_config.pacing_factor);
+ return RateControlSettings(field_trials)
+ .GetPacingFactor()
+ .value_or(default_pacing_config.pacing_factor);
}
int GetEncoderPriorityBitrate(std::string codec_name,
@@ -485,9 +484,8 @@
enable_alr_bw_probing = true;
queue_time_limit_ms = alr_settings->max_paced_queue_time;
} else {
- RateControlSettings rate_control_settings =
- RateControlSettings::ParseFromKeyValueConfig(&env_.field_trials());
- enable_alr_bw_probing = rate_control_settings.UseAlrProbing();
+ enable_alr_bw_probing =
+ RateControlSettings(env_.field_trials()).UseAlrProbing();
queue_time_limit_ms = pacing_config_.max_pacing_delay.Get().ms();
}
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index efc5df0..8ae14f1 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -660,8 +660,7 @@
number_of_cores_(number_of_cores),
settings_(settings),
allocation_cb_type_(allocation_cb_type),
- rate_control_settings_(
- RateControlSettings::ParseFromKeyValueConfig(&env_.field_trials())),
+ rate_control_settings_(env_.field_trials()),
encoder_selector_from_constructor_(encoder_selector),
encoder_selector_from_factory_(
encoder_selector_from_constructor_
