From d97af7b1b9fd05509cec0e788aad283360c5babc Mon Sep 17 00:00:00 2001 From: Diep Bui Date: Fri, 13 May 2022 08:08:07 +0000 Subject: [PATCH] Clean loss based bwe 2. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is for cleaning loss based bwe v2 implementation according to some comments from https://webrtc-review.googlesource.com/c/src/+/261240. Bug: webrtc:12707 Change-Id: I2cb278f136cddcd0eeb2c5e4c319a9cc6aab94a0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262251 Commit-Queue: Diep Bui Reviewed-by: Björn Terelius Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#36875} --- .../goog_cc/loss_based_bwe_v2.cc | 32 +++++++------------ .../goog_cc/loss_based_bwe_v2.h | 3 +- .../goog_cc/loss_based_bwe_v2_test.cc | 23 +++++++++++++ 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc index 0d19e567e8..9005956dff 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -149,13 +149,13 @@ DataRate LossBasedBweV2::GetBandwidthEstimate( "statistics before it can be used."; } } - return DataRate::PlusInfinity(); + return IsValid(delay_based_limit) ? delay_based_limit + : DataRate::PlusInfinity(); } if (delay_based_limit.IsFinite()) { return std::min({current_estimate_.loss_limited_bandwidth, - GetInstantUpperBound(), - delay_based_limit * config_->delay_based_limit_factor}); + GetInstantUpperBound(), delay_based_limit}); } else { return std::min(current_estimate_.loss_limited_bandwidth, GetInstantUpperBound()); @@ -221,7 +221,7 @@ void LossBasedBweV2::UpdateBandwidthEstimate( last_time_estimate_reduced_ = last_send_time_most_recent_observation_; } - // Bound the estimate increasement if: + // Bound the estimate increase if: // 1. The estimate is limited due to loss, and // 2. The estimate has been increased for less than `delayed_increase_window` // ago, and @@ -299,9 +299,8 @@ absl::optional LossBasedBweV2::CreateConfig( "BwBackoffLowerBoundFactor", 1.0); FieldTrialParameter trendline_integration_enabled( "TrendlineIntegrationEnabled", false); - FieldTrialParameter delay_based_limit_factor("DelayBasedLimitFactor", - 1.0); - FieldTrialParameter trendline_window_size("TrendlineWindowSize", 20); + FieldTrialParameter trendline_observations_window_size( + "TrendlineObservationsWindowSize", 20); FieldTrialParameter max_increase_factor("MaxIncreaseFactor", 1000.0); FieldTrialParameter delayed_increase_window( "DelayedIncreaseWindow", TimeDelta::Millis(300)); @@ -333,8 +332,7 @@ absl::optional LossBasedBweV2::CreateConfig( &temporal_weight_factor, &bandwidth_backoff_lower_bound_factor, &trendline_integration_enabled, - &delay_based_limit_factor, - &trendline_window_size, + &trendline_observations_window_size, &max_increase_factor, &delayed_increase_window, &use_acked_bitrate_only_when_overusing}, @@ -381,8 +379,8 @@ absl::optional LossBasedBweV2::CreateConfig( config->bandwidth_backoff_lower_bound_factor = bandwidth_backoff_lower_bound_factor.Get(); config->trendline_integration_enabled = trendline_integration_enabled.Get(); - config->delay_based_limit_factor = delay_based_limit_factor.Get(); - config->trendline_window_size = trendline_window_size.Get(); + config->trendline_observations_window_size = + trendline_observations_window_size.Get(); config->max_increase_factor = max_increase_factor.Get(); config->delayed_increase_window = delayed_increase_window.Get(); config->use_acked_bitrate_only_when_overusing = @@ -536,15 +534,9 @@ bool LossBasedBweV2::IsConfigValid() const { << config_->bandwidth_backoff_lower_bound_factor; valid = false; } - if (config_->delay_based_limit_factor < 1.0) { - RTC_LOG(LS_WARNING) - << "The delay based limit factor must not be less than 1: " - << config_->delay_based_limit_factor; - valid = false; - } - if (config_->trendline_window_size < 2) { + if (config_->trendline_observations_window_size < 2) { RTC_LOG(LS_WARNING) << "The trendline window size must be at least 2: " - << config_->trendline_window_size; + << config_->trendline_observations_window_size; valid = false; } if (config_->max_increase_factor <= 0.0) { @@ -861,7 +853,7 @@ bool LossBasedBweV2::PushBackObservation( BandwidthUsage delay_detector_state) { delay_detector_states_.push_front(delay_detector_state); if (static_cast(delay_detector_states_.size()) > - config_->trendline_window_size) { + config_->trendline_observations_window_size) { delay_detector_states_.pop_back(); } diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h index 9db711250b..aee9ebd809 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -85,8 +85,7 @@ class LossBasedBweV2 { double temporal_weight_factor = 0.0; double bandwidth_backoff_lower_bound_factor = 0.0; bool trendline_integration_enabled = false; - double delay_based_limit_factor = 1.0; - int trendline_window_size = 0; + int trendline_observations_window_size = 0; double max_increase_factor = 0.0; TimeDelta delayed_increase_window = TimeDelta::Zero(); bool use_acked_bitrate_only_when_overusing = false; diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc b/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc index 8e002b4bd6..8e09533966 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc @@ -179,6 +179,29 @@ TEST_P(LossBasedBweV2Test, EXPECT_FALSE(loss_based_bandwidth_estimator.IsEnabled()); } +TEST_P(LossBasedBweV2Test, ReturnsDelayBasedEstimateWhenDisabled) { + ExplicitKeyValueConfig key_value_config( + Config(/*enabled=*/false, /*valid=*/true, + /*trendline_integration_enabled=*/GetParam())); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + + EXPECT_EQ(loss_based_bandwidth_estimator.GetBandwidthEstimate( + /*delay_based_limit=*/DataRate::KilobitsPerSec(100)), + DataRate::KilobitsPerSec(100)); +} + +TEST_P(LossBasedBweV2Test, + ReturnsDelayBasedEstimateWhenWhenGivenNonValidConfigurationValues) { + ExplicitKeyValueConfig key_value_config( + Config(/*enabled=*/true, /*valid=*/false, + /*trendline_integration_enabled=*/GetParam())); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + + EXPECT_EQ(loss_based_bandwidth_estimator.GetBandwidthEstimate( + /*delay_based_limit=*/DataRate::KilobitsPerSec(100)), + DataRate::KilobitsPerSec(100)); +} + TEST_P(LossBasedBweV2Test, BandwidthEstimateGivenInitializationAndThenFeedback) { std::vector enough_feedback =