[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