[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Wed Apr 23 23:05:37 CEST 2008
Hi and thanks for the review
Michael Niedermayer wrote:
> On Mon, Apr 21, 2008 at 06:36:53PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Apr 18, 2008 at 05:35:40PM +0200, Vitor Sessak wrote:
>>>> Hi
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Sat, Apr 12, 2008 at 04:40:24PM +0200, Vitor Sessak wrote:
>>>>>> Hi
>>> [...]
>>>>> [...]
>>>>>> // 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:
>>> something approximately like: (in/out might not be hadled ideally)
>> I've done as you suggested. The code is more complex, but it is less ugly
>> and more functional (now the comma links one or more filters). I guess that
>> this would be needed sooner or later anyway for implementing all that is
>> planned...
>
> [...]
>> static AVFilterInOut *extract_inout(const char *label, AVFilterInOut **links)
>> {
>> AVFilterInOut *ret;
>> AVFilterInOut *p;
>>
>
>> if(!links || !*links)
>> return NULL;
>>
>> if(!strcmp((*links)->name, label)) {
>> ret = *links;
>> *links = (*links)->next;
>> return ret;
>> }
>>
>> /* First check if the label is not in the openLinks list */
>> for(p = *links; p->next && strcmp(p->next->name, label); p = p->next);
>>
>> if(!p->next)
>> return NULL;
>>
>> ret = p->next;
>>
>> p->next = p->next->next;
>>
>> return ret;
>
> ugh ...
>
> while(*links && strcmp((*links)->name, label))
> links= &((*links)->next);
>
> ret= *links;
> if(ret)
> *links= ret->next;
>
> return ret;
Nice, thanks. Why the hell do they teach at college the ugly way to
linked lists?
>> }
>>
>>
>
>> static int link_filter_inouts(AVFilterContext *filter,
>> AVFilterInOut **currInputs,
>> AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>> AVFilterInOut *p;
>> int pad = 0;
>>
>> pad = filter->input_count;
>
>> while(pad) {
>> p = *currInputs;
>> pad--;
>
> while(pad--)
Changed there and elsewhere
>
>
>> if(!p) {
>> av_log(log_ctx, AV_LOG_ERROR,
>> "Not enough inputs specified for the \"%s\" filter.\n",
>> filter->name);
>> return -1;
>> }
>>
>> if(p->filter) {
>> if(link_filter(p->filter, p->pad_idx, filter, pad, log_ctx))
>> return -1;
>> *currInputs = (*currInputs)->next;
>> av_free(p);
>> } else {
>
>> p = *currInputs;
>
> redundant
>
>> *currInputs = (*currInputs)->next;
>
> can be factored out
>
> maybe you should try to simplify your code before submitting?
I always reread my code before sending. This time I focused in giving
decent error messages and failing in broken syntaxes. Yes, a second pass
was needed.
>> p->filter = filter;
>> p->pad_idx = pad;
>> p->next = *openLinks;
>> *openLinks = p;
>> }
>> }
>>
>>
>> if(*currInputs) {
>> av_log(log_ctx, AV_LOG_ERROR,
>> "Too many inputs specified for the \"%s\" filter.\n",
>> filter->name);
>> return -1;
>> }
>>
>> pad = filter->output_count;
>> while(pad) {
>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>> pad--;
>> currlinkn->name = NULL;
>> currlinkn->type = LinkTypeOut;
>> currlinkn->filter = filter;
>> currlinkn->pad_idx = pad;
>> currlinkn->next = *currInputs;
>> *currInputs = currlinkn;
>
> somehow i thik this can be factored with the above as well
Maybe, but without worsening a lot readability?
> [...]
>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>> AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>> int pad = 0;
>> AVFilterInOut *p;
>>
>> while (**buf == '[') {
>> char *name;
>>
>> parse_link_name(buf, &name, log_ctx);
>>
>> if(!name)
>> return -1;
>>
>> /* First check if the label is not in the openLinks list */
>> p = extract_inout(name, openLinks);
>>
>> /* Not in the list, so add it as an input */
>> if(!p) {
>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>
>
>> currlinkn->name = name;
>> currlinkn->type = LinkTypeIn;
>> currlinkn->filter = NULL;
>> currlinkn->pad_idx = pad;
>> currlinkn->next = *currInputs;
>> *currInputs = currlinkn;
>
> Ive seen similar code a few times already here somewhere ...
This code appears three times. But doing something like
currlinkn = new_link(NULL, LinkTypeOut, filter, pad, *currInputs);
wouldn't make this new_link() function a senseless wrapper?
>
>
>> } else {
>
> I prefer if(p) else over if(!p) else, not important it just seems more natural
Changed.
>> /* A label of a open link. Make it one of the inputs of the next
>> filter */
>> AVFilterInOut *currlinkn = p;
>> if (p->type != LinkTypeOut) {
>> av_log(log_ctx, AV_LOG_ERROR,
>> "Label \"%s\" appears twice as input!\n", p->name);
>> return -1;
>> }
>
>> currlinkn->next = *currInputs;
>> *currInputs = currlinkn;
>
> factorize ...
Did everywhere
> [...]
>
>> static int parse_outputs(const char **buf, AVFilterInOut **currInputs,
>> AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>> int pad = 0;
>>
>> while (**buf == '[') {
>> char *name;
>> AVFilterInOut *match;
>>
>> parse_link_name(buf, &name, log_ctx);
>>
>> if(!name)
>> return -1;
>>
>> /* First check if the label is not in the openLinks list */
>> match = extract_inout(name, openLinks);
>
>
> this functions has a duplicate smell to it ...
That didn't escape my read-before-sending procedure. I didn't see a way
of merging them that is worth the extra obfuscation.
> [...]
>
>> /**
>> * Add to a graph a graph described by a string.
>> * @param graph the filter graph where to link the parsed graph context
>> * @param filters string to be parsed
>> * @param in input to the graph to be parsed (TODO: allow several)
>> * @param inpad pad index of the input
>> * @param out output to the graph to be parsed (TODO: allow several)
>> * @param outpad pad index of the output
>> * @return zero on success, -1 on error
>> */
>> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
>> AVFilterContext *in, int inpad,
>> AVFilterContext *out, int outpad,
>> AVClass *log_ctx);
>
> Maybe a AVFilterInOut array could be used as argument, this would be cleaner
> Then arrays of in/outpads and filter contexts.
I like this idea. Done.
New patch attached.
-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 10873 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080423/a1ba2edb/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.h
Type: text/x-chdr
Size: 1728 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080423/a1ba2edb/attachment.h>
More information about the ffmpeg-devel
mailing list