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

[Stefano Sabatini]

On date Sunday 2009-04-19 20:24:13 +0200, Michael Niedermayer encoded:
> On Sun, Apr 19, 2009 at 06:37:13PM +0200, Stefano Sabatini wrote:
[...]
> > Mmh, do you mean for key=val pairs?
> > 
> > What about to use the '?' to separate filter from params?
> > 
> > filter?key1=val1:key2=val2:...:keyN=valN
> > 
> > The '=' for key=val pairs seems the more natural choice.
> 
> '=' is not a symbol that needs escaping in the parameter string
> 
> only terminating symbols and \ ' need escaping
> 
> scale=key1=val1:key2=val2
> is not ambigous
> 
> in that sense i dont think much escaping should be needed

Yet maybe such a syntax:
filter?key1=val1:key2=val2:...:keyN=valN

may simplify the graphparser parsing, and also drop the need for
escaping the = chars.

[...]
> > +char *av_consume_string(const char **buf, const char *term)
> 
> the function name is no good
> unescape, get_token ...

av_get_token()?
 
> 
> > +{
> > +    char *out = av_malloc(strlen(*buf) + 1);
> > +    char *ret = out;
> > +    const char *p = *buf;
> > +    p += strspn(p, WHITESPACES);
> > +
> > +    do {
> > +        char c = *p++;
> > +        switch (c) {
> > +        case '\\':
> > +            *out++ = *p++;
> > +        case '\'':
> 
> are you missing a break here?

Fixed locally.

> > +            while(*p && *p != '\'')
> > +                *out++ = *p++;
> > +            if(*p)
> > +                p++;
> > +            break;
> > +        default:
> > +            if (!c || strspn((p-1), term))
> > +                *out++ = 0;
> > +            else
> > +                *out++ = c;
> > +        }
> > +    } while(out[-1]);
> > +
> > +    p--;
> > +    p += strspn(p, WHITESPACES);
> > +    *buf = p;
> > +
> > +    return ret;
> > +}
> > +
> > +#define TERMINATING_CHARS ":=\n"
> 
> i dont see why \n is one besides they depend on key vs. value

Mmh, OK.

> 
> > +
> > +/**
> > + * Stores the value in the field in ctx that is named like key.
> > + * ctx must be a AVClass context, storing is done using AVOptions
> > + *
> > + * @param buf buffer to parse, buf will be updated to point to the
> > + * character just after the parsed string
> 
> > + * @return 0 if successfull, a negative value otherwise
> 
> >=0 if sucessfull

OK. 

> [...]
> > +#ifndef AVFILTER_PARSE_OPTIONS
> > +#define AVFILTER_PARSE_OPTIONS
> > +
> > +#include "libavcodec/opt.h"
> > +
> > +/**
> > + * Unescapes the given string until a non escaped terminating char.
> > + *
> 
> > + * The normal \ and ' escaping is supported, 
> 
> that likely should also be controled by a argument
> 
> 
> > special chars can also be
> > + * escaped by duplicating them.
> 
> really? i know i wrote it but you didnt change to code to actually
> support it :)

That's true at least for '\\'.

> so either remove this claim or change the code, i actually dont
> even know which is better, i guess just drop the comment

Yes I prefer to drop it.

[...]
> > +/**
> > + * Parses the options in opts.
> > + *
> > + * opts contains a list of "key=value" pairs separated by the ":"
> > + * chars. The '=' and ':' chars can be escaped using the '\' char.
> > + *
> > + * For each key/value pair found, stores the value in the field in ctx
> > + * that is named like key. ctx must be an AVClass context, storing is
> > + * done using AVOptions.
> > + *
> > + * @return 0 if successfull, a negative number otherwise
> > + */
> > +int avfilter_parse_options(void *ctx, const char *opts);
> 
> no, av_ not avfilter, this code is not a filter or in any way
> related to avfilter beyond just being using by avfilters at first

This suggests a better name for the files, parseutils.[hc], but then I
wonder lavfi is not the better location for them.

And since they also require opt.[hc] then the better place looks lavc.
 
> also the = : chars should be arguments not hardcoded

Something like param_separator, option_term?
Do you prefer them to be a single char or a list of chars?

Regards.
-- 
FFmpeg = Frenzy and Fiendish Mysterious Patchable Encoding/decoding Gem