[FFmpeg-soc] libavfilter review

[Michael Niedermayer]

On Fri, Aug 17, 2007 at 09:55:23PM -0400, Bobby Bingham wrote:
[...]
> > 
> > 
> > [...]
> > > 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.

no, leave it if you prefer


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

ok


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

the file loading could at least be seperated into a (optional) vf_foobar.c


[...]
> 
> > [...]
> > 
> > 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?

hmm i didnt find much except avg_bits_per_pixel() but that isnt exported
currently, i guess we could give it a av_ prefix and export it ...

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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-soc/attachments/20070818/4797e1d1/attachment.pgp>