Remove GetFeedbackInterval in sender side BWE.

webrtc/modules/congestion_controller/delay_based_bwe.cc

Index: webrtc/modules/congestion_controller/delay_based_bwe.cc
diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.cc b/webrtc/modules/congestion_controller/delay_based_bwe.cc
index fb6ffd157aa1c1fa76654b38e1c2c832914e7bcd..47d3147611f59ef8bd78622c63bc4ed51e6b651e 100644
--- a/webrtc/modules/congestion_controller/delay_based_bwe.cc
+++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc
@@ -45,7 +45,6 @@ DelayBasedBwe::DelayBasedBwe(Clock* clock)
       estimator_(),
       detector_(OverUseDetectorOptions()),
       receiver_incoming_bitrate_(kBitrateWindowMs, 8000),
-      last_update_ms_(-1),
       last_seen_packet_ms_(-1),
       uma_recorded_(false) {
   network_thread_.DetachFromThread();
@@ -60,21 +59,21 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
                               BweNames::kBweNamesMax);
     uma_recorded_ = true;
   }
+
+  int64_t now_ms = clock_->TimeInMilliseconds();
   Result aggregated_result;
   for (const auto& packet_info : packet_feedback_vector) {
-    Result result = IncomingPacketInfo(packet_info);
-    if (result.updated)
-      aggregated_result = result;
+    aggregated_result = IncomingPacketInfo(packet_info, now_ms);
   }
+  aggregated_result.target_bitrate_bps =
+      rate_control_.UpdateBandwidthEstimate(now_ms);
+  aggregated_result.updated = rate_control_.ValidEstimate();
   return aggregated_result;
 }
 
-DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
-    const PacketInfo& info) {
-  int64_t now_ms = clock_->TimeInMilliseconds();
-
+DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(const PacketInfo& info,
+                                                        int64_t now_ms) {
   receiver_incoming_bitrate_.Update(info.payload_size, info.arrival_time_ms);
-  Result result;
   // Reset if the stream has timed out.
   if (last_seen_packet_ms_ == -1 ||
       now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) {
@@ -112,38 +111,17 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
   if (info.probe_cluster_id != PacketInfo::kNotAProbe) {
     probing_bps = probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
   }
-
-  // Currently overusing the bandwidth.
-  if (detector_.State() == kBwOverusing) {
-    rtc::Optional<uint32_t> incoming_rate =
-        receiver_incoming_bitrate_.Rate(info.arrival_time_ms);
-    if (incoming_rate &&
-        rate_control_.TimeToReduceFurther(now_ms, *incoming_rate)) {

[stefan-webrtc, 2016/10/26 15:19:20]

Isn't this check still desirable? We shouldn't reduce the bitrate more than once
per rtt since we can't see the effect of it before that?

[michaelt, 2016/10/27 14:42:59]

I removed it because it seamed strange to increase the the bit-rate without
waiting time, but on the other hand wait in the overuse case. To me it would
make most sense to wait mean_rtt between all bit-rate changes. 

[stefan-webrtc, 2016/10/31 15:08:49]

I don't think I agree. Increasing is fine to do in small steps every time we get
an ack of a packet delivery. That gives us a smooth increase over time.

Reducing the bitrate is better done in one big step, and then waiting to see if
it had the desired effect. If it did not we could decrease more. The logic we
applied for increasing the rate doesn't apply since it doesn't make sense to
take small steps down for every ack we get...

-      result.updated = UpdateEstimate(info.arrival_time_ms, now_ms,
-                                      &result.target_bitrate_bps);
-    }
-  } else if (probing_bps > 0) {
+  if (detector_.State() != kBwOverusing && probing_bps > 0) {
     // No overuse, but probing measured a bitrate.
     rate_control_.SetEstimate(probing_bps, info.arrival_time_ms);
-    result.probe = true;
-    result.updated = UpdateEstimate(info.arrival_time_ms, now_ms,
-                                    &result.target_bitrate_bps);
-  }
-  if (!result.updated &&
-      (last_update_ms_ == -1 ||
-       now_ms - last_update_ms_ > rate_control_.GetFeedbackInterval())) {
-    result.updated = UpdateEstimate(info.arrival_time_ms, now_ms,
-                                    &result.target_bitrate_bps);
+    UpdateEstimate(info.arrival_time_ms, now_ms);
+    return Result(true, 0);
   }
-  if (result.updated)
-    last_update_ms_ = now_ms;
-
-  return result;
+  UpdateEstimate(info.arrival_time_ms, now_ms);
+  return Result();

[stefan-webrtc, 2016/10/26 15:19:20]

I find this method a bit strange now. It basically never returns a target
bitrate, even if it actually has updated it? What's the point of that?

[michaelt, 2016/10/27 14:42:59]

Since we work with a vector of packets, it seamed strange to me, asking
aimd_rate_control for a new bit rate after each packet. Doing this after the
vector seams to me the better solution. 

But of course there might be a better impl. for this.

[stefan-webrtc, 2016/10/31 15:08:49]

I agree with that. Seems odd to me that we return an empty result here and a
result with bitrate 0 on line 118 though... Should we just have this method
return bool instead?

 }
 
-bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
-                                   int64_t now_ms,
-                                   uint32_t* target_bitrate_bps) {
+void DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms, int64_t now_ms) {
   // The first overuse should immediately trigger a new estimate.
   // We also have to update the estimate immediately if we are overusing
   // and the target bitrate is too high compared to what we are receiving.
@@ -151,8 +129,6 @@ bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
                                receiver_incoming_bitrate_.Rate(arrival_time_ms),
                                estimator_->var_noise());
   rate_control_.Update(&input, now_ms);
-  *target_bitrate_bps = rate_control_.UpdateBandwidthEstimate(now_ms);
-  return rate_control_.ValidEstimate();
 }
 
 void DelayBasedBwe::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) {