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

[Vitor Sessak]

Hi

Michael Niedermayer wrote:
> Hi
> 
> On Sun, Mar 16, 2008 at 07:57:29PM +0100, Vitor Sessak wrote:
> [...]> >>>
>>>>>> 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.
> 
> iam for avoiding extra complexity

Done.

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

Done.

>>>
>>>>> [...]
>>>>>>     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...
> 
> Maybe its crazy but, why dont you just parse the string twice once with
> and once without in/out ? One of these should fail due to duplicate in/out

Works fine, without too much additional complexity.

> Anyway i suspect we can cleanup the parser a bit more, my review just stoped
> early as i was confused by the "escaped" and thought its simpler to ask what
> its good for ...

Well, that first message was, as I said, more of a RFC than a patch. 
Now, I'm attaching a more cleaned up version for reviewing...

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: avfiltergraphdesc.c
Type: text/x-csrc
Size: 9167 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080317/8854a527/attachment.c>