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