Ensure a second probe can be sent immediately

Ensure a second probe can be sent, after the first probe has been sent, even though no large
media packets have been sent.
This fixes a bug in https://webrtc-review.googlesource.com/c/src/+/294521

This cl also refactor and simplify a bit. Remove the unecessary state kSuspended.

Bug: webrtc:14928
Change-Id: Ia561441ea3d8b648b025eedd0618c82cca03b418
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296882
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39547}

modules/pacing/bitrate_prober.cc

@@ -12,11 +12,7 @@
 
 #include <algorithm>
 
-#include "absl/memory/memory.h"
-#include "api/rtc_event_log/rtc_event.h"
-#include "api/rtc_event_log/rtc_event_log.h"
 #include "api/units/data_size.h"
-#include "logging/rtc_event_log/events/rtc_event_probe_cluster_created.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
 
@@ -56,15 +52,27 @@ void BitrateProber::SetEnabled(bool enable) {
   }
 }
 
+bool BitrateProber::ReadyToSetActiveState(DataSize packet_size) const {
+  if (clusters_.empty()) {
+    RTC_DCHECK(probing_state_ == ProbingState::kDisabled ||
+               probing_state_ == ProbingState::kInactive);
+    return false;
+  }
+  switch (probing_state_) {
+    case ProbingState::kDisabled:
+    case ProbingState::kActive:
+      return false;
+    case ProbingState::kInactive:
+      // If config_.min_packet_size > 0, a "large enough" packet must be sent
+      // first, before a probe can be generated and sent. Otherwise, send the
+      // probe asap.
+      return packet_size >=
+             std::min(RecommendedMinProbeSize(), config_.min_packet_size.Get());
+  }
+}
+
 void BitrateProber::OnIncomingPacket(DataSize packet_size) {
-  // Don't initialize probing unless we have something large enough to start
-  // probing.
-  // Note that the pacer can send several packets at once when sending a probe,
-  // and thus, packets can be smaller than needed for a probe.
-  if (probing_state_ == ProbingState::kInactive && !clusters_.empty() &&
-      packet_size >=
-          std::min(RecommendedMinProbeSize(), config_.min_packet_size.Get())) {
-    // Send next probe right away.
+  if (ReadyToSetActiveState(packet_size)) {
     next_probe_time_ = Timestamp::MinusInfinity();
     probing_state_ = ProbingState::kActive;
   }
@@ -93,22 +101,12 @@ void BitrateProber::CreateProbeCluster(
   cluster.pace_info.probe_cluster_id = cluster_config.id;
   clusters_.push(cluster);
 
-  if (config_.min_packet_size.Get() > DataSize::Zero()) {
-    // If we are already probing, continue to do so. Otherwise set it to
-    // kInactive and wait for OnIncomingPacket with a large enough packet
-    // to start the probing.
-    if (probing_state_ != ProbingState::kActive) {
-      probing_state_ = ProbingState::kInactive;
-    }
-  } else {
-    // Probing is allowed to start without first sending a "large enough"
-    // packet.
-    if (probing_state_ == ProbingState::kInactive) {
-      // Send next probe right away.
-      next_probe_time_ = Timestamp::MinusInfinity();
-      probing_state_ = ProbingState::kActive;
-    }
+  if (ReadyToSetActiveState(/*packet_size=*/DataSize::Zero())) {
+    next_probe_time_ = Timestamp::MinusInfinity();
+    probing_state_ = ProbingState::kActive;
   }
+  RTC_DCHECK(probing_state_ == ProbingState::kActive ||
+             probing_state_ == ProbingState::kInactive);
 
   RTC_LOG(LS_INFO) << "Probe cluster (bitrate_bps:min bytes:min packets): ("
                    << cluster.pace_info.send_bitrate_bps << ":"
@@ -141,7 +139,7 @@ absl::optional<PacedPacketInfo> BitrateProber::CurrentCluster(Timestamp now) {
                          << "), discarding probe cluster.";
     clusters_.pop();
     if (clusters_.empty()) {
-      probing_state_ = ProbingState::kSuspended;
+      probing_state_ = ProbingState::kInactive;
       return absl::nullopt;
     }
   }
@@ -178,7 +176,7 @@ void BitrateProber::ProbeSent(Timestamp now, DataSize size) {
       clusters_.pop();
     }
     if (clusters_.empty()) {
-      probing_state_ = ProbingState::kSuspended;
+      probing_state_ = ProbingState::kInactive;
     }
   }
 }

modules/pacing/bitrate_prober.h

@@ -84,14 +84,12 @@ class BitrateProber {
   enum class ProbingState {
     // Probing will not be triggered in this state at all times.
     kDisabled,
-    // Probing is enabled and ready to trigger on the first packet arrival.
+    // Probing is enabled and ready to trigger on the first packet arrival if
+    // there is a probe cluster.
     kInactive,
     // Probe cluster is filled with the set of data rates to be probed and
     // probes are being sent.
     kActive,
-    // Probing is enabled, but currently suspended until an explicit trigger
-    // to start probing again.
-    kSuspended,
   };
 
   // A probe cluster consists of a set of probes. Each probe in turn can be
@@ -107,6 +105,7 @@ class BitrateProber {
   };
 
   Timestamp CalculateNextProbeTime(const ProbeCluster& cluster) const;
+  bool ReadyToSetActiveState(DataSize packet_size) const;
 
   ProbingState probing_state_;
 

modules/pacing/bitrate_prober_unittest.cc

@@ -12,6 +12,7 @@
 
 #include <algorithm>
 
+#include "api/transport/network_types.h"
 #include "api/units/data_rate.h"
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
@@ -368,4 +369,43 @@ TEST(BitrateProberTest, ProbeClusterTimeout) {
 
   EXPECT_FALSE(prober.is_probing());
 }
+
+TEST(BitrateProberTest, CanProbeImmediatelyIfConfigured) {
+  const test::ExplicitKeyValueConfig trials(
+      "WebRTC-Bwe-ProbingBehavior/min_packet_size:0/");
+
+  BitrateProber prober(trials);
+  prober.CreateProbeCluster({.at_time = Timestamp::Zero(),
+                             .target_data_rate = DataRate::KilobitsPerSec(300),
+                             .target_duration = TimeDelta::Millis(15),
+                             .target_probe_count = 5,
+                             .id = 0});
+  EXPECT_TRUE(prober.is_probing());
+}
+
+TEST(BitrateProberTest, CanProbeImmediatelyAgainAfterProbeIfConfigured) {
+  const test::ExplicitKeyValueConfig trials(
+      "WebRTC-Bwe-ProbingBehavior/min_packet_size:0/");
+
+  BitrateProber prober(trials);
+  ProbeClusterConfig cluster_config = {
+      .at_time = Timestamp::Zero(),
+      .target_data_rate = DataRate::KilobitsPerSec(300),
+      .target_duration = TimeDelta::Millis(15),
+      .target_probe_count = 1,
+      .id = 0};
+  prober.CreateProbeCluster(cluster_config);
+  ASSERT_TRUE(prober.is_probing());
+  (cluster_config.target_data_rate * cluster_config.target_duration).bytes();
+  prober.ProbeSent(
+      Timestamp::Zero() + TimeDelta::Millis(1),
+      cluster_config.target_data_rate * cluster_config.target_duration);
+  ASSERT_FALSE(prober.is_probing());
+
+  cluster_config.id = 2;
+  cluster_config.at_time = Timestamp::Zero() + TimeDelta::Millis(100);
+  prober.CreateProbeCluster(cluster_config);
+  EXPECT_TRUE(prober.is_probing());
+}
+
 }  // namespace webrtc