[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Sat May 10 19:04:21 CEST 2008
Hi
Michael Niedermayer wrote:
> On Fri, May 09, 2008 at 08:44:23PM +0200, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Mon, May 05, 2008 at 10:01:36PM +0200, Vitor Sessak wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Wed, Apr 23, 2008 at 11:05:37PM +0200, Vitor Sessak wrote:
>>>>>> Hi and thanks for the review
>>>>> [...]
>>>>>>>> p->filter = filter;
>>>>>>>> p->pad_idx = pad;
>>>>>>>> p->next = *openLinks;
>>>>>>>> *openLinks = p;
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> if(*currInputs) {
>>>>>>>> av_log(log_ctx, AV_LOG_ERROR,
>>>>>>>> "Too many inputs specified for the \"%s\" filter.\n",
>>>>>>>> filter->name);
>>>>>>>> return -1;
>>>>>>>> }
>>>>>>>>
>>>>>>>> pad = filter->output_count;
>>>>>>>> while(pad) {
>>>>>>>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>>>>>>> pad--;
>>>>>>>> currlinkn->name = NULL;
>>>>>>>> currlinkn->type = LinkTypeOut;
>>>>>>>> currlinkn->filter = filter;
>>>>>>>> currlinkn->pad_idx = pad;
>>>>>>>> currlinkn->next = *currInputs;
>>>>>>>> *currInputs = currlinkn;
>>>>>>> somehow i thik this can be factored with the above as well
>>>>>> Maybe, but without worsening a lot readability?
>>>>> We need to find a way to simplify the whole parser without reducing
>>>>> readability.
>>>>> Its much more complex than i would like ...
>>>> Do you really think it can be so much more simpler? The "complex" part
>>>> of it (free_inout(), extract_inout(), link_filter_inouts(),
>>>> parse_inputs(), parse_outputs(), avfilter_parse_graph()) make 240 lines
>>>> of code.
>>> What i really dont like on the code is that it is complex and at the
>>> same time only supports a small subset of what we wanted to support.
>>> 300 lines would be fine for a complete implemantation with ; * () user
>>> defined filters, and all the other things discussed
>>> Saying ok to the code to then see it grow to 3 times that to support
>>> everything is something i really dont want to see ...
>>> Also when i just look at the API i cant help but it feels quite hostile
>>> to an implemantation of "user defined filters", and not only that.
>>> [...]
>>>>>>>> AVFilterInOut *currlinkn =
>>>>>>>> av_malloc(sizeof(AVFilterInOut));
>>>>>>>>
>>>>>>>> currlinkn->name = name;
>>>>>>>> currlinkn->type = LinkTypeIn;
>>>>>>>> currlinkn->filter = NULL;
>>>>>>>> currlinkn->pad_idx = pad;
>>>>>>>> currlinkn->next = *currInputs;
>>>>>>>> *currInputs = currlinkn;
>>>>>>> Ive seen similar code a few times already here somewhere ...
>>>>>> This code appears three times. But doing something like
>>>>>>
>>>>>> currlinkn = new_link(NULL, LinkTypeOut, filter, pad, *currInputs);
>>>>>>
>>>>>> wouldn't make this new_link() function a senseless wrapper?
>>>>> maybe iam not sure, do what you prefer
>>>> I did instead
>>>>
>>>> AVFilterInOut tmp_link = { LinkTypeIn,
>>>> name,
>>>> NULL,
>>>> pad,
>>>> *currInputs,
>>>> };
>>>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>>> *currlinkn = tmp_link;
>>>>
>>>> I think it is more readable.
>>> I dont, i prefered the original
>>> also
>>> currlinkn->next = *currInputs;
>>> *currInputs = currlinkn;
>>> is duplicated, its a insert_blah()
>> done
>>
>
>>>>> Also name could be moved to AVFilterLink, that way we would still have
>>>>> the names after the filter graph is build as a sideeffect.
>>>> I agree that would be a nice side-effect (that could be done even
>>>> without changing AVFilterInOut).
>>> What i was aiming at was to remove the AVFilterInOut struct completely.
>>> I just am not sure yet if its a good idea or not, but moving name out is a
>>> good idea even on its own.
>>> Leaving it in AVFilterInOut while adding it to AVFilterLink would be just
>>> a duplicate field.
>> Yes, but I find ugly to make a linked list of AVFilterLink to use either
>> only src and srcpad or only dst and dstpad. And I don't see a non-ugly
>> way to use both...
>
> Well it cannot work without using both, as a complete AVFilterLink needs both
> set to be a complete link.
>
> Also if it allows some simplificatins then
> src/dst* could be changed to filter[2] and filter_pad[2] in AVFilterLink
>
>
> [...]
>>> [...]
>>>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>>>> AVFilterInOut **openLinks, AVClass *log_ctx)
>>>> {
>>>> int pad = 0;
>>>>
>>>> while(**buf == '[') {
>>>> char *name;
>>>> AVFilterInOut *link_to_add;
>>>> AVFilterInOut *match;
>>>>
>>>> parse_link_name(buf, &name, log_ctx);
>>>>
>>>> if(!name)
>>>> return -1;
>>>>
>>>> /* First check if the label is not in the openLinks list */
>>>> match = extract_inout(name, openLinks);
>>>>
>>>> if(match) {
>>>> /* A label of a open link. Make it one of the inputs of the
>>>> next
>>>> filter */
>>>> if(match->type != LinkTypeOut) {
>>>> av_log(log_ctx, AV_LOG_ERROR,
>>>> "Label \"%s\" appears twice as input!\n",
>>>> match->name);
>>>> return -1;
>>>> }
>>> Just a totally wild idea, what about spliting openLinks into a list of
>>> open inputs and a list of open outputs? that would avoid these type checks
>>> and possibly the type field as well?
>> That was a good idea. Done, and it simplified slightly the code.
>
> great, i already started to be depressed by the lack of sucessfull
> simplifications.
It is nice indeed, but I'm afraid it is not the groundbreaking
simplification you want to get it in svn.
>
> [...]
>> 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.
-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 10312 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080510/60b76022/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/20080510/60b76022/attachment.h>
More information about the ffmpeg-devel
mailing list