| Message ID | 20220319030407.45503-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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? - Hendrik
Quoting James Almer (2022-03-19 04:04:07) > The function is not meant to clear codec parameters, and the lavf demux code > relies on this behavior. Maybe it shouldn't? Which code is it exactly?
On 3/19/2022 10:47 AM, Anton Khirnov wrote: > Quoting James Almer (2022-03-19 04:04:07) >> The function is not meant to clear codec parameters, and the lavf demux code >> relies on this behavior. > > Maybe it shouldn't? > > Which code is it exactly? The parser included by demux.c. That file calls avcodec_close() in certain situations, but the avctx is still used when calling av_parser_parse2() as it's expected that all the parameters are left intact, and all were until 327efa6633, where avctx->ch_layout started being uninitialized, and it shouldn't.
On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: > On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 3/20/2022 2:03 PM, James Almer wrote: > On 3/19/2022 10:47 AM, Anton Khirnov wrote: >> Quoting James Almer (2022-03-19 04:04:07) >>> The function is not meant to clear codec parameters, and the lavf >>> demux code >>> relies on this behavior. >> >> Maybe it shouldn't? >> >> Which code is it exactly? > > The parser included by demux.c. That file calls avcodec_close() in > certain situations, but the avctx is still used when calling > av_parser_parse2() as it's expected that all the parameters are left > intact, and all were until 327efa6633, where avctx->ch_layout started > being uninitialized, and it shouldn't. An alternative is > diff --git a/libavformat/demux.c b/libavformat/demux.c > index c1c9422ac0..c14e44cb07 100644 > --- a/libavformat/demux.c > +++ b/libavformat/demux.c > @@ -2957,6 +2957,7 @@ find_stream_info_err: > av_freep(&sti->info); > } > avcodec_close(sti->avctx); > + avcodec_parameters_to_context(sti->avctx, st->codecpar); > av_bsf_free(&sti->extract_extradata.bsf); > } > if (ic->pb) { Which works around this specific issue, but either way, avcodec_close(avctx) should not nuke avctx->ch_layout.
On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote: > > On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: > > On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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_*
On 3/20/2022 7:01 PM, Hendrik Leppkes wrote: > On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial@gmail.com> wrote: >> >> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: >>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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 <jamrial@gmail.com> wrote: >>> >>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: >>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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. Regards, Marton
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 <jamrial@gmail.com> wrote: >>>> >>>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: >>>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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. 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. > > 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-request@ffmpeg.org with subject "unsubscribe".
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 <jamrial@gmail.com> wrote: >>>>> >>>>> On 3/19/2022 4:50 AM, Hendrik Leppkes wrote: >>>>>> On Sat, Mar 19, 2022 at 4:04 AM James Almer <jamrial@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 <jamrial@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
James Almer: > 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 <jamrial@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); avcodec_close() has always* called av_opt_free() and therefore uninitialized allocated options (the documentation of avcodec_close() states that it "frees all the data associated with it"); the new channel layout API is (potentially) based on allocations, so ch_layout belongs to this group of elements. If the demux code wants to preserve ch_layout, then the demux code should do it itself; it should not life in avcodec_close() (where it would be a hack). - Andreas *: or at least for a very long time -- I didn't do archeology for this statement.
On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > James Almer: > > 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 <jamrial@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); > > avcodec_close() has always* called av_opt_free() and therefore > uninitialized allocated options (the documentation of avcodec_close() > states that it "frees all the data associated with it"); the new channel > layout API is (potentially) based on allocations, so ch_layout belongs > to this group of elements. > If the demux code wants to preserve ch_layout, then the demux code > should do it itself; it should not life in avcodec_close() (where it > would be a hack). > But is it expected that this is going to happen? It doesn't reset any other codec properties, and if I set ch_layout without using avoptions (you know, like 99% of all callers, if you use an codec context in code then avoptions are rather clumsy), would I expect it to be reset? ch_layout just happens to get reset because it potentially has allocations, not because every codec property gets reset, its a technical detail not a logical conclusion. I think there is such a thing as being too strict about weird semantics here. - Hendrik
Quoting Hendrik Leppkes (2022-03-21 08:37:43) > On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt > <andreas.rheinhardt@outlook.com> wrote: > > > > James Almer: > > > 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 <jamrial@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); > > > > avcodec_close() has always* called av_opt_free() and therefore > > uninitialized allocated options (the documentation of avcodec_close() > > states that it "frees all the data associated with it"); the new channel > > layout API is (potentially) based on allocations, so ch_layout belongs > > to this group of elements. > > If the demux code wants to preserve ch_layout, then the demux code > > should do it itself; it should not life in avcodec_close() (where it > > would be a hack). > > > > But is it expected that this is going to happen? It doesn't reset any > other codec properties, and if I set ch_layout without using avoptions > (you know, like 99% of all callers, if you use an codec context in > code then avoptions are rather clumsy), would I expect it to be reset? > ch_layout just happens to get reset because it potentially has > allocations, not because every codec property gets reset, its a > technical detail not a logical conclusion. I think there is such a > thing as being too strict about weird semantics here. People should not use avcodec_close() anyway.
On 3/21/2022 4:51 AM, Anton Khirnov wrote: > Quoting Hendrik Leppkes (2022-03-21 08:37:43) >> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt >> <andreas.rheinhardt@outlook.com> wrote: >>> >>> James Almer: >>>> 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 <jamrial@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); >>> >>> avcodec_close() has always* called av_opt_free() and therefore >>> uninitialized allocated options (the documentation of avcodec_close() >>> states that it "frees all the data associated with it"); the new channel >>> layout API is (potentially) based on allocations, so ch_layout belongs >>> to this group of elements. >>> If the demux code wants to preserve ch_layout, then the demux code >>> should do it itself; it should not life in avcodec_close() (where it >>> would be a hack). >>> >> >> But is it expected that this is going to happen? It doesn't reset any >> other codec properties, and if I set ch_layout without using avoptions >> (you know, like 99% of all callers, if you use an codec context in >> code then avoptions are rather clumsy), would I expect it to be reset? >> ch_layout just happens to get reset because it potentially has >> allocations, not because every codec property gets reset, its a >> technical detail not a logical conclusion. I think there is such a >> thing as being too strict about weird semantics here. > > People should not use avcodec_close() anyway. What's your opinion on the "don't free" AVOption flag Hendrik suggested?
James Almer: > On 3/21/2022 4:51 AM, Anton Khirnov wrote: >> Quoting Hendrik Leppkes (2022-03-21 08:37:43) >>> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt >>> <andreas.rheinhardt@outlook.com> wrote: >>>> >>>> James Almer: >>>>> 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 <jamrial@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); >>>> >>>> avcodec_close() has always* called av_opt_free() and therefore >>>> uninitialized allocated options (the documentation of avcodec_close() >>>> states that it "frees all the data associated with it"); the new >>>> channel >>>> layout API is (potentially) based on allocations, so ch_layout belongs >>>> to this group of elements. >>>> If the demux code wants to preserve ch_layout, then the demux code >>>> should do it itself; it should not life in avcodec_close() (where it >>>> would be a hack). >>>> >>> >>> But is it expected that this is going to happen? It doesn't reset any >>> other codec properties, and if I set ch_layout without using avoptions >>> (you know, like 99% of all callers, if you use an codec context in >>> code then avoptions are rather clumsy), would I expect it to be reset? >>> ch_layout just happens to get reset because it potentially has >>> allocations, not because every codec property gets reset, its a >>> technical detail not a logical conclusion. I think there is such a >>> thing as being too strict about weird semantics here. >> >> People should not use avcodec_close() anyway. > > What's your opinion on the "don't free" AVOption flag Hendrik suggested? If one uses this on AVCodecContext.ch_layout, avcodec_close+av_free might leak. So I don't see a usecase for it. - Andreas
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);
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 <jamrial@gmail.com> --- libavcodec/avcodec.c | 6 ++++++ 1 file changed, 6 insertions(+)