[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Michael Niedermayer
michaelni
Sun May 18 00:02:28 CEST 2008
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20080518/69df6d2e/attachment.pgp>
More information about the ffmpeg-devel
mailing list