[FFmpeg-soc] libavfilter review
Michael Niedermayer
michaelni at gmx.at
Fri Aug 17 21:10:15 CEST 2007
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
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?
[...]
> /* 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
[...]
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 ...
[...]
> 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 ...
[...]
> /* 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
>
> 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
[...]
> 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 :)
>
> /* 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?
> 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
>
> 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
> }
>
> 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
[...]
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
[...]
> 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
[...]
> 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
[...]
> 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);
[...]
avfilter.h:
[...]
> int64_t pts; ///< presentation timestamp in milliseconds
milliseconds is insufficient
1/AV_TIME_BASE would be better
[...]
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
[...]
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
[...]
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
[...]
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
[...]
> AVFilter vsrc_ppm =
this (and others) should have some prefix to avoid name clashes
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- 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/20070817/7b57c830/attachment.pgp>
More information about the FFmpeg-soc
mailing list