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

[Marton Balint]

On Mon, 26 Feb 2024, 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?

The more I think about it, it is actually a broader problem.

You are changing the semantics of existing AV_OPT_TYPE_xxx types. So 
previously an option with AV_OPT_TYPE_STRING used to have default value in 
default_val.str. After your patch, it will be either default_val.str, or 
default_val.str[1], based on if it is an array or not.

I think the API user safely assumed that if the option type is known to 
him, he will always find the default value in the relevant default_val 
field. It is actually a bigger issue for an array of AV_OPT_TYPE_INT, 
because previously to get the default value AVOption->default_val.i64 was 
used, and now .str[1] should be instead...

The way I see it, the ARRAY modifier should either be a flag/mask of the 
option type (not AVOption->flags), or it should be a entierly new type, 
AV_OPT_TYPE_ARRAY, and its base type should be stored elsewhere.

For new opt types, we can define the semantics of default_val as we want, 
so Andreas's suggestion is good to make default_val point to an 
AVArrayOptionSettings struct or something like that.

Regards,
Marton