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

[James Almer]

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.