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

[Stefano Sabatini]

On date Thursday 2012-11-15 21:34:15 +0100, Nicolas George encoded:
> Do not skip the end delimiter.
> Reserve positive return values.
> This is an API break, but the function was introduced less than
> two weeks ago.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  cmdutils.c          |    2 ++
>  doc/APIchanges      |    6 +++---
>  libavutil/opt.c     |    4 ++--
>  libavutil/opt.h     |    7 ++++---
>  libavutil/version.h |    2 +-
>  5 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/cmdutils.c b/cmdutils.c
> index 1c392d7..5cdb183 100644
> --- a/cmdutils.c
> +++ b/cmdutils.c
> @@ -582,6 +582,8 @@ static int init_report(const char *env)
>                         av_err2str(ret));
>              break;
>          }
> +        if (*env)
> +            env++;
>          count++;
>          if (!strcmp(key, "file")) {
>              filename_template = val;
> diff --git a/doc/APIchanges b/doc/APIchanges
> index ede8f0c..fde2eee 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,12 +15,12 @@ libavutil:     2012-10-22
>  
>  API changes, most recent first:
>  
> +2012-11-15 - xxxxxxx - lavu 52.7.100 - opt.h
> +  Add av_opt_get_key_value().
> +
>  2012-11-13 - xxxxxxx - lavfi 3.23.100 - avfilter.h
>    Add channels field to AVFilterBufferRefAudioProps.
>  
> -2012-11-02 - xxxxxxx - lavu 52.4.100 - opt.h
> -  Add av_opt_get_key_value().
> -
>  2012-11-03 - xxxxxxx - lavu 52.3.100 - opt.h
>    Add AV_OPT_TYPE_SAMPLE_FMT value to AVOptionType enum.
>  
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 05ae864..7a7f893 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -870,8 +870,6 @@ int av_opt_get_key_value(const char **ropts,
>          av_free(key);
>          return AVERROR(ENOMEM);
>      }
> -    if (*opts && strchr(pairs_sep, *opts))
> -        opts++;
>      *ropts = opts;
>      *rkey  = key;
>      *rval  = val;
> @@ -904,6 +902,8 @@ int av_opt_set_from_string(void *ctx, const char *opts,
>                         av_err2str(ret));
>              return ret;
>          }
> +        if (*opts)
> +            opts++;
>          if (parsed_key) {
>              key = parsed_key;
>              while (*shorthand) /* discard all remaining shorthand */
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 4a3b7f5..1a8a4a2 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -459,7 +459,8 @@ int av_opt_set_dict(void *obj, struct AVDictionary **options);
>   * Extract a key-value pair from the beginning of a string.
>   *
>   * @param ropts        pointer to the options string, will be updated to
> - *                     point to the rest of the string
> + *                     point to the rest of the string (one of the pairs_sep
> + *                     or the final NUL)

Nit: NULL

>   * @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
> @@ -468,8 +469,8 @@ int av_opt_set_dict(void *obj, struct AVDictionary **options);
>   * @param rkey         parsed key; must be freed using av_free()
>   * @param rval         parsed value; must be freed using av_free()
>   *
> - * @return  0 for success, or a negative value corresponding to an AVERROR
> - *          code in case of error; in particular:
> + * @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

Patch LGTM, thanks.

Some more considerations.

Since there is nothing _opt related it may make sense to move the
function to parseutils.h or avstring.h.

Also I wonder if it should not return an error in case of empty string
"" (but it should in case of ":").
-- 
FFmpeg = Fascinating & Frightening Mind-dumbing Philosophical Extroverse Governor