Do not allow estimate to increase above the estimate when HOLD started.
To ensure padding, we increase 1 bit instead of 1kbps to avoid that 1kbps adds up over time.
Not have unit test for this, but did manual/hamrit tests many times.
Bug: webrtc:12707
Change-Id: I9b3160ab1808cb3a21ff0609446359a4ec3a4949
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325520
Commit-Queue: Diep Bui <diepbp@webrtc.org>
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41056}
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 15921cc..445be08 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 @@
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 @@
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 @@
}
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 @@
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 70da76d..228d598 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 @@
Timestamp padding_timestamp = Timestamp::MinusInfinity();
};
+ struct HoldInfo {
+ Timestamp timestamp = Timestamp::MinusInfinity();
+ TimeDelta duration = TimeDelta::Zero();
+ DataRate rate = DataRate::PlusInfinity();
+ };
+
static absl::optional<Config> CreateConfig(
const FieldTrialsView* key_value_config);
bool IsConfigValid() const;
@@ -207,8 +213,7 @@
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 77d3f02..16aefcc 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 @@
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, 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 @@
/*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) {
