[FFmpeg-devel] [PATCH 2/4] parse_key_value_pair: Support keys without values with AV_DICT_ALLOW_NULLVAL

[Stefano Sabatini]

On date Sunday 2013-07-21 13:54:13 +0200, Michael Niedermayer encoded:
> On Sun, Jul 21, 2013 at 01:27:31PM +0200, Stefano Sabatini wrote:
> > "parse_key_value_pair" is not useful as module name, especially given
> > we have different implementations with different levels of NIH-ness.
> > 
> 
> > Also any reason this is not merged with the previous patch?
> 
> improves git bisect usefullness if theres a bug
> 
> 
> > 
> > On date Saturday 2013-07-20 18:55:03 +0200, Michael Niedermayer encoded:
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libavutil/dict.c |   12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/libavutil/dict.c b/libavutil/dict.c
> > > index f965492..40434b6 100644
> > > --- a/libavutil/dict.c
> > > +++ b/libavutil/dict.c
> > > @@ -116,6 +116,7 @@ static int parse_key_value_pair(AVDictionary **pm, const char **buf,
> > >                                  const char *key_val_sep, const char *pairs_sep,
> > >                                  int flags)
> > >  {
> > 
> > > +    char *buf2= *buf;
> > 
> > Nit: *buf2 = *buf;
> > 
> > Who gives a bit about vertical align anyway.
> > 
> > >      char *key = av_get_token(buf, key_val_sep);
> > >      char *val = NULL;
> > >      int ret;
> > > @@ -125,8 +126,19 @@ static int parse_key_value_pair(AVDictionary **pm, const char **buf,
> > >          val = av_get_token(buf, pairs_sep);
> > >      }
> > >  
> > 

> > > +    if (flags & AV_DICT_ALLOW_NULLVAL) {
> > > +        char *key2 = av_get_token(&buf2, pairs_sep);
> > > +        if (!key || (key2 && strlen(key2) < strlen(key))) {
> > > +            FFSWAP(char *, *buf, buf2);
> > > +            FFSWAP(char *, key, key2);
> > > +            av_freep(&val);
> > > +        }
> > > +        av_freep(&key2);
> > > +    }
> > 
> > What's the final outcome of this hack?
> 
> allowing ommiting values, that is a key without value. and that then
> results in a null value 

That's not what the current libx264 code does, which is setting the
value to "1" rather than to NULL if unspecified.
 
> > 
> > >      if (key && *key && val && *val)
> > >          ret = av_dict_set(pm, key, val, flags);
> > > +    else if (key && *key && !val && (flags & AV_DICT_ALLOW_NULLVAL))
> > > +        ret = av_dict_set(pm, key, val, flags);
> > >      else
> > >          ret = AVERROR(EINVAL);
> > 
> > The result is that now we have several option list functions with
> > different behaviours (av_dict_parse_string(), av_opt_get_key_value(),
> > av_opt_set_from_string()). Ideally we should try to unify them, rather
> > than adding more arbitrary differences.
> 

> The current stuation is that libx264 has 2 slightly incompatible
> option syntaxes. Id like to fix that.

Yet this is introducing complexity in the core for a small disputable
gain in the libx264 wrapper (note: I find the code above not very
readable). What I propose is to drop one of the options at the next
bump, document the difference and deprecate one of the them in the
meanwhile.

> Iam somewhat reluctant to change other unrelated functions and add
> dead codepathes to them. (dead because nothing would set
> AV_DICT_ALLOW_NULLVAL for them) 
-- 
FFmpeg = Freak and Faithless Merciful Political Everlasting Gorilla