[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description

[Vitor Sessak]

Hi

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...

>> /**
>>  * Parse "[linkname]"
>>  * @arg name a pointer (that need to be free'd after use) to the name between
>>  *           parenthesis
>>  */
>> static void parse_link_name(const char **buf, char **name, AVClass *log_ctx)
>> {
>>     const char *start = *buf;
>>     (*buf)++;
>>
>>     *name = consume_string(buf);
>>
>>     if(!*name[0]) {
>>         av_log(log_ctx, AV_LOG_ERROR,
>>                "Bad (empty?) label found in the following: \"%s\".\n", start);
>>         goto fail;
>>     }
>>
>>     if(*(*buf)++ != ']') {
>>         av_log(log_ctx, AV_LOG_ERROR,
>>                "Mismatched '[' found in the following: \"%s\".\n", start);
>>     fail:
>>         av_freep(name);
>>     }
>> }
> 
> this function should return char * not void.

done

>> static void free_inout(AVFilterInOut *head)
>> {
>>     while(head) {
>>         AVFilterInOut *next = head->next;
>>         av_free(head);
>>         head = next;
>>     }
>> }
>>
> 
>> static AVFilterInOut *extract_inout(const char *label, AVFilterInOut **links)
>> {
>>     AVFilterInOut *ret;
>>
>>     while(*links && strcmp((*links)->name, label))
>>         links = &((*links)->next);
>>
>>     ret = *links;
>>
>>     if(ret)
>>         *links = ret->next;
>>
>>     return ret;
>> }
> 
> Maybe it would be cleaner to split this in find and remove ?

Why? Also it forces the use of (*match)->field which is more ugly...

> 
> static AVFilterInOut **find_inout(const char *label, AVFilterInOut **links)
> {
>     while(*links && strcmp((*links)->name, label))
>         links = &((*links)->next);
> 
>     return links;
> }
> 
> x= find_inout()
> if(*x)
>     *x = (*x)->next;
> 
> the if() exists already after all extract_inout() calls
>>
>> static int link_filter_inouts(AVFilterContext *filter,
>>                               AVFilterInOut **currInputs,
>>                               AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>>     int pad = filter->input_count;
>>
>>     while(pad--) {
> 
>>         AVFilterInOut *p = *currInputs;
>>         *currInputs = (*currInputs)->next;
>>         if(!p) {
> 
>>             av_log(log_ctx, AV_LOG_ERROR,
> 
> unreachable, this would have segfaulted already

fixed

> [...]
>> 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.

-Vitor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 10604 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080509/35f11354/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/20080509/35f11354/attachment.h>