From 29d4a013bc90abdd78cffbe0a671d81a08010539 Mon Sep 17 00:00:00 2001 From: Diep Bui Date: Mon, 25 Sep 2023 07:44:33 +0000 Subject: [PATCH] Reland: use loss based bwe v2 in the start phase. Original CL: https://webrtc-review.googlesource.com/c/src/+/320840 Bug: webrtc:12707 Change-Id: Iff3a0c76c26aeb7cb0ac24c1f7aab3529c4a1659 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321420 Commit-Queue: Diep Bui Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#40799} --- .../goog_cc/loss_based_bwe_v2.cc | 18 ++++++++++--- .../goog_cc/loss_based_bwe_v2.h | 8 +++++- .../goog_cc/loss_based_bwe_v2_test.cc | 27 +++++++++++++++++++ .../goog_cc/send_side_bandwidth_estimation.cc | 6 ++--- 4 files changed, 50 insertions(+), 9 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 3ad910bdb1..b58f0f7520 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -136,6 +136,10 @@ bool LossBasedBweV2::IsReady() const { num_observations_ > 0; } +bool LossBasedBweV2::ReadyToUseInStartPhase() const { + return IsReady() && config_->use_in_start_phase; +} + LossBasedBweV2::Result LossBasedBweV2::GetLossBasedResult() const { Result result; result.state = current_state_; @@ -238,9 +242,12 @@ void LossBasedBweV2::UpdateBandwidthEstimate( SetProbeBitrate(probe_bitrate); if (!IsValid(current_estimate_.loss_limited_bandwidth)) { - RTC_LOG(LS_VERBOSE) - << "The estimator must be initialized before it can be used."; - return; + if (!IsValid(delay_based_estimate)) { + RTC_LOG(LS_WARNING) << "The delay based estimate must be finite: " + << ToString(delay_based_estimate); + return; + } + current_estimate_.loss_limited_bandwidth = delay_based_estimate; } ChannelParameters best_candidate = current_estimate_; @@ -416,6 +423,7 @@ absl::optional LossBasedBweV2::CreateConfig( TimeDelta::Seconds(10)); FieldTrialParameter not_use_acked_rate_in_alr("NotUseAckedRateInAlr", false); + FieldTrialParameter use_in_start_phase("UseInStartPhase", false); if (key_value_config) { ParseFieldTrial({&enabled, &bandwidth_rampup_upper_bound_factor, @@ -453,7 +461,8 @@ absl::optional LossBasedBweV2::CreateConfig( &high_loss_rate_threshold, &bandwidth_cap_at_high_loss_rate, &slope_of_bwe_high_loss_func, - ¬_use_acked_rate_in_alr}, + ¬_use_acked_rate_in_alr, + &use_in_start_phase}, key_value_config->Lookup("WebRTC-Bwe-LossBasedBweV2")); } @@ -516,6 +525,7 @@ absl::optional LossBasedBweV2::CreateConfig( config->probe_integration_enabled = probe_integration_enabled.Get(); config->probe_expiration = probe_expiration.Get(); config->not_use_acked_rate_in_alr = not_use_acked_rate_in_alr.Get(); + config->use_in_start_phase = use_in_start_phase.Get(); return config; } 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 649e7652bb..cd49d05c97 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -57,11 +57,13 @@ class LossBasedBweV2 { // initialized with a BWE and then has received enough `PacketResult`s. bool IsReady() const; + // Returns true if loss based BWE is ready to be used in the start phase. + bool ReadyToUseInStartPhase() const; + // Returns `DataRate::PlusInfinity` if no BWE can be calculated. Result GetLossBasedResult() const; void SetAcknowledgedBitrate(DataRate acknowledged_bitrate); - void SetBandwidthEstimate(DataRate bandwidth_estimate); void SetMinMaxBitrate(DataRate min_bitrate, DataRate max_bitrate); void UpdateBandwidthEstimate( rtc::ArrayView packet_results, @@ -70,6 +72,9 @@ class LossBasedBweV2 { absl::optional probe_bitrate, bool in_alr); + // For unit testing only. + void SetBandwidthEstimate(DataRate bandwidth_estimate); + private: struct ChannelParameters { double inherent_loss = 0.0; @@ -114,6 +119,7 @@ class LossBasedBweV2 { bool probe_integration_enabled = false; TimeDelta probe_expiration = TimeDelta::Zero(); bool not_use_acked_rate_in_alr = false; + bool use_in_start_phase = false; }; struct Derivatives { 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 bdd7498392..780bd79a51 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 @@ -1530,6 +1530,33 @@ TEST_P(LossBasedBweV2Test, BackOffToAckedRateIfNotInAlr) { acked_rate); } +TEST_P(LossBasedBweV2Test, NotReadyToUseInStartPhase) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "Enabled:true,UseInStartPhase:true/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + // Make sure that the estimator is not ready to use in start phase because of + // lacking TWCC feedback. + EXPECT_FALSE(loss_based_bandwidth_estimator.ReadyToUseInStartPhase()); +} + +TEST_P(LossBasedBweV2Test, + ReadyToUseInStartPhase) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "Enabled:true,ObservationDurationLowerBound:200ms,UseInStartPhase:true/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + std::vector enough_feedback = + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero()); + + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback, /*delay_based_estimate=*/DataRate::KilobitsPerSec(600), + BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, /*in_alr=*/false); + EXPECT_TRUE(loss_based_bandwidth_estimator.ReadyToUseInStartPhase()); +} + INSTANTIATE_TEST_SUITE_P(LossBasedBweV2Tests, LossBasedBweV2Test, ::testing::Bool()); diff --git a/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc b/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc index 9f97c065c9..31024662ff 100644 --- a/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc +++ b/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc @@ -492,7 +492,8 @@ void SendSideBandwidthEstimation::UpdateEstimate(Timestamp at_time) { // We trust the REMB and/or delay-based estimate during the first 2 seconds if // we haven't had any packet loss reported, to allow startup bitrate probing. - if (last_fraction_loss_ == 0 && IsInStartPhase(at_time)) { + if (last_fraction_loss_ == 0 && IsInStartPhase(at_time) && + !loss_based_bandwidth_estimator_v2_.ReadyToUseInStartPhase()) { DataRate new_bitrate = current_target_; // TODO(srte): We should not allow the new_bitrate to be larger than the // receiver limit here. @@ -503,9 +504,6 @@ void SendSideBandwidthEstimation::UpdateEstimate(Timestamp at_time) { if (LossBasedBandwidthEstimatorV1Enabled()) { loss_based_bandwidth_estimator_v1_.Initialize(new_bitrate); } - if (LossBasedBandwidthEstimatorV2Enabled()) { - loss_based_bandwidth_estimator_v2_.SetBandwidthEstimate(new_bitrate); - } if (new_bitrate != current_target_) { min_bitrate_history_.clear();