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..f08f97790f8113358dcdf39ae6beeb8d2a5fe30f 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,12 @@ 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;
+    if (result.probe) {
+      aggregated_result.probe = true;
+      aggregated_result.target_bitrate_bps = result.target_bitrate_bps;
+    }
   }
+  MaybeUpdateEstimate(&aggregated_result);

[terelius, 2017/02/08 12:44:45]

Yes, I think we should run some tests and/or monitor Chrome stats. In the
interest of landing this now, how about moving the MaybeUpdateEstimate into the
for-loop above?

[michaelt, 2017/02/08 12:57:11]

I'm a bit afraid that if we move the function in to the loop that we'll never
move it out again. And i would i general prefer to have this function not in the
loop.

Do you have any particular test in mind which could proof that we did nothing
wrong here? 

Or should we use a field_trial for this ?
 

[terelius, 2017/02/21 13:51:48]

Note that the current version of the code essentially calls MaybeUpdateEstimate
inside the loop. Is there any particular reason for updating the estimate after
the loop?

Yes, one option is to set up a field trial and do an experiment in Chrome canary
build. This will require some extra work. Stefan, what do you think?

[michaelt, 2017/02/21 14:52:43]

To me, it is cleaner when we feed the overused defector in the loop and then
update UpdateEstimate after the loop. Since It seams a bit strange to
UpdateEstimate for each packets, if we get the packets in a vector. Especially
since we have some time dependent functions in UpdateEstimate.  

[terelius, 2017/02/21 15:10:09]

I agree it is cleaner, but the rest of the code is tuned based on the assumption
that we get an overuse signal for each packet. It is really hard to say what
happens if we only check for overuse every 10-15 packets. It might work great,
but then again, it might not...

[stefan-webrtc, 2017/02/21 15:14:08]

I don't think we should run a finch experiment since it's quite a lot of work
and it isn't very likely to reveal a noticeable difference.

What do you think of changing MaybeUpdateEstimate so that it acts on
kBwOverusing if any of the acked packets in packet_feedback_vector led to an
overuse? That'd be closer to today's behavior I think.

[michaelt, 2017/02/21 15:27:56]

Sounds ok to me.

[michaelt, 2017/02/22 09:40:48]

Done.

   return aggregated_result;
 }
 
@@ -313,46 +321,40 @@ 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);
+    if (probing_bps > 0) {
+      result.target_bitrate_bps = probing_bps;
+      result.probe = true;
+    }
   }
+
+  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 (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->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);
+  } else {
+    if (result->probe) {
+      rate_control_.SetEstimate(result->target_bitrate_bps, now_ms);
+    }
+    result->updated =
+        UpdateEstimate(now_ms, acked_bitrate_bps, &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
@@ -360,6 +362,7 @@ bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
   const RateControlInput input(detector_.State(), acked_bitrate_bps, 0);
   rate_control_.Update(&input, now_ms);
   *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();
 }