[FFmpeg-devel] [PATCH v0 09/14] avcodec: add private side data set to AVCodecInternal

[Jan Ekström]

On Sun, Mar 26, 2023 at 10:00 PM Anton Khirnov <anton at khirnov.net> wrote:
>
> Quoting Jan Ekström (2023-03-24 18:34:28)
> > On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton at khirnov.net> wrote:
> > >
> > > Quoting Jan Ekström (2023-03-21 00:34:03)
> > > > This allows configuring an encoder by using AVFrameSideData.
> > > > ---
> > > >  libavcodec/avcodec.c  | 1 +
> > > >  libavcodec/internal.h | 7 +++++++
> > > >  libavcodec/options.c  | 5 +++++
> > > >  3 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > > > index c110b19e08..3faabe77d1 100644
> > > > --- a/libavcodec/avcodec.c
> > > > +++ b/libavcodec/avcodec.c
> > > > @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
> > > >      avci->nb_draining_errors = 0;
> > > >      av_frame_unref(avci->buffer_frame);
> > > >      av_packet_unref(avci->buffer_pkt);
> > > > +    av_side_data_set_wipe(&avci->side_data_set);
> > > >
> > > >      if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
> > > >          ff_thread_flush(avctx);
> > > > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > > > index f21101752d..c658e97313 100644
> > > > --- a/libavcodec/internal.h
> > > > +++ b/libavcodec/internal.h
> > > > @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
> > > >       * a boolean to describe whether context is opened or not.
> > > >       */
> > > >      unsigned int ctx_opened;
> > > > +
> > > > +    /**
> > > > +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> > > > +     * - encoding: set by user
> > > > +     * - decoding: unused
> > > > +     */
> > > > +    AVFrameSideDataSet side_data_set;
> > >
> > > Why put it here and not in the public struct? It seems way more natural
> > > there.
> > >
> >
> > The general idea was that if you want to make people utilize helpers
> > and not touch entries willy-nilly,
>
> But do we? Why?
>
> In this case I see no advantage to having a public function over having
> two public fields. The function is strictly worse because it cannot be
> extended, and enlarges the symbol table.

The overarching idea has been that looking at the recent changes of
adding private internal structs and deprecating previously public
stuff from the public structs was to be careful.

1. Post a patch with things added to the relevant private struct.
2. Allow specific access with a helper
3. See if anyone requires the information to be part of the public
struct. At which point it may be moved there.

Additionally, only having things available via helpers means that you
know how exactly the fields were being poked (instead of "we don't
know how API users utilize them, and thus we cannot do X" (see the
avcodec context related helpers where we still have to support close &
av_freep which popped up in another patch), and given that the
argument is a struct, the function can technically be extended
(although a struct being in the middle of a struct might stop from it
being extended).

I hope this opens up my thoughts and that I'm not a religious zealot
about this selection. Just that I wanted to start where it's not
public, so that hopefully someone would explain why it should be
public if they would wish it to be such.

Jan