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

[Michael Niedermayer]

On Sun, Jul 21, 2013 at 07:40:31PM +0200, Stefano Sabatini wrote:
> 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.

no its also not libx264 but a string parser.
The parser parses the keys that come without values
the code using the parser then passes it to libx264 and that could
if needed set "1" as value (which is not needed)


>  
> > > 
> > > >      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

the reason that started this is indeed small and disputable but i
dont think the ability to parse key1=val1:key2:key3=val3
is a small and disputable feature if the code works


> (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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130721/819b4f90/attachment.asc>