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

[Jan Ekström]

On Sun, Mar 26, 2023 at 10:03 PM James Almer <jamrial at gmail.com> wrote:
>
>
>
> On 3/26/2023 4:00 PM, Anton Khirnov 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.
>
> Also, is this new AVFrameSideDataSet struct necessary? This looks sort
> of like the opposite of coded_side_data.

It indeed is the other side of the coded_side_data, which is something
that gets added after avctx init.

And the main reason was because the addition function being a pointer
to a struct, instead of (AVFrameSideData ***sd, int *nb_side_data).

Jan