James Almer: > > > On 3/20/2022 8:26 PM, Andreas Rheinhardt wrote: >> James Almer: >>> It can uninitialize fields that may still be used after the context >>> was closed, >>> so do it instead in avcodec_free_context(). >>> >>> Signed-off-by: James Almer <[email protected]> >>> --- >>> libavcodec/avcodec.c | 1 - >>> libavcodec/options.c | 2 +- >>> 2 files changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c >>> index 38bdaad4fa..122d09b63a 100644 >>> --- a/libavcodec/avcodec.c >>> +++ b/libavcodec/avcodec.c >>> @@ -524,7 +524,6 @@ av_cold int avcodec_close(AVCodecContext *avctx) >>> if (avctx->priv_data && avctx->codec && >>> avctx->codec->priv_class) >>> av_opt_free(avctx->priv_data); >>> - av_opt_free(avctx); >>> av_freep(&avctx->priv_data); >>> if (av_codec_is_encoder(avctx->codec)) { >>> av_freep(&avctx->extradata); >>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>> index 33f11480a7..91335415c1 100644 >>> --- a/libavcodec/options.c >>> +++ b/libavcodec/options.c >>> @@ -172,7 +172,7 @@ void avcodec_free_context(AVCodecContext **pavctx) >>> av_freep(&avctx->intra_matrix); >>> av_freep(&avctx->inter_matrix); >>> av_freep(&avctx->rc_override); >>> - av_channel_layout_uninit(&avctx->ch_layout); >>> + av_opt_free(avctx); >>> av_freep(pavctx); >>> } >> >> This will lead to memleaks for users that use avcodec_close(avctx) + >> av_free(avctx) to free an AVCodecContext (e.g. our frame-threaded >> encoders do this). Notice that avcodec_free_context() violates the >> documentation of AVCodecContext.extradata (documented to not be freed >> for decoders) and AVCodecContext.subtitle_header and >> AVCodecContext.rc_override (documented to not be freed by lavc for >> encoders), so there is a reason for using it instead of >> avcodec_free_context() (even when not reusing the context). > > That's an absolute mess of a situation. av_free(avctx) should not be an > allowed or supported scenario when avcodec_free_context() exists. And > why is the latter violating its own documentation? >
It is not violating its own documentation, but the documentation of the relevant AVCodecContext fields. IIRC Anton wanted a function that just frees the whole context, even if this meant that fields which are documented as being owned by the user are freed. Even documenting the current state of affairs in avcodec.h doesn't change the fact that there is a valid reason to use avcodec_close()+av_free(), so we can't pretend it doesn't happen. - Andreas _______________________________________________ ffmpeg-devel mailing list [email protected] https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email [email protected] with subject "unsubscribe".
