[FFmpeg-soc] libavfilter review

[Bobby Bingham]

On Fri, 17 Aug 2007 21:10:15 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> Hi
> 
> after the matroska muxer and dirac heres my first try of a lavfilter
> review this is based on the files a day or 2 ago so please forgive me
> if iam complaining about things which have been changed already
> 
> ffmpeg.diff:
> please split this patch, for example the restructuring which is not
> under ENABLE_AVFILTER could be in a seperate patch
> also dont hesitate to put a README.ffmpeg.diff or so in svn describing
> what exactly is done and why it might safe some time with reviewing

I split this soon.

> 
> avfiltergraph.c:
> this is quite complex, is this complexity really needed?
> this contains several methods to build filter graphs, why is a
> single one not enough?

I think the one to create a filter graph from a string in a format
similar to mplayer is useful, though maybe it should be made to work on
top of the filter description structure that exists now.

The one that creates graphs from a list of strings is probably not
useful.  I'll remove it.

> 
> [...]
> > /* given the link between the dummy filter and an internal filter
> > whose input
> >  * is being exported outside the graph, this returns the externally
> > visible
> >  * link */
> > static inline AVFilterLink *get_extern_input_link(AVFilterLink
> > *link)
> 
> not doxygen compatible

fixed

> 
> 
> [...]
> 
> avfilter.c:
> [...]
> > AVFilterPicRef *avfilter_ref_pic(AVFilterPicRef *ref, int pmask)
> > {
> >     AVFilterPicRef *ret = av_malloc(sizeof(AVFilterPicRef));
> 
> >     memcpy(ret, ref, sizeof(AVFilterPicRef));
> 
> *ret= *ref;
> advantages: type checking and its simpler ...

done

> 
> 
> [...]
> > void avfilter_insert_pad(unsigned idx, unsigned *count, size_t
> > padidx_off, AVFilterPad **pads, AVFilterLink ***links,
> >                          AVFilterPad *newpad)
> > {
> >     unsigned i;
> > 
> >     idx = FFMIN(idx, *count);
> > 
> >     *pads  = av_realloc(*pads,  sizeof(AVFilterPad)   * (*count +
> > 1)); *links = av_realloc(*links, sizeof(AVFilterLink*) * (*count +
> > 1)); memmove(*pads +idx+1, *pads +idx, sizeof(AVFilterPad)   *
> > (*count-idx)); memmove(*links+idx+1, *links+idx,
> > sizeof(AVFilterLink*) * (*count-idx)); memcpy(*pads+idx, newpad,
> > sizeof(AVFilterPad)); (*links)[idx] = NULL;
> > 
> >     (*count) ++;
> 
> >     for(i = idx+1; i < *count; i ++)
> >         if(*links[i])
> >             (*(unsigned *)((uint8_t *)(*links[i]) + padidx_off)) ++;
> 
> well, it does increase the *pad though iam not sure if not maybe it
> would be better to not use a byte offset into the struct to identify
> which of the 2 should be used ...

It was either that or duplicate the code to handle both source and dest
pads.  I can do that instead if you think it's cleaner.

> 
> 
> [...]
> >     /* find a format both filters support - TODO: auto-insert
> > conversion filter */ link->format = -1;
> >     if(link->src->output_pads[link->srcpad].query_formats)
> >         fmts[0] =
> > link->src->output_pads[link->srcpad].query_formats(link); else
> >         fmts[0] = avfilter_default_query_output_formats(link);
> >     fmts[1] =
> > link->dst->input_pads[link->dstpad].query_formats(link); for(i = 0;
> > fmts[0][i] != -1; i ++) for(j = 0; fmts[1][j] != -1; j ++)
> >             if(fmts[0][i] == fmts[1][j]) {
> >                 link->format = fmts[0][i];
> >                 goto format_done;
> >             }
> 
> the query_formats system is very flexible, it takes a AVFilterLink
> so a filter could have a fixed list of supported input pix_fmts and
> make the output query_formats depend on the input pix_fmt or the
> other was around well i dont think the other way around would work
> but how should the user know that?
> and whats the sense of this overly flexible system if it doesnt work
> with anything but the obvious simple supported output pix_fmt depends
> upon input formats, it would be alot clearer if query_formats would
> take a list/array of input pix_fmts as argument (or a array or
> pix_fmt, flags pairs) where the flags could indicate if the pix_fmt
> can be provided without colorspace conversation, but maybe thats not
> really usefull and a simpler prefer first possible format in the list
> system would work equally well
> 
> 
> also what happens in the following case:
> src -> filter -> destination
> 
> src supports formats A and C
> destination supports formats B anc C
> and the filter supports A,B,C inputs and output=input
> 
> if i understood the code correctly this would fail

Yes, this would fail currently.  I know the colorspace negotiation
still needs work.  I'm working on it.

> 
> 
> > 
> > format_done:
> >     av_free(fmts[0]);
> >     av_free(fmts[1]);
> >     if(link->format == -1)
> >         return -1;
> > 
> >     if(!(config_link =
> > link->src->output_pads[link->srcpad].config_props)) config_link  =
> > avfilter_default_config_output_link; if(config_link(link))
> >             return -1;
> > 
> >     if(!(config_link =
> > link->dst->input_pads[link->dstpad].config_props)) config_link  =
> > avfilter_default_config_input_link; if(config_link(link))
> >             return -1;
> > 
> >     return 0;
> > }
> > 
> > AVFilterPicRef *avfilter_get_video_buffer(AVFilterLink *link, int
> > perms) {
> >     AVFilterPicRef *ret = NULL;
> > 
> >     if(link->dst->input_pads[link->dstpad].get_video_buffer)
> >         ret =
> > link->dst->input_pads[link->dstpad].get_video_buffer(link, perms);
> > 
> >     if(!ret)
> >         ret = avfilter_default_get_video_buffer(link, perms);
> > 
> >     return ret;
> > }
> > 
> > int avfilter_request_frame(AVFilterLink *link)
> > {
> >     const AVFilterPad *pad = &link->src->output_pads[link->srcpad];
> > 
> >     if(pad->request_frame)
> >         return pad->request_frame(link);
> >     else if(link->src->inputs[0])
> >         return avfilter_request_frame(link->src->inputs[0]);
> >     else return -1;
> > }
> > 
> > /* 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 *);
> > 
> >     start_frame = link->dst->input_pads[link->dstpad].start_frame;
> >     if(!start_frame)
> >         start_frame = avfilter_default_start_frame;
> > 
> >     start_frame(link, picref);
> 
> you are mixing
> func_ptr = A
> if(!func_ptr)
>     func_ptr= B;
> func_ptr();
> 
> and
> if(A)
>     A(x)
> else
>     B(x)
> 
> and
> if(!(func_ptr= B))
>     func_ptr= A;
> func_ptr();
> 
> i think a little bit consistency would make things more readable

fixed

> 
> 
> [...]
> > static int filter_cmp(const void *aa, const void *bb)
> > {
> >     const AVFilter *a = *(const AVFilter **)aa, *b = *(const
> > AVFilter **)bb; return strcmp(a->name, b->name);
> > }
> 
> for some reason i always loved giving these cmp functions arguments
> with correct types instead of temporary variables (and casts)
> it IMHO looks cleaner
> 
> 
> > 
> > AVFilter *avfilter_get_by_name(char *name)
> > {
> >     AVFilter key = { .name = name, };
> >     AVFilter *key2 = &key;
> >     AVFilter **ret;
> > 
> >     ret = bsearch(&key2, filters, filter_count, sizeof(AVFilter
> > **), filter_cmp); if(ret)
> >         return *ret;
> >     return NULL;
> > }
> 
> do we search for filters often enough that a bsearch is worth it?
> (no iam not complaining, just asking)
> 
> also a simple linked list is O(1) insert O(n) search
> we insert all filters, tough likely we use less than all
> 
> your code is O(n log n) insert O(log n) search if above hypothesis is
> true that wouldnt be faster
> delaying the sort until the first avfilter_get_by_name() would
> help but well, the whole doesnt seem to matter speedwise so ill stop
> giving stupid suggestions on how to improve it here :)
> 

Changed to a linked list.  We almost certainly do much more inserting
than searching.

> 
> 
> > 
> > /* FIXME: insert in order, rather than insert at end + resort */
> > void avfilter_register(AVFilter *filter)
> > {
> >     filters = av_realloc(filters, sizeof(AVFilter*) *
> > (filter_count+1)); filters[filter_count] = filter;
> >     qsort(filters, ++filter_count, sizeof(AVFilter **), filter_cmp);
> > }
> > 
> [...]
> > AVFilterContext *avfilter_create(AVFilter *filter, char *inst_name)
> > {
> >     AVFilterContext *ret = av_malloc(sizeof(AVFilterContext));
> > 
> >     ret->av_class = av_mallocz(sizeof(AVClass));
> >     ret->av_class->item_name = filter_name;
> >     ret->filter   = filter;
> >     ret->name     = inst_name ? av_strdup(inst_name) : NULL;
> >     ret->priv     = av_mallocz(filter->priv_size);
> > 
> >     ret->input_count  = pad_count(filter->inputs);
> 
> >     ret->input_pads   = av_malloc(sizeof(AVFilterPad) *
> > ret->input_count); memcpy(ret->input_pads, filter->inputs,
> > sizeof(AVFilterPad)*ret->input_count);
> 
> why are the filter pads copied from AVFilter to AVFilterContext?
> wouldnt a pointer do?

The idea was to allow filters to modify their pads on the fly.  For
example, the filter graph adds pads dynamically to export the pads of
filters contained by the graph to appear as if they were pads of the
graph itself.

Other filters could also modify the callback functions for the pads
based on eg. available SIMD instructions or even the arguments to the
filter.

This means that the pads need to be per-AVFilterContext.

> 
> 
> >     ret->inputs       = av_mallocz(sizeof(AVFilterLink*) *
> > ret->input_count);
> > 
> >     ret->output_count = pad_count(filter->outputs);
> >     ret->output_pads  = av_malloc(sizeof(AVFilterPad) *
> > ret->output_count); memcpy(ret->output_pads, filter->outputs,
> > sizeof(AVFilterPad)*ret->output_count); ret->outputs      =
> > av_mallocz(sizeof(AVFilterLink*) * ret->output_count);
> > 
> >     return ret;
> > }
> 
> also whats your oppinion about changing the name to avfilter_open()
> it after all doesnt create a AVFilter

done

> 
> 
> > 
> > void avfilter_destroy(AVFilterContext *filter)
> > {
> >     int i;
> > 
> >     if(filter->filter->uninit)
> >         filter->filter->uninit(filter);
> > 
> >     for(i = 0; i < filter->input_count; i ++) {
> >         if(filter->inputs[i])
> >             filter->inputs[i]->src->outputs[filter->inputs[i]->srcpad]
> > = NULL; av_free(filter->inputs[i]);
> >     }
> >     for(i = 0; i < filter->output_count; i ++) {
> >         if(filter->outputs[i])
> >             filter->outputs[i]->dst->inputs[filter->outputs[i]->dstpad]
> > = NULL; av_free(filter->outputs[i]);
> >     }
> > 
> >     av_free(filter->name);
> >     av_free(filter->input_pads);
> >     av_free(filter->output_pads);
> >     av_free(filter->inputs);
> >     av_free(filter->outputs);
> >     av_free(filter->priv);
> >     av_free(filter->av_class);
> >     av_free(filter);
> 
> id change that all to av_freep() to reduce the chance of using freed
> memory
> 

done

> 
> > }
> > 
> > AVFilterContext *avfilter_create_by_name(char *name, char
> > *inst_name) {
> >     AVFilter *filt;
> > 
> >     if(!(filt = avfilter_get_by_name(name))) return NULL;
> >     return avfilter_create(filt, inst_name);
> > }
> 
> i think the user can call these 2 functions himself, no need to
> provide a wraper and thus complicate the API by yet another function
> 

removed

> 
> 
> avfiltergraphdesc.c:
> [...]
> > /* a comment is a line which is empty, or starts with whitespace,
> > ';' or '#' */ static inline int is_line_comment(char *line)
> 
> doxygenize please

done

> 
> 
> [...]
> > static Section parse_section_name(char *line)
> > {
> >          if(!strncmp(line, strFilters, strlen(strFilters))) return
> > SEC_FILTERS; else if(!strncmp(line, strLinks,
> > strlen(strLinks)))   return SEC_LINKS; else if(!strncmp(line,
> > strInputs,  strlen(strInputs)))  return SEC_INPUTS; else
> > if(!strncmp(line, strOutputs, strlen(strOutputs))) return
> > SEC_OUTPUTS;
> 
> an array of strings, Section would simplify this to a loop
> 

done

> 
> [...]
> > AVFilterGraphDesc *avfilter_graph_load_desc(const char *filename)
> > {
> >     AVFilterGraphDesc       *ret    = NULL;
> >     AVFilterGraphDescFilter *filter = NULL;
> >     AVFilterGraphDescLink   *link   = NULL;
> >     AVFilterGraphDescExport *input  = NULL;
> >     AVFilterGraphDescExport *output = NULL;
> > 
> >     Section section;
> >     char line[LINESIZE];
> >     void *next;
> >     FILE *in;
> > 
> >     /* TODO: maybe allow searching in a predefined set of
> > directories to
> >      * allow users to build up libraries of useful graphs? */
> >     if(!(in = fopen(filename, "r")))
> >         goto fail;
> 
> i do not like this at all, the libavfilter core has no bushiness
> doing (f)open() its not hard to do
> 
> graph_from_file_filter="`cat myfile`" or similar on the command line
> or let the app do the file reading and provide a char* to avfilter

I was afraid you'd say that.

Here's what I had in mind with this.  The whole reason I made the
filter graph into a type of filter was so that it could be used
transparently as a filter in part of a larger filter graph.  It makes
for easy definition of combinations of filters that are often used
together.  For example, a user might find themselves using a filter
chain like ivtc->denoise3d a lot.  By defining this in a file, the can
easily reuse that sequence of filters.

Your suggestion works fine for the first such graph loaded from a
file.  But it could also be useful if that loaded file itself could
reference other graphs defined by other files.  That's what the code
does currently.

I agree that fopen and friends probably don't belong in a function like
this which is called in the process of creating the graph_file filter.
I'm thinking a cleaner solution would be to allow the application to
register a list of graphs that it has loaded from files similar to how
the filters are registered at init.  But I still think it would be
useful to provide a function the application can use to load a graph
from a file if it's only ever called from the app directly, and not in
some other hidden places of libavfilter as is currently the case.

> 
> 
> [...]
> >         case SEC_FILTERS:
> >             if(!(next = parse_filter(line)))
> >                 goto fail;
> >             if(filter)
> >                 filter = filter->next = next;
> >             else
> >                 ret->filters = filter = next;
> >             break;
> 
> AVFilterGraphDescFilter **filterp = &ret->filters;
> and
> 
> *filterp= next;
> filterp= &(next->next);
> 

done

> [...]
> 
> avfilter.h:
> 
> [...]
> >     int64_t pts;                ///< presentation timestamp in
> > milliseconds
> 
> milliseconds is insufficient
> 1/AV_TIME_BASE would be better
> 

done

> [...]
> 
> default.c:
> [...]
> > /**
> >  * default config_link() implementation for output video links to
> > simplify
> >  * the implementation of one input one output video filters */
> > int avfilter_default_config_output_link(AVFilterLink *link)
> > {
> >     if(link->src->input_count && link->src->inputs[0]) {
> >         link->w = link->src->inputs[0]->w;
> >         link->h = link->src->inputs[0]->h;
> >     } else {
> >         /* XXX: any non-simple filter which would cause this branch
> > to be taken
> >          * really should implement its own config_props() for this
> > link. */ link->w =
> >         link->h = 0;
> 
> then why is this not a return -1; assert(0) or whatever
> 

changed to return -1

> [...]
> 
> vf_fps.c:
> [...]
> > static int init(AVFilterContext *ctx, const char *args, void
> > *opaque) {
> >     FPSContext *fps = ctx->priv;
> >     int framerate;
> > 
> >     /* TODO: support framerates specified as decimals or fractions
> > */ if(args && sscanf(args, "%d", &framerate))
> >         fps->timebase = 1000 / framerate;
> >     else
> >         /* default to 25 fps */
> >         fps->timebase = 1000 / 25;
> 
> timebase must be AVRational
> things like 25000/1001 must be possible, they are quite common

as per the comment, this is a known TODO

> [...]
> 
> vf_overlay.c:
> [...]
> >     switch(link->format) {
> >     case PIX_FMT_RGB32:
> >     case PIX_FMT_BGR32:
> >         over->bpp = 4;
> >         break;
> >     case PIX_FMT_RGB24:
> >     case PIX_FMT_BGR24:
> >         over->bpp = 3;
> >         break;
> >     case PIX_FMT_RGB565:
> >     case PIX_FMT_RGB555:
> >     case PIX_FMT_BGR565:
> >     case PIX_FMT_BGR555:
> >     case PIX_FMT_GRAY16BE:
> >     case PIX_FMT_GRAY16LE:
> >         over->bpp = 2;
> >         break;
> >     default:
> >         over->bpp = 1;
> >     }
> 
> this code is duplicated, it occurs in several filters

and there's very similar code duplicated in swsscale_internal.h and in
many of the codecs.  Is there an existing function somewhere that could
be used everywhere?

> [...]
> 
> vsrc_ppm.c:
> [...]
> 
> > static int init(AVFilterContext *ctx, const char *args, void
> > *opaque) {
> >     PPMContext *ppm = ctx->priv;
> >     int max;
> > 
> >     if(!args) return -1;
> >     if(!(ppm->in = fopen(args, "r"))) return -1;
> 
> access should be done through URLProtocol or libavformat or something
> else direct fopen() is highly unflexible (for testing its of course
> ok but a final read image filter shouldnt do it like this)
> 
> 
> [...]
> > static int request_frame(AVFilterLink *link)
> > {
> >     PPMContext *ppm = link->src->priv;
> >     AVFilterPicRef *out;
> > 
> >     int y;
> >     uint8_t *row;
> > 
> >     if(!ppm->pic) {
> >         ppm->pic = avfilter_get_video_buffer(link,
> >                         AV_PERM_WRITE | AV_PERM_PRESERVE |
> > AV_PERM_REUSE);
> > 
> >         row = ppm->pic->data[0];
> >         for(y = 0; y < ppm->h; y ++) {
> >             fread(row, 3, ppm->w, ppm->in);
> >             row += ppm->pic->linesize[0];
> >         }
> > 
> >         fclose(ppm->in);
> >         ppm->in = NULL;
> >     } else ppm->pic->pts += 30;
>                               ^^
> this should be user configureable

This entire filter was intended only for testing.  I was going to
remove it soon.

> 
> 
> [...]
> 
> > AVFilter vsrc_ppm =
> 
> this (and others) should have some prefix to avoid name clashes
> 

will do

> [...]
> 


-- 
Bobby Bingham
Never trust atoms.  Or anything made of atoms.
このメールは再利用されたバイトでできている。