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

[Michael Niedermayer]

On Mon, May 05, 2008 at 10:01:36PM +0200, Vitor Sessak wrote:
> Hi
>
> 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() 


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


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


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

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


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20080507/66a56177/attachment.pgp>