[FFmpeg-devel] lavu/hwcontext_vaapi: cope with race map for YUV420P

There is a race condition for AV_PIX_FMT_YUV420P when mapping from pix_fmt
to fourcc, both VA_FOURCC_I420 and VA_FOURCC_YV12 could be found by pix_fmt.

Currently, vaapi_get_image_format will go through the query results of
pix_fmt and returned the first matched result according to the declared
order in driver.This may leads to a wrong image_format.

Modify to find image_format via fourcc.

Fix vaapi CSC to I420:
ffmpeg -hwaccel vaapi -vaapi_device /dev/dri/renderD128 -f rawvideo
-pix_fmt nv12 -s:v 1280x720 -i NV12.yuv -vf
'format=nv12,hwupload,scale_vaapi=format=yuv420p,hwdownload,format=yuv420p'
-f rawvideo -vsync passthrough -vframes 10 -y aa.yuv

Reviewed-by: Zhong Li <zhong.li@intel.com>
Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavutil/hwcontext_vaapi.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

On 15/01/2020 06:55, Linjie Fu wrote:
> There is a race condition for AV_PIX_FMT_YUV420P when mapping from pix_fmt
> to fourcc, both VA_FOURCC_I420 and VA_FOURCC_YV12 could be found by pix_fmt.

The title and this comment are very confusing.  As far as I can see this has nothing to do with a race condition, since only one thread is ever involved.

> Currently, vaapi_get_image_format will go through the query results of
> pix_fmt and returned the first matched result according to the declared
> order in driver.This may leads to a wrong image_format.
> 
> Modify to find image_format via fourcc.
> 
> Fix vaapi CSC to I420:
> ffmpeg -hwaccel vaapi -vaapi_device /dev/dri/renderD128 -f rawvideo
> -pix_fmt nv12 -s:v 1280x720 -i NV12.yuv -vf
> 'format=nv12,hwupload,scale_vaapi=format=yuv420p,hwdownload,format=yuv420p'
> -f rawvideo -vsync passthrough -vframes 10 -y aa.yuv
> 
> Reviewed-by: Zhong Li <zhong.li@intel.com>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavutil/hwcontext_vaapi.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index cd215a0..199f425 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -63,6 +63,7 @@ typedef struct VAAPIDevicePriv {
>  typedef struct VAAPISurfaceFormat {
>      enum AVPixelFormat pix_fmt;
>      VAImageFormat image_format;
> +    uint32_t fourcc;
>  } VAAPISurfaceFormat;
>  
>  typedef struct VAAPIDeviceContext {
> @@ -178,15 +179,21 @@ static int vaapi_get_image_format(AVHWDeviceContext *hwdev,
>                                    VAImageFormat **image_format)
>  {
>      VAAPIDeviceContext *ctx = hwdev->internal->priv;
> +    VAAPIFormatDescriptor *desc;
>      int i;
>  
> +    desc = vaapi_format_from_pix_fmt(pix_fmt);

This would now only find the first entry in the map, so YV12 and YV16 could never work, nor could the IYUV alias for old drivers.

The point of this function is that it finds the first matching format supported by the driver being used, but I can see that that goes wrong when a driver declares support for the same format under multiple different names which are not interchangable.

What conflicting formats are actually being advertised in the broken case?

> +    if (!desc || !image_format)
> +        goto fail;
> +
>      for (i = 0; i < ctx->nb_formats; i++) {
> -        if (ctx->formats[i].pix_fmt == pix_fmt) {
> -            if (image_format)
> -                *image_format = &ctx->formats[i].image_format;
> +        if (ctx->formats[i].fourcc == desc->fourcc) {
> +            *image_format = &ctx->formats[i].image_format;
>              return 0;
>          }
>      }
> +
> +fail:
>      return AVERROR(EINVAL);
>  }
>  
> @@ -375,6 +382,7 @@ static int vaapi_device_init(AVHWDeviceContext *hwdev)
>              av_log(hwdev, AV_LOG_DEBUG, "Format %#x -> %s.\n",
>                     fourcc, av_get_pix_fmt_name(pix_fmt));
>              ctx->formats[ctx->nb_formats].pix_fmt      = pix_fmt;
> +            ctx->formats[ctx->nb_formats].fourcc       = fourcc;
>              ctx->formats[ctx->nb_formats].image_format = image_list[i];
>              ++ctx->nb_formats;
>          }
> 

Thanks,

- Mark