From 98e71f57eaa94dc085e712d939c19c0bf57b9b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Wed, 27 Sep 2023 14:10:08 +0200 Subject: [PATCH] Subtract an additional 5kbps of the bitrate when backing off. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Traditionally, we'd back off to 85% of the measured throughput in response to an overuse. However, this backoff doesn't appear to be sufficient to drain the queues in some low-bitrate scenarios, and the problem has gotten a bit worse with the RobustThroughputEstimator. (The new estimate looks more stable. The old estimator had more variation, the lowest points were lower, causing backoffs to lower rates.) With this change, we back off to 0.85*thoughput-5kbps. The difference is negligible except at low bitrates. Bug: webrtc:13402,b/298636540 Change-Id: I53328953c056b8ad77f6c7561d6017f171b2dfbc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321701 Commit-Queue: Björn Terelius Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#40827} --- .../aimd_rate_control.cc | 9 ++++++++- .../aimd_rate_control.h | 2 ++ .../aimd_rate_control_unittest.cc | 18 +++++++++++------- ...bitrate_estimator_single_stream_unittest.cc | 6 +++--- ...remote_bitrate_estimator_unittest_helper.cc | 3 ++- 5 files changed, 26 insertions(+), 12 deletions(-) diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc index 5ac4ce829d..fde66657e7 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -79,7 +79,9 @@ AimdRateControl::AimdRateControl(const FieldTrialsView& key_value_config, rtt_(kDefaultRtt), send_side_(send_side), no_bitrate_increase_in_alr_( - key_value_config.IsEnabled("WebRTC-DontIncreaseDelayBasedBweInAlr")) { + key_value_config.IsEnabled("WebRTC-DontIncreaseDelayBasedBweInAlr")), + subtract_additional_backoff_term_(!key_value_config.IsDisabled( + "WebRTC-Bwe-SubtractAdditionalBackoffTerm")) { ParseFieldTrial( {&disable_estimate_bounded_increase_, &use_current_estimate_as_min_upper_bound_}, @@ -287,6 +289,11 @@ void AimdRateControl::ChangeBitrate(const RateControlInput& input, // Set bit rate to something slightly lower than the measured throughput // to get rid of any self-induced delay. decreased_bitrate = estimated_throughput * beta_; + if (decreased_bitrate > DataRate::KilobitsPerSec(5) && + subtract_additional_backoff_term_) { + decreased_bitrate -= DataRate::KilobitsPerSec(5); + } + if (decreased_bitrate > current_bitrate_) { // TODO(terelius): The link_capacity estimate may be based on old // throughput measurements. Relying on them may lead to unnecessary diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.h b/modules/remote_bitrate_estimator/aimd_rate_control.h index 4efde54410..97fa490adf 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -103,6 +103,8 @@ class AimdRateControl { // Allow the delay based estimate to only increase as long as application // limited region (alr) is not detected. const bool no_bitrate_increase_in_alr_; + // If true, subtract an additional 5kbps when backing off. + const bool subtract_additional_backoff_term_; // If "Disabled", estimated link capacity is not used as upper bound. FieldTrialFlag disable_estimate_bounded_increase_{"Disabled"}; FieldTrialParameter use_current_estimate_as_min_upper_bound_{"c_upper", diff --git a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc index 5b8b0caffe..f26afe995c 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc @@ -106,18 +106,22 @@ TEST(AimdRateControlTest, DefaultPeriodUntilFirstOveruse) { EXPECT_NE(aimd_rate_control.GetExpectedBandwidthPeriod(), kDefaultPeriod); } -TEST(AimdRateControlTest, ExpectedPeriodAfter20kbpsDropAnd5kbpsIncrease) { +TEST(AimdRateControlTest, ExpectedPeriodAfterTypicalDrop) { AimdRateControl aimd_rate_control(ExplicitKeyValueConfig("")); - constexpr DataRate kInitialBitrate = DataRate::BitsPerSec(110'000); + // The rate increase at 216 kbps should be 12 kbps. If we drop from + // 216 + 4*12 = 264 kbps, it should take 4 seconds to recover. Since we + // back off to 0.85*acked_rate-5kbps, the acked bitrate needs to be 260 + // kbps to end up at 216 kbps. + constexpr DataRate kInitialBitrate = DataRate::BitsPerSec(264'000); + constexpr DataRate kUpdatedBitrate = DataRate::BitsPerSec(216'000); + const DataRate kAckedBitrate = + (kUpdatedBitrate + DataRate::BitsPerSec(5'000)) / kFractionAfterOveruse; Timestamp now = kInitialTime; aimd_rate_control.SetEstimate(kInitialBitrate, now); now += TimeDelta::Millis(100); - // Make the bitrate drop by 20 kbps to get to 90 kbps. - // The rate increase at 90 kbps should be 5 kbps, so the period should be 4 s. - const DataRate kAckedBitrate = - (kInitialBitrate - DataRate::BitsPerSec(20'000)) / kFractionAfterOveruse; aimd_rate_control.Update({BandwidthUsage::kBwOverusing, kAckedBitrate}, now); - EXPECT_EQ(aimd_rate_control.GetNearMaxIncreaseRateBpsPerSecond(), 5'000); + EXPECT_EQ(aimd_rate_control.LatestEstimate(), kUpdatedBitrate); + EXPECT_EQ(aimd_rate_control.GetNearMaxIncreaseRateBpsPerSecond(), 12'000); EXPECT_EQ(aimd_rate_control.GetExpectedBandwidthPeriod(), TimeDelta::Seconds(4)); } diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc index 64ef39d935..3d72807698 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc @@ -53,7 +53,7 @@ TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropOneStreamWrap) { } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropTwoStreamsWrap) { - CapacityDropTestHelper(2, true, 767, 0); + CapacityDropTestHelper(2, true, 567, 0); } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropThreeStreamsWrap) { @@ -61,11 +61,11 @@ TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropThreeStreamsWrap) { } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropThirteenStreamsWrap) { - CapacityDropTestHelper(13, true, 567, 0); + CapacityDropTestHelper(13, true, 767, 0); } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropNineteenStreamsWrap) { - CapacityDropTestHelper(19, true, 700, 0); + CapacityDropTestHelper(19, true, 767, 0); } TEST_F(RemoteBitrateEstimatorSingleTest, CapacityDropThirtyStreamsWrap) { diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc index ee9644530a..f8652b455e 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc @@ -477,7 +477,8 @@ void RemoteBitrateEstimatorTest::CapacityDropTestHelper( uint32_t bitrate_bps = SteadyStateRun( kDefaultSsrc, steady_state_time * kFramerate, kStartBitrate, kMinExpectedBitrate, kMaxExpectedBitrate, kInitialCapacityBps); - EXPECT_NEAR(kInitialCapacityBps, bitrate_bps, 130000u); + EXPECT_GE(bitrate_bps, 0.85 * kInitialCapacityBps); + EXPECT_LE(bitrate_bps, 1.05 * kInitialCapacityBps); bitrate_observer_->Reset(); // Add an offset to make sure the BWE can handle it.