diff mbox series

[FFmpeg-devel,1/7] avcodec/avcodec: Actually honour the documentation of subtitle_header

Message ID HE1PR0301MB21541428E57529FCA7463B668F499@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit a1ed984e049c90122e80bd60c2028f9bb22cc31c
Headers show
Series [FFmpeg-devel,1/7] avcodec/avcodec: Actually honour the documentation of subtitle_header
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 19, 2021, 2 a.m. UTC
It is only supposed to be freed by libavcodec for decoders, yet
avcodec_open2() always frees it on failure.
Furthermore, avcodec_close() doesn't free it for decoders.
Both of this has been changed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This might be squashed with the next patch.

 libavcodec/avcodec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

James Almer April 24, 2021, 12:52 p.m. UTC | #1
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@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.
Andreas Rheinhardt April 24, 2021, 12:53 p.m. UTC | #2
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@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.

- Andreas
James Almer April 24, 2021, 1:14 p.m. UTC | #3
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@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.
diff mbox series

Patch

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 760a98d8ef..24f6922d4f 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -418,7 +418,8 @@  FF_ENABLE_DEPRECATION_WARNINGS
 
     av_dict_free(&tmp);
     av_freep(&avctx->priv_data);
-    av_freep(&avctx->subtitle_header);
+    if (av_codec_is_decoder(avctx->codec))
+        av_freep(&avctx->subtitle_header);
 
 #if FF_API_OLD_ENCDEC
     av_frame_free(&avci->to_free);
@@ -589,7 +590,9 @@  FF_DISABLE_DEPRECATION_WARNINGS
         av_frame_free(&avctx->coded_frame);
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
-    }
+    } else if (av_codec_is_decoder(avctx->codec))
+        av_freep(&avctx->subtitle_header);
+
     avctx->codec = NULL;
     avctx->active_thread_type = 0;