[FFmpeg-devel] [PATCH v5 2/2] lavc/vaapi_decode: add support for HWACCEL_CAP_RESET_WITHOUT_UNINIT

[Mark Thompson]

On 14/11/2022 01:16, Fei Wang wrote:
> This can fix vp9 decode image corruption when the frame size is change,
> but the pervious frames still be referenced.
> 
> Surfaces don't need to be bound to vaContext only after VAAPI 1.0.0:
> https://github.com/intel/libva/commit/492b692005ccd0d8da190209d5b3ae7b7825f4b8
> 
> Signed-off-by: Fei Wang <fei.w.wang at intel.com>
> ---
>   libavcodec/vaapi_decode.c | 11 ++++++++---
>   libavcodec/vaapi_decode.h |  1 +
>   libavcodec/vaapi_vp9.c    |  4 ++++
>   3 files changed, 13 insertions(+), 3 deletions(-)

This always segfaults immediately on anything unsupported.  E.g. with fate/hevc/paramchange_yuv420p_yuv420p10.hevc:

[hevc @ 0x555557c0e7c0] Format vaapi chosen by get_format().
[hevc @ 0x555557c0e7c0] Format vaapi requires hwaccel initialisation.
[hevc @ 0x555557c0e7c0] Hardware does not support image size 1056x8440 (constraints: width 0-4096 height 0-4096).

Thread 20 "av:hevc:df0" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffb4ff9700 (LWP 509456)]
ff_vaapi_decode_uninit (avctx=0x555557c0e7c0) at src/libavcodec/vaapi_decode.c:714
714             vas = vaDestroyContext(ctx->hwctx->display, ctx->va_context);
(gdb) bt
#0  ff_vaapi_decode_uninit (avctx=0x555557c0e7c0) at src/libavcodec/vaapi_decode.c:714
#1  0x00005555563073d7 in ff_vaapi_decode_init (avctx=0x555557c0e7c0) at src/libavcodec/vaapi_decode.c:704
#2  0x0000555555e62fee in hwaccel_init (avctx=0x555557c0e7c0, hw_config=0x55555728f770 <__compound_literal.0>) at src/libavcodec/decode.c:1121
#3  0x0000555555e634ec in ff_get_format (avctx=0x555557c0e7c0, fmt=0x7fffb4ff8ccc) at src/libavcodec/decode.c:1261
#4  0x00005555561ca829 in ff_thread_get_format (avctx=0x555557c0e7c0, fmt=0x7fffb4ff8ccc) at src/libavcodec/pthread_frame.c:1048
#5  0x0000555555f68f37 in get_format (s=0x555557c3e6c0, sps=0x555557c21f80) at src/libavcodec/hevcdec.c:505
#6  0x0000555555f69621 in hls_slice_header (s=0x555557c3e6c0) at src/libavcodec/hevcdec.c:618
#7  0x0000555555f7472d in decode_nal_unit (s=0x555557c3e6c0, nal=0x7fff8802e920) at src/libavcodec/hevcdec.c:3173
#8  0x0000555555f7508a in decode_nal_units (s=0x555557c3e6c0, buf=0x7ffff637c010 "", length=159280) at src/libavcodec/hevcdec.c:3355
#9  0x0000555555f756d6 in hevc_decode_frame (avctx=0x555557c0e7c0, rframe=0x555557c0ecc0, got_output=0x555557c0d690, avpkt=0x555557c0ef40) at src/libavcodec/hevcdec.c:3497
#10 0x00005555561c839c in frame_worker_thread (arg=0x555557c0d580) at src/libavcodec/pthread_frame.c:241
#11 0x00007ffff68ccea7 in start_thread (arg=<optimized out>) at pthread_create.c:477
#12 0x00007ffff67ecaef in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb)


Also, I don't see how this is testing whether the driver supports changing the resolution at runtime?  The note in libva that you cite allows new switching render targets in the context, but I don't see why different resolution would be allowed given that it's a parameter passed to vaCreateContext()?

Looking at the Mesa driver it appears that the internally-allocated references are not going to allow size changes (where does the template width get updated?).  I don't have any hardware to test that, though - are you able to try this on recent AMD hardware with VP9 support?

What have you done to verify the METHOD_HW_FRAMES_CTX behaviour?  This has changed so that vaapi_decode_make_config() is no longer called, which feels possibly-bad though I'm unsure of the exact consequences.

As I said last time, I do think you should only do this in exactly the places where it is required, and not change any other behaviour.

Thanks,

- Mark

> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> index 134f10eca5..d950471b6d 100644
> --- a/libavcodec/vaapi_decode.c
> +++ b/libavcodec/vaapi_decode.c
> @@ -658,9 +658,6 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>       VAStatus vas;
>       int err;
>   
> -    ctx->va_config  = VA_INVALID_ID;
> -    ctx->va_context = VA_INVALID_ID;
> -
>       err = ff_decode_get_hw_frames_ctx(avctx, AV_HWDEVICE_TYPE_VAAPI);
>       if (err < 0)
>           goto fail;
> @@ -670,6 +667,12 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>       ctx->device = ctx->frames->device_ctx;
>       ctx->hwctx  = ctx->device->hwctx;
>   
> +    if (ctx->inited)
> +        return 0;
> +
> +    ctx->va_config  = VA_INVALID_ID;
> +    ctx->va_context = VA_INVALID_ID;
> +
>       err = vaapi_decode_make_config(avctx, ctx->frames->device_ref,
>                                      &ctx->va_config, NULL);
>       if (err)
> @@ -691,6 +694,8 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>       av_log(avctx, AV_LOG_DEBUG, "Decode context initialised: "
>              "%#x/%#x.\n", ctx->va_config, ctx->va_context);
>   
> +    ctx->inited = 1;
> +
>       return 0;
>   
>   fail:
> diff --git a/libavcodec/vaapi_decode.h b/libavcodec/vaapi_decode.h
> index 6beda14e52..62a4f37ed9 100644
> --- a/libavcodec/vaapi_decode.h
> +++ b/libavcodec/vaapi_decode.h
> @@ -61,6 +61,7 @@ typedef struct VAAPIDecodeContext {
>       int                   surface_count;
>   
>       VASurfaceAttrib       pixel_format_attribute;
> +    int                   inited;
>   } VAAPIDecodeContext;
>   
>   
> diff --git a/libavcodec/vaapi_vp9.c b/libavcodec/vaapi_vp9.c
> index 776382f683..245b7a1b3a 100644
> --- a/libavcodec/vaapi_vp9.c
> +++ b/libavcodec/vaapi_vp9.c
> @@ -181,5 +181,9 @@ const AVHWAccel ff_vp9_vaapi_hwaccel = {
>       .uninit               = ff_vaapi_decode_uninit,
>       .frame_params         = ff_vaapi_common_frame_params,
>       .priv_data_size       = sizeof(VAAPIDecodeContext),
> +#if VA_CHECK_VERSION(1, 0, 0)
> +    .caps_internal        = HWACCEL_CAP_ASYNC_SAFE | HWACCEL_CAP_RESET_WITHOUT_UNINIT,
> +#else
>       .caps_internal        = HWACCEL_CAP_ASYNC_SAFE,
> +#endif
>   };