[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