Fix bug in BitrateProber where an old probe added at a high bitrate will stay active indefinitely if the bandwidth estimate becomes too low to probe at that bitrate.

This is solved by timing out probe clusters after 5 seconds of being initiated.

BUG=webrtc:7043
R=terelius@webrtc.org

Review-Url: https://codereview.webrtc.org/2681733004 .
Cr-Commit-Position: refs/heads/master@{#16495}

webrtc/modules/pacing/bitrate_prober.cc

@@ -41,6 +41,8 @@ constexpr int kMaxRetryAttempts = 3;
 // we have a min probe packet size of 200 bytes.
 constexpr size_t kMinProbePacketSize = 200;
 
+constexpr int64_t kProbeClusterTimeoutMs = 5000;
+
 }  // namespace
 
 BitrateProber::BitrateProber()
@@ -78,12 +80,18 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) {
   }
 }
 
-void BitrateProber::CreateProbeCluster(int bitrate_bps) {
+void BitrateProber::CreateProbeCluster(int bitrate_bps, int64_t now_ms) {
   RTC_DCHECK(probing_state_ != ProbingState::kDisabled);
+  while (!clusters_.empty() &&
+         now_ms - clusters_.front().time_created_ms > kProbeClusterTimeoutMs) {
+    clusters_.pop();
+  }
+
   ProbeCluster cluster;
   cluster.min_probes = kMinProbePacketsSent;
   cluster.min_bytes = bitrate_bps * kMinProbeDurationMs / 8000;
   cluster.bitrate_bps = bitrate_bps;
+  cluster.time_created_ms = now_ms;
   cluster.id = next_cluster_id_++;
   clusters_.push(cluster);
 
@@ -96,7 +104,7 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps) {
     probing_state_ = ProbingState::kInactive;
 }
 
-void BitrateProber::ResetState() {
+void BitrateProber::ResetState(int64_t now_ms) {
   RTC_DCHECK(probing_state_ == ProbingState::kActive);
 
   // Recreate all probing clusters.
@@ -104,7 +112,7 @@ void BitrateProber::ResetState() {
   clusters.swap(clusters_);
   while (!clusters.empty()) {
     if (clusters.front().retries < kMaxRetryAttempts) {
-      CreateProbeCluster(clusters.front().bitrate_bps);
+      CreateProbeCluster(clusters.front().bitrate_bps, now_ms);
       clusters_.back().retries = clusters.front().retries + 1;
     }
     clusters.pop();
@@ -122,7 +130,7 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   if (next_probe_time_ms_ >= 0) {
     time_until_probe_ms = next_probe_time_ms_ - now_ms;
     if (time_until_probe_ms < -kMaxProbeDelayMs) {
-      ResetState();
+      ResetState(now_ms);
       return -1;
     }
   }

webrtc/modules/pacing/bitrate_prober.h

@@ -38,7 +38,7 @@ class BitrateProber {
 
   // Create a cluster used to probe for |bitrate_bps| with |num_probes| number
   // of probes.
-  void CreateProbeCluster(int bitrate_bps);
+  void CreateProbeCluster(int bitrate_bps, int64_t now_ms);
 
   // Returns the number of milliseconds until the next probe should be sent to
   // get accurate probing.
@@ -81,13 +81,14 @@ class BitrateProber {
 
     int sent_probes = 0;
     int sent_bytes = 0;
+    int64_t time_created_ms = -1;
     int64_t time_started_ms = -1;
 
     int retries = 0;
   };
 
   // Resets the state of the prober and clears any cluster/timing data tracked.
-  void ResetState();
+  void ResetState(int64_t now_ms);
 
   int64_t GetNextProbeTime(const ProbeCluster& cluster);
 

webrtc/modules/pacing/bitrate_prober_unittest.cc

@@ -27,8 +27,8 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) {
   const int kProbeSize = 1000;
   const int kMinProbeDurationMs = 15;
 
-  prober.CreateProbeCluster(kTestBitrate1);
-  prober.CreateProbeCluster(kTestBitrate2);
+  prober.CreateProbeCluster(kTestBitrate1, now_ms);
+  prober.CreateProbeCluster(kTestBitrate2, now_ms);
   EXPECT_FALSE(prober.IsProbing());
 
   prober.OnIncomingPacket(kProbeSize);
@@ -78,7 +78,7 @@ TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) {
   int64_t now_ms = 0;
   EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
 
-  prober.CreateProbeCluster(900000);
+  prober.CreateProbeCluster(900000, now_ms);
   EXPECT_FALSE(prober.IsProbing());
 
   prober.OnIncomingPacket(1000);
@@ -111,7 +111,7 @@ TEST(BitrateProberTest, VerifyProbeSizeOnHighBitrate) {
   BitrateProber prober;
   constexpr unsigned kHighBitrateBps = 10000000;  // 10 Mbps
 
-  prober.CreateProbeCluster(kHighBitrateBps);
+  prober.CreateProbeCluster(kHighBitrateBps, 0);
   // Probe size should ensure a minimum of 1 ms interval.
   EXPECT_GT(prober.RecommendedMinProbeSize(), kHighBitrateBps / 8000);
 }
@@ -123,7 +123,7 @@ TEST(BitrateProberTest, MinumumNumberOfProbingPackets) {
   constexpr int kBitrateBps = 100000;  // 100 kbps
   constexpr int kPacketSizeBytes = 1000;
 
-  prober.CreateProbeCluster(kBitrateBps);
+  prober.CreateProbeCluster(kBitrateBps, 0);
   prober.OnIncomingPacket(kPacketSizeBytes);
   for (int i = 0; i < 5; ++i) {
     EXPECT_TRUE(prober.IsProbing());
@@ -139,7 +139,7 @@ TEST(BitrateProberTest, ScaleBytesUsedForProbing) {
   constexpr int kPacketSizeBytes = 1000;
   constexpr int kExpectedBytesSent = kBitrateBps * 15 / 8000;
 
-  prober.CreateProbeCluster(kBitrateBps);
+  prober.CreateProbeCluster(kBitrateBps, 0);
   prober.OnIncomingPacket(kPacketSizeBytes);
   int bytes_sent = 0;
   while (bytes_sent < kExpectedBytesSent) {
@@ -151,4 +151,33 @@ TEST(BitrateProberTest, ScaleBytesUsedForProbing) {
   EXPECT_FALSE(prober.IsProbing());
 }
 
+TEST(BitrateProberTest, ProbeClusterTimeout) {
+  BitrateProber prober;
+  constexpr int kBitrateBps = 300000;  // 300 kbps
+  constexpr int kSmallPacketSize = 20;
+  // Expecting two probe clusters of 5 packets each.
+  constexpr int kExpectedBytesSent = 20 * 2 * 5;
+  constexpr int64_t kTimeoutMs = 5000;
+
+  int64_t now_ms = 0;
+  prober.CreateProbeCluster(kBitrateBps, now_ms);
+  prober.OnIncomingPacket(kSmallPacketSize);
+  EXPECT_FALSE(prober.IsProbing());
+  now_ms += kTimeoutMs;
+  prober.CreateProbeCluster(kBitrateBps / 10, now_ms);
+  prober.OnIncomingPacket(kSmallPacketSize);
+  EXPECT_FALSE(prober.IsProbing());
+  now_ms += 1;
+  prober.CreateProbeCluster(kBitrateBps / 10, now_ms);
+  prober.OnIncomingPacket(kSmallPacketSize);
+  EXPECT_TRUE(prober.IsProbing());
+  int bytes_sent = 0;
+  while (bytes_sent < kExpectedBytesSent) {
+    ASSERT_TRUE(prober.IsProbing());
+    prober.ProbeSent(0, kSmallPacketSize);
+    bytes_sent += kSmallPacketSize;
+  }
+
+  EXPECT_FALSE(prober.IsProbing());
+}
 }  // namespace webrtc

webrtc/modules/pacing/paced_sender.cc

@@ -269,7 +269,7 @@ PacedSender::~PacedSender() {}
 
 void PacedSender::CreateProbeCluster(int bitrate_bps) {
   CriticalSectionScoped cs(critsect_.get());
-  prober_->CreateProbeCluster(bitrate_bps);
+  prober_->CreateProbeCluster(bitrate_bps, clock_->TimeInMilliseconds());
 }
 
 void PacedSender::Pause() {