From af7b785f025e6ad514e21f2658046d12f82037a1 Mon Sep 17 00:00:00 2001 From: Per K Date: Fri, 20 Oct 2023 11:10:29 +0200 Subject: [PATCH] Ensure LossBased BWE do not decrease due to acked bitrate Ensure acked bitrate is not used for lower loss based estimate if estimate improve. Ensure LossBasedBweV2 is in state DelayBased if reached max rate. Bug: webrtc:12707 Change-Id: I20230b99e0c2b530570e2f2de8ea88179f795c50 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/324140 Reviewed-by: Diep Bui Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#40977} --- .../goog_cc/loss_based_bwe_v2.cc | 18 +-- .../goog_cc/loss_based_bwe_v2_test.cc | 105 ++++++++++++++---- 2 files changed, 95 insertions(+), 28 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 c0d7d1011b..da94baaa8b 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -282,12 +282,10 @@ void LossBasedBweV2::UpdateBandwidthEstimate( // Bound the best candidate by the acked bitrate. if (increasing_when_loss_limited && IsValid(acknowledged_bitrate_)) { best_candidate.loss_limited_bandwidth = - IsValid(best_candidate.loss_limited_bandwidth) - ? std::min(best_candidate.loss_limited_bandwidth, - config_->bandwidth_rampup_upper_bound_factor * - (*acknowledged_bitrate_)) - : config_->bandwidth_rampup_upper_bound_factor * - (*acknowledged_bitrate_); + std::max(current_best_estimate_.loss_limited_bandwidth, + std::min(best_candidate.loss_limited_bandwidth, + config_->bandwidth_rampup_upper_bound_factor * + (*acknowledged_bitrate_))); } } @@ -323,11 +321,13 @@ void LossBasedBweV2::UpdateResult() { if (IsEstimateIncreasingWhenLossLimited( /*old_estimate=*/loss_based_result_.bandwidth_estimate, /*new_estimate=*/bounded_bandwidth_estimate) && - bounded_bandwidth_estimate < delay_based_estimate_) { + bounded_bandwidth_estimate < delay_based_estimate_ && + bounded_bandwidth_estimate < max_bitrate_) { loss_based_result_.state = LossBasedState::kIncreasing; - } else if (bounded_bandwidth_estimate < delay_based_estimate_) { + } else if (bounded_bandwidth_estimate < delay_based_estimate_ && + bounded_bandwidth_estimate < max_bitrate_) { loss_based_result_.state = LossBasedState::kDecreasing; - } else if (bounded_bandwidth_estimate >= delay_based_estimate_) { + } else { loss_based_result_.state = LossBasedState::kDelayBasedEstimate; } loss_based_result_.bandwidth_estimate = bounded_bandwidth_estimate; 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 3ce750039c..a803f7d54d 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 @@ -711,7 +711,7 @@ TEST_F(LossBasedBweV2Test, } TEST_F(LossBasedBweV2Test, - LossBasedStateIsNotDelayBasedEstimateIfDelayBasedEsimtateInfinite) { + LossBasedStateIsNotDelayBasedEstimateIfDelayBasedEstimateInfinite) { ExplicitKeyValueConfig key_value_config( ShortObservationConfig("CandidateFactors:100|1|0.5," "InstantUpperBoundBwBalance:10000kbps," @@ -750,17 +750,14 @@ TEST_F(LossBasedBweV2Test, // a factor of acked bitrate. TEST_F(LossBasedBweV2Test, IncreaseByFactorOfAckedBitrateAfterLossBasedBweBacksOff) { - ExplicitKeyValueConfig key_value_config( - ShortObservationConfig("LossThresholdOfHighBandwidthPreference:0.99," - "BwRampupUpperBoundFactor:1.2," - "InherentLossUpperBoundOffset:0.9")); + ExplicitKeyValueConfig key_value_config(ShortObservationConfig( + "LossThresholdOfHighBandwidthPreference:0.99," + "BwRampupUpperBoundFactor:1.2," + // Set InstantUpperBoundBwBalance high to disable InstantUpperBound cap. + "InstantUpperBoundBwBalance:10000kbps,")); std::vector enough_feedback_1 = CreatePacketResultsWith100pLossRate( /*first_packet_timestamp=*/Timestamp::Zero()); - std::vector enough_feedback_2 = - CreatePacketResultsWith10pLossRate( - /*first_packet_timestamp=*/Timestamp::Zero() + - kObservationDurationLowerBound); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); DataRate delay_based_estimate = DataRate::KilobitsPerSec(5000); @@ -771,19 +768,42 @@ TEST_F(LossBasedBweV2Test, loss_based_bandwidth_estimator.UpdateBandwidthEstimate(enough_feedback_1, delay_based_estimate, /*in_alr=*/false); + ASSERT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDecreasing); + LossBasedBweV2::Result result = + loss_based_bandwidth_estimator.GetLossBasedResult(); + DataRate estimate_1 = result.bandwidth_estimate; + ASSERT_LT(estimate_1.kbps(), 600); - // Change the acked bitrate to make sure that the estimate is bounded by a - // factor of acked bitrate. - DataRate acked_bitrate = DataRate::KilobitsPerSec(50); - loss_based_bandwidth_estimator.SetAcknowledgedBitrate(acked_bitrate); - loss_based_bandwidth_estimator.UpdateBandwidthEstimate(enough_feedback_2, - delay_based_estimate, - /*in_alr=*/false); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate(estimate_1 * 0.9); + + int feedback_count = 1; + while (feedback_count < 5 && result.state != LossBasedState::kIncreasing) { + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + feedback_count++ * kObservationDurationLowerBound), + delay_based_estimate, + /*in_alr=*/false); + result = loss_based_bandwidth_estimator.GetLossBasedResult(); + } + ASSERT_EQ(result.state, LossBasedState::kIncreasing); // The estimate is capped by acked_bitrate * BwRampupUpperBoundFactor. - DataRate estimate_2 = - loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate; - EXPECT_EQ(estimate_2, acked_bitrate * 1.2); + EXPECT_EQ(result.bandwidth_estimate, estimate_1 * 0.9 * 1.2); + + // But if acked bitrate decrease, BWE does not decrease when there is no + // loss. + loss_based_bandwidth_estimator.SetAcknowledgedBitrate(estimate_1 * 0.9); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + feedback_count++ * kObservationDurationLowerBound), + delay_based_estimate, + /*in_alr=*/false); + EXPECT_EQ( + loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, + result.bandwidth_estimate); } // After loss based bwe backs off, the estimate is bounded during the delayed @@ -1368,5 +1388,52 @@ TEST_F(LossBasedBweV2Test, HasIncreaseStateBecauseOfLowerBound) { LossBasedState::kIncreasing); } +TEST_F(LossBasedBweV2Test, HasDelayBasedStateIfLossBasedBweIsMax) { + ExplicitKeyValueConfig key_value_config(ShortObservationConfig("")); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + loss_based_bandwidth_estimator.SetMinMaxBitrate( + /*min_bitrate=*/DataRate::KilobitsPerSec(10), + /*max_bitrate=*/DataRate::KilobitsPerSec(1000)); + + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + /*feedback = */ CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero()), + /*delay_based_estimate=*/DataRate::KilobitsPerSec(2000), + /*in_alr=*/false); + EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDelayBasedEstimate); + EXPECT_EQ( + loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, + DataRate::KilobitsPerSec(1000)); + + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + /*feedback=*/CreatePacketResultsWith50pLossRate( + /*first_packet_timestamp=*/Timestamp::Zero() + + kObservationDurationLowerBound), + /*delay_based_estimate=*/DataRate::KilobitsPerSec(2000), + /*in_alr=*/false); + LossBasedBweV2::Result result = + loss_based_bandwidth_estimator.GetLossBasedResult(); + ASSERT_EQ(result.state, LossBasedState::kDecreasing); + ASSERT_LT(result.bandwidth_estimate, DataRate::KilobitsPerSec(1000)); + + // Eventually the estimator recovers to delay based state. + int feedback_count = 2; + while (feedback_count < 5 && + result.state != LossBasedState::kDelayBasedEstimate) { + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + /*feedback = */ CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + feedback_count++ * kObservationDurationLowerBound), + /*delay_based_estimate=*/DataRate::KilobitsPerSec(2000), + /*in_alr=*/false); + result = loss_based_bandwidth_estimator.GetLossBasedResult(); + } + EXPECT_EQ(result.state, LossBasedState::kDelayBasedEstimate); + EXPECT_EQ( + loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, + DataRate::KilobitsPerSec(1000)); +} + } // namespace } // namespace webrtc