[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description

[Vitor Sessak]

Michael Niedermayer wrote:
> On Mon, May 05, 2008 at 10:01:36PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Wed, Apr 23, 2008 at 11:05:37PM +0200, Vitor Sessak wrote:
>>>> Hi and thanks for the review
>>> [...]
>>>>>>             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?
>>> We need to find a way to simplify the whole parser without reducing
>>> readability.
>>> Its much more complex than i would like ...
>> Do you really think it can be so much more simpler? The "complex" part
>> of it (free_inout(), extract_inout(), link_filter_inouts(),
>> parse_inputs(), parse_outputs(), avfilter_parse_graph()) make 240 lines
>> of code.
> 
> What i really dont like on the code is that it is complex and at the
> same time only supports a small subset of what we wanted to support.
> 300 lines would be fine for a complete implemantation with ; * () user
> defined filters, and all the other things discussed
> Saying ok to the code to then see it grow to 3 times that to support
> everything is something i really dont want to see ...

Maybe a way to simplify it would be to use nop filters in the following
way: to parse "filter1 [T1], filter2" do

-create the filter "filter1"
-check if there is already a filter with instance name "T1" (using
already existent avfilter_graph_get_filter())
-if not create a nop filter with instance name "T1"
-link "filter1":0 with this nop filter (created or found)
-create "filter2"
-link "filter1":1 with "filter2"

so now every label has one (and only one) corresponding nop filter.

This have some advantages:
1. Remove the whole need for the openLinks list
2. AVFilterInOut could be just a linked list (or an array) of pointers
to AVFilterContext
3. Preserve the link name without adding anything else to the existing
API. Also this link can be retrieved at any moment with
avfilter_graph_get_filter().
4. Allows for implementing user-defined filters (calling
avfilter_parse_graph() recursively). But for this maybe your idea of
passing the inputs as parameters and getting the outputs as return value
may be useful...

And some downsides:
1. Create a bunch of useless nop filters
2. May have problems with name clash when calling recursively
avfilter_parse_graph() for user-defined filters

-Vitor