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

[Vitor Sessak]

Hi, and thanks for the review

Michael Niedermayer wrote:
> On Thu, Apr 10, 2008 at 08:55:24PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Sun, Apr 06, 2008 at 10:39:00PM +0200, Vitor Sessak wrote:
> [...]
>>> [...]
>>>> /**
>>>>  * 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...
> 
> Well, i will think more about this, but as is the parser currently looks
> too complex for what it supports ...

I agree it looks kind of complex for what it does, but a good part of 
the complexity comes from whitespace handling and giving decent error 
messages.

> [...]
>> Also, are you ok to commit it without svn history? It was rewritten from 
>> scratch...
> 
> Iam ok with ommiting unrelated history / what is prior to your rewrite ...
> 
> 
> [...]
>> /**
>>  * For use in av_log
>>  */
>> static const char *log_name(void *p)
>> {
>>     return "Filter parser";
>> }
>>
>> static const AVClass filter_parser_class = {
>>     "Filter parser",
>>     log_name
>> };
>>
>> static const AVClass *log_ctx = &filter_parser_class;
> 
> I do not like this. This is a hack not the correct way to use av_log()

Options:

1- Make AVFilterGraph a context for av_log
     a) Good points: Instance specific
     b) Bad  points: Has nothing specifically to do with the parser

2- Make a AVFilterInOut linked list a member of a context
     a) Good points: The best candidate for a parser context
     b) Bad  points: Useless struct

4- Malloc'ing a instance of AVClass in the beginning of 
avfilter_parse_graph and passing it to every function
     a) Good points: Instance specific
     b) Bad  points: Useless function parameters

5- av_log(NULL, ...)

>> static AVFilterContext *create_filter(AVFilterGraph *ctx, int index,
>>                                       char *name, char *args)
> 
> shouldnt name and args be const

Yes

> 
> [...]
>> /**
>>  * 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) + 1);
>>     const char *in = *buf;
>>     char *ret = out;
>>
>>     consume_whitespace(buf);
>>
>>     do{
>>         char c = *in++;
> 
> you change buf but use in, also why arent you useing just one variable ...

My original idea was to avoid constructs like *(*buf)++ = , but I'm not 
against it, so changed.

> 
>>         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;
>>         }
>>     } while(out[-1]);
> 
> What about trailing whitespace?

Weren't handled properly (it was a nice thing of consume_char()). Fixed.

> 
> 
>>     *buf = in-1;
>>     return ret;
>> }
>>
> 
>> /**
>>  * 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)
>> {
>>     const char *start = *buf;
>>     (*buf)++;
>>
>>     *name = consume_string(buf);
>>
>>     if(!*name[0]) {
>>         av_log(&log_ctx, AV_LOG_ERROR,
>>                "Bad (empty?) label found in the following: \"%s\".\n", start);
>>         goto fail;
>>     }
>>
>>     if(*(*buf)++ != ']') {
>>         av_log(&log_ctx, AV_LOG_ERROR,
>>                "Mismatched '[' found in the following: \"%s\".\n", start);
>>         goto fail;
>>     }
>>
>>     return;
>>
>>  fail:
>>     av_freep(name);
>> }
> 
> you can at least move the fail: free to where the second goto is, this also
> makes the return redundant.

consume_string() will alloc a string in both cases (an empty one in the 
first goto).

> [...]
>> /**
>>  * Parse "[a1][link2] ... [etc]"
>>  */
>> static int parse_inouts(const char **buf, AVFilterInOut **inout, int pad,
>>                         enum LinkType type, AVFilterContext *filter)
>> {
>>     while (**buf == '[') {
>>         AVFilterInOut *inoutn = av_malloc(sizeof(AVFilterInOut));
>>         parse_link_name(buf, &inoutn->name);
>>
>>         if (!inoutn->name) {
>>             av_free(inoutn);
>>             return -1;
>>         }
>>
> 
>>         inoutn->type = type;
>>         inoutn->filter = filter;
>>         inoutn->pad_idx = pad++;
>>         inoutn->next = *inout;
> 
> vertical align

done

> 
> [...]
>> /**
>>  * Parse a string describing a filter graph.
>>  */
>> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
>>                          AVFilterContext *in, int inpad,
>>                          AVFilterContext *out, int outpad)
>> {
>>     AVFilterInOut *inout=NULL;
>>     AVFilterInOut  *head=NULL;
>>
>>     int index = 0;
>>     char chr = 0;
>>     int pad = 0;
>>     int has_out = 0;
>>
>>     AVFilterContext *last_filt = NULL;
>>
>>     consume_whitespace(&filters);
>>
>>     do {
>>         AVFilterContext *filter;
>>         int oldpad = pad;
>>         const char *inouts = filters;
>>
>>         // We need to parse the inputs of the filter after we create it, so
>>         // skip it by now
>>         filters = skip_inouts(filters);
>>
>>         if(!(filter = parse_filter(&filters, graph, index)))
>>             goto fail;
>>
>>         pad = parse_inouts(&inouts, &inout, chr == ',', LinkTypeIn, filter);
>>
>>         if(pad < 0)
>>             goto fail;
>>
>>         // 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;
>>         }
>>
>>         if(chr == ',') {
>>             if(link_filter(last_filt, oldpad, filter, 0) < 0)
>>                 goto fail;
>>         }
>>
>>         pad = parse_inouts(&filters, &inout, 0, LinkTypeOut, filter);
>>         chr = *filters++;
>>         index++;
>>         last_filt = filter;
>>     } while (chr == ',' || chr == ';');
>>
>>     head = inout;
>>     for (; inout != NULL; inout = inout->next) {
> 
> Why dont you build the graph in 1 pass?

Why? In the 2 pass version, I can be sure to find a match for every 
label. Also, it makes two nicely separated blocks of code, the linking 
can be cleanly moved to another function, for example.

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: avfiltergraph.c
Type: text/x-csrc
Size: 4871 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080411/eb53af7d/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avfiltergraph.h
Type: text/x-chdr
Size: 1624 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080411/eb53af7d/attachment.h>