[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Sat Apr 12 16:40:24 CEST 2008
Hi
Michael Niedermayer wrote:
> 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
Ok, I'm with 6.
> 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 :)
10l
>
>
>>>> 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);
> }
> }
Ok, I got it. 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.
>
> It makes the code more complex ...
It wasn't obvious at first it would be less complex after merging, but I
think I succeeded in simplifying it a bit by doing so...
>
>> /*
>> * filter graphs
>
> typo
> or was there something else wrong hmmmmmmm ;)
Wow, 100l
A final question: should I, instead of having as a prototype
int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
AVFilterContext *in, int inpad,
AVFilterContext *out, int outpad,
AVClass *log_ctx);
use
int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
AVFilterContext **in, int *inpad,
AVFilterContext **out, int *outpad,
AVClass *log_ctx);
So I could extend it later to have several inputs/outputs without
changing the public API? Are you ok with a NULL-termined array of
(AVFilterContext *) for several inputs?
-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 9633 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080412/80f78d42/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.h
Type: text/x-chdr
Size: 1643 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080412/80f78d42/attachment.h>
More information about the ffmpeg-devel
mailing list