diff --git a/modules/congestion_controller/goog_cc/BUILD.gn b/modules/congestion_controller/goog_cc/BUILD.gn index e802f3bd2d..173b658cb5 100644 --- a/modules/congestion_controller/goog_cc/BUILD.gn +++ b/modules/congestion_controller/goog_cc/BUILD.gn @@ -212,6 +212,7 @@ if (rtc_include_tests) { "../../pacing", "../../remote_bitrate_estimator", "../../rtp_rtcp:rtp_rtcp_format", + "//system_wrappers:field_trial_api", "//testing/gmock", "//third_party/abseil-cpp/absl/memory", ] diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index bc39b41de2..b801cc8e62 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -239,11 +239,9 @@ DelayBasedBwe::Result DelayBasedBwe::MaybeUpdateEstimate( result.updated = UpdateEstimate(now_ms, acked_bitrate_bps, &result.target_bitrate_bps); } else if (!acked_bitrate_bps && rate_control_.ValidEstimate() && - rate_control_.TimeToReduceFurther( - now_ms, rate_control_.LatestEstimate() / 2 - 1)) { - // Overusing before we have a measured acknowledged bitrate. We check - // TimeToReduceFurther (with a fake acknowledged bitrate) to avoid - // reducing too often. + rate_control_.InitialTimeToReduceFurther(now_ms)) { + // Overusing before we have a measured acknowledged bitrate. Reduce send + // rate by 50% every 200 ms. // TODO(tschumim): Improve this and/or the acknowledged bitrate estimator // so that we (almost) always have a bitrate estimate. rate_control_.SetEstimate(rate_control_.LatestEstimate() / 2, now_ms); diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc index bcc3ee4b50..b2add3e6e9 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc @@ -13,6 +13,7 @@ #include "modules/pacing/paced_sender.h" #include "rtc_base/constructormagic.h" #include "system_wrappers/include/clock.h" +#include "system_wrappers/include/field_trial.h" #include "test/field_trial.h" #include "test/gtest.h" @@ -234,4 +235,58 @@ TEST_F(DelayBasedBweTest, TestInitialOveruse) { EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000); } +class DelayBasedBweTestWithBackoffTimeoutExperiment : public DelayBasedBweTest { + public: + DelayBasedBweTestWithBackoffTimeoutExperiment() + : DelayBasedBweTest("WebRTC-BweInitialBackOffInterval/Enabled-200/") {} +}; + +// This test subsumes and improves DelayBasedBweTest.TestInitialOveruse above. +TEST_F(DelayBasedBweTestWithBackoffTimeoutExperiment, TestInitialOveruse) { + const uint32_t kStartBitrate = 300e3; + const uint32_t kInitialCapacityBps = 200e3; + const uint32_t kDummySsrc = 0; + // High FPS to ensure that we send a lot of packets in a short time. + const int kFps = 90; + + stream_generator_->AddStream(new test::RtpStream(kFps, kStartBitrate)); + stream_generator_->set_capacity_bps(kInitialCapacityBps); + + // Needed to initialize the AimdRateControl. + bitrate_estimator_->SetStartBitrate(kStartBitrate); + + // Produce 30 frames (in 1/3 second) and give them to the estimator. + uint32_t bitrate_bps = kStartBitrate; + bool seen_overuse = false; + for (int frames = 0; frames < 30 && !seen_overuse; ++frames) { + bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps); + // The purpose of this test is to ensure that we back down even if we don't + // have any acknowledged bitrate estimate yet. Hence, if the test works + // as expected, we should not have a measured bitrate yet. + EXPECT_FALSE(acknowledged_bitrate_estimator_->bitrate_bps().has_value()); + if (overuse) { + EXPECT_TRUE(bitrate_observer_.updated()); + EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate / 2, 15000); + bitrate_bps = bitrate_observer_.latest_bitrate(); + seen_overuse = true; + } else if (bitrate_observer_.updated()) { + bitrate_bps = bitrate_observer_.latest_bitrate(); + bitrate_observer_.Reset(); + } + } + EXPECT_TRUE(seen_overuse); + // Continue generating an additional 15 frames (equivalent to 167 ms) and + // verify that we don't back down further. + for (int frames = 0; frames < 15 && seen_overuse; ++frames) { + bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps); + EXPECT_FALSE(overuse); + if (bitrate_observer_.updated()) { + bitrate_bps = bitrate_observer_.latest_bitrate(); + EXPECT_GE(bitrate_bps, kStartBitrate / 2 - 15000); + EXPECT_LE(bitrate_bps, kInitialCapacityBps + 15000); + bitrate_observer_.Reset(); + } + } +} + } // namespace webrtc diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc index b564c1a468..ff9392c2f6 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc @@ -148,7 +148,20 @@ int64_t StreamGenerator::GenerateFrame(std::vector* packets, } // namespace test DelayBasedBweTest::DelayBasedBweTest() - : clock_(100000000), + : field_trial(), + clock_(100000000), + acknowledged_bitrate_estimator_( + absl::make_unique()), + bitrate_estimator_(new DelayBasedBwe(nullptr)), + stream_generator_(new test::StreamGenerator(1e6, // Capacity. + clock_.TimeInMicroseconds())), + arrival_time_offset_ms_(0), + first_update_(true) {} + +DelayBasedBweTest::DelayBasedBweTest(const std::string& field_trial_string) + : field_trial( + absl::make_unique(field_trial_string)), + clock_(100000000), acknowledged_bitrate_estimator_( absl::make_unique()), bitrate_estimator_(new DelayBasedBwe(nullptr)), diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h index 3699945ae3..b3996999b9 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -22,6 +23,7 @@ #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/constructormagic.h" #include "system_wrappers/include/clock.h" +#include "test/field_trial.h" #include "test/gtest.h" namespace webrtc { @@ -114,6 +116,7 @@ class StreamGenerator { class DelayBasedBweTest : public ::testing::Test { public: DelayBasedBweTest(); + explicit DelayBasedBweTest(const std::string& field_trial_string); virtual ~DelayBasedBweTest(); protected: @@ -163,6 +166,8 @@ class DelayBasedBweTest : public ::testing::Test { static const uint32_t kDefaultSsrc; + std::unique_ptr + field_trial; // Must be initialized first. SimulatedClock clock_; // Time at the receiver. test::TestBitrateObserver bitrate_observer_; std::unique_ptr acknowledged_bitrate_estimator_; diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc index 6fbe450880..8db2a6102a 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -10,20 +10,21 @@ #include "modules/remote_bitrate_estimator/aimd_rate_control.h" +#include + #include #include #include #include #include -#include "rtc_base/checks.h" -#include "rtc_base/numerics/safe_minmax.h" - #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/remote_bitrate_estimator/overuse_detector.h" #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h" +#include "rtc_base/checks.h" #include "rtc_base/logging.h" +#include "rtc_base/numerics/safe_minmax.h" #include "system_wrappers/include/field_trial.h" namespace webrtc { @@ -31,8 +32,11 @@ namespace webrtc { static const int64_t kDefaultRttMs = 200; static const int64_t kMaxFeedbackIntervalMs = 1000; static const float kDefaultBackoffFactor = 0.85f; +static const int64_t kDefaultInitialBackOffIntervalMs = 200; const char kBweBackOffFactorExperiment[] = "WebRTC-BweBackOffFactor"; +const char kBweInitialBackOffIntervalExperiment[] = + "WebRTC-BweInitialBackOffInterval"; float ReadBackoffFactor() { std::string experiment_string = @@ -54,6 +58,25 @@ float ReadBackoffFactor() { return kDefaultBackoffFactor; } +int64_t ReadInitialBackoffIntervalMs() { + std::string experiment_string = + webrtc::field_trial::FindFullName(kBweInitialBackOffIntervalExperiment); + int64_t backoff_interval; + int parsed_values = + sscanf(experiment_string.c_str(), "Enabled-%" SCNd64, &backoff_interval); + if (parsed_values == 1) { + if (10 <= backoff_interval && backoff_interval <= 200) { + return backoff_interval; + } + RTC_LOG(WARNING) + << "Initial back-off interval must be between 10 and 200 ms."; + } + RTC_LOG(LS_WARNING) << "Failed to parse parameters for " + << kBweInitialBackOffIntervalExperiment + << " experiment. Using default."; + return kDefaultInitialBackOffIntervalMs; +} + AimdRateControl::AimdRateControl() : min_configured_bitrate_bps_(congestion_controller::GetMinBitrateBps()), max_configured_bitrate_bps_(30000000), @@ -64,6 +87,7 @@ AimdRateControl::AimdRateControl() rate_control_state_(kRcHold), rate_control_region_(kRcMaxUnknown), time_last_bitrate_change_(-1), + time_last_bitrate_decrease_(-1), time_first_throughput_estimate_(-1), bitrate_is_initialized_(false), beta_(webrtc::field_trial::IsEnabled(kBweBackOffFactorExperiment) @@ -72,7 +96,15 @@ AimdRateControl::AimdRateControl() rtt_(kDefaultRttMs), in_experiment_(!AdaptiveThresholdExperimentIsDisabled()), smoothing_experiment_( - webrtc::field_trial::IsEnabled("WebRTC-Audio-BandwidthSmoothing")) { + webrtc::field_trial::IsEnabled("WebRTC-Audio-BandwidthSmoothing")), + in_initial_backoff_interval_experiment_( + webrtc::field_trial::IsEnabled(kBweInitialBackOffIntervalExperiment)), + initial_backoff_interval_ms_(kDefaultInitialBackOffIntervalMs) { + if (in_initial_backoff_interval_experiment_) { + initial_backoff_interval_ms_ = ReadInitialBackoffIntervalMs(); + RTC_LOG(LS_INFO) << "Using aimd rate control with initial back-off interval" + << " " << initial_backoff_interval_ms_ << " ms."; + } RTC_LOG(LS_INFO) << "Using aimd rate control with back off factor " << beta_; } @@ -105,11 +137,11 @@ int64_t AimdRateControl::GetFeedbackInterval() const { } bool AimdRateControl::TimeToReduceFurther( - int64_t time_now, + int64_t now_ms, uint32_t estimated_throughput_bps) const { const int64_t bitrate_reduction_interval = std::max(std::min(rtt_, 200), 10); - if (time_now - time_last_bitrate_change_ >= bitrate_reduction_interval) { + if (now_ms - time_last_bitrate_change_ >= bitrate_reduction_interval) { return true; } if (ValidEstimate()) { @@ -121,6 +153,20 @@ bool AimdRateControl::TimeToReduceFurther( return false; } +bool AimdRateControl::InitialTimeToReduceFurther(int64_t now_ms) const { + if (!in_initial_backoff_interval_experiment_) { + return ValidEstimate() && + TimeToReduceFurther(now_ms, LatestEstimate() / 2 - 1); + } + // TODO(terelius): We could use the RTT (clamped to suitable limits) instead + // of a fixed bitrate_reduction_interval. + if (time_last_bitrate_decrease_ == -1 || + now_ms - time_last_bitrate_decrease_ >= initial_backoff_interval_ms_) { + return true; + } + return false; +} + uint32_t AimdRateControl::LatestEstimate() const { return current_bitrate_bps_; } @@ -156,8 +202,12 @@ uint32_t AimdRateControl::Update(const RateControlInput* input, void AimdRateControl::SetEstimate(int bitrate_bps, int64_t now_ms) { bitrate_is_initialized_ = true; + uint32_t prev_bitrate_bps = current_bitrate_bps_; current_bitrate_bps_ = ClampBitrate(bitrate_bps, bitrate_bps); time_last_bitrate_change_ = now_ms; + if (current_bitrate_bps_ < prev_bitrate_bps) { + time_last_bitrate_decrease_ = now_ms; + } } int AimdRateControl::GetNearMaxIncreaseRateBps() const { @@ -273,6 +323,7 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps, // Stay on hold until the pipes are cleared. rate_control_state_ = kRcHold; time_last_bitrate_change_ = now_ms; + time_last_bitrate_decrease_ = now_ms; break; default: diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.h b/modules/remote_bitrate_estimator/aimd_rate_control.h index 0ff36bbffa..8ec4357694 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -33,12 +33,16 @@ class AimdRateControl { void SetStartBitrate(int start_bitrate_bps); void SetMinBitrate(int min_bitrate_bps); int64_t GetFeedbackInterval() const; + // Returns true if the bitrate estimate hasn't been changed for more than // an RTT, or if the estimated_throughput is less than half of the current // estimate. Should be used to decide if we should reduce the rate further // when over-using. - bool TimeToReduceFurther(int64_t time_now, + bool TimeToReduceFurther(int64_t now_ms, uint32_t estimated_throughput_bps) const; + // As above. To be used if overusing before we have measured a throughput. + bool InitialTimeToReduceFurther(int64_t now_ms) const; + uint32_t LatestEstimate() const; void SetRtt(int64_t rtt); uint32_t Update(const RateControlInput* input, int64_t now_ms); @@ -83,12 +87,15 @@ class AimdRateControl { RateControlState rate_control_state_; RateControlRegion rate_control_region_; int64_t time_last_bitrate_change_; + int64_t time_last_bitrate_decrease_; int64_t time_first_throughput_estimate_; bool bitrate_is_initialized_; float beta_; int64_t rtt_; - bool in_experiment_; - bool smoothing_experiment_; + const bool in_experiment_; + const bool smoothing_experiment_; + const bool in_initial_backoff_interval_experiment_; + int64_t initial_backoff_interval_ms_; absl::optional last_decrease_; }; } // namespace webrtc