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

[Stefano Sabatini]

On date Monday 2008-08-04 11:42:02 -0700, Art Clarke encoded:
> 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:
[...]
> > > 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?
[...]
> 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?

I continue to insist with a different solution, according to me we
should close the codecs even jsut after we call
av_input_find_stream_info(), having duplicated the same code in
avcodec_close() and av_close_input_stream() doesn't look like a
particularly good idea.

Said that I'm not a maintainer of anything so I can just express my
opinion but I have no final word.

As for an alternative solution, I started to hack a solution, then I
got stuck with some extradata related SIGSEGVs, so I left it apart,
I'll hopefully come back to it in some time.

> - Art

> #include <stdio.h>
> #include <libavformat/avformat.h>
> 
> int
> main(int argc, char**argv)
> {
>   AVFormatContext* context=0;
>   int retval = -1;
>   const char* filename = 0;
> 
>   /*
>   int blah;
>   if (blah == 2)
>   {
>     printf("this should give a valgrind error");
>   }
>   */
> 
>   if (argc != 2 || !argv[1] || !*argv[1])
>   {
>     printf("no input file specified\n");
>     return -1;
>   }
>   filename = argv[1];
> 
>   av_register_all();
> 
>   retval = av_open_input_file(&context, filename, NULL, 0, NULL);
>   if (retval < 0)
>   {
>     printf("cannot open input file: %s\n", filename);
>     return -1;
>   }
> 
>   retval = av_find_stream_info(context);
>   if (retval < 0)
>   {
>     printf("cannot find information about streams in: %s\n",
>         filename);
>     return -1;
>   }
> 
>   dump_format(context, 0, filename, 0);
> 
>   av_close_input_file(context);
>   if (retval < 0)
>   {
>     printf("cannot close file: %s\n", filename);
>     return -1;
>   }
>   return 0;
> }

Regards.
-- 
FFmpeg = Frenzy Fantastic Maxi Pitiless Elastic God