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

[Michael Niedermayer]

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


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


> + * @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;
> +}
> +


> +/**
> + * Parses a key/value parameter of the form "filter=params",

makes sense


>  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


[...]

> +#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 ...


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


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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/02f2ed17/attachment.pgp>