Commit 667dc36e authored by David Staessens's avatar David Staessens Committed by Chromium LUCI CQ

Revert "media/gpu/vaapiWrapper: Enforce supported resolutions in CreateContext()"

This reverts commit 72c52cf1.

Reason for revert: Reason for revert: This breaks lower resolution test cases in ARC++/ARCVM: b/175515392

Original change's description:
> media/gpu/vaapiWrapper: Enforce supported resolutions in CreateContext()
>
> VaapiWrapper reports the supported min and max resolutions by
> VaapiWrapper::GetSupportedDec/EncodeProfiles(). They might be
> different from the min and max resolutions supported by a
> hardware and driver for some reason. For instance, we set
> the minimum encoder resolution to 321x241 and the webrtc encoder
> enforces the resolution per kWebRtcUseMinMaxVEADimensions
> (e.g. crbug.com/1008491).
> Clients of VaapiWrapper were expected to respect the supported
> resolutions, but could still request values outside of that
> range, thus using hardware codecs unintentionally. This CL
> enforces the supported range in VaapiWrapper::CreateContext().
> This also introduces a feature,
> VaapiEnforceVideoMinMaxResolution, to disable this check for
> testing.
>
> Bug: b:171041334
> Test: video.Play.vp9_hw on atlas and cyan
> Test: video_decode_accelerator_tests on atlas and cyan
> Test: video_encode_accelerator_tests on atlas and cyan
> Test: appr.tc/?vsc=h264&vrc=h264&debug=loopback on atlas and cyan
> Change-Id: Ide589ee4d1456553e400a271cb8b3383b472b4d4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2549161
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Auto-Submit: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
> Reviewed-by: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#832609}

TBR=dalecurtis@chromium.org,mcasas@chromium.org,hiroh@chromium.org,andrescj@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: b:171041334,b:175509204,b:175515392
Change-Id: I18e172dac47aae5949329b183373b59ee862c1d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589257Reviewed-by: default avatarHirokazu Honda <hiroh@chromium.org>
Reviewed-by: default avatarDavid Staessens <dstaessens@chromium.org>
Commit-Queue: David Staessens <dstaessens@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836958}
parent 5f474e36
...@@ -465,11 +465,6 @@ const base::Feature kVaapiAV1Decoder{"VaapiAV1Decoder", ...@@ -465,11 +465,6 @@ const base::Feature kVaapiAV1Decoder{"VaapiAV1Decoder",
const base::Feature kVaapiLowPowerEncoderGen9x{ const base::Feature kVaapiLowPowerEncoderGen9x{
"VaapiLowPowerEncoderGen9x", base::FEATURE_DISABLED_BY_DEFAULT}; "VaapiLowPowerEncoderGen9x", base::FEATURE_DISABLED_BY_DEFAULT};
// Deny specific (likely small) resolutions for VA-API hardware decode and
// encode acceleration.
const base::Feature kVaapiEnforceVideoMinMaxResolution{
"VaapiEnforceVideoMinMaxResolution", base::FEATURE_ENABLED_BY_DEFAULT};
// Enable VA-API hardware encode acceleration for VP8. // Enable VA-API hardware encode acceleration for VP8.
const base::Feature kVaapiVP8Encoder{"VaapiVP8Encoder", const base::Feature kVaapiVP8Encoder{"VaapiVP8Encoder",
base::FEATURE_ENABLED_BY_DEFAULT}; base::FEATURE_ENABLED_BY_DEFAULT};
......
...@@ -184,7 +184,6 @@ MEDIA_EXPORT extern const base::Feature kVaapiVideoDecodeLinux; ...@@ -184,7 +184,6 @@ MEDIA_EXPORT extern const base::Feature kVaapiVideoDecodeLinux;
#endif // (defined(OS_LINUX) || defined(OS_FREEBSD)) && !defined(OS_CHROMEOS) #endif // (defined(OS_LINUX) || defined(OS_FREEBSD)) && !defined(OS_CHROMEOS)
MEDIA_EXPORT extern const base::Feature kVaapiAV1Decoder; MEDIA_EXPORT extern const base::Feature kVaapiAV1Decoder;
MEDIA_EXPORT extern const base::Feature kVaapiLowPowerEncoderGen9x; MEDIA_EXPORT extern const base::Feature kVaapiLowPowerEncoderGen9x;
MEDIA_EXPORT extern const base::Feature kVaapiEnforceVideoMinMaxResolution;
MEDIA_EXPORT extern const base::Feature kVaapiVP8Encoder; MEDIA_EXPORT extern const base::Feature kVaapiVP8Encoder;
MEDIA_EXPORT extern const base::Feature kVaapiVP9Encoder; MEDIA_EXPORT extern const base::Feature kVaapiVP9Encoder;
MEDIA_EXPORT extern const base::Feature kVideoBlitColorAccuracy; MEDIA_EXPORT extern const base::Feature kVideoBlitColorAccuracy;
......
...@@ -48,11 +48,6 @@ const std::vector<base::Feature> kDisabledFeaturesForVideoEncoderTest = { ...@@ -48,11 +48,6 @@ const std::vector<base::Feature> kDisabledFeaturesForVideoEncoderTest = {
// with this feature flag. We disable the feature to use VpxVideoDecoder to // with this feature flag. We disable the feature to use VpxVideoDecoder to
// decode any vp8 stream in BitstreamValidator. // decode any vp8 stream in BitstreamValidator.
kFFmpegDecodeOpaqueVP8, kFFmpegDecodeOpaqueVP8,
#if BUILDFLAG(USE_VAAPI)
// Disable this feature so that the encoder test can test a resolution
// which is denied for the sake of performance. See crbug.com/1008491.
kVaapiEnforceVideoMinMaxResolution,
#endif
}; };
uint32_t GetDefaultTargetBitrate(const gfx::Size& resolution, uint32_t GetDefaultTargetBitrate(const gfx::Size& resolution,
......
...@@ -56,15 +56,7 @@ VideoPlayerTestEnvironment::VideoPlayerTestEnvironment( ...@@ -56,15 +56,7 @@ VideoPlayerTestEnvironment::VideoPlayerTestEnvironment(
media::kVaapiAV1Decoder, media::kVaapiAV1Decoder,
#endif #endif
}, },
/*disabled_features=*/ /*disabled_featureas=*/{}),
{
#if BUILDFLAG(USE_VAAPI)
// Disable this feature so that the decoder test can test a
// resolution which is denied for the sake of performance. See
// b/171041334.
kVaapiEnforceVideoMinMaxResolution,
#endif
}),
video_(std::move(video)), video_(std::move(video)),
enable_validator_(enable_validator), enable_validator_(enable_validator),
implementation_(implementation), implementation_(implementation),
......
...@@ -1108,26 +1108,6 @@ bool VASupportedProfiles::FillProfileInfo_Locked( ...@@ -1108,26 +1108,6 @@ bool VASupportedProfiles::FillProfileInfo_Locked(
return false; return false;
} }
if (base::FeatureList::IsEnabled(kVaapiEnforceVideoMinMaxResolution) &&
va_profile != VAProfileJPEGBaseline) {
// Deny unreasonably small resolutions (e.g. 0x0) for VA-API hardware video
// decode and encode acceleration.
profile_info->min_resolution.SetToMax(gfx::Size(16, 16));
if (entrypoint == VAEntrypointEncSliceLP ||
entrypoint == VAEntrypointEncSlice) {
// Using VA-API for accelerated encoding frames smaller than a certain
// size is less efficient than using a software encoder.
constexpr gfx::Size kMinEncodeResolution(320 + 1, 240 + 1);
if (!gfx::Rect(profile_info->min_resolution)
.Contains(gfx::Rect(kMinEncodeResolution))) {
profile_info->min_resolution.SetToMax(kMinEncodeResolution);
DVLOG(2) << "Setting the minimum supported encoding resolution to "
<< profile_info->min_resolution.ToString() << " for "
<< vaProfileStr(va_profile);
}
}
}
// Create a new configuration to find the supported RT formats. We don't pass // Create a new configuration to find the supported RT formats. We don't pass
// required attributes here because we want the driver to tell us all the // required attributes here because we want the driver to tell us all the
// supported RT formats. // supported RT formats.
...@@ -1412,7 +1392,10 @@ VaapiWrapper::GetSupportedEncodeProfiles() { ...@@ -1412,7 +1392,10 @@ VaapiWrapper::GetSupportedEncodeProfiles() {
VideoEncodeAccelerator::SupportedProfile profile; VideoEncodeAccelerator::SupportedProfile profile;
profile.profile = media_profile; profile.profile = media_profile;
profile.min_resolution = profile_info->min_resolution; // Using VA-API for accelerated encoding frames smaller than a certain
// size is less efficient than using a software encoder.
const gfx::Size kMinEncodeResolution = gfx::Size(320 + 1, 240 + 1);
profile.min_resolution = kMinEncodeResolution;
profile.max_resolution = profile_info->max_resolution; profile.max_resolution = profile_info->max_resolution;
// Maximum framerate of encoded profile. This value is an arbitrary // Maximum framerate of encoded profile. This value is an arbitrary
// limit and not taken from HW documentation. // limit and not taken from HW documentation.
...@@ -1452,7 +1435,7 @@ VaapiWrapper::GetSupportedDecodeProfiles( ...@@ -1452,7 +1435,7 @@ VaapiWrapper::GetSupportedDecodeProfiles(
VideoDecodeAccelerator::SupportedProfile profile; VideoDecodeAccelerator::SupportedProfile profile;
profile.profile = media_profile; profile.profile = media_profile;
profile.max_resolution = profile_info->max_resolution; profile.max_resolution = profile_info->max_resolution;
profile.min_resolution = profile_info->min_resolution; profile.min_resolution.SetSize(16, 16);
profiles.push_back(profile); profiles.push_back(profile);
} }
return profiles; return profiles;
...@@ -1879,25 +1862,6 @@ bool VaapiWrapper::CreateContext(const gfx::Size& size) { ...@@ -1879,25 +1862,6 @@ bool VaapiWrapper::CreateContext(const gfx::Size& size) {
// vpp, just passing 0x0. // vpp, just passing 0x0.
const int flag = mode_ != kVideoProcess ? VA_PROGRESSIVE : 0x0; const int flag = mode_ != kVideoProcess ? VA_PROGRESSIVE : 0x0;
const gfx::Size picture_size = mode_ != kVideoProcess ? size : gfx::Size(); const gfx::Size picture_size = mode_ != kVideoProcess ? size : gfx::Size();
if (mode_ != kVideoProcess) {
const VASupportedProfiles::ProfileInfo* profile_info =
VASupportedProfiles::Get().IsProfileSupported(mode_, va_profile_,
va_entrypoint_);
DCHECK(profile_info);
const bool is_picture_within_bounds =
gfx::Rect(picture_size)
.Contains(gfx::Rect(profile_info->min_resolution)) &&
gfx::Rect(profile_info->max_resolution)
.Contains(gfx::Rect(picture_size));
if (!is_picture_within_bounds) {
VLOG(2) << "Requested resolution=" << picture_size.ToString()
<< " is not within bounds ["
<< profile_info->min_resolution.ToString() << ", "
<< profile_info->max_resolution.ToString() << "]";
return false;
}
}
VAStatus va_res = vaCreateContext( VAStatus va_res = vaCreateContext(
va_display_, va_config_id_, picture_size.width(), picture_size.height(), va_display_, va_config_id_, picture_size.width(), picture_size.height(),
flag, empty_va_surfaces_ids_pointer, empty_va_surfaces_ids_size, flag, empty_va_surfaces_ids_pointer, empty_va_surfaces_ids_size,
......
...@@ -238,10 +238,7 @@ std::unique_ptr<base::test::ScopedFeatureList> CreateScopedFeatureList() { ...@@ -238,10 +238,7 @@ std::unique_ptr<base::test::ScopedFeatureList> CreateScopedFeatureList() {
media::kVaapiLowPowerEncoderGen9x, media::kVaapiLowPowerEncoderGen9x,
// TODO(crbug.com/811912): remove once enabled by default. // TODO(crbug.com/811912): remove once enabled by default.
media::kVaapiVP9Encoder}; media::kVaapiVP9Encoder};
std::vector<base::Feature> disabled_features = { scoped_feature_list->InitWithFeatures(enabled_features, {});
media::kVaapiEnforceVideoMinMaxResolution,
};
scoped_feature_list->InitWithFeatures(enabled_features, disabled_features);
return scoped_feature_list; return scoped_feature_list;
#else #else
return nullptr; return nullptr;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment