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

[Vitor Sessak]

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.

The only thing that can be split is the general graph code 
(avfilter_graph_add_filter(), avfilter_destroy_graph(), 
avfilter_graph_config_formats(), etc) from the parser. If you think it 
is a good idea, I'm not against it.

> 
> 
> [...]
>>     char tmp[20];
>>
>>     snprintf(tmp, 20, "%d", index);
>                     ^^
> sizeof

Agreed. But are you ok with the instance name/instance lookup logic here?

> 
> 
>>     if(!(filterdef = avfilter_get_by_name(name)) ||
>>        !(filt = avfilter_open(filterdef, tmp))) {
>>         av_log(&log_ctx, AV_LOG_ERROR,
>>                "error creating filter '%s'\n", name);
> 
> The purpose of the context is so the user can associate it with some
> specific instance of something. using a global context defeats this.

Yes. Any suggestion?

> 
> 
> [...]

I'll look at the rest later this week.

-Vitor