[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description

[Michael Niedermayer]

On Tue, Mar 18, 2008 at 09:04:18PM +0100, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Mon, Mar 17, 2008 at 08:12:08PM +0100, Vitor Sessak wrote:
>>> Hi
>>>
>>> Michael Niedermayer wrote:
>> [...]
>>> /**
>>>  * For use in av_log
>>>  */
>>> static const char *log_name(void *p)
>>> {
>>>     return "Filter parser";
>>> }
>>>
>>> static const AVClass filter_parser_class = {
>>>     "Filter parser",
>>>     log_name
>>> };
>>>
>>> static const AVClass *log_ctx = &filter_parser_class;
>> I was more thinking of passing a context as argument into the parser.
>
> Well, for example ffmpeg.c, which context will it pass to the parser? But 
> maybe a good idea would be passing a context and, if NULL is passed, using 
> log_ctx...

hmm, well, leave log_ctx for now this is pretty much irrelevant we dont really
need a user supplied ctx it was just an idea ...


>
>>> static void consume_whitespace(const char **buf)
>>> {
>>>     *buf += strspn(*buf, " \n\t");
>>> }
>>>
>>> /**
>>>  * get the next non-whitespace char
>>>  */
>>> static char consume_char(const char **buf)
>>> {
>>>     char out;
>>>     consume_whitespace(buf);
>>>
>>>     out = **buf;
>>>
>>>     if (out)
>>>         (*buf)++;
>>>
>>>     return out;
>>> }
>> is there some place which needs the extra-complex checks? or would:
>> consume_whitespace(buf);
>> return *(*buf)++;
>> work as well ?
>
> The idea is to have a safe parser. That way, you can call consume_char as 
> much as you like, it'll never go past the end of the string and 
> consume_string will never return NULL (but maybe an empty string).

In which case of consume_char() use can the removed check cause it
to go over the end of the buffer without a goto fail or similar first?


>
>>> /**
>>>  * remove the quotation marks from a string. Ex: "aaa'bb'cc" -> "aaabbcc"
>>>  */
>>> static void unquote(char *str)
>>> {
>>>     char *p1, *p2;
>>>     p1=p2=str;
>>>     while (p1[0] != 0) {
>>>         if (p1[0] == '\'') p1++;
>>>         p2[0] = p1[0];
>>>         p1++;
>>>         p2++;
>>>     }
>>>
>>>     p2[0] = 0;
>>> }
>> How do we support
>> drawtext=various special chars:$%&'"=\
>> ?
>
> $ ffmpeg -i in.avi -vfilters "drawtext='various special chars:$%&\"=\\'" 
> out.avi
>
> But there are two things that is complicated: the "'" char that can't be 
> escaped by now (and it's the only one) 

yes, thats why i brought it up ...


> and the ":" (that is not an especial 
> character to this parser, but is parsed independently by each filter).
>
> Maybe parsing the different args in the parser and instead of passing to 
> the filter something like
>
> static int init(AVFilterContext *ctx, const char *args, void *opaque)
>
> passing instead
>
> static int init(AVFilterContext *ctx, int argc, const char **argv, void 
> *opaque)
>
> ?

no, IMHO just pass the single string, its very easy for the filter to feed
this to sscanf()


[...]
>>>     start = *buf;
>>>
>>>     *buf += strcspn(*buf, " ()=,'");
>>>
>>>     if (**buf == '\'') {
>>>         char *p = strchr(*buf + 1, '\'');
>>>         if (p)
>>>             *buf = p + 1;
>>>         else
>>>             *buf += strlen(*buf); // Move the pointer to the null end 
>>> byte
>>>     }
>>>
>>>     size = *buf - start + 1;
>>>     ret = av_malloc(size*sizeof(char));
>>>     memcpy(ret, start, size - 1);
>>>     ret[size-1] = 0;
>>>
>>>     unquote(ret);
>> A unquote with src & dst seems more natural.
>
> Well, but should I allocate and free memory just for that?

currently you memcpy into dst and then
unquote(dst);
what i meant was
unquote(dst, src); which would avoid the memcpy and be cleaner IMHO


[...]
>>>         parse_link_name(buf, &inoutn->name);
>>>         inoutn->type = type;
>>>         inoutn->instance = instance;
>>>         inoutn->pad_idx = pad++;
>>>         inoutn->next = *inout;
>>>         *inout = inoutn;
>>>     }
>>>     return pad;
>>> }
>>>
>>> static AVFilterGraphDesc *parse_chain(const char *filters, int has_in)
>> Why does the parser work with AVFilterGraphDesc* instead of 
>> libavfilter/avfilter.h structs ?
>
> Good question. I didn't start yet to work with avfiltergraph.c, but I'd say 
> Bobby's idea was that a third party app using the library would prefer to 
> assemble its graphs using such a struct. Anyway, do you mind if I leave 
> this for when I send avfiltergraph.c for review?

Hmm, this is moving the review in a somewhat entangled situation.
I really do not like avfiltergraph.*. So if this
depends on it then the parser will have to wait for a clear demonstration of
the need of avfiltergraph.

The way i imagined it was that the parser would just create filters and links
as it parses them with no intermediate APIs.

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/20080318/e308bb17/attachment.pgp>