[FFmpeg-devel] [PATCH] opt: implement av_opt_set_from_string().
Stefano Sabatini
stefasab at gmail.com
Mon Aug 20 00:47:42 CEST 2012
On date Monday 2012-08-13 17:33:32 +0200, Nicolas George encoded:
> It is similar to av_set_options_string() but accepts a list
> of options that can be in shorthand: if the key is omitted
> on the first fields, the keys from the shorthand list are
> assumed, in order.
>
> It also does not allow to specify the separators, which was
> a feature never used anyway.
>
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
> libavutil/opt.c | 58 +++++++++++++++++++++++++++++++++++++++++++--------
> libavutil/opt.h | 21 +++++++++++++++++++
> libavutil/version.h | 2 +-
> 3 files changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 0adbddd..b65dea1 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -728,6 +728,11 @@ void av_opt_set_defaults2(void *s, int mask, int flags)
> * separate key from value
> * @param pairs_sep a 0-terminated list of characters used to separate
> * two pairs from each other
> + * @param key_short_sep a 0-terminated list of characters that can terminate
> + * keys: either identical to key_val_sep or to the union of key_val_sep and
> + * pairs_sep
Add a note here that the union is required in case shorthand options
are used.
> + * @param shorthand a pointer to a pointer to a NULL terminated array of
> + * option names, for shorthand syntax
> * @return 0 if the key/value pair has been successfully parsed and
> * set, or a negative value corresponding to an AVERROR code in case
> * of error:
> @@ -736,28 +741,40 @@ void av_opt_set_defaults2(void *s, int mask, int flags)
> * cannot be set
> */
> static int parse_key_value_pair(void *ctx, const char **buf,
> - const char *key_val_sep, const char *pairs_sep)
> + const char *key_val_sep, const char *pairs_sep,
> + const char *key_short_sep,
> + const char *const **shorthand)
> {
> - char *key = av_get_token(buf, key_val_sep);
> + char *first_token = av_get_token(buf, key_short_sep);
> + const char *key;
> char *val;
> int ret;
> - if (*key && strspn(*buf, key_val_sep)) {
> + if (!first_token)
> + return AVERROR(ENOMEM);
> + if (*first_token && strspn(*buf, key_val_sep)) {
> (*buf)++;
> + key = first_token;
> val = av_get_token(buf, pairs_sep);
> + while (**shorthand) /* named option -> no more shorthand */
> + (*shorthand)++;
Here you're consuming all the shorthands options, right? The comment
could be made more explicit.
> } else {
> - av_log(ctx, AV_LOG_ERROR, "Missing key or no key/value separator found after key '%s'\n", key);
> - av_free(key);
> - return AVERROR(EINVAL);
> + if (!**shorthand) {
> + av_log(ctx, AV_LOG_ERROR, "Missing key or no key/value separator found after key '%s'\n", first_token);
> + av_free(first_token);
> + return AVERROR(EINVAL);
> + }
> + key = *((*shorthand)++);
> + val = first_token;
> + first_token = NULL;
> }
> -
> av_log(ctx, AV_LOG_DEBUG, "Setting entry with key '%s' to value '%s'\n", key, val);
>
> ret = av_opt_set(ctx, key, val, 0);
> if (ret == AVERROR_OPTION_NOT_FOUND)
> av_log(ctx, AV_LOG_ERROR, "Key '%s' not found.\n", key);
>
> - av_free(key);
> + av_free(first_token);
> av_free(val);
> return ret;
> }
> @@ -766,12 +783,13 @@ int av_set_options_string(void *ctx, const char *opts,
> const char *key_val_sep, const char *pairs_sep)
> {
> int ret, count = 0;
> + const char *dummy_shorthand = NULL, *const *shorthand = &dummy_shorthand;
>
> if (!opts)
> return 0;
>
> while (*opts) {
> - if ((ret = parse_key_value_pair(ctx, &opts, key_val_sep, pairs_sep)) < 0)
> + if ((ret = parse_key_value_pair(ctx, &opts, key_val_sep, pairs_sep, key_val_sep, &shorthand)) < 0)
> return ret;
> count++;
>
> @@ -782,6 +800,27 @@ int av_set_options_string(void *ctx, const char *opts,
> return count;
> }
>
> +int av_opt_set_from_string(void *ctx, const char *opts,
> + const char *const *shorthand)
I'm not sure if we should drop the possibility to specify
separators. Indeed while we're not using the feature, it could be
useful to the application layer (and we may need to specify different
option separators in different contexts, e.g. "," for protocol
options).
As a compromise we could have:
int av_opt_set_from_string(void *ctx, const char *opts, const char *key_val_sep, const char *pair_sep, const char *const *shorthand);
int av_opt_set_from_string_s(void *ctx, const char *opts, const char *const *shorthand);
or use library-internal variants (e.g. ff_opt_set_from_string_lavfi())
or whatever color people prefer.
> +{
> + int ret, count = 0;
> + const char *dummy_shorthand = NULL;
> +
> + if (!opts)
> + return 0;
> + if (!shorthand)
> + shorthand = &dummy_shorthand;
> +
> + while (*opts) {
> + if ((ret = parse_key_value_pair(ctx, &opts, "=", ":", "=:", &shorthand)) < 0)
> + return ret;
> + count++;
> + if (*opts)
> + opts++;
> + }
> + return count;
> +}
> +
> void av_opt_free(void *obj)
> {
> const AVOption *o = NULL;
> @@ -932,6 +971,7 @@ int main(void)
> {
> int i;
>
> +
> printf("\nTesting av_set_options_string()\n");
> {
> TestContext test_ctx = { 0 };
spurious hunk
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 3bf30a5..cef820d 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -394,6 +394,27 @@ int av_set_options_string(void *ctx, const char *opts,
> const char *key_val_sep, const char *pairs_sep);
>
> /**
> + * Parse the key=value pairs list in opts. For each key=value pair found,
> + * set the value of the corresponding option in ctx.
> + *
> + * @param ctx the AVClass object to set options
> + * @param opts the options string, "key=value" pairs separated by ':'
> + * @param shorthand a NULL-terminated array of options names for shorthand
> + * notation: if the first field in opts has no "key="
> + * part, the key is taken from the first element of
> + * shorthand; then again for the second, etc., until
> + * either opts is finished, shorthand is finished or a
> + * named option is found; after that, all options must be
> + * named
> + * @return the number of successfully set key=value pairs, or a negative
> + * value corresponding to an AVERROR code in case of error:
> + * AVERROR(EINVAL) if opts cannot be parsed,
> + * the error code issued by av_set_string3() if a key/value pair
> + * cannot be set
> + */
> +int av_opt_set_from_string(void *ctx, const char *opts,
> + const char *const *shorthand);
> +/**
> * Free all string and binary options in obj.
> */
> void av_opt_free(void *obj);
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 0e381b7..0b9ae39 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -39,7 +39,7 @@
> */
You may add an example in opt-test for testing/documentation
purposes.
Code should be OK, but I would like to discuss the separator bikeshed.
--
FFmpeg = Fanciful and Free Meaningful Philosofic Enlightened Gospel
More information about the ffmpeg-devel
mailing list