[FFmpeg-devel] [PATCH] Implement options parsing for avfilter filters

[Stefano Sabatini]

On date Sunday 2009-04-19 04:21:39 +0200, Michael Niedermayer encoded:
> On Sun, Apr 19, 2009 at 01:23:27AM +0200, Stefano Sabatini wrote:
> > Hi,
> > as recently discussed, plus application to the libavfilter-soc scale
> > filter.
> > 
> > I'm not very happy with the parse_options.[hc] name, suggestions are
> > welcome.
> 
> [...]
> 
> > +static int consume_whitespace(const char *buf)
> > +{
> > +    return strspn(buf, " \n\t");
> > +}
> 
> useless wraper function

Replaced by a macro, maybe is not what you meant. At least we should
factorize the definition of whitespaces.

> > +
> > +/**
> > + * Consumes a string from *buf.
> 
> no it unescapes the string from buf until one of several undocumenetd
> chars
> please document the code and use sane function names.
> 
> rule of thumb, if the code cannot be used OR the function cannot be
> implemented purely based on the documentation then the documentation is
> crap.

OK, just note that I copied code and names from graphparser.c, so
maybe we should clean-up that too.
 
> > + * @return a copy of the consumed string, which should be free'd after use
> > + */
> > +static char *consume_string(const char **buf)
> > +{
> > +    char *out = av_malloc(strlen(*buf) + 1);
> > +    char *ret = out;
> > +
> > +    *buf += consume_whitespace(*buf);
> > +
> > +    do {
> > +        char c = *(*buf)++;
> > +        switch (c) {
> > +        case 0:
> > +        case '=':
> > +        case ':':
> > +            *out++ = 0;
> > +            break;
> > +        default:
> > +            *out++ = c;
> > +        }
> > +    } while(out[-1]);
> > +
> > +    (*buf)--;
> > +    *buf += consume_whitespace(*buf);
> > +
> > +    return ret;
> > +}
> > +

While I was at it I simplified the *(*buf)++ mess and implemented
support to '\' escaping, as done in graphparser.c. I wonder if I
should also support '\'' escaping.

As for now we have two level of escaping so for example an option may
be expressed like this:
... -vfilters "filter=key\=key1\\\\=value"

"filter=key\=key1\\\=value1"
-> first escaping (graphparser)
 filter = key=key1\=value1
-> second escaping:
          key = key1=value

not to speak about the shell baroque escaping rules, making in
practice the use of more than one level of escaping completely
ureliable.
 
> > +/**
> > + * Parses a key/value parameter of the form "filter=params",
> 
> makes sense

Uh, I meant "key=value".
 
> 
> >  and set
> > + * it in the class context.
> 
> makes no sense
> 
> 
> > + *
> > + * @return 0 if the parsing was successfull, a negative value otherwise
> > + */
> > +static int parse_option(void *ctx, const char **buf)
> 
> and call it parse_key_value_pair please

OK.
 
> [...]
> 
> > +#define CREATE_FILTER_CLASS(type,name)      \
> > +static const char *filter_name(void *ctx)   \
> > +{                                           \
> > +    return #name;                           \
> > +}                                           \
> > +static const AVClass type##_##name##_class = { \
> > +    #name,                                  \
> > +    filter_name,                            \
> > +    options                                 \
> > +}
> 
> i appreciate your attempt at simplifying code that one day might
> exist but until it does please remove this
> smaller patches are quicker to pass review ...

OK, split in a new patch.
 
> > +
> > +/**
> > + * Parses the option list in \p opts, and set their values in \p class.
> 
> i will not approve it with the \p, thats also true for all other doxy

I removed the "\p"-s from the docs, but we should find an agreement on
this.

Other patches are attached just as example, I'll repost them again for
review when this one will be applied if you find it impractical to
review them.

Regards.
-- 
FFmpeg = Faboulous and Freak Martial Peaceful Epic Gadget
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lavfi-add-parse-options.patch
Type: text/x-diff
Size: 6121 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/eb873f8d/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lavfi-create-filter-class.patch
Type: text/x-diff
Size: 906 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/eb873f8d/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: scale-add-parse-options-support.patch
Type: text/x-diff
Size: 2181 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/eb873f8d/attachment-0002.patch>