On 2017/8/10 5:11, Mark Thompson wrote: > On 02/08/17 06:53, Jun Zhao wrote: >> V2: Change the slice/parameter buffers to dynamic alloc and split >> the mutil-slice support for AVC/HEVC. >> >> From 39fd7852df0c96217310c525107da06a7ec0a062 Mon Sep 17 00:00:00 2001 >> From: Jun Zhao <jun.z...@intel.com> >> Date: Mon, 31 Jul 2017 22:46:23 -0400 >> Subject: [PATCH V2 1/4] lavc/vaapi_encode: Change the slice/parameter buffers >> to dynamic alloc. >> >> Change the slice/parameter buffers to be allocated dynamically. >> >> Signed-off-by: Wang, Yi A <yi.a.w...@intel.com> >> Signed-off-by: Jun Zhao <jun.z...@intel.com> >> --- >> libavcodec/vaapi_encode.c | 27 ++++++++++++++++++++++++--- >> libavcodec/vaapi_encode.h | 6 ++---- >> 2 files changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c >> index 2de5f76cab..11d9803b5d 100644 >> --- a/libavcodec/vaapi_encode.c >> +++ b/libavcodec/vaapi_encode.c >> @@ -36,13 +36,19 @@ static int >> vaapi_encode_make_packed_header(AVCodecContext *avctx, >> VAAPIEncodeContext *ctx = avctx->priv_data; >> VAStatus vas; >> VABufferID param_buffer, data_buffer; >> + VABufferID *tmp; >> VAEncPackedHeaderParameterBuffer params = { >> .type = type, >> .bit_length = bit_len, >> .has_emulation_bytes = 1, >> }; >> >> - av_assert0(pic->nb_param_buffers + 2 <= MAX_PARAM_BUFFERS); >> + tmp = av_realloc_array(pic->param_buffers, sizeof(*tmp), >> (pic->nb_param_buffers + 2)); > > Redundant parentheses? > >> + if (!tmp) { >> + av_freep(&pic->param_buffers); > > This leaks the actual buffers which were allocated > (pic->param_buffers[0..pic->nb_param_buffers-1]).
After double-check the code, I think if av_realloc_array fail in vaapi_encode_make_packed_header/vaapi_encode_make_param_buffer, we can just return error number ENOMEM, then vaapi_encode_issue will free the VA buffer/pic->param_buffers/...in error handling. > >> + return AVERROR(ENOMEM); >> + } >> + pic->param_buffers = tmp; >> >> vas = vaCreateBuffer(ctx->hwctx->display, ctx->va_context, >> VAEncPackedHeaderParameterBufferType, >> @@ -77,9 +83,15 @@ static int vaapi_encode_make_param_buffer(AVCodecContext >> *avctx, >> { >> VAAPIEncodeContext *ctx = avctx->priv_data; >> VAStatus vas; >> + VABufferID *tmp; >> VABufferID buffer; >> >> - av_assert0(pic->nb_param_buffers + 1 <= MAX_PARAM_BUFFERS); >> + tmp = av_realloc_array(pic->param_buffers, sizeof(*tmp), >> (pic->nb_param_buffers + 1)); >> + if (!tmp) { >> + av_freep(&pic->param_buffers); > > Same here. > >> + return AVERROR(ENOMEM); >> + } >> + pic->param_buffers = tmp; >> >> vas = vaCreateBuffer(ctx->hwctx->display, ctx->va_context, >> type, len, 1, data, &buffer); >> @@ -122,6 +134,8 @@ static int vaapi_encode_wait(AVCodecContext *avctx, >> // Input is definitely finished with now. >> av_frame_free(&pic->input_image); >> >> + av_freep(&pic->param_buffers); > > Maybe it would be cleaner to free param_buffers at the end of > vaapi_encode_issue() - the buffers themselves are no longer usable at that > point. Ok > >> + >> pic->encode_complete = 1; >> return 0; >> } >> @@ -313,7 +327,10 @@ static int vaapi_encode_issue(AVCodecContext *avctx, >> } >> } >> >> - av_assert0(pic->nb_slices <= MAX_PICTURE_SLICES); >> + pic->slices = (VAAPIEncodeSlice **)av_malloc(sizeof(VAAPIEncodeSlice *) >> * pic->nb_slices); >> + if (pic->slices == NULL) >> + goto fail; >> + >> for (i = 0; i < pic->nb_slices; i++) { >> slice = av_mallocz(sizeof(*slice)); > > How about just making pic->slices be VAAPIEncodeSlice*, pointing to an array > of the structures? This code is now allocating both an array of pointers and > then each element of that array individually. > >> if (!slice) { >> @@ -427,6 +444,8 @@ fail: >> vaDestroyBuffer(ctx->hwctx->display, pic->param_buffers[i]); >> fail_at_end: >> av_freep(&pic->codec_picture_params); >> + av_freep(&pic->param_buffers); >> + av_freep(&pic->slices); >> av_frame_free(&pic->recon_image); >> av_buffer_unref(&pic->output_buffer_ref); >> pic->output_buffer = VA_INVALID_ID; >> @@ -544,6 +563,8 @@ static int vaapi_encode_free(AVCodecContext *avctx, >> av_frame_free(&pic->input_image); >> av_frame_free(&pic->recon_image); >> >> + av_freep(&pic->param_buffers); >> + av_freep(&pic->slices); >> // Output buffer should already be destroyed. >> av_assert0(pic->output_buffer == VA_INVALID_ID); >> >> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h >> index 0edf27e4cb..b542772aed 100644 >> --- a/libavcodec/vaapi_encode.h >> +++ b/libavcodec/vaapi_encode.h >> @@ -35,8 +35,6 @@ enum { >> MAX_CONFIG_ATTRIBUTES = 4, >> MAX_GLOBAL_PARAMS = 4, >> MAX_PICTURE_REFERENCES = 2, >> - MAX_PICTURE_SLICES = 112, >> - MAX_PARAM_BUFFERS = 128, >> MAX_REORDER_DELAY = 16, >> MAX_PARAM_BUFFER_SIZE = 1024, >> }; >> @@ -73,7 +71,7 @@ typedef struct VAAPIEncodePicture { >> VASurfaceID recon_surface; >> >> int nb_param_buffers; >> - VABufferID param_buffers[MAX_PARAM_BUFFERS]; >> + VABufferID *param_buffers; >> >> AVBufferRef *output_buffer_ref; >> VABufferID output_buffer; >> @@ -85,7 +83,7 @@ typedef struct VAAPIEncodePicture { >> struct VAAPIEncodePicture *refs[MAX_PICTURE_REFERENCES]; >> >> int nb_slices; >> - VAAPIEncodeSlice *slices[MAX_PICTURE_SLICES]; >> + VAAPIEncodeSlice **slices; >> } VAAPIEncodePicture; >> >> typedef struct VAAPIEncodeContext { >> -- >> 2.11.0 >> > > Actual aim of patch looks fine to me. > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel