Fix BitrateProber to match the requested bitrate more precisely

webrtc/modules/pacing/bitrate_prober.cc

Index: webrtc/modules/pacing/bitrate_prober.cc
diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc
index 431aa218811f37d1a411bffbf8dd3e44b8406edc..475f7a393f856b8c6b87256f3148075ce75512f0 100644
--- a/webrtc/modules/pacing/bitrate_prober.cc
+++ b/webrtc/modules/pacing/bitrate_prober.cc
@@ -20,24 +20,14 @@ namespace webrtc {
 
 namespace {
 
-// Inactivity threshold above which probing is restarted.
-constexpr int kInactivityThresholdMs = 5000;
-
 // A minimum interval between probes to allow scheduling to be feasible.
 constexpr int kMinProbeDeltaMs = 1;
 
-int ComputeDeltaFromBitrate(size_t probe_size, uint32_t bitrate_bps) {
-  RTC_CHECK_GT(bitrate_bps, 0);
-  // Compute the time delta needed to send probe_size bytes at bitrate_bps
-  // bps. Result is in milliseconds.
-  return static_cast<int>((1000ll * probe_size * 8) / bitrate_bps);
-}
 }  // namespace
 
 BitrateProber::BitrateProber()
     : probing_state_(ProbingState::kDisabled),
-      probe_size_last_sent_(0),
-      time_last_probe_sent_ms_(-1),
+      next_probe_time_ms_(-1),
       next_cluster_id_(0) {
   SetEnabled(true);
 }
@@ -64,6 +54,8 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) {
   if (probing_state_ == ProbingState::kInactive &&
       !clusters_.empty() &&
       packet_size >= PacedSender::kMinProbePacketSize) {
+    // Send next probe right away.
+    next_probe_time_ms_ = -1;
     probing_state_ = ProbingState::kActive;
   }
 }
@@ -83,9 +75,6 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps, int num_probes) {
 }
 
 void BitrateProber::ResetState() {
-  time_last_probe_sent_ms_ = -1;
-  probe_size_last_sent_ = 0;
-
   // Recreate all probing clusters.
   std::queue<ProbeCluster> clusters;
   clusters.swap(clusters_);
@@ -103,36 +92,19 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   // Probing is not active or probing is already complete.
   if (probing_state_ != ProbingState::kActive || clusters_.empty())
     return -1;
-  // time_last_probe_sent_ms_ of -1 indicates no probes have yet been sent.
-  int64_t elapsed_time_ms;
-  if (time_last_probe_sent_ms_ == -1) {
-    elapsed_time_ms = 0;
-  } else {
-    elapsed_time_ms = now_ms - time_last_probe_sent_ms_;
-  }
-  // If no probes have been sent for a while, abort current probing and
-  // reset.
-  if (elapsed_time_ms > kInactivityThresholdMs) {

[Sergey Ulanov, 2017/01/03 23:28:59]

I couldn't see any case when this condition would be useful. As far as I can
tell probes would be suspended below by kMaxProbeDelayMs before reaching 5000 ms
here. Am I missing something?

-    ResetState();
-    return -1;
-  }
-  // We will send the first probe packet immediately if no packet has been
-  // sent before.
+
   int time_until_probe_ms = 0;
-  if (probe_size_last_sent_ != 0 && probing_state_ == ProbingState::kActive) {
-    int next_delta_ms = ComputeDeltaFromBitrate(
-        probe_size_last_sent_, clusters_.front().probe_bitrate_bps);
-    time_until_probe_ms = next_delta_ms - elapsed_time_ms;
-    // If we have waited more than 3 ms for a new packet to probe with we will
-    // consider this probing session over.
+  if (next_probe_time_ms_ >= 0) {
+    time_until_probe_ms = next_probe_time_ms_ - now_ms;
+    // If we have waited more than 3 ms for a new packet we will restart probing
+    // again later.
     const int kMaxProbeDelayMs = 3;
-    if (next_delta_ms < kMinProbeDeltaMs ||
-        time_until_probe_ms < -kMaxProbeDelayMs) {
-      probing_state_ = ProbingState::kSuspended;

[Sergey Ulanov, 2017/01/03 23:28:59]

This logic seems to be incorrect. It could suspend probing in a middle of a
cluster and leave that cluster in that state. The remaining part of that cluster
would be restarted when probing is requested again, which makes little sense. I
changed this code to call ResetState() when probe is delayed by more than 3 ms.

-      LOG(LS_INFO) << "Delta too small or missed probing accurately, suspend";
-      time_until_probe_ms = 0;
+    if (time_until_probe_ms < -kMaxProbeDelayMs) {
+      ResetState();
+      return -1;
     }
   }
+
   return std::max(time_until_probe_ms, 0);
 }
 
@@ -147,22 +119,40 @@ int BitrateProber::CurrentClusterId() const {
 // feasible.
 size_t BitrateProber::RecommendedMinProbeSize() const {
   RTC_DCHECK(!clusters_.empty());
-  return clusters_.front().probe_bitrate_bps * 2 * kMinProbeDeltaMs /
+  return (clusters_.front().probe_bitrate_bps * 2 * kMinProbeDeltaMs) /
          (8 * 1000);
 }
 
 void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) {
   RTC_DCHECK(probing_state_ == ProbingState::kActive);
   RTC_DCHECK_GT(bytes, 0);
-  probe_size_last_sent_ = bytes;
-  time_last_probe_sent_ms_ = now_ms;
+
   if (!clusters_.empty()) {
     ProbeCluster* cluster = &clusters_.front();
+    if (cluster->sent_probes == 0) {
+      RTC_DCHECK_EQ(cluster->time_started_ms, -1);
+      cluster->time_started_ms = now_ms;
+    }
     ++cluster->sent_probes;
+    cluster->bytes_sent += bytes;
+    next_probe_time_ms_ = GetNextProbeTime(clusters_.front());
     if (cluster->sent_probes == cluster->max_probes)
       clusters_.pop();
     if (clusters_.empty())

[philipel, 2017/01/04 10:13:18]

if (clusters_.empty()) {
  probing_state_ = ProbingState::kSuspended;
  next_probe_time_ms_ = -1;
}

[Sergey Ulanov, 2017/01/05 23:31:53]

next_probe_time_ms_ is only used in kActive state and it's reset when probing is
activated, so resetting it here would not make any difference

[philipel, 2017/01/09 09:34:34]

Acknowledged.

       probing_state_ = ProbingState::kSuspended;
   }
 }
+
+int64_t BitrateProber::GetNextProbeTime(const ProbeCluster& cluster) {
+  RTC_CHECK_GT(cluster.probe_bitrate_bps, 0);
+  RTC_CHECK_GE(cluster.time_started_ms, 0);
+  // Compute the time delta needed to send the probe at the target bitrate.
+  // Result is in milliseconds.
+  return cluster.time_started_ms +

[philipel, 2017/01/04 10:13:18]

Not super important, but can you break this calculation into maybe two steps? I
think that would be cleaner.

Also, I can't see why you need to cast to int?

[Sergey Ulanov, 2017/01/05 23:31:53]

Will do.
yes, the cast is redundant

+         static_cast<int>(
+             (1000ll * cluster.bytes_sent * 8 + cluster.probe_bitrate_bps / 2) /
+             cluster.probe_bitrate_bps);
+}
+
+
 }  // namespace webrtc