Add probing to recover faster from large bitrate drops.

webrtc/modules/congestion_controller/probe_controller.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.
  */
 
 #include "webrtc/modules/congestion_controller/probe_controller.h"
 
 #include <algorithm>
 #include <initializer_list>
 
 #include "webrtc/rtc_base/logging.h"
 #include "webrtc/rtc_base/safe_conversions.h"
+#include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
 
 namespace {
 // Maximum waiting time from the time of initiating probing to getting
 // the measured results back.
 constexpr int64_t kMaxWaitingTimeForProbingResultMs = 1000;
 
 // Value of |min_bitrate_to_probe_further_bps_| that indicates
 // further probing is disabled.
 constexpr int kExponentialProbingDisabled = 0;
 
 // Default probing bitrate limit. Applied only when the application didn't
 // specify max bitrate.
 constexpr int64_t kDefaultMaxProbingBitrateBps = 5000000;
 
 // This is a limit on how often probing can be done when there is a BW
 // drop detected in ALR.
-constexpr int64_t kAlrProbingIntervalMinMs = 5000;
+constexpr int64_t kMinAlrProbingIntervalMs = 5000;
 
 // Interval between probes when ALR periodic probing is enabled.
 constexpr int64_t kAlrPeriodicProbingIntervalMs = 5000;
 
 // Minimum probe bitrate percentage to probe further for repeated probes,
 // relative to the previous probe. For example, if 1Mbps probe results in
 // 80kbps, then we'll probe again at 1.6Mbps. In that case second probe won't be
 // sent if we get 600kbps from the first one.
 constexpr int kRepeatedProbeMinPercentage = 70;
 
+// Timeout for probing after leaving ALR. If the bitrate drops significantly,
+// (as determined by the delay based estimator) and we leave ALR, then we will
+// send a probe if we recover within |kLeftAlrTimeoutMs| ms.
+constexpr int kAlrEndedTimeoutMs = 3000;
+
+// Use probing to recover faster after large bitrate estimate drops.
+constexpr char kBweRapidRecoveryExperiment[] =
+    "WebRTC-BweRapidRecoveryExperiment";
+
 }  // namespace
 
 ProbeController::ProbeController(PacedSender* pacer, const Clock* clock)
     : pacer_(pacer), clock_(clock), enable_periodic_alr_probing_(false) {
   Reset();
+  in_rapid_recovery_experiment_ = webrtc::field_trial::FindFullName(
+                                      kBweRapidRecoveryExperiment) == "Enabled";
 }
 
 void ProbeController::SetBitrates(int64_t min_bitrate_bps,
                                   int64_t start_bitrate_bps,
                                   int64_t max_bitrate_bps) {
   rtc::CritScope cs(&critsect_);
 
   if (start_bitrate_bps > 0)  {
     start_bitrate_bps_ = start_bitrate_bps;
     estimated_bitrate_bps_ = start_bitrate_bps;
@@ skipping 76 matching lines @@
                  << " Minimum to probe further: "
                  << min_bitrate_to_probe_further_bps_;
 
     if (min_bitrate_to_probe_further_bps_ != kExponentialProbingDisabled &&
         bitrate_bps > min_bitrate_to_probe_further_bps_) {
       // Double the probing bitrate.
       InitiateProbing(now_ms, {2 * bitrate_bps}, true);
     }
   }
 
-  // Detect a drop in estimated BW when operating in ALR and not already
-  // probing. The current response is to initiate a single probe session at the
-  // previous bitrate and immediately use the reported bitrate as the new
-  // bitrate.
-  //
-  // If the probe session fails, the assumption is that this drop was a
-  // real one from a competing flow or something else on the network and
-  // it ramps up from bitrate_bps.
-  if (state_ == State::kProbingComplete &&
-      pacer_->GetApplicationLimitedRegionStartTime() &&
-      bitrate_bps < 2 * estimated_bitrate_bps_ / 3 &&
-      (now_ms - last_alr_probing_time_) > kAlrProbingIntervalMinMs) {
-    LOG(LS_INFO) << "Detected big BW drop in ALR, start probe.";
-    // Track how often we probe in response to BW drop in ALR.
-    RTC_HISTOGRAM_COUNTS_10000("WebRTC.BWE.AlrProbingIntervalInS",
-                               (now_ms - last_alr_probing_time_) / 1000);
-    InitiateProbing(now_ms, {estimated_bitrate_bps_}, false);
-    last_alr_probing_time_ = now_ms;
-
-    // TODO(isheriff): May want to track when we did ALR probing in order
-    // to reset |last_alr_probing_time_| if we validate that it was a
-    // drop due to exogenous event.
-  }
-
   estimated_bitrate_bps_ = bitrate_bps;
 }
 
 void ProbeController::EnablePeriodicAlrProbing(bool enable) {
   rtc::CritScope cs(&critsect_);
   enable_periodic_alr_probing_ = enable;
 }
 
+void ProbeController::SetAlrEndedTimeMs(int64_t alr_end_time_ms) {
+  rtc::CritScope cs(&critsect_);
+  alr_end_time_ms_.emplace(alr_end_time_ms);
+}
+
+void ProbeController::RequestProbe(uint32_t suggested_probing_bitrate_bps) {
+  int64_t now_ms = clock_->TimeInMilliseconds();
+  rtc::CritScope cs(&critsect_);
+  // Called once we have returned to normal state after a large drop in
+  // estimated bandwidth. The current response is to initiate a single probe
+  // session (if not already probing) at the previous bitrate.
+  //
+  // If the probe session fails, the assumption is that this drop was a
+  // real one from a competing flow or a network change.
+  if (state_ == State::kProbingComplete &&
+      suggested_probing_bitrate_bps > estimated_bitrate_bps_ &&

[philipel, 2017/07/25 11:26:04]

You might want to use a factor of the probing bitrate to avoid probing when the
BWE is close to the suggested probing bitrate, probing being somewhat jittery
and all.

[terelius, 2017/07/27 21:02:45]

Done. I set the "probe uncertainty" to 5% of the target rate. Does that seem
reasonable?

+      (now_ms - last_bwe_drop_probing_time_ms_) > kMinAlrProbingIntervalMs) {
+    bool in_alr = pacer_->GetApplicationLimitedRegionStartTime().has_value();
+    bool alr_ended_recently =
+        (alr_end_time_ms_.has_value() &&

[philipel, 2017/07/25 11:26:04]

I think it's cleaner to implement ALR end time in the ALR detector, this way you
can avoid SetAlrEndedTimeMs and |alr_end_time_ms_|.

[terelius, 2017/07/27 21:02:45]

In principle, I agree. However, that would require us to add a GetAlrEndTime to
the pacer, and it still wouldn't be reusable in the AcknowledgedBitrateEstimator
because we want to react to the ALR change exactly once. Because different
classes may want to determine when they have handled the "ALR ended" signal, we
decided to just pass the state change from SendSideCongestionController and let
each class track it separately.

Besides, if the experiment is positive we'll probably make it default which
means that we won't have to test for "ALR ended" anyway.

If you have any ideas on how to implement it in the AlrDetector in a way that
will work for both the AcknowledgedBitrateEstimator and the ProbeController, I'm
all ears. :)

[terelius, 2017/07/28 17:14:47]

Since the method you proposed will involve changes to the pacer, the alr
detector and the acknowledged bitrate estimator I'd prefer doing that in a
follow-up CL.

[philipel, 2017/07/31 09:40:17]

Acknowledged.

+         now_ms - alr_end_time_ms_.value() < kAlrEndedTimeoutMs);
+    if (in_alr || alr_ended_recently || in_rapid_recovery_experiment_) {
+      LOG(LS_INFO) << "Detected big bandwidth drop, start probing.";
+      // Track how often we probe in response to bandwidth drop in ALR.
+      RTC_HISTOGRAM_COUNTS_10000(
+          "WebRTC.BWE.BweDropProbingIntervalInS",
+          (now_ms - last_bwe_drop_probing_time_ms_) / 1000);
+      InitiateProbing(now_ms, {suggested_probing_bitrate_bps}, false);
+      last_bwe_drop_probing_time_ms_ = now_ms;
+
+      // TODO(isheriff): May want to track when we did ALR probing in order

[philipel, 2017/07/25 11:26:04]

Does this comment make sense anymore? Otherwise remove it.

[terelius, 2017/07/27 21:02:45]

I have no idea what it means/meant. I just copied it from the old code. Do you
think I should remove it?

[terelius, 2017/07/28 17:14:47]

Removed it.

+      // to reset |last_bwe_drop_probing_time_ms_| if we validate that it was a
+      // drop due to exogenous event.
+    }
+  }
+}
+
 void ProbeController::Reset() {
   rtc::CritScope cs(&critsect_);
   network_state_ = kNetworkUp;
   state_ = State::kInit;
   min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
   time_last_probing_initiated_ms_ = 0;
   estimated_bitrate_bps_ = 0;
   start_bitrate_bps_ = 0;
   max_bitrate_bps_ = 0;
-  last_alr_probing_time_ = clock_->TimeInMilliseconds();
+  last_bwe_drop_probing_time_ms_ = clock_->TimeInMilliseconds();
+  alr_end_time_ms_.reset();
   mid_call_probing_waiting_for_result_ = false;
 }
 
 void ProbeController::Process() {
   rtc::CritScope cs(&critsect_);
 
   int64_t now_ms = clock_->TimeInMilliseconds();
 
   if (now_ms - time_last_probing_initiated_ms_ >
       kMaxWaitingTimeForProbingResultMs) {
@@ skipping 41 matching lines @@
     state_ = State::kWaitingForProbingResult;
     min_bitrate_to_probe_further_bps_ =
         (*(bitrates_to_probe.end() - 1)) * kRepeatedProbeMinPercentage / 100;
   } else {
     state_ = State::kProbingComplete;
     min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
   }
 }
 
 }  // namespace webrtc