From 24923e8cfade0a362f918a12c58fe770d748b870 Mon Sep 17 00:00:00 2001 From: Jonas Olsson Date: Wed, 27 Mar 2019 14:19:04 +0100 Subject: [PATCH] Make some constants in the bitrate prober configurable. This lets us change how many bytes and packets goes into the probes, as well as some other things. Bug: webrtc:10394 Change-Id: I26bb26a644e6f00366e9275228760c8744d63735 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/128424 Commit-Queue: Jonas Olsson Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#27312} --- .../goog_cc/probe_controller.cc | 5 +-- .../goog_cc/probe_controller_unittest.cc | 2 +- modules/pacing/bitrate_prober.cc | 44 +++++++++---------- modules/pacing/bitrate_prober.h | 24 +++++++++- modules/pacing/bitrate_prober_unittest.cc | 29 ++++++++---- modules/pacing/paced_sender.cc | 2 +- 6 files changed, 65 insertions(+), 41 deletions(-) diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index dfcb7f6fd5..e5a6d1fd01 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -73,9 +73,6 @@ constexpr char kBweRapidRecoveryExperiment[] = // Never probe higher than configured by OnMaxTotalAllocatedBitrate(). constexpr char kCappedProbingFieldTrialName[] = "WebRTC-BweCappedProbing"; -constexpr char kConfigurableProbingFieldTrialName[] = - "WebRTC-Bwe-ConfigurableProbing"; - void MaybeLogProbeClusterCreated(RtcEventLog* event_log, const ProbeClusterConfig& probe) { RTC_DCHECK(event_log); @@ -104,7 +101,7 @@ ProbeControllerConfig::ProbeControllerConfig( {&first_exponential_probe_scale_, &second_exponential_probe_scale_, &further_exponential_probe_scale_, &further_probe_threshold, &alr_probing_interval_, &alr_probe_scale_}, - key_value_config->Lookup(kConfigurableProbingFieldTrialName)); + key_value_config->Lookup("WebRTC-Bwe-ProbingConfiguration")); } ProbeControllerConfig::ProbeControllerConfig(const ProbeControllerConfig&) = diff --git a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc index 04dc4407fb..7aba8e1374 100644 --- a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc +++ b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc @@ -314,7 +314,7 @@ TEST_F(ProbeControllerTest, TestAllocatedBitrateCap) { TEST_F(ProbeControllerTest, ConfigurableProbingFieldTrial) { test::ScopedFieldTrials trials( - "WebRTC-Bwe-ConfigurableProbing/" + "WebRTC-Bwe-ProbingConfiguration/" "p1:2,p2:5,step_size:3,further_probe_threshold:0.8/"); probe_controller_.reset( new ProbeController(&field_trial_config_, &mock_rtc_event_log)); diff --git a/modules/pacing/bitrate_prober.cc b/modules/pacing/bitrate_prober.cc index c81a18c3b4..52f0bc196e 100644 --- a/modules/pacing/bitrate_prober.cc +++ b/modules/pacing/bitrate_prober.cc @@ -23,20 +23,6 @@ namespace webrtc { namespace { - -// A minimum interval between probes to allow scheduling to be feasible. -constexpr int kMinProbeDeltaMs = 1; - -// The minimum number probing packets used. -constexpr int kMinProbePacketsSent = 5; - -// The minimum probing duration in ms. -constexpr int kMinProbeDurationMs = 15; - -// Maximum amount of time each probe can be delayed. Probe cluster is reset and -// retried from the start when this limit is reached. -constexpr int kMaxProbeDelayMs = 3; - // The min probe packet size is scaled with the bitrate we're probing at. // This defines the max min probe packet size, meaning that on high bitrates // we have a min probe packet size of 200 bytes. @@ -46,7 +32,16 @@ constexpr int64_t kProbeClusterTimeoutMs = 5000; } // namespace -BitrateProber::BitrateProber() : BitrateProber(nullptr) {} +BitrateProberConfig::BitrateProberConfig( + const WebRtcKeyValueConfig* key_value_config) + : min_probe_packets_sent("min_probe_packets_sent", 5), + min_probe_delta("min_probe_delta", TimeDelta::ms(1)), + min_probe_duration("min_probe_duration", TimeDelta::ms(15)), + max_probe_delay("max_probe_delay", TimeDelta::ms(3)) { + ParseFieldTrial({&min_probe_packets_sent, &min_probe_delta, + &min_probe_duration, &max_probe_delay}, + key_value_config->Lookup("WebRTC-Bwe-ProbingConfiguration")); +} BitrateProber::~BitrateProber() { RTC_HISTOGRAM_COUNTS_1000("WebRTC.BWE.Probing.TotalProbeClustersRequested", @@ -55,12 +50,12 @@ BitrateProber::~BitrateProber() { total_failed_probe_count_); } -// TODO(psla): Remove this constructor in a follow up change. -BitrateProber::BitrateProber(RtcEventLog* event_log) +BitrateProber::BitrateProber(const WebRtcKeyValueConfig& field_trials) : probing_state_(ProbingState::kDisabled), next_probe_time_ms_(-1), total_probe_count_(0), - total_failed_probe_count_(0) { + total_failed_probe_count_(0), + config_(&field_trials) { SetEnabled(true); } @@ -107,9 +102,10 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps, ProbeCluster cluster; cluster.time_created_ms = now_ms; - cluster.pace_info.probe_cluster_min_probes = kMinProbePacketsSent; - cluster.pace_info.probe_cluster_min_bytes = static_cast( - static_cast(bitrate_bps) * kMinProbeDurationMs / 8000); + cluster.pace_info.probe_cluster_min_probes = config_.min_probe_packets_sent; + cluster.pace_info.probe_cluster_min_bytes = + static_cast(static_cast(bitrate_bps) * + config_.min_probe_duration->ms() / 8000); RTC_DCHECK_GE(cluster.pace_info.probe_cluster_min_bytes, 0); cluster.pace_info.send_bitrate_bps = bitrate_bps; cluster.pace_info.probe_cluster_id = cluster_id; @@ -133,7 +129,7 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { int time_until_probe_ms = 0; if (next_probe_time_ms_ >= 0) { time_until_probe_ms = next_probe_time_ms_ - now_ms; - if (time_until_probe_ms < -kMaxProbeDelayMs) { + if (time_until_probe_ms < -config_.max_probe_delay->ms()) { RTC_DLOG(LS_WARNING) << "Probe delay too high" << " (next_ms:" << next_probe_time_ms_ << ", now_ms: " << now_ms << ")"; @@ -155,8 +151,8 @@ PacedPacketInfo BitrateProber::CurrentCluster() const { // feasible. size_t BitrateProber::RecommendedMinProbeSize() const { RTC_DCHECK(!clusters_.empty()); - return clusters_.front().pace_info.send_bitrate_bps * 2 * kMinProbeDeltaMs / - (8 * 1000); + return clusters_.front().pace_info.send_bitrate_bps * 2 * + config_.min_probe_delta->ms() / (8 * 1000); } void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { diff --git a/modules/pacing/bitrate_prober.h b/modules/pacing/bitrate_prober.h index e970eee1d1..84b2c12a49 100644 --- a/modules/pacing/bitrate_prober.h +++ b/modules/pacing/bitrate_prober.h @@ -15,17 +15,35 @@ #include #include +#include "api/transport/field_trial_based_config.h" #include "api/transport/network_types.h" +#include "rtc_base/experiments/field_trial_parser.h" namespace webrtc { class RtcEventLog; +struct BitrateProberConfig { + explicit BitrateProberConfig(const WebRtcKeyValueConfig* key_value_config); + BitrateProberConfig(const BitrateProberConfig&) = default; + BitrateProberConfig& operator=(const BitrateProberConfig&) = default; + ~BitrateProberConfig() = default; + + // The minimum number probing packets used. + FieldTrialParameter min_probe_packets_sent; + // A minimum interval between probes to allow scheduling to be feasible. + FieldTrialParameter min_probe_delta; + // The minimum probing duration. + FieldTrialParameter min_probe_duration; + // Maximum amount of time each probe can be delayed. Probe cluster is reset + // and retried from the start when this limit is reached. + FieldTrialParameter max_probe_delay; +}; + // Note that this class isn't thread-safe by itself and therefore relies // on being protected by the caller. class BitrateProber { public: - BitrateProber(); - explicit BitrateProber(RtcEventLog* event_log); + explicit BitrateProber(const WebRtcKeyValueConfig& field_trials); ~BitrateProber(); void SetEnabled(bool enable); @@ -101,6 +119,8 @@ class BitrateProber { int total_probe_count_; int total_failed_probe_count_; + + BitrateProberConfig config_; }; } // namespace webrtc diff --git a/modules/pacing/bitrate_prober_unittest.cc b/modules/pacing/bitrate_prober_unittest.cc index 1770ac51a3..380d345e6f 100644 --- a/modules/pacing/bitrate_prober_unittest.cc +++ b/modules/pacing/bitrate_prober_unittest.cc @@ -14,8 +14,10 @@ namespace webrtc { TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); EXPECT_FALSE(prober.IsProbing()); + int64_t now_ms = 0; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); @@ -71,8 +73,9 @@ TEST(BitrateProberTest, VerifyStatesAndTimeBetweenProbes) { } TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) { - BitrateProber prober; - EXPECT_FALSE(prober.IsProbing()); + const FieldTrialBasedConfig config; + BitrateProber prober(config); + int64_t now_ms = 0; EXPECT_EQ(-1, prober.TimeUntilNextProbe(now_ms)); @@ -93,7 +96,9 @@ TEST(BitrateProberTest, DoesntProbeWithoutRecentPackets) { } TEST(BitrateProberTest, DoesntInitializeProbingForSmallPackets) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); + prober.SetEnabled(true); EXPECT_FALSE(prober.IsProbing()); @@ -102,7 +107,9 @@ TEST(BitrateProberTest, DoesntInitializeProbingForSmallPackets) { } TEST(BitrateProberTest, VerifyProbeSizeOnHighBitrate) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); + constexpr unsigned kHighBitrateBps = 10000000; // 10 Mbps prober.CreateProbeCluster(kHighBitrateBps, 0, /*cluster_id=*/0); @@ -111,7 +118,8 @@ TEST(BitrateProberTest, VerifyProbeSizeOnHighBitrate) { } TEST(BitrateProberTest, MinumumNumberOfProbingPackets) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); // Even when probing at a low bitrate we expect a minimum number // of packets to be sent. constexpr int kBitrateBps = 100000; // 100 kbps @@ -128,7 +136,8 @@ TEST(BitrateProberTest, MinumumNumberOfProbingPackets) { } TEST(BitrateProberTest, ScaleBytesUsedForProbing) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); constexpr int kBitrateBps = 10000000; // 10 Mbps constexpr int kPacketSizeBytes = 1000; constexpr int kExpectedBytesSent = kBitrateBps * 15 / 8000; @@ -146,7 +155,8 @@ TEST(BitrateProberTest, ScaleBytesUsedForProbing) { } TEST(BitrateProberTest, HighBitrateProbing) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); constexpr int kBitrateBps = 1000000000; // 1 Gbps. constexpr int kPacketSizeBytes = 1000; constexpr int kExpectedBytesSent = (kBitrateBps / 8000) * 15; @@ -164,7 +174,8 @@ TEST(BitrateProberTest, HighBitrateProbing) { } TEST(BitrateProberTest, ProbeClusterTimeout) { - BitrateProber prober; + const FieldTrialBasedConfig config; + BitrateProber prober(config); constexpr int kBitrateBps = 300000; // 300 kbps constexpr int kSmallPacketSize = 20; // Expecting two probe clusters of 5 packets each. diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 83dd01fa39..e6331f20f8 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -78,7 +78,7 @@ PacedSender::PacedSender(Clock* clock, paused_(false), media_budget_(0), padding_budget_(0), - prober_(event_log), + prober_(field_trials), probing_send_failure_(false), estimated_bitrate_bps_(0), min_send_bitrate_kbps_(0u),