[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Michael Niedermayer
michaelni
Tue Apr 22 22:35:01 CEST 2008
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;
> }
>
>
> 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--)
> 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?
> 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
[...]
> 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 ...
> } else {
I prefer if(p) else over if(!p) else, not important it just seems more natural
> /* 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 ...
[...]
> 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 ...
[...]
> /**
> * 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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20080422/2a1be089/attachment.pgp>
More information about the ffmpeg-devel
mailing list