Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1688703002 .

Cr-Commit-Position: refs/heads/master@{#11634}

webrtc/modules/pacing/bitrate_prober.cc

@@ -52,7 +52,13 @@ bool BitrateProber::IsProbing() const {
   return probing_state_ == kProbing;
 }
 
-void BitrateProber::MaybeInitializeProbe(int bitrate_bps) {
+void BitrateProber::OnIncomingPacket(int bitrate_bps,
+                                     size_t packet_size,
+                                     int64_t now_ms) {
+  // Don't initialize probing unless we have something large enough to start
+  // probing.
+  if (packet_size < PacedSender::kMinProbePacketSize)
+    return;
   if (probing_state_ != kAllowedToProbe)
     return;
   probe_bitrates_.clear();
@@ -74,6 +80,9 @@ void BitrateProber::MaybeInitializeProbe(int bitrate_bps) {
   }
   bitrate_log << ", num packets: " << probe_bitrates_.size();
   LOG(LS_INFO) << bitrate_log.str().c_str();
+  // Set last send time to current time so TimeUntilNextProbe doesn't short
+  // circuit due to inactivity.
+  time_last_send_ms_ = now_ms;
   probing_state_ = kProbing;
 }
 
@@ -81,16 +90,23 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) {
   if (probing_state_ != kDisabled && probe_bitrates_.empty()) {
     probing_state_ = kWait;
   }
-  if (probe_bitrates_.empty()) {
-    // No probe started, or waiting for next probe.
+  if (probe_bitrates_.empty() || time_last_send_ms_ == -1) {
+    // No probe started, probe finished, or too long since last probe packet.
     return -1;
   }
   int64_t elapsed_time_ms = now_ms - time_last_send_ms_;
+  // If no packets have been sent for n milliseconds, temporarily deactivate to
+  // not keep spinning.
+  static const int kInactiveSendDeltaMs = 5000;
+  if (elapsed_time_ms > kInactiveSendDeltaMs) {
+    time_last_send_ms_ = -1;
+    probing_state_ = kAllowedToProbe;
+    return -1;
+  }
   // We will send the first probe packet immediately if no packet has been
   // sent before.
   int time_until_probe_ms = 0;
-  if (packet_size_last_send_ > PacedSender::kMinProbePacketSize &&
-      probing_state_ == kProbing) {
+  if (packet_size_last_send_ != 0 && probing_state_ == kProbing) {
     int next_delta_ms = ComputeDeltaFromBitrate(packet_size_last_send_,
                                                 probe_bitrates_.front());
     time_until_probe_ms = next_delta_ms - elapsed_time_ms;
@@ -119,6 +135,8 @@ size_t BitrateProber::RecommendedPacketSize() const {
 
 void BitrateProber::PacketSent(int64_t now_ms, size_t packet_size) {
   assert(packet_size > 0);
+  if (packet_size < PacedSender::kMinProbePacketSize)
+    return;
   packet_size_last_send_ = packet_size;
   time_last_send_ms_ = now_ms;
   if (probing_state_ != kProbing)

webrtc/modules/pacing/bitrate_prober.h

@@ -31,8 +31,10 @@ class BitrateProber {
   // TimeUntilNextProbe().
   bool IsProbing() const;
 
-  // Initializes a new probing session if the prober is allowed to probe.
-  void MaybeInitializeProbe(int bitrate_bps);
+  // Initializes a new probing session if the prober is allowed to probe. Does
+  // not initialize the prober unless the packet size is large enough to probe
+  // with.
+  void OnIncomingPacket(int bitrate_bps, size_t packet_size, int64_t now_ms);
 
   // Returns the number of milliseconds until the next packet should be sent to
   // get accurate probing.

webrtc/modules/pacing/bitrate_prober_unittest.cc

@@ -24,9 +24,10 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) {
   prober.SetEnabled(true);
   EXPECT_FALSE(prober.IsProbing());
 
-  prober.MaybeInitializeProbe(300000);
+  prober.OnIncomingPacket(300000, 1000, now_ms);
   EXPECT_TRUE(prober.IsProbing());
 
+  // First packet should probe as soon as possible.
   EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
   prober.PacketSent(now_ms, 1000);
 
@@ -48,4 +49,43 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) {
   EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
   EXPECT_FALSE(prober.IsProbing());
 }
+
+TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) {
+  BitrateProber prober;
+  EXPECT_FALSE(prober.IsProbing());
+  int64_t now_ms = 0;
+  EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
+
+  prober.SetEnabled(true);
+  EXPECT_FALSE(prober.IsProbing());
+
+  prober.OnIncomingPacket(300000, 1000, now_ms);
+  EXPECT_TRUE(prober.IsProbing());
+  EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
+  // Let time pass, no large enough packets put into prober.
+  now_ms += 6000;
+  EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
+  // Insert a small packet, not a candidate for probing.
+  prober.OnIncomingPacket(300000, 100, now_ms);
+  prober.PacketSent(now_ms, 100);
+  EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms));
+  // Insert a large-enough packet after downtime while probing should reset to
+  // perform a new probe since the requested one didn't finish.
+  prober.OnIncomingPacket(300000, 1000, now_ms);
+  EXPECT_EQ(0, prober.TimeUntilNextProbe(now_ms));
+  prober.PacketSent(now_ms, 1000);
+  // Next packet should be part of new probe and be sent with non-zero delay.
+  prober.OnIncomingPacket(300000, 1000, now_ms);
+  EXPECT_GT(prober.TimeUntilNextProbe(now_ms), 0);
+}
+
+TEST(BitrateProberTest, DoesntInitializeProbingForSmallPackets) {
+  BitrateProber prober;
+  prober.SetEnabled(true);
+  EXPECT_FALSE(prober.IsProbing());
+
+  prober.OnIncomingPacket(300000, 100, 0);
+  EXPECT_FALSE(prober.IsProbing());
+}
+
 }  // namespace webrtc

webrtc/modules/pacing/paced_sender.cc

@@ -323,9 +323,9 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority,
 
   if (probing_enabled_ && !prober_->IsProbing())
     prober_->SetEnabled(true);
-  prober_->MaybeInitializeProbe(bitrate_bps_);
-
   int64_t now_ms = clock_->TimeInMilliseconds();
+  prober_->OnIncomingPacket(bitrate_bps_, bytes, now_ms);
+
   if (capture_time_ms < 0)
     capture_time_ms = now_ms;