From dd3eae5f943d26d025de1091cc9943435c0bf4de Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Fri, 18 May 2018 07:12:15 +0000 Subject: [PATCH] Revert "Configure and use max bitrate to limit the AIMD controller estimates." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 18d7c7ea7e56444d6d7e6c8fb95b5f426fd7b953. Reason for revert: This seems to cause the auto roller to Chrome to fail on Linux and Mac on the browsertest WebRtcSimulcastBrowserTest.TestVgaReturnsTwoSimulcastStreams https://chromium-review.googlesource.com/c/chromium/src/+/1064736 Original change's description: > Configure and use max bitrate to limit the AIMD controller estimates. > > Bug: webrtc:9275 > Change-Id: I9625cd473e1cb198abe08020f5462f1bd64bf2a5 > Reviewed-on: https://webrtc-review.googlesource.com/77081 > Reviewed-by: Stefan Holmer > Reviewed-by: Sebastian Jansson > Commit-Queue: Björn Terelius > Cr-Commit-Position: refs/heads/master@{#23287} TBR=terelius@webrtc.org,stefan@webrtc.org,srte@webrtc.org Change-Id: I8ed827ab6b2f7d2b70b9889e5a88701bfb974d35 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9275 Reviewed-on: https://webrtc-review.googlesource.com/77660 Reviewed-by: Per Kjellander Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/master@{#23291} --- .../congestion_controller/delay_based_bwe.cc | 5 +- .../congestion_controller/delay_based_bwe.h | 2 +- .../goog_cc/delay_based_bwe.cc | 5 +- .../goog_cc/delay_based_bwe.h | 3 +- .../goog_cc/goog_cc_network_control.cc | 9 ++-- .../send_side_congestion_controller.cc | 8 ++- .../aimd_rate_control.cc | 53 ++++++------------- .../aimd_rate_control.h | 2 +- .../aimd_rate_control_unittest.cc | 45 ---------------- .../remote_bitrate_estimator_abs_send_time.cc | 3 +- .../remote_bitrate_estimator_single_stream.cc | 3 +- .../test/estimators/send_side.cc | 2 +- 12 files changed, 32 insertions(+), 108 deletions(-) diff --git a/modules/congestion_controller/delay_based_bwe.cc b/modules/congestion_controller/delay_based_bwe.cc index 95f235c659..b0024e769e 100644 --- a/modules/congestion_controller/delay_based_bwe.cc +++ b/modules/congestion_controller/delay_based_bwe.cc @@ -314,11 +314,10 @@ void DelayBasedBwe::SetStartBitrate(int start_bitrate_bps) { rate_control_.SetStartBitrate(start_bitrate_bps); } -void DelayBasedBwe::SetBitrateConstraints(int min_bitrate_bps, - int max_bitrate_bps) { +void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) { // Called from both the configuration thread and the network thread. Shouldn't // be called from the network thread in the future. - rate_control_.SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); + rate_control_.SetMinBitrate(min_bitrate_bps); } int64_t DelayBasedBwe::GetExpectedBwePeriodMs() const { diff --git a/modules/congestion_controller/delay_based_bwe.h b/modules/congestion_controller/delay_based_bwe.h index caa4661ce6..dbe759ebf5 100644 --- a/modules/congestion_controller/delay_based_bwe.h +++ b/modules/congestion_controller/delay_based_bwe.h @@ -52,7 +52,7 @@ class DelayBasedBwe { bool LatestEstimate(std::vector* ssrcs, uint32_t* bitrate_bps) const; void SetStartBitrate(int start_bitrate_bps); - void SetBitrateConstraints(int min_bitrate_bps, int max_bitrate_bps); + void SetMinBitrate(int min_bitrate_bps); int64_t GetExpectedBwePeriodMs() const; private: diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index f96b843a09..fea5a66e5a 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -318,11 +318,10 @@ void DelayBasedBwe::SetStartBitrate(int start_bitrate_bps) { rate_control_.SetStartBitrate(start_bitrate_bps); } -void DelayBasedBwe::SetBitrateConstraints(int min_bitrate_bps, - int max_bitrate_bps) { +void DelayBasedBwe::SetMinBitrate(int min_bitrate_bps) { // Called from both the configuration thread and the network thread. Shouldn't // be called from the network thread in the future. - rate_control_.SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); + rate_control_.SetMinBitrate(min_bitrate_bps); } int64_t DelayBasedBwe::GetExpectedBwePeriodMs() const { diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.h b/modules/congestion_controller/goog_cc/delay_based_bwe.h index 91f233eb35..485f687c39 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h @@ -53,9 +53,8 @@ class DelayBasedBwe { void OnRttUpdate(int64_t avg_rtt_ms); bool LatestEstimate(std::vector* ssrcs, uint32_t* bitrate_bps) const; - void SetStartBitrate(int start_bitrate_bps); - void SetBitrateConstraints(int min_bitrate_bps, int max_bitrate_bps); + void SetMinBitrate(int min_bitrate_bps); int64_t GetExpectedBwePeriodMs() const; private: diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc index 5f5760ef31..b0bf6f31f0 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -129,9 +129,7 @@ GoogCcNetworkController::GoogCcNetworkController(RtcEventLog* event_log, max_total_allocated_bitrate_(DataRate::Zero()), in_cwnd_experiment_(CwndExperimentEnabled()), accepted_queue_ms_(kDefaultAcceptedQueueMs) { - constexpr int kMaxBitrateUnchanged = -1; - delay_based_bwe_->SetBitrateConstraints( - congestion_controller::GetMinBitrateBps(), kMaxBitrateUnchanged); + delay_based_bwe_->SetMinBitrate(congestion_controller::GetMinBitrateBps()); UpdateBitrateConstraints(config.constraints, config.starting_bandwidth); OnStreamsConfig(config.stream_based_config); if (in_cwnd_experiment_ && @@ -165,7 +163,7 @@ NetworkControlUpdate GoogCcNetworkController::OnNetworkRouteChange( delay_based_bwe_.reset(new DelayBasedBwe(event_log_)); acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator()); delay_based_bwe_->SetStartBitrate(start_bitrate_bps); - delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); + delay_based_bwe_->SetMinBitrate(min_bitrate_bps); probe_controller_->Reset(msg.at_time.ms()); probe_controller_->SetBitrates(min_bitrate_bps, start_bitrate_bps, @@ -264,10 +262,9 @@ void GoogCcNetworkController::UpdateBitrateConstraints( bandwidth_estimation_->SetBitrates(start_bitrate_bps, min_bitrate_bps, max_bitrate_bps); - if (start_bitrate_bps > 0) delay_based_bwe_->SetStartBitrate(start_bitrate_bps); - delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); + delay_based_bwe_->SetMinBitrate(min_bitrate_bps); } NetworkControlUpdate GoogCcNetworkController::OnTransportLossReport( diff --git a/modules/congestion_controller/send_side_congestion_controller.cc b/modules/congestion_controller/send_side_congestion_controller.cc index a6fd0916b3..4a1793311f 100644 --- a/modules/congestion_controller/send_side_congestion_controller.cc +++ b/modules/congestion_controller/send_side_congestion_controller.cc @@ -137,9 +137,7 @@ SendSideCongestionController::SendSideCongestionController( webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")), transport_overhead_bytes_per_packet_(0), pacer_pushback_experiment_(IsPacerPushbackExperimentEnabled()) { - constexpr int kMaxBitrateUnchanged = -1; - delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps_, - kMaxBitrateUnchanged); + delay_based_bwe_->SetMinBitrate(min_bitrate_bps_); if (in_cwnd_experiment_ && !ReadCwndExperimentParameter(&accepted_queue_ms_)) { RTC_LOG(LS_WARNING) << "Failed to parse parameters for CwndExperiment " @@ -188,7 +186,7 @@ void SendSideCongestionController::SetBweBitrates(int min_bitrate_bps, if (start_bitrate_bps > 0) delay_based_bwe_->SetStartBitrate(start_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; - delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); + delay_based_bwe_->SetMinBitrate(min_bitrate_bps_); } MaybeTriggerOnNetworkChanged(); } @@ -222,8 +220,8 @@ void SendSideCongestionController::OnNetworkRouteChanged( min_bitrate_bps_ = min_bitrate_bps; delay_based_bwe_.reset(new DelayBasedBwe(event_log_, clock_)); acknowledged_bitrate_estimator_.reset(new AcknowledgedBitrateEstimator()); - delay_based_bwe_->SetBitrateConstraints(min_bitrate_bps, max_bitrate_bps); delay_based_bwe_->SetStartBitrate(bitrate_bps); + delay_based_bwe_->SetMinBitrate(min_bitrate_bps); } probe_controller_->Reset(); diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc index 0b8a2ec7b5..c494f50d80 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -28,14 +28,9 @@ namespace webrtc { -namespace { -constexpr int64_t kDefaultRttMs = 200; -constexpr int64_t kMinFeedbackIntervalMs = 200; -constexpr int64_t kMaxFeedbackIntervalMs = 1000; -constexpr float kDefaultBackoffFactor = 0.85f; -constexpr int kDefaultMaxBandwidthBps = 30000000; -constexpr int kDefaultStartBandwidthBps = 300000; -} // namespace +static const int64_t kDefaultRttMs = 200; +static const int64_t kMaxFeedbackIntervalMs = 1000; +static const float kDefaultBackoffFactor = 0.85f; const char kBweBackOffFactorExperiment[] = "WebRTC-BweBackOffFactor"; @@ -61,8 +56,8 @@ float ReadTrendlineFilterWindowSize() { AimdRateControl::AimdRateControl() : min_configured_bitrate_bps_(congestion_controller::GetMinBitrateBps()), - max_configured_bitrate_bps_(kDefaultMaxBandwidthBps), - current_bitrate_bps_(kDefaultStartBandwidthBps), + max_configured_bitrate_bps_(30000000), + current_bitrate_bps_(max_configured_bitrate_bps_), avg_max_bitrate_kbps_(-1.0f), var_max_bitrate_kbps_(0.4f), rate_control_state_(kRcHold), @@ -83,30 +78,13 @@ AimdRateControl::AimdRateControl() AimdRateControl::~AimdRateControl() {} void AimdRateControl::SetStartBitrate(int start_bitrate_bps) { - // TODO(terelius): It would make sense to not change the estimate if we have - // already initialized a bitrate. However, some code recreates and resets - // parameters multiple times and those tests break when changing this. - if (start_bitrate_bps > 0) { - // TODO(terelius): I think this should clamp the bitrate to the configured - // max and min, but that would be a brekaing change because SetStartBitrate - // and SetBitrateConstraints are separate calls that can occur in either - // order. Consider merging the calls in the future. - current_bitrate_bps_ = start_bitrate_bps; - bitrate_is_initialized_ = true; - } + current_bitrate_bps_ = start_bitrate_bps; + bitrate_is_initialized_ = true; } -void AimdRateControl::SetBitrateConstraints(int min_bitrate_bps, - int max_bitrate_bps) { - if (min_bitrate_bps > 0) - min_configured_bitrate_bps_ = min_bitrate_bps; - if (max_bitrate_bps > 0) { - max_configured_bitrate_bps_ = - rtc::SafeMax(min_configured_bitrate_bps_, max_bitrate_bps); - } - current_bitrate_bps_ = rtc::SafeClamp(current_bitrate_bps_, - min_configured_bitrate_bps_, - max_configured_bitrate_bps_); +void AimdRateControl::SetMinBitrate(int min_bitrate_bps) { + min_configured_bitrate_bps_ = min_bitrate_bps; + current_bitrate_bps_ = std::max(min_bitrate_bps, current_bitrate_bps_); } bool AimdRateControl::ValidEstimate() const { @@ -119,6 +97,7 @@ int64_t AimdRateControl::GetFeedbackInterval() const { static const int kRtcpSize = 80; const int64_t interval = static_cast( kRtcpSize * 8.0 * 1000.0 / (0.05 * current_bitrate_bps_) + 0.5); + const int64_t kMinFeedbackIntervalMs = 200; return rtc::SafeClamp(interval, kMinFeedbackIntervalMs, kMaxFeedbackIntervalMs); } @@ -198,9 +177,10 @@ int AimdRateControl::GetExpectedBandwidthPeriodMs() const { if (!last_decrease_) return smoothing_experiment_ ? kMinPeriodMs : kDefaultPeriodMs; - return rtc::SafeClamp( - 1000 * static_cast(*last_decrease_) / increase_rate, - kMinPeriodMs, kMaxPeriodMs); + return std::min(kMaxPeriodMs, + std::max(1000 * static_cast(*last_decrease_) / + increase_rate, + kMinPeriodMs)); } uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps, @@ -300,8 +280,7 @@ uint32_t AimdRateControl::ClampBitrate(uint32_t new_bitrate_bps, // We allow a bit more lag at very low rates to not too easily get stuck if // the encoder produces uneven outputs. const uint32_t max_bitrate_bps = - rtc::SafeMin(static_cast(1.5f * incoming_bitrate_bps) + 10000, - max_configured_bitrate_bps_); + static_cast(1.5f * incoming_bitrate_bps) + 10000; if (new_bitrate_bps > current_bitrate_bps_ && new_bitrate_bps > max_bitrate_bps) { new_bitrate_bps = std::max(current_bitrate_bps_, max_bitrate_bps); diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.h b/modules/remote_bitrate_estimator/aimd_rate_control.h index 19fdbe48b5..a9ecb67c34 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -30,7 +30,7 @@ class AimdRateControl { // otherwise. bool ValidEstimate() const; void SetStartBitrate(int start_bitrate_bps); - void SetBitrateConstraints(int min_bitrate_bps, int max_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 incoming_bitrate is less than half of the current diff --git a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc index c56ed48bc4..f6844b0e94 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc @@ -105,50 +105,6 @@ TEST(AimdRateControlTest, GetIncreaseRateAndBandwidthPeriodSmoothingExp) { states.aimd_rate_control->GetExpectedBandwidthPeriodMs()); } -TEST(AimdRateControlTest, BweLimitedByConfiguredMin) { - auto states = CreateAimdRateControlStates(); - constexpr int kMinBitrate = 50000; - constexpr int kMaxBitrate = 200000; - constexpr int kStartBitrate = kMinBitrate + 1; - states.aimd_rate_control->SetBitrateConstraints(kMinBitrate, kMaxBitrate); - states.aimd_rate_control->SetStartBitrate(kStartBitrate); - states.aimd_rate_control->SetEstimate( - kStartBitrate, states.simulated_clock->TimeInMilliseconds()); - while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime < - 20000) { - // Don't decrease below minimum even if the actual throughput is lower. - constexpr int kAckedBitrate = kMinBitrate - 1000; - UpdateRateControl(states, BandwidthUsage::kBwOverusing, kAckedBitrate, - states.simulated_clock->TimeInMilliseconds()); - states.simulated_clock->AdvanceTimeMilliseconds(100); - } - ASSERT_TRUE(states.aimd_rate_control->ValidEstimate()); - EXPECT_EQ(static_cast(kMinBitrate), - states.aimd_rate_control->LatestEstimate()); -} - -TEST(AimdRateControlTest, BweLimitedByConfiguredMax) { - auto states = CreateAimdRateControlStates(); - constexpr int kMinBitrate = 50000; - constexpr int kMaxBitrate = 200000; - constexpr int kStartBitrate = kMaxBitrate - 1; - states.aimd_rate_control->SetBitrateConstraints(kMinBitrate, kMaxBitrate); - states.aimd_rate_control->SetStartBitrate(kStartBitrate); - states.aimd_rate_control->SetEstimate( - kStartBitrate, states.simulated_clock->TimeInMilliseconds()); - while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime < - 20000) { - // Don't increase above maximum even if the actual throughput is higher. - constexpr int kAckedBitrate = kMaxBitrate + 10000; - UpdateRateControl(states, BandwidthUsage::kBwNormal, kAckedBitrate, - states.simulated_clock->TimeInMilliseconds()); - states.simulated_clock->AdvanceTimeMilliseconds(100); - } - ASSERT_TRUE(states.aimd_rate_control->ValidEstimate()); - EXPECT_EQ(static_cast(kMaxBitrate), - states.aimd_rate_control->LatestEstimate()); -} - TEST(AimdRateControlTest, BweLimitedByAckedBitrate) { auto states = CreateAimdRateControlStates(); constexpr int kAckedBitrate = 10000; @@ -266,7 +222,6 @@ TEST(AimdRateControlTest, BandwidthPeriodIsNotAboveMaxSmoothingExp) { test::ScopedFieldTrials override_field_trials(kSmoothingExpFieldTrial); auto states = CreateAimdRateControlStates(); constexpr int kInitialBitrate = 50000000; - states.aimd_rate_control->SetBitrateConstraints(10000, 60000000); states.aimd_rate_control->SetEstimate( kInitialBitrate, states.simulated_clock->TimeInMilliseconds()); states.simulated_clock->AdvanceTimeMilliseconds(100); diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index edcefe53bf..fdc1523700 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -412,7 +412,6 @@ void RemoteBitrateEstimatorAbsSendTime::SetMinBitrate(int min_bitrate_bps) { // Called from both the configuration thread and the network thread. Shouldn't // be called from the network thread in the future. rtc::CritScope lock(&crit_); - constexpr int kMaxBitrateUnchanged = -1; - remote_rate_.SetBitrateConstraints(min_bitrate_bps, kMaxBitrateUnchanged); + remote_rate_.SetMinBitrate(min_bitrate_bps); } } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index f59494e978..a914a84673 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -250,8 +250,7 @@ AimdRateControl* RemoteBitrateEstimatorSingleStream::GetRemoteRate() { void RemoteBitrateEstimatorSingleStream::SetMinBitrate(int min_bitrate_bps) { rtc::CritScope cs(&crit_sect_); - constexpr int kMaxBitrateUnchanged = -1; - remote_rate_->SetBitrateConstraints(min_bitrate_bps, kMaxBitrateUnchanged); + remote_rate_->SetMinBitrate(min_bitrate_bps); } } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/test/estimators/send_side.cc b/modules/remote_bitrate_estimator/test/estimators/send_side.cc index 4bafb678f2..6e03eee942 100644 --- a/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -44,7 +44,7 @@ SendSideBweSender::SendSideBweSender(int kbps, bitrate_controller_->SetStartBitrate(1000 * kbps); bitrate_controller_->SetMinMaxBitrate(1000 * kMinBitrateKbps, 1000 * kMaxBitrateKbps); - bwe_->SetBitrateConstraints(1000 * kMinBitrateKbps, 1000 * kMaxBitrateKbps); + bwe_->SetMinBitrate(1000 * kMinBitrateKbps); } SendSideBweSender::~SendSideBweSender() {}