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

[James Almer]

On 2/26/2024 2:14 PM, Anton Khirnov wrote:
> 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.
If there's no other solution, then adding a new field is ok.