[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Fri Apr 18 17:35:40 CEST 2008
Hi
Michael Niedermayer wrote:
> On Sat, Apr 12, 2008 at 04:40:24PM +0200, Vitor Sessak wrote:
>> 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?
>
> I like neither and i think that using filter list + pad index lists is
> not very convenient. This is especially the case in a more complex
> parser supporting more than "1 link from the previous filter".
> Thus i think the API might change when the parser is improved to support
> more of what was discussed.
> For the moment the choice between the 2 doesnt matter IMHO.
>
>
> [...]
>
>> /**
>> * Process a link. This funcion looks for a matching label in the *inout
>> * linked list. If none is found, it adds this link to the list.
>> */
>> static int handle_link(char *name, AVFilterInOut **inout, int pad,
>> enum LinkType type, AVFilterContext *filter,
>> AVClass *log_ctx)
>> {
>> AVFilterInOut *p = *inout;
>>
>> for (; p && strcmp(p->name, name); p = p->next);
>>
>> if(!p) {
>> // First label apearence, add it to the linked list
>> AVFilterInOut *inoutn = av_malloc(sizeof(AVFilterInOut));
>>
>> inoutn->name = name;
>> inoutn->type = type;
>> inoutn->filter = filter;
>> inoutn->pad_idx = pad;
>> inoutn->next = *inout;
>> *inout = inoutn;
>> return 0;
>> }
>>
>> if(p->type == LinkTypeIn && type == LinkTypeOut) {
>> if(link_filter(filter, pad, p->filter, p->pad_idx, log_ctx) < 0)
>> goto fail;
>> } else if(p->type == LinkTypeOut && type == LinkTypeIn) {
>> if(link_filter(p->filter, p->pad_idx, filter, pad, log_ctx) < 0)
>> goto fail;
>> } else {
>> av_log(log_ctx, AV_LOG_ERROR,
>> "Two links named '%s' are either both input or both output\n",
>> name);
>> goto fail;
>> }
>>
>> p->filter = NULL;
>>
>> return 0;
>> fail:
>> return -1;
>
> as return -1 is the only thing done after goto fail, you can as well replace
> all goto fail be it.
>
>
>> }
>>
>>
>
>> /**
>> * Parse "[a1][link2] ... [etc]"
>> */
>> static int parse_inouts(const char **buf, AVFilterInOut **inout, int pad,
>> enum LinkType type, AVFilterContext *filter,
>> AVClass *log_ctx)
>> {
>> while (**buf == '[') {
>> char *name;
>>
>> parse_link_name(buf, &name, log_ctx);
>>
>> if(!name)
>> return -1;
>>
>> if(handle_link(name, inout, pad++, type, filter, log_ctx) < 0)
>> return -1;
>
> handle_link() is such a nice meaningless name, why not inline the code
> here, its not used anywhere else anyway?
>
>
> [...]
>> // 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, log_ctx)))
>> goto fail;
>>
>> pad = parse_inouts(&inouts, &inout, chr == ',', LinkTypeIn, filter,
>> log_ctx);
>
> I do not like this design.
Me neither. But how else could I parse the following:
(in) (T1) picInPic, rotate, split (T2) (out) ; (T2) vflip (T1)
What will the parser do in the first "(T1)"? It don't have a pointer yet
to the AVFilterContext of the picInPic filter to store in the InOut
list. Nor does it have opened the vflip filter to make the link. Your
point about linking several filters is only valid for things like:
vsrc=a.avi (T1); (in) (T1) picInPic (out)
but even
(in) (T1) picInPic (out) ; vsrc=a.avi (T1)
would fail.
-Vitor
More information about the ffmpeg-devel
mailing list