From 89b750f6d41ebc19d9dfa37e16cc86ae68074d99 Mon Sep 17 00:00:00 2001 From: Fanny Linderborg Date: Fri, 15 Oct 2021 15:59:25 +0000 Subject: [PATCH] Ensure `LossBasedBweV2` can generate candidates if it is enabled To be able to generate candidates the configuration must support at least one of the following: * enable acknowledged bitrate as a candidate, * enable delay based bwe as a candidate, or * enabled a candidate factor other than 1.0. If none of the above is supported then the configuration will be marked as invalid and thus the `LossBasedBweV2` will be disabled. Bug: webrtc:12707 Change-Id: I836ee59a396497f577b34fe0650ffc79f6cadc31 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235210 Reviewed-by: Per Kjellander Reviewed-by: Ying Wang Commit-Queue: Fanny Linderborg Cr-Commit-Position: refs/heads/main@{#35239} --- .../goog_cc/loss_based_bwe_v2.cc | 22 ++++++++++ .../goog_cc/loss_based_bwe_v2_test.cc | 44 +++++++++++++++---- 2 files changed, 57 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 91fd719319..44041143bf 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -356,6 +356,28 @@ bool LossBasedBweV2::IsConfigValid() const { << config_->rampup_acceleration_maxout_time.seconds(); valid = false; } + for (double candidate_factor : config_->candidate_factors) { + if (candidate_factor <= 0.0) { + RTC_LOG(LS_WARNING) << "All candidate factors must be greater than zero: " + << candidate_factor; + valid = false; + } + } + + // Ensure that the configuration allows generation of at least one candidate + // other than the current estimate. + if (!config_->append_acknowledged_rate_candidate && + !config_->append_delay_based_estimate_candidate && + !absl::c_any_of(config_->candidate_factors, + [](double cf) { return cf != 1.0; })) { + RTC_LOG(LS_WARNING) + << "The configuration does not allow generating candidates. Specify " + "a candidate factor other than 1.0, allow the acknowledged rate " + "to be a candidate, and/or allow the delay based estimate to be a " + "candidate."; + valid = false; + } + if (config_->higher_bandwidth_bias_factor < 0.0) { RTC_LOG(LS_WARNING) << "The higher bandwidth bias factor must be non-negative: " 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 ff8e828588..05334885f2 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 @@ -25,6 +25,8 @@ namespace webrtc { namespace { +using ::webrtc::test::ExplicitKeyValueConfig; + constexpr TimeDelta kObservationDurationLowerBound = TimeDelta::Millis(200); std::string Config(bool enabled, bool valid) { @@ -65,7 +67,7 @@ std::string Config(bool enabled, bool valid) { } TEST(LossBasedBweV2Test, EnabledWhenGivenValidConfigurationValues) { - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -73,7 +75,7 @@ TEST(LossBasedBweV2Test, EnabledWhenGivenValidConfigurationValues) { } TEST(LossBasedBweV2Test, DisabledWhenGivenDisabledConfiguration) { - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/false, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -81,13 +83,37 @@ TEST(LossBasedBweV2Test, DisabledWhenGivenDisabledConfiguration) { } TEST(LossBasedBweV2Test, DisabledWhenGivenNonValidConfigurationValues) { - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/false)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); EXPECT_FALSE(loss_based_bandwidth_estimator.IsEnabled()); } +TEST(LossBasedBweV2Test, DisabledWhenGivenNonPositiveCandidateFactor) { + ExplicitKeyValueConfig key_value_config_negative_candidate_factor( + "WebRTC-Bwe-LossBasedBweV2/Enabled:true,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/"); + LossBasedBweV2 loss_based_bandwidth_estimator_2( + &key_value_config_zero_candidate_factor); + EXPECT_FALSE(loss_based_bandwidth_estimator_2.IsEnabled()); +} + +TEST(LossBasedBweV2Test, + DisabledWhenGivenConfigurationThatDoesNotAllowGeneratingCandidates) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "Enabled:true,CandidateFactors:1.0,AckedRateCandidate:false," + "DelayBasedCandidate:false/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + EXPECT_FALSE(loss_based_bandwidth_estimator.IsEnabled()); +} + TEST(LossBasedBweV2Test, BandwidthEstimateGivenInitializationAndThenFeedback) { PacketResult enough_feedback[2]; enough_feedback[0].sent_packet.size = DataSize::Bytes(15'000); @@ -100,7 +126,7 @@ TEST(LossBasedBweV2Test, BandwidthEstimateGivenInitializationAndThenFeedback) { enough_feedback[1].receive_time = Timestamp::Zero() + 2 * kObservationDurationLowerBound; - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -125,7 +151,7 @@ TEST(LossBasedBweV2Test, NoBandwidthEstimateGivenNoInitialization) { enough_feedback[1].receive_time = Timestamp::Zero() + 2 * kObservationDurationLowerBound; - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -151,7 +177,7 @@ TEST(LossBasedBweV2Test, NoBandwidthEstimateGivenNotEnoughFeedback) { not_enough_feedback[1].receive_time = Timestamp::Zero() + kObservationDurationLowerBound; - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -194,7 +220,7 @@ TEST(LossBasedBweV2Test, enough_feedback_2[1].receive_time = Timestamp::Zero() + 4 * kObservationDurationLowerBound; - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); @@ -243,7 +269,7 @@ TEST(LossBasedBweV2Test, enough_feedback_2[1].receive_time = Timestamp::Zero() + 4 * kObservationDurationLowerBound; - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator_1(&key_value_config); LossBasedBweV2 loss_based_bandwidth_estimator_2(&key_value_config); @@ -291,7 +317,7 @@ TEST(LossBasedBweV2Test, enough_feedback_no_received_packets[1].receive_time = Timestamp::PlusInfinity(); - test::ExplicitKeyValueConfig key_value_config( + ExplicitKeyValueConfig key_value_config( Config(/*enabled=*/true, /*valid=*/true)); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config);