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

[Michael Niedermayer]

On Sat, Feb 09, 2008 at 07:32:16PM +0100, Vitor Sessak wrote:
[...]

> /*
>  * Filter layer
[...]
> #include <stdarg.h>
> #include <stdlib.h>
[...]
> #include <stdio.h>

i wonder which of these includes are actually needed ...


[...]

> int avfilter_insert_filter(AVFilterLink *link, AVFilterContext *filt,
>                            unsigned in, unsigned out)
> {
>     AVFilterFormats *formats;
> 
>     av_log(NULL, AV_LOG_INFO, "auto-inserting filter '%s'\n",
>             filt->filter->name);

NULL context is bad


> 
>     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


>     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 ...


[...]
> /* 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.


[...]

>         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 ...

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20080209/56ac19b3/attachment.pgp>