[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Sat May 24 23:01:12 CEST 2008
Michael Niedermayer wrote:
> On Sat, May 17, 2008 at 04:34:16PM +0200, Vitor Sessak wrote:
>> 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).
>
> Iam not completely happy with the parser but i have no clear idea on how to
> simplify it so commit it. We can simplify/clean it up when someone has an
> idea how.
Commited.
-Vitor
More information about the ffmpeg-devel
mailing list