[FFmpeg-devel] [PATCH] avcodec/avcodec: don't uninitialize ch_layout in avcodec_close()

[Hendrik Leppkes]

On Sun, Mar 20, 2022 at 7:52 PM James Almer <jamrial at 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 at 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 at 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_*