[FFmpeg-devel] [PATCH 4/6] lavu/hwcontext_vaapi: add vaapi_format_map support for AYUV/Y210/Y410

[Mark Thompson]

On 24/09/2019 04:21, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Mark Thompson
>> Sent: Friday, September 13, 2019 07:48
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 4/6] lavu/hwcontext_vaapi: add
>> vaapi_format_map support for AYUV/Y210/Y410
>>
>> On 10/09/2019 17:07, Linjie Fu wrote:
>>> There is no VA_RT_FORMAT_AYUV in defined in libva, and currently in
>>> media-driver, VA_FOURCC_AYUV is used to represent
>> VA_RT_FORMAT_AYUV.
>>
>> That doesn't make sense - VA_RT_FORMAT_* is a bit mask, so using
>> VA_FOURCC_AYUV looks like a random combination of other values.
>>>
>>> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
>>> index cf11764..724bbeb 100644
>>> --- a/libavutil/hwcontext_vaapi.c
>>> +++ b/libavutil/hwcontext_vaapi.c
>>> @@ -116,6 +116,15 @@ static const VAAPIFormatDescriptor
>> vaapi_format_map[] = {
>>>  #endif
>>>      MAP(UYVY, YUV422,  UYVY422, 0),
>>>      MAP(YUY2, YUV422,  YUYV422, 0),
>>> +#ifdef VA_FOURCC_Y210
>>> +    MAP(Y210, YUV422_10,Y210, 0),
>>> +#endif
>>> +#define VA_RT_FORMAT_AYUV VA_FOURCC_AYUV
>>> +    MAP(AYUV,   AYUV,     AYUV, 0),
>>> +#undef VA_RT_FORMAT_AYUV
>>> +#ifdef VA_FOURCC_Y410
>>> +    MAP(Y410, YUV444_10,Y410, 0),
>>
>> That looks suspicious - you've defined Y410 as having an alpha channel, but
>> that render target format doesn't have one.
>>
> 
> Reconsider about these concerns and collect some information in driver.
> 
> 1. VA_RT_FORMAT describes the sampling format, several fourcc may have the
> Same VA_RT_FORMAT. (NV12, YV12, I420 have VA_RT_FORMAT_YUV420) 
> so YUV444 is good for AYUV, and YUV444_10 is good for Y410. 
> (alpha channel is not relevant with the sampling format)
> 
> 2. In media driver,  mediaFMT is actually used for decode which is decided by expected_fourcc.
> DDI_MEDIA_FORMAT mediaFmt = DdiMedia_OsFormatToMediaFormat(expected_fourcc,format);
> https://github.com/intel/media-driver/blob/cf80fa0e3c81361696ada1285ea17a417114be47/media_driver/linux/common/ddi/media_libva.cpp#L2222
> 
> No declaring for a RT_FORMAT with alpha channel won't do harm to the decode procedure since mediaFMT
> is actually mapped by FourCC.
> 
> 3. in CreateDecAttributes and MediaLibvaCaps::CheckEncRTFormat,
> VA_RT_FORMAT_YUV444 and VA_RT_FORMAT_YUV444_10 are used for MAIN444 and MAIN444_10
> 
> https://github.com/intel/media-driver/blob/cf80fa0e3c81361696ada1285ea17a417114be47/media_driver/linux/gen11/ddi/media_libva_caps_g11.cpp#L1189
> 
> https://github.com/intel/media-driver/blob/cf80fa0e3c81361696ada1285ea17a417114be47/media_driver/linux/common/ddi/media_libva_caps.cpp#L350
> 
> 3. If no against, I would like to refine the patch for AYUV and Y410 as follow, and resend the patch.
> +    MAP(AYUV,   YUV444,     AYUV, 0),
> +#ifdef VA_FOURCC_Y410
> +    MAP(Y410, YUV444_10,  Y410, 0),
> +#endif

That render target format makes more sense to me.

However, the confusion about the presence of alpha channels feels like a bad idea - you're defining formats with alpha channels, then selectively ignoring the alpha channel in some places but not others.

>From what you've said so far, I get the impression that the alpha channel in your AYUV and Y410 formats is not actually supported at all in the hardware you have?  (That is, there is no support for decoding video with an alpha channel, such as two-layer H.265.)

If that's true, I think you should be defining a 0YUV format and using that everywhere rather than AYUV.  Declaring that it contains an alpha channel when it doesn't can only cause confusion later, especially around automatic conversions and when combining video.

- Mark