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 27 matching lines @@
 }  // namespace
 
 namespace webrtc {
 
 DelayBasedBwe::DelayBasedBwe(Clock* clock)
     : clock_(clock),
       inter_arrival_(),
       estimator_(),
       detector_(OverUseDetectorOptions()),
       receiver_incoming_bitrate_(kBitrateWindowMs, 8000),
-      last_update_ms_(-1),
       last_seen_packet_ms_(-1),
       uma_recorded_(false) {
   network_thread_.DetachFromThread();
 }
 
 DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
     const std::vector<PacketInfo>& packet_feedback_vector) {
   RTC_DCHECK(network_thread_.CalledOnValidThread());
   if (!uma_recorded_) {
     RTC_HISTOGRAM_ENUMERATION(kBweTypeHistogram,
                               BweNames::kSendSideTransportSeqNum,
                               BweNames::kBweNamesMax);
     uma_recorded_ = true;
   }
+
+  int64_t now_ms = clock_->TimeInMilliseconds();
   Result aggregated_result;
   for (const auto& packet_info : packet_feedback_vector) {
-    Result result = IncomingPacketInfo(packet_info);
-    if (result.updated)
-      aggregated_result = result;
+    aggregated_result = IncomingPacketInfo(packet_info, now_ms);
   }
+  aggregated_result.target_bitrate_bps =
+      rate_control_.UpdateBandwidthEstimate(now_ms);
+  aggregated_result.updated = rate_control_.ValidEstimate();
   return aggregated_result;
 }
 
-DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(
-    const PacketInfo& info) {
-  int64_t now_ms = clock_->TimeInMilliseconds();
-
+DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo(const PacketInfo& info,
+                                                        int64_t now_ms) {
   receiver_incoming_bitrate_.Update(info.payload_size, info.arrival_time_ms);
-  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(
         new InterArrival((kTimestampGroupLengthMs << kInterArrivalShift) / 1000,
                          kTimestampToMs, true));
     estimator_.reset(new OveruseEstimator(OverUseDetectorOptions()));
   }
   last_seen_packet_ms_ = now_ms;
 
@@ skipping 17 matching lines @@
     estimator_->Update(t_delta, ts_delta_ms, size_delta, detector_.State(),
                        info.arrival_time_ms);
     detector_.Detect(estimator_->offset(), ts_delta_ms,
                      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);
   }
-
-  // Currently overusing the bandwidth.
-  if (detector_.State() == kBwOverusing) {
-    rtc::Optional<uint32_t> incoming_rate =
-        receiver_incoming_bitrate_.Rate(info.arrival_time_ms);
-    if (incoming_rate &&
-        rate_control_.TimeToReduceFurther(now_ms, *incoming_rate)) {

[stefan-webrtc, 2016/10/26 15:19:20]

Isn't this check still desirable? We shouldn't reduce the bitrate more than once
per rtt since we can't see the effect of it before that?

[michaelt, 2016/10/27 14:42:59]

I removed it because it seamed strange to increase the the bit-rate without
waiting time, but on the other hand wait in the overuse case. To me it would
make most sense to wait mean_rtt between all bit-rate changes. 

[stefan-webrtc, 2016/10/31 15:08:49]

I don't think I agree. Increasing is fine to do in small steps every time we get
an ack of a packet delivery. That gives us a smooth increase over time.

Reducing the bitrate is better done in one big step, and then waiting to see if
it had the desired effect. If it did not we could decrease more. The logic we
applied for increasing the rate doesn't apply since it doesn't make sense to
take small steps down for every ack we get...

-      result.updated = UpdateEstimate(info.arrival_time_ms, now_ms,
-                                      &result.target_bitrate_bps);
-    }
-  } else if (probing_bps > 0) {
+  if (detector_.State() != kBwOverusing && 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,
-                                    &result.target_bitrate_bps);
+    UpdateEstimate(info.arrival_time_ms, now_ms);
+    return Result(true, 0);
   }
-  if (!result.updated &&
-      (last_update_ms_ == -1 ||
-       now_ms - last_update_ms_ > rate_control_.GetFeedbackInterval())) {
-    result.updated = UpdateEstimate(info.arrival_time_ms, now_ms,
-                                    &result.target_bitrate_bps);
-  }
-  if (result.updated)
-    last_update_ms_ = now_ms;
-
-  return result;
+  UpdateEstimate(info.arrival_time_ms, now_ms);
+  return Result();

[stefan-webrtc, 2016/10/26 15:19:20]

I find this method a bit strange now. It basically never returns a target
bitrate, even if it actually has updated it? What's the point of that?

[michaelt, 2016/10/27 14:42:59]

Since we work with a vector of packets, it seamed strange to me, asking
aimd_rate_control for a new bit rate after each packet. Doing this after the
vector seams to me the better solution. 

But of course there might be a better impl. for this.

[stefan-webrtc, 2016/10/31 15:08:49]

I agree with that. Seems odd to me that we return an empty result here and a
result with bitrate 0 on line 118 though... Should we just have this method
return bool instead?

 }
 
-bool DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms,
-                                   int64_t now_ms,
-                                   uint32_t* target_bitrate_bps) {
+void DelayBasedBwe::UpdateEstimate(int64_t arrival_time_ms, int64_t now_ms) {
   // The first overuse should immediately trigger a new estimate.
   // We also have to update the estimate immediately if we are overusing
   // and the target bitrate is too high compared to what we are receiving.
   const RateControlInput input(detector_.State(),
                                receiver_incoming_bitrate_.Rate(arrival_time_ms),
                                estimator_->var_noise());
   rate_control_.Update(&input, now_ms);
-  *target_bitrate_bps = rate_control_.UpdateBandwidthEstimate(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 {
   // Currently accessed from both the process thread (see
   // ModuleRtpRtcpImpl::Process()) and the configuration thread (see
   // Call::GetStats()). Should in the future only be accessed from a single
   // thread.
   RTC_DCHECK(ssrcs);
   RTC_DCHECK(bitrate_bps);
   if (!rate_control_.ValidEstimate())
     return false;
 
   *ssrcs = {kFixedSsrc};
   *bitrate_bps = rate_control_.LatestEstimate();
   return true;
 }
 
 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);
 }
 }  // namespace webrtc