Skip to content

Commit

Permalink
Revert "Ignore allocated bitrate during initial exponential BWE."
Browse files Browse the repository at this point in the history 
This reverts commit 33cc835.

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}

(cherry picked from commit 501c4f37bfee47b26999ee291c5355ad64554df7)

Bug: chromium:335337923,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-Original-Commit-Position: refs/heads/main@{#42081}
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/348722
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/branch-heads/6422@{#2}
Cr-Branched-From: b831eb8-refs/heads/main@{#42072}
  • Loading branch information
@perkj
perkj authored and WebRTC LUCI CQ committed Apr 23, 2024
1 parent 89e26b7 commit 8505a98
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 109 deletions.
4 changes: 0 additions & 4 deletions api/transport/network_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ struct StreamsConfig {
~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.
Expand Down
8 changes: 3 additions & 5 deletions call/rtp_transport_controller_send.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,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;
Expand All @@ -280,6 +275,9 @@ void RtpTransportControllerSend::ReconfigureBandwidthEstimation(
UpdateNetworkAvailability();
}
}
pacer_.SetAllowProbeWithoutMediaPacket(
bwe_settings_.allow_probe_without_media &&
packet_router_.SupportsRtxPayloadPadding());
}

void RtpTransportControllerSend::RegisterTargetTransferRateObserver(
Expand Down
4 changes: 0 additions & 4 deletions modules/congestion_controller/goog_cc/goog_cc_network_control.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<DataRate> total_bitrate =
initial_config_->stream_based_config.max_total_allocated_bitrate;
if (total_bitrate) {
Expand Down
39 changes: 8 additions & 31 deletions modules/congestion_controller/goog_cc/probe_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,22 +272,6 @@ std::vector<ProbeClusterConfig> ProbeController::OnNetworkAvailability(
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_);
Expand All @@ -303,8 +287,6 @@ std::vector<ProbeClusterConfig> 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);
}

Expand All @@ -325,7 +307,6 @@ std::vector<ProbeClusterConfig> 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();
Expand Down Expand Up @@ -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<int64_t> alr_start_time_ms) {
if (alr_start_time_ms) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -477,7 +452,8 @@ std::vector<ProbeClusterConfig> 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) {
Expand All @@ -504,14 +480,14 @@ std::vector<ProbeClusterConfig> 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
Expand Down Expand Up @@ -579,14 +555,15 @@ std::vector<ProbeClusterConfig> 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;
}
Expand Down
6 changes: 0 additions & 6 deletions modules/congestion_controller/goog_cc/probe_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int64_t> alr_start_time);
void SetAlrEndedTimeMs(int64_t alr_end_time);
Expand Down Expand Up @@ -151,7 +148,6 @@ class ProbeController {
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(
Expand All @@ -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_;
Expand Down
59 changes: 0 additions & 59 deletions modules/congestion_controller/goog_cc/probe_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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/");
Expand All @@ -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<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 =
Expand Down

0 comments on commit 8505a98

Please sign in to comment.