[FFmpeg-devel] [PATCH] opt: implement av_opt_set_from_string().
Stefano Sabatini
stefasab at gmail.com
Mon Sep 10 13:21:04 CEST 2012
On date Sunday 2012-09-09 15:13:23 +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.
>
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
> libavutil/opt.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> libavutil/opt.h | 26 +++++++++++++
> 2 files changed, 142 insertions(+)
>
>
> Reading the key directly instead of using the tokenizer makes the code,
> IMHO, much simpler and the error reporting clearer. It does not limit the
> user, as the key names are short and made of alphanumeric characters.
>
> A small dilemma: if shorthand = { "a", "b", NULL }, how should ":42" be
> parsed? As "a=:b=42" (setting both a and b) or as "b=42" (leaving a to its
> default)? The problem is that since the value is parsed using the tokenizer,
> we can not distinguish ":42" from "'':42".
Flat syntax works this way:
val1:val2
If you want to set b, you *must* set a, so :42 to me means:
key1=val1:key2=val2
so in this case:
:42
=>
a=:b=42
Should be also possibly simpler to implement.
>
>
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 0957281..b800daa 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -790,6 +790,91 @@ int av_set_options_string(void *ctx, const char *opts,
> return count;
> }
>
> +#define WHITESPACES " \n\t"
\r\t\f?
> +
> +static int is_key_char(char c)
> +{
> + return (unsigned)((c | 32) - 'a') < 26 ||
> + (unsigned)(c - '0') < 10 ||
> + c == '-' || c == '_' || c == '/' || c == '.';
> +}
> +
> +/**
> + * Read a key from a string. The key consists of is_key_char characters and
> + * is terminated by a character from the delim string; spaces are ignored.
Nit: MUST be terminated
> + * The key buffer must be 4 bytes larger than the longest acceptable key.
> + * If the key is too long, an ellipsis will be written at the end.
> + * @return 0 for success (even with ellipsis), <0 for failure
> + */
> +static int get_key(const char **ropts, const char *delim, char *key, unsigned key_size)
> +{
> + unsigned pos = 0;
Nit: key_pos for enhanced readability
> + const char *opts = *ropts;
> +
> + opts += strspn(opts, WHITESPACES);
> + while (is_key_char(*opts)) {
> + key[pos++] = *opts;
> + if (pos == key_size)
> + pos--;
> + (opts)++;
> + }
> + opts += strspn(opts, WHITESPACES);
> + if (!*opts || !strchr(delim, *opts))
> + return AVERROR(EINVAL);
> + opts++;
> + key[pos++] = 0;
this pos++ increment is a bit confusing (and possibly unnecessary)
> + if (pos == key_size)
> + key[pos - 4] = key[pos - 3] = key[pos - 2] = '.';
> + *ropts = opts;
> + return 0;
> +}
> +
> +int av_opt_set_from_string(void *ctx, const char *opts,
> + const char *const *shorthand,
> + const char *key_val_sep, const char *pairs_sep)
> +{
> + int ret, count = 0;
> + const char *dummy_shorthand = NULL;
> + char key_buf[68], *value;
> + const char *key;
> +
> + if (!opts)
> + return 0;
> + if (!shorthand)
> + shorthand = &dummy_shorthand;
> +
> + while (*opts) {
> + if ((ret = get_key(&opts, key_val_sep, key_buf, sizeof(key_buf))) < 0) {
> + if (*shorthand) {
> + key = *(shorthand++);
> + } else {
> + av_log(ctx, AV_LOG_ERROR, "No option name near '%s'\n", opts);
> + return AVERROR(EINVAL);
> + }
> + } else {
> + key = key_buf;
> + while (*shorthand) /* discard all remaining shorthand */
> + shorthand++;
> + }
> +
> + if (!(value = av_get_token(&opts, pairs_sep)))
> + return AVERROR(ENOMEM);
> + if (*opts && strchr(pairs_sep, *opts))
> + opts++;
> +
> + av_log(ctx, AV_LOG_DEBUG, "Setting '%s' to value '%s'\n", key, value);
> + if ((ret = av_opt_set(ctx, key, value, 0)) < 0) {
> + if (ret == AVERROR_OPTION_NOT_FOUND)
> + av_log(ctx, AV_LOG_ERROR, "Option '%s' not found.\n", key);
Nit: strip trailing "."
> + return ret;
> + }
> +
> + av_free(value);
> + count++;
> + }
> + return count;
> +}
> +
> void av_opt_free(void *obj)
> {
> const AVOption *o = NULL;
> @@ -986,6 +1071,37 @@ int main(void)
> av_freep(&test_ctx.string);
> }
>
> + printf("\nTesting av_opt_set_from_string()\n");
> + {
> + TestContext test_ctx = { 0 };
> + const char *options[] = {
> + "",
> + "5",
> + "5:hello",
> + "5:hello:size=pal",
> + "5:size=pal:hello",
> + ":",
> + "=",
> + " 5 : hello : size = pal ",
> + "a_very_long_option_name_that_will_need_to_be_ellipsized_around_here=42"
> + };
> + const char *shorthand[] = { "num", "string", NULL };
> +
> + test_ctx.class = &test_class;
> + av_opt_set_defaults(&test_ctx);
> + test_ctx.string = av_strdup("default");
Note: this should not be necessary anymore, since we support default
string values since a long time.
Could you show the output? (I don't remember if the output of the test
is checked by FATE).
> +
> + av_log_set_level(AV_LOG_DEBUG);
> +
> + for (i=0; i < FF_ARRAY_ELEMS(options); i++) {
> + av_log(&test_ctx, AV_LOG_DEBUG, "Setting options string '%s'\n", options[i]);
> + if (av_opt_set_from_string(&test_ctx, options[i], shorthand, "=", ":") < 0)
> + av_log(&test_ctx, AV_LOG_ERROR, "Error setting options string: '%s'\n", options[i]);
> + printf("\n");
> + }
> + av_freep(&test_ctx.string);
> + }
> +
> return 0;
> }
>
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 285d854..5c9b35e 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -394,6 +394,32 @@ 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
nit: to set option on? (on which/where to set options)
> + * @param opts the options string, key-value pairs separated by a
> + * delimiter
> + * @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
> + * @param key_val_sep a 0-terminated list of characters used to separate
> + * key from value, for example '='
> + * @param pairs_sep a 0-terminated list of characters used to separate
> + * two pairs from each other, for example ':' or ','
> + * @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
> + */
I don't know if this is the right place where to document the key
limitations, possibly somewhere in opt.h/AVOption.name is a better
place (rather than in this patch).
> +int av_opt_set_from_string(void *ctx, const char *opts,
> + const char *const *shorthand,
> + const char *key_val_sep, const char *pairs_sep);
> +/**
> * Free all string and binary options in obj.
> */
> void av_opt_free(void *obj);
Looks nice otherwise, thanks so far for your work on this.
--
FFmpeg = Fanciful Frenzy Martial Prodigious Esoteric Gigant
More information about the ffmpeg-devel
mailing list