[FFmpeg-devel] [PATCH 2/3] avfiltergraph: don't query formats if filter has uninitialized inputs

[Matthieu Bouron]

On Thu, May 03, 2012 at 03:16:40PM +0200, Nicolas George wrote:
> Le quintidi 15 floréal, an CCXX, Matthieu Bouron a écrit :
> > ---
> >  libavfilter/avfiltergraph.c |   46 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> Can you explain why this is necessary? I have some ideas, but I would like
> to compare ideas.
> 

The complex filter system add vsrc_buffer and asrc_buffer (in next patch) at
the end of the avfilter context array and query_formats is currently called
sequentially it. It can lead to a filter initialization failure for filters
with input dependencies (like amerge)

The following example will fail on the amerge filter since the two abbufer
are not initialized:
  [ amerge ] [ abuffer ] [ abuffer ] [ abuffersink ]

> > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> > index 9d7b956..064e678 100644
> > --- a/libavfilter/avfiltergraph.c
> > +++ b/libavfilter/avfiltergraph.c
> > @@ -202,17 +202,49 @@ static int insert_conv_filter(AVFilterGraph *graph, AVFilterLink *link,
> >  
> >  static int query_formats(AVFilterGraph *graph, AVClass *log_ctx)
> >  {
> > -    int i, j, ret;
> > +    int i, j, k, ret;
> >      char filt_args[128];
> >      AVFilterFormats *formats, *chlayouts, *packing;
> > +    int filters_init;
> > +    int *filters_state = av_calloc(graph->filter_count, sizeof(int));
> >  
> >      /* ask all the sub-filters for their supported media formats */
> > -    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]);
> > -    }
> > +    do {
> > +        filters_init = 1;
> > +        for (i = 0; i < graph->filter_count; i++) {
> > +            int do_query = 1;
> > +            AVFilterContext *cur = graph->filters[i];
> > +
> > +            if (filters_state[i])
> > +                continue;
> > +
> > +            /* Check state of input links */
> > +            for (j = 0; j < cur->input_count; j++) {
> > +                AVFilterContext *ifilter = cur->inputs[j]->src;
> > +
> > +                for(k = 0; k < graph->filter_count; k++) {
> > +                    if (ifilter == graph->filters[k]) {
> > +                        break;
> > +                    }
> > +                }
> 
> This looks quadratic in the number of filters in the graph.
> 
> > +
> > +                if (!filters_state[k]) {
> > +                    do_query = 0;
> > +                    filters_init = 0;
> > +                }
> > +            }
> > +
> > +            if (do_query) {
> > +                filters_state[i] = 1;
> > +                if (graph->filters[i]->filter->query_formats)
> > +                    graph->filters[i]->filter->query_formats(graph->filters[i]);
> > +                else
> > +                    avfilter_default_query_formats(graph->filters[i]);
> > +            }
> > +        }
> > +    } while (!filters_init);
> > +
> > +    av_freep(&filters_state);
> 
> On the whole, it looks like you want to do the queries in dependency order.
> There may be more efficient solutions.

I agree, this implementation is naive.

Another solution whould be to order the avfilter context array when avfilter_link
is called.


[...]