| Message ID | 20230320233408.134255-10-jeebjp@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | encoder AVCodecContext configuration side data | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@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, you put it outside of the API client's view. Or in other words, "only making something public when it's absolutely required means that no unnecessary things are part of the ABI structs" Of course looking at the fun times with avci, I might just end up doing what you note... I just wanted to give it a try to have a private structure which was available before init() was called :) . Jan
Quoting Jan Ekström (2023-03-24 18:34:28) > On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@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.
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@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.
On Sun, Mar 26, 2023 at 10:00 PM Anton Khirnov <anton@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@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
On Sun, Mar 26, 2023 at 10:03 PM James Almer <jamrial@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@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
Quoting Jan Ekström (2023-03-27 08:37:07) > On Sun, Mar 26, 2023 at 10:00 PM Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Jan Ekström (2023-03-24 18:34:28) > > > 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. The fields being removed from public structs are actually private, that's the only reason they are being hidden. The thing you are adding is not private, it is public API. Having a setter for it rather than making it a field in a public struct is merely a different way of making it public, which * is inconsistent with other similar APIs * has the disadvantages I mentioned in the previous email > 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). There is no constructor for the struct, so it cannot be extended without a major bump.
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; } AVCodecInternal; /** diff --git a/libavcodec/options.c b/libavcodec/options.c index f8fab164fb..acd3472fde 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -184,12 +184,17 @@ AVCodecContext *avcodec_alloc_context3(const AVCodec *codec) void avcodec_free_context(AVCodecContext **pavctx) { AVCodecContext *avctx = *pavctx; + AVCodecInternal *avci = NULL; if (!avctx) return; avcodec_close(avctx); + avci = avctx->internal; + if (avci) + av_side_data_set_wipe(&avci->side_data_set); + av_freep(&avctx->internal); av_freep(&avctx->extradata);