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

Michael Niedermayer michaelni
Fri Apr 11 23:58:26 CEST 2008


On Fri, Apr 11, 2008 at 06:43:10PM +0200, Vitor Sessak wrote:
> 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, ...)

6 - let the user provide a AVClass / NULL
7 - return the error message per char ** instead of through av_log()

4 and 5 are unacceptable
i do like 3 ... especially because there is no 3 :)


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

if you like sensless abstraction there is bytestream_get_byte()


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


        goto fail;
    ...
        goro fail;
    }
    return;
    fail:
    av_freep(name);
}
is the same as

        goto fail;
    ...
    fail:
        av_freep(name);
    }
}


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

It makes the code more complex ...

[...]

> /*
>  * filter graphs

typo
or was there something else wrong hmmmmmmm ;)


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20080411/3f39b3ec/attachment.pgp>



More information about the ffmpeg-devel mailing list