Re: [FFmpeg-devel] [PATCH 15/38] lavu/opt: add array options

[Anton Khirnov <anton@khirnov.net>]

Quoting Marton Balint (2024-02-23 20:05:06)
> 
> 
> On Fri, 23 Feb 2024, Anton Khirnov wrote:
> 
> > AVOption.array_max_size is added before AVOption.unit to avoid
> > increasing sizeof(AVOption).
> > ---
> > doc/APIchanges        |   3 +
> > libavutil/opt.c       | 344 ++++++++++++++++++++++++++++++++++++------
> > libavutil/opt.h       |  26 ++++
> > libavutil/tests/opt.c |  34 +++++
> > tests/ref/fate/opt    |  23 ++-
> > 5 files changed, 385 insertions(+), 45 deletions(-)
> >
> [...]
> 
> > --- a/libavutil/opt.h
> > +++ b/libavutil/opt.h
> > @@ -288,6 +288,16 @@ enum AVOptionType{
> >  */
> > #define AV_OPT_FLAG_CHILD_CONSTS    (1 << 18)
> > 
> > +/**
> > + * The option is an array.
> > + *
> > + * When adding array options to an object, @ref AVOption.offset should refer to
> > + * a pointer corresponding to the option type. The pointer should be immediately
> > + * followed by an unsigned int that will store the number of elements in the
> > + * array.
> > + */
> > +#define AV_OPT_FLAG_ARRAY           (1 << 19)
> > +
> > /**
> >  * AVOption
> >  */
> > @@ -313,6 +323,16 @@ typedef struct AVOption {
> >     union {
> >         int64_t i64;
> >         double dbl;
> > +
> > +        /**
> > +         * This member is always used for AV_OPT_FLAG_ARRAY options. When
> > +         * non-NULL, the first character of the string must be the separator to
> > +         * be used for (de)serializing lists to/from strings with av_opt_get(),
> 
> This is quite ugly. Also it breaks the assumption that if the user sets an 
> option value to the default value of the option, than it will work.

I don't follow, what assumption are you talking about?

> So let's just remove this feature for now.
> 
> Eventually I think some new struct should be introduced, e.g. 
> AVOptionExtension, which can be used to specify additional option 
> settings, such as array min/max size, and maybe separator. It would be a 
> lot more clean and future proof than filling the holes in AVOption.

I've actually considered that, but don't see a clean way of linking such
an extension with its option. We only have an int-sized hole, so can't
add a new pointer field to AVOption. I suppose there could be a new
array of extensions in AVClass, linked to options by name, but that
seems even more ugly and inefficient.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".