Remove GetFeedbackInterval in sender side BWE.

webrtc/modules/congestion_controller/delay_based_bwe.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 113 matching lines @@
 
 namespace webrtc {
 
 DelayBasedBwe::BitrateEstimator::BitrateEstimator()
     : sum_(0),
       current_win_ms_(0),
       prev_time_ms_(-1),
       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;
   // We use a larger window at the beginning to get a more stable sample that
   // we can use to initialize the estimate.
   if (bitrate_estimate_ < 0.f)
     rate_window_ms = kInitialRateWindowMs;
   float bitrate_sample = UpdateWindow(now_ms, bytes, rate_window_ms);
   if (bitrate_sample < 0.0f)
     return;
@@ skipping 55 matching lines @@
 
 DelayBasedBwe::DelayBasedBwe(Clock* clock)
     : in_trendline_experiment_(TrendlineFilterExperimentIsEnabled()),
       in_median_slope_experiment_(MedianSlopeFilterExperimentIsEnabled()),
       clock_(clock),
       inter_arrival_(),
       kalman_estimator_(),
       trendline_estimator_(),
       detector_(),
       receiver_incoming_bitrate_(),
-      last_update_ms_(-1),
       last_seen_packet_ms_(-1),
       uma_recorded_(false),
       trendline_window_size_(kDefaultTrendlineWindowSize),
       trendline_smoothing_coeff_(kDefaultTrendlineSmoothingCoeff),
       trendline_threshold_gain_(kDefaultTrendlineThresholdGain),
       probing_interval_estimator_(&rate_control_),
       median_slope_window_size_(kDefaultMedianSlopeWindowSize),
       median_slope_threshold_gain_(kDefaultMedianSlopeThresholdGain) {
   if (in_trendline_experiment_) {
     ReadTrendlineFilterExperimentParameters(&trendline_window_size_,
@@ skipping 13 matching lines @@
   RTC_DCHECK(network_thread_.CalledOnValidThread());
   if (!uma_recorded_) {
     RTC_HISTOGRAM_ENUMERATION(kBweTypeHistogram,
                               BweNames::kSendSideTransportSeqNum,
                               BweNames::kBweNamesMax);
     uma_recorded_ = true;
   }
   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;
 }
 
 DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
     const PacketInfo& info) {
   int64_t now_ms = clock_->TimeInMilliseconds();
 
   receiver_incoming_bitrate_.Update(info.arrival_time_ms, info.payload_size);
   Result result;
   // Reset if the stream has timed out.
@@ skipping 41 matching lines @@
                        info.arrival_time_ms);
     } else {
       kalman_estimator_->Update(t_delta, ts_delta_ms, size_delta,
                                 detector_.State(), info.arrival_time_ms);
       detector_.Detect(kalman_estimator_->offset(), ts_delta_ms,
                        kalman_estimator_->num_of_deltas(),
                        info.arrival_time_ms);
     }
   }
 
-  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)) {
-      result.updated =
-          UpdateEstimate(info.arrival_time_ms, now_ms, acked_bitrate_bps,
-                         &result.target_bitrate_bps);
+    int probing_bps =
+        probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
+    if (probing_bps > 0) {
+      result.target_bitrate_bps = probing_bps;
+      result.probe = true;
     }
-  } 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,
+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(now_ms, acked_bitrate_bps,
+                                       &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);
+  }
+}
+
+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
   // removed. In the meantime, we set it to zero.
   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();
 }
 
 void DelayBasedBwe::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) {
   rate_control_.SetRtt(avg_rtt_ms);
 }
 
 bool DelayBasedBwe::LatestEstimate(std::vector<uint32_t>* ssrcs,
                                    uint32_t* bitrate_bps) const {
   // Currently accessed from both the process thread (see
@@ skipping 13 matching lines @@
 void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) {
   // Called from both the configuration thread and the network thread. Shouldn't
   // be called from the network thread in the future.
   rate_control_.SetMinBitrate(min_bitrate_bps);
 }
 
 int64_t DelayBasedBwe::GetProbingIntervalMs() const {
   return probing_interval_estimator_.GetIntervalMs();
 }
 }  // namespace webrtc