Limit BWE reductions before first measured throughput.

After detecting overuse of the network capacity, the target
bitrate is reduced. Normally, this should happen at most once
per RTT to prevent repeated reductions from the same overuse
signal. This CL fixes a bug that allowed repeated reductions
if an overuse was detected before it had the first reliable
throughput measurement.

The fix is guarded by a field trial. To enable the fix, use
WebRTC-BweInitialBackOffInterval/Enabled-200/

Bug: webrtc:9493
Change-Id: Iae566227fd94ebb8a4449406572158a8b79d9c53
Reviewed-on: https://webrtc-review.googlesource.com/88765
Commit-Queue: Björn Terelius <terelius@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24021}

modules/congestion_controller/goog_cc/BUILD.gn

@@ -212,6 +212,7 @@ if (rtc_include_tests) {
       "../../pacing",
       "../../remote_bitrate_estimator",
       "../../rtp_rtcp:rtp_rtcp_format",
+      "//system_wrappers:field_trial_api",
       "//testing/gmock",
       "//third_party/abseil-cpp/absl/memory",
     ]

modules/congestion_controller/goog_cc/delay_based_bwe.cc

@@ -239,11 +239,9 @@ DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate(
       result.updated =
           UpdateEstimate(now_ms, acked_bitrate_bps, &result.target_bitrate_bps);
     } else if (!acked_bitrate_bps && rate_control_.ValidEstimate() &&
-               rate_control_.TimeToReduceFurther(
-                   now_ms, rate_control_.LatestEstimate() / 2 - 1)) {
-      // Overusing before we have a measured acknowledged bitrate. We check
-      // TimeToReduceFurther (with a fake acknowledged bitrate) to avoid
-      // reducing too often.
+               rate_control_.InitialTimeToReduceFurther(now_ms)) {
+      // Overusing before we have a measured acknowledged bitrate. Reduce send
+      // rate by 50% every 200 ms.
       // TODO(tschumim): Improve this and/or the acknowledged bitrate estimator
       // so that we (almost) always have a bitrate estimate.
       rate_control_.SetEstimate(rate_control_.LatestEstimate() / 2, now_ms);

modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc

@@ -13,6 +13,7 @@
 #include "modules/pacing/paced_sender.h"
 #include "rtc_base/constructormagic.h"
 #include "system_wrappers/include/clock.h"
+#include "system_wrappers/include/field_trial.h"
 #include "test/field_trial.h"
 #include "test/gtest.h"
 
@@ -234,4 +235,58 @@ TEST_F(DelayBasedBweTest, TestInitialOveruse) {
   EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000);
 }
 
+class DelayBasedBweTestWithBackoffTimeoutExperiment : public DelayBasedBweTest {
+ public:
+  DelayBasedBweTestWithBackoffTimeoutExperiment()
+      : DelayBasedBweTest("WebRTC-BweInitialBackOffInterval/Enabled-200/") {}
+};
+
+// This test subsumes and improves DelayBasedBweTest.TestInitialOveruse above.
+TEST_F(DelayBasedBweTestWithBackoffTimeoutExperiment, TestInitialOveruse) {
+  const uint32_t kStartBitrate = 300e3;
+  const uint32_t kInitialCapacityBps = 200e3;
+  const uint32_t kDummySsrc = 0;
+  // High FPS to ensure that we send a lot of packets in a short time.
+  const int kFps = 90;
+
+  stream_generator_->AddStream(new test::RtpStream(kFps, kStartBitrate));
+  stream_generator_->set_capacity_bps(kInitialCapacityBps);
+
+  // Needed to initialize the AimdRateControl.
+  bitrate_estimator_->SetStartBitrate(kStartBitrate);
+
+  // Produce 30 frames (in 1/3 second) and give them to the estimator.
+  uint32_t bitrate_bps = kStartBitrate;
+  bool seen_overuse = false;
+  for (int frames = 0; frames < 30 && !seen_overuse; ++frames) {
+    bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps);
+    // The purpose of this test is to ensure that we back down even if we don't
+    // have any acknowledged bitrate estimate yet. Hence, if the test works
+    // as expected, we should not have a measured bitrate yet.
+    EXPECT_FALSE(acknowledged_bitrate_estimator_->bitrate_bps().has_value());
+    if (overuse) {
+      EXPECT_TRUE(bitrate_observer_.updated());
+      EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000);
+      bitrate_bps = bitrate_observer_.latest_bitrate();
+      seen_overuse = true;
+    } else if (bitrate_observer_.updated()) {
+      bitrate_bps = bitrate_observer_.latest_bitrate();
+      bitrate_observer_.Reset();
+    }
+  }
+  EXPECT_TRUE(seen_overuse);
+  // Continue generating an additional 15 frames (equivalent to 167 ms) and
+  // verify that we don't back down further.
+  for (int frames = 0; frames < 15 && seen_overuse; ++frames) {
+    bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps);
+    EXPECT_FALSE(overuse);
+    if (bitrate_observer_.updated()) {
+      bitrate_bps = bitrate_observer_.latest_bitrate();
+      EXPECT_GE(bitrate_bps, kStartBitrate / 2 - 15000);
+      EXPECT_LE(bitrate_bps, kInitialCapacityBps + 15000);
+      bitrate_observer_.Reset();
+    }
+  }
+}
+
 }  // namespace webrtc

modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc

@@ -148,7 +148,20 @@ int64_t StreamGenerator::GenerateFrame(std::vector<PacketFeedback>* packets,
 }  // namespace test
 
 DelayBasedBweTest::DelayBasedBweTest()
-    : clock_(100000000),
+    : field_trial(),
+      clock_(100000000),
+      acknowledged_bitrate_estimator_(
+          absl::make_unique<AcknowledgedBitrateEstimator>()),
+      bitrate_estimator_(new DelayBasedBwe(nullptr)),
+      stream_generator_(new test::StreamGenerator(1e6,  // Capacity.
+                                                  clock_.TimeInMicroseconds())),
+      arrival_time_offset_ms_(0),
+      first_update_(true) {}
+
+DelayBasedBweTest::DelayBasedBweTest(const std::string& field_trial_string)
+    : field_trial(
+          absl::make_unique<test::ScopedFieldTrials>(field_trial_string)),
+      clock_(100000000),
       acknowledged_bitrate_estimator_(
           absl::make_unique<AcknowledgedBitrateEstimator>()),
       bitrate_estimator_(new DelayBasedBwe(nullptr)),

modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h

@@ -14,6 +14,7 @@
 #include <list>
 #include <map>
 #include <memory>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -22,6 +23,7 @@
 #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "rtc_base/constructormagic.h"
 #include "system_wrappers/include/clock.h"
+#include "test/field_trial.h"
 #include "test/gtest.h"
 
 namespace webrtc {
@@ -114,6 +116,7 @@ class StreamGenerator {
 class DelayBasedBweTest : public ::testing::Test {
  public:
   DelayBasedBweTest();
+  explicit DelayBasedBweTest(const std::string& field_trial_string);
   virtual ~DelayBasedBweTest();
 
  protected:
@@ -163,6 +166,8 @@ class DelayBasedBweTest : public ::testing::Test {
 
   static const uint32_t kDefaultSsrc;
 
+  std::unique_ptr<test::ScopedFieldTrials>
+      field_trial;        // Must be initialized first.
   SimulatedClock clock_;  // Time at the receiver.
   test::TestBitrateObserver bitrate_observer_;
   std::unique_ptr<AcknowledgedBitrateEstimator> acknowledged_bitrate_estimator_;

modules/remote_bitrate_estimator/aimd_rate_control.cc

@@ -10,20 +10,21 @@
 
 #include "modules/remote_bitrate_estimator/aimd_rate_control.h"
 
+#include <inttypes.h>
+
 #include <algorithm>
 #include <cassert>
 #include <cmath>
 #include <cstdio>
 #include <string>
 
-#include "rtc_base/checks.h"
-#include "rtc_base/numerics/safe_minmax.h"
-
 #include "modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "modules/remote_bitrate_estimator/overuse_detector.h"
 #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h"
+#include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
+#include "rtc_base/numerics/safe_minmax.h"
 #include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
@@ -31,8 +32,11 @@ namespace webrtc {
 static const int64_t kDefaultRttMs = 200;
 static const int64_t kMaxFeedbackIntervalMs = 1000;
 static const float kDefaultBackoffFactor = 0.85f;
+static const int64_t kDefaultInitialBackOffIntervalMs = 200;
 
 const char kBweBackOffFactorExperiment[] = "WebRTC-BweBackOffFactor";
+const char kBweInitialBackOffIntervalExperiment[] =
+    "WebRTC-BweInitialBackOffInterval";
 
 float ReadBackoffFactor() {
   std::string experiment_string =
@@ -54,6 +58,25 @@ float ReadBackoffFactor() {
   return kDefaultBackoffFactor;
 }
 
+int64_t ReadInitialBackoffIntervalMs() {
+  std::string experiment_string =
+      webrtc::field_trial::FindFullName(kBweInitialBackOffIntervalExperiment);
+  int64_t backoff_interval;
+  int parsed_values =
+      sscanf(experiment_string.c_str(), "Enabled-%" SCNd64, &backoff_interval);
+  if (parsed_values == 1) {
+    if (10 <= backoff_interval && backoff_interval <= 200) {
+      return backoff_interval;
+    }
+    RTC_LOG(WARNING)
+        << "Initial back-off interval must be between 10 and 200 ms.";
+  }
+  RTC_LOG(LS_WARNING) << "Failed to parse parameters for "
+                      << kBweInitialBackOffIntervalExperiment
+                      << " experiment. Using default.";
+  return kDefaultInitialBackOffIntervalMs;
+}
+
 AimdRateControl::AimdRateControl()
     : min_configured_bitrate_bps_(congestion_controller::GetMinBitrateBps()),
       max_configured_bitrate_bps_(30000000),
@@ -64,6 +87,7 @@ AimdRateControl::AimdRateControl()
       rate_control_state_(kRcHold),
       rate_control_region_(kRcMaxUnknown),
       time_last_bitrate_change_(-1),
+      time_last_bitrate_decrease_(-1),
       time_first_throughput_estimate_(-1),
       bitrate_is_initialized_(false),
       beta_(webrtc::field_trial::IsEnabled(kBweBackOffFactorExperiment)
@@ -72,7 +96,15 @@ AimdRateControl::AimdRateControl()
       rtt_(kDefaultRttMs),
       in_experiment_(!AdaptiveThresholdExperimentIsDisabled()),
       smoothing_experiment_(
-          webrtc::field_trial::IsEnabled("WebRTC-Audio-BandwidthSmoothing")) {
+          webrtc::field_trial::IsEnabled("WebRTC-Audio-BandwidthSmoothing")),
+      in_initial_backoff_interval_experiment_(
+          webrtc::field_trial::IsEnabled(kBweInitialBackOffIntervalExperiment)),
+      initial_backoff_interval_ms_(kDefaultInitialBackOffIntervalMs) {
+  if (in_initial_backoff_interval_experiment_) {
+    initial_backoff_interval_ms_ = ReadInitialBackoffIntervalMs();
+    RTC_LOG(LS_INFO) << "Using aimd rate control with initial back-off interval"
+                     << " " << initial_backoff_interval_ms_ << " ms.";
+  }
   RTC_LOG(LS_INFO) << "Using aimd rate control with back off factor " << beta_;
 }
 
@@ -105,11 +137,11 @@ int64_t AimdRateControl::GetFeedbackInterval() const {
 }
 
 bool AimdRateControl::TimeToReduceFurther(
-    int64_t time_now,
+    int64_t now_ms,
     uint32_t estimated_throughput_bps) const {
   const int64_t bitrate_reduction_interval =
       std::max<int64_t>(std::min<int64_t>(rtt_, 200), 10);
-  if (time_now - time_last_bitrate_change_ >= bitrate_reduction_interval) {
+  if (now_ms - time_last_bitrate_change_ >= bitrate_reduction_interval) {
     return true;
   }
   if (ValidEstimate()) {
@@ -121,6 +153,20 @@ bool AimdRateControl::TimeToReduceFurther(
   return false;
 }
 
+bool AimdRateControl::InitialTimeToReduceFurther(int64_t now_ms) const {
+  if (!in_initial_backoff_interval_experiment_) {
+    return ValidEstimate() &&
+           TimeToReduceFurther(now_ms, LatestEstimate() / 2 - 1);
+  }
+  // TODO(terelius): We could use the RTT (clamped to suitable limits) instead
+  // of a fixed bitrate_reduction_interval.
+  if (time_last_bitrate_decrease_ == -1 ||
+      now_ms - time_last_bitrate_decrease_ >= initial_backoff_interval_ms_) {
+    return true;
+  }
+  return false;
+}
+
 uint32_t AimdRateControl::LatestEstimate() const {
   return current_bitrate_bps_;
 }
@@ -156,8 +202,12 @@ uint32_t AimdRateControl::Update(const RateControlInput* input,
 
 void AimdRateControl::SetEstimate(int bitrate_bps, int64_t now_ms) {
   bitrate_is_initialized_ = true;
+  uint32_t prev_bitrate_bps = current_bitrate_bps_;
   current_bitrate_bps_ = ClampBitrate(bitrate_bps, bitrate_bps);
   time_last_bitrate_change_ = now_ms;
+  if (current_bitrate_bps_ < prev_bitrate_bps) {
+    time_last_bitrate_decrease_ = now_ms;
+  }
 }
 
 int AimdRateControl::GetNearMaxIncreaseRateBps() const {
@@ -273,6 +323,7 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps,
       // Stay on hold until the pipes are cleared.
       rate_control_state_ = kRcHold;
       time_last_bitrate_change_ = now_ms;
+      time_last_bitrate_decrease_ = now_ms;
       break;
 
     default:

modules/remote_bitrate_estimator/aimd_rate_control.h

@@ -33,12 +33,16 @@ class AimdRateControl {
   void SetStartBitrate(int start_bitrate_bps);
   void SetMinBitrate(int min_bitrate_bps);
   int64_t GetFeedbackInterval() const;
+
   // Returns true if the bitrate estimate hasn't been changed for more than
   // an RTT, or if the estimated_throughput is less than half of the current
   // estimate. Should be used to decide if we should reduce the rate further
   // when over-using.
-  bool TimeToReduceFurther(int64_t time_now,
+  bool TimeToReduceFurther(int64_t now_ms,
                            uint32_t estimated_throughput_bps) const;
+  // As above. To be used if overusing before we have measured a throughput.
+  bool InitialTimeToReduceFurther(int64_t now_ms) const;
+
   uint32_t LatestEstimate() const;
   void SetRtt(int64_t rtt);
   uint32_t Update(const RateControlInput* input, int64_t now_ms);
@@ -83,12 +87,15 @@ class AimdRateControl {
   RateControlState rate_control_state_;
   RateControlRegion rate_control_region_;
   int64_t time_last_bitrate_change_;
+  int64_t time_last_bitrate_decrease_;
   int64_t time_first_throughput_estimate_;
   bool bitrate_is_initialized_;
   float beta_;
   int64_t rtt_;
-  bool in_experiment_;
-  bool smoothing_experiment_;
+  const bool in_experiment_;
+  const bool smoothing_experiment_;
+  const bool in_initial_backoff_interval_experiment_;
+  int64_t initial_backoff_interval_ms_;
   absl::optional<int> last_decrease_;
 };
 }  // namespace webrtc