[FFmpeg-devel] [PATCH 1/2] lavu/opt: fix av_opt_get_key_value() API.

[Stefano Sabatini]

On date Saturday 2012-11-10 13:17:39 +0100, Nicolas George encoded:
> Add an argument to return the final delimiter.
> This is an API break, but the function was introduced only
> a week ago.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  cmdutils.c          |    3 ++-
>  doc/APIchanges      |    3 ++-
>  libavutil/opt.c     |    9 ++++++---
>  libavutil/opt.h     |    3 ++-
>  libavutil/version.h |    2 +-
>  5 files changed, 13 insertions(+), 7 deletions(-)
> 
> 
> Not completely sure:
> 
> The end delimiter is useful for parsing, for example to parse a string like
> "[foo=bar:baz=qux]" and know whether we have reached the ']' or not.
> 
> But does anyone believe that the '=' delimiter is useful too?

Reading the current docs I realize it is not very clear. What I'd
expect:

[foo=bar:baz=qux]
 ^

after parsing:
[foo=bar:baz=qux]
        ^
        ropts

second call (after updating ropts):
[foo=bar:baz=qux]
                ^
                ropts

According to docs:

|Extract a key-value pair from the beginning of a string.

so I'd assume this "consumes" the parsed input. This is not always
very practical but I'd assume it is the most natural expected
behavior for a parsing function.

|@param ropts        pointer to the options string, will be updated to
|                    point to the rest of the string

Here "rest of string" is ambiguous.

While at it, I also suggest to change:
 
|@return  0 for success, or a negative value corresponding to an AVERROR
|           code in case of error; in particular:
|           AVERROR(EINVAL) if no key is present

in order to return >= 0 in case of success, in case we want to provide
more information.

For example: it might return 0 in case of empty string (so not
considered like an error), or the number of consumed chars, which
might turn useful when iterating.

[...]
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 4a3b7f5..d95d56a 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -467,6 +467,7 @@ int av_opt_set_dict(void *obj, struct AVDictionary **options);
>   * @param flags        flags; see the AV_OPT_FLAG_* values below
>   * @param rkey         parsed key; must be freed using av_free()
>   * @param rval         parsed value; must be freed using av_free()
> + * @param rdelim       if not NULL, used to return the delimiter at the end
>   *
>   * @return  0 for success, or a negative value corresponding to an AVERROR
>   *          code in case of error; in particular:
> @@ -476,7 +477,7 @@ int av_opt_set_dict(void *obj, struct AVDictionary **options);
>  int av_opt_get_key_value(const char **ropts,
>                           const char *key_val_sep, const char *pairs_sep,
>                           unsigned flags,
> -                         char **rkey, char **rval);
> +                         char **rkey, char **rval, char *rdelim);

If you want my opinion, I'd rather change the way ropt is updated, and
add the check if (*ropts && strchr(delims, *ropts)) *ropts++; in the
surrounding code, even if a little annoying looks more predictable.

[...]
-- 
FFmpeg = Fancy & Funny Magnificient Prodigious Extravagant Gorilla