From 54199f9b4be54f5c0567d090f08041bcfc3e6c14 Mon Sep 17 00:00:00 2001 From: Per K Date: Sat, 11 Mar 2023 17:41:37 +0100 Subject: [PATCH] Ensure a second probe can be sent immediately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#39547} --- modules/pacing/bitrate_prober.cc | 56 +++++++++++------------ modules/pacing/bitrate_prober.h | 7 ++- modules/pacing/bitrate_prober_unittest.cc | 40 ++++++++++++++++ 3 files changed, 70 insertions(+), 33 deletions(-) diff --git a/modules/pacing/bitrate_prober.cc b/modules/pacing/bitrate_prober.cc index b218846c76..3151a35075 100644 --- a/modules/pacing/bitrate_prober.cc +++ b/modules/pacing/bitrate_prober.cc @@ -12,11 +12,7 @@ #include -#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 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; } } } diff --git a/modules/pacing/bitrate_prober.h b/modules/pacing/bitrate_prober.h index 4d8ec68c4f..82aba6ee3a 100644 --- a/modules/pacing/bitrate_prober.h +++ b/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_; diff --git a/modules/pacing/bitrate_prober_unittest.cc b/modules/pacing/bitrate_prober_unittest.cc index 3be7d2d99e..3c1c93f1f4 100644 --- a/modules/pacing/bitrate_prober_unittest.cc +++ b/modules/pacing/bitrate_prober_unittest.cc @@ -12,6 +12,7 @@ #include +#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