[FFmpeg-devel] [PATCH V2 1/4] lavc/vaapi_encode: Change the slice/parameter buffers to dynamic alloc.
Jun Zhao
mypopydev at gmail.com
Fri Aug 11 10:20:12 EEST 2017
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.zhao at 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.wang at intel.com>
>> Signed-off-by: Jun Zhao <jun.zhao at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list