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

[Michael Niedermayer]

On Wed, Apr 02, 2008 at 07:02:27PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Tue, Apr 01, 2008 at 11:43:55PM +0200, Michael Niedermayer wrote:
>>> On Tue, Apr 01, 2008 at 10:49:39PM +0200, Vitor Sessak wrote:
>>>> Hi
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Sun, Mar 30, 2008 at 01:50:51PM +0200, Vitor Sessak wrote:
>>>> (...)
>>>>
>>>>>> static int query_formats(AVFilterGraph *graph)
>>>>>> {
>>>>> Now i dont think this belongs in the parser code ...
>>>>> There should be IMHO
>>>>> A. code for a filter graph (a single flat filter graph that is a filter 
>>>>> graph
>>>>>    is not a filter) If we ever see the need for having filter graphs be
>                                                            ^^^^^^^^^^^^^
>>>>>    filters this can be added later as a filter easily. Not weirdly
>         ^^^^^^^
>>>>>    intermingled with everything else.
>>>>> B. the parser
>>>> This function is _not_ passed as a callback to AVFilter.query_formats 
>>>> (but it was some svn revs ago). It is called by the code 
>>>> (avfilter_graph_config_formats()) that tries to set a agreed upon filter 
>>>> format for all filters.
>>> This does NOT belong in the parser! Absolutely nothing related to 
>>> colorspace
>>> negotation belongs in the parser. No matter in what form.
>> A Parser takes a string and builds a graph of filters. At that point the
>> parser is done with its work and you have a bunch of filters with links
>> between them.
>> Next and indepednant on how the graph was constructed some colorspace
>> format resolving code would be run over the graph, this might insert
>> scale filters.
>> And as last step all the remaining init/config is done this again should
>> be independant of how the previous steps have been done.
>> Is there some problem/disadvantage with my suggestion? If so please 
>> elaborate.
>
> No, I agree that it hasn't much to do with the parser. What I thought was 
> that the 100 lines of the common graph code didn't deserve its own file. 
> When I replied to your previous message, I thought you didn't notice that 
> filter graphs as a filter is gone, so I wanted to make clear it is.
>
> But anyway, I'm not against of having this code in its own file, as 
> attached.

thanks, i think its much better to have this in a seperate file.


[...]
> /**
>  * @todo insert in sorted order
>  */
> void avfilter_graph_add_filter(AVFilterGraph *graph, AVFilterContext *filter)

Why would you want to sort them? (yes i know it allow binary search for
finding them by name, but 
1. it makes the insert O(n) and 
2. why does search by name matter speedwise at all?
3. it would break access by numeric index as the index for filters would
   change.


[...]
> /* search intelligently, once we insert in order */
> AVFilterContext *avfilter_graph_get_filter(AVFilterGraph *graph, char *name)
> {
>     int i;
> 
>     if(!name)
>         return NULL;

what is the point of allowing name==NULL?


[...]
> 
> static int query_formats(AVFilterGraph *graph)
> {
>     int i, j;
> 
>     /* ask all the sub-filters for their supported colorspaces */
>     for(i = 0; i < graph->filter_count; i ++) {
>         if(graph->filters[i]->filter->query_formats)
>             graph->filters[i]->filter->query_formats(graph->filters[i]);
>         else
>             avfilter_default_query_formats(graph->filters[i]);
>     }
> 
>     /* go through and merge as many format lists as possible */
>     for(i = 0; i < graph->filter_count; i ++) {
>         AVFilterContext *filter = graph->filters[i];
> 
>         for(j = 0; j < filter->input_count; j ++) {

>             AVFilterLink *link;
>             if(!(link = filter->inputs[j]))
>                 continue;
>             if(link->in_formats != link->out_formats) {

ehm

AVFilterLink *link= filter->inputs[j];

if(link && link->in_formats != link->out_formats) {



>                 if(!avfilter_merge_formats(link->in_formats,
>                                            link->out_formats)) {
>                     /* couldn't merge format lists. auto-insert scale filter */

>                     AVFilterContext *scale;
> 
>                     if(!(scale =
>                          avfilter_open(avfilter_get_by_name("scale"), NULL)))
>                         return -1;

same comment as above.
Also it should have a instance name like "auto inserted scaler 123" IMHO.
(something shorter would be fine as well ...)


>                     if(scale->filter->init(scale, NULL, NULL) ||
>                        avfilter_insert_filter(link, scale, 0, 0)) {
>                         avfilter_destroy(scale);
>                         return -1;
>                     }
> 
>                     avfilter_graph_add_filter(graph, scale);
>                     scale->filter->query_formats(scale);

>                     if(!avfilter_merge_formats(scale-> inputs[0]->in_formats,
>                                               scale-> inputs[0]->out_formats) ||
>                        !avfilter_merge_formats(scale->outputs[0]->in_formats,
>                                               scale->outputs[0]->out_formats))

vertical align


[...]
> /**
>  * Configure the parameters (resolution, etc) of all links in the graph.
>  */
> int avfilter_graph_config_links(AVFilterGraph *graphctx);

that is implemented where?


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20080403/9b56f0e9/attachment.pgp>