Fix BitrateProber to match the requested bitrate more precisely

Previously BirateProber was calculating delay between probes based on
the size of the previous probe. Because of that the actual sent bitrate
can deviate greatly from the target value. With this change it uses
total number of bytes in the cluster to estimate delay before each
probe.

BUG=webrtc:6952

Review-Url: https://codereview.webrtc.org/2613543003
Cr-Original-Commit-Position: refs/heads/master@{#15971}
Committed: 599c5011e7
Review-Url: https://codereview.webrtc.org/2613543003
Cr-Commit-Position: refs/heads/master@{#16127}

webrtc/modules/pacing/bitrate_prober.cc

@@ -20,9 +20,6 @@ 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;
 
@@ -32,18 +29,18 @@ constexpr int kMinProbePacketsSent = 5;
 // The minimum probing duration in ms.
 constexpr int kMinProbeDurationMs = 15;
 
-int ComputeDeltaFromBitrate(size_t probe_size, uint32_t bitrate_bps) {
+// Maximum amount of time each probe can be delayed. Probe cluster is reset and
-  RTC_CHECK_GT(bitrate_bps, 0);
+// retried from the start when this limit is reached.
-  // Compute the time delta needed to send probe_size bytes at bitrate_bps
+constexpr int kMaxProbeDelayMs = 3;
-  // bps. Result is in milliseconds.
+
-  return static_cast<int>((1000ll * probe_size * 8) / bitrate_bps);
+// Number of times probing is retried before the cluster is dropped.
-}
+constexpr int kMaxRetryAttempts = 3;
+
 }  // namespace
 
 BitrateProber::BitrateProber()
     : probing_state_(ProbingState::kDisabled),
-      probe_size_last_sent_(0),
+      next_probe_time_ms_(-1),
-      time_last_probe_sent_ms_(-1),
       next_cluster_id_(0) {
   SetEnabled(true);
 }
@@ -70,6 +67,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;
   }
 }
@@ -93,18 +92,19 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps) {
 }
 
 void BitrateProber::ResetState() {
-  time_last_probe_sent_ms_ = -1;
+  RTC_DCHECK(probing_state_ == ProbingState::kActive);
-  probe_size_last_sent_ = 0;
 
   // Recreate all probing clusters.
   std::queue<ProbeCluster> clusters;
   clusters.swap(clusters_);
   while (!clusters.empty()) {
+    if (clusters.front().retries < kMaxRetryAttempts) {
       CreateProbeCluster(clusters.front().bitrate_bps);
+      clusters_.back().retries = clusters.front().retries + 1;
+    }
     clusters.pop();
   }
-  // If its enabled, reset to inactive.
+
-  if (probing_state_ != ProbingState::kDisabled)
   probing_state_ = ProbingState::kInactive;
 }
 
@@ -112,36 +112,16 @@ 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;
+  int time_until_probe_ms = 0;
-  if (time_last_probe_sent_ms_ == -1) {
+  if (next_probe_time_ms_ >= 0) {
-    elapsed_time_ms = 0;
+    time_until_probe_ms = next_probe_time_ms_ - now_ms;
-  } else {
+    if (time_until_probe_ms < -kMaxProbeDelayMs) {
-    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) {
       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().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.
-    const int kMaxProbeDelayMs = 3;
-    if (next_delta_ms < kMinProbeDeltaMs ||
-        time_until_probe_ms < -kMaxProbeDelayMs) {
-      probing_state_ = ProbingState::kSuspended;
-      LOG(LS_INFO) << "Delta too small or missed probing accurately, suspend";
-      time_until_probe_ms = 0;
-    }
   }
+
   return std::max(time_until_probe_ms, 0);
 }
 
@@ -162,12 +142,16 @@ size_t BitrateProber::RecommendedMinProbeSize() const {
 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_bytes += static_cast<int>(bytes);
     cluster->sent_probes += 1;
+    next_probe_time_ms_ = GetNextProbeTime(clusters_.front());
     if (cluster->sent_bytes >= cluster->min_bytes &&
         cluster->sent_probes >= cluster->min_probes) {
       clusters_.pop();
@@ -176,4 +160,17 @@ void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) {
       probing_state_ = ProbingState::kSuspended;
   }
 }
+
+int64_t BitrateProber::GetNextProbeTime(const ProbeCluster& cluster) {
+  RTC_CHECK_GT(cluster.bitrate_bps, 0);
+  RTC_CHECK_GE(cluster.time_started_ms, 0);
+
+  // Compute the time delta from the cluster start to ensure probe bitrate stays
+  // close to the target bitrate. Result is in milliseconds.
+  int64_t delta_ms = (8000ll * cluster.sent_bytes + cluster.bitrate_bps / 2) /
+                  cluster.bitrate_bps;
+  return cluster.time_started_ms + delta_ms;
+}
+
+
 }  // namespace webrtc

webrtc/modules/pacing/bitrate_prober.h

@@ -72,29 +72,38 @@ class BitrateProber {
   };
 
   // A probe cluster consists of a set of probes. Each probe in turn can be
-  // divided into a number of packets to accomodate the MTU on the network.
+  // divided into a number of packets to accommodate the MTU on the network.
   struct ProbeCluster {
     int min_probes = 0;
-    int sent_probes = 0;
     int min_bytes = 0;
-    int sent_bytes = 0;
     int bitrate_bps = 0;
     int id = -1;
+
+    int sent_probes = 0;
+    int sent_bytes = 0;
+    int64_t time_started_ms = -1;
+
+    int retries = 0;
   };
 
   // Resets the state of the prober and clears any cluster/timing data tracked.
   void ResetState();
 
+  int64_t GetNextProbeTime(const ProbeCluster& cluster);
+
   ProbingState probing_state_;
+
   // Probe bitrate per packet. These are used to compute the delta relative to
   // the previous probe packet based on the size and time when that packet was
   // sent.
   std::queue<ProbeCluster> clusters_;
-  // A probe can include one or more packets.
+
-  size_t probe_size_last_sent_;
+  // Time the next probe should be sent when in kActive state.
-  // The last time a probe was sent.
+  int64_t next_probe_time_ms_;
-  int64_t time_last_probe_sent_ms_;
+
   int next_cluster_id_;
 };
+
 }  // namespace webrtc
+
 #endif  // WEBRTC_MODULES_PACING_BITRATE_PROBER_H_

webrtc/modules/pacing/bitrate_prober_unittest.cc

@@ -21,35 +21,53 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) {
   int64_t now_ms = 0;
   EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
 
-  prober.CreateProbeCluster(900000);
+  const int kTestBitrate1 = 900000;
-  prober.CreateProbeCluster(1800000);
+  const int kTestBitrate2 = 1800000;
+  const int kClusterSize = 5;
+  const int kProbeSize = 1000;
+  const int kMinProbeDurationMs = 15;
+
+  prober.CreateProbeCluster(kTestBitrate1);
+  prober.CreateProbeCluster(kTestBitrate2);
   EXPECT_FALSE(prober.IsProbing());
 
-  prober.OnIncomingPacket(1000);
+  prober.OnIncomingPacket(kProbeSize);
   EXPECT_TRUE(prober.IsProbing());
   EXPECT_EQ(0, prober.CurrentClusterId());
 
   // First packet should probe as soon as possible.
   EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
-  prober.ProbeSent(now_ms, 1000);
 
-  for (int i = 0; i < 4; ++i) {
+  for (int i = 0; i < kClusterSize; ++i) {
-    EXPECT_EQ(8, prober.TimeUntilNextProbe(now_ms));
+    now_ms += prober.TimeUntilNextProbe(now_ms);
-    now_ms += 4;
-    EXPECT_EQ(4, prober.TimeUntilNextProbe(now_ms));
-    now_ms += 4;
     EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
     EXPECT_EQ(0, prober.CurrentClusterId());
-    prober.ProbeSent(now_ms, 1000);
+    prober.ProbeSent(now_ms, kProbeSize);
   }
-  for (int i = 0; i < 5; ++i) {
+
-    EXPECT_EQ(4, prober.TimeUntilNextProbe(now_ms));
+  EXPECT_GE(now_ms, kMinProbeDurationMs);
-    now_ms += 4;
+  // Verify that the actual bitrate is withing 10% of the target.
+  double bitrate = kProbeSize * (kClusterSize - 1) * 8 * 1000.0 / now_ms;
+  EXPECT_GT(bitrate, kTestBitrate1 * 0.9);
+  EXPECT_LT(bitrate,  kTestBitrate1 * 1.1);
+
+  now_ms += prober.TimeUntilNextProbe(now_ms);
+  int64_t probe2_started = now_ms;
+
+  for (int i = 0; i < kClusterSize; ++i) {
+    now_ms += prober.TimeUntilNextProbe(now_ms);
     EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
     EXPECT_EQ(1, prober.CurrentClusterId());
-    prober.ProbeSent(now_ms, 1000);
+    prober.ProbeSent(now_ms, kProbeSize);
   }
 
+  // Verify that the actual bitrate is withing 10% of the target.
+  int duration = now_ms - probe2_started;
+  EXPECT_GE(duration, kMinProbeDurationMs);
+  bitrate = kProbeSize * (kClusterSize - 1) * 8 * 1000.0 / duration;
+  EXPECT_GT(bitrate, kTestBitrate2 * 0.9);
+  EXPECT_LT(bitrate,  kTestBitrate2 * 1.1);
+
   EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
   EXPECT_FALSE(prober.IsProbing());
 }

webrtc/modules/pacing/paced_sender_unittest.cc

@@ -828,6 +828,7 @@ TEST_F(PacedSenderTest, ProbingWithInsertedPackets) {
               kFirstClusterBps, kBitrateProbingError);
   EXPECT_EQ(0, packet_sender.padding_sent());
 
+  clock_.AdvanceTimeMilliseconds(send_bucket_->TimeUntilNextProcess());
   start = clock_.TimeInMilliseconds();
   while (packet_sender.packets_sent() < 10) {
     int time_until_process = send_bucket_->TimeUntilNextProcess();
@@ -836,8 +837,8 @@ TEST_F(PacedSenderTest, ProbingWithInsertedPackets) {
   }
   packets_sent = packet_sender.packets_sent() - packets_sent;
   // Validate second cluster bitrate.
-  EXPECT_NEAR(
+  EXPECT_NEAR((packets_sent - 1) * kPacketSize * 8000 /
-      packets_sent * kPacketSize * 8000 / (clock_.TimeInMilliseconds() - start),
+                  (clock_.TimeInMilliseconds() - start),
               kSecondClusterBps, kBitrateProbingError);
 }