[FFmpeg-devel] [PATCH] [1/??] [2/3] Basic infrastructure

[Vitor Sessak]

Hi and thanks for the review

Michael Niedermayer wrote:
> On Sat, Feb 09, 2008 at 07:32:16PM +0100, Vitor Sessak wrote:
>> /*
>>  * Filter layer

[...]

>>     link->dst->inputs[link->dstpad] = NULL;
>>     if(avfilter_link(filt, out, link->dst, link->dstpad)) {
>>         /* failed to link output filter to new filter */
>>         link->dst->inputs[link->dstpad] = link;
>>         return -1;
>>     }
>>
>>     /* re-hookup the link to the new destination filter we inserted */
>>     link->dst = filt;
>>     link->dstpad = in;
>>     filt->inputs[in] = link;
>>
>>     /* if any information on supported colorspaces already exists on the
>>      * link, we need to preserve that */
>>     if((formats = link->out_formats))
>>         avfilter_formats_changeref(&link->out_formats,
>>                                    &filt->outputs[out]->out_formats);
>>
> 
> this looks odd, formats is set in the if() but never read afterwards

Probably some dead code leftover. Just removed var.

> 
> 
>>     return 0;
>> }
>>
>> int avfilter_config_links(AVFilterContext *filter)
>> {
>>     int (*config_link)(AVFilterLink *);
>>     unsigned i;
>>
>>     for(i = 0; i < filter->input_count; i ++) {
> 
>>         AVFilterLink *link;
>>
>>         if(!(link = filter->inputs[i])) continue;
> 
> AVFilterLink *link= filter->inputs[i];
> 
> if(!link) continue;
> 
> 
>>         switch(link->init_state) {
>>         case AVLINK_INIT:
>>             continue;
>>         case AVLINK_STARTINIT:
>>             av_log(filter, AV_LOG_ERROR, "circular filter chain detected\n");
>>             return -1;
>>         case AVLINK_UNINIT:
>>             link->init_state = AVLINK_STARTINIT;
>>
>>             if(avfilter_config_links(link->src))
>>                 return -1;
>>
>>             if(!(config_link = link_spad(link).config_props))
>>                 config_link  = avfilter_default_config_output_link;
>>             if(config_link(link))
>>                 return -1;
>>
>>             if((config_link = link_dpad(link).config_props))
>>             if(config_link(link))
>>                 return -1;
>>
>>             link->init_state = AVLINK_INIT;
>>         }
>>     }
> 
> what does the above mean for filter graphs like:
> 
> ->mix--->duplicate--->
>    ^         |
>    |         v
>     \------delay
> 
> aka an infinite impulse response video filter :)
> it looks like it would return -1 for that ...

What should be done in these cases? Something like av_log("Circular 
video chain, expect trouble\n")?

> 
> 
> [...]
>> /* XXX: should we do the duplicating of the picture ref here, instead of
>>  * forcing the source filter to do it? */
>> void avfilter_start_frame(AVFilterLink *link, AVFilterPicRef *picref)
>> {
>>     void (*start_frame)(AVFilterLink *, AVFilterPicRef *);
>>
>>     if(!(start_frame = link_dpad(link).start_frame))
>>         start_frame = avfilter_default_start_frame;
>>
>>     /* prepare to copy the picture if it has insufficient permissions */
>>     if((link_dpad(link).min_perms & picref->perms) != link_dpad(link).min_perms ||
>>         link_dpad(link).rej_perms & picref->perms) {
>>         /*
>>         av_log(link->dst, AV_LOG_INFO,
>>                 "frame copy needed (have perms %x, need %x, reject %x)\n",
>>                 picref->perms,
>>                 link_dpad(link).min_perms, link_dpad(link).rej_perms);
>>         */
>>
>>         link->cur_pic = avfilter_default_get_video_buffer(link, link_dpad(link).min_perms);
> 
> maybe link_dpad(link) should be in a variable instead? link_dpad does a
> bunch of derefs which would be duplicated even if one doesnt see this
> in the source.

Since it looks like this is the only function that really abuses it, I 
changed it only in avfilter_start_frame().

> 
> 
> [...]
> 
>>         for(j = 0; j < h; j ++) {
>>             memcpy(dst[0], src[0], link->cur_pic->linesize[0]);
>>             src[0] += link->srcpic ->linesize[0];
>>             dst[0] += link->cur_pic->linesize[0];
>>         }
>>         for(i = 1; i < 4; i ++) {
>>             if(!src[i]) continue;
>>
>>             for(j = 0; j < h >> vsub; j ++) {
>>                 memcpy(dst[i], src[i], link->cur_pic->linesize[i]);
>>                 src[i] += link->srcpic ->linesize[i];
>>                 dst[i] += link->cur_pic->linesize[i];
>>             }
>>         }
> 
> the 2 loops can be merged
> 
> 
> [...]
>> int avfilter_init_filter(AVFilterContext *filter, const char *args, void *opaque)
>> {
>>     int ret;
>>
>>     if(filter->filter->init)
>>         if((ret = filter->filter->init(filter, args, opaque))) return ret;
>>     return 0;
> 
> int ret=0;
> 
> if(filter->filter->init)
>     ret = filter->filter->init(filter, args, opaque);
> return ret;
> 
> 
> [...]
>> /**
>>  * Returns a fairly comprehensive list of colorspaces which are supported by
>>  * many of the included filters. This is not truly "all" the colorspaces, but
>>  * it is most of them, and it is the most commonly supported large subset.
>>  */
>> AVFilterFormats *avfilter_all_colorspaces(void);
> 
> as has already been noticed, the name isnt too good ...

Actually, the name is good but the comment is wrong (see my recent 
discussion with Bobby about it in -soc). Comment changed.

All the other points I don't mention, agreed and changed. New code in 
http://svn.mplayerhq.hu/soc/libavfilter/avfilter.c?content-type=text%2Fplain&view=co 
(Diego, nits changed in the soc tree too).

-Vitor