[FFmpeg-devel] [PATCH 2/2] lavc/vaapi_decode: recreate hw_frames_ctx without destroy va_context

[Mark Thompson]

On 07/07/2019 17:38, Linjie Fu wrote:
> VP9 allows resolution changes per frame. Currently in VAAPI, resolution
> changes leads to va context destroy and reinit.

Which is correct - it needs to remake the context because the old one is for the wrong resolution.

>                                                 This will cause
> reference frame surface lost and produce garbage.

This isn't right - the reference frame surfaces aren't lost.  See VP9Context.s.refs[], which holds references to the frames (including their hardware frame contexts) until the stream indicates that they can be discarded by overwriting their reference frame slots.

> As libva allows re-create surface separately without changing the
> context, this issue could be handled by only recreating hw_frames_ctx.
> 
> Could be verified by:
> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -i
> ./resolutions.ivf
> -pix_fmt p010le -f rawvideo -vframes 20 -y vaapi.yuv
> 
> Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> ---
>  libavcodec/decode.c       |  8 ++++----
>  libavcodec/vaapi_decode.c | 40 ++++++++++++++++++++++------------------
>  2 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 0863b82..a81b857 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1254,7 +1254,6 @@ int ff_decode_get_hw_frames_ctx(AVCodecContext *avctx,
>  
>      frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
>  
> -
>      if (frames_ctx->initial_pool_size) {
>          // We guarantee 4 base work surfaces. The function above guarantees 1
>          // (the absolute minimum), so add the missing count.
> @@ -1333,7 +1332,7 @@ static int hwaccel_init(AVCodecContext *avctx,
>          return AVERROR_PATCHWELCOME;
>      }
>  
> -    if (hwaccel->priv_data_size) {
> +    if (hwaccel->priv_data_size && !avctx->internal->hwaccel_priv_data) {
>          avctx->internal->hwaccel_priv_data =
>              av_mallocz(hwaccel->priv_data_size);
>          if (!avctx->internal->hwaccel_priv_data)
> @@ -1397,8 +1396,9 @@ int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
>  
>      for (;;) {
>          // Remove the previous hwaccel, if there was one.
> -        hwaccel_uninit(avctx);
> -
> +        // VAAPI allows reinit hw_frames_ctx only
> +        if (choices[0] != AV_PIX_FMT_VAAPI_VLD)

Including codec-specific conditions in the generic code suggests that something very shady is going on.

> +            hwaccel_uninit(avctx);>          user_choice = avctx->get_format(avctx, choices);
>          if (user_choice == AV_PIX_FMT_NONE) {
>              // Explicitly chose nothing, give up.
> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> index 69512e1..13f0cf0 100644
> --- a/libavcodec/vaapi_decode.c
> +++ b/libavcodec/vaapi_decode.c
> @@ -613,8 +613,10 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>      VAStatus vas;
>      int err;
>  
> -    ctx->va_config  = VA_INVALID_ID;
> -    ctx->va_context = VA_INVALID_ID;
> +    if (!ctx->va_config && !ctx->va_context){
> +        ctx->va_config  = VA_INVALID_ID;
> +        ctx->va_context = VA_INVALID_ID;
> +    }
>  
>  #if FF_API_STRUCT_VAAPI_CONTEXT
>      if (avctx->hwaccel_context) {
> @@ -642,7 +644,6 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>          // present, so set it here to avoid the behaviour changing.
>          ctx->hwctx->driver_quirks =
>              AV_VAAPI_DRIVER_QUIRK_RENDER_PARAM_BUFFERS;
> -
>      }
>  #endif
>  
> @@ -655,7 +656,8 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>                 "context: %#x/%#x.\n", ctx->va_config, ctx->va_context);
>      } else {
>  #endif
> -
> +    // Get a new hw_frames_ctx even if there is already one
> +    // recreate surface without reconstuct va_context
>      err = ff_decode_get_hw_frames_ctx(avctx, AV_HWDEVICE_TYPE_VAAPI);
>      if (err < 0)
>          goto fail;
> @@ -670,21 +672,23 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
>      if (err)
>          goto fail;
>  
> -    vas = vaCreateContext(ctx->hwctx->display, ctx->va_config,
> -                          avctx->coded_width, avctx->coded_height,
> -                          VA_PROGRESSIVE,
> -                          ctx->hwfc->surface_ids,
> -                          ctx->hwfc->nb_surfaces,
> -                          &ctx->va_context);
> -    if (vas != VA_STATUS_SUCCESS) {
> -        av_log(avctx, AV_LOG_ERROR, "Failed to create decode "
> -               "context: %d (%s).\n", vas, vaErrorStr(vas));
> -        err = AVERROR(EIO);
> -        goto fail;
> -    }
> +    if (ctx->va_context == VA_INVALID_ID) {
> +        vas = vaCreateContext(ctx->hwctx->display, ctx->va_config,
> +                              avctx->coded_width, avctx->coded_height,
> +                              VA_PROGRESSIVE,
> +                              ctx->hwfc->surface_ids,
> +                              ctx->hwfc->nb_surfaces,
> +                              &ctx->va_context);
> +        if (vas != VA_STATUS_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to create decode "
> +                   "context: %d (%s).\n", vas, vaErrorStr(vas));
> +            err = AVERROR(EIO);
> +            goto fail;
> +        }
>  
> -    av_log(avctx, AV_LOG_DEBUG, "Decode context initialised: "
> -           "%#x/%#x.\n", ctx->va_config, ctx->va_context);
> +        av_log(avctx, AV_LOG_DEBUG, "Decode context initialised: "
> +               "%#x/%#x.\n", ctx->va_config, ctx->va_context);
> +    }

If I'm reading this correctly, this changes to only creating one context ever even when the resolution changes.

How can that work if the resolution increases?  For example you start decoding at 1280x720 and create a context for that, then the resolution changes to 3840x2160 and it tries to decode using the 1280x720 context.

>  #if FF_API_STRUCT_VAAPI_CONTEXT
>      }
>  #endif
> 

- Mark