diff --git a/api/video/video_stream_encoder_settings.h b/api/video/video_stream_encoder_settings.h index e49305b375..c8cb368ce7 100644 --- a/api/video/video_stream_encoder_settings.h +++ b/api/video/video_stream_encoder_settings.h @@ -57,25 +57,6 @@ 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 diff --git a/media/BUILD.gn b/media/BUILD.gn index 08e924a91b..c39f75c8c8 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -92,6 +92,7 @@ rtc_library("rtc_media_base") { "../api/transport/rtp:rtp_source", "../api/units:time_delta", "../api/video:recordable_encoded_frame", + "../api/video:resolution", "../api/video:video_bitrate_allocation", "../api/video:video_bitrate_allocator_factory", "../api/video:video_frame", @@ -159,6 +160,7 @@ rtc_library("video_adapter") { ] deps = [ ":video_common", + "../api/video:resolution", "../api/video:video_frame", "../common_video", "../rtc_base:checks", diff --git a/media/base/video_adapter.cc b/media/base/video_adapter.cc index bb1515b12b..35bd722fbd 100644 --- a/media/base/video_adapter.cc +++ b/media/base/video_adapter.cc @@ -211,6 +211,28 @@ bool VideoAdapter::AdaptFrameResolution(int in_width, return false; } + // Adjust input width and height without modifying aspect ratio as to be less + // than or equal to the `requested_resolution_`. + if (requested_resolution_.has_value()) { + // Make frame and requested resolution have matching orientation. + webrtc::Resolution requested_resolution = requested_resolution_.value(); + if ((in_width < in_height) != + (requested_resolution_->width < requested_resolution_->height)) { + requested_resolution = {.width = requested_resolution_->height, + .height = requested_resolution_->width}; + } + // Downscale by smallest scaling factor, if necessary. + if (in_width > 0 && in_height > 0 && + (requested_resolution.width < in_width || + requested_resolution.height < in_height)) { + double scale_factor = std::min( + requested_resolution.width / static_cast(in_width), + requested_resolution.height / static_cast(in_height)); + in_width = std::round(in_width * scale_factor); + in_height = std::round(in_height * scale_factor); + } + } + // Calculate how the input should be cropped. if (!target_aspect_ratio || target_aspect_ratio->first <= 0 || target_aspect_ratio->second <= 0) { @@ -341,6 +363,13 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) { max_framerate_request_ = sink_wants.max_framerate_fps; resolution_alignment_ = cricket::LeastCommonMultiple( source_resolution_alignment_, sink_wants.resolution_alignment); + // Convert from std::optional to + // std::optional. Both are {int,int}. + requested_resolution_ = std::nullopt; + if (sink_wants.requested_resolution.has_value()) { + requested_resolution_ = {.width = sink_wants.requested_resolution->width, + .height = sink_wants.requested_resolution->height}; + } // If requested_resolution is used, and there are no active encoders // that are NOT using requested_resolution (aka newapi), then override @@ -371,7 +400,7 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) { } if (!stashed_output_format_request_) { - // The active output format request is about to be rewritten by + // The active output format request is about to be cleared due to // request_resolution. We need to save it for later use in case the encoder // which doesn't use request_resolution logic become active in the future. stashed_output_format_request_ = output_format_request_; @@ -379,22 +408,9 @@ void VideoAdapter::OnSinkWants(const rtc::VideoSinkWants& sink_wants) { << stashed_output_format_request_->ToString(); } - auto res = *sink_wants.requested_resolution; - if (res.width < res.height) { - // Adjust `res` to landscape mode. - res.width = sink_wants.requested_resolution->height; - res.height = sink_wants.requested_resolution->width; - } - auto pixel_count = res.width * res.height; - output_format_request_.target_landscape_aspect_ratio = - std::make_pair(res.width, res.height); - output_format_request_.max_landscape_pixel_count = pixel_count; - output_format_request_.target_portrait_aspect_ratio = - std::make_pair(res.height, res.width); - output_format_request_.max_portrait_pixel_count = pixel_count; - output_format_request_.max_fps = max_framerate_request_; - RTC_LOG(LS_INFO) << "Setting output_format_request_ based on sink_wants: " - << output_format_request_.ToString(); + // Clear the output format request. Requested resolution will be applied + // instead which happens inside AdaptFrameResolution(). + output_format_request_ = {}; } int VideoAdapter::GetTargetPixels() const { diff --git a/media/base/video_adapter.h b/media/base/video_adapter.h index 35ed6726e8..4cc4a38cfb 100644 --- a/media/base/video_adapter.h +++ b/media/base/video_adapter.h @@ -17,6 +17,7 @@ #include #include +#include "api/video/resolution.h" #include "api/video/video_source_interface.h" #include "common_video/framerate_controller.h" #include "media/base/video_common.h" @@ -148,6 +149,8 @@ class RTC_EXPORT VideoAdapter { int resolution_request_target_pixel_count_ RTC_GUARDED_BY(mutex_); int resolution_request_max_pixel_count_ RTC_GUARDED_BY(mutex_); int max_framerate_request_ RTC_GUARDED_BY(mutex_); + std::optional requested_resolution_ + RTC_GUARDED_BY(mutex_); // Stashed OutputFormatRequest that is used to save value of // OnOutputFormatRequest in case all active encoders are using diff --git a/media/base/video_adapter_unittest.cc b/media/base/video_adapter_unittest.cc index 5011438363..404d8e7f7d 100644 --- a/media/base/video_adapter_unittest.cc +++ b/media/base/video_adapter_unittest.cc @@ -59,13 +59,8 @@ rtc::VideoSinkWants BuildSinkWants( wants.resolution_alignment = 1; wants.is_active = true; if (requested_resolution) { - wants.target_pixel_count = requested_resolution->PixelCount(); - wants.max_pixel_count = requested_resolution->PixelCount(); wants.requested_resolution.emplace(rtc::VideoSinkWants::FrameSize( requested_resolution->width, requested_resolution->height)); - } else { - wants.target_pixel_count = kWidth * kHeight; - wants.max_pixel_count = kWidth * kHeight; } wants.aggregates.emplace(rtc::VideoSinkWants::Aggregates()); wants.aggregates->any_active_without_requested_resolution = @@ -1306,6 +1301,19 @@ TEST_P(VideoAdapterTest, RequestedResolutionIsOrientationAgnostic) { } } +TEST_P(VideoAdapterTest, RequestedResolutionMaintainsAspectRatio) { + // Request 720x720. + adapter_.OnSinkWants( + BuildSinkWants(Resolution{.width = 720, .height = 720}, + /* any_active_without_requested_resolution= */ false)); + + // A 1280x720 frame restricted to 720x720 produces 720x405. + EXPECT_THAT( + AdaptFrameResolution(/* input frame */ {.width = 1280, .height = 720}) + .first, + Eq(Resolution{.width = 720, .height = 405})); +} + class VideoAdapterWithSourceAlignmentTest : public VideoAdapterTest { protected: static constexpr int kSourceResolutionAlignment = 7; diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 61b9483521..da42b2a137 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -891,18 +891,12 @@ void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config, } } } - const bool signal_requested_resolution = - !settings_.use_standard_requested_resolution; - if ((signal_requested_resolution && - requested_resolution != - video_source_sink_controller_.requested_resolution()) || + if (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)) { - if (signal_requested_resolution) { - video_source_sink_controller_.SetRequestedResolution( - requested_resolution); - } + video_source_sink_controller_.SetRequestedResolution(requested_resolution); if (max_framerate >= 0) { video_source_sink_controller_.SetFrameRateUpperLimit(max_framerate); } else { diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index bf56316bdc..9e0626c83c 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -2530,56 +2530,14 @@ TEST_F(VideoStreamEncoderTest, FrameRateLimitCanBeReset) { video_stream_encoder_->Stop(); } -// 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 VideoStreamEncoderResolutionTest - : public VideoStreamEncoderTest, - public ::testing::WithParamInterface { - public: - VideoStreamEncoderResolutionTest() - : VideoStreamEncoderTest(), requested_resolution_version_(GetParam()) {} +TEST_F(VideoStreamEncoderTest, RequestInSinkWantsBeforeFirstFrame) { + // 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); - 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( - VideoStreamEncoderResolutionTest, - VideoStreamEncoderResolutionTest, - ::testing::Values( - RequestedResolutionVersion::kLegacyRequestedResolution, - RequestedResolutionVersion::kStandardRequestedResolution)); - -TEST_P(VideoStreamEncoderResolutionTest, RequestInSinkWantsBeforeFirstFrame) { ASSERT_THAT(video_encoder_config_.simulcast_layers, SizeIs(1)); video_encoder_config_.simulcast_layers[0].requested_resolution.emplace( Resolution({.width = 320, .height = 160})); @@ -2587,37 +2545,28 @@ TEST_P(VideoStreamEncoderResolutionTest, RequestInSinkWantsBeforeFirstFrame) { 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; - } + EXPECT_EQ(video_source_.sink_wants().requested_resolution, + rtc::VideoSinkWants::FrameSize(320, 160)); 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; - } + EXPECT_EQ(video_source_.sink_wants().requested_resolution, + rtc::VideoSinkWants::FrameSize(640, 320)); video_stream_encoder_->Stop(); } -TEST_P(VideoStreamEncoderResolutionTest, RequestInWrongAspectRatioWithAdapter) { +TEST_F(VideoStreamEncoderTest, RequestInWrongAspectRatioWithAdapter) { + // 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); + // Use a source that adapts resolution based on OnSinkWants. AdaptingFrameForwarder source(&time_controller_); source.set_adaptation_enabled(true); @@ -2633,16 +2582,8 @@ TEST_P(VideoStreamEncoderResolutionTest, RequestInWrongAspectRatioWithAdapter) { // 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; - } + // Expect 30x15, maintaining aspect ratio. + WaitForEncodedFrame(30, 15); video_stream_encoder_->Stop(); }