From b812b7a86b2af074e50e8974ed6c274c67d39d73 Mon Sep 17 00:00:00 2001 From: Per K Date: Sat, 15 Apr 2023 08:47:44 +0200 Subject: [PATCH] Delay probes after route change until transport is writable Ensure probes are not created until after the transport becomes writable even if the network route change. DTLS negotiation must complete before there is a point in sending probes. Bug: webrtc:14928 Change-Id: Ib3cb93aef9f38e306b094dd700ed595cf9eb3f32 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301362 Reviewed-by: Diep Bui Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#39870} --- .../goog_cc_network_control_unittest.cc | 23 +++++++++++++++ .../goog_cc/probe_controller.cc | 4 +-- .../goog_cc/probe_controller.h | 3 +- .../goog_cc/probe_controller_unittest.cc | 29 +++++++++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc index 18111f52ba..b44f2e7d21 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc @@ -23,6 +23,7 @@ #include "test/gtest.h" #include "test/scenario/scenario.h" +using ::testing::IsEmpty; using ::testing::NiceMock; namespace webrtc { @@ -359,6 +360,28 @@ TEST(GoogCcNetworkControllerTest, ProbeOnRouteChange) { update = controller->OnProcessInterval({.at_time = current_time}); } +TEST(GoogCcNetworkControllerTest, ProbeAfterRouteChangeWhenTransportWritable) { + NetworkControllerTestFixture fixture; + std::unique_ptr controller = + fixture.CreateController(); + Timestamp current_time = Timestamp::Millis(123); + + NetworkControlUpdate update = controller->OnNetworkAvailability( + {.at_time = current_time, .network_available = false}); + EXPECT_THAT(update.probe_cluster_configs, IsEmpty()); + + update = controller->OnNetworkRouteChange( + CreateRouteChange(current_time, 2 * kInitialBitrate, DataRate::Zero(), + 20 * kInitialBitrate)); + // Transport is not writable. So not point in sending a probe. + EXPECT_THAT(update.probe_cluster_configs, IsEmpty()); + + // Probe is sent when transport becomes writable. + update = controller->OnNetworkAvailability( + {.at_time = current_time, .network_available = true}); + EXPECT_THAT(update.probe_cluster_configs, Not(IsEmpty())); +} + // Bandwidth estimation is updated when feedbacks are received. // Feedbacks which show an increasing delay cause the estimation to be reduced. TEST(GoogCcNetworkControllerTest, UpdatesDelayBasedEstimate) { diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index d425ccef59..8fde515934 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -157,7 +157,8 @@ ProbeControllerConfig::~ProbeControllerConfig() = default; ProbeController::ProbeController(const FieldTrialsView* key_value_config, RtcEventLog* event_log) - : enable_periodic_alr_probing_(false), + : network_available_(true), + enable_periodic_alr_probing_(false), in_rapid_recovery_experiment_(absl::StartsWith( key_value_config->Lookup(kBweRapidRecoveryExperiment), "Enabled")), @@ -368,7 +369,6 @@ void ProbeController::SetNetworkStateEstimate( } void ProbeController::Reset(Timestamp at_time) { - network_available_ = true; bandwidth_limited_cause_ = BandwidthLimitedCause::kDelayBasedLimited; state_ = State::kInit; min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); diff --git a/modules/congestion_controller/goog_cc/probe_controller.h b/modules/congestion_controller/goog_cc/probe_controller.h index fc6f493009..ae00182c68 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.h +++ b/modules/congestion_controller/goog_cc/probe_controller.h @@ -136,7 +136,8 @@ class ProbeController { void SetNetworkStateEstimate(webrtc::NetworkStateEstimate estimate); // Resets the ProbeController to a state equivalent to as if it was just - // created EXCEPT for `enable_periodic_alr_probing_`. + // created EXCEPT for `enable_periodic_alr_probing_` and + // `network_available_`. void Reset(Timestamp at_time); ABSL_MUST_USE_RESULT std::vector Process( diff --git a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc index ebe3b4a8f7..99efde80e0 100644 --- a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc +++ b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc @@ -21,6 +21,7 @@ #include "test/gmock.h" #include "test/gtest.h" +using ::testing::IsEmpty; using ::testing::NiceMock; namespace webrtc { @@ -428,6 +429,34 @@ TEST(ProbeControllerTest, PeriodicProbingAfterReset) { EXPECT_EQ(probes[0].target_data_rate, kStartBitrate * 2); } +TEST(ProbeControllerTest, NoProbesWhenTransportIsNotWritable) { + ProbeControllerFixture fixture; + std::unique_ptr probe_controller = + fixture.CreateController(); + probe_controller->EnablePeriodicAlrProbing(true); + + std::vector probes = + probe_controller->OnNetworkAvailability({.network_available = false}); + EXPECT_THAT(probes, IsEmpty()); + EXPECT_THAT(probe_controller->SetBitrates(kMinBitrate, kStartBitrate, + kMaxBitrate, fixture.CurrentTime()), + IsEmpty()); + fixture.AdvanceTime(TimeDelta::Seconds(10)); + EXPECT_THAT(probe_controller->Process(fixture.CurrentTime()), IsEmpty()); + + // Controller is reset after a network route change. + // But, a probe should not be sent since the transport is not writable. + // Transport is not writable until after DTLS negotiation completes. + // However, the bitrate constraints may change. + probe_controller->Reset(fixture.CurrentTime()); + EXPECT_THAT( + probe_controller->SetBitrates(2 * kMinBitrate, 2 * kStartBitrate, + 2 * kMaxBitrate, fixture.CurrentTime()), + IsEmpty()); + fixture.AdvanceTime(TimeDelta::Seconds(10)); + EXPECT_THAT(probe_controller->Process(fixture.CurrentTime()), IsEmpty()); +} + TEST(ProbeControllerTest, TestExponentialProbingOverflow) { ProbeControllerFixture fixture; std::unique_ptr probe_controller =