[FFmpeg-devel] [PATCH 25/35] avutil/opt: add av_opt_to_string
Nicolas George
george at nsup.org
Tue Jun 8 17:32:43 EEST 2021
Diederick Niehorster (12021-06-08):
> This function allows formatting an option value stored in a double (such as the min and max fields of an AVOption, or min_value and max_value of an AVOptionRange) properly, e.g. 1 for a AV_OPT_TYPE_PIXEL_FMT -> yuyv422. Useful when printing more info about an option than just its value. Usage will be shown in upcoming device_get_capabilities example. av_opt_get (body changed) still passes FATE.
Please wrap this.
>
> Signed-off-by: Diederick Niehorster <dcnieho at gmail.com>
> ---
> libavutil/opt.c | 93 +++++++++++++++++++++++++++++++++++++--------
> libavutil/opt.h | 12 +++++-
> libavutil/version.h | 2 +-
> 3 files changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index ab127b30fa..3e385852eb 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -776,24 +776,14 @@ static void format_duration(char *buf, size_t size, int64_t d)
> *(--e) = 0;
> }
>
> -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.
> {
> - 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.
> int len, i, ret;
> int64_t i64;
>
> - if (!o || !target_obj || (o->offset<=0 && o->type != AV_OPT_TYPE_CONST))
> - return AVERROR_OPTION_NOT_FOUND;
> -
> - if (o->flags & AV_OPT_FLAG_DEPRECATED)
> - av_log(obj, AV_LOG_WARNING, "The \"%s\" option is deprecated: %s\n", name, o->help);
> -
> - dst = (uint8_t *)target_obj + o->offset;
> -
> buf[0] = 0;
> - switch (o->type) {
> + switch (type) {
> case AV_OPT_TYPE_BOOL:
> ret = snprintf(buf, sizeof(buf), "%s", (char *)av_x_if_null(get_bool_name(*(int *)dst), "invalid"));
> break;
> @@ -819,9 +809,6 @@ int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val)
> case AV_OPT_TYPE_RATIONAL:
> ret = snprintf(buf, sizeof(buf), "%d/%d", ((AVRational *)dst)->num, ((AVRational *)dst)->den);
> break;
> - 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.
> case AV_OPT_TYPE_STRING:
> if (*(uint8_t **)dst) {
> *out_val = av_strdup(*(uint8_t **)dst);
> @@ -889,6 +876,80 @@ int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val)
> return *out_val ? 0 : AVERROR(ENOMEM);
> }
>
> +int av_opt_get(void *obj, const char *name, int search_flags, uint8_t **out_val)
> +{
> + void *dst, *target_obj;
> + const AVOption *o = av_opt_find2(obj, name, NULL, 0, search_flags, &target_obj);
> + uint8_t buf[128];
> + int ret;
> +
> + if (!o || !target_obj || (o->offset<=0 && o->type != AV_OPT_TYPE_CONST))
> + return AVERROR_OPTION_NOT_FOUND;
> +
> + if (o->flags & AV_OPT_FLAG_DEPRECATED)
> + av_log(obj, AV_LOG_WARNING, "The \"%s\" option is deprecated: %s\n", name, o->help);
> +
> + dst = (uint8_t *)target_obj + o->offset;
> +
> +
> + if (o->type != AV_OPT_TYPE_CONST)
> + return print_option(dst, o->type, search_flags, out_val);
> +
> + // special case for a const
> + ret = snprintf(buf, sizeof(buf), "%f", o->default_val.dbl);
> + if (ret >= sizeof(buf))
> + return AVERROR(EINVAL);
> + *out_val = av_strdup(buf);
> + return *out_val ? 0 : AVERROR(ENOMEM);
> +}
> +int av_opt_to_string(double val, enum AVOptionType type, uint8_t** out_val)
> +{
> + *out_val = NULL;
> +
> + switch (type) {
> + case AV_OPT_TYPE_FLAGS:
> + case AV_OPT_TYPE_BOOL:
> + case AV_OPT_TYPE_INT:
> + case AV_OPT_TYPE_PIXEL_FMT:
> + case AV_OPT_TYPE_SAMPLE_FMT:
> + {
> + int temp = lrint(val);
> + return print_option(&temp, type, 0, out_val);
> + }
> + case AV_OPT_TYPE_INT64:
> + case AV_OPT_TYPE_DURATION:
> + case AV_OPT_TYPE_CHANNEL_LAYOUT:
> + {
> + int64_t temp = llrint(val);
> + return print_option(&temp, type, 0, out_val);
> + }
> + case AV_OPT_TYPE_UINT64:
> + {
> + uint64_t temp = llrint(val);
> + return print_option(&temp, type, 0, out_val);
> + }
> + case AV_OPT_TYPE_FLOAT:
> + {
> + float temp = (float)val;
> + return print_option(&temp, type, 0, out_val);
> + }
> + case AV_OPT_TYPE_DOUBLE:
> + return print_option(&val, type, 0, out_val);
> +
> + default:
> + // AV_OPT_TYPE_DICT,
> + // AV_OPT_TYPE_COLOR,
> + // AV_OPT_TYPE_BINARY,
> + // AV_OPT_TYPE_STRING,
> + // AV_OPT_TYPE_RATIONAL,
> + // AV_OPT_TYPE_IMAGE_SIZE,
> + // AV_OPT_TYPE_VIDEO_RATE,
> + // AV_OPT_TYPE_CONST
> + // cannot be stored in a single double, and are thus not a valid input
> + return AVERROR(EINVAL);
> + }
> +}
> +
> static int get_number(void *obj, const char *name, const AVOption **o_out, double *num, int *den, int64_t *intnum,
> int search_flags)
> {
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index ac0b4567a6..bd386c411b 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -361,7 +361,7 @@ typedef struct AVOptionRanges {
> * for (range_index = 0; range_index < ranges->nb_ranges; range_index++) {
> * for (component_index = 0; component_index < ranges->nb_components; component_index++)
> * range[component_index] = ranges->range[ranges->nb_ranges * component_index + range_index];
> - * //do something with range here.
> + * //do something with range here. For printing the value, av_opt_to_string() may be of use.
> * }
> * av_opt_freep_ranges(&ranges);
> * @endcode
> @@ -760,6 +760,16 @@ int av_opt_get_channel_layout(void *obj, const char *name, int search_flags, int
> * be freed with av_dict_free() by the caller
> */
> int av_opt_get_dict_val(void *obj, const char *name, int search_flags, AVDictionary **out_val);
> +/**
Empty line please.
> + * 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.
> /**
> * @}
> */
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 34b83112de..77ca4becd6 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
> */
>
> #define LIBAVUTIL_VERSION_MAJOR 57
> -#define LIBAVUTIL_VERSION_MINOR 1
> +#define LIBAVUTIL_VERSION_MINOR 2
> #define LIBAVUTIL_VERSION_MICRO 100
>
> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
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/20210608/1ecff57e/attachment.sig>
More information about the ffmpeg-devel
mailing list