[FFmpeg-devel] [PATCH] indeo3: remove unnecessary iv_free_func() function

[Reimar Döffinger]

On 18 May 2011, at 12:28, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Wed, May 18, 2011 at 10:24:38AM +0200, Stefano Sabatini wrote:
>> On date Wednesday 2011-05-18 07:49:35 +0200, Reimar Döffinger encoded:
>>> 
>>> 
>>> On 17 May 2011, at 22:30, Stefano Sabatini <stefano.sabatini-lala at poste.it> wrote:
>>> 
>>>> Call the freeing code directly in indeo3_decode_close(). Simplify.
>>>> ---
>>>> libavcodec/indeo3.c |   13 +++----------
>>>> 1 files changed, 3 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/libavcodec/indeo3.c b/libavcodec/indeo3.c
>>>> index c7ca61d..993850c 100644
>>>> --- a/libavcodec/indeo3.c
>>>> +++ b/libavcodec/indeo3.c
>>>> @@ -149,13 +149,6 @@ static av_cold int iv_alloc_frames(Indeo3DecodeContext *s)
>>>>    return 0;
>>>> }
>>>> 
>>>> -static av_cold void iv_free_func(Indeo3DecodeContext *s)
>>>> -{
>>>> -    av_freep(&s->buf);
>>>> -    av_freep(&s->ModPred);
>>>> -    av_freep(&s->corrector_type);
>>>> -}
>>>> -
>>>> struct ustr {
>>>>    int xpos;
>>>>    int ypos;
>>>> @@ -979,8 +972,6 @@ static av_cold int indeo3_decode_init(AVCodecContext *avctx)
>>>> 
>>>>    if (!(ret = build_modpred(s)))
>>>>        ret = iv_alloc_frames(s);
>>>> -    if (ret)
>>>> -        iv_free_func(s);
>>>> 
>>>>    return ret;
>>>> }
>>> 
>>> Huh? I don't think decode_end is called if init fails.
>> 
>> This is not very clear from the code.
>> 
>> I'm used to the lavfi model:
>> find filter
>> avfilter_open -> create the context
>> init filter
>> uninit filter
>> free filter (automatically call uninit, uninit is idempotent)
>> 
>> With codecs we follow a different path:
>> find codec
>> allocate context with avcodec_alloc_context()
>> open codec -> fill and init the codec context
>> close codec and free associated memory
>> free codec context
>> 
>> In the second case the codec context is not freed by a codec-specific
>> function (e.g. avcodec_free()), so _close() may not be called and we
>> don't request it explicitely.
>> 
>> Maybe we could modify the use model, but that's an entirely different
>> matter so patch dropped.
> 
> i dont think the patch should be droped.
> indeo3_decode_end() should be called instead of iv_free_func() in
> indeo3_decode_init()

That is actually what I meant to suggest, and that is the reason the API is the way it is: it is trivial to call the end function when init fails, however in some cases it might be difficult to write a end/close function that can handle any intermediate state during init.