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 7a20b3eedb4fea1556931d8935c5218591a71a8d..1edec365ea5a56bbf36c32f9c2751506111d6ad9 100644
--- a/webrtc/modules/congestion_controller/delay_based_bwe.cc
+++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc
@@ -131,15 +131,21 @@ DelayBasedBwe::BitrateEstimator::BitrateEstimator()
       bitrate_estimate_(-1.0f),
       bitrate_estimate_var_(50.0f),
       old_estimator_(kBitrateWindowMs, 8000),
-      in_experiment_(BitrateEstimateExperimentIsEnabled()) {}
+      in_experiment_(BitrateEstimateExperimentIsEnabled()),
+      last_result_was_valid_(false) {}
 
 void DelayBasedBwe::BitrateEstimator::Update(int64_t now_ms, int bytes) {
   if (!in_experiment_) {
     old_estimator_.Update(bytes, now_ms);
     rtc::Optional<uint32_t> rate = old_estimator_.Rate(now_ms);
     bitrate_estimate_ = -1.0f;
-    if (rate)
+    if (rate) {
       bitrate_estimate_ = *rate / 1000.0f;
+      last_result_was_valid_ = true;

[terelius, 2017/02/07 15:35:28]

Please be aware that the "!in_experiment"-path will be removed at some point. If
you rely an a very specific behavior, you should check that it works with the
new BitrateEstimator.

[michaelt, 2017/02/08 12:05:32]

This just e fix for the old bitrate_estimate_. It behave strange when it gets
unordered packet arrival times. 

+    } else if (last_result_was_valid_) {
+      old_estimator_.Reset();
+      last_result_was_valid_ = false;
+    }
     return;
   }
   int rate_window_ms = kRateWindowMs;
@@ -215,7 +221,6 @@ DelayBasedBwe::DelayBasedBwe(Clock* clock)
       trendline_estimator_(),
       detector_(),
       receiver_incoming_bitrate_(),
-      last_update_ms_(-1),
       last_seen_packet_ms_(-1),
       uma_recorded_(false),
       trendline_window_size_(kDefaultTrendlineWindowSize),
@@ -252,6 +257,27 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
     if (result.updated)
       aggregated_result = result;
   }
+

[terelius, 2017/02/07 15:35:28]

I like the idea of checking for overuse once per RTCP packet instead of for
every acked RTP packet. However, it introduces a subtle but possibly significant
change. The trendline estimators store the delay for the most recent packets to
decide whether the delay is increasing. If we only check the slope occasionally
(marked with ^ in the figure), we might miss the place where the delay
increases.


delay |        ______________
      |       /   ^        ^ 
      |      /
      |     /
      | ___/
      |__^___________________ 
                          time

This is probably not a problem in practice, because the most likely reason for
the delay leveling off is that we have filled the network buffers, and in this
case the loss based estimator should kick in. However, it shows that this is
non-trivial, and why we should expect this change to make the estimators less
sensitive. Updating the detector thresholds may be necessary to compensate.

[michaelt, 2017/02/08 12:05:32]

I hope my impl. change will not influence the behavior of the trendline
estimators. 
As far that i can see it it still gets the same input.

[terelius, 2017/02/08 12:19:52]

Yes, it will get the same input and the detector will end up in the same state.
The difference is that you are checking the state less often. This means that if
the detector goes from "normal" to "overusing" and then back to "normal", we
might miss the overusing period. This is not necessarily a problem, but it is
definitely a change in behavior.

+  rtc::Optional<uint32_t> acked_bitrate_bps =
+      receiver_incoming_bitrate_.bitrate_bps();
+  int64_t now_ms = clock_->TimeInMilliseconds();
+  // Currently overusing the bandwidth.
+  if (detector_.State() == kBwOverusing) {

[stefan-webrtc, 2017/02/07 14:43:13]

Given that you have to check the state also in IncomingPacketInfo it's not clear
to me that this is becoming simpler.

[michaelt, 2017/02/07 15:07:07]

The idea was to split the Update in two parts. 
1. Update the overuse detector and the rate_control (both done in the loop)
2. Updated the BWE and inform the listeners (not in the loop)
The advantage of this would be not to do unnecessary calls on Updated the BWE. 

If you think the additional added complexity overweight this advantage i can
revert this part of the CL. 

[stefan-webrtc, 2017/02/07 15:10:42]

Yes, I think that might be a good idea. It however seems like we are updating
the rate control both inside and outside the loop. I suggest we move all of it
outside.

[michaelt, 2017/02/07 15:12:56]

Ok will try to clean this up :)

+    if (acked_bitrate_bps &&
+        rate_control_.TimeToReduceFurther(now_ms, *acked_bitrate_bps)) {
+      aggregated_result.updated = UpdateEstimate(
+          now_ms, acked_bitrate_bps, &aggregated_result.target_bitrate_bps);
+    }
+  } else {
+    if (!aggregated_result.updated) {
+      aggregated_result.updated = UpdateEstimate(
+          now_ms, acked_bitrate_bps, &aggregated_result.target_bitrate_bps);
+    }
+  }
+  if (aggregated_result.updated) {
+    BWE_TEST_LOGGING_PLOT(1, "target_bitrate_bps", now_ms,
+                          aggregated_result.target_bitrate_bps);
+  }
   return aggregated_result;
 }
 
@@ -313,46 +339,23 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
     }
   }
 
-  int probing_bps = 0;
   if (info.probe_cluster_id != PacketInfo::kNotAProbe) {
-    probing_bps = probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
-  }
-  rtc::Optional<uint32_t> acked_bitrate_bps =
-      receiver_incoming_bitrate_.bitrate_bps();
-  // Currently overusing the bandwidth.
-  if (detector_.State() == kBwOverusing) {
-    if (acked_bitrate_bps &&
-        rate_control_.TimeToReduceFurther(now_ms, *acked_bitrate_bps)) {
+    int probing_bps =
+        probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
+    if (probing_bps > 0) {
+      // No overuse, but probing measured a bitrate.

[stefan-webrtc, 2017/02/07 14:43:13]

You have to check for overuse. Maybe you should move this out to
IncomingPacketFeedbackVector as well and only Update the
probe_bitrate_estimator_ with new packets (basically split
HandleProbeAndEstimateBitrate() in two functions, Update() and GetEstimate())?

[michaelt, 2017/02/08 12:05:32]

I used now the target_bitrate_bps to pass the probing rate to
MaybeUpdateEstimate.
Would you still prefer to split HandleProbeAndEstimateBitrate.

[stefan-webrtc, 2017/02/08 12:16:03]

I don't see the change, did you forgot to upload?

+      rate_control_.SetEstimate(probing_bps, info.arrival_time_ms);
+      result.probe = true;
       result.updated =
-          UpdateEstimate(info.arrival_time_ms, now_ms, acked_bitrate_bps,
+          UpdateEstimate(now_ms, receiver_incoming_bitrate_.bitrate_bps(),
                          &result.target_bitrate_bps);
     }
-  } else if (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, acked_bitrate_bps,
-                       &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, acked_bitrate_bps,
-                       &result.target_bitrate_bps);
-  }
-  if (result.updated) {
-    last_update_ms_ = now_ms;
-    BWE_TEST_LOGGING_PLOT(1, "target_bitrate_bps", now_ms,
-                          result.target_bitrate_bps);
   }
 
   return result;
 }
 
-bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
-                                   int64_t now_ms,
+bool DelayBasedBwe::UpdateEstimate(int64_t now_ms,
                                    rtc::Optional<uint32_t> acked_bitrate_bps,
                                    uint32_t* target_bitrate_bps) {
   // TODO(terelius): RateControlInput::noise_var is deprecated and will be