[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Michael Niedermayer
michaelni
Sun May 11 21:36:07 CEST 2008
On Sat, May 10, 2008 at 07:04:21PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
[...]
>> [...]
>>> static int link_filter_inouts(AVFilterContext *filter,
>>> AVFilterInOut **currInputs,
>>> AVFilterInOut **openInputs, AVClass
>>> *log_ctx)
>>> {
>>> int pad = filter->input_count;
>>>
>>> while(pad--) {
>>> AVFilterInOut *p = *currInputs;
>>> if(!p) {
>>> av_log(log_ctx, AV_LOG_ERROR,
>>> "Not enough inputs specified for the \"%s\"
>>> filter.\n",
>>> filter->filter->name);
>>> return -1;
>>> }
>>> *currInputs = (*currInputs)->next;
>>>
>>> if(p->filter) {
>>> if(link_filter(p->filter, p->pad_idx, filter, pad, log_ctx))
>>> return -1;
>>> av_free(p);
>>> } else {
>>> p->filter = filter;
>>> p->pad_idx = pad;
>>> p->next = *openInputs;
>>> *openInputs = p;
>> duplicate
>
> fixed
>
>>> }
>>> }
>> *currInputs = (*currInputs)->next;
>> can be moved to the end of the loop
>
> I don't think so. The line av_free(p) frees *currInputs.
>
>>>
>>> if(*currInputs) {
>>> av_log(log_ctx, AV_LOG_ERROR,
>>> "Too many inputs specified for the \"%s\" filter.\n",
>>> filter->filter->name);
>>> return -1;
>>> }
>>>
>>> pad = filter->output_count;
>>> while(pad--) {
>>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>> currlinkn->name = NULL;
>> use mallocz and this becomes unneeded
>
> Done everywhere.
>
>>> currlinkn->filter = filter;
>>> currlinkn->pad_idx = pad;
>>> insert_inout(currInputs, currlinkn);
>>> }
>>>
>>> return 0;
>>> }
>>>
>>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>>> AVFilterInOut **openOutputs, AVClass *log_ctx)
>>> {
>>> int pad = 0;
>>>
>>> while(**buf == '[') {
>>> char *name = parse_link_name(buf, log_ctx);
>>> AVFilterInOut *match;
>>>
>>> if(!name)
>>> return -1;
>>>
>>> /* First check if the label is not in the openOutputs list */
>>> match = extract_inout(name, openOutputs);
>>>
>>> if(match) {
>>> /* A label of a open link. Make it one of the inputs of the
>>> next
>>> filter */
>>> insert_inout(currInputs, match);
>>> } else {
>>> /* Not in the list, so add it as an input */
>>> AVFilterInOut *link_to_add =
>>> av_malloc(sizeof(AVFilterInOut));
>>>
>>> link_to_add->name = name;
>>> link_to_add->filter = NULL;
>>> link_to_add->pad_idx = pad;
>>> insert_inout(currInputs, link_to_add);
>>> }
>> Try to simplify your code _please_!
>> if(!match){
>> match= av_mallocz(sizeof(AVFilterInOut));
>> match->name = name;
>> match->pad_idx = pad;
>> }
>> insert_inout(currInputs, match);
>
> 10l, thanks
>
>>> *buf += consume_whitespace(*buf);
>>> pad++;
>>> }
>>>
>>> return pad;
>>> }
>>>
>>> static int parse_outputs(const char **buf, AVFilterInOut **currInputs,
>>> AVFilterInOut **openInputs,
>>> AVFilterInOut **openOutputs, AVClass *log_ctx)
>>> {
>>> int pad = 0;
>>>
>>> while(**buf == '[') {
>>> char *name = parse_link_name(buf, log_ctx);
>>> AVFilterInOut *match;
>>>
>>> AVFilterInOut *input = *currInputs;
>>> *currInputs = (*currInputs)->next;
>> belongs to the end of the loop
>
> Same problem: "av_free(input)"
>
>> [...]
>>> /**
>>> * Parse a string describing a filter graph.
>>> */
>>> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
>>> AVFilterInOut *openInputs,
>>> AVFilterInOut *openOutputs, AVClass *log_ctx)
>>> {
>>> int index = 0;
>>> char chr = 0;
>>> int pad = 0;
>>>
>>> AVFilterInOut *currInputs = NULL;
>>>
>>> do {
>>> AVFilterContext *filter;
>>> filters += consume_whitespace(filters);
>>>
>>> pad = parse_inputs(&filters, &currInputs, &openOutputs, log_ctx);
>>>
>>> if(pad < 0)
>>> goto fail;
>>>
>>> if(!(filter = parse_filter(&filters, graph, index, log_ctx)))
>>> goto fail;
>> inconsistant style
>
> Fixed.
[...]
> static AVFilterContext *create_filter(AVFilterGraph *ctx, int index,
> const char *name, const char *args,
> AVClass *log_ctx)
> {
> AVFilterContext *filt;
>
> AVFilter *filterdef;
> char inst_name[30];
>
> snprintf(inst_name, sizeof(inst_name), "Parsed filter %d", index);
>
> filterdef = avfilter_get_by_name(name);
>
> if(!filterdef) {
> av_log(log_ctx, AV_LOG_ERROR,
> "no such filter: '%s'\n", name);
> return NULL;
> }
>
> filt = avfilter_open(filterdef, inst_name);
> if(!filt) {
> av_log(log_ctx, AV_LOG_ERROR,
> "error creating filter '%s'\n", name);
> return NULL;
> }
>
> if(avfilter_graph_add_filter(ctx, filt) < 0)
> return NULL;
>
> if(avfilter_init_filter(filt, args, NULL)) {
> av_log(log_ctx, AV_LOG_ERROR,
> "error initializing filter '%s' with args '%s'\n", name, args);
> return NULL;
> }
are there any memleaks in the error returns?
>
> return filt;
> }
>
> /**
> * Parse "filter=params"
> * @arg name a pointer (that need to be free'd after use) to the name of the
> * filter
> * @arg ars a pointer (that need to be free'd after use) to the args of the
> * filter
> */
what is @arg do you mean @param or is @arg valid too? and what is ars?
> static AVFilterContext *parse_filter(const char **buf, AVFilterGraph *graph,
> int index, AVClass *log_ctx)
> {
> char *opts = NULL;
> char *name = consume_string(buf);
>
> if(**buf == '=') {
> (*buf)++;
> opts = consume_string(buf);
> }
>
> return create_filter(graph, index, name, opts, log_ctx);
are name/opts freed anywhere?
If no please run your code through valgrind to find all leaks, i suspect
there are more
[...]
> /**
> * Parse a string describing a filter graph.
> */
> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
whats the sense of the comment, there alraedy is one in the header
> AVFilterInOut *openInputs,
> AVFilterInOut *openOutputs, AVClass *log_ctx)
> {
> int index = 0;
> char chr = 0;
> int pad = 0;
>
> AVFilterInOut *currInputs = NULL;
>
> do {
> AVFilterContext *filter;
> filters += consume_whitespace(filters);
>
> pad = parse_inputs(&filters, &currInputs, &openOutputs, log_ctx);
>
> if(pad < 0)
> goto fail;
>
> filter = parse_filter(&filters, graph, index, log_ctx);
>
> if(!filter)
> goto fail;
>
> if(filter->input_count == 1 && !currInputs && !index) {
> /* First input can be ommitted if it is "[in]" */
> const char *tmp = "[in]";
> pad = parse_inputs(&tmp, &currInputs, &openOutputs, log_ctx);
> if(pad < 0)
> goto fail;
pad is unused besides the if() goto fail thus the variable isnt needed
> }
>
> if(link_filter_inouts(filter, &currInputs, &openInputs, log_ctx) < 0)
> goto fail;
>
> pad = parse_outputs(&filters, &currInputs, &openInputs, &openOutputs,
> log_ctx);
>
> if(pad < 0)
> goto fail;
same
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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-devel/attachments/20080511/ede0c143/attachment.pgp>
More information about the ffmpeg-devel
mailing list