[FFmpeg-devel] [PATCH] Implement options parsing for avfilter filters
Michael Niedermayer
michaelni
Sun Apr 19 20:24:13 CEST 2009
On Sun, Apr 19, 2009 at 06:37:13PM +0200, Stefano Sabatini wrote:
> 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.
'=' 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
[...]
> Index: libavfilter-soc/ffmpeg/libavfilter/parse_options.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ libavfilter-soc/ffmpeg/libavfilter/parse_options.c 2009-04-19 18:29:06.000000000 +0200
> @@ -0,0 +1,123 @@
> +/*
> + * copyright (c) 2009 Stefano Sabatini
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file libavfilter/parse_options.c
> + * filter options parsing utils
> + */
> +
> +#include <string.h>
> +#include "parse_options.h"
> +
> +#define WHITESPACES " \n\t"
> +
> +char *av_consume_string(const char **buf, const char *term)
the function name is no good
unescape, 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?
> + 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
> +
> +/**
> + * 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
[...]
> +#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 :)
so either remove this claim or change the code, i actually dont
even know which is better, i guess just drop the comment
> Leading and trailing whitespaces are
> + * 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
> + */
> +static char *av_consume_string(const char **buf, const char *term);
> +
> +/**
> + * 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
also the = : chars should be arguments not hardcoded
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/c1e01bed/attachment.pgp>
More information about the ffmpeg-devel
mailing list