[FFmpeg-devel] [PATCH 06/29] lavu/opt: add array options

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Anton Khirnov:
>> +/**
>> + * Must be set as default_val for AV_OPT_TYPE_FLAG_ARRAY options.
>> + */
>> +typedef struct AVOptionArrayDef {
>> +    /**
>> +     * Must be set to sizeof(AVOptionArrayDef), in order to allow extending this
>> +     * struct without breaking ABI.
>> +     */
>> +    size_t              sizeof_self;
> 
> I do not really get the point of this field: It is not sufficient for
> detecting whether a user used a version that set a certain field due to
> trailing padding (i.e. an additional field need not increase
> sizeof(AVOptionArrayDef); this is actually relevant for this structure,
> because on x64 at least a new int/unsigned would fit into trailing padding).
> Luckily we already have a way to know the user's lavu version, as it is
> contained in the AVClass.
> 

I do not really see a reply to the above comment.

>> +
>> +    /**
>> +     * Native access only.
>> +     *
>> +     * Default value of the option, as would be serialized by av_opt_get() (i.e.
>> +     * using the value of sep as the separator).
>> +     */
>> +    const char         *def;
>> +
>> +    /**
>> +     * Minimum number of elements in the array. When this field is non-zero, def
>> +     * must be non-NULL and contain at least this number of elements.
>> +     */
>> +    unsigned            size_min;
>> +    /**
>> +     * Maximum number of elements in the array, 0 when unlimited.
>> +     */
>> +    unsigned            size_max;
>> +
>> +    /**
>> +     * Separator between array elements in string representations of this
>> +     * option, used by av_opt_set() and av_opt_get(). It must be a printable
>> +     * ASCII character, excluding alphanumeric and the backslash. A comma is
>> +     * used when sep=0.
>> +     *
>> +     * The separator and the backslash must be backslash-escaped in order to
>> +     * appear in string representations of the option value.
>> +     */
>> +    uint8_t             sep;
> 
> If this is a printable ASCII character, then it should be a char.
> 
>> +} AVOptionArrayDef;
>> +
>>  /**
>>   * AVOption
>>   */
>> @@ -320,8 +371,7 @@ typedef struct AVOption {
>>      enum AVOptionType type;
>>  
>>      /**
>> -     * Native access only.
>> -     *
>> +     * Native access only, except when documented otherwise.
>>       * the default value for scalar options
>>       */
>>      union {
>> @@ -330,6 +380,14 @@ typedef struct AVOption {
>>          const char *str;
>>          /* TODO those are unused now */
>>          AVRational q;
>> +
>> +        /**
>> +         * Used for AV_OPT_TYPE_FLAG_ARRAY options. May be NULL.
>> +         *
>> +         * Foreign access to some members allowed, as noted in AVOptionArrayDef
>> +         * documentation.
>> +         */
>> +        const AVOptionArrayDef *arr;
>>      } default_val;
>>      double min;                 ///< minimum valid value for the option
>>      double max;                 ///< maximum valid value for the option
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".