[FFmpeg-devel] [PATCH] avcodec/wrapped_avframe: allocate a buffer with padding

[Marton Balint]

On Wed, 22 Feb 2017, wm4 wrote:

> On Wed, 22 Feb 2017 00:14:32 +0100
> Marton Balint <cus at passwd.hu> wrote:
>
>> This ensures that the wrapped avframe will not get reallocated later, which
>> would invalidate internal references such as extended data.
>> 
>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> ---
>>  libavcodec/wrapped_avframe.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c
>> index 13c8d8a..1436032 100644
>> --- a/libavcodec/wrapped_avframe.c
>> +++ b/libavcodec/wrapped_avframe.c
>> @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt,
>>                       const AVFrame *frame, int *got_packet)
>>  {
>>      AVFrame *wrapped = av_frame_clone(frame);
>> +    uint8_t *data;
>> +    int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE;
>>
>>      if (!wrapped)
>>          return AVERROR(ENOMEM);
>> 
>> -    pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped),
>> +    data = av_mallocz(size);
>> +    if (!data) {
>> +        av_frame_free(&wrapped);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    pkt->buf = av_buffer_create(data, size,
>>                                  wrapped_avframe_release_buffer, NULL,
>>                                  AV_BUFFER_FLAG_READONLY);
>>      if (!pkt->buf) {
>>          av_frame_free(&wrapped);
>> +        av_freep(&data);
>>          return AVERROR(ENOMEM);
>>      }
>> 
>> -    pkt->data = (uint8_t *)wrapped;
>> +    av_frame_move_ref((AVFrame*)data, wrapped);
>> +    av_frame_free(&wrapped);
>
> I think this could be done as just
>
>  av_frame_ref((AVFrame*)data, frame);
>
> without calling av_frame_clone(), but it doesn't hurt either. (Changing
> it might be an optional, minor improvement.)

You are right, yet I used av_frame_move_ref, because it overwrites the 
contents of AVFrame directly, so it does not matter that I allocated it 
with mallocz instead of av_frame_alloc, and av_frame_ref docs explicitly 
requires an unref-ed or alloc-ed AVFrame. So this seemed more safe, even 
if it involves a few more code lines.

>
>> +
>> +    pkt->data = data;
>>      pkt->size = sizeof(*wrapped);
>>
>>      pkt->flags |= AV_PKT_FLAG_KEY;
>
> Patch looks good, and I like it better than checking the codec ID.

Ok, will apply soon.

Thanks,
Marton