diff mbox series

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

Message ID 20240328012631.777476-1-fei.w.wang@intel.com
State New
Headers show
Series [FFmpeg-devel,v1,1/7] lavc/vaapi_dec: Create VA parameters dynamically | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Wang, Fei W March 28, 2024, 1:26 a.m. UTC 
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(-)

Comments

Mark Thompson April 1, 2024, 7:52 p.m. UTC | #1 
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
Wang, Fei W April 2, 2024, 6:16 a.m. UTC | #2 
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".
diff mbox series

Patch

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));
+        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;
+    int                   param_allocated;
 
     int                nb_slices;
     VABufferID           *slice_buffers;