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 15921ccffb..445be082e0 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 @@ LossBasedBweV2::LossBasedBweV2(const FieldTrialsView* key_value_config) instant_upper_bound_temporal_weights_.resize( config_->observation_window_size); CalculateTemporalWeights(); - hold_duration_ = kInitHoldDuration; + last_hold_info_.duration = kInitHoldDuration; } bool LossBasedBweV2::IsEnabled() const { @@ -305,7 +305,7 @@ void LossBasedBweV2::UpdateBandwidthEstimate( current_best_estimate_.loss_limited_bandwidth) { best_candidate.loss_limited_bandwidth = current_best_estimate_.loss_limited_bandwidth + - DataRate::KilobitsPerSec(1); + DataRate::BitsPerSec(1); } } } @@ -340,12 +340,12 @@ void LossBasedBweV2::UpdateResult() { } if (loss_based_result_.state == LossBasedState::kDecreasing && - last_hold_timestamp_ > last_send_time_most_recent_observation_ && + last_hold_info_.timestamp > last_send_time_most_recent_observation_ && bounded_bandwidth_estimate < delay_based_estimate_) { - // BWE is not allowed to increase during the HOLD duration. The purpose of + // BWE is not allowed to increase above the HOLD rate. The purpose of // HOLD is to not immediately ramp up BWE to a rate that may cause loss. - loss_based_result_.bandwidth_estimate = std::min( - loss_based_result_.bandwidth_estimate, bounded_bandwidth_estimate); + loss_based_result_.bandwidth_estimate = + std::min(last_hold_info_.rate, bounded_bandwidth_estimate); return; } @@ -372,18 +372,23 @@ void LossBasedBweV2::UpdateResult() { RTC_LOG(LS_INFO) << this << " " << "Switch to HOLD. Bounded BWE: " << bounded_bandwidth_estimate.kbps() - << ", duration: " << hold_duration_.seconds(); - last_hold_timestamp_ = - last_send_time_most_recent_observation_ + hold_duration_; - hold_duration_ = std::min(kMaxHoldDuration, - hold_duration_ * config_->hold_duration_factor); + << ", duration: " << last_hold_info_.duration.ms(); + last_hold_info_ = { + .timestamp = last_send_time_most_recent_observation_ + + last_hold_info_.duration, + .duration = + std::min(kMaxHoldDuration, last_hold_info_.duration * + config_->hold_duration_factor), + .rate = bounded_bandwidth_estimate}; } last_padding_info_ = PaddingInfo(); loss_based_result_.state = LossBasedState::kDecreasing; } else { - // Reset the HOLD duration if delay based estimate works to avoid getting + // Reset the HOLD info if delay based estimate works to avoid getting // stuck in low bitrate. - hold_duration_ = kInitHoldDuration; + last_hold_info_ = {.timestamp = Timestamp::MinusInfinity(), + .duration = kInitHoldDuration, + .rate = DataRate::PlusInfinity()}; last_padding_info_ = PaddingInfo(); loss_based_result_.state = LossBasedState::kDelayBasedEstimate; } 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 70da76d20a..228d5985f7 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -155,6 +155,12 @@ class LossBasedBweV2 { Timestamp padding_timestamp = Timestamp::MinusInfinity(); }; + struct HoldInfo { + Timestamp timestamp = Timestamp::MinusInfinity(); + TimeDelta duration = TimeDelta::Zero(); + DataRate rate = DataRate::PlusInfinity(); + }; + static absl::optional CreateConfig( const FieldTrialsView* key_value_config); bool IsConfigValid() const; @@ -207,8 +213,7 @@ class LossBasedBweV2 { DataRate max_bitrate_ = DataRate::PlusInfinity(); DataRate delay_based_estimate_ = DataRate::PlusInfinity(); LossBasedBweV2::Result loss_based_result_ = LossBasedBweV2::Result(); - Timestamp last_hold_timestamp_ = Timestamp::MinusInfinity(); - TimeDelta hold_duration_ = TimeDelta::Zero(); + HoldInfo last_hold_info_ = HoldInfo(); PaddingInfo last_padding_info_ = PaddingInfo(); }; 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 77d3f02207..16aefcc1d2 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 @@ -853,8 +853,7 @@ TEST_F(LossBasedBweV2Test, EnsureIncreaseEvenIfAckedBitrateBound) { ASSERT_EQ(result.state, LossBasedState::kIncreasing); // The estimate increases by 1kbps. - EXPECT_EQ(result.bandwidth_estimate, - estimate_1 + DataRate::KilobitsPerSec(1)); + EXPECT_EQ(result.bandwidth_estimate, estimate_1 + DataRate::BitsPerSec(1)); } // After loss based bwe backs off, the estimate is bounded during the delayed @@ -1678,7 +1677,7 @@ TEST_F(LossBasedBweV2Test, IncreaseEstimateIfNotHold) { TEST_F(LossBasedBweV2Test, IncreaseEstimateAfterHoldDuration) { ExplicitKeyValueConfig key_value_config( - ShortObservationConfig("HoldDurationFactor:3")); + ShortObservationConfig("HoldDurationFactor:10")); LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); loss_based_bandwidth_estimator.SetBandwidthEstimate( DataRate::KilobitsPerSec(2500)); @@ -1727,36 +1726,50 @@ TEST_F(LossBasedBweV2Test, IncreaseEstimateAfterHoldDuration) { /*in_alr=*/false); EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, LossBasedState::kDecreasing); - estimate = + DataRate estimate_at_hold = loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate; - // During the hold duration, e.g. next 900ms, the estimate cannot increase. + // In the hold duration, e.g. next 3s, the estimate cannot increase above the + // hold rate. Get some lost packets to get lower estimate than the HOLD rate. for (int i = 4; i <= 6; ++i) { loss_based_bandwidth_estimator.UpdateBandwidthEstimate( - CreatePacketResultsWithReceivedPackets( + CreatePacketResultsWith100pLossRate( /*first_packet_timestamp=*/Timestamp::Zero() + kObservationDurationLowerBound * i), /*delay_based_estimate=*/DataRate::PlusInfinity(), /*in_alr=*/false); EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, LossBasedState::kDecreasing); - EXPECT_EQ( + EXPECT_LT( loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, - estimate); + estimate_at_hold); } - // After the hold duration, the estimate can increase again. - loss_based_bandwidth_estimator.UpdateBandwidthEstimate( - CreatePacketResultsWithReceivedPackets( - /*first_packet_timestamp=*/Timestamp::Zero() + - kObservationDurationLowerBound * 7), - /*delay_based_estimate=*/DataRate::PlusInfinity(), - /*in_alr=*/false); - EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, - LossBasedState::kIncreasing); - EXPECT_GE( - loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, - estimate); + int feedback_id = 7; + while (loss_based_bandwidth_estimator.GetLossBasedResult().state != + LossBasedState::kIncreasing) { + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + kObservationDurationLowerBound * feedback_id), + /*delay_based_estimate=*/DataRate::PlusInfinity(), + /*in_alr=*/false); + if (loss_based_bandwidth_estimator.GetLossBasedResult().state == + LossBasedState::kDecreasing) { + // In the hold duration, the estimate can not go higher than estimate at + // hold. + EXPECT_LE(loss_based_bandwidth_estimator.GetLossBasedResult() + .bandwidth_estimate, + estimate_at_hold); + } else if (loss_based_bandwidth_estimator.GetLossBasedResult().state == + LossBasedState::kIncreasing) { + // After the hold duration, the estimate can increase again. + EXPECT_GT(loss_based_bandwidth_estimator.GetLossBasedResult() + .bandwidth_estimate, + estimate_at_hold); + } + feedback_id++; + } } TEST_F(LossBasedBweV2Test, EndHoldDurationIfDelayBasedEstimateWorks) {