On Sun, 20 Mar 2022, James Almer wrote:
On 3/20/2022 9:05 PM, Marton Balint wrote:
On Mon, 21 Mar 2022, Marton Balint wrote:
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 <jamr...@gmail.com>
---
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/20220319030407.45503-1-jamr...@gmail.com/
which should have the same effect and never fail, unlike param copy.
The latter would also leak memory on avcodec_close()+av_freep().
Sorry, I meant the first one.
avformat_free_context() calls avcodec_free_context() on the relevant
AVCodecContexts.
Sorry, I meant the second case, if you do the dance with
AVCodecContext->ch_layout in avcodec_close() then
avcodec_close()+av_freep(avctx) will leak ch_layout.map on custom layouts.
I hope I finally make sense :)
Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
