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 135 matching lines @@
   return rtc::Optional<uint32_t>(bitrate_estimate_ * 1000);
 }
 
 DelayBasedBwe::DelayBasedBwe(RtcEventLog* event_log, const Clock* clock)
     : event_log_(event_log),
       clock_(clock),
       inter_arrival_(),
       trendline_estimator_(),
       detector_(),
       receiver_incoming_bitrate_(),
-      last_update_ms_(-1),
       last_seen_packet_ms_(-1),
       uma_recorded_(false),
       probe_bitrate_estimator_(event_log),
       trendline_window_size_(kDefaultTrendlineWindowSize),
       trendline_smoothing_coeff_(kDefaultTrendlineSmoothingCoeff),
       trendline_threshold_gain_(kDefaultTrendlineThresholdGain),
       probing_interval_estimator_(&rate_control_),
       consecutive_delayed_feedbacks_(0),
       last_logged_bitrate_(0),
       last_logged_state_(BandwidthUsage::kBwNormal) {
@@ skipping 10 matching lines @@
   std::vector<PacketFeedback> sorted_packet_feedback_vector;
   SortPacketFeedbackVector(packet_feedback_vector,
                            &sorted_packet_feedback_vector);
 
   if (!uma_recorded_) {
     RTC_HISTOGRAM_ENUMERATION(kBweTypeHistogram,
                               BweNames::kSendSideTransportSeqNum,
                               BweNames::kBweNamesMax);
     uma_recorded_ = true;
   }
-  Result aggregated_result;
+  bool overusing = false;
   bool delayed_feedback = true;
   for (const auto& packet_feedback : sorted_packet_feedback_vector) {
     if (packet_feedback.send_time_ms < 0)
       continue;
     delayed_feedback = false;
-    Result result = IncomingPacketFeedback(packet_feedback);
-    if (result.updated)
-      aggregated_result = result;
+    IncomingPacketFeedback(packet_feedback);
+    overusing |= detector_.State() == BandwidthUsage::kBwOverusing;
   }
   if (delayed_feedback) {
     ++consecutive_delayed_feedbacks_;
   } else {
     consecutive_delayed_feedbacks_ = 0;
   }
   if (consecutive_delayed_feedbacks_ >= kMaxConsecutiveFailedLookups) {
-    aggregated_result = OnLongFeedbackDelay(
+    consecutive_delayed_feedbacks_ = 0;
+    return OnLongFeedbackDelay(
         sorted_packet_feedback_vector.back().arrival_time_ms);
-    consecutive_delayed_feedbacks_ = 0;
   }
-  return aggregated_result;
+  return MaybeUpdateEstimate(overusing);

[terelius, 2017/04/11 14:21:38]

Wouldn't it be easier and cleaner to just not call MaybeUpdateEstimate if the
feedback is delayed?

[michaelt, 2017/04/11 14:48:12]

Hmm i don't have a strong opinion here. 
On one hand both paths may would update/set the estimation.
So they would both fit in to a function which is called "MaybeUpdateEstimate". 

On the other hand MaybeUpdateEstimate gets another path which makes the function
almost too complex.

So if you prefer the have the delayed_feedback logic in
IncomingPacketFeedbackVector i would be Ok with it as well.

 }
 
 DelayBasedBwe::Result DelayBasedBwe::OnLongFeedbackDelay(
     int64_t arrival_time_ms) {
   // Estimate should always be valid since a start bitrate always is set in the
   // Call constructor. An alternative would be to return an empty Result here,
   // or to estimate the throughput based on the feedback we received.
   RTC_DCHECK(rate_control_.ValidEstimate());
   rate_control_.SetEstimate(rate_control_.LatestEstimate() / 2,
                             arrival_time_ms);
   Result result;
   result.updated = true;
   result.probe = false;
   result.target_bitrate_bps = rate_control_.LatestEstimate();
   LOG(LS_WARNING) << "Long feedback delay detected, reducing BWE to "
                   << result.target_bitrate_bps;
   return result;
 }
 
-DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedback(
+void DelayBasedBwe::IncomingPacketFeedback(
     const PacketFeedback& packet_feedback) {
   int64_t now_ms = clock_->TimeInMilliseconds();
 
   receiver_incoming_bitrate_.Update(packet_feedback.arrival_time_ms,
                                     packet_feedback.payload_size);
   Result result;
   // Reset if the stream has timed out.
   if (last_seen_packet_ms_ == -1 ||
       now_ms - last_seen_packet_ms_ > kStreamTimeOutMs) {
     inter_arrival_.reset(
@@ skipping 22 matching lines @@
   if (inter_arrival_->ComputeDeltas(timestamp, packet_feedback.arrival_time_ms,
                                     now_ms, packet_feedback.payload_size,
                                     &ts_delta, &t_delta, &size_delta)) {
     double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift);
     trendline_estimator_->Update(t_delta, ts_delta_ms,
                                  packet_feedback.arrival_time_ms);
     detector_.Detect(trendline_estimator_->trendline_slope(), ts_delta_ms,
                      trendline_estimator_->num_of_deltas(),
                      packet_feedback.arrival_time_ms);
   }
-
-  int probing_bps = 0;
   if (packet_feedback.pacing_info.probe_cluster_id !=
       PacedPacketInfo::kNotAProbe) {
-    probing_bps =
-        probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(packet_feedback);
+    probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(packet_feedback);
   }
+}
+
+DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate(bool overusing) {
+  Result result;
   rtc::Optional<uint32_t> acked_bitrate_bps =
       receiver_incoming_bitrate_.bitrate_bps();
+  int64_t now_ms = clock_->TimeInMilliseconds();
+  rtc::Optional<int> probe_bitrate_bps =
+      probe_bitrate_estimator_.FetchAndResetLastEstimatedBitrateBps();
   // Currently overusing the bandwidth.
-  if (detector_.State() == BandwidthUsage::kBwOverusing) {
+  if (overusing) {
     if (acked_bitrate_bps &&
         rate_control_.TimeToReduceFurther(now_ms, *acked_bitrate_bps)) {
-      result.updated =
-          UpdateEstimate(packet_feedback.arrival_time_ms, now_ms,
-                         acked_bitrate_bps, &result.target_bitrate_bps);
+      result.updated = UpdateEstimate(now_ms, acked_bitrate_bps, overusing,
+                                      &result.target_bitrate_bps);
     }
-  } else if (probing_bps > 0) {
-    // No overuse, but probing measured a bitrate.
-    rate_control_.SetEstimate(probing_bps, packet_feedback.arrival_time_ms);
-    result.probe = true;
-    result.updated =
-        UpdateEstimate(packet_feedback.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(packet_feedback.arrival_time_ms, now_ms,
-                       acked_bitrate_bps, &result.target_bitrate_bps);
+  } else {
+    if (probe_bitrate_bps) {
+      rate_control_.SetEstimate(*probe_bitrate_bps, now_ms);
+      result.probe = true;
+    }
+    result.updated = UpdateEstimate(now_ms, acked_bitrate_bps, overusing,
+                                    &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);
     if (event_log_ && (result.target_bitrate_bps != last_logged_bitrate_ ||
                        detector_.State() != last_logged_state_)) {
       event_log_->LogDelayBasedBweUpdate(result.target_bitrate_bps,
                                          detector_.State());
       last_logged_bitrate_ = result.target_bitrate_bps;
       last_logged_state_ = detector_.State();
     }
   }
-
   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,
+                                   bool overusing,
                                    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);
+  const RateControlInput input(
+      overusing ? BandwidthUsage::kBwOverusing : detector_.State(),
+      acked_bitrate_bps, 0);
   *target_bitrate_bps = rate_control_.Update(&input, now_ms);
   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 {
@@ skipping 19 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