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

[Michael Niedermayer]

On Sat, Mar 15, 2008 at 09:29:03PM +0100, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Mon, Feb 25, 2008 at 06:36:40PM +0100, Vitor Sessak wrote:
>> [...]
>>>>> And for
>>>>>
>>>>>   in --> crop --> rotate --> vflip --> out
>>>>>
>>>>> crop=400:300,rotate=1,vflip,split
>>>> yes (in) (out) should be default at the ends
>>>>
>>>>
>>>>> meaning that when there is no semicolon, the (in) in the beginning and 
>>>>> the (out) in the end can be omitted. Also, I don't understand the point 
>>>>> of explicitly putting a "+" in new vertexes...
>>>> filter1(abc),(def)filter2
>>>>
>>>> filter1->(abc)
>>>> (def)->filter2
>>>>
>>>> filter1(+abc),(+def)filter2
>>>>
>>>> filter1->filter2
>>>>   |         ^
>>>>   v         |
>>>> (abc)     (def)
>>>>
>>>> again, this was all just an idea ...
>>>> your ';' achives the same, though it adds an additional char to the ones
>>>> needing escaping if used in filters. Iam not even sure if we shoud use 
>>>> ',' as
>>>> filter seperator ...
>>>> '|' would be an alternative but it would need filter chains to be under 
>>>> ""
>>>> though that applies to ; () [] as well
>>> I prefer the comma. I like the syntax
>>>
>>> ffmpeg -i in.avi -vfilters vflip,scale=300:400 out.avi
>>>
>>> so as for filter graphs that are just a chain we have a simple, unescaped 
>>> command line. If for more complex graphs "" are needed, it bother me 
>>> less.
>>>
>>>> other random ideas ...
>>>>
>>>> crop=400:300|[tmp1]picInPic=50:50|rotate=1|split[tmp2]|vflip[out];[tmp2]hflip|delay[tmp1]
>>>>
>>>> crop=400:300 | [tmp1]picInPic=50:50 | rotate=1 | split[tmp2] | 
>>>> vflip[out] ; [tmp2]hflip | delay[tmp1]
>>>>
>>>> crop=400:300 | [tmp1] picInPic=50:50 | rotate=1 | split [tmp2] | vflip 
>>>> [out] ; [tmp2] hflip | delay [tmp1]
>>> Wouldn't it be a good idea to ignore whitespace and newlines completely?
>> That was the idea, sorry ive just been testing how different whitespace
>> placements would look ...
>>>> crop=400:300 | [tmp1] picInPic=50:50 | rotate=1 | split {hflip | 
>>>> delay[tmp1];} | vflip
>>>>
>>>>
>>>>
>>>>> Finally, I prefer parenthesis instead of brackets... What do you think?
>>>> Passing eval.c equation like sin(5)*eq(a,b) might get trickier escaping
>>>> wise and parsing wise with (), besides this iam fine with () as well, 
>>>> that
>>>> is if you provide a solution to passing equations to eval based filters
>>>> without requireing each () to be escaped individually.
>>> I agree. But I'll have to add quotes to the syntax anyway for thing like
>>>
>>> vflip,drawtext='text, a   =)   :-]':arial:10:10,hflip
>> well mplayer survivied without proper quote handling, and that results in
>> eval stuff looking really nasty ...
>
> Well, this is more of a RFC than a patch, since parsers are not my strong 
> point. The file was rewritten almost from scratch. Now the it accepts 
> things like
>
>  (  in  )   split(i1),fifo,(i2)overlay=0:240(out);(i1)vflip(i2)
>
> or
>
> vflip,scale=10:30


[...]

> typedef struct Parser
> {
>     char next_chr;
>     char *buf;
> } Parser;

Could you explain what this next_chr thing is good for?
Iam no string parser expert but my gut feeling says char *buf would be enough
and if so we could avoid the struct.


> 
> static void consume_whitespace(Parser *p)
> {
>     while (p->next_chr == ' ' || p->next_chr == '\n' || p->next_chr == '\t')
>         p->next_chr = *(++p->buf);
> }

id replace all consume_whitespace() by
ptr += strspn(ptr, " \n\t");


[...]


> 
> static char *consume_string(Parser *p, int escaped)
> {

What does the "escaped" do? Document or remove it please!


[...]
> static int parse_link_name(Parser *p, char **name)
> {
>     if (consume_char(p) != '(')
>         goto fail;
> 
>     *name = consume_string(p,0);
> 
>     if (!*name[0])
>         goto fail;
> 
>     if (consume_char(p) != ')')
>         goto fail;
> 
>     return 0;
> 
>  fail:
>     *name = NULL;
>     return AVERROR_INVALIDDATA;
> }

We need some clear error messages for the various failures. Not only here
of course ...


[...]
>     single_chain = (strchr(filters, '(') == NULL);

why the special case?


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

There will always be a question for which you do not know the correct awnser.
-------------- 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/20080316/899a7a0d/attachment.pgp>