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;
+    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;
 }
 
 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);

[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;
     }
-  } 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);
   }
 
+  result.overusing = detector_.State() == kBwOverusing;
   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 (result->overusing) {
+    if (acked_bitrate_bps &&
+        rate_control_.TimeToReduceFurther(now_ms, *acked_bitrate_bps)) {
+      result->updated = UpdateEstimate(now_ms, acked_bitrate_bps, result);
+    }
+  } else {
+    if (result->probe) {
+      rate_control_.SetEstimate(result->target_bitrate_bps, now_ms);
+    }
+    result->updated = UpdateEstimate(now_ms, acked_bitrate_bps, result);
+  }
+}
+
+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();
 }
 
 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