Revert "SCReAM: Scale backoff by feedback interval and reset ref window on congestion" This reverts commit 6214048b9dccb3abf7e1ab858396c4cfb0849703. Reason for revert: Downstream failures Failure Link: Link shared over chat Original change's description: > SCReAM: Scale backoff by feedback interval and reset ref window on congestion > > This CL: > 1. Tracks feedback interval in ScreamV2 and scales backoff relative > to effective virtual RTT (max of virtual RTT and feedback interval) > to account for how often feedback actually arrives. This avoids > stalled backoff during low bitrate audio with large feedback intervals. > 2. Clamps the reference window to data in flight on first congestion > in delay-based congestion control to eliminate initial rampup overshoot. > 3. Adds 'FeedbackIntervalAvgG' and 'UseFeedbackIntervalForVirtualRtt' > parameters to ScreamV2Parameters. > 4. Tightens expectations in AudioOnlyBothDirectionsLinkCapacity40To90KbpsRtt200ms > to reflect lower RTT and tighter BWE. > > Bug: webrtc:447037083 > Change-Id: I79d8cfff9e4ac034d8192a3668ea3342812ce141 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/506900 > Commit-Queue: Per Kjellander <perkj@webrtc.org> > Reviewed-by: Björn Terelius <terelius@webrtc.org> > Cr-Commit-Position: refs/heads/main@{#48777} Bug: webrtc:447037083 No-Presubmit: true No-Tree-Checks: true No-Try: true Change-Id: Id9bcc80855c272e447938b99708d6565b379857c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/507060 Auto-Submit: Tomas Gunnarsson <tommi@webrtc.org> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#48779}
diff --git a/modules/congestion_controller/scream/scream_network_controller_unittest.cc b/modules/congestion_controller/scream/scream_network_controller_unittest.cc index 21e1c69..d5a34e9 100644 --- a/modules/congestion_controller/scream/scream_network_controller_unittest.cc +++ b/modules/congestion_controller/scream/scream_network_controller_unittest.cc
@@ -1145,60 +1145,5 @@ PacerConfig::kDefaultTimeInterval); } -void TestClampsRefWindowOnFirstCongestionWhenStartingRateIsHigh( - bool send_as_ect1) { - SimulatedClock clock(Timestamp::Seconds(1'234)); - Environment env = CreateTestEnvironment({.time = &clock}); - NetworkControllerConfig config(env); - config.constraints.starting_rate = DataRate::KilobitsPerSec(300); - config.constraints.min_data_rate = DataRate::KilobitsPerSec(20); - config.constraints.max_data_rate = DataRate::KilobitsPerSec(1000); - config.stream_based_config.max_total_allocated_bitrate = - DataRate::KilobitsPerSec(1000); - ScreamNetworkController scream_controller(config); - scream_controller.OnNetworkAvailability( - {.at_time = clock.CurrentTime(), .network_available = true}); - - // RTT is 200ms (100ms one-way delay), link capacity is 30 kbps. - CcFeedbackGenerator feedback_generator({ - .network_config = {.queue_delay_ms = 100, - .link_capacity = DataRate::KilobitsPerSec(30)}, - .send_as_ect1 = send_as_ect1, - .packet_size = DataSize::Bytes(150), - }); - - TimeDelta rtt = TimeDelta::Millis(200); - Timestamp start_time = clock.CurrentTime(); - DataRate target_rate = config.constraints.starting_rate.value(); - while (target_rate > DataRate::KilobitsPerSec(60) && - clock.CurrentTime() - start_time < 10 * rtt) { - TransportPacketsFeedback feedback = - feedback_generator.ProcessUntilNextFeedback( - target_rate, clock, [&](const SentPacket& packet) { - scream_controller.OnSentPacket(packet); - }); - NetworkControlUpdate update = - scream_controller.OnTransportPacketsFeedback(feedback); - if (update.target_rate.has_value()) { - target_rate = update.target_rate->target_rate; - } - } - EXPECT_LE(target_rate, DataRate::KilobitsPerSec(60)); - EXPECT_GE(target_rate, DataRate::KilobitsPerSec(20)); - EXPECT_LE(clock.CurrentTime() - start_time, 2 * rtt); -} - -TEST(ScreamControllerTest, - ClampsRefWindowOnFirstDelayCongestionWhenStartingRateIsHigh) { - TestClampsRefWindowOnFirstCongestionWhenStartingRateIsHigh( - /*send_as_ect1=*/false); -} - -TEST(ScreamControllerTest, - ClampsRefWindowOnFirstCeCongestionWhenStartingRateIsHigh) { - TestClampsRefWindowOnFirstCongestionWhenStartingRateIsHigh( - /*send_as_ect1=*/true); -} - } // namespace } // namespace webrtc
diff --git a/modules/congestion_controller/scream/scream_v2.cc b/modules/congestion_controller/scream/scream_v2.cc index ec23cbb..33ece85 100644 --- a/modules/congestion_controller/scream/scream_v2.cc +++ b/modules/congestion_controller/scream/scream_v2.cc
@@ -28,13 +28,13 @@ namespace webrtc { + + ScreamV2::ScreamV2(const Environment& env) : env_(env), params_(env_.field_trials()), ref_window_(params_.min_ref_window.Get()), loss_estimator_(params_), - allow_initial_ref_window_clamping_( - params_.allow_initial_ref_window_clamping.Get()), delay_based_congestion_control_(params_) {} void ScreamV2::SetTargetBitrateConstraints(DataRate min, @@ -93,7 +93,6 @@ first_feedback_processed_ = true; } - UpdateFeedbackInterval(feedback.feedback_time); UpdateL4SAlpha(feedback); UpdateRefWindow(feedback); UpdateTargetRate(feedback); @@ -133,46 +132,33 @@ bool is_ce = parsed.num_ce_marked_packets > 0; bool is_loss = loss_estimator_.Update(parsed, delay_based_congestion_control_.rtt()); - bool is_virtual_ce = delay_based_congestion_control_.IsQueueDelayDetected(); + bool is_virtual_ce = false; + if (delay_based_congestion_control_.IsQueueDelayDetected()) { + is_virtual_ce = true; + } if (is_loss) { - if (!loss_estimator_.congested() && !is_virtual_ce) { + if (!loss_estimator_.congested() && + !delay_based_congestion_control_.IsQueueDelayDetected()) { is_loss = false; } } DataSize previous_ref_window = ref_window_; - TimeDelta time_since_last_reaction = - parsed.feedback_time - last_reaction_to_congestion_time_; - TimeDelta reaction_interval = - params_.use_feedback_interval_for_virtual_rtt.Get() - ? std::max(params_.virtual_rtt.Get(), feedback_interval_) - : params_.virtual_rtt.Get(); - if ((is_virtual_ce || is_ce || is_loss) && - time_since_last_reaction >= - std::min(delay_based_congestion_control_.rtt(), reaction_interval)) { + parsed.feedback_time - last_reaction_to_congestion_time_ >= + std::min(delay_based_congestion_control_.rtt(), + params_.virtual_rtt.Get())) { last_reaction_to_congestion_time_ = parsed.feedback_time; - if (allow_initial_ref_window_clamping_ && received_rate_.IsFinite()) { - // Clamp ref_window to received rate on the first congestion event, - // to adjust if starting rate was much higher than link capacity. - ref_window_ = - std::clamp(received_rate_ * (delay_based_congestion_control_.rtt() + - feedback_hold_time_), - params_.min_ref_window.Get(), ref_window_); - allow_initial_ref_window_clamping_ = false; - } - double backoff = 0.0; if (is_loss) { // Back off due to loss backoff = 1.0 - params_.beta_loss.Get(); } else if (is_ce) { // Backoff due to ECN-CE marking backoff = l4s_alpha_ / 2.0; // Scale down backoff when RTT is high as several backoff events occur - // per RTT. Scaling is relative to reaction_interval to account for - // how often feedback is actually received. + // per RTT backoff /= std::max( - 1.0, delay_based_congestion_control_.rtt() / reaction_interval); + 1.0, delay_based_congestion_control_.rtt() / params_.virtual_rtt); if (!delay_based_congestion_control_.IsQueueDelayDetected()) { // Scale down backoff if close to the last known max reference window @@ -188,18 +174,16 @@ .ref_window_scale_factor_due_to_avg_min_delay()); } - if (time_since_last_reaction.IsFinite() && - time_since_last_reaction > - params_.number_of_rtts_between_reset_ref_window_i_on_congestion - .Get() * - std::max(params_.virtual_rtt.Get(), - delay_based_congestion_control_.rtt()) && - max_data_in_flight_prev_rtt_ > DataSize::Zero()) { + if (parsed.feedback_time - last_reaction_to_congestion_time_ > + params_.number_of_rtts_between_reset_ref_window_i_on_congestion + .Get() * + std::max(params_.virtual_rtt.Get(), + delay_based_congestion_control_.rtt())) { // A long time(> 100 RTTs) since last congested because - // link throughput exceeds max video bitrate. + // link throughput exceeds max video bitrate. (or first congestion) // There is a certain risk that ref_wnd has increased way above // bytes in flight, so we reduce it here to get it better on - // track and thus the congestion episode is shortened. + // track and thus the congestion episode is shortened ref_window_ = std::clamp(max_data_in_flight_prev_rtt_, params_.min_ref_window.Get(), ref_window_); // In addition, bump up l4sAlpha to a more credible value @@ -209,8 +193,8 @@ } } else if (is_virtual_ce) { // Back off due to delay backoff = delay_based_congestion_control_.l4s_alpha_v() / 2.0; - backoff /= std::max( - 1.0, delay_based_congestion_control_.rtt() / reaction_interval); + backoff /= std::max(1.0, delay_based_congestion_control_.rtt() / + params_.virtual_rtt.Get()); } ref_window_ = (1.0 - backoff) * ref_window_; } @@ -339,21 +323,6 @@ (1.0 - params_.feedback_hold_time_avg_g.Get()) * feedback_hold_time_; } -void ScreamV2::UpdateFeedbackInterval(Timestamp feedback_time) { - if (last_feedback_time_.IsFinite()) { - TimeDelta delta = feedback_time - last_feedback_time_; - if (delta > TimeDelta::Zero()) { - if (feedback_interval_.IsZero()) { - feedback_interval_ = delta; - } else { - const double g = params_.feedback_interval_avg_g.Get(); - feedback_interval_ = delta * g + feedback_interval_ * (1.0 - g); - } - } - } - last_feedback_time_ = feedback_time; -} - void ScreamV2::UpdateTargetRate(const ScreamFeedback& parsed) { // Avoid division by zero. const TimeDelta non_zero_smoothed_rtt =
diff --git a/modules/congestion_controller/scream/scream_v2.h b/modules/congestion_controller/scream/scream_v2.h index 87c2f37..59add75 100644 --- a/modules/congestion_controller/scream/scream_v2.h +++ b/modules/congestion_controller/scream/scream_v2.h
@@ -132,7 +132,6 @@ void UpdateL4SAlpha(const ScreamFeedback& parsed); void UpdateRefWindow(const ScreamFeedback& parsed); void UpdateFeedbackHoldTime(TimeDelta feedback_hold_time); - void UpdateFeedbackInterval(Timestamp feedback_time); void UpdateTargetRate(const ScreamFeedback& parsed); void UpdateReceiveRate(const ScreamFeedback& parsed); @@ -164,14 +163,11 @@ LossEstimator loss_estimator_; TimeDelta feedback_hold_time_ = TimeDelta::Zero(); - Timestamp last_feedback_time_ = Timestamp::MinusInfinity(); - TimeDelta feedback_interval_ = TimeDelta::Zero(); // Per-RTT stats Timestamp last_data_in_flight_update_ = Timestamp::MinusInfinity(); DataSize max_data_in_flight_this_rtt_ = DataSize::Zero(); DataSize max_data_in_flight_prev_rtt_ = DataSize::Zero(); - bool allow_initial_ref_window_clamping_ = true; DataRate received_rate_ = DataRate::Zero(); DataSize accumulated_received_bytes_ = DataSize::Zero(); Timestamp last_window_receive_time_ = Timestamp::MinusInfinity();
diff --git a/modules/congestion_controller/scream/scream_v2_parameters.cc b/modules/congestion_controller/scream/scream_v2_parameters.cc index 8e1e4df..13d72de 100644 --- a/modules/congestion_controller/scream/scream_v2_parameters.cc +++ b/modules/congestion_controller/scream/scream_v2_parameters.cc
@@ -28,6 +28,7 @@ l4s_avg_g_down("L4sAvgGDown", 1.0 / 128.0), rtts_with_loss_before_backoff("RttsWithLossBeforeBackoff", 3), lossless_rtts_before_clear("LosslessRttsBeforeClear", 2), + smoothed_rtt_avg_g("SmoothedRttAvgG", 1.0 / 8.0), smoothed_rtt_avg_in_alr_g("SmoothedRttAvgInAlrG", 1.0 / 128.0), max_segment_size("MaxSegmentSize", DataSize::Bytes(1280)), @@ -71,10 +72,6 @@ pacing_factor("PacingFactor", 1.1), pacing_rate_received_factor("PacingRateReceivedFactor", 0.8), feedback_hold_time_avg_g("FeedbackHoldTimeAvgG", 1.0 / 8.0), - feedback_interval_avg_g("FeedbackIntervalAvgG", 0.1), - use_feedback_interval_for_virtual_rtt("UseFeedbackIntervalForVirtualRtt", - true), - allow_initial_ref_window_clamping("AllowInitialRefWindowClamping", true), allow_large_pacing_bursts_after_congestion_time( "AllowLargePacingBurstsAfterCongestionTime", TimeDelta::Seconds(15)), @@ -125,9 +122,6 @@ &pacing_factor, &pacing_rate_received_factor, &feedback_hold_time_avg_g, - &feedback_interval_avg_g, - &use_feedback_interval_for_virtual_rtt, - &allow_initial_ref_window_clamping, &allow_large_pacing_bursts_after_congestion_time, &enable_alr, &alr_threshold,
diff --git a/modules/congestion_controller/scream/scream_v2_parameters.h b/modules/congestion_controller/scream/scream_v2_parameters.h index a919c67..ee822d0 100644 --- a/modules/congestion_controller/scream/scream_v2_parameters.h +++ b/modules/congestion_controller/scream/scream_v2_parameters.h
@@ -154,18 +154,6 @@ // received until feedback is sent. If zero, this delay is ignored. FieldTrialParameter<double> feedback_hold_time_avg_g; - // Exponentially Weighted Moving Average (EWMA) factor for tracking average - // feedback interval. - FieldTrialParameter<double> feedback_interval_avg_g; - - // If true, effective virtual_rtt is calculated as - // std::max(virtual_rtt, feedback_interval). Otherwise, virtual_rtt is used. - FieldTrialParameter<bool> use_feedback_interval_for_virtual_rtt; - - // If true, reference window is clamped to received rate on the first - // congestion event after the first RTT. - FieldTrialParameter<bool> allow_initial_ref_window_clamping; - // If the time since last reaction to congestion is larger than this, the // pacing window is increased. I.e. packets are allowed to be sent in larger // bursts.
diff --git a/test/peer_scenario/bwe_integration_tests/scream_test.cc b/test/peer_scenario/bwe_integration_tests/scream_test.cc index 2e4b966..f708265 100644 --- a/test/peer_scenario/bwe_integration_tests/scream_test.cc +++ b/test/peer_scenario/bwe_integration_tests/scream_test.cc
@@ -1159,17 +1159,17 @@ ASSERT_GT(result.caller().subspan(2).size(), 0u); EXPECT_THAT(result.caller().subspan(2), Each(CurrentRoundTripTimeIsBetween(TimeDelta::Millis(180), - TimeDelta::Millis(500)))); + TimeDelta::Millis(700)))); ASSERT_GT(result.callee().subspan(2).size(), 0u); EXPECT_THAT(result.callee().subspan(2), Each(CurrentRoundTripTimeIsBetween(TimeDelta::Millis(180), - TimeDelta::Millis(500)))); + TimeDelta::Millis(700)))); EXPECT_THAT(result.caller().subspan(2), Each(AvailableSendBitrateIsBetween( - DataRate::KilobitsPerSec(5), DataRate::KilobitsPerSec(60)))); + DataRate::KilobitsPerSec(5), DataRate::KilobitsPerSec(90)))); EXPECT_THAT(result.callee().subspan(2), Each(AvailableSendBitrateIsBetween( - DataRate::KilobitsPerSec(5), DataRate::KilobitsPerSec(60)))); + DataRate::KilobitsPerSec(5), DataRate::KilobitsPerSec(90)))); } } // namespace