[FFmpeg-devel] [PATCH 8/8] avcodec/avcodec: Document current behaviour for extradata

[James Almer]

On 4/24/2021 9:53 AM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/24/2021 9:29 AM, Andreas Rheinhardt wrote:
>>> Despite the documentation saying that it is not freed by libavcodec
>>> for a decoder, avcodec_free_context() does so and has been doing so
>>> since this function has been added more than seven years ago.
>>>
>>> Honouring the current documentation in avcodec_free_context() would
>>> add memleaks to all users of it that don't free their extradata
>>> manually; given how long this behaviour has been around we can safely
>>> assume that these are many (i.e. the fftools are among them, as is
>>> libavformat as well as parts of libavcodec itself).
>>>
>>> Therefore adapt the documentation to match actual behaviour.
>>> This fixes ticket #5027.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>> ---
>>>    libavcodec/avcodec.h | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index b9b487be41..4596d12647 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -633,6 +633,8 @@ typedef struct AVCodecContext {
>>>         * Must be allocated with the av_malloc() family of functions.
>>>         * - encoding: Set/allocated/freed by libavcodec.
>>>         * - decoding: Set/allocated/freed by user.
>>> +     * Additionally, avcodec_free_context() frees it regardless of
>>> whether
>>> +     * the context is used for encoding or not.
>>
>> I'd prefer to instead change the decoding line to
>>
>> * - decoding: Set/allocated by user, freed by libavcodec.
> 
> This would be wrong as avcodec_close() does not free it for decoders.
> Changing the behaviour would be a breaking change.

avcodec_free_context() needs to be called to free any AVCodecContext. 
doing avcodec_close(avctx) + av_free(avctx) (Or worse, reusing the avctx 
after avcodec_close()) is not only heavily discouraged by the doxy, but 
already leads to leaks if you don't free a bunch of things, like 
extradata, intra_matrix and inter_matrix.

Maybe also (or instead) change the

* Must be allocated with the av_malloc() family of functions.

line with

* Must be allocated with the av_malloc() family of functions, and will 
be freed in avcodec_free_context().

Which puts the extradata doxy in line with the other two fields, both of 
which are handled in a similar way.
Leaving the line about freed by user is IMO pointless, considering you 
even mentioned in the patch message how nobody ever bothers to free it.

Honestly? avcodec_close() should have been deprecated alongside 
avcodec_get_context_defaults3() and avcodec_copy_context(), so maybe we 
should do it now. Having a function with its doxy saying "Do not use 
this function" is pretty silly.