[FFmpeg-devel] [PATCH 25/35] avutil/opt: add av_opt_to_string

[Diederick C. Niehorster]

On Tue, Jun 8, 2021 at 4:32 PM Nicolas George <george at nsup.org> wrote:
>
> Diederick Niehorster (12021-06-08):
> Please wrap this.

Will do.

> > -int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val)
> > +static int print_option(void* dst, enum AVOptionType type, int search_flags, uint8_t** out_val)
>
> I really do not like the fact that it always makes a dynamic allocation,
> especially since in most cases we know the buffer has a small bounded
> size.
>
> I'd rather have AVWriter here, but in the meantime, please consider
> using AVBPrint.

The guts of this function is unchanged from av_opt_get, just moved
here, should i address this in this patch, or is it a separate issue?
Happy to do either.

> >  {
> > -    void *dst, *target_obj;
> > -    const AVOption *o = av_opt_find2(obj, name, NULL, 0, search_flags, &target_obj);
> > -    uint8_t *bin, buf[128];
>
> > +    uint8_t* bin, buf[128];
>
> Here and everywhere, the pointer mark belongs with the variable name.
> This is an obvious example of why: written like this, it seems that bin
> and buf[128] are both uint8_t*. Correctly written, it's obvious that
> only bin is a pointer.

Absolutely, auto-formatter got me.

> > -    case AV_OPT_TYPE_CONST:
> > -        ret = snprintf(buf, sizeof(buf), "%f", o->default_val.dbl);
> > -        break;
>
> I think you should have left the case here.

ok

> >  int av_opt_get_dict_val(void *obj, const char *name, int search_flags, AVDictionary **out_val);
> > +/**
>
> Empty line please.

done.

> > + * Format option value and output to string.
> > + * @param[in] val an option value that can be represented as a double.
> > + * @param[in] type of the option.
> > + * @param[out] out_val value of the option will be written here
> > + * @return >=0 on success, a negative error code otherwise
> > + *
> > + * @note the returned string will be av_malloc()ed and must be av_free()ed by the caller
> > + */
> > +int av_opt_to_string(double val, enum AVOptionType type, uint8_t **out_val);
>
> If it only works for number-like options, then the name should reflect
> it, and the documentation should state it.
>
> Also, I have reservations about using double here: large integers cannot
> be accurately coded with double.

I understand your concerns. Perhaps its good to think about the use
case (which is more narrow than the current name suggests):
av_opt_query_ranges and the device capabilities API return a bunch of
AVOptionRange, which contains the fields:
double value_min, value_max;
I need a function to format these properly (e.g. "yuyv422" instead of
1), and that does not take a AVOption as input, since these min and
max values are not stored in an AVOption (and this new function could
be used for formatting the values stored in the fields double min and
double max in an AVOption as well). Hence the input to the function is
double.

I agree thats not ideal, nor is it great that values are stored as
doubles in a AVOptionRange, since that limits its use to things
representable as double (which arguably anything with a range is,
logically, but as you said, double can't represent everything). My
ideal solution would be to change the following def in AVOption:

union {
        int64_t i64;
        double dbl;
        const char *str;
        /* TODO those are unused now */
        AVRational q;
    } default_val;

into a named union, and use that for the default_val of an AVOption,
and for AVOptionRange's value_min and value_max. (and possibly also
for AVOption's min and max fields, but that may be too big a change).
Thats a significant API change, but AVOptionRange is hardly used in
the ffmpeg code base (I have no idea about user code, but since
they're hardly exposed, i'd expect the same?), but would allow
expressing ranges precisely, regardless of type. That would make a
more general to_string function possible as well.

Anyway, I'd be pretty happy with the solution of just adding a
function with a restricted use case and better name. What do you
think?

Thanks and all the best,
Dee