From 34f4ec26e3514e68e895004e3b455659d9c5cf69 Mon Sep 17 00:00:00 2001 From: Diep Bui Date: Fri, 18 Nov 2022 08:54:00 +0000 Subject: [PATCH] Fix the loss based bwe state. When best candidate estimate increases above the delay based estimate, the state should be DelayBasedEstimate because the final esimate is bounded by delay based bwe anyway. Bug: webrtc:12707 Change-Id: I0bcae684b33e5f1e9a7c57cb32c431b4eb58fd35 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/283802 Reviewed-by: Per Kjellander Commit-Queue: Diep Bui Cr-Commit-Position: refs/heads/main@{#38677} --- .../goog_cc/loss_based_bwe_v2.cc | 9 +- .../goog_cc/loss_based_bwe_v2_test.cc | 100 ++++++++++++++++++ 2 files changed, 104 insertions(+), 5 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 0b2200e474..f9cea09d1f 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -309,13 +309,12 @@ void LossBasedBweV2::UpdateBandwidthEstimate( } } - if (IsEstimateIncreasingWhenLossLimited(best_candidate)) { + if (IsEstimateIncreasingWhenLossLimited(best_candidate) && + best_candidate.loss_limited_bandwidth < delay_based_estimate) { current_state_ = LossBasedState::kIncreasing; - } else if (IsValid(delay_based_estimate_) && - best_candidate.loss_limited_bandwidth < delay_based_estimate_) { + } else if (best_candidate.loss_limited_bandwidth < delay_based_estimate_) { current_state_ = LossBasedState::kDecreasing; - } else if (IsValid(delay_based_estimate_) && - best_candidate.loss_limited_bandwidth == delay_based_estimate_) { + } else if (best_candidate.loss_limited_bandwidth >= delay_based_estimate_) { current_state_ = LossBasedState::kDelayBasedEstimate; } current_estimate_ = best_candidate; 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 1e1ae0d306..3cbc6f798e 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 @@ -783,6 +783,106 @@ TEST_P(LossBasedBweV2Test, result_at_loss.bandwidth_estimate * 1.5); } +TEST_P(LossBasedBweV2Test, + LossBasedStateIsDelayBasedEstimateAfterNetworkRecovering) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "Enabled:true,CandidateFactors:100|1|0.5,AckedRateCandidate:true," + "ObservationWindowSize:2,ObservationDurationLowerBound:200ms," + "InstantUpperBoundBwBalance:10000kbps," + "DelayBasedCandidate:true,MaxIncreaseFactor:100," + "BwRampupUpperBoundFactor:" + "2.0/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + DataRate delay_based_estimate = DataRate::KilobitsPerSec(600); + DataRate acked_rate = DataRate::KilobitsPerSec(300); + loss_based_bandwidth_estimator.SetBandwidthEstimate( + DataRate::KilobitsPerSec(600)); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate(acked_rate); + + // Create some loss to create the loss limited scenario. + std::vector enough_feedback_1 = + CreatePacketResultsWith100pLossRate( + /*first_packet_timestamp=*/Timestamp::Zero()); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback_1, delay_based_estimate, BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, + /*upper_link_capacity=*/DataRate::PlusInfinity()); + ASSERT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDecreasing); + + // Network recovers after loss. + std::vector enough_feedback_2 = + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + kObservationDurationLowerBound); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate( + DataRate::KilobitsPerSec(600)); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback_2, delay_based_estimate, BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, + /*upper_link_capacity=*/DataRate::PlusInfinity()); + EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDelayBasedEstimate); + + // Network recovers continuing. + std::vector enough_feedback_3 = + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + kObservationDurationLowerBound * 2); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate( + DataRate::KilobitsPerSec(600)); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback_3, delay_based_estimate, BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, + /*upper_link_capacity=*/DataRate::PlusInfinity()); + EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDelayBasedEstimate); +} + +TEST_P(LossBasedBweV2Test, + LossBasedStateIsNotDelayBasedEstimateIfDelayBasedEsimtateInfinite) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "Enabled:true,CandidateFactors:100|1|0.5,AckedRateCandidate:true," + "ObservationWindowSize:2,ObservationDurationLowerBound:200ms," + "InstantUpperBoundBwBalance:10000kbps," + "DelayBasedCandidate:true,MaxIncreaseFactor:100," + "BwRampupUpperBoundFactor:" + "2.0/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + DataRate delay_based_estimate = DataRate::PlusInfinity(); + DataRate acked_rate = DataRate::KilobitsPerSec(300); + loss_based_bandwidth_estimator.SetBandwidthEstimate( + DataRate::KilobitsPerSec(600)); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate(acked_rate); + + // Create some loss to create the loss limited scenario. + std::vector enough_feedback_1 = + CreatePacketResultsWith100pLossRate( + /*first_packet_timestamp=*/Timestamp::Zero()); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback_1, delay_based_estimate, BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, + /*upper_link_capacity=*/DataRate::PlusInfinity()); + ASSERT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDecreasing); + + // Network recovers after loss. + std::vector enough_feedback_2 = + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + kObservationDurationLowerBound); + loss_based_bandwidth_estimator.SetAcknowledgedBitrate( + DataRate::KilobitsPerSec(600)); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + enough_feedback_2, delay_based_estimate, BandwidthUsage::kBwNormal, + /*probe_estimate=*/absl::nullopt, + /*upper_link_capacity=*/DataRate::PlusInfinity()); + EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kIncreasing); +} + // After loss based bwe backs off, the next estimate is capped by // a factor of acked bitrate. TEST_P(LossBasedBweV2Test,