| 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 | expand |
| 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 |
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.
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
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 --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;
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(-)