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 <perkj@webrtc.org> Reviewed-by: Ying Wang <yinwa@webrtc.org> Commit-Queue: Fanny Linderborg <linderborg@webrtc.org> Cr-Commit-Position: refs/heads/main@{#35239}
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 91fd719..4404114 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 @@ << 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 ff8e828..0533488 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 { +using ::webrtc::test::ExplicitKeyValueConfig; + constexpr TimeDelta kObservationDurationLowerBound = TimeDelta::Millis(200); std::string Config(bool enabled, bool valid) { @@ -65,7 +67,7 @@ } 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, 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, 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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);