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

[Nicolas George]

Diederick C. Niehorster (12021-06-08):
> 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.

The critical part is the API of the new public function, because this we
cannot fix later.

Unfortunately, to get a good API, you will not be able to reuse the
implementation of av_opt_get() as it is. Therefore, you will probably
need two changes: (1) change the implementation of av_opt_get() and (2)
add the new function.

Whether this should be done in one or two patches depends on the
specifics, on the amount of code in each change and how much they cover
each other.

> > > +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:

I see the issue. Since we have a double, we must do with a double.

> 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.

I have long-term projects of rewriting the options system, with each
type (channel layouts, pixel formats, color ranges, etc.) coming with a
descriptor for printing and parsing it, and a de-centralized way of
adding new types. Unfortunately, first I need to convince people here
that we need a better strings API.

In the meantime:

> 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?

In the meantime, as I suggested, I think using AVBPrint is the best
option:

void av_num_opt_bprint(AVBPrint *bp, AVOptionType type, double val);

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210609/0e2eff6f/attachment.sig>