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

["Wang, Fei W" <fei.w.wang-at-intel.com@ffmpeg.org>]

On Mon, 2024-04-01 at 20:52 +0100, Mark Thompson wrote:
> 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).

VVC support 1000 slices and 990 tile columns, and other buffer like
APS/tile_rows... So there will be 1990+ buffers needed at most in
theory.

For other non-VVC case, 16 buffers will be created which is same with
before.

> 
> > 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?

It is should not be a problem, all buffers visited and destroyed
through nb_param_buffers which record if the buffer is valid.

Thanks
Fei

> 
> > +    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".
_______________________________________________
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".