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

Vitor Sessak vitor1001
Thu Apr 10 20:55:24 CEST 2008


Michael Niedermayer wrote:
> On Sun, Apr 06, 2008 at 10:39:00PM +0200, Vitor Sessak wrote:
>> Hi
>>
>> Michael Niedermayer wrote:
>>> On Tue, Apr 01, 2008 at 10:49:39PM +0200, Vitor Sessak wrote:
>>> [...]
>>>>> [...]
>>>>>>     char tmp[20];
>>>>>>
>>>>>>     snprintf(tmp, 20, "%d", index);
>>>>>                     ^^
>>>>> sizeof
>>>> Agreed. But are you ok with the instance name/instance lookup logic here?
>>> Well as you mention it now, i realize what you are actually doing there.
>>> No i do not like this design at all.
>>> What i dont understand is why dont you just use pointers to 
>>> AVFilterContext
>>> instead of these numbers in char* identifers? I mean without looking
>>> at the code common sense tells me whereever you store the numbers you 
>>> could
>>> store pointers to AVFilterContext as well.
>> Well, it made more sense before the new parser was written. Changed that.
>>
>>>>>>     if(!(filterdef = avfilter_get_by_name(name)) ||
>>>>>>        !(filt = avfilter_open(filterdef, tmp))) {
>>>>>>         av_log(&log_ctx, AV_LOG_ERROR,
>>>>>>                "error creating filter '%s'\n", name);
>>>>> The purpose of the context is so the user can associate it with some
>>>>> specific instance of something. using a global context defeats this.
>>>> Yes. Any suggestion?
>>> Its not very important ...
>>> The solution is to put that AVClass in some context, something related to
>>> the parser ...
>> I don't see any candidates...
>>
>> Another change is that I changed the notation of labels to '[in]' to allow 
>> implementing parenthesis later.
> 
> [...]
> 
>>     if (avfilter_graph_add_filter(ctx, filt) < 0)
>>         return NULL;
>>
>>     if(avfilter_init_filter(filt, args, NULL)) {
> 
> the whitespace betweem if and ( looks inconsistant

fixed

> [...]
>> /**
>>  * Consumes a string from *buf.
>>  * @return a copy of the consumed string, which should be free'd after use
>>  */
>> static char *consume_string(const char **buf)
>> {
>>     char *out = av_malloc(strlen(*buf));
> 
> strlen+1 (for the zero at the end) ?

fixed, 10l

>>     const char *in = *buf;
>>     char *ret = out;
>>
>>     consume_whitespace(buf);
>>
>>     do{
>>         char c = *in++;
>>         switch (c) {
>>         case '\\':
>>             *out++= *in++;
>>             break;
>>         case '\'':
>>             while(*in && *in != '\'')
>>                 *out++= *in++;
>>             if(*in) in++;
>>             break;
>>         case 0:
>>         case ']':
>>         case '[':
>>         case '=':
>>         case ',':
> 
>>             *out++= 0;
>>             break;
>>         default:
>>             *out++= c;
> 
> i wonder if 
>     c=0;
> defailt:
>     *out++= c;
> 
> would be too hackish ...

I don't think it is hackish, but I have the tendency of "seeing" a 
nonexistent "break;" when I read such code...

> [...]
>> /**
>>  * Parse "(linkname)"
>>  * @arg name a pointer (that need to be free'd after use) to the name between
>>  *           parenthesis
>>  */
>> static void parse_link_name(const char **buf, char **name)
>> {
>>     (*buf)++;
>>
>>     *name = consume_string(buf);
>>
>>     if (!*name[0])
>>         goto fail;
>>
>>     if (*(*buf)++ != ']')
>>         goto fail;
>>
>>     return;
>>  fail:
>>     av_freep(name);
> 
>>     av_log(&log_ctx, AV_LOG_ERROR, "Could not parse link name!\n");
>> }
> 
> These are the kind of error messages people love in long complicated commands
> id suggest to also print buf or something.
> And possibly this could be done more generically, that is always print an
> error message with the remaining string. Of course only if this is simpler
> than custom av_log calls with specific arguments for each case.

It's better now, but I really don't like to print the remaining string. 
For unmatched '[', for example, the remaining string can be just void. 
In this function, I show the start of the string been parsed, should be 
pretty clear.

> [...]
>> /**
>>  * A linked-list of the inputs/outputs of the filter chain.
>>  */
>> typedef struct AVFilterInOut {
>>     enum LinkType type;
>>     char *name;
>>     AVFilterContext *filter;
>>     int pad_idx;
>>
>>     struct AVFilterInOut *next;
>> } AVFilterInOut;
> 
> I wonder if an array would be simpler than a linked list ...

I'm not sure it would be really simpler. For me, it will only save one 
or two malloc lines...

>> static void free_inout(AVFilterInOut *head)
>> {
>>     while (head) {
> 
>>         AVFilterInOut *next;
>>         next = head->next;
> 
> declaration and statement can be merged
> 
> 
>>         av_free(head);
>>         head = next;
>>     }
>> }
>>
> 
>> /**
>>  * Parse "(a1)(link2) ... (etc)"
> 
> hmm [] () ?

Fixed

>>  */
>> static int parse_inouts(const char **buf, AVFilterInOut **inout, int firstpad,
>>                         enum LinkType type, AVFilterContext *filter)
>> {
> 
>>     int pad = firstpad;
> 
> redundant

fixed

>>     while (**buf == '[') {
>>         AVFilterInOut *inoutn = av_malloc(sizeof(AVFilterInOut));
>>         parse_link_name(buf, &inoutn->name);
>>         inoutn->type = type;
>>         inoutn->filter = filter;
>>         inoutn->pad_idx = pad++;
>>         inoutn->next = *inout;
>>         *inout = inoutn;
>>     }
> 
> what about [a b c] and such?

See below

>>     return pad;
>> }
>>
> 
>> static const char *skip_inouts(const char *buf)
>> {
>>     while (*buf == '[') {
> 
>>         buf += strcspn(buf, "]");
>>         buf++;
> 
> buf += strcspn(buf, "]") + 1;
> 
> 
>>     }
> 
> superflous {}

fixed both

> [...]
>>         // If the first filter has an input and none was given, it is
>>         // implicitly the input of the whole graph.
>>         if (pad == 0 && filter->input_count == 1) {
>>             if(link_filter(in, inpad, filter, 0))
>>                 goto fail;
>>         }
> 
> so ',' links only one pad and ';' links nothing ?

Yes. That was my original syntax (without any extension like the '*'). 
My timeline is:

1- Define a decent syntax for graphs (mostly done now)
2- Get a parser commited with the following properties:
     a) Compatibility with syntax decided in (1)
     b) Flexible enough to describe any graph
     c) Simple enough not to delay too much lavfi inclusion
3- Finish lavfi inclusion in svn
4- Make --enable-avfilter default in svn
5- Allows for several inputs/outputs in ffmpeg.c
6- Extend parser to do all decided in (1). It can be done in small 
incremental steps.

That is why this parser don't do all that was decided in the ML...

Also, are you ok to commit it without svn history? It was rewritten from 
scratch...

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 8860 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080410/d4edf68a/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.h
Type: text/x-chdr
Size: 1550 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080410/d4edf68a/attachment.h>



More information about the ffmpeg-devel mailing list