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

[Michael Niedermayer]

On Sun, Apr 19, 2009 at 12:18:48PM +0200, Stefano Sabatini wrote:
> 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.

no, i want strspn() be used directly
strspn(buf, " \n\t");
is clear, anyone knowing C should know or at least guess what it is
consume_whitespace() is not clear especially with the other uses
of the word consumed in other functions


> 
> > > +
> > > +/**
> > > + * 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.

yes, i noticed, and graphparser likely should be removed from ffmpeg svn
i back then just accepted it because it appeared to hold up other work
of merging libavfilter and the reviews completely failed to improve
graphparser (that may have been partly my fault too in not being able
to suggest how to improve the code ....)


>  
> > > + * @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.

pick seperator chars that dont need escaping please.

also "consume_string" should be shared between graphparser and your
code, well i guess graphparser should use parse_options.c/h


>  
> > > +/**
> > > + * 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.

if you want code approved -> no \p
you are free to start a discussion about it if you disagree, maybe people
want unreadable doxy tags in comments that primarely are used without doxygen
though i doubt it


[...]

> +/**
> + * Reads from buf a key or value string, stripping the leading and
> + * trailing chars.

no
at no point is this function depening on its argument being a key or
value.


> + * The terminating chars are '=' and ':', the '\' char is used for
> + * escaping the next char after it.

no


my try:
Unescapes the given string until a non escaped terminating char.

The normal \ and ' escaping is supported, special chars can also
be escaped by duplicating them. Leading and trailing whitespace is
removed.

@param term 0 terminated list of terminating chars 
@param buf buffer to parse, buf will be updated to point to the
           terminating char

@return the malloced unescaped string, which must be av_freed by the user


> + *
> + * @param buf the pointer to the buffer to parse, puts here the
> + * pointer to the character just after the parsed string
> + * @return a copy of the consumed string, which should be freed after
> + * use
> + */
> +static char *parse_key_or_value(const char **buf)
> +{
> +    char *out = av_malloc(strlen(*buf) + 1);
> +    char *ret = out;
> +    char *p = *buf;
> +    p += CONSUME_WHITESPACES(p);
> +
> +    do {
> +        char c = *p++;
> +        switch (c) {
> +        case '\\':
> +            *out++ = *p++;
> +        case 0:
> +        case '=':
> +        case ':':
> +            *out++ = 0;
> +            break;
> +        default:
> +            *out++ = c;
> +        }
> +    } while(out[-1]);
> +
> +    p--;
> +    p += CONSUME_WHITESPACES(p);
> +    *buf = p;
> +
> +    return ret;
> +}
> +
> +/**
> + * Parses a key/value pair of the form "key=value".

> + * If a key/value string pairs is found, sets in the AVClass context
> + * ctx the option with name key using the string in the value.

uhmrwell ...
i know what the function does still i cant make sense of this

here is a variant that should be readable by humans

Stores the value in the field in ctx that is named like key.
ctx must be a AVClass context, storing is done using AVOptions


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/f5171fd2/attachment.pgp>