On Sun, 20 Mar 2022, James Almer wrote:
On 3/20/2022 8:34 PM, Andreas Rheinhardt wrote:
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
Ok, do i add a codecpar copy like i suggested in
http://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294312.html, then? It
works, but it feels really weird doing that in what's the cleanup portion of
the function.
Alternatively, add the dance from
https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/
which should have the same effect and never fail, unlike param copy.
The latter would also leak memory on avcodec_close()+av_freep().
Regards,
Marton
_______________________________________________
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".
