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

[Michael Niedermayer]

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


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



> 
> static AVFilterContext *create_filter(AVFilterGraph *ctx, int index,
>                                       char *name, char *args)

shouldnt name and args be const


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



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


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


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

[...]
> /**
>  * 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?

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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20080410/5e560ca2/attachment.pgp>