From c941579e952193195d56f812574ce442d9576834 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 9 Oct 2023 15:28:57 +0200 Subject: [PATCH] Move field trial check WebRTC-DisableRtxRateLimiter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking in sending classes avoids using global field trial string in favor of the injected one. In addition to that RateLimiter looks wrong layer for check that field trial: checking inside RateLimiter class might be surprising if it is used for limiting something else than RTX bitrate. evaluating field trial for each retransmitting packet might be expensive Bug: webrtc:15184, webrtc:10335 Change-Id: I87bae3522bbd9692629d4f9b6caa119be03f2bd6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322720 Commit-Queue: Danil Chapovalov Reviewed-by: Ying Wang Reviewed-by: Mirko Bonadei Reviewed-by: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#40908} --- audio/channel_send.cc | 6 ++++-- call/rtp_video_sender.cc | 4 +++- rtc_base/BUILD.gn | 2 -- rtc_base/rate_limiter.cc | 4 +--- rtc_base/rate_limiter_unittest.cc | 14 -------------- 5 files changed, 8 insertions(+), 22 deletions(-) diff --git a/audio/channel_send.cc b/audio/channel_send.cc index e3058fca0d..aee2fc4305 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -411,8 +411,10 @@ ChannelSend::ChannelSend( configuration.event_log = event_log_; configuration.rtt_stats = rtcp_rtt_stats; - configuration.retransmission_rate_limiter = - retransmission_rate_limiter_.get(); + if (!field_trials.IsEnabled("WebRTC-DisableRtxRateLimiter")) { + configuration.retransmission_rate_limiter = + retransmission_rate_limiter_.get(); + } configuration.extmap_allow_mixed = extmap_allow_mixed; configuration.rtcp_report_interval_ms = rtcp_report_interval_ms; configuration.rtcp_packet_type_counter_observer = this; diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index c7907a0224..303a70b62a 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -226,7 +226,9 @@ std::vector CreateRtpStreamSenders( configuration.send_bitrate_observer = observers.bitrate_observer; configuration.send_packet_observer = observers.send_packet_observer; configuration.event_log = event_log; - configuration.retransmission_rate_limiter = retransmission_rate_limiter; + if (!trials.IsEnabled("WebRTC-DisableRtxRateLimiter")) { + configuration.retransmission_rate_limiter = retransmission_rate_limiter; + } configuration.rtp_stats_callback = observers.rtp_stats; configuration.frame_encryptor = frame_encryptor; configuration.require_frame_encryption = diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 83f728a809..6aff8cc823 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -525,7 +525,6 @@ rtc_library("rate_limiter") { ":macromagic", ":rate_statistics", "../system_wrappers", - "../system_wrappers:field_trial", "synchronization:mutex", ] absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] @@ -1964,7 +1963,6 @@ if (rtc_include_tests) { "../api/units:time_delta", "../api/units:timestamp", "../system_wrappers", - "../test:field_trial", "../test:fileutils", "../test:test_main", "../test:test_support", diff --git a/rtc_base/rate_limiter.cc b/rtc_base/rate_limiter.cc index 4740b26f81..0f3f343aed 100644 --- a/rtc_base/rate_limiter.cc +++ b/rtc_base/rate_limiter.cc @@ -14,7 +14,6 @@ #include "absl/types/optional.h" #include "system_wrappers/include/clock.h" -#include "system_wrappers/include/field_trial.h" namespace webrtc { @@ -35,8 +34,7 @@ bool RateLimiter::TryUseRate(size_t packet_size_bytes) { MutexLock lock(&lock_); int64_t now_ms = clock_->TimeInMilliseconds(); absl::optional current_rate = current_rate_.Rate(now_ms); - if (!webrtc::field_trial::IsEnabled("WebRTC-DisableRtxRateLimiter") && - current_rate) { + if (current_rate) { // If there is a current rate, check if adding bytes would cause maximum // bitrate target to be exceeded. If there is NOT a valid current rate, // allow allocating rate even if target is exceeded. This prevents diff --git a/rtc_base/rate_limiter_unittest.cc b/rtc_base/rate_limiter_unittest.cc index a830446d60..07dda5609e 100644 --- a/rtc_base/rate_limiter_unittest.cc +++ b/rtc_base/rate_limiter_unittest.cc @@ -15,7 +15,6 @@ #include "rtc_base/event.h" #include "rtc_base/platform_thread.h" #include "system_wrappers/include/clock.h" -#include "test/field_trial.h" #include "test/gtest.h" namespace webrtc { @@ -107,19 +106,6 @@ TEST_F(RateLimitTest, WindowSizeLimits) { EXPECT_FALSE(rate_limiter->SetWindowSize(kWindowSizeMs + 1)); } -TEST_F(RateLimitTest, DiablesRtxRateLimiterByFieldTrial) { - webrtc::test::ScopedFieldTrials trial( - "WebRTC-DisableRtxRateLimiter/Enabled/"); - - // Fill rate, extend window to full size. - EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2)); - clock_.AdvanceTimeMilliseconds(kWindowSizeMs - 1); - EXPECT_TRUE(rate_limiter->TryUseRate(kRateFillingBytes / 2)); - - // Does not limit rate even when all rate consumed. - EXPECT_TRUE(rate_limiter->TryUseRate(1)); -} - static constexpr TimeDelta kMaxTimeout = TimeDelta::Seconds(30); class ThreadTask {