Fix integer overflow in ProbeController.

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 10573c302941044fc236b95250207b9a0965055d..19a4a2e5ec107216132f09f3ca058f5464c80a17 100644
--- a/webrtc/modules/congestion_controller/probe_controller.cc
+++ b/webrtc/modules/congestion_controller/probe_controller.cc
@@ -14,6 +14,7 @@
 #include <initializer_list>
 
 #include "webrtc/base/logging.h"
+#include "webrtc/base/safe_conversions.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
@@ -44,8 +45,11 @@ constexpr int64_t kAlrProbingIntervalMinMs = 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.
-constexpr int kRepeatedProbeMinPercentage = 125;
+// 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;
 
 }  // namespace
 
@@ -87,11 +91,11 @@ void ProbeController::SetBitrates(int min_bitrate_bps,
 
     case State::kProbingComplete:
       // Initiate probing when |max_bitrate_| was increased mid-call.
-      if (estimated_bitrate_bps_ != 0 &&
+      if (estimated_bitrate_bps_ != kExponentialProbingDisabled &&
           estimated_bitrate_bps_ < old_max_bitrate_bps &&
           max_bitrate_bps_ > old_max_bitrate_bps) {
         InitiateProbing(clock_->TimeInMilliseconds(), {max_bitrate_bps},
-                        kExponentialProbingDisabled);
+                        /*probe_furter=*/false);

[philipel, 2016/12/13 14:26:27]

In general, if you want to describe the parameter just:
const bool probe_further = false;
InitiateProbing(..., probe_further);

I don't think you need to describe it though, just use |true| or |false|.

[Sergey Ulanov, 2016/12/13 19:38:09]

Done.

       }
       break;
   }
@@ -112,8 +116,9 @@ void ProbeController::InitiateExponentialProbing() {
   // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of
   // 1.2 Mbps to continue probing.
   InitiateProbing(clock_->TimeInMilliseconds(),
-                  {3 * start_bitrate_bps_, 6 * start_bitrate_bps_},
-                  4 * start_bitrate_bps_);
+                  {3 * static_cast<int64_t>(start_bitrate_bps_),
+                   6 * static_cast<int64_t>(start_bitrate_bps_)},
+                  /*probe_furter=*/true);
 }
 
 void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
@@ -128,10 +133,9 @@ void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
                  << 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 and expect a minimum of 25% gain to
-      // continue probing.
-      InitiateProbing(now_ms, {2 * bitrate_bps},
-                      bitrate_bps * kRepeatedProbeMinPercentage / 100);
+      // Double the probing bitrate.
+      InitiateProbing(now_ms, {2 * static_cast<int64_t>(bitrate_bps)},
+                      /*probe_furter=*/true);
     }
   }
 
@@ -151,8 +155,7 @@ void ProbeController::SetEstimatedBitrate(int bitrate_bps) {
     // 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);
+    InitiateProbing(now_ms, {estimated_bitrate_bps_}, /*probe_furter=*/false);
     last_alr_probing_time_ = now_ms;
 
     // TODO(isheriff): May want to track when we did ALR probing in order
@@ -192,22 +195,25 @@ void ProbeController::Process() {
         std::max(*alr_start_time, time_last_probing_initiated_ms_) +
         kAlrPeriodicProbingIntervalMs;
     if (now_ms >= next_probe_time_ms) {
-      InitiateProbing(
-          now_ms, {estimated_bitrate_bps_ * 2},
-          estimated_bitrate_bps_ * kRepeatedProbeMinPercentage / 100);
+      InitiateProbing(now_ms,
+                      {static_cast<int64_t>(estimated_bitrate_bps_) * 2},
+                      /*probe_furter=*/true);
     }
   }
 }
 
 void ProbeController::InitiateProbing(
     int64_t now_ms,
-    std::initializer_list<int> bitrates_to_probe,
-    int min_bitrate_to_probe_further_bps) {
+    std::initializer_list<int64_t> bitrates_to_probe,
+    bool probe_further) {
   bool first_cluster = true;
-  for (int bitrate : bitrates_to_probe) {
+  for (int64_t bitrate : bitrates_to_probe) {
     int max_probe_bitrate_bps =
         max_bitrate_bps_ > 0 ? max_bitrate_bps_ : kDefaultMaxProbingBitrateBps;
-    bitrate = std::min(bitrate, max_probe_bitrate_bps);
+    if (bitrate > max_probe_bitrate_bps) {
+      bitrate = max_probe_bitrate_bps;
+      probe_further = false;
+    }
     if (first_cluster) {
       pacer_->CreateProbeCluster(bitrate, kProbeDeltasPerCluster + 1);
       first_cluster = false;
@@ -215,12 +221,15 @@ void ProbeController::InitiateProbing(
       pacer_->CreateProbeCluster(bitrate, kProbeDeltasPerCluster);
     }
   }
-  min_bitrate_to_probe_further_bps_ = min_bitrate_to_probe_further_bps;
   time_last_probing_initiated_ms_ = now_ms;
-  if (min_bitrate_to_probe_further_bps == kExponentialProbingDisabled)
-    state_ = State::kProbingComplete;
-  else
+  if (probe_further) {
     state_ = State::kWaitingForProbingResult;
+    min_bitrate_to_probe_further_bps_ = rtc::checked_cast<int>(

[philipel, 2016/12/13 14:26:27]

As I see it, we are ok to store bitrates in 32 bit integers (maxing out at 2.1
Gbps).

Don't you think it is easier to just do the calculations in 64 bits and then
continue to store the bitrates in ints? If not, I think we should use int64_t
for all bitrates.

[Sergey Ulanov, 2016/12/13 19:38:09]

Changed all bps variables in this class to int64_t. FWIW style guide says the
following:
So yes, int64_t is a better choice for bps values.

+        (*(bitrates_to_probe.end() - 1)) * kRepeatedProbeMinPercentage / 100);
+  } else {
+    state_ = State::kProbingComplete;
+    min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled;
+  }
 }
 
 }  // namespace webrtc