[FFmpeg-devel] [PATCH] add init/uninit code to cmdutils.c

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> On Sat, Sep 25, 2010 at 04:06:02PM +0200, Stefano Sabatini wrote:
>> On date Saturday 2010-09-25 15:52:50 +0200, Reimar D?ffinger encoded:
>> > On Sat, Sep 25, 2010 at 02:26:09PM +0100, M?ns Rullg?rd wrote:
>> > > Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
>> [...]
>> > > >> Index: cmdutils.c
>> > > >> ===================================================================
>> > > >> --- cmdutils.c	(revision 25163)
>> > > >> +++ cmdutils.c	(working copy)
>> > > >> @@ -56,6 +56,25 @@
>> > > >>  
>> > > >>  const int this_year = 2010;
>> > > >>  
>> > > >> +void init_opts(void)
>> > > >> +{
>> > > >> +    int i;
>> > > >> +    for (i = 0; i < AVMEDIA_TYPE_NB; i++)
>> > > >> +        avcodec_opts[i] = avcodec_alloc_context2(i);
>> > > >> +    avformat_opts = avformat_alloc_context();
>> > > >> +    sws_opts = sws_getContext(16, 16, 0, 16, 16, 0, SWS_BICUBIC, NULL, NULL, NULL);
>> > > >> +}
>> > > >> +
>> > > >> +void uninit_opts(void)
>> > > >> +{
>> > > >> +    int i;
>> > > >> +    for (i = 0; i < AVMEDIA_TYPE_NB; i++)
>> > > >> +        av_freep(&avcodec_opts[i]);
>> > > >
>> > > >> +    av_freep(&avformat_opts->key);
>> > > >
>> > > > Why is AVFormatContext.key special?
>> > > 
>> > > It is the only FF_OPT_TYPE_BINARY option.
>> > > 
>> > > > Shouldn't we have a function for automatically clean it up?
>> > > 
>> > > I suppose a function traversing the list and freeing all
>> > > FF_OPT_TYPE_BINARY things might make sense.  Would you like to write
>> > > one?
>> > 
>> > It should also free all FF_OPT_TYPE_STRING.
>> > However you can't traverse the option list since you don't have one.
>> > I can take this one out if you mind the hack, however I at least
>> > won't be writing a proper solution to this any time soon.
>> > (the proper solution would probably to free anything in the
>> > contexts that is a pointer type and marked as allocated by the
>> > user in the documentation).
>> 
>> We have avformat_alloc_context(), what about adding
>> avformat_free_context()?
>
> While that is a good idea, avformat_free_context can _only_!
> free things that libavformat has allocated, anything else
> could e.g. be a constant array the application has put there,
> and trying to free it could abort.
> Or in short: That has nothing at all do with this issue.

So let's fix the leak the simple way for now.

-- 
M?ns Rullg?rd
mans at mansr.com