Re: [FFmpeg-devel] [PATCH v1 1/7] lavc/vaapi_dec: Create VA parameters dynamically

[Mark Thompson <sw@jkqxz.net>]

On 28/03/2024 01:26, fei.w.wang-at-intel.com@ffmpeg.org wrote:
> From: Fei Wang <fei.w.wang@intel.com>
> 
> Signed-off-by: Fei Wang <fei.w.wang@intel.com>
> ---
>   libavcodec/vaapi_decode.c | 29 ++++++++++++++++++++++-------
>   libavcodec/vaapi_decode.h |  7 ++-----
>   2 files changed, 24 insertions(+), 12 deletions(-)

This is because the VVC code is going to want to make a lot more of these param buffers - can we just set a slightly larger fixed limit?

If you always need 20 buffers (say), then this has turned 1 allocation per picture into 3 and used more memory in the non-VVC case as well because of the overhead of that (but if you might variably need up to 200 then this is completely fair).

> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> index cca94b5336..1b1972a2a9 100644
> --- a/libavcodec/vaapi_decode.c
> +++ b/libavcodec/vaapi_decode.c
> @@ -38,12 +38,23 @@ int ff_vaapi_decode_make_param_buffer(AVCodecContext *avctx,
>   {
>       VAAPIDecodeContext *ctx = avctx->internal->hwaccel_priv_data;
>       VAStatus vas;
> -    VABufferID buffer;
>   
> -    av_assert0(pic->nb_param_buffers + 1 <= MAX_PARAM_BUFFERS);
> +    av_assert0(pic->nb_param_buffers <= pic->param_allocated);
> +    if (pic->nb_param_buffers == pic->param_allocated) {
> +        pic->param_buffers =
> +            av_realloc_array(pic->param_buffers,
> +                             pic->param_allocated + 16,
> +                             sizeof(*pic->param_buffers));

Use av_reallocp_array() to avoid leaking the pointer on failure.

> +        if (!pic->param_buffers)
> +            return AVERROR(ENOMEM);
> +
> +        pic->param_allocated += 16;
> +    }
> +    av_assert0(pic->nb_param_buffers + 1 <= pic->param_allocated);
>   
>       vas = vaCreateBuffer(ctx->hwctx->display, ctx->va_context,
> -                         type, size, 1, (void*)data, &buffer);
> +                         type, size, 1, (void*)data,
> +                         &pic->param_buffers[pic->nb_param_buffers]);
>       if (vas != VA_STATUS_SUCCESS) {
>           av_log(avctx, AV_LOG_ERROR, "Failed to create parameter "
>                  "buffer (type %d): %d (%s).\n",
> @@ -51,14 +62,14 @@ int ff_vaapi_decode_make_param_buffer(AVCodecContext *avctx,
>           return AVERROR(EIO);
>       }
>   
> -    pic->param_buffers[pic->nb_param_buffers++] = buffer;
> -
>       av_log(avctx, AV_LOG_DEBUG, "Param buffer (type %d, %zu bytes) "
> -           "is %#x.\n", type, size, buffer);
> +           "is %#x.\n", type, size, pic->param_buffers[pic->nb_param_buffers]);
> +
> +    ++pic->nb_param_buffers;
> +
>       return 0;
>   }
>   
> -
>   int ff_vaapi_decode_make_slice_buffer(AVCodecContext *avctx,
>                                         VAAPIDecodePicture *pic,
>                                         const void *params_data,
> @@ -215,6 +226,8 @@ fail:
>   fail_at_end:
>   exit:
>       pic->nb_param_buffers = 0;
> +    pic->param_allocated  = 0;
> +    av_freep(&pic->param_buffers);
>       pic->nb_slices        = 0;
>       pic->slices_allocated = 0;
>       av_freep(&pic->slice_buffers);
> @@ -228,6 +241,8 @@ int ff_vaapi_decode_cancel(AVCodecContext *avctx,
>       ff_vaapi_decode_destroy_buffers(avctx, pic);
>   
>       pic->nb_param_buffers = 0;
> +    pic->param_allocated  = 0;
> +    av_freep(&pic->param_buffers);
>       pic->nb_slices        = 0;
>       pic->slices_allocated = 0;
>       av_freep(&pic->slice_buffers);
> diff --git a/libavcodec/vaapi_decode.h b/libavcodec/vaapi_decode.h
> index 6beda14e52..a41d7ff2ff 100644
> --- a/libavcodec/vaapi_decode.h
> +++ b/libavcodec/vaapi_decode.h
> @@ -32,15 +32,12 @@ static inline VASurfaceID ff_vaapi_get_surface_id(AVFrame *pic)
>       return (uintptr_t)pic->data[3];
>   }
>   
> -enum {
> -    MAX_PARAM_BUFFERS = 16,
> -};
> -
>   typedef struct VAAPIDecodePicture {
>       VASurfaceID           output_surface;
>   
>       int                nb_param_buffers;
> -    VABufferID            param_buffers[MAX_PARAM_BUFFERS];
> +    VABufferID           *param_buffers;

Previously the array was zeroed at allocation but now it isn't.  Can you confirm that that isn't a problem?

> +    int                   param_allocated;

Maybe "nb_param_buffers_allocated" would be clearer.

>   
>       int                nb_slices;
>       VABufferID           *slice_buffers;

Thanks,

- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".