From f317f7106a7a15a04da7cd30c2e2ddb1b3025bc6 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 17 May 2024 16:31:00 +0200 Subject: [PATCH] Remove option to parse RateControlSettings from the global field trial string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:42220378 Change-Id: Iff016f0f53f427ff59df816d8d87dc4a8119db65 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/350921 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#42348} --- .../utility/simulcast_rate_allocator.cc | 6 - .../utility/simulcast_rate_allocator.h | 1 - rtc_base/experiments/BUILD.gn | 2 - rtc_base/experiments/rate_control_settings.cc | 5 - rtc_base/experiments/rate_control_settings.h | 2 - .../rate_control_settings_unittest.cc | 168 +++++++----------- video/video_stream_encoder_unittest.cc | 3 +- 7 files changed, 65 insertions(+), 122 deletions(-) diff --git a/modules/video_coding/utility/simulcast_rate_allocator.cc b/modules/video_coding/utility/simulcast_rate_allocator.cc index 3ef4d43a5e..89522e6ca3 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator.cc +++ b/modules/video_coding/utility/simulcast_rate_allocator.cc @@ -58,12 +58,6 @@ float SimulcastRateAllocator::GetTemporalRateAllocation( return kLayerRateAllocation[num_layers - 1][temporal_id]; } -SimulcastRateAllocator::SimulcastRateAllocator(const VideoCodec& codec) - : codec_(codec), - stable_rate_settings_(StableTargetRateExperiment::ParseFromFieldTrials()), - rate_control_settings_(RateControlSettings::ParseFromFieldTrials()), - legacy_conference_mode_(false) {} - SimulcastRateAllocator::SimulcastRateAllocator(const Environment& env, const VideoCodec& codec) : codec_(codec), diff --git a/modules/video_coding/utility/simulcast_rate_allocator.h b/modules/video_coding/utility/simulcast_rate_allocator.h index 60b2018380..0b10011019 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator.h +++ b/modules/video_coding/utility/simulcast_rate_allocator.h @@ -27,7 +27,6 @@ namespace webrtc { class SimulcastRateAllocator : public VideoBitrateAllocator { public: - [[deprecated]] explicit SimulcastRateAllocator(const VideoCodec& codec); SimulcastRateAllocator(const Environment& env, const VideoCodec& codec); ~SimulcastRateAllocator() override; diff --git a/rtc_base/experiments/BUILD.gn b/rtc_base/experiments/BUILD.gn index 98d5062395..dfce9ee2d4 100644 --- a/rtc_base/experiments/BUILD.gn +++ b/rtc_base/experiments/BUILD.gn @@ -134,10 +134,8 @@ rtc_library("rate_control_settings") { "..:logging", "..:safe_conversions", "../../api:field_trials_view", - "../../api/transport:field_trial_based_config", "../../api/units:data_size", "../../api/video_codecs:video_codecs_api", - "../../system_wrappers:field_trial", "../../video/config:encoder_config", ] absl_deps = [ diff --git a/rtc_base/experiments/rate_control_settings.cc b/rtc_base/experiments/rate_control_settings.cc index b81432d8af..597bc736e4 100644 --- a/rtc_base/experiments/rate_control_settings.cc +++ b/rtc_base/experiments/rate_control_settings.cc @@ -16,7 +16,6 @@ #include #include "absl/strings/match.h" -#include "api/transport/field_trial_based_config.h" #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" @@ -87,10 +86,6 @@ RateControlSettings::RateControlSettings( RateControlSettings::~RateControlSettings() = default; RateControlSettings::RateControlSettings(RateControlSettings&&) = default; -RateControlSettings RateControlSettings::ParseFromFieldTrials() { - return RateControlSettings(FieldTrialBasedConfig()); -} - bool RateControlSettings::UseCongestionWindow() const { return static_cast(congestion_window_config_.queue_size_ms); } diff --git a/rtc_base/experiments/rate_control_settings.h b/rtc_base/experiments/rate_control_settings.h index ab9e1ed536..0dd4a14a6e 100644 --- a/rtc_base/experiments/rate_control_settings.h +++ b/rtc_base/experiments/rate_control_settings.h @@ -52,8 +52,6 @@ class RateControlSettings final { RateControlSettings(RateControlSettings&&); ~RateControlSettings(); - static RateControlSettings ParseFromFieldTrials(); - // When CongestionWindowPushback is enabled, the pacer is oblivious to // the congestion window. The relation between outstanding data and // the congestion window affects encoder allocations directly. diff --git a/rtc_base/experiments/rate_control_settings_unittest.cc b/rtc_base/experiments/rate_control_settings_unittest.cc index 91ebf531bd..72470cbd7f 100644 --- a/rtc_base/experiments/rate_control_settings_unittest.cc +++ b/rtc_base/experiments/rate_control_settings_unittest.cc @@ -11,7 +11,8 @@ #include "rtc_base/experiments/rate_control_settings.h" #include "api/video_codecs/video_codec.h" -#include "test/field_trial.h" +#include "test/explicit_key_value_config.h" +#include "test/gmock.h" #include "test/gtest.h" #include "video/config/video_encoder_config.h" @@ -19,171 +20,130 @@ namespace webrtc { namespace { -TEST(RateControlSettingsTest, CongestionWindow) { - EXPECT_TRUE( - RateControlSettings::ParseFromFieldTrials().UseCongestionWindow()); +using test::ExplicitKeyValueConfig; +using ::testing::DoubleEq; +using ::testing::Optional; - test::ScopedFieldTrials field_trials( - "WebRTC-CongestionWindow/QueueSize:100/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_after.UseCongestionWindow()); - EXPECT_EQ(settings_after.GetCongestionWindowAdditionalTimeMs(), 100); +RateControlSettings ParseFrom(absl::string_view field_trials) { + return RateControlSettings(ExplicitKeyValueConfig(field_trials)); +} + +TEST(RateControlSettingsTest, CongestionWindow) { + EXPECT_TRUE(ParseFrom("").UseCongestionWindow()); + + const RateControlSettings settings = + ParseFrom("WebRTC-CongestionWindow/QueueSize:100/"); + EXPECT_TRUE(settings.UseCongestionWindow()); + EXPECT_EQ(settings.GetCongestionWindowAdditionalTimeMs(), 100); } TEST(RateControlSettingsTest, CongestionWindowPushback) { - EXPECT_TRUE(RateControlSettings::ParseFromFieldTrials() - .UseCongestionWindowPushback()); + EXPECT_TRUE(ParseFrom("").UseCongestionWindowPushback()); - test::ScopedFieldTrials field_trials( - "WebRTC-CongestionWindow/QueueSize:100,MinBitrate:100000/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_after.UseCongestionWindowPushback()); - EXPECT_EQ(settings_after.CongestionWindowMinPushbackTargetBitrateBps(), - 100000u); + const RateControlSettings settings = + ParseFrom("WebRTC-CongestionWindow/QueueSize:100,MinBitrate:100000/"); + EXPECT_TRUE(settings.UseCongestionWindowPushback()); + EXPECT_EQ(settings.CongestionWindowMinPushbackTargetBitrateBps(), 100000u); } TEST(RateControlSettingsTest, CongestionWindowPushbackDropframe) { - EXPECT_TRUE(RateControlSettings::ParseFromFieldTrials() - .UseCongestionWindowPushback()); + EXPECT_TRUE(ParseFrom("").UseCongestionWindowPushback()); - test::ScopedFieldTrials field_trials( + const RateControlSettings settings = ParseFrom( "WebRTC-CongestionWindow/" "QueueSize:100,MinBitrate:100000,DropFrame:true/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_after.UseCongestionWindowPushback()); - EXPECT_EQ(settings_after.CongestionWindowMinPushbackTargetBitrateBps(), - 100000u); - EXPECT_TRUE(settings_after.UseCongestionWindowDropFrameOnly()); + EXPECT_TRUE(settings.UseCongestionWindowPushback()); + EXPECT_EQ(settings.CongestionWindowMinPushbackTargetBitrateBps(), 100000u); + EXPECT_TRUE(settings.UseCongestionWindowDropFrameOnly()); } TEST(RateControlSettingsTest, CongestionWindowPushbackDefaultConfig) { - const RateControlSettings settings = - RateControlSettings::ParseFromFieldTrials(); + const RateControlSettings settings = ParseFrom(""); EXPECT_TRUE(settings.UseCongestionWindowPushback()); EXPECT_EQ(settings.CongestionWindowMinPushbackTargetBitrateBps(), 30000u); EXPECT_TRUE(settings.UseCongestionWindowDropFrameOnly()); } TEST(RateControlSettingsTest, PacingFactor) { - EXPECT_FALSE(RateControlSettings::ParseFromFieldTrials().GetPacingFactor()); + EXPECT_FALSE(ParseFrom("").GetPacingFactor()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/pacing_factor:1.2/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - // Need to explicitly dereference the absl::optional - // for the EXPECT_DOUBLE_EQ to compile. - ASSERT_TRUE(settings_after.GetPacingFactor()); - EXPECT_DOUBLE_EQ(*settings_after.GetPacingFactor(), 1.2); + EXPECT_THAT( + ParseFrom("WebRTC-VideoRateControl/pacing_factor:1.2/").GetPacingFactor(), + Optional(DoubleEq(1.2))); } TEST(RateControlSettingsTest, AlrProbing) { - EXPECT_FALSE(RateControlSettings::ParseFromFieldTrials().UseAlrProbing()); + EXPECT_FALSE(ParseFrom("").UseAlrProbing()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/alr_probing:1/"); - EXPECT_TRUE(RateControlSettings::ParseFromFieldTrials().UseAlrProbing()); + EXPECT_TRUE( + ParseFrom("WebRTC-VideoRateControl/alr_probing:1/").UseAlrProbing()); } TEST(RateControlSettingsTest, LibvpxVp8QpMax) { - EXPECT_FALSE(RateControlSettings::ParseFromFieldTrials().LibvpxVp8QpMax()); + EXPECT_FALSE(ParseFrom("").LibvpxVp8QpMax()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/vp8_qp_max:50/"); - EXPECT_EQ(RateControlSettings::ParseFromFieldTrials().LibvpxVp8QpMax(), 50); + EXPECT_EQ( + ParseFrom("WebRTC-VideoRateControl/vp8_qp_max:50/").LibvpxVp8QpMax(), 50); } TEST(RateControlSettingsTest, DoesNotGetTooLargeLibvpxVp8QpMaxValue) { - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/vp8_qp_max:70/"); - EXPECT_FALSE(RateControlSettings::ParseFromFieldTrials().LibvpxVp8QpMax()); + EXPECT_FALSE( + ParseFrom("WebRTC-VideoRateControl/vp8_qp_max:70/").LibvpxVp8QpMax()); } TEST(RateControlSettingsTest, LibvpxVp8MinPixels) { - EXPECT_FALSE( - RateControlSettings::ParseFromFieldTrials().LibvpxVp8MinPixels()); + EXPECT_FALSE(ParseFrom("").LibvpxVp8MinPixels()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/vp8_min_pixels:50000/"); - EXPECT_EQ(RateControlSettings::ParseFromFieldTrials().LibvpxVp8MinPixels(), + EXPECT_EQ(ParseFrom("WebRTC-VideoRateControl/vp8_min_pixels:50000/") + .LibvpxVp8MinPixels(), 50000); } TEST(RateControlSettingsTest, DoesNotGetTooSmallLibvpxVp8MinPixelValue) { - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/vp8_min_pixels:0/"); - EXPECT_FALSE( - RateControlSettings::ParseFromFieldTrials().LibvpxVp8MinPixels()); + EXPECT_FALSE(ParseFrom("WebRTC-VideoRateControl/vp8_min_pixels:0/") + .LibvpxVp8MinPixels()); } TEST(RateControlSettingsTest, LibvpxTrustedRateController) { - const RateControlSettings settings_before = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_before.LibvpxVp8TrustedRateController()); - EXPECT_TRUE(settings_before.LibvpxVp9TrustedRateController()); + const RateControlSettings default_settings = ParseFrom(""); + EXPECT_TRUE(default_settings.LibvpxVp8TrustedRateController()); + EXPECT_TRUE(default_settings.LibvpxVp9TrustedRateController()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/trust_vp8:0,trust_vp9:0/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_FALSE(settings_after.LibvpxVp8TrustedRateController()); - EXPECT_FALSE(settings_after.LibvpxVp9TrustedRateController()); + const RateControlSettings settings = + ParseFrom("WebRTC-VideoRateControl/trust_vp8:0,trust_vp9:0/"); + EXPECT_FALSE(settings.LibvpxVp8TrustedRateController()); + EXPECT_FALSE(settings.LibvpxVp9TrustedRateController()); } TEST(RateControlSettingsTest, Vp8BaseHeavyTl3RateAllocationLegacyKey) { - const RateControlSettings settings_before = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_FALSE(settings_before.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_FALSE(ParseFrom("").Vp8BaseHeavyTl3RateAllocation()); - test::ScopedFieldTrials field_trials( - "WebRTC-UseBaseHeavyVP8TL3RateAllocation/Enabled/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_after.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_TRUE(ParseFrom("WebRTC-UseBaseHeavyVP8TL3RateAllocation/Enabled/") + .Vp8BaseHeavyTl3RateAllocation()); } TEST(RateControlSettingsTest, Vp8BaseHeavyTl3RateAllocationVideoRateControlKey) { - const RateControlSettings settings_before = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_FALSE(settings_before.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_FALSE(ParseFrom("").Vp8BaseHeavyTl3RateAllocation()); - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/vp8_base_heavy_tl3_alloc:1/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_TRUE(settings_after.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_TRUE(ParseFrom("WebRTC-VideoRateControl/vp8_base_heavy_tl3_alloc:1/") + .Vp8BaseHeavyTl3RateAllocation()); } TEST(RateControlSettingsTest, Vp8BaseHeavyTl3RateAllocationVideoRateControlKeyOverridesLegacyKey) { - const RateControlSettings settings_before = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_FALSE(settings_before.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_FALSE(ParseFrom("").Vp8BaseHeavyTl3RateAllocation()); - test::ScopedFieldTrials field_trials( - "WebRTC-UseBaseHeavyVP8TL3RateAllocation/Enabled/WebRTC-VideoRateControl/" - "vp8_base_heavy_tl3_alloc:0/"); - const RateControlSettings settings_after = - RateControlSettings::ParseFromFieldTrials(); - EXPECT_FALSE(settings_after.Vp8BaseHeavyTl3RateAllocation()); + EXPECT_FALSE(ParseFrom("WebRTC-UseBaseHeavyVP8TL3RateAllocation/Enabled/" + "WebRTC-VideoRateControl/vp8_base_heavy_tl3_alloc:0/") + .Vp8BaseHeavyTl3RateAllocation()); } TEST(RateControlSettingsTest, UseEncoderBitrateAdjuster) { - // Should be on by default. - EXPECT_TRUE( - RateControlSettings::ParseFromFieldTrials().UseEncoderBitrateAdjuster()); + EXPECT_TRUE(ParseFrom("").UseEncoderBitrateAdjuster()); - { - // Can be turned off via field trial. - test::ScopedFieldTrials field_trials( - "WebRTC-VideoRateControl/bitrate_adjuster:false/"); - EXPECT_FALSE(RateControlSettings::ParseFromFieldTrials() - .UseEncoderBitrateAdjuster()); - } + EXPECT_FALSE(ParseFrom("WebRTC-VideoRateControl/bitrate_adjuster:false/") + .UseEncoderBitrateAdjuster()); } } // namespace diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 38fe5d4c2c..10ad57a743 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -7300,8 +7300,7 @@ TEST_F(VideoStreamEncoderTest, DropsFramesWhenEncoderOvershoots) { // of video, verify number of drops. Rate needs to be slightly changed in // order to force the rate to be reconfigured. double overshoot_factor = 2.0; - const RateControlSettings trials = - RateControlSettings::ParseFromFieldTrials(); + const RateControlSettings trials(env_.field_trials()); if (trials.UseEncoderBitrateAdjuster()) { // With bitrate adjuster, when need to overshoot even more to trigger // frame dropping since the adjuter will try to just lower the target