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

[Vitor Sessak]

Michael Niedermayer wrote:
> 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.

buf =  'test,blah\0'

To read the 'test' part in consume_string() without any memcpy, I put a 
\0 after 'test' in the buffer and return a pointer to the start of the 
string. But to do that, I have to overwrite the comma. So I put the 
comma in the next_chr var.


> 
> 
>> 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");

Looks a good idea, I'll look into it.

> 
> 
> [...]
> 
> 
>> static char *consume_string(Parser *p, int escaped)
>> {
> 
> What does the "escaped" do? Document or remove it please!

buf = "'bbb;ccc)ddd'"

consume_string(p, 1) == "bbb;ccc)ddd"
consume_string(p, 0) == "'bbb"

But I agree it should be documented.

> 
> 
> [...]
>> 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 ...

What should I do? av_log(NULL, AV_LOG_ERROR, "...")? Is in those cases 
ok to have no context?

> 
> 
> [...]
>>     single_chain = (strchr(filters, '(') == NULL);
> 
> why the special case?

To make

(in) vflip, hflip (out)

the same as

flip, hflip

without erroneously adding a '(in)' in the beginning of the following:

movie_src=test.avi, vflip, (in)overlay(out)

The syntax is that adding explicitly '(in)' and '(out)' is mandated for 
anything more complicated than a single chain.

-Vitor