admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Avoid signaling requested_resolution back to the adapting source.

Browse Source 
When requested_resolution uses a different aspect ratio than the source
the encoder will restrict the frame without changing its aspect ratio,
e.g. a 60x30 input frame that is restricted to 30x30 results in 30x15,
not 30x30.

While this logic works correctly in isolation, if the source also adapts
the frame size based on the sink_wants.requested_resolution that is
signaled back to the source, then the source will produce stretched
30x30 prior to the encoder which happily sends 30x30 not knowing any
wiser.

This is incompatible with the spec[1] and makes this WPT[2] fail. The
correct behavior is to NOT signal the requested_resolution back to the
source, the encoder already configures the correct resolution so this
isn't actually needed and the source shouldn't need to know this API.

In order not to break downstream projects, the new behavior is landed
behind a flag and both behaviors are tested with TEST_P.

This unblocks launching scaleResolutionDownTo API on Web. Migrating
from old to new code path and deleting the flag is a follow-up AI:
webrtc:366284861.

[1] https://w3c.github.io/webrtc-extensions/#dom-rtcrtpencodingparameters-scaleresolutiondownto
[2] https://chromium-review.googlesource.com/c/chromium/src/+/5853944

# Relying on previous green runs for confidence due to purple bots atm,
# see b/367211396
NOTRY=True
NOPRESUBMIT=True

Bug: webrtc:366067962, webrtc:366284861
Change-Id: I7fd1016e9cc6f0b0b9b8c23b0708e521f8e12642
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362541
Reviewed-by: Henrik Andreassson <henrika@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43024}
This commit is contained in:
Henrik Boström 2024-09-16 12:57:42 +02:00 committed by WebRTC LUCI CQ
parent 8487d3248b
commit cbf5122333
4 changed files with 140 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
api/video/video_stream_encoder_settings.h
View File

@ -57,6 +57,25 @@ struct VideoStreamEncoderSettings {
// Enables the frame instrumentation generator that is required for automatic
// corruption detection.
bool enable_frame_instrumentation_generator = false;
// According to spec, `requested_resolution` (called scaleResolutionDownTo in
// the web API) MUST NOT modify the aspect ratio of the frame, e.g. a 1280x720
// frame being restricted to maxWidth by maxHeight 720x720 should result in
// 720x405. In order for this to work, the video source must not adapt the
// input frame to the value of `requested_resolution`, as that would result in
// stretched 720x720.
//
// In order not to break backwards compatibility with C++ usage of this API,
// when `use_standard_requested_resolution` is false, the
// `requested_resolution` is signaled back to the video source. This works as
// long as the aspect ratio is the same, but breaks the web API use case.
//
// Spec:
// https://w3c.github.io/webrtc-extensions/#dom-rtcrtpencodingparameters-scaleresolutiondownto
//
// TODO(https://crbug.com/webrtc/366284861): Change the default to true,
// delete this flag and any code handling the legacy behavior.
bool use_standard_requested_resolution = false;
};
} // namespace webrtc

15
media/base/video_adapter.cc
View File

@ -342,12 +342,6 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) {
resolution_alignment_ = cricket::LeastCommonMultiple(
source_resolution_alignment_, sink_wants.resolution_alignment);
if (!sink_wants.aggregates) {
RTC_LOG(LS_WARNING)
<< "These should always be created by VideoBroadcaster!";
return;
}
// If requested_resolution is used, and there are no active encoders
// that are NOT using requested_resolution (aka newapi), then override
// calls to OnOutputFormatRequest and use values from requested_resolution
@ -365,7 +359,14 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) {
return;
}
if (sink_wants.aggregates->any_active_without_requested_resolution) {
// The code below is only needed when `requested_resolution` is signalled back
// to the video source which only happens if
// `VideoStreamEncoderSettings::use_standard_requested_resolution` is false.
// TODO(https://crbug.com/webrtc/366284861): Delete the code below as part of
// deleting this flag and only supporting the standard behavior.
if (sink_wants.aggregates.has_value() &&
sink_wants.aggregates->any_active_without_requested_resolution) {
return;
}

12
video/video_stream_encoder.cc
View File

@ -891,12 +891,18 @@ void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config,
}
}
}
if (requested_resolution !=
video_source_sink_controller_.requested_resolution() ||
const bool signal_requested_resolution =
!settings_.use_standard_requested_resolution;
if ((signal_requested_resolution &&
requested_resolution !=
video_source_sink_controller_.requested_resolution()) ||
active != video_source_sink_controller_.active() ||
max_framerate !=
video_source_sink_controller_.frame_rate_upper_limit().value_or(-1)) {
video_source_sink_controller_.SetRequestedResolution(requested_resolution);
if (signal_requested_resolution) {
video_source_sink_controller_.SetRequestedResolution(
requested_resolution);
}
if (max_framerate >= 0) {
video_source_sink_controller_.SetFrameRateUpperLimit(max_framerate);
} else {

102
video/video_stream_encoder_unittest.cc
View File

@ -2530,23 +2530,121 @@ TEST_F(VideoStreamEncoderTest, FrameRateLimitCanBeReset) {
video_stream_encoder_->Stop();
}
TEST_F(VideoStreamEncoderTest,
ResolutionLimitPropagatedToSinkWantsBeforeFirstFrame) {
// Tests both the standard and non-standard version of `requested_resolution`
// (also known as scaleResolutionDownTo) which behave differently with regards
// to signaling `requested_resolution` back to the source.
enum class RequestedResolutionVersion {
kLegacyRequestedResolution,
kStandardRequestedResolution,
};
class VideoStreamEncoderStandardOrLegacyRequestedResolutionTest
: public VideoStreamEncoderTest,
public ::testing::WithParamInterface<RequestedResolutionVersion> {
public:
VideoStreamEncoderStandardOrLegacyRequestedResolutionTest()
: VideoStreamEncoderTest(), requested_resolution_version_(GetParam()) {}
void SetUp() override {
VideoStreamEncoderTest::SetUp();
// Configure `use_standard_requested_resolution` based on test param.
switch (requested_resolution_version_) {
case RequestedResolutionVersion::kLegacyRequestedResolution:
video_send_config_.encoder_settings.use_standard_requested_resolution =
false;
break;
case RequestedResolutionVersion::kStandardRequestedResolution:
video_send_config_.encoder_settings.use_standard_requested_resolution =
true;
break;
}
// Use a real video stream factory or else `requested_resolution` is not
// applied correctly.
video_encoder_config_.video_stream_factory = nullptr;
ConfigureEncoder(video_encoder_config_.Copy());
video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources(
kTargetBitrate, kTargetBitrate, kTargetBitrate, 0, 0, 0);
}
protected:
const RequestedResolutionVersion requested_resolution_version_;
};
INSTANTIATE_TEST_SUITE_P(
VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
::testing::Values(
RequestedResolutionVersion::kLegacyRequestedResolution,
RequestedResolutionVersion::kStandardRequestedResolution));
TEST_P(VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
ResolutionLimitMaybePropagatedToSinkWantsBeforeFirstFrame) {
ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1));
video_encoder_config_.simulcast_layers[0].requested_resolution.emplace(
Resolution({.width = 320, .height = 160}));
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
// The legacy path signals `requested_resolution` back to the source. The
// standard path does not.
switch (requested_resolution_version_) {
case RequestedResolutionVersion::kLegacyRequestedResolution:
EXPECT_EQ(video_source_.sink_wants().requested_resolution,
rtc::VideoSinkWants::FrameSize(320, 160));
break;
case RequestedResolutionVersion::kStandardRequestedResolution:
EXPECT_FALSE(video_source_.sink_wants().requested_resolution.has_value());
break;
}
video_encoder_config_.simulcast_layers[0].requested_resolution->height = 320;
video_encoder_config_.simulcast_layers[0].requested_resolution->width = 640;
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
switch (requested_resolution_version_) {
case RequestedResolutionVersion::kLegacyRequestedResolution:
EXPECT_EQ(video_source_.sink_wants().requested_resolution,
rtc::VideoSinkWants::FrameSize(640, 320));
break;
case RequestedResolutionVersion::kStandardRequestedResolution:
EXPECT_FALSE(video_source_.sink_wants().requested_resolution.has_value());
break;
}
video_stream_encoder_->Stop();
}
TEST_P(VideoStreamEncoderStandardOrLegacyRequestedResolutionTest,
RequestedResolutionInWrongAspectRatioAndSourceIsAdapting) {
// Use a source that adapts resolution based on OnSinkWants.
AdaptingFrameForwarder source(&time_controller_);
source.set_adaptation_enabled(true);
video_stream_encoder_->SetSource(
&source, webrtc::DegradationPreference::MAINTAIN_FRAMERATE);
ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1));
video_encoder_config_.simulcast_layers[0].requested_resolution = {
.width = 30, .height = 30};
video_stream_encoder_->ConfigureEncoder(video_encoder_config_.Copy(),
kMaxPayloadLength);
// Capture a 60x30 frame.
source.IncomingCapturedFrame(CreateFrame(1, 60, 30));
// The 60x30 frame does not fit inside the 30x30 restrictions.
// - The non-standard path produces 30x30 due to OnSinkWants() signaling.
// - The standard path produces 30x15, maintaining aspect ratio.
switch (requested_resolution_version_) {
case RequestedResolutionVersion::kLegacyRequestedResolution:
WaitForEncodedFrame(30, 30);
break;
case RequestedResolutionVersion::kStandardRequestedResolution:
WaitForEncodedFrame(30, 15);
break;
}
video_stream_encoder_->Stop();
}