Move BWE period calculation from ProbingIntervalEstimator to AimdRateControl.

webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc

Index: webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc
diff --git a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc
index 5b0ba5a22b27be88a8e7d803f5092b95ddcad3c5..5bfd6714f0e30559da92598b1b0ed368456c6df0 100644
--- a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc
+++ b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc
@@ -126,16 +126,27 @@ int AimdRateControl::GetNearMaxIncreaseRateBps() const {
   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,
@@ -182,7 +193,6 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps,
       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 =
@@ -197,7 +207,8 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t new_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);
       }
@@ -206,6 +217,7 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps,
         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;