From e02fbb040e253d9e0449ad2085e32575394f88d8 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Fri, 28 Oct 2022 19:00:11 +0000 Subject: [PATCH] Revert "Periodically probe if current estimate lower than a ratio of NetworkState estimate" This reverts commit c371a13273c399249fb9bf602efed22e70e27166. Reason for revert: Speculative revert (breaks downstream project) Original change's description: > Periodically probe if current estimate lower than a ratio of NetworkState estimate > > This replace the immmediate probing if NetworkState estimate change. > > > Bug: webrtc:14392 > Change-Id: I2cc79c21015a4da2e6cba2098f1bc3c69944821f > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280741 > Reviewed-by: Diep Bui > Commit-Queue: Per Kjellander > Cr-Commit-Position: refs/heads/main@{#38495} Bug: webrtc:14392 Change-Id: I83cc8ab9986171e58971fb443d3e5d83afab3a2c No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280948 Owners-Override: Artem Titov Commit-Queue: Artem Titov Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Auto-Submit: Artem Titov Cr-Commit-Position: refs/heads/main@{#38497} --- .../goog_cc/probe_controller.cc | 68 +++++----- .../goog_cc/probe_controller.h | 13 +- .../goog_cc/probe_controller_unittest.cc | 125 +++++++++++++----- 3 files changed, 135 insertions(+), 71 deletions(-) diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index 1b98dcd872..501f14b874 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -87,12 +87,9 @@ ProbeControllerConfig::ProbeControllerConfig( alr_probe_scale("alr_scale", 2), network_state_estimate_probing_interval("network_state_interval", TimeDelta::PlusInfinity()), - probe_if_estimate_lower_than_network_state_estimate_ratio( - "est_lower_than_network_ratio", - 0), - estimate_lower_than_network_state_estimate_probing_interval( - "est_lower_than_network_interval", - TimeDelta::Seconds(3)), + network_state_estimate_fast_rampup_rate("network_state_fast_rampup_rate", + 0), + network_state_estimate_drop_down_rate("network_state_drop_down_rate", 0), network_state_probe_scale("network_state_scale", 1.0), network_state_probe_duration("network_state_probe_duration", TimeDelta::Millis(15)), @@ -114,10 +111,10 @@ ProbeControllerConfig::ProbeControllerConfig( &alr_probing_interval, &alr_probe_scale, &first_allocation_probe_scale, &second_allocation_probe_scale, &allocation_allow_further_probing, &min_probe_duration, &network_state_estimate_probing_interval, - &probe_if_estimate_lower_than_network_state_estimate_ratio, - &estimate_lower_than_network_state_estimate_probing_interval, - &network_state_probe_scale, &network_state_probe_duration, - &min_probe_packets_sent, &limit_probe_target_rate_to_loss_bwe, + &network_state_estimate_fast_rampup_rate, + &network_state_estimate_drop_down_rate, &network_state_probe_scale, + &network_state_probe_duration, &min_probe_packets_sent, + &limit_probe_target_rate_to_loss_bwe, &skip_if_estimate_larger_than_fraction_of_max}, key_value_config->Lookup("WebRTC-Bwe-ProbingConfiguration")); @@ -370,6 +367,24 @@ void ProbeController::SetMaxBitrate(DataRate max_bitrate) { void ProbeController::SetNetworkStateEstimate( webrtc::NetworkStateEstimate estimate) { + if (config_.network_state_estimate_fast_rampup_rate > 0 && + estimated_bitrate_ < estimate.link_capacity_upper && + (!network_estimate_ || + estimate.link_capacity_upper >= + config_.network_state_estimate_fast_rampup_rate * + network_estimate_->link_capacity_upper)) { + send_probe_on_next_process_interval_ = true; + } + if (config_.network_state_estimate_drop_down_rate > 0 && network_estimate_ && + !estimate.link_capacity_upper.IsZero() && + (estimated_bitrate_ > estimate.link_capacity_upper || + bwe_limited_due_to_packet_loss_) && + estimate.link_capacity_upper <= + config_.network_state_estimate_drop_down_rate * + network_estimate_->link_capacity_upper) { + send_probe_on_next_process_interval_ = true; + } + network_estimate_ = estimate; } @@ -390,6 +405,7 @@ void ProbeController::Reset(Timestamp at_time) { time_of_last_large_drop_ = now; bitrate_before_last_large_drop_ = DataRate::Zero(); max_total_allocated_bitrate_ = DataRate::Zero(); + send_probe_on_next_process_interval_ = false; } bool ProbeController::TimeForAlrProbe(Timestamp at_time) const { @@ -403,34 +419,13 @@ bool ProbeController::TimeForAlrProbe(Timestamp at_time) const { } bool ProbeController::TimeForNetworkStateProbe(Timestamp at_time) const { - if (!network_estimate_ || - network_estimate_->link_capacity_upper.IsInfinite()) { - return false; - } - - bool probe_due_to_low_estimate = - !bwe_limited_due_to_packet_loss_ && - estimated_bitrate_ < - config_.probe_if_estimate_lower_than_network_state_estimate_ratio * - network_estimate_->link_capacity_upper; - if (probe_due_to_low_estimate && - config_.estimate_lower_than_network_state_estimate_probing_interval - ->IsFinite()) { - Timestamp next_probe_time = - time_last_probing_initiated_ + - config_.estimate_lower_than_network_state_estimate_probing_interval; - return at_time >= next_probe_time; - } - - bool periodic_probe = - estimated_bitrate_ < network_estimate_->link_capacity_upper; - if (periodic_probe && - config_.network_state_estimate_probing_interval->IsFinite()) { + if (config_.network_state_estimate_probing_interval->IsFinite() && + network_estimate_ && network_estimate_->link_capacity_upper.IsFinite() && + estimated_bitrate_ < network_estimate_->link_capacity_upper) { Timestamp next_probe_time = time_last_probing_initiated_ + config_.network_state_estimate_probing_interval; return at_time >= next_probe_time; } - return false; } @@ -448,7 +443,8 @@ std::vector ProbeController::Process(Timestamp at_time) { if (estimated_bitrate_.IsZero() || state_ != State::kProbingComplete) { return {}; } - if (TimeForAlrProbe(at_time) || TimeForNetworkStateProbe(at_time)) { + if (send_probe_on_next_process_interval_ || TimeForAlrProbe(at_time) || + TimeForNetworkStateProbe(at_time)) { return InitiateProbing( at_time, {estimated_bitrate_ * config_.alr_probe_scale}, true); } @@ -495,6 +491,8 @@ std::vector ProbeController::InitiateProbing( std::min(max_probe_bitrate, max_total_allocated_bitrate_ * 2); } + send_probe_on_next_process_interval_ = false; + std::vector pending_probes; for (DataRate bitrate : bitrates_to_probe) { RTC_DCHECK(!bitrate.IsZero()); diff --git a/modules/congestion_controller/goog_cc/probe_controller.h b/modules/congestion_controller/goog_cc/probe_controller.h index 4bffc14439..e1ee08fc99 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.h +++ b/modules/congestion_controller/goog_cc/probe_controller.h @@ -50,12 +50,12 @@ struct ProbeControllerConfig { // Configures how often we send probes if NetworkStateEstimate is available. FieldTrialParameter network_state_estimate_probing_interval; - // Periodically probe as long as the the ratio beteeen current estimate and - // NetworkStateEstimate is lower then this. - FieldTrialParameter - probe_if_estimate_lower_than_network_state_estimate_ratio; - FieldTrialParameter - estimate_lower_than_network_state_estimate_probing_interval; + // If the network state estimate increase more than this rate, a probe is sent + // the next process interval. + FieldTrialParameter network_state_estimate_fast_rampup_rate; + // If the network state estimate decreases more than this rate, a probe is + // sent the next process interval. + FieldTrialParameter network_state_estimate_drop_down_rate; FieldTrialParameter network_state_probe_scale; // Overrides min_probe_duration if network_state_estimate_probing_interval // is set and a network state estimate is known. @@ -155,6 +155,7 @@ class ProbeController { DataRate min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); Timestamp time_last_probing_initiated_ = Timestamp::MinusInfinity(); DataRate estimated_bitrate_ = DataRate::Zero(); + bool send_probe_on_next_process_interval_; absl::optional network_estimate_; DataRate start_bitrate_ = DataRate::Zero(); DataRate max_bitrate_ = DataRate::PlusInfinity(); diff --git a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc index d7b580c3a0..2b2d71205e 100644 --- a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc +++ b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc @@ -657,12 +657,88 @@ TEST(ProbeControllerTest, CanSetLongerProbeDurationAfterNetworkStateEstimate) { EXPECT_EQ(probes[0].target_duration, TimeDelta::Millis(100)); } -TEST(ProbeControllerTest, ProbeIfEstimateLowerThanNetworkStateEstimate) { - // Periodic probe every 1 second if estimate is lower than 50% of the - // NetworkStateEstimate. +TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateIncrease) { ProbeControllerFixture fixture( - "WebRTC-Bwe-ProbingConfiguration/est_lower_than_network_interval:1s," - "est_lower_than_network_ratio:0.5,limit_probe_" + "WebRTC-Bwe-ProbingConfiguration/" + "network_state_interval:5s,network_state_fast_rampup_rate:2.0/"); + std::unique_ptr probe_controller = + fixture.CreateController(); + + auto probes = probe_controller->SetBitrates( + kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime()); + probes = probe_controller->SetEstimatedBitrate( + kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false, + fixture.CurrentTime()); + // Need to wait at least one second before process can trigger a new probe. + fixture.AdvanceTime(TimeDelta::Millis(1100)); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + NetworkStateEstimate state_estimate; + state_estimate.link_capacity_upper = kStartBitrate; + probe_controller->SetNetworkStateEstimate(state_estimate); + // No probe since NetworkStateEstimate is not higher than the set + // estimated bitrate. + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + // If NetworkState increase just a bit, dont expect the probe to be sent + // immediately. + state_estimate.link_capacity_upper = kStartBitrate * 1.4; + probe_controller->SetNetworkStateEstimate(state_estimate); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + // If NetworkState increase dramatically, expect a probe to be sent. + state_estimate.link_capacity_upper = kStartBitrate * 1.4 * 2; + probe_controller->SetNetworkStateEstimate(state_estimate); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_EQ(probes.size(), 1u); +} + +TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateDrop) { + ProbeControllerFixture fixture( + "WebRTC-Bwe-ProbingConfiguration/" + "network_state_interval:5s,network_state_drop_down_rate:0.5/"); + std::unique_ptr probe_controller = + fixture.CreateController(); + + auto probes = probe_controller->SetBitrates( + kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime()); + probes = probe_controller->SetEstimatedBitrate( + kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false, + fixture.CurrentTime()); + // Need to wait at least one second before process can trigger a new probe. + fixture.AdvanceTime(TimeDelta::Millis(1100)); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + NetworkStateEstimate state_estimate; + state_estimate.link_capacity_upper = kStartBitrate; + probe_controller->SetNetworkStateEstimate(state_estimate); + // No probe since NetworkStateEstimate is not lower than the set + // estimated bitrate. + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + // If NetworkState decrease just a bit, dont expect the probe to be sent + // immediately. + state_estimate.link_capacity_upper = kStartBitrate * 0.9; + probe_controller->SetNetworkStateEstimate(state_estimate); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_TRUE(probes.empty()); + + // If NetworkState decrease dramatically, expect a probe to be sent. + state_estimate.link_capacity_upper = kStartBitrate * 0.9 * 0.5; + probe_controller->SetNetworkStateEstimate(state_estimate); + probes = probe_controller->Process(fixture.CurrentTime()); + EXPECT_EQ(probes.size(), 1u); +} + +TEST(ProbeControllerTest, ProbeAfterLargeNetworkStateDropLossLimited) { + ProbeControllerFixture fixture( + "WebRTC-Bwe-ProbingConfiguration/" + "network_state_interval:5s,network_state_drop_down_rate:0.5,limit_probe_" "target_rate_to_loss_bwe:true/"); std::unique_ptr probe_controller = fixture.CreateController(); @@ -683,25 +759,17 @@ TEST(ProbeControllerTest, ProbeIfEstimateLowerThanNetworkStateEstimate) { probes = probe_controller->Process(fixture.CurrentTime()); EXPECT_TRUE(probes.empty()); - state_estimate.link_capacity_upper = kStartBitrate * 3; + // Loss limited. + probes = probe_controller->SetEstimatedBitrate( + kStartBitrate / 3, /*bwe_limited_due_to_packet_loss=*/true, + fixture.CurrentTime()); + // If NetworkState decrease dramatically, expect a probe to be sent. + // But limited to loss based estimate. + state_estimate.link_capacity_upper = kStartBitrate / 2; probe_controller->SetNetworkStateEstimate(state_estimate); probes = probe_controller->Process(fixture.CurrentTime()); ASSERT_EQ(probes.size(), 1u); - EXPECT_GT(probes[0].target_data_rate, kStartBitrate); - - // If network state not increased, send another probe. - fixture.AdvanceTime(TimeDelta::Millis(1100)); - probes = probe_controller->Process(fixture.CurrentTime()); - EXPECT_FALSE(probes.empty()); - - // Stop probing if estimate increase. We might probe further here though. - probes = probe_controller->SetEstimatedBitrate( - 2 * kStartBitrate, - /*bwe_limited_due_to_packet_loss=*/false, fixture.CurrentTime()); - // No more periodic probes. - fixture.AdvanceTime(TimeDelta::Millis(1100)); - probes = probe_controller->Process(fixture.CurrentTime()); - EXPECT_TRUE(probes.empty()); + EXPECT_EQ(probes[0].target_data_rate, kStartBitrate / 3); } TEST(ProbeControllerTest, DontProbeFurtherWhenLossLimited) { @@ -818,7 +886,7 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) { ProbeControllerFixture fixture( "WebRTC-Bwe-ProbingConfiguration/" "network_state_interval:2s,skip_if_est_larger_than_fraction_of_max:0.9," - "/"); + "network_state_drop_down_rate:0.5/"); std::unique_ptr probe_controller = fixture.CreateController(); auto probes = probe_controller->SetBitrates( @@ -831,15 +899,12 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) { fixture.CurrentTime()); EXPECT_TRUE(probes.empty()); - // Need to wait at least two seconds before process can trigger a new probe. - fixture.AdvanceTime(TimeDelta::Millis(2100)); + // Need to wait at least one second before process can trigger a new probe. + fixture.AdvanceTime(TimeDelta::Millis(1100)); - probes = probe_controller->SetEstimatedBitrate( - kStartBitrate, /*bwe_limited_due_to_packet_loss=*/false, - fixture.CurrentTime()); - EXPECT_TRUE(probes.empty()); + // Sends a probe immediately if NetworkState estimate decrease. probe_controller->SetNetworkStateEstimate( - {.link_capacity_upper = 2 * kStartBitrate}); + {.link_capacity_upper = kStartBitrate}); probes = probe_controller->Process(fixture.CurrentTime()); EXPECT_FALSE(probes.empty()); } @@ -847,7 +912,7 @@ TEST(ProbeControllerTest, SendsProbeIfNetworkStateEstimateLowerThanMaxProbe) { TEST(ProbeControllerTest, DontSendProbeIfNetworkStateEstimateIsZero) { ProbeControllerFixture fixture( "WebRTC-Bwe-ProbingConfiguration/" - "network_state_interval:5s,limit_probe_" + "network_state_interval:5s,network_state_drop_down_rate:0.5,limit_probe_" "target_rate_to_loss_bwe:true/"); std::unique_ptr probe_controller = fixture.CreateController();