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

[Michael Niedermayer]

On Sun, Mar 16, 2008 at 05:09:37PM +0100, Vitor Sessak wrote:
> 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.

i do not like this design


> 
> 
> > 
> > 
> >> 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 why do we need the =0 case?


> 
> 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 

maybe


> ok to have no context?

no



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

what about:
()movie_src=test.avi, vflip, (in)overlay

() would override the default ...

or test for the occurance of (in) / (out) and add them at the begin/end only
if they dont occur anywhere. 

Anyway iam strongly against this special case syntax ...

and your fails with
eval='f(x,y)'

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/22db7d3c/attachment.pgp>