Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl.

webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 108 matching lines @@
   bitrate_is_initialized_ = true;
   current_bitrate_bps_ = ClampBitrate(bitrate_bps, bitrate_bps);
   time_last_bitrate_change_ = now_ms;
 }
 
 int AimdRateControl::GetNearMaxIncreaseRateBps() const {
   RTC_DCHECK_GT(current_bitrate_bps_, 0);
   double bits_per_frame = static_cast<double>(current_bitrate_bps_) / 30.0;
   double packets_per_frame = std::ceil(bits_per_frame / (8.0 * 1200.0));
   double avg_packet_size_bits = bits_per_frame / packets_per_frame;
+  constexpr double kMinIncreaseRateBps = 4000;

[michaelt, 2017/04/05 07:06:02]

What was the reason for this change ?

[terelius, 2017/04/05 10:58:03]

Originally wanted to make avg_packet_size into a constant, and in that case it
makes sense to keep all parameters together. Do you want me to move it back?

[michaelt, 2017/04/06 11:35:13]

Would be nice since then the const is defined where its used. 

[terelius, 2017/04/07 11:55:04]

Done. However, I don't think there is any advantage in defining constants where
they are used. I prefer collecting all behavior-controlling parameters before
the code that they affect, i.e. at the beginning of the function or the file.

+
   // Approximate the over-use estimator delay to 100 ms.
   const int64_t response_time = in_experiment_ ? (rtt_ + 100) * 2 : rtt_ + 100;
-
-  constexpr double kMinIncreaseRateBps = 4000;
   return static_cast<int>(std::max(
       kMinIncreaseRateBps, (avg_packet_size_bits * 1000) / response_time));
 }
 
-rtc::Optional<int> AimdRateControl::GetLastBitrateDecreaseBps() const {
-  return last_decrease_;
+int AimdRateControl::GetExpectedBandwidthPeriodMs() const {
+  constexpr int kMinIntervalMs = 2000;
+  constexpr int kDefaultIntervalMs = 3000;
+  constexpr int kMaxIntervalMs = 50000;
+
+  int increase_rate = GetNearMaxIncreaseRateBps();
+  if (!last_decrease_)
+    return kDefaultIntervalMs;
+
+  return std::min(kMaxIntervalMs,
+                  std::max<int>(1000 * static_cast<int64_t>(*last_decrease_) /
+                                    increase_rate,
+                                kMinIntervalMs));
 }
 
 uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps,
                                         const RateControlInput& input,
                                         int64_t now_ms) {
   uint32_t incoming_bitrate_bps =
       input.incoming_bitrate.value_or(current_bitrate_bps_);
 
   // An over-use should always trigger us to reduce the bitrate, even though
   // we have not yet established our first estimate. By acting on the over-use,
@@ skipping 26 matching lines @@
       } else {
         uint32_t multiplicative_increase_bps = MultiplicativeRateIncrease(
             now_ms, time_last_bitrate_change_, new_bitrate_bps);
         new_bitrate_bps += multiplicative_increase_bps;
       }
 
       time_last_bitrate_change_ = now_ms;
       break;
 
     case kRcDecrease:
-      bitrate_is_initialized_ = true;

[michaelt, 2017/04/05 07:06:02]

This change seams unrelated with the topic of the CL. 

[terelius, 2017/04/05 10:58:03]

Since the unit tests now use the AimdRateControl logic rather than mocking it, I
had to do something to make the tests pass. Before this change you'd set
last_decrease even if current_bitrate_bps_ had not been initialized. The
alternative would be to always call SetStartBitrate() in the unit tests, but
that seems less robust. Doing it this way means that the code will produce a
sensible BWE period even if the starting bitrate has not been set.

[michaelt, 2017/04/06 11:35:13]

Acknowledged.

       // Set bit rate to something slightly lower than max
       // to get rid of any self-induced delay.
       new_bitrate_bps =
           static_cast<uint32_t>(beta_ * incoming_bitrate_bps + 0.5);
       if (new_bitrate_bps > current_bitrate_bps_) {
         // Avoid increasing the rate when over-using.
         if (rate_control_region_ != kRcMaxUnknown) {
           new_bitrate_bps = static_cast<uint32_t>(
               beta_ * avg_max_bitrate_kbps_ * 1000 + 0.5f);
         }
         new_bitrate_bps = std::min(new_bitrate_bps, current_bitrate_bps_);
       }
       ChangeRegion(kRcNearMax);
 
-      if (incoming_bitrate_bps < current_bitrate_bps_) {
+      if (bitrate_is_initialized_ &&
+          incoming_bitrate_bps < current_bitrate_bps_) {
         last_decrease_ =
             rtc::Optional<int>(current_bitrate_bps_ - new_bitrate_bps);
       }
       if (incoming_bitrate_kbps <
           avg_max_bitrate_kbps_ - 3 * std_max_bit_rate) {
         avg_max_bitrate_kbps_ = -1.0f;
       }
 
+      bitrate_is_initialized_ = true;
       UpdateMaxBitRateEstimate(incoming_bitrate_kbps);
       // Stay on hold until the pipes are cleared.
       rate_control_state_ = kRcHold;
       time_last_bitrate_change_ = now_ms;
       break;
 
     default:
       assert(false);
   }
   return ClampBitrate(new_bitrate_bps, incoming_bitrate_bps);
@@ skipping 77 matching lines @@
     default:
       assert(false);
   }
 }
 
 void AimdRateControl::ChangeRegion(RateControlRegion region) {
   rate_control_region_ = region;
 }
 
 }  // namespace webrtc