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..a715c3a907122f13bfbb287f647358143c492835 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;
+    } 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),
@@ -249,9 +254,13 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
   Result aggregated_result;
   for (const auto& packet_info : packet_feedback_vector) {
     Result result = IncomingPacketInfo(packet_info);
-    if (result.updated)
-      aggregated_result = result;
+    aggregated_result.overusing |= result.overusing;
+    if (result.probe) {
+      aggregated_result.probe = true;
+      aggregated_result.target_bitrate_bps = result.target_bitrate_bps;
+    }
   }
+  MaybeUpdateEstimate(&aggregated_result);
   return aggregated_result;
 }
 
@@ -313,53 +322,49 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
     }
   }
 
-  int probing_bps = 0;
   if (info.probe_cluster_id != PacketInfo::kNotAProbe) {
-    probing_bps = probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
+    int probing_bps =
+        probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);

[stefan-webrtc, 2017/02/24 14:25:26]

I wonder if it makes sense to also split this up, so we update the prober here,
but we don't read the result until later. Then we could return bool from
DelayBasedBwe::IncomingPacketInfo instead of Result, and Result wouldn't have to
contain "overusing".

In addition, MaybeUpdateEstimate could simply return a Result instead of
updating an existing Result and returning void.

WDYT?

[michaelt, 2017/02/27 10:45:41]

In general I like the idea.
I took a look how we could split HandleProbeAndEstimateBitrate in to a Update
and a GetEstimateBitrate function.
The Problem i run in to was that GetEstimateBitrate would need the cluster_id to
get the right bitrate. 
So we would need to pass the cluster_id as well to MaybeUpdateEstimate.
This will lead to a problem when we have two different cluster_id's in the
vector.  

[stefan-webrtc, 2017/03/07 12:09:33]

Now you lost me, which GetEstimateBitrate? Are we talking about after you have
split HandleProbeAndEstimateBitrate into two methods:
void HandleProbe(info)
and 
int GetEstimatedBitrate()
?

I can't easily see how that's needed, but I could be missing something.

[michaelt, 2017/03/07 14:35:53]

Yea exactly. Sorry if i was not clear enough.
void HandleProbe(info) => would be easy to impl.
but
int GetEstimatedBitrate() would not work.
We would have to make the interface like that:
int GetEstimatedBitrate(int cluster_id)
Since we need the cluster id to get the probe estimations. 

[stefan-webrtc, 2017/03/07 15:56:06]

But can't we just get the last probe estimation without an id?

[stefan-webrtc, 2017/03/07 15:57:30]

I'm thinking basically to have HandleProbeAndEstimateBitrate() unchanged, just
that it won't return an estimate, but instead store it. And then have a getter
to get it later?

+    if (probing_bps > 0) {
+      result.target_bitrate_bps = probing_bps;
+      result.probe = true;
+    }
   }
+
+  result.overusing = detector_.State() == kBwOverusing;
+  return result;
+}
+
+void DelayBasedBwe::MaybeUpdateEstimate(Result* result) {
+  RTC_DCHECK(result);
   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) {
+  if (result->overusing) {
     if (acked_bitrate_bps &&
         rate_control_.TimeToReduceFurther(now_ms, *acked_bitrate_bps)) {
-      result.updated =
-          UpdateEstimate(info.arrival_time_ms, now_ms, acked_bitrate_bps,
-                         &result.target_bitrate_bps);
+      result->updated = UpdateEstimate(now_ms, acked_bitrate_bps, result);
     }
-  } 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);
+  } else {
+    if (result->probe) {
+      rate_control_.SetEstimate(result->target_bitrate_bps, now_ms);
+    }
+    result->updated = UpdateEstimate(now_ms, acked_bitrate_bps, result);
   }
-
-  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) {
+                                   Result* result) {
   // TODO(terelius): RateControlInput::noise_var is deprecated and will be
   // removed. In the meantime, we set it to zero.
-  const RateControlInput input(detector_.State(), acked_bitrate_bps, 0);
+  const RateControlInput input(
+      result->overusing ? kBwOverusing : detector_.State(), acked_bitrate_bps,
+      0);
   rate_control_.Update(&input, now_ms);
-  *target_bitrate_bps = rate_control_.UpdateBandwidthEstimate(now_ms);
+  result->target_bitrate_bps = rate_control_.UpdateBandwidthEstimate(now_ms);
+  BWE_TEST_LOGGING_PLOT(1, "target_bitrate_bps", now_ms, *target_bitrate_bps);
   return rate_control_.ValidEstimate();
 }