[FFmpeg-devel] [PATCH] Implement av_set_options_string()

[Stefano Sabatini]

On date Wednesday 2009-05-06 05:15:40 +0200, Michael Niedermayer encoded:
> On Wed, May 06, 2009 at 12:33:28AM +0200, Stefano Sabatini wrote:
[...]
> >  parseutils.c |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  parseutils.h |   17 +++++++
> >  2 files changed, 146 insertions(+)
> > 0ba9f5103f7f28a4d4255fa591c33610a9817246  parseutils-implement-set-options.patch
> > Index: libavfilter-soc/ffmpeg/libavfilter/parseutils.c
> > ===================================================================
> > --- libavfilter-soc.orig/ffmpeg/libavfilter/parseutils.c	2009-05-06 00:28:02.000000000 +0200
> > +++ libavfilter-soc/ffmpeg/libavfilter/parseutils.c	2009-05-06 00:29:32.000000000 +0200
> > @@ -248,10 +248,109 @@
> >      return 0;
> >  }
> >  
> > +/**
> > + * Stores the value in the field in ctx that is named like key.
> > + * ctx must be an AVClass context, storing is done using AVOptions.
> > + *
> 
> > + * @param buf the buffer to parse, buf will be updated to point to the
> > + * character just after the parsed string
> 
> the string to parse ...

Fixed.
 
> > + * @param key_val_sep a 0-terminated list of the characters used to
> > + * separate key from value
> > + * @param pairs_sep a 0-terminated list of the characters used to
> > + * separate pairs
> 
> ... seperate 2 pairs from each other 

Fixed.

> > + * @return 0 if no key/value pair has been found, 1 if a key/value
> > + * pair has been successfully parsed and set, a negative value
> > + * otherwise
> > + */
> > +static int parse_key_value_pair(void *ctx, const char **buf,
> > +                                const char *key_val_sep, const char *pairs_sep)
> > +{
> > +    char *key = av_get_token(buf, key_val_sep);
> 
> > +    char *val = NULL;
> 
> redundant init

Fixed.
 
> [...]
> > +int av_set_options_string(void *ctx, const char *opts,
> > +                          const char *key_val_sep, const char *pairs_sep)
> > +{
> > +    int count = 0;
> > +
> > +    while (*opts) {
> > +        if (parse_key_value_pair(ctx, &opts, key_val_sep, pairs_sep) < 0)
> > +            return -1;
> > +        count++;
> 
> > +        if (*opts)
> > +            opts++;
> 
> is that really needed?

In the case of the last key/value pair, opts is updated to point to
the terminating 0 char, then if it is unconditionally increased it
could point to any char.
 
> 
> > +    }
> > +
> > +    if (*opts) {
> 
> unreachablee

Removed. 


> [...]
> > +static const AVOption options[]= {
> > +{"num", "set num", OFFSET(num), FF_OPT_TYPE_INT, 0, 0, 100},
> > +{"toggle", "set toggle", OFFSET(toggle), FF_OPT_TYPE_INT, 0, 0, 1},
> > +{"string", "set string", OFFSET(string), FF_OPT_TYPE_STRING, DEFAULT, CHAR_MIN, CHAR_MAX},
> > +{"flags", "set flags", OFFSET(flags), FF_OPT_TYPE_FLAGS, DEFAULT, 0, INT_MAX, 0, "flags"},
> > +{"cool", "set cool flag ", 0, FF_OPT_TYPE_CONST, TEST_FLAG_COOL, INT_MIN, INT_MAX, 0, "flags"},
> > +{"lame", "set lame flag ", 0, FF_OPT_TYPE_CONST, TEST_FLAG_LAME, INT_MIN, INT_MAX, 0, "flags"},
> > +{"mu", "set mu flag ", 0, FF_OPT_TYPE_CONST, TEST_FLAG_MU, INT_MIN, INT_MAX, 0, "flags"},
> > +{"rational", "set rational", OFFSET(rational), FF_OPT_TYPE_RATIONAL, DEFAULT, 0, 10},
> > +{NULL},
> > +};
> 
> vertical align

Fixed.

> > +
> > +AV_DEFINE_CLASS(test);
> > +
> >  int main()
> >  {
> >      int i;
> 
> ive not approved AV_DEFINE_CLASS() yet

Removed.
 
> > @@ -319,6 +418,36 @@
> >          }
> >      }
> >  
> > +    printf("\nTesting av_set_options_string()\n");
> > +    {
> > +        TestContext test_ctx;
> > +        const char *options[] = {
> > +            "",
> > +            "foo=",
> > +            "foo",
> > +            "foo=val",
> > +            "toggle=1:foo",
> > +            "toggle=100",
> > +            "flags=+mu-lame:num=42:toggle=0",
> > +            "num=42:string=blahblah",
> > +            "rational=0:rational=1/2:rational=1/-1",
> > +            "rational=-1/0",
> > +        };
> 
> a little whitespacein the strings would help readability

Fixed. 

> [...]
> > Index: libavfilter-soc/ffmpeg/libavfilter/parseutils.h
> > ===================================================================
> > --- libavfilter-soc.orig/ffmpeg/libavfilter/parseutils.h	2009-05-06 00:28:08.000000000 +0200
> > +++ libavfilter-soc/ffmpeg/libavfilter/parseutils.h	2009-05-06 00:29:32.000000000 +0200
> > @@ -63,4 +63,21 @@
> >      options                                         \
> >  }
> >  
> > +/**
> > + * Parses the key/value pairs list in opts. For each key/value pair
> > + * found, stores the value in the field in ctx that is named like the
> > + * key. ctx must be an AVClass context, storing is done using
> > + * AVOptions.
> > + *
> > + * @param key_val_sep a 0-terminated list of the characters used to
> > + * separate key from value
> 
> > + * @param pairs_sep a 0-terminated list of the characters used to
> > + * separate key/value pairs
> 
> seperate 2 ...

Added some more test cases, result is now:

Testing av_set_options_string()
[test @ 0xbfa56988]Setting options string ''

[test @ 0xbfa56988]Setting options string ':'
[test @ 0xbfa56988]Missing key or no key/value separator found after key ':'
[test @ 0xbfa56988]Error setting options string: ':'

[test @ 0xbfa56988]Setting options string '='
[test @ 0xbfa56988]Missing key or no key/value separator found after key ''
[test @ 0xbfa56988]Error setting options string: '='

[test @ 0xbfa56988]Setting options string 'foo  =  :'
[test @ 0xbfa56988]Setting value '' for key 'foo'
[test @ 0xbfa56988]Unknown option key 'foo'
[test @ 0xbfa56988]Error setting options string: 'foo  =  :'

[test @ 0xbfa56988]Setting options string ':  =  foo'
[test @ 0xbfa56988]Setting value 'foo' for key ':'
[test @ 0xbfa56988]Unknown option key ':'
[test @ 0xbfa56988]Error setting options string: ':  =  foo'

[test @ 0xbfa56988]Setting options string '=  foo'
[test @ 0xbfa56988]Missing key or no key/value separator found after key ''
[test @ 0xbfa56988]Error setting options string: '=  foo'

[test @ 0xbfa56988]Setting options string 'foo='
[test @ 0xbfa56988]Setting value '' for key 'foo'
[test @ 0xbfa56988]Unknown option key 'foo'
[test @ 0xbfa56988]Error setting options string: 'foo='

[test @ 0xbfa56988]Setting options string 'foo'
[test @ 0xbfa56988]Missing key or no key/value separator found after key 'foo'
[test @ 0xbfa56988]Error setting options string: 'foo'

[test @ 0xbfa56988]Setting options string 'foo=val'
[test @ 0xbfa56988]Setting value 'val' for key 'foo'
[test @ 0xbfa56988]Unknown option key 'foo'
[test @ 0xbfa56988]Error setting options string: 'foo=val'

[test @ 0xbfa56988]Setting options string 'foo==val'
[test @ 0xbfa56988]Setting value '=val' for key 'foo'
[test @ 0xbfa56988]Unknown option key 'foo'
[test @ 0xbfa56988]Error setting options string: 'foo==val'

[test @ 0xbfa56988]Setting options string 'toggle=:'
[test @ 0xbfa56988]Setting value '' for key 'toggle'
Unable to parse option value "": undefined constant or missing (
[test @ 0xbfa56988]Invalid value '' for key 'toggle'
[test @ 0xbfa56988]Error setting options string: 'toggle=:'

[test @ 0xbfa56988]Setting options string 'string=:'
[test @ 0xbfa56988]Setting value '' for key 'string'

[test @ 0xbfa56988]Setting options string 'toggle=1 : foo'
[test @ 0xbfa56988]Setting value '1' for key 'toggle'
[test @ 0xbfa56988]Missing key or no key/value separator found after key 'foo'
[test @ 0xbfa56988]Error setting options string: 'toggle=1 : foo'

[test @ 0xbfa56988]Setting options string 'toggle=100'
[test @ 0xbfa56988]Setting value '100' for key 'toggle'
Value 100.000000 for parameter 'toggle' out of range
[test @ 0xbfa56988]Invalid value '100' for key 'toggle'
[test @ 0xbfa56988]Error setting options string: 'toggle=100'

[test @ 0xbfa56988]Setting options string 'toggle==1'
[test @ 0xbfa56988]Setting value '=1' for key 'toggle'
Unable to parse option value "=1": undefined constant or missing (
[test @ 0xbfa56988]Invalid value '=1' for key 'toggle'
[test @ 0xbfa56988]Error setting options string: 'toggle==1'

[test @ 0xbfa56988]Setting options string 'flags=+mu-lame : num=42: toggle=0'
[test @ 0xbfa56988]Setting value '+mu-lame' for key 'flags'
[test @ 0xbfa56988]Setting value '42' for key 'num'
[test @ 0xbfa56988]Setting value '0' for key 'toggle'

[test @ 0xbfa56988]Setting options string 'num=42 : string=blahblah'
[test @ 0xbfa56988]Setting value '42' for key 'num'
[test @ 0xbfa56988]Setting value 'blahblah' for key 'string'

[test @ 0xbfa56988]Setting options string 'rational=0 : rational=1/2 : rational=1/-1'
[test @ 0xbfa56988]Setting value '0' for key 'rational'
[test @ 0xbfa56988]Setting value '1/2' for key 'rational'
[test @ 0xbfa56988]Setting value '1/-1' for key 'rational'
Unable to parse option value "1/-1": undefined constant or missing (
[test @ 0xbfa56988]Invalid value '1/-1' for key 'rational'
[test @ 0xbfa56988]Error setting options string: 'rational=0 : rational=1/2 : rational=1/-1'

[test @ 0xbfa56988]Setting options string 'rational=-1/0'
[test @ 0xbfa56988]Setting value '-1/0' for key 'rational'
Value -inf for parameter 'rational' out of range
[test @ 0xbfa56988]Invalid value '-1/0' for key 'rational'
[test @ 0xbfa56988]Error setting options string: 'rational=-1/0'

Regards.
-- 
FFmpeg = Fundamentalist Faithless Multimedia Power Elaborated Gadget
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parseutils-implement-set-options.patch
Type: text/x-diff
Size: 6509 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090506/9494a671/attachment.patch>