Add probing to recover faster from large bitrate drops.

webrtc/modules/congestion_controller/probe_controller.cc

Index: webrtc/modules/congestion_controller/probe_controller.cc
diff --git a/webrtc/modules/congestion_controller/probe_controller.cc b/webrtc/modules/congestion_controller/probe_controller.cc
index 43889bce9abe924c4409db6d828595d48d1978bf..1f1fd5973884ae3a9ee758dde3d45ed0e1f66816 100644
--- a/webrtc/modules/congestion_controller/probe_controller.cc
+++ b/webrtc/modules/congestion_controller/probe_controller.cc
@@ -15,6 +15,7 @@
 
 #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 {
@@ -34,7 +35,7 @@ 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;
@@ -45,11 +46,22 @@ constexpr int64_t kAlrPeriodicProbingIntervalMs = 5000;
 // 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,
@@ -146,30 +158,6 @@ void ProbeController::SetEstimatedBitrate(int64_t bitrate_bps) {
     }
   }
 
-  // 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;
 }
 
@@ -178,6 +166,43 @@ void ProbeController::EnablePeriodicAlrProbing(bool enable) {
   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;
@@ -187,7 +212,8 @@ void ProbeController::Reset() {
   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;
 }