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

[Vitor Sessak]

Hi

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

Ok, changed.

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

My idea was to use it for anything other than the filter parameters 
(there is something wrong if the filter/link name is escaped). But maybe 
it don't justify the additional complexity.

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

I like this idea. It allows for things like

(tmp)overlay,vflip,split(tmp)

as an alias to

(in)(tmp)overlay,vflip,split(tmp)(out)

The only problem would be searching for (in), from all the possible cases:

(    in   )vflip(out)

(
   in   )vflip(out)

(in) vflip(out)

drawtext='text in pic'

are you ok with the extra complexity it'll add? And I cannot test for it 
during parsing, since I have to set the input pad indexes for the first 
filter, ie, in

(tmp)overlay,vflip,split(tmp)

I have to set

link.name="tmp";
link.type=input;
link.pad = 1; // And not 0, since in pad 0 there is the input!

and to know (tmp) is in the second pad, I have to know beforehand that 
there is no (in) in the chain...

-Vitor