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;
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; ^^
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) [...] 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
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@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. このメールは再利用されたバイトでできている。
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
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs. This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable. -- Bobby Bingham Never trust atoms. Or anything made of atoms. このメールは再利用されたバイトでできている。
Hi On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable.
its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
Hi
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable.
its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions. I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing. Of course, I'm open to any ideas anybody has regarding a solution to colorspace negotiation... -- Bobby Bingham Never trust atoms. Or anything made of atoms. このメールは再利用されたバイトでできている。
Hi On Sun, Aug 19, 2007 at 12:36:15PM -0400, Bobby Bingham wrote:
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
Hi
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable.
its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions.
I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing.
ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
Hi On Sun, Aug 19, 2007 at 12:36:15PM -0400, Bobby Bingham wrote:
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
Hi
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer <michaelni@gmx.at> wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable.
its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions.
I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing.
Of course, I'm open to any ideas anybody has regarding a solution to colorspace negotiation...
ive thought about cs negotiation a little and ive come up with the following possible solution each filter link has 2 IDs (integers or pointers or something) one corresponding to the input into the link and one corresponding to the output (alternatively we could have always 2 links with a dummy filter between) now if 2 IDs anywhere within the filter graph are equal that means that the colorspace must be equal and each ID has a list of valid colorspaces now to solve it you just merge the 2 IDs on each link if both the corresponding lists have a common colorspace then merge is possible if not conversation is needed example: the case from above:
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
src b-->c filter c-->d dst b={A,C} c={A,B,C} d={B,C} first we merger b and c leading to src b-->b filter b-->d dst b={A,C} d={B,C} and next: b and d src b-->b filter b-->b dst b={C} [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact
Hi, and sorry to resurrect an old thread. Michael Niedermayer wrote:
Hi
On Sun, Aug 19, 2007 at 12:36:15PM -0400, Bobby Bingham wrote:
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer wrote:
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable. its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions.
I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing.
Of course, I'm open to any ideas anybody has regarding a solution to colorspace negotiation...
ive thought about cs negotiation a little and ive come up with the following possible solution
each filter link has 2 IDs (integers or pointers or something) one corresponding to the input into the link and one corresponding to the output (alternatively we could have always 2 links with a dummy filter between)
now if 2 IDs anywhere within the filter graph are equal that means that the colorspace must be equal and each ID has a list of valid colorspaces
now to solve it you just merge the 2 IDs on each link if both the corresponding lists have a common colorspace then merge is possible if not conversation is needed
example: the case from above:
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
src b-->c filter c-->d dst
b={A,C} c={A,B,C} d={B,C}
first we merger b and c leading to
src b-->b filter b-->d dst
b={A,C} d={B,C}
and next: b and d
src b-->b filter b-->b dst
b={C}
That will work if the filters have either input_fmt=output_fmt or fixed output format. But for the case of, for example, a gamma filter which would turn RGB24 into RGB32 and BGR24 into BGR32 it won't work. In schema: src --> filter --> dst src supports RGB24 and BGR24 dest supports BGR32 filter outputs BGR32 if input is BGR24 and RGB32 if input is RGB24 The solution without conversion is src BGR24 -> BGR24 filter BGR32 -> BGR32 dest -Vitor
On Sun, 04 Nov 2007 21:33:10 +0100 Vitor <vitor1001@gmail.com> wrote:
Hi, and sorry to resurrect an old thread.
Michael Niedermayer wrote:
Hi
On Sun, Aug 19, 2007 at 12:36:15PM -0400, Bobby Bingham wrote:
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer wrote:
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer wrote:
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
I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable. its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions.
I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing.
Of course, I'm open to any ideas anybody has regarding a solution to colorspace negotiation...
ive thought about cs negotiation a little and ive come up with the following possible solution
each filter link has 2 IDs (integers or pointers or something) one corresponding to the input into the link and one corresponding to the output (alternatively we could have always 2 links with a dummy filter between)
now if 2 IDs anywhere within the filter graph are equal that means that the colorspace must be equal and each ID has a list of valid colorspaces
now to solve it you just merge the 2 IDs on each link if both the corresponding lists have a common colorspace then merge is possible if not conversation is needed
example: the case from above:
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
src b-->c filter c-->d dst
b={A,C} c={A,B,C} d={B,C}
first we merger b and c leading to
src b-->b filter b-->d dst
b={A,C} d={B,C}
and next: b and d
src b-->b filter b-->b dst
b={C}
That will work if the filters have either input_fmt=output_fmt or fixed output format. But for the case of, for example, a gamma filter which would turn RGB24 into RGB32 and BGR24 into BGR32 it won't work. In schema:
src --> filter --> dst
src supports RGB24 and BGR24 dest supports BGR32 filter outputs BGR32 if input is BGR24 and RGB32 if input is RGB24
The solution without conversion is
src BGR24 -> BGR24 filter BGR32 -> BGR32 dest
-Vitor
As far as I'm aware, RGB24 and RGB32 only differ in that RGB32 has an unused byte for each pixel, where an alpha value would go for RGBA. So unless I miss something, I don't see why a filter would do that exactly. But it still raises a good point. When I had been brainstorming ideas for colorspace negotiation, I tried taking such cases into account, and it just made the whole thing so much more complicated. After seeing Michael's suggestion, I figured that in almost all cases, a filter will either have output format = input format, or will be some sort of colorspace conversion filter where the output and input formats don't need to be related. How common is this case where the possible output formats depend on the input format, but do not equal it, really? If it's rather uncommon like I suspect, I'd rather have those filters deal with it themselves than introduce all the extra complexity into libavfilter. For example, in your example above, the filter could without too much trouble also have a routine to perform the operation on RGB24 -> BGR32. It's just two bytes swapped from what it would normally do. -- Bobby, who finally found time to implement Michael's suggestion today ...
Hi Bobby Bingham wrote:
On Sun, 04 Nov 2007 21:33:10 +0100 Vitor <vitor1001@gmail.com> wrote:
Hi, and sorry to resurrect an old thread.
Michael Niedermayer wrote:
Hi
On Sun, Aug 19, 2007 at 12:36:15PM -0400, Bobby Bingham wrote:
On Sat, 18 Aug 2007 19:22:38 +0200 Michael Niedermayer wrote:
On Sat, Aug 18, 2007 at 12:12:08PM -0400, Bobby Bingham wrote:
On Fri, 17 Aug 2007 21:10:15 +0200 Michael Niedermayer wrote:
> 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 > I think I've got an idea which will be less absurdly flexible, and will support graphs like your example without requiring conversion. The only thing is that I might need to set the restriction that all the inputs of a filter must always be operating on the same colorspace, and similar for outputs.
This of course doesn't affect all those filters with only simple inputs and outputs. And I expect most filter authors would only support this case anyway. But before I start coding it, I want to check if such a restriction would be acceptable. its probably ok (i cant think of a case where a filter would want 2 inputs with differing colorspace)
Unfortunately, I thought of a case. The filtergraph filter, being simply a composite of various subfilters, may legitimately have inputs with multiple colorspaces. And doing so may optimize the number of conversions.
I'm afraid the more I look at the problem, the more I realize it's not something I can come up with a good solution to by Monday. I think I'll add support for autoconversion to the current system so that it at least works in all cases, and I'll have to work on a better system after the end of summer of code, because there are enough other areas I can actually make improvements on before the deadline if I don't get hung up on this one thing.
Of course, I'm open to any ideas anybody has regarding a solution to colorspace negotiation... ive thought about cs negotiation a little and ive come up with the following possible solution
each filter link has 2 IDs (integers or pointers or something) one corresponding to the input into the link and one corresponding to the output (alternatively we could have always 2 links with a dummy filter between)
now if 2 IDs anywhere within the filter graph are equal that means that the colorspace must be equal and each ID has a list of valid colorspaces
now to solve it you just merge the 2 IDs on each link if both the corresponding lists have a common colorspace then merge is possible if not conversation is needed
example: the case from above:
> 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 src b-->c filter c-->d dst
b={A,C} c={A,B,C} d={B,C}
first we merger b and c leading to
src b-->b filter b-->d dst
b={A,C} d={B,C}
and next: b and d
src b-->b filter b-->b dst
b={C}
That will work if the filters have either input_fmt=output_fmt or fixed output format. But for the case of, for example, a gamma filter which would turn RGB24 into RGB32 and BGR24 into BGR32 it won't work. In schema:
src --> filter --> dst
src supports RGB24 and BGR24 dest supports BGR32 filter outputs BGR32 if input is BGR24 and RGB32 if input is RGB24
The solution without conversion is
src BGR24 -> BGR24 filter BGR32 -> BGR32 dest
-Vitor
As far as I'm aware, RGB24 and RGB32 only differ in that RGB32 has an unused byte for each pixel, where an alpha value would go for RGBA. So unless I miss something, I don't see why a filter would do that exactly.
You're right... Actually, when I thought about my example I was thinking more of RGBA and YUVA, but I didn't find any PIX_FMT_YUVA...
But it still raises a good point. When I had been brainstorming ideas for colorspace negotiation, I tried taking such cases into account, and it just made the whole thing so much more complicated. After seeing Michael's suggestion, I figured that in almost all cases, a filter will either have output format = input format, or will be some sort of colorspace conversion filter where the output and input formats don't need to be related.
How common is this case where the possible output formats depend on the input format, but do not equal it, really? If it's rather uncommon like I suspect, I'd rather have those filters deal with it themselves than introduce all the extra complexity into libavfilter. For example, in your example above, the filter could without too much trouble also have a routine to perform the operation on RGB24 -> BGR32. It's just two bytes swapped from what it would normally do.
I agree. For such rare cases, I don't think it is really worth the extra complexity. I took some time to find the example of the gamma filter and I can't think of any other examples...
-- Bobby, who finally found time to implement Michael's suggestion today ...
Cool! Looks like we'll start the long "[PATCH]" threads in -devel soon :-) -Vitor
participants (3)
-
Bobby Bingham -
Michael Niedermayer -
Vitor