Revert "Configure and use max bitrate to limit the AIMD controller es…

…timates."

This reverts commit 18d7c7e.

Reason for revert: 
This seems to cause the auto roller to Chrome to fail on Linux and Mac on the browsertest
WebRtcSimulcastBrowserTest.TestVgaReturnsTwoSimulcastStreams

https://chromium-review.googlesource.com/c/chromium/src/+/1064736


Original change's description:
> Configure and use max bitrate to limit the AIMD controller estimates.
> 
> Bug: webrtc:9275
> Change-Id: I9625cd473e1cb198abe08020f5462f1bd64bf2a5
> Reviewed-on: https://webrtc-review.googlesource.com/77081
> Reviewed-by: Stefan Holmer <[email protected]>
> Reviewed-by: Sebastian Jansson <[email protected]>
> Commit-Queue: Björn Terelius <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#23287}

[email protected],[email protected],[email protected]

Change-Id: I8ed827ab6b2f7d2b70b9889e5a88701bfb974d35
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:9275
Reviewed-on: https://webrtc-review.googlesource.com/77660
Reviewed-by: Per Kjellander <[email protected]>
Commit-Queue: Per Kjellander <[email protected]>
Cr-Commit-Position: refs/heads/master@{#23291}

modules/congestion_controller/delay_based_bwe.cc

@@ -314,11 +314,10 @@ void DelayBasedBwe::SetStartBitrate(int start_bitrate_bps) {
   rate_control_.SetStartBitrate(start_bitrate_bps);
 }
 
-void DelayBasedBwe::SetBitrateConstraints(int min_bitrate_bps,
-                                          int max_bitrate_bps) {
+void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) {
   // Called from both the configuration thread and the network thread. Shouldn't
   // be called from the network thread in the future.
-  rate_control_.SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
+  rate_control_.SetMinBitrate(min_bitrate_bps);
 }
 
 int64_t DelayBasedBwe::GetExpectedBwePeriodMs() const {

modules/congestion_controller/delay_based_bwe.h

@@ -52,7 +52,7 @@ class DelayBasedBwe {
   bool LatestEstimate(std::vector<uint32_t>* ssrcs,
                       uint32_t* bitrate_bps) const;
   void SetStartBitrate(int start_bitrate_bps);
-  void SetBitrateConstraints(int min_bitrate_bps, int max_bitrate_bps);
+  void SetMinBitrate(int min_bitrate_bps);
   int64_t GetExpectedBwePeriodMs() const;
 
  private:

modules/congestion_controller/goog_cc/delay_based_bwe.cc

@@ -318,11 +318,10 @@ void DelayBasedBwe::SetStartBitrate(int start_bitrate_bps) {
   rate_control_.SetStartBitrate(start_bitrate_bps);
 }
 
-void DelayBasedBwe::SetBitrateConstraints(int min_bitrate_bps,
-                                          int max_bitrate_bps) {
+void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) {
   // Called from both the configuration thread and the network thread. Shouldn't
   // be called from the network thread in the future.
-  rate_control_.SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
+  rate_control_.SetMinBitrate(min_bitrate_bps);
 }
 
 int64_t DelayBasedBwe::GetExpectedBwePeriodMs() const {

modules/congestion_controller/goog_cc/delay_based_bwe.h

@@ -53,9 +53,8 @@ class DelayBasedBwe {
   void OnRttUpdate(int64_t avg_rtt_ms);
   bool LatestEstimate(std::vector<uint32_t>* ssrcs,
                       uint32_t* bitrate_bps) const;
-
   void SetStartBitrate(int start_bitrate_bps);
-  void SetBitrateConstraints(int min_bitrate_bps, int max_bitrate_bps);
+  void SetMinBitrate(int min_bitrate_bps);
   int64_t GetExpectedBwePeriodMs() const;
 
  private:

modules/congestion_controller/goog_cc/goog_cc_network_control.cc

@@ -129,9 +129,7 @@ GoogCcNetworkController::GoogCcNetworkController(RtcEventLog* event_log,
       max_total_allocated_bitrate_(DataRate::Zero()),
       in_cwnd_experiment_(CwndExperimentEnabled()),
       accepted_queue_ms_(kDefaultAcceptedQueueMs) {
-  constexpr int kMaxBitrateUnchanged = -1;
-  delay_based_bwe_->SetBitrateConstraints(
-      congestion_controller::GetMinBitrateBps(), kMaxBitrateUnchanged);
+  delay_based_bwe_->SetMinBitrate(congestion_controller::GetMinBitrateBps());
   UpdateBitrateConstraints(config.constraints, config.starting_bandwidth);
   OnStreamsConfig(config.stream_based_config);
   if (in_cwnd_experiment_ &&
@@ -165,7 +163,7 @@ NetworkControlUpdate GoogCcNetworkController::OnNetworkRouteChange(
   delay_based_bwe_.reset(new DelayBasedBwe(event_log_));
   acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator());
   delay_based_bwe_->SetStartBitrate(start_bitrate_bps);
-  delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
+  delay_based_bwe_->SetMinBitrate(min_bitrate_bps);
 
   probe_controller_->Reset(msg.at_time.ms());
   probe_controller_->SetBitrates(min_bitrate_bps, start_bitrate_bps,
@@ -264,10 +262,9 @@ void GoogCcNetworkController::UpdateBitrateConstraints(
 
   bandwidth_estimation_->SetBitrates(start_bitrate_bps, min_bitrate_bps,
                                      max_bitrate_bps);
-
   if (start_bitrate_bps > 0)
     delay_based_bwe_->SetStartBitrate(start_bitrate_bps);
-  delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
+  delay_based_bwe_->SetMinBitrate(min_bitrate_bps);
 }
 
 NetworkControlUpdate GoogCcNetworkController::OnTransportLossReport(

modules/congestion_controller/send_side_congestion_controller.cc

@@ -137,9 +137,7 @@ SendSideCongestionController::SendSideCongestionController(
           webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")),
       transport_overhead_bytes_per_packet_(0),
       pacer_pushback_experiment_(IsPacerPushbackExperimentEnabled()) {
-  constexpr int kMaxBitrateUnchanged = -1;
-  delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps_,
-                                          kMaxBitrateUnchanged);
+  delay_based_bwe_->SetMinBitrate(min_bitrate_bps_);
   if (in_cwnd_experiment_ &&
       !ReadCwndExperimentParameter(&accepted_queue_ms_)) {
     RTC_LOG(LS_WARNING) << "Failed to parse parameters for CwndExperiment "
@@ -188,7 +186,7 @@ void SendSideCongestionController::SetBweBitrates(int min_bitrate_bps,
     if (start_bitrate_bps > 0)
       delay_based_bwe_->SetStartBitrate(start_bitrate_bps);
     min_bitrate_bps_ = min_bitrate_bps;
-    delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
+    delay_based_bwe_->SetMinBitrate(min_bitrate_bps_);
   }
   MaybeTriggerOnNetworkChanged();
 }
@@ -222,8 +220,8 @@ void SendSideCongestionController::OnNetworkRouteChanged(
     min_bitrate_bps_ = min_bitrate_bps;
     delay_based_bwe_.reset(new DelayBasedBwe(event_log_, clock_));
     acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator());
-    delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps);
     delay_based_bwe_->SetStartBitrate(bitrate_bps);
+    delay_based_bwe_->SetMinBitrate(min_bitrate_bps);
   }
 
   probe_controller_->Reset();

modules/remote_bitrate_estimator/aimd_rate_control.cc

@@ -28,14 +28,9 @@
 
 namespace webrtc {
 
-namespace {
-constexpr int64_t kDefaultRttMs = 200;
-constexpr int64_t kMinFeedbackIntervalMs = 200;
-constexpr int64_t kMaxFeedbackIntervalMs = 1000;
-constexpr float kDefaultBackoffFactor = 0.85f;
-constexpr int kDefaultMaxBandwidthBps = 30000000;
-constexpr int kDefaultStartBandwidthBps = 300000;
-}  // namespace
+static const int64_t kDefaultRttMs = 200;
+static const int64_t kMaxFeedbackIntervalMs = 1000;
+static const float kDefaultBackoffFactor = 0.85f;
 
 const char kBweBackOffFactorExperiment[] = "WebRTC-BweBackOffFactor";
 
@@ -61,8 +56,8 @@ float ReadTrendlineFilterWindowSize() {
 
 AimdRateControl::AimdRateControl()
     : min_configured_bitrate_bps_(congestion_controller::GetMinBitrateBps()),
-      max_configured_bitrate_bps_(kDefaultMaxBandwidthBps),
-      current_bitrate_bps_(kDefaultStartBandwidthBps),
+      max_configured_bitrate_bps_(30000000),
+      current_bitrate_bps_(max_configured_bitrate_bps_),
       avg_max_bitrate_kbps_(-1.0f),
       var_max_bitrate_kbps_(0.4f),
       rate_control_state_(kRcHold),
@@ -83,30 +78,13 @@ AimdRateControl::AimdRateControl()
 AimdRateControl::~AimdRateControl() {}
 
 void AimdRateControl::SetStartBitrate(int start_bitrate_bps) {
-  // TODO(terelius): It would make sense to not change the estimate if we have
-  // already initialized a bitrate. However, some code recreates and resets
-  // parameters multiple times and those tests break when changing this.
-  if (start_bitrate_bps > 0) {
-    // TODO(terelius): I think this should clamp the bitrate to the configured
-    // max and min, but that would be a brekaing change because SetStartBitrate
-    // and SetBitrateConstraints are separate calls that can occur in either
-    // order. Consider merging the calls in the future.
-    current_bitrate_bps_ = start_bitrate_bps;
-    bitrate_is_initialized_ = true;
-  }
+  current_bitrate_bps_ = start_bitrate_bps;
+  bitrate_is_initialized_ = true;
 }
 
-void AimdRateControl::SetBitrateConstraints(int min_bitrate_bps,
-                                            int max_bitrate_bps) {
-  if (min_bitrate_bps > 0)
-    min_configured_bitrate_bps_ = min_bitrate_bps;
-  if (max_bitrate_bps > 0) {
-    max_configured_bitrate_bps_ =
-        rtc::SafeMax(min_configured_bitrate_bps_, max_bitrate_bps);
-  }
-  current_bitrate_bps_ = rtc::SafeClamp<uint32_t>(current_bitrate_bps_,
-                                                  min_configured_bitrate_bps_,
-                                                  max_configured_bitrate_bps_);
+void AimdRateControl::SetMinBitrate(int min_bitrate_bps) {
+  min_configured_bitrate_bps_ = min_bitrate_bps;
+  current_bitrate_bps_ = std::max<int>(min_bitrate_bps, current_bitrate_bps_);
 }
 
 bool AimdRateControl::ValidEstimate() const {
@@ -119,6 +97,7 @@ int64_t AimdRateControl::GetFeedbackInterval() const {
   static const int kRtcpSize = 80;
   const int64_t interval = static_cast<int64_t>(
       kRtcpSize * 8.0 * 1000.0 / (0.05 * current_bitrate_bps_) + 0.5);
+  const int64_t kMinFeedbackIntervalMs = 200;
   return rtc::SafeClamp(interval, kMinFeedbackIntervalMs,
                         kMaxFeedbackIntervalMs);
 }
@@ -198,9 +177,10 @@ int AimdRateControl::GetExpectedBandwidthPeriodMs() const {
   if (!last_decrease_)
     return smoothing_experiment_ ? kMinPeriodMs : kDefaultPeriodMs;
 
-  return rtc::SafeClamp<int>(
-      1000 * static_cast<int64_t>(*last_decrease_) / increase_rate,
-      kMinPeriodMs, kMaxPeriodMs);
+  return std::min(kMaxPeriodMs,
+                  std::max<int>(1000 * static_cast<int64_t>(*last_decrease_) /
+                                    increase_rate,
+                                kMinPeriodMs));
 }
 
 uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps,
@@ -300,8 +280,7 @@ uint32_t AimdRateControl::ClampBitrate(uint32_t new_bitrate_bps,
   // We allow a bit more lag at very low rates to not too easily get stuck if
   // the encoder produces uneven outputs.
   const uint32_t max_bitrate_bps =
-      rtc::SafeMin(static_cast<uint32_t>(1.5f * incoming_bitrate_bps) + 10000,
-                   max_configured_bitrate_bps_);
+      static_cast<uint32_t>(1.5f * incoming_bitrate_bps) + 10000;
   if (new_bitrate_bps > current_bitrate_bps_ &&
       new_bitrate_bps > max_bitrate_bps) {
     new_bitrate_bps = std::max(current_bitrate_bps_, max_bitrate_bps);

modules/remote_bitrate_estimator/aimd_rate_control.h

@@ -30,7 +30,7 @@ class AimdRateControl {
   // otherwise.
   bool ValidEstimate() const;
   void SetStartBitrate(int start_bitrate_bps);
-  void SetBitrateConstraints(int min_bitrate_bps, int max_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 incoming_bitrate is less than half of the current

modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc

@@ -105,50 +105,6 @@ TEST(AimdRateControlTest, GetIncreaseRateAndBandwidthPeriodSmoothingExp) {
             states.aimd_rate_control->GetExpectedBandwidthPeriodMs());
 }
 
-TEST(AimdRateControlTest, BweLimitedByConfiguredMin) {
-  auto states = CreateAimdRateControlStates();
-  constexpr int kMinBitrate = 50000;
-  constexpr int kMaxBitrate = 200000;
-  constexpr int kStartBitrate = kMinBitrate + 1;
-  states.aimd_rate_control->SetBitrateConstraints(kMinBitrate, kMaxBitrate);
-  states.aimd_rate_control->SetStartBitrate(kStartBitrate);
-  states.aimd_rate_control->SetEstimate(
-      kStartBitrate, states.simulated_clock->TimeInMilliseconds());
-  while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime <
-         20000) {
-    // Don't decrease below minimum even if the actual throughput is lower.
-    constexpr int kAckedBitrate = kMinBitrate - 1000;
-    UpdateRateControl(states, BandwidthUsage::kBwOverusing, kAckedBitrate,
-                      states.simulated_clock->TimeInMilliseconds());
-    states.simulated_clock->AdvanceTimeMilliseconds(100);
-  }
-  ASSERT_TRUE(states.aimd_rate_control->ValidEstimate());
-  EXPECT_EQ(static_cast<uint32_t>(kMinBitrate),
-            states.aimd_rate_control->LatestEstimate());
-}
-
-TEST(AimdRateControlTest, BweLimitedByConfiguredMax) {
-  auto states = CreateAimdRateControlStates();
-  constexpr int kMinBitrate = 50000;
-  constexpr int kMaxBitrate = 200000;
-  constexpr int kStartBitrate = kMaxBitrate - 1;
-  states.aimd_rate_control->SetBitrateConstraints(kMinBitrate, kMaxBitrate);
-  states.aimd_rate_control->SetStartBitrate(kStartBitrate);
-  states.aimd_rate_control->SetEstimate(
-      kStartBitrate, states.simulated_clock->TimeInMilliseconds());
-  while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime <
-         20000) {
-    // Don't increase above maximum even if the actual throughput is higher.
-    constexpr int kAckedBitrate = kMaxBitrate + 10000;
-    UpdateRateControl(states, BandwidthUsage::kBwNormal, kAckedBitrate,
-                      states.simulated_clock->TimeInMilliseconds());
-    states.simulated_clock->AdvanceTimeMilliseconds(100);
-  }
-  ASSERT_TRUE(states.aimd_rate_control->ValidEstimate());
-  EXPECT_EQ(static_cast<uint32_t>(kMaxBitrate),
-            states.aimd_rate_control->LatestEstimate());
-}
-
 TEST(AimdRateControlTest, BweLimitedByAckedBitrate) {
   auto states = CreateAimdRateControlStates();
   constexpr int kAckedBitrate = 10000;
@@ -266,7 +222,6 @@ TEST(AimdRateControlTest, BandwidthPeriodIsNotAboveMaxSmoothingExp) {
   test::ScopedFieldTrials override_field_trials(kSmoothingExpFieldTrial);
   auto states = CreateAimdRateControlStates();
   constexpr int kInitialBitrate = 50000000;
-  states.aimd_rate_control->SetBitrateConstraints(10000, 60000000);
   states.aimd_rate_control->SetEstimate(
       kInitialBitrate, states.simulated_clock->TimeInMilliseconds());
   states.simulated_clock->AdvanceTimeMilliseconds(100);

modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc

@@ -412,7 +412,6 @@ void RemoteBitrateEstimatorAbsSendTime::SetMinBitrate(int min_bitrate_bps) {
   // Called from both the configuration thread and the network thread. Shouldn't
   // be called from the network thread in the future.
   rtc::CritScope lock(&crit_);
-  constexpr int kMaxBitrateUnchanged = -1;
-  remote_rate_.SetBitrateConstraints(min_bitrate_bps, kMaxBitrateUnchanged);
+  remote_rate_.SetMinBitrate(min_bitrate_bps);
 }
 }  // namespace webrtc

modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc

@@ -250,8 +250,7 @@ AimdRateControl* RemoteBitrateEstimatorSingleStream::GetRemoteRate() {
 
 void RemoteBitrateEstimatorSingleStream::SetMinBitrate(int min_bitrate_bps) {
   rtc::CritScope cs(&crit_sect_);
-  constexpr int kMaxBitrateUnchanged = -1;
-  remote_rate_->SetBitrateConstraints(min_bitrate_bps, kMaxBitrateUnchanged);
+  remote_rate_->SetMinBitrate(min_bitrate_bps);
 }
 
 }  // namespace webrtc

modules/remote_bitrate_estimator/test/estimators/send_side.cc

@@ -44,7 +44,7 @@ SendSideBweSender::SendSideBweSender(int kbps,
   bitrate_controller_->SetStartBitrate(1000 * kbps);
   bitrate_controller_->SetMinMaxBitrate(1000 * kMinBitrateKbps,
                                         1000 * kMaxBitrateKbps);
-  bwe_->SetBitrateConstraints(1000 * kMinBitrateKbps, 1000 * kMaxBitrateKbps);
+  bwe_->SetMinBitrate(1000 * kMinBitrateKbps);
 }
 
 SendSideBweSender::~SendSideBweSender() {}