On Sun, 20 Mar 2022, James Almer wrote:
On 3/20/2022 7:34 PM, Marton Balint wrote:
On Sun, 20 Mar 2022, James Almer wrote:
On 3/20/2022 7:01 PM, Hendrik Leppkes wrote:
On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamr...@gmail.com> wrote:
On 3/19/2022 4:50 AM, Hendrik Leppkes wrote:
On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamr...@gmail.com>
wrote:
The function is not meant to clear codec parameters, and the lavf
demux
code
relies on this behavior.
Regression since 327efa66331ebdc0087c6b656059a8df2f404019.
Signed-off-by: James Almer <jamr...@gmail.com>
---
libavcodec/avcodec.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 38bdaad4fa..253c9f56cc 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -469,6 +469,7 @@ void avsubtitle_free(AVSubtitle *sub)
av_cold int avcodec_close(AVCodecContext *avctx)
{
+ AVChannelLayout ch_layout;
int i;
if (!avctx)
@@ -524,7 +525,12 @@ 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() will uninitialize avctx->ch_layout, but we
want
to keep it.
+ It will be uninitialized in avcodec_free_context() */
+ ch_layout = avctx->ch_layout;
+ memset(&avctx->ch_layout, 0, sizeof(avctx->ch_layout));
av_opt_free(avctx);
+ avctx->ch_layout = ch_layout;
av_freep(&avctx->priv_data);
if (av_codec_is_encoder(avctx->codec)) {
av_freep(&avctx->extradata);
This feels pretty ugly and still a bit risky that any call to
av_opt_free could invalidate data its not supposed to. Maybe we
should
have a flag for AVOptions instead where av_opt_free won't touch an
entry, because its only there to set/get it, not manage its memory?
Where would that flag be set? av_opt_free() takes none. And that
function exists purely to free strings, dictionaries, and now
uninitialize AVChannelLayout elements in a struct. If you don't want
to
free what av_opt_set() allocated, you shouldn't call av_opt_free() at
all.
On the AVOption element in the table, with the other AV_OPT_FLAG_*
Oh, you meant flagging the actual AVOption. My bad, i for some reason
thought you meant having the user flag which options they wanted to
"own".
I'll try to implement this. Any suggestion for the flag name?
AV_OPT_FLAG_NO_FREE?
This also looks hackish to me. Isn't it a lot simpler to remove
av_opt_free from avcodec_close()? It is deprecated for freeing data of an
avcodec context since 2014. Actually maybe avcodec_close() should be
deprecated as well.
It frees a lot of things. Extradata if encoder, subtitle_header if decoder,
hw_frames_ctx, hw_device_ctx, the entirety of AVCodecInternal, etc.
Yeah, but it still kind of vague which fields are supposed to be freed or
reset and which fields are not... So in the long run deprecation of
avcodec_close() should be considered IMHO.
But yes, we could move the av_opt_free() call to avcodec_free_context(). The
only options of type STRING are sub_charenc, dump_separator, and
codec_whitelist, and afair you're not supposed to reuse an AVCodecContext
after closing it, so it should be fine freeing those alongside the context.
I kind of prefer this. avcodec_close() documents that reusing a closed
context is not supported anymore. (anymore meaning since 2016...)
Thanks,
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".
