[FFmpeg-devel] avcodec/mpegvideo_enc: fix memory leak

When the input frames contain side data, it will accumulate endlessly in
the coded frame, as av_frame_copy_props will append any new side data.
---
 libavcodec/mpegvideo_enc.c | 1 +
 1 file changed, 1 insertion(+)

2016-09-25 15:43 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
> When the input frames contain side data, it will accumulate endlessly in
> the coded frame, as av_frame_copy_props will append any new side data.

But there is no leak, no?
Is this related to ticket #5799?

Carl Eugen

On 9/25/2016 3:44 PM, Carl Eugen Hoyos wrote:
> 2016-09-25 15:43 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
>> When the input frames contain side data, it will accumulate endlessly in
>> the coded frame, as av_frame_copy_props will append any new side data.
> 
> But there is no leak, no?
> Is this related to ticket #5799?

This fixes that ticket for me, yes.
And without this, ffmpeg keeps using more and more memory.
So while the memory is eventually freed when the encoding is done, it
still seems like a leak to me.

2016-09-25 15:47 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
> On 9/25/2016 3:44 PM, Carl Eugen Hoyos wrote:
>> 2016-09-25 15:43 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
>>> When the input frames contain side data, it will accumulate
>>> endlessly in the coded frame, as av_frame_copy_props will
>>> append any new side data.
>>
>> But there is no leak, no?
>> Is this related to ticket #5799?
>
> This fixes that ticket for me, yes.

Please mention this in the commit message.

> And without this, ffmpeg keeps using more and more memory.
> So while the memory is eventually freed when the encoding is done

I believe this defines it as not being a leak.

Thank you for fixing this, Carl Eugen

On 9/25/2016 3:55 PM, Carl Eugen Hoyos wrote:
> 2016-09-25 15:47 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
>> On 9/25/2016 3:44 PM, Carl Eugen Hoyos wrote:
>>> 2016-09-25 15:43 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:
>>>> When the input frames contain side data, it will accumulate
>>>> endlessly in the coded frame, as av_frame_copy_props will
>>>> append any new side data.
>>>
>>> But there is no leak, no?
>>> Is this related to ticket #5799?
>>
>> This fixes that ticket for me, yes.
> 
> Please mention this in the commit message.
> 

Added locally.

>> And without this, ffmpeg keeps using more and more memory.
>> So while the memory is eventually freed when the encoding is done
> 
> I believe this defines it as not being a leak.

What else would it be? I think speaking of a leak here is still valid.

> Thank you for fixing this, Carl Eugen

Le quartidi 4 vendémiaire, an CCXXV, Carl Eugen Hoyos a écrit :
> > And without this, ffmpeg keeps using more and more memory.
> > So while the memory is eventually freed when the encoding is done
> I believe this defines it as not being a leak.

I believe you are wrong. It is not the same kind of leaks than usual, and
not the kind of leak that valgrind can detect, but any repeated allocations
that are not freed in a timely fashion when they are not needed anymore
constitute a leak.

Regards,

2016-09-25 15:59 GMT+02:00 Timo Rothenpieler <timo@rothenpieler.org>:

>>> And without this, ffmpeg keeps using more and more memory.
>>> So while the memory is eventually freed when the encoding is done
>>
>> I believe this defines it as not being a leak.
>
> What else would it be?

(Massive) memory consumption.

Carl Eugen

2016-09-25 16:00 GMT+02:00 Nicolas George <george@nsup.org>:
> Le quartidi 4 vendémiaire, an CCXXV, Carl Eugen Hoyos a écrit :
>> > And without this, ffmpeg keeps using more and more memory.
>> > So while the memory is eventually freed when the encoding is done
>> I believe this defines it as not being a leak.
>
> I believe you are wrong. It is not the same kind of leaks than usual, and
> not the kind of leak that valgrind can detect, but any repeated allocations
> that are not freed in a timely fashion when they are not needed anymore
> constitute a leak.

Fine with me.
(I am just surprised that other developers have often used
another definition on our various bug trackers that makes
much more sense to me.)

Carl Eugen

Le quartidi 4 vendémiaire, an CCXXV, Carl Eugen Hoyos a écrit :
> (Massive) memory consumption.

That would be if the consumption was necessary.

Regards,

> When the input frames contain side data, it will accumulate endlessly in
> the coded frame, as av_frame_copy_props will append any new side data.
> ---
>  libavcodec/mpegvideo_enc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> index 87d7954..5cd654f 100644
> --- a/libavcodec/mpegvideo_enc.c
> +++ b/libavcodec/mpegvideo_enc.c
> @@ -1735,6 +1735,7 @@ static void frame_end(MpegEncContext *s)
>  
>  #if FF_API_CODED_FRAME
>  FF_DISABLE_DEPRECATION_WARNINGS
> +    av_frame_unref(s->avctx->coded_frame);
>      av_frame_copy_props(s->avctx->coded_frame, s->current_picture.f);
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
> 

Will push later today, unless someone has a better idea how to address
this issue.
Maybe some way to not use av_frame_copy_props in the first place?

> Will push later today, unless someone has a better idea how to address
> this issue.
> Maybe some way to not use av_frame_copy_props in the first place?

applied