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

[Anton Khirnov]

Quoting Hendrik Leppkes (2022-03-21 08:37:43)
> On Mon, Mar 21, 2022 at 12:38 AM Andreas Rheinhardt
> <andreas.rheinhardt at 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 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);
> >
> > 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.

-- 
Anton Khirnov