Fixes incorrect feedback to EncoderBitrateAdjuster [perf note]

At the point where an EncodedImage is reported to the
EncoderBitrateAdjuster (in order to estimate utilization), the image
data has been cleared so the size is 0 - meaning the esimtated
utilization is 0 so pushback is in effect only applied at the
beginning before an estimate is available.

This CL fixes that by explicitly using spatial/temporal id and size in
bytes, rather than passing along the EncodedImage proxy.

It is unclear when this broke, but the regression seems rather old.

This CL will affect the encoded bitrate (and thus indirectly BWE
ramp-up rate), but should avoid exessive delay at low bitrates.
Perf bots will likely trigger alerts, this is expected.

In case there are undesired side-effects, we can entirely disable the
adjuster using existing field-trials.

Bug: webrtc:12606
Change-Id: I936c2045f554696d8b4bb518eee6871ffc12c47d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212900
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33550}
This commit is contained in:
Erik Språng 2021-03-24 11:10:09 +01:00 committed by Commit Bot
parent ef036cdff2
commit 73cf80a932
5 changed files with 11 additions and 18 deletions

View File

@ -314,15 +314,14 @@ void EncoderBitrateAdjuster::OnEncoderInfo(
AdjustRateAllocation(current_rate_control_parameters_);
}
void EncoderBitrateAdjuster::OnEncodedFrame(const EncodedImage& encoded_image,
void EncoderBitrateAdjuster::OnEncodedFrame(DataSize size,
int spatial_index,
int temporal_index) {
++frames_since_layout_change_;
// Detectors may not exist, for instance if ScreenshareLayers is used.
auto& detector =
overshoot_detectors_[encoded_image.SpatialIndex().value_or(0)]
[temporal_index];
auto& detector = overshoot_detectors_[spatial_index][temporal_index];
if (detector) {
detector->OnEncodedFrame(encoded_image.size(), rtc::TimeMillis());
detector->OnEncodedFrame(size.bytes(), rtc::TimeMillis());
}
}

View File

@ -47,7 +47,7 @@ class EncoderBitrateAdjuster {
void OnEncoderInfo(const VideoEncoder::EncoderInfo& encoder_info);
// Updates the overuse detectors according to the encoded image size.
void OnEncodedFrame(const EncodedImage& encoded_image, int temporal_index);
void OnEncodedFrame(DataSize size, int spatial_index, int temporal_index);
void Reset();

View File

@ -160,15 +160,12 @@ class EncoderBitrateAdjusterTest : public ::testing::Test {
int sequence_idx = sequence_idx_[si][ti];
sequence_idx_[si][ti] = (sequence_idx_[si][ti] + 1) % kSequenceLength;
const size_t frame_size_bytes =
const DataSize frame_size = DataSize::Bytes(
(sequence_idx < kSequenceLength / 2)
? media_frame_size - network_frame_size_diff_bytes
: media_frame_size + network_frame_size_diff_bytes;
: media_frame_size + network_frame_size_diff_bytes);
EncodedImage image;
image.SetEncodedData(EncodedImageBuffer::Create(frame_size_bytes));
image.SetSpatialIndex(si);
adjuster_->OnEncodedFrame(image, ti);
adjuster_->OnEncodedFrame(frame_size, si, ti);
sequence_idx = ++sequence_idx % kSequenceLength;
}
}

View File

@ -2159,7 +2159,8 @@ void VideoStreamEncoder::RunPostEncode(const EncodedImage& encoded_image,
stream_resource_manager_.OnEncodeCompleted(encoded_image, time_sent_us,
encode_duration_us);
if (bitrate_adjuster_) {
bitrate_adjuster_->OnEncodedFrame(encoded_image, temporal_index);
bitrate_adjuster_->OnEncodedFrame(
frame_size, encoded_image.SpatialIndex().value_or(0), temporal_index);
}
}

View File

@ -6953,11 +6953,7 @@ TEST_F(VideoStreamEncoderTest, DropsFramesWhenEncoderOvershoots) {
// doesn't push back as hard so we don't need quite as much overshoot.
// These numbers are unfortunately a bit magical but there's not trivial
// way to algebraically infer them.
if (trials.BitrateAdjusterCanUseNetworkHeadroom()) {
overshoot_factor = 2.4;
} else {
overshoot_factor = 4.0;
}
overshoot_factor = 3.0;
}
fake_encoder_.SimulateOvershoot(overshoot_factor);
video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources(