From 501c4f37bfee47b26999ee291c5355ad64554df7 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Tue, 16 Apr 2024 09:08:29 +0000 Subject: [PATCH] Revert "Ignore allocated bitrate during initial exponential BWE." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 33cc83595ae7dd144c57c614fb62d286d9d7bf68. Reason for revert: Perf bots showed that this cl cause a change in metrics. It looks like it is for the better, but we want this to be behind a field trial. Original change's description: > Ignore allocated bitrate during initial exponential BWE. > > The reason why we want to do this is because audio can allocate a needed bitrate before video when starting a call, which may lead to a race between the first probe result and updating the allocated bitrate. > That is the, initial probe will try to probe up to the max configured bitrate. > > ProbeController::SetFirstProbeToMaxBitrate will allow the first probe to > continue up to the max configured bitrate, regardless of of the max > allocated bitrate. > > Bug: webrtc:14928 > Change-Id: I6e0ae90e21a78466527f3464951e6033dc846470 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/346760 > Reviewed-by: Diep Bui > Commit-Queue: Per Kjellander > Reviewed-by: Erik Språng > Reviewed-by: Per Kjellander > Cr-Commit-Position: refs/heads/main@{#42049} Bug: webrtc:14928 Change-Id: I56ba58560b6857b6069552c02df822691f7af64d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/347622 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Per Kjellander Reviewed-by: Diep Bui Owners-Override: Per Kjellander Cr-Commit-Position: refs/heads/main@{#42081} --- api/transport/network_types.h | 4 -- call/rtp_transport_controller_send.cc | 8 +-- .../goog_cc/goog_cc_network_control.cc | 4 -- .../goog_cc/probe_controller.cc | 39 +++--------- .../goog_cc/probe_controller.h | 6 -- .../goog_cc/probe_controller_unittest.cc | 59 ------------------- 6 files changed, 11 insertions(+), 109 deletions(-) diff --git a/api/transport/network_types.h b/api/transport/network_types.h index 46470d6b5d..258a3a4350 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -46,10 +46,6 @@ struct StreamsConfig { ~StreamsConfig(); Timestamp at_time = Timestamp::PlusInfinity(); absl::optional requests_alr_probing; - // If `initial_probe_to_max_bitrate` is set to true, the first probe - // may probe up to the max configured bitrate and can ignore - // max_total_allocated_bitrate. - absl::optional initial_probe_to_max_bitrate; absl::optional pacing_factor; // TODO(srte): Use BitrateAllocationLimits here. diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index d739ed1ddd..0ee6751035 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -263,11 +263,6 @@ void RtpTransportControllerSend::ReconfigureBandwidthEstimation( RTC_DCHECK_RUN_ON(&sequence_checker_); bwe_settings_ = settings; - bool allow_probe_without_media = bwe_settings_.allow_probe_without_media && - packet_router_.SupportsRtxPayloadPadding(); - streams_config_.initial_probe_to_max_bitrate = allow_probe_without_media; - pacer_.SetAllowProbeWithoutMediaPacket(allow_probe_without_media); - if (controller_) { // Recreate the controller and handler. control_handler_ = nullptr; @@ -281,6 +276,9 @@ void RtpTransportControllerSend::ReconfigureBandwidthEstimation( UpdateNetworkAvailability(); } } + pacer_.SetAllowProbeWithoutMediaPacket( + bwe_settings_.allow_probe_without_media && + packet_router_.SupportsRtxPayloadPadding()); } void RtpTransportControllerSend::RegisterTargetTransferRateObserver( diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc index 815520ace2..d8a0ce9d64 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -218,10 +218,6 @@ NetworkControlUpdate GoogCcNetworkController::OnProcessInterval( probe_controller_->EnablePeriodicAlrProbing( *initial_config_->stream_based_config.requests_alr_probing); } - if (initial_config_->stream_based_config.initial_probe_to_max_bitrate) { - probe_controller_->SetFirstProbeToMaxBitrate( - *initial_config_->stream_based_config.initial_probe_to_max_bitrate); - } absl::optional total_bitrate = initial_config_->stream_based_config.max_total_allocated_bitrate; if (total_bitrate) { diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index f9848b563b..3fc8677e87 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -272,22 +272,6 @@ std::vector ProbeController::OnNetworkAvailability( return std::vector(); } -void ProbeController::UpdateState(State new_state) { - switch (new_state) { - case State::kInit: - state_ = State::kInit; - break; - case State::kWaitingForProbingResult: - state_ = State::kWaitingForProbingResult; - break; - case State::kProbingComplete: - state_ = State::kProbingComplete; - waiting_for_initial_probe_result_ = false; - min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); - break; - } -} - std::vector ProbeController::InitiateExponentialProbing( Timestamp at_time) { RTC_DCHECK(network_available_); @@ -303,8 +287,6 @@ std::vector ProbeController::InitiateExponentialProbing( probes.push_back(config_.second_exponential_probe_scale.Value() * start_bitrate_); } - waiting_for_initial_probe_result_ = true; - return InitiateProbing(at_time, probes, true); } @@ -325,7 +307,6 @@ std::vector ProbeController::SetEstimatedBitrate( if (config_.abort_further_probe_if_max_lower_than_current && (bitrate > max_bitrate_ || (!max_total_allocated_bitrate_.IsZero() && - !(waiting_for_initial_probe_result_ && first_probe_to_max_bitrate_) && bitrate > 2 * max_total_allocated_bitrate_))) { // No need to continue probing. min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); @@ -354,11 +335,6 @@ void ProbeController::EnablePeriodicAlrProbing(bool enable) { enable_periodic_alr_probing_ = enable; } -void ProbeController::SetFirstProbeToMaxBitrate( - bool first_probe_to_max_bitrate) { - first_probe_to_max_bitrate_ = first_probe_to_max_bitrate; -} - void ProbeController::SetAlrStartTimeMs( absl::optional alr_start_time_ms) { if (alr_start_time_ms) { @@ -415,7 +391,6 @@ void ProbeController::SetNetworkStateEstimate( void ProbeController::Reset(Timestamp at_time) { bandwidth_limited_cause_ = BandwidthLimitedCause::kDelayBasedLimited; state_ = State::kInit; - waiting_for_initial_probe_result_ = false; min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); time_last_probing_initiated_ = Timestamp::Zero(); estimated_bitrate_ = DataRate::Zero(); @@ -477,7 +452,8 @@ std::vector ProbeController::Process(Timestamp at_time) { kMaxWaitingTimeForProbingResult) { if (state_ == State::kWaitingForProbingResult) { RTC_LOG(LS_INFO) << "kWaitingForProbingResult: timeout"; - UpdateState(State::kProbingComplete); + state_ = State::kProbingComplete; + min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); } } if (estimated_bitrate_.IsZero() || state_ != State::kProbingComplete) { @@ -504,14 +480,14 @@ std::vector ProbeController::InitiateProbing( : std::min(max_total_allocated_bitrate_, max_bitrate_); if (std::min(network_estimate, estimated_bitrate_) > config_.skip_if_estimate_larger_than_fraction_of_max * max_probe_rate) { - UpdateState(State::kProbingComplete); + state_ = State::kProbingComplete; + min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); return {}; } } DataRate max_probe_bitrate = max_bitrate_; - if (max_total_allocated_bitrate_ > DataRate::Zero() && - !waiting_for_initial_probe_result_) { + if (max_total_allocated_bitrate_ > DataRate::Zero()) { // If a max allocated bitrate has been configured, allow probing up to 2x // that rate. This allows some overhead to account for bursty streams, // which otherwise would have to ramp up when the overshoot is already in @@ -579,14 +555,15 @@ std::vector ProbeController::InitiateProbing( } time_last_probing_initiated_ = now; if (probe_further) { - UpdateState(State::kWaitingForProbingResult); + state_ = State::kWaitingForProbingResult; // Dont expect probe results to be larger than a fraction of the actual // probe rate. min_bitrate_to_probe_further_ = std::min(estimate_capped_bitrate, (*(bitrates_to_probe.end() - 1))) * config_.further_probe_threshold; } else { - UpdateState(State::kProbingComplete); + state_ = State::kProbingComplete; + min_bitrate_to_probe_further_ = DataRate::PlusInfinity(); } return pending_probes; } diff --git a/modules/congestion_controller/goog_cc/probe_controller.h b/modules/congestion_controller/goog_cc/probe_controller.h index ec078adbc1..cec6157851 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.h +++ b/modules/congestion_controller/goog_cc/probe_controller.h @@ -121,9 +121,6 @@ class ProbeController { Timestamp at_time); void EnablePeriodicAlrProbing(bool enable); - // The first initial probe ignores allocated bitrate constraints and probe up - // to max configured bitrate configured via SetBitrates. - void SetFirstProbeToMaxBitrate(bool first_probe_to_max_bitrate); void SetAlrStartTimeMs(absl::optional alr_start_time); void SetAlrEndedTimeMs(int64_t alr_end_time); @@ -151,7 +148,6 @@ class ProbeController { kProbingComplete, }; - void UpdateState(State new_state); ABSL_MUST_USE_RESULT std::vector InitiateExponentialProbing(Timestamp at_time); ABSL_MUST_USE_RESULT std::vector InitiateProbing( @@ -162,8 +158,6 @@ class ProbeController { bool TimeForNetworkStateProbe(Timestamp at_time) const; bool network_available_; - bool waiting_for_initial_probe_result_ = false; - bool first_probe_to_max_bitrate_ = false; BandwidthLimitedCause bandwidth_limited_cause_ = BandwidthLimitedCause::kDelayBasedLimited; State state_; diff --git a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc index fb988d4c18..aa62c476d5 100644 --- a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc +++ b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc @@ -327,32 +327,6 @@ TEST(ProbeControllerTest, TestExponentialProbing) { EXPECT_EQ(probes[0].target_data_rate.bps(), 2 * 1800); } -TEST(ProbeControllerTest, ExponentialProbingStopIfMaxBitrateLow) { - ProbeControllerFixture fixture( - "WebRTC-Bwe-ProbingConfiguration/abort_further:true/"); - std::unique_ptr probe_controller = - fixture.CreateController(); - ASSERT_THAT( - probe_controller->OnNetworkAvailability({.network_available = true}), - IsEmpty()); - auto probes = probe_controller->SetBitrates( - kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime()); - ASSERT_THAT(probes, SizeIs(Gt(0))); - - // Repeated probe normally is sent when estimated bitrate climbs above - // 0.7 * 6 * kStartBitrate = 1260. But since max bitrate is low, expect - // exponential probing to stop. - probes = probe_controller->SetBitrates(kMinBitrate, kStartBitrate, - /*max_bitrate=*/kStartBitrate, - fixture.CurrentTime()); - EXPECT_THAT(probes, IsEmpty()); - - probes = probe_controller->SetEstimatedBitrate( - DataRate::BitsPerSec(1800), BandwidthLimitedCause::kDelayBasedLimited, - fixture.CurrentTime()); - EXPECT_THAT(probes, IsEmpty()); -} - TEST(ProbeControllerTest, ExponentialProbingStopIfMaxAllocatedBitrateLow) { ProbeControllerFixture fixture( "WebRTC-Bwe-ProbingConfiguration/abort_further:true/"); @@ -378,39 +352,6 @@ TEST(ProbeControllerTest, ExponentialProbingStopIfMaxAllocatedBitrateLow) { EXPECT_THAT(probes, IsEmpty()); } -TEST(ProbeControllerTest, - InitialProbingIgnoreLowMaxAllocatedbitrateIfSetFirstProbeToMaxBitrate) { - ProbeControllerFixture fixture( - "WebRTC-Bwe-ProbingConfiguration/abort_further:true/"); - std::unique_ptr probe_controller = - fixture.CreateController(); - ASSERT_THAT( - probe_controller->OnNetworkAvailability({.network_available = true}), - IsEmpty()); - auto probes = probe_controller->SetBitrates( - kMinBitrate, kStartBitrate, kMaxBitrate, fixture.CurrentTime()); - ASSERT_THAT(probes, SizeIs(Gt(0))); - probe_controller->SetFirstProbeToMaxBitrate(true); - - // Repeated probe is sent when estimated bitrate climbs above - // 0.7 * 6 * kStartBitrate = 1260. During the initial probe, we ignore the - // allocation limit and probe up to the max. - probes = probe_controller->OnMaxTotalAllocatedBitrate(kStartBitrate, - fixture.CurrentTime()); - EXPECT_THAT(probes, IsEmpty()); - - probes = probe_controller->SetEstimatedBitrate( - DataRate::BitsPerSec(1800), BandwidthLimitedCause::kDelayBasedLimited, - fixture.CurrentTime()); - EXPECT_EQ(probes.size(), 1u); - EXPECT_EQ(probes[0].target_data_rate.bps(), 2 * 1800); - - probes = probe_controller->SetEstimatedBitrate( - probes[0].target_data_rate, BandwidthLimitedCause::kDelayBasedLimited, - fixture.CurrentTime()); - EXPECT_EQ(probes.size(), 1u); -} - TEST(ProbeControllerTest, TestExponentialProbingTimeout) { ProbeControllerFixture fixture; std::unique_ptr probe_controller =