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

Stefano Sabatini stefano.sabatini-lala
Sun Apr 19 18:37:13 CEST 2009


On date Sunday 2009-04-19 15:46:42 +0200, Michael Niedermayer encoded:
> 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

OK.

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

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.

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

Extended av_consume_string(), we can factorize it in a following
patch.
  
> >  
> > > > +/**
> > > > + * 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

All fixed according to your indications.

Regards.
-- 
FFmpeg = Fierce Freak Muttering Picky Elegant Gangster
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lavfi-add-parse-options.patch
Type: text/x-diff
Size: 6420 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090419/1686ebf4/attachment.patch>



More information about the ffmpeg-devel mailing list