From f1d417eee50038811aaa6452de8e74f149612c78 Mon Sep 17 00:00:00 2001 From: Diep Bui Date: Tue, 17 Oct 2023 10:59:37 +0000 Subject: [PATCH] Clean up loss_based_bwe_v2_unittest and add flag MinNumObservations. MinNumObservations is set to 3 per default as loss based BWE should not be ready if it has few feedbacks. We use a flag, rather than a const since we want to customize it for our unit tests, which often have 1-2 packet feedbacks only, and customize it later in prod if necessary. Bug: webrtc:12707 Change-Id: Id1cd21aaf6137996de2e51cb5e33fc2a4bb07d8a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/323780 Reviewed-by: Per Kjellander Commit-Queue: Diep Bui Cr-Commit-Position: refs/heads/main@{#40952} --- .../goog_cc/loss_based_bwe_v2.cc | 14 +- .../goog_cc/loss_based_bwe_v2.h | 1 + .../goog_cc/loss_based_bwe_v2_test.cc | 147 +++++++----------- 3 files changed, 64 insertions(+), 98 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 f3727be679..a938e936ba 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -132,7 +132,7 @@ bool LossBasedBweV2::IsEnabled() const { bool LossBasedBweV2::IsReady() const { return IsEnabled() && IsValid(current_estimate_.loss_limited_bandwidth) && - num_observations_ > 0; + num_observations_ >= config_->min_num_observations; } bool LossBasedBweV2::ReadyToUseInStartPhase() const { @@ -151,7 +151,7 @@ LossBasedBweV2::Result LossBasedBweV2::GetLossBasedResult() const { RTC_LOG(LS_WARNING) << "The estimator must be initialized before it can be used."; } - if (num_observations_ <= 0) { + if (num_observations_ <= config_->min_num_observations) { RTC_LOG(LS_WARNING) << "The estimator must receive enough loss " "statistics before it can be used."; } @@ -392,6 +392,7 @@ absl::optional LossBasedBweV2::CreateConfig( FieldTrialParameter not_use_acked_rate_in_alr("NotUseAckedRateInAlr", true); FieldTrialParameter use_in_start_phase("UseInStartPhase", false); + FieldTrialParameter min_num_observations("MinNumObservations", 3); if (key_value_config) { ParseFieldTrial({&enabled, &bandwidth_rampup_upper_bound_factor, @@ -426,7 +427,8 @@ absl::optional LossBasedBweV2::CreateConfig( &bandwidth_cap_at_high_loss_rate, &slope_of_bwe_high_loss_func, ¬_use_acked_rate_in_alr, - &use_in_start_phase}, + &use_in_start_phase, + &min_num_observations}, key_value_config->Lookup("WebRTC-Bwe-LossBasedBweV2")); } @@ -485,6 +487,7 @@ absl::optional LossBasedBweV2::CreateConfig( config->slope_of_bwe_high_loss_func = slope_of_bwe_high_loss_func.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(); + config->min_num_observations = min_num_observations.Get(); return config; } @@ -665,6 +668,11 @@ bool LossBasedBweV2::IsConfigValid() const { << config_->high_loss_rate_threshold; valid = false; } + if (config_->min_num_observations <= 0) { + RTC_LOG(LS_WARNING) << "The min number of observations must be positive: " + << config_->min_num_observations; + valid = false; + } return valid; } 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 321c6f4b07..09d4d87d1c 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -112,6 +112,7 @@ class LossBasedBweV2 { double slope_of_bwe_high_loss_func = 1000.0; bool not_use_acked_rate_in_alr = false; bool use_in_start_phase = false; + int min_num_observations = 0; }; 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 a7437ae538..d1d5fade97 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 @@ -28,7 +28,7 @@ namespace { using ::webrtc::test::ExplicitKeyValueConfig; -constexpr TimeDelta kObservationDurationLowerBound = TimeDelta::Millis(200); +constexpr TimeDelta kObservationDurationLowerBound = TimeDelta::Millis(250); constexpr TimeDelta kDelayedIncreaseWindow = TimeDelta::Millis(300); constexpr double kMaxIncreaseFactor = 1.5; @@ -53,7 +53,6 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { } config_string << ",CandidateFactors:1.1|1.0|0.95,HigherBwBiasFactor:0.01," - "DelayBasedCandidate:true," "InherentLossLowerBound:0.001,InherentLossUpperBoundBwBalance:" "14kbps," "InherentLossUpperBoundOffset:0.9,InitialInherentLossEstimate:0.01," @@ -61,7 +60,8 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { "SendingRateSmoothingFactor:0.01," "InstantUpperBoundTemporalWeightFactor:0.97," "InstantUpperBoundBwBalance:90kbps," - "InstantUpperBoundLossOffset:0.1,TemporalWeightFactor:0.98"; + "InstantUpperBoundLossOffset:0.1,TemporalWeightFactor:0.98," + "MinNumObservations:1"; config_string.AppendFormat( ",ObservationDurationLowerBound:%dms", @@ -75,6 +75,18 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { return config_string.str(); } + std::string ShortObservationConfig(std::string custom_config) { + char buffer[1024]; + rtc::SimpleStringBuilder config_string(buffer); + + config_string << "WebRTC-Bwe-LossBasedBweV2/" + "MinNumObservations:1,ObservationWindowSize:2,"; + config_string << custom_config; + config_string << "/"; + + return config_string.str(); + } + std::vector CreatePacketResultsWithReceivedPackets( Timestamp first_packet_timestamp) { std::vector enough_feedback(2); @@ -161,13 +173,13 @@ TEST_F(LossBasedBweV2Test, DisabledWhenGivenNonValidConfigurationValues) { TEST_F(LossBasedBweV2Test, DisabledWhenGivenNonPositiveCandidateFactor) { ExplicitKeyValueConfig key_value_config_negative_candidate_factor( - "WebRTC-Bwe-LossBasedBweV2/Enabled:true,CandidateFactors:-1.3|1.1/"); + "WebRTC-Bwe-LossBasedBweV2/CandidateFactors:-1.3|1.1/"); LossBasedBweV2 loss_based_bandwidth_estimator_1( &key_value_config_negative_candidate_factor); EXPECT_FALSE(loss_based_bandwidth_estimator_1.IsEnabled()); ExplicitKeyValueConfig key_value_config_zero_candidate_factor( - "WebRTC-Bwe-LossBasedBweV2/Enabled:true,CandidateFactors:0.0|1.1/"); + "WebRTC-Bwe-LossBasedBweV2/CandidateFactors:0.0|1.1/"); LossBasedBweV2 loss_based_bandwidth_estimator_2( &key_value_config_zero_candidate_factor); EXPECT_FALSE(loss_based_bandwidth_estimator_2.IsEnabled()); @@ -177,7 +189,7 @@ TEST_F(LossBasedBweV2Test, DisabledWhenGivenConfigurationThatDoesNotAllowGeneratingCandidates) { ExplicitKeyValueConfig key_value_config( "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.0,AckedRateCandidate:false," + "CandidateFactors:1.0,AckedRateCandidate:false," "DelayBasedCandidate:false/"); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); EXPECT_FALSE(loss_based_bandwidth_estimator.IsEnabled()); @@ -607,13 +619,12 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, IncreaseByMaxIncreaseFactorAfterLossBasedBweBacksOff) { - ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.2|1|0.5,AckedRateCandidate:true," - "ObservationWindowSize:2,ObservationDurationLowerBound:200ms," + ExplicitKeyValueConfig key_value_config(ShortObservationConfig( + "CandidateFactors:1.2|1|0.5," "InstantUpperBoundBwBalance:10000kbps," - "DelayBasedCandidate:true,MaxIncreaseFactor:1.5,BwRampupUpperBoundFactor:" - "2.0,NotIncreaseIfInherentLossLessThanAverageLoss:false/"); + "MaxIncreaseFactor:1.5,NotIncreaseIfInherentLossLessThanAverageLoss:" + "false")); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::KilobitsPerSec(5000); DataRate acked_rate = DataRate::KilobitsPerSec(300); @@ -650,14 +661,11 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, LossBasedStateIsDelayBasedEstimateAfterNetworkRecovering) { - ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:100|1|0.5,AckedRateCandidate:true," - "ObservationWindowSize:2,ObservationDurationLowerBound:200ms," + ExplicitKeyValueConfig key_value_config(ShortObservationConfig( + "CandidateFactors:100|1|0.5," "InstantUpperBoundBwBalance:10000kbps," - "DelayBasedCandidate:true,MaxIncreaseFactor:100," - "BwRampupUpperBoundFactor:" - "2.0,NotIncreaseIfInherentLossLessThanAverageLoss:false/"); + "MaxIncreaseFactor:100," + "NotIncreaseIfInherentLossLessThanAverageLoss:false")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::KilobitsPerSec(600); DataRate acked_rate = DataRate::KilobitsPerSec(300); @@ -706,12 +714,10 @@ TEST_F(LossBasedBweV2Test, LossBasedStateIsNotDelayBasedEstimateIfDelayBasedEsimtateInfinite) { ExplicitKeyValueConfig key_value_config( "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:100|1|0.5,AckedRateCandidate:true," - "ObservationWindowSize:2,ObservationDurationLowerBound:200ms," + "CandidateFactors:100|1|0.5," + "ObservationWindowSize:2," "InstantUpperBoundBwBalance:10000kbps," - "DelayBasedCandidate:true,MaxIncreaseFactor:100," - "BwRampupUpperBoundFactor:" - "2.0/"); + "MaxIncreaseFactor:100/"); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::PlusInfinity(); DataRate acked_rate = DataRate::KilobitsPerSec(300); @@ -748,10 +754,9 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, IncreaseByFactorOfAckedBitrateAfterLossBasedBweBacksOff) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,LossThresholdOfHighBandwidthPreference:0.99," - "BwRampupUpperBoundFactor:1.2," - "InherentLossUpperBoundOffset:0.9,ObservationDurationLowerBound:200ms/"); + ShortObservationConfig("LossThresholdOfHighBandwidthPreference:0.99," + "BwRampupUpperBoundFactor:1.2," + "InherentLossUpperBoundOffset:0.9")); std::vector enough_feedback_1 = CreatePacketResultsWith100pLossRate( /*first_packet_timestamp=*/Timestamp::Zero()); @@ -882,15 +887,10 @@ TEST_F(LossBasedBweV2Test, KeepIncreasingEstimateAfterDelayedIncreaseWindow) { } TEST_F(LossBasedBweV2Test, NotIncreaseIfInherentLossLessThanAverageLoss) { - ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.2,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms," - "NotIncreaseIfInherentLossLessThanAverageLoss:true/"); + ExplicitKeyValueConfig key_value_config(ShortObservationConfig( + "CandidateFactors:1.2," + "NotIncreaseIfInherentLossLessThanAverageLoss:true")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); - DataRate delay_based_estimate = DataRate::KilobitsPerSec(5000); loss_based_bandwidth_estimator.SetBandwidthEstimate( DataRate::KilobitsPerSec(600)); @@ -899,7 +899,8 @@ TEST_F(LossBasedBweV2Test, NotIncreaseIfInherentLossLessThanAverageLoss) { CreatePacketResultsWith10pLossRate( /*first_packet_timestamp=*/Timestamp::Zero()); loss_based_bandwidth_estimator.UpdateBandwidthEstimate( - enough_feedback_10p_loss_1, delay_based_estimate, + enough_feedback_10p_loss_1, + /*delay_based_estimate=*/DataRate::PlusInfinity(), /*in_alr=*/false); std::vector enough_feedback_10p_loss_2 = @@ -907,7 +908,8 @@ TEST_F(LossBasedBweV2Test, NotIncreaseIfInherentLossLessThanAverageLoss) { /*first_packet_timestamp=*/Timestamp::Zero() + kObservationDurationLowerBound); loss_based_bandwidth_estimator.UpdateBandwidthEstimate( - enough_feedback_10p_loss_2, delay_based_estimate, + enough_feedback_10p_loss_2, + /*delay_based_estimate=*/DataRate::PlusInfinity(), /*in_alr=*/false); // Do not increase the bitrate because inherent loss is less than average loss @@ -918,14 +920,9 @@ TEST_F(LossBasedBweV2Test, NotIncreaseIfInherentLossLessThanAverageLoss) { TEST_F(LossBasedBweV2Test, SelectHighBandwidthCandidateIfLossRateIsLessThanThreshold) { - ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.2|0.8,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "20,NotIncreaseIfInherentLossLessThanAverageLoss:false/"); + ExplicitKeyValueConfig key_value_config(ShortObservationConfig( + "LossThresholdOfHighBandwidthPreference:0.20," + "NotIncreaseIfInherentLossLessThanAverageLoss:false")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::KilobitsPerSec(5000); @@ -959,13 +956,7 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, SelectLowBandwidthCandidateIfLossRateIsIsHigherThanThreshold) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.2|0.8,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "05/"); + ShortObservationConfig("LossThresholdOfHighBandwidthPreference:0.05")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::KilobitsPerSec(5000); @@ -999,13 +990,7 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, StricterBoundUsingHighLossRateThresholdAt10pLossRate) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.0,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "05,HighLossRateThreshold:0.09/"); + ShortObservationConfig("HighLossRateThreshold:0.09")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1041,13 +1026,7 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, StricterBoundUsingHighLossRateThresholdAt50pLossRate) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.0,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "05,HighLossRateThreshold:0.3/"); + ShortObservationConfig("HighLossRateThreshold:0.3")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1083,13 +1062,7 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, StricterBoundUsingHighLossRateThresholdAt100pLossRate) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.0,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "05,HighLossRateThreshold:0.3/"); + ShortObservationConfig("HighLossRateThreshold:0.3")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1124,13 +1097,7 @@ TEST_F(LossBasedBweV2Test, TEST_F(LossBasedBweV2Test, EstimateRecoversAfterHighLoss) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.1|1.0|0.9,AckedRateCandidate:false," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms,HigherBwBiasFactor:1000," - "HigherLogBwBiasFactor:1000,LossThresholdOfHighBandwidthPreference:0." - "05,HighLossRateThreshold:0.3/"); + ShortObservationConfig("HighLossRateThreshold:0.3")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1202,11 +1169,7 @@ TEST_F(LossBasedBweV2Test, EstimateIsNotHigherThanMaxBitrate) { TEST_F(LossBasedBweV2Test, NotBackOffToAckedRateInAlr) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.1|1.0|0.9,AckedRateCandidate:true," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms/"); + ShortObservationConfig("InstantUpperBoundBwBalance:100kbps")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1237,11 +1200,7 @@ TEST_F(LossBasedBweV2Test, NotBackOffToAckedRateInAlr) { TEST_F(LossBasedBweV2Test, BackOffToAckedRateIfNotInAlr) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,CandidateFactors:1.1|1.0|0.9,AckedRateCandidate:true," - "ObservationWindowSize:2," - "DelayBasedCandidate:true,InstantUpperBoundBwBalance:100kbps," - "ObservationDurationLowerBound:200ms/"); + ShortObservationConfig("InstantUpperBoundBwBalance:100kbps")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetMinMaxBitrate( /*min_bitrate=*/DataRate::KilobitsPerSec(10), @@ -1268,8 +1227,7 @@ TEST_F(LossBasedBweV2Test, BackOffToAckedRateIfNotInAlr) { TEST_F(LossBasedBweV2Test, NotReadyToUseInStartPhase) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,UseInStartPhase:true/"); + ShortObservationConfig("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. @@ -1278,8 +1236,7 @@ TEST_F(LossBasedBweV2Test, NotReadyToUseInStartPhase) { TEST_F(LossBasedBweV2Test, ReadyToUseInStartPhase) { ExplicitKeyValueConfig key_value_config( - "WebRTC-Bwe-LossBasedBweV2/" - "Enabled:true,ObservationDurationLowerBound:200ms,UseInStartPhase:true/"); + ShortObservationConfig("UseInStartPhase:true")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); std::vector enough_feedback = CreatePacketResultsWithReceivedPackets(