Prevent BWE rampdowns without new loss reports.

webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc

 /*
  *  Copyright (c) 2012 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 31 matching lines @@
 
 }
 
 SendSideBandwidthEstimation::SendSideBandwidthEstimation()
     : accumulate_lost_packets_Q8_(0),
       accumulate_expected_packets_(0),
       bitrate_(0),
       min_bitrate_configured_(kDefaultMinBitrateBps),
       max_bitrate_configured_(kDefaultMaxBitrateBps),
       last_low_bitrate_log_ms_(-1),
-      time_last_receiver_block_ms_(0),
+      has_decreased_since_last_fraction_loss_(false),
+      time_last_receiver_block_ms_(-1),
       last_fraction_loss_(0),
       last_round_trip_time_ms_(0),
       bwe_incoming_(0),
       time_last_decrease_ms_(0),
       first_report_time_ms_(-1),
       initially_lost_packets_(0),
       bitrate_at_2_seconds_kbps_(0),
       uma_update_state_(kNoUpdate),
-      rampup_uma_stats_updated_(kNumUmaRampupMetrics, false) {
-}
+      rampup_uma_stats_updated_(kNumUmaRampupMetrics, false) {}
 
 SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {}
 
 void SendSideBandwidthEstimation::SetSendBitrate(int bitrate) {
   RTC_DCHECK_GT(bitrate, 0);
   bitrate_ = bitrate;
 
   // Clear last sent bitrate history so the new value can be used directly
   // and not capped.
   min_bitrate_history_.clear();
@@ skipping 38 matching lines @@
 
   // Update RTT.
   last_round_trip_time_ms_ = rtt;
 
   // Check sequence number diff and weight loss report
   if (number_of_packets > 0) {
     // Calculate number of lost packets.
     const int num_lost_packets_Q8 = fraction_loss * number_of_packets;
     // Accumulate reports.
     accumulate_lost_packets_Q8_ += num_lost_packets_Q8;
     accumulate_expected_packets_ += number_of_packets;

[mflodman, 2015/10/22 07:06:58]

Old, but I'd like to change name on this variable, to me accumulate(d) sounds
more like number of packets from the start. Can we change to something like
since last?

[pbos-webrtc, 2015/10/22 13:16:06]

Done.

[mflodman, 2015/10/22 14:34:34]

Thanks!

 
     // Report loss if the total report is based on sufficiently many packets.

[stefan-webrtc, 2015/10/22 09:20:47]

Maybe change this comment to explain the if-statement and not what comes after
it.

[pbos-webrtc, 2015/10/22 13:16:05]

Done.

-    if (accumulate_expected_packets_ >= kLimitNumPackets) {
-      last_fraction_loss_ =
-          accumulate_lost_packets_Q8_ / accumulate_expected_packets_;
+    if (accumulate_expected_packets_ < kLimitNumPackets)
+      return;
 
-      // Reset accumulators.
-      accumulate_lost_packets_Q8_ = 0;
-      accumulate_expected_packets_ = 0;
-    } else {
-      // Early return without updating estimate.
-      return;
-    }
+    has_decreased_since_last_fraction_loss_ = false;
+    last_fraction_loss_ =
+        accumulate_lost_packets_Q8_ / accumulate_expected_packets_;
+
+    // Reset accumulators.
+    accumulate_lost_packets_Q8_ = 0;
+    accumulate_expected_packets_ = 0;
   }
   time_last_receiver_block_ms_ = now_ms;
   UpdateEstimate(now_ms);
   UpdateUmaStats(now_ms, rtt, (fraction_loss * number_of_packets) >> 8);
 }
 
 void SendSideBandwidthEstimation::UpdateUmaStats(int64_t now_ms,
                                                  int64_t rtt,
                                                  int lost_packets) {
   int bitrate_kbps = static_cast<int>((bitrate_ + 500) / 1000);
@@ skipping 34 matching lines @@
   // packet loss reported, to allow startup bitrate probing.
   if (last_fraction_loss_ == 0 && IsInStartPhase(now_ms) &&
       bwe_incoming_ > bitrate_) {
     bitrate_ = CapBitrateToThresholds(now_ms, bwe_incoming_);
     min_bitrate_history_.clear();
     min_bitrate_history_.push_back(std::make_pair(now_ms, bitrate_));
     return;
   }
   UpdateMinHistory(now_ms);
   // Only start updating bitrate when receiving receiver blocks.
-  if (time_last_receiver_block_ms_ != 0) {
+  if (time_last_receiver_block_ms_ != -1) {

[mflodman, 2015/10/22 07:06:57]

I'd like to discuss if we should check already here or not.

[stefan-webrtc, 2015/10/22 09:20:47]

Are you talking about whether we should check for a receiver report before we
allow any ramp-up to start (current behaviour), or whether we should only allow
a ramp-up when receiving a new receiver report?

Current behavior is to ramp-up in small steps over a receiver report interval.

[mflodman, 2015/10/22 11:22:29]

Yes. But also, what happens if we're in an increasing state without REMB
negotiated and we stop receiving RR? I see the point when having REMB as a cap
and that is the normal state for us, but I think it would be interesting to
quickly discuss what we think is the right behavior.

I'll admit though it was a while since I dug deep into this code...

[stefan-webrtc, 2015/10/22 11:28:06]

Yes, if we don't have REMB this isn't great. Overall, I think we need some
mechanism to handle the case where we don't receive feedback, for instance
0-2 seconds since feedback: continue as normal
2-4 seconds since feedback: no increase
4-10 seconds since feedback: ramp-down towards min bitrate

But let's discuss!

[pbos-webrtc, 2015/10/22 13:16:06]

Sure, I put a TODO for now.

     if (last_fraction_loss_ <= 5) {
       // Loss < 2%: Increase rate by 8% of the min bitrate in the last
       // kBweIncreaseIntervalMs.
       // Note that by remembering the bitrate over the last second one can
       // rampup up one second faster than if only allowed to start ramping
       // at 8% per second rate now. E.g.:
       //   If sending a constant 100kbps it can rampup immediatly to 108kbps
       //   whenever a receiver report is received with lower packet loss.
       //   If instead one would do: bitrate_ *= 1.08^(delta time), it would
       //   take over one second since the lower packet loss to achieve 108kbps.
       bitrate_ = static_cast<uint32_t>(
           min_bitrate_history_.front().second * 1.08 + 0.5);
 
       // Add 1 kbps extra, just to make sure that we do not get stuck
       // (gives a little extra increase at low rates, negligible at higher
       // rates).
       bitrate_ += 1000;
 
     } else if (last_fraction_loss_ <= 26) {
       // Loss between 2% - 10%: Do nothing.
-
     } else {
       // Loss > 10%: Limit the rate decreases to once a kBweDecreaseIntervalMs +
       // rtt.
-      if ((now_ms - time_last_decrease_ms_) >=
-          (kBweDecreaseIntervalMs + last_round_trip_time_ms_)) {
+      if (!has_decreased_since_last_fraction_loss_ &&

[mflodman, 2015/10/22 07:06:57]

You can use 'accumulate_expected_packets_' instead of a new bool.

[pbos-webrtc, 2015/10/22 13:16:05]

Nope, accumulate_expected_packets_ gets reset every time last_fraction_lost_
gets updated, and not after UpdateEstimate succeeds.

[mflodman, 2015/10/22 14:34:34]

I was unclear in this comment, but we can avoid the bool if we use
'accumulate_expected+packets_' where I suggested in my comment above and reset
it after the Update function.

[pbos-webrtc, 2015/10/22 14:59:45]

That sounds like it'd be a bit harder to understand though?

+          (now_ms - time_last_decrease_ms_) >=
+              (kBweDecreaseIntervalMs + last_round_trip_time_ms_)) {
         time_last_decrease_ms_ = now_ms;
 
         // Reduce rate:
         //   newRate = rate * (1 - 0.5*lossRate);
         //   where packetLoss = 256*lossRate;
         bitrate_ = static_cast<uint32_t>(
             (bitrate_ * static_cast<double>(512 - last_fraction_loss_)) /
             512.0);
+        has_decreased_since_last_fraction_loss_ = true;
       }
     }
   }
   bitrate_ = CapBitrateToThresholds(now_ms, bitrate_);
 }
 
 bool SendSideBandwidthEstimation::IsInStartPhase(int64_t now_ms) const {
   return first_report_time_ms_ == -1 ||
          now_ms - first_report_time_ms_ < kStartPhaseMs;
 }
@@ skipping 32 matching lines @@
       LOG(LS_WARNING) << "Estimated available bandwidth " << bitrate / 1000
                       << " kbps is below configured min bitrate "
                       << min_bitrate_configured_ / 1000 << " kbps.";
       last_low_bitrate_log_ms_ = now_ms;
     }
     bitrate = min_bitrate_configured_;
   }
   return bitrate;
 }
 }  // namespace webrtc