[FFmpeg-soc] libavfilter review
Michael Niedermayer
michaelni at gmx.at
Sat Aug 18 04:13:25 CEST 2007
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>
More information about the FFmpeg-soc
mailing list