Implement periodic bandwidth probing in application-limited region.

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 272c1eb4e598c82b859beab9548e5258b8dcea4e..ece0a773d15fab71b93fd751004e03aff6a52a43 100644
--- a/webrtc/modules/congestion_controller/probe_controller.cc
+++ b/webrtc/modules/congestion_controller/probe_controller.cc
@@ -39,7 +39,10 @@ constexpr int kDefaultMaxProbingBitrateBps = 100000000;
 
 // This is a limit on how often probing can be done when there is a BW
 // drop detected in ALR region.
-constexpr int kAlrProbingIntervalLimitMs = 5000;
+constexpr int64_t kAlrProbingIntervalMinMs = 5000;
+
+// Increase periodic probes intervals up to 2 minutes.
+constexpr int64_t kAlrProbingIntervalMaxMs = 120000;
 
 }  // namespace
 
@@ -53,7 +56,9 @@ ProbeController::ProbeController(PacedSender* pacer, Clock* clock)
       estimated_bitrate_bps_(0),
       start_bitrate_bps_(0),
       max_bitrate_bps_(0),
-      last_alr_probing_time_(clock_->TimeInMilliseconds()) {}
+      last_alr_probing_time_(clock_->TimeInMilliseconds()),
+      enable_periodic_probing_(false),
+      last_periodic_probing_time_(clock_->TimeInMilliseconds()) {}
 
 void ProbeController::SetBitrates(int min_bitrate_bps,
                                   int start_bitrate_bps,
@@ -80,7 +85,8 @@ void ProbeController::SetBitrates(int min_bitrate_bps,
       if (estimated_bitrate_bps_ != 0 &&
           estimated_bitrate_bps_ < old_max_bitrate_bps &&
           max_bitrate_bps_ > old_max_bitrate_bps) {
-        InitiateProbing({max_bitrate_bps}, kExponentialProbingDisabled);
+        InitiateProbing(clock_->TimeInMilliseconds(), {max_bitrate_bps},
+                        kExponentialProbingDisabled);
       }
       break;
   }
@@ -100,14 +106,17 @@ void ProbeController::InitiateExponentialProbing() {
 
   // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of
   // 1.2 Mbps to continue probing.
-  InitiateProbing({3 * start_bitrate_bps_, 6 * start_bitrate_bps_},
+  InitiateProbing(clock_->TimeInMilliseconds(),
+                  {3 * start_bitrate_bps_, 6 * start_bitrate_bps_},
                   4 * start_bitrate_bps_);
 }
 
 void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
   rtc::CritScope cs(&critsect_);
+  int64_t now_ms = clock_->TimeInMilliseconds();
+
   if (state_ == State::kWaitingForProbingResult) {
-    if ((clock_->TimeInMilliseconds() - time_last_probing_initiated_ms_) >
+    if ((now_ms - time_last_probing_initiated_ms_) >
         kMaxWaitingTimeForProbingResultMs) {
       LOG(LS_INFO) << "kWaitingForProbingResult: timeout";
       state_ = State::kProbingComplete;
@@ -122,38 +131,81 @@ void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
           bitrate_bps > min_bitrate_to_probe_further_bps_) {
         // Double the probing bitrate and expect a minimum of 25% gain to
         // continue probing.
-        InitiateProbing({2 * bitrate_bps}, 1.25 * bitrate_bps);
-      }
-    }
-  } else {
-    // 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 (pacer_->InApplicationLimitedRegion() &&
-        bitrate_bps < 0.5 * estimated_bitrate_bps_) {
-      int64_t now_ms = clock_->TimeInMilliseconds();
-      if ((now_ms - last_alr_probing_time_) > kAlrProbingIntervalLimitMs) {
-        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({estimated_bitrate_bps_}, kExponentialProbingDisabled);
-        last_alr_probing_time_ = now_ms;
+        InitiateProbing(now_ms, {2 * bitrate_bps}, bitrate_bps * 5 / 4);
+      } else {
+        // Stop exponential probing.
+        state_ = State::kProbingComplete;
+        min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
       }
     }
+  }
+
+  // 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() &&

[philipel, 2016/11/16 14:59:15]

I think it's cleaner to keep the InApplicationLimitedRegion function in addition
to the new GetApplicationLimitedRegionStartTime.

[Sergey Ulanov, 2016/11/21 09:06:03]

I don't think keeping InApplicationLimitedRegion() next to
GetApplicationLimitedRegionStartTime() is the right thing to do, because
separating them makes it easier to use them incorrectly. Particularly because
pacer_ runs on a different threads, and so ALR state may change between the two
calls. Another alternative here is to have GetApplicationLimitedRegionState()
that would return both the bool state and start time, e.g. as a struct:
 struct AlrState {
   bool in_alr;
   int64_t start_time;
 }

But this looks like it adds more complexity than we need here. Another option is
to return rtc::Option<int64_t> from GetApplicationLimitedRegionStartTime().
WDYT?

[philipel, 2016/11/21 12:05:42]

I see what you mean about having both InApplicationLimitedRegion and
GetApplicationLimitedRegionStartTime, so I agree it's better to only have
GetApplicationLimitedRegionStartTime.

I think using rtc::Optional is the best option in this case (we want to move
away from magic constants).

[Sergey Ulanov, 2016/11/21 19:28:18]

Done.

+      bitrate_bps < estimated_bitrate_bps_ / 2 &&
+      (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_},
+                    kExponentialProbingDisabled);
+    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::EnablePeriodicProbing(bool enable) {
+  rtc::CritScope cs(&critsect_);
+  enable_periodic_probing_ = enable;
+}
+
+void ProbeController::Process() {
+  rtc::CritScope cs(&critsect_);
+
+  if (state_ != State::kProbingComplete || !enable_periodic_probing_)
+    return;
+
+  // Probe bandwidth periodically when in ALR state.
+  int64_t alr_start_time = pacer_->GetApplicationLimitedRegionStartTime();
+  if (alr_start_time > 0) {
+    int64_t now_ms = clock_->TimeInMilliseconds();
+    int64_t time_in_alr_ms = now_ms - alr_start_time;
+
+    // Calculate probing period as half of the time passed since ALR started,
+    // clamped between min and max values. This results in the interval
+    // increasing from the minimum exponentially until it reaches the maximum.
+    // With min and max values set to 5s and 2m the probes are sent at 5s, 10s,
+    // 20s, 40s, 80s, 160s and every 2 minutes after that.
+    int alr_probe_period_ms =

[philipel, 2016/11/16 14:59:15]

Probing is very cheap (but jittery) so I don't think think we need exponential
backoff. I think an interval of 5-10 seconds would work well.

Have you done any tests regarding this behavior?

[Sergey Ulanov, 2016/11/21 09:06:03]

The idea here was that the longer the sender stays in ALR the less likely it is
that it will need the extra bandwidth, but yes, it's premature to implement
exponential backoff here. Changed it to regular interval of 5 second. Will see
how well that works.

What did you mean by "jittery"?

[philipel, 2016/11/21 12:05:42]

Lets say we have a stable link that does not change, probing this link will
result in estimates that are fairly jittery, i.e. you will get different results
every time (sometimes even higher than the actual available bandwidth).

[Sergey Ulanov, 2016/11/21 19:28:18]

Right, there is a lot of noise in the probes result, but I don't think it's an
issue in ALR, it won't be noticeable to the user.

+        std::min(std::max(kAlrProbingIntervalMinMs, time_in_alr_ms / 2),
+                 kAlrProbingIntervalMaxMs);
+
+    int time_since_last_probe_ms =

[philipel, 2016/11/16 14:59:15]

I'm not sure why you added the |now_ms| argument to InitiateProbing (since it's
not used), but don't you think it's better if we calculate the time since the
last probing was initiated?

[Sergey Ulanov, 2016/11/21 09:06:03]

That was a mistake, fixed that function to use now_ms.
Done. Also simplified this code a bit.

+        std::min(time_in_alr_ms, now_ms - last_periodic_probing_time_);
+    if (time_since_last_probe_ms >= alr_probe_period_ms) {
+      InitiateProbing(now_ms, {estimated_bitrate_bps_ * 2},
+                      estimated_bitrate_bps_ * 5 / 4);
+      last_periodic_probing_time_ = now_ms;
+    }
+  }
+}
+
 void ProbeController::InitiateProbing(
+    int64_t now_ms,

[philipel, 2016/11/16 14:59:15]

Not used?

[Sergey Ulanov, 2016/11/21 09:06:03]

Done.

     std::initializer_list<int> bitrates_to_probe,
     int min_bitrate_to_probe_further_bps) {
   bool first_cluster = true;