[FFmpeg-devel] [PATCH] Fix for rc_eq memory leak

[Art Clarke]

On Sat, Aug 2, 2008 at 4:46 AM, Stefano Sabatini <
stefano.sabatini-lala at poste.it> wrote:

> On date Thursday 2008-07-31 11:21:45 -0700, Art Clarke encoded:
> > Stefano submitted a fix for the rc_eq setting bug earlier this month:
> > http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2008-July/049805.html
> >
> > As he pointed out, this introduced a memory leak on
> AVFormatContext:rc_eq.
> >
> > This patch fixes one cause of leaking this in the libav* libraries (for
> well
> > behaved applications): closing an input stream.
> >
> > There isn't an easy fix for programs that allocate their own
> AVFormatContext
> > objects (e.g. for output).  Folks will have to fix their sources for
> that.
> >
> > Tests with both ffmpeg and vlideshow pass @ 100%, and leak is visible
> before
> > fix and not after fix.
> >
> > - Art
>
> > Index: libavformat/utils.c
> > ===================================================================
> > --- libavformat/utils.c       (revision 14487)
> > +++ libavformat/utils.c       (working copy)
> > @@ -2212,6 +2212,7 @@
> >          }
> >          av_free(st->index_entries);
> >          av_free(st->codec->extradata);
> > +        av_free(st->codec->rc_eq);
> >          av_free(st->codec);
> >          av_free(st->filename);
> >          av_free(st->priv_data);
>
> No this shouldn't be necessary anymore since r14260, unless you still
> didn't already close all the codecs with avcodec_close(), which I
> think is *required* by the current API.
>
> (BTW currently avcodec_close() doesn't free extradata, I think this is a
> memleak, also it could be a good idea to use av_freep() rather than
> av_free() to save some SIGSEGV.)
>
> For example ffmpeg.c does exactly this, it closes all the decoders
> before to call av_close_input_stream().
>
> Maybe a better solution would be to do call avcodec_close() in
> av_close_input_stream(), at least conditionally, for example:
>
>   if(st->codec->codec)  /* already closed or never opened */
>        avcodec_close(st->codec);
>
> This looks like a better solution because it would simplify ffmpeg.c
> (and others applications as well).
>
> In the case of ffmpeg.c this could eliminate the need for the loop:
>    for(i=0;i<nb_istreams;i++) {
>        ist = ist_table[i];
>        if (ist->decoding_needed) {
>            avcodec_close(ist->st->codec);
>        }
>    }
>
> at the end of av_encode().
>
> Opinions?
>
> Regards.
> --
> FFmpeg = Fostering and Fiendish Mind-dumbing Pitiful Everlasting Gymnast
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>


Good point;  The problem here is that to generate the leak, the test program
doesn't need to open a codec.  Many formats (e.g. FLV) will open a code
behind the scenes when av_find_stream_info(context) is called.

Try compiling the attached test program, and then passing in an FLV file for
input.  You'll see the source code is a well behaved
libav* program, closing up after itself.

That's why I put the change I put in in where I did (using an av_free() call
rather than av_freep to stay with the conventions in that code).  In some
cases the user will have called avcodec_close because they opened the codec,
in which case "rc_eq" will be null and av_free is safe.  If they haven't
called avcodec_close, because they didn't open the codec (e.g.
av_find_stream_info actually opened the codec), then we free it then.

As for the memory leak in avcodec_close on 'extradata', it does look like
you're right, but I don't have a test case that shows it (hence I'm not
patching that ) :)  If anyone can suggest a good test case, and or provide a
file, I'll add it to our test suite and create a patch.

Given that argument, are you OK with the patch?

- Art
-------------- next part --------------
A non-text attachment was scrubbed...
Name: FfmpegLeak.c
Type: text/x-csrc
Size: 914 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080804/d0531421/attachment.c>