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

[Michael Niedermayer]

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


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

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

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

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/35bbaedc/attachment.pgp>