Revert "Ignore allocated bitrate during initial exponential BWE."
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 <diepbp@webrtc.org>
> Commit-Queue: Per Kjellander <perkj@webrtc.org>
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Reviewed-by: Per Kjellander <perkj@webrtc.org>
> 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 <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Diep Bui <diepbp@webrtc.org>
Owners-Override: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42081}
diff --git a/api/transport/network_types.h b/api/transport/network_types.h
index 46470d6..258a3a4 100644
--- a/api/transport/network_types.h
+++ b/api/transport/network_types.h
@@ -46,10 +46,6 @@
~StreamsConfig();
Timestamp at_time = Timestamp::PlusInfinity();
absl::optional<bool> 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<bool> initial_probe_to_max_bitrate;
absl::optional<double> pacing_factor;
// TODO(srte): Use BitrateAllocationLimits here.
diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc
index d739ed1..0ee6751 100644
--- a/call/rtp_transport_controller_send.cc
+++ b/call/rtp_transport_controller_send.cc
@@ -263,11 +263,6 @@
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 @@
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 815520a..d8a0ce9 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 @@
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<DataRate> 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 f9848b5..3fc8677 100644
--- a/modules/congestion_controller/goog_cc/probe_controller.cc
+++ b/modules/congestion_controller/goog_cc/probe_controller.cc
@@ -272,22 +272,6 @@
return std::vector<ProbeClusterConfig>();
}
-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<ProbeClusterConfig> ProbeController::InitiateExponentialProbing(
Timestamp at_time) {
RTC_DCHECK(network_available_);
@@ -303,8 +287,6 @@
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 @@
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 @@
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<int64_t> alr_start_time_ms) {
if (alr_start_time_ms) {
@@ -415,7 +391,6 @@
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 @@
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::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 @@
}
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 ec078ad..cec6157 100644
--- a/modules/congestion_controller/goog_cc/probe_controller.h
+++ b/modules/congestion_controller/goog_cc/probe_controller.h
@@ -121,9 +121,6 @@
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<int64_t> alr_start_time);
void SetAlrEndedTimeMs(int64_t alr_end_time);
@@ -151,7 +148,6 @@
kProbingComplete,
};
- void UpdateState(State new_state);
ABSL_MUST_USE_RESULT std::vector<ProbeClusterConfig>
InitiateExponentialProbing(Timestamp at_time);
ABSL_MUST_USE_RESULT std::vector<ProbeClusterConfig> InitiateProbing(
@@ -162,8 +158,6 @@
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 fb988d4..aa62c47 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 @@
EXPECT_EQ(probes[0].target_data_rate.bps(), 2 * 1800);
}
-TEST(ProbeControllerTest, ExponentialProbingStopIfMaxBitrateLow) {
- ProbeControllerFixture fixture(
- "WebRTC-Bwe-ProbingConfiguration/abort_further:true/");
- std::unique_ptr<ProbeController> 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 @@
EXPECT_THAT(probes, IsEmpty());
}
-TEST(ProbeControllerTest,
- InitialProbingIgnoreLowMaxAllocatedbitrateIfSetFirstProbeToMaxBitrate) {
- ProbeControllerFixture fixture(
- "WebRTC-Bwe-ProbingConfiguration/abort_further:true/");
- std::unique_ptr<ProbeController> 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<ProbeController> probe_controller =
