[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Sat May 17 16:34:16 CEST 2008
Michael Niedermayer wrote:
> 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?
Not after they are added to the graph (they will be freed when the graph
is destroyed). Fixed for (avfilter_graph_add_filter(ctx, filt) < 0).
>
>
>> 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?
Indeed, it doesn't make much sense. Fixed.
>> 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
No more memleaks according to valgrind (at least no more than when not
invoking with -vfilters).
> [...]
>> /**
>> * 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
ok
>> 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
Removed the pad var (but I think that returning the pad from
parse_outputs() may be useful one day, so I kept it).
-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 10268 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080517/67a40b6f/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.h
Type: text/x-chdr
Size: 1701 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080517/67a40b6f/attachment.h>
More information about the ffmpeg-devel
mailing list