From 6bf6cfb465b6977f55bc0e3d8c8d964893876284 Mon Sep 17 00:00:00 2001 From: Diep Bui Date: Fri, 25 Oct 2024 08:57:28 +0000 Subject: [PATCH] Do not remove the highest loss observation if we suddenly send a lot more. Bug: webrtc:12707 Change-Id: Iccb05694867e681690e623e4d648efd60a2b1d5f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366643 Reviewed-by: Per Kjellander Commit-Queue: Diep Bui Cr-Commit-Position: refs/heads/main@{#43307} --- .../goog_cc/loss_based_bwe_v2.cc | 39 +++++++++++- .../goog_cc/loss_based_bwe_v2.h | 2 + .../goog_cc/loss_based_bwe_v2_test.cc | 60 +++++++++++++++---- 3 files changed, 88 insertions(+), 13 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 f7852fd6de..f91b8ffeb3 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -469,6 +469,8 @@ std::optional LossBasedBweV2::CreateConfig( FieldTrialParameter bound_best_candidate("BoundBestCandidate", false); FieldTrialParameter pace_at_loss_based_estimate( "PaceAtLossBasedEstimate", false); + FieldTrialParameter median_sending_rate_factor( + "MedianSendingRateFactor", 2.0); if (key_value_config) { ParseFieldTrial({&enabled, &bandwidth_rampup_upper_bound_factor, @@ -509,7 +511,8 @@ std::optional LossBasedBweV2::CreateConfig( &use_byte_loss_rate, &padding_duration, &bound_best_candidate, - &pace_at_loss_based_estimate}, + &pace_at_loss_based_estimate, + &median_sending_rate_factor}, key_value_config->Lookup("WebRTC-Bwe-LossBasedBweV2")); } @@ -575,6 +578,7 @@ std::optional LossBasedBweV2::CreateConfig( config.padding_duration = padding_duration.Get(); config.bound_best_candidate = bound_best_candidate.Get(); config.pace_at_loss_based_estimate = pace_at_loss_based_estimate.Get(); + config.median_sending_rate_factor = median_sending_rate_factor.Get(); return config; } @@ -816,7 +820,7 @@ double LossBasedBweV2::CalculateAverageReportedByteLossRatio() const { DataSize max_lost_bytes = DataSize::Zero(); DataSize min_bytes_received = DataSize::Zero(); DataSize max_bytes_received = DataSize::Zero(); - + DataRate send_rate_of_max_loss_observation = DataRate::Zero(); for (const Observation& observation : observations_) { if (!observation.IsInitialized()) { continue; @@ -836,6 +840,7 @@ double LossBasedBweV2::CalculateAverageReportedByteLossRatio() const { max_loss_rate = loss_rate; max_lost_bytes = instant_temporal_weight * observation.lost_size; max_bytes_received = instant_temporal_weight * observation.size; + send_rate_of_max_loss_observation = observation.sending_rate; } if (loss_rate < min_loss_rate) { min_loss_rate = loss_rate; @@ -844,6 +849,13 @@ double LossBasedBweV2::CalculateAverageReportedByteLossRatio() const { } } } + if (GetMedianSendingRate() * config_->median_sending_rate_factor <= + send_rate_of_max_loss_observation) { + // If the median sending rate is less than half of the sending rate of the + // observation with max loss rate, i.e. we suddenly send a lot of data, then + // the loss rate might not be due to a spike. + return lost_bytes / total_bytes; + } return (lost_bytes - min_lost_bytes - max_lost_bytes) / (total_bytes - max_bytes_received - min_bytes_received); } @@ -1155,6 +1167,8 @@ bool LossBasedBweV2::PushBackObservation( const TimeDelta observation_duration = last_send_time - last_send_time_most_recent_observation_; // Too small to be meaningful. + // To consider: what if it is too long?, i.e. we did not receive any packets + // for a long time, then all the packets we received are too old. if (observation_duration <= TimeDelta::Zero() || observation_duration < config_->observation_duration_lower_bound) { return false; @@ -1205,4 +1219,25 @@ bool LossBasedBweV2::PaceAtLossBasedEstimate() const { loss_based_result_.state != LossBasedState::kDelayBasedEstimate; } +DataRate LossBasedBweV2::GetMedianSendingRate() const { + std::vector sending_rates; + for (const Observation& observation : observations_) { + if (!observation.IsInitialized() || !IsValid(observation.sending_rate) || + observation.sending_rate.IsZero()) { + continue; + } + sending_rates.push_back(observation.sending_rate); + } + if (sending_rates.empty()) { + return DataRate::Zero(); + } + absl::c_sort(sending_rates); + if (sending_rates.size() % 2 == 0) { + return (sending_rates[sending_rates.size() / 2 - 1] + + sending_rates[sending_rates.size() / 2]) / + 2; + } + return sending_rates[sending_rates.size() / 2]; +} + } // namespace webrtc 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 3e543f2655..a81e2067ef 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -128,6 +128,7 @@ class LossBasedBweV2 { TimeDelta padding_duration = TimeDelta::Zero(); bool bound_best_candidate = false; bool pace_at_loss_based_estimate = false; + double median_sending_rate_factor = 0.0; }; struct Derivatives { @@ -199,6 +200,7 @@ class LossBasedBweV2 { DataRate new_estimate); bool IsInLossLimitedState() const; bool CanKeepIncreasingState(DataRate estimate) const; + DataRate GetMedianSendingRate() const; std::optional acknowledged_bitrate_; std::optional config_; 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 60a45fce71..fddbb2e31d 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 @@ -147,19 +147,23 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { } std::vector CreatePacketResultsWith100pLossRate( - Timestamp first_packet_timestamp) { - std::vector enough_feedback(2); - enough_feedback[0].sent_packet.sequence_number = + Timestamp first_packet_timestamp, unsigned num_packets = 2) { + std::vector enough_feedback(num_packets); + for (unsigned i = 0; i < num_packets - 1; ++i) { + enough_feedback[i].sent_packet.sequence_number = + transport_sequence_number_++; + enough_feedback[i].sent_packet.size = DataSize::Bytes(kPacketSize); + enough_feedback[i].sent_packet.send_time = + first_packet_timestamp + TimeDelta::Millis(i * 10); + enough_feedback[i].receive_time = Timestamp::PlusInfinity(); + } + enough_feedback[num_packets - 1].sent_packet.sequence_number = transport_sequence_number_++; - enough_feedback[1].sent_packet.sequence_number = - transport_sequence_number_++; - enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize); - enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize); - enough_feedback[0].sent_packet.send_time = first_packet_timestamp; - enough_feedback[1].sent_packet.send_time = + enough_feedback[num_packets - 1].sent_packet.size = + DataSize::Bytes(kPacketSize); + enough_feedback[num_packets - 1].sent_packet.send_time = first_packet_timestamp + kObservationDurationLowerBound; - enough_feedback[0].receive_time = Timestamp::PlusInfinity(); - enough_feedback[1].receive_time = Timestamp::PlusInfinity(); + enough_feedback[num_packets - 1].receive_time = Timestamp::PlusInfinity(); return enough_feedback; } @@ -1842,6 +1846,40 @@ TEST_F(LossBasedBweV2Test, UseByteLossRateIgnoreLossSpike) { kDelayBasedEstimate); } +TEST_F(LossBasedBweV2Test, UseByteLossRateDoesNotIgnoreLossSpikeOnSendBurst) { + ExplicitKeyValueConfig key_value_config( + "WebRTC-Bwe-LossBasedBweV2/" + "UseByteLossRate:true,ObservationWindowSize:5/"); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + const DataRate kDelayBasedEstimate = DataRate::KilobitsPerSec(500); + loss_based_bandwidth_estimator.SetBandwidthEstimate(kDelayBasedEstimate); + + // Fill the observation window. + for (int i = 0; i < 5; ++i) { + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + CreatePacketResultsWithReceivedPackets( + /*first_packet_timestamp=*/Timestamp::Zero() + + i * kObservationDurationLowerBound), + kDelayBasedEstimate, + /*in_alr=*/false); + } + + // If the loss happens when increasing sending rate, then + // the BWE should back down. + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + CreatePacketResultsWith100pLossRate( + /*first_packet_timestamp=*/Timestamp::Zero() + + 5 * kObservationDurationLowerBound, + /*num_packets=*/5), + kDelayBasedEstimate, + /*in_alr=*/false); + EXPECT_EQ(loss_based_bandwidth_estimator.GetLossBasedResult().state, + LossBasedState::kDecreasing); + EXPECT_LE( + loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, + kDelayBasedEstimate); +} + TEST_F(LossBasedBweV2Test, PaceAtLossBasedEstimate) { ExplicitKeyValueConfig key_value_config(ShortObservationConfig( "PaceAtLossBasedEstimate:true,PaddingDuration:1000ms"));