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

[Michael Niedermayer]

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


>
>> [...]
>>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>>>                         AVFilterInOut **openLinks, AVClass *log_ctx)
>>> {
>>>     int pad = 0;
>>>     AVFilterInOut *p;
>>>
>>>     while (**buf == '[') {
>>>         char *name;
>>>
>>>         parse_link_name(buf, &name, log_ctx);
>>>
>>>         if(!name)
>>>             return -1;
>>>
>>>         /* First check if the label is not in the openLinks list */
>>>         p = extract_inout(name, openLinks);
>>>
>>>         /* Not in the list, so add it as an input */
>>>         if(!p) {
>>>             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


[...]
> static void consume_whitespace(const char **buf)
> {
>     *buf += strspn(*buf, " \n\t");
> }

i think a

static int count_whitespace(const char *buf)
{
    return strspn(buf, " \n\t");
}

would be clearer

a(&x)
vs.
x += a(x)

second makes it much clearer that the pointer is increased.


[...]
> static int link_filter_inouts(AVFilterContext *filter,
>                               AVFilterInOut **currInputs,
>                               AVFilterInOut **openLinks, AVClass *log_ctx)
> {

>     int pad = 0;
> 
>     pad = filter->input_count;

hmm


[...]
> static int parse_outputs(const char **buf, AVFilterInOut **currInputs,
>                          AVFilterInOut **openLinks, AVClass *log_ctx)
> {
>     int pad = 0;
> 
>     while(**buf == '[') {
>         char *name;
>         AVFilterInOut *match;
> 
>         AVFilterInOut *input = *currInputs;
>         *currInputs = (*currInputs)->next;
> 
>         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. Link it. */
>             if(match->type != LinkTypeIn) {
>                 av_log(log_ctx, AV_LOG_ERROR,
>                        "Label \"%s\" appears twice as output!\n", match->name);
>                 return -1;
>             }
> 
>             if(link_filter(input->filter, input->pad_idx,
>                            match->filter, match->pad_idx, log_ctx) < 0)
>                 return -1;
>             av_free(match);
>             av_free(input);
>         } else {
>             /* Not in the list, so add the first input as a openLink */
>             input->next = *openLinks;
>             input->type = LinkTypeOut;
>             input->name = name;
>             *openLinks = input;
>         }

Maybe some of the code surrounding link_filter() could be simplified if
avfilter_link wasnt used? I mean now the code needs to handle a few cases
1. Input filter available, output not
2. Input filter available, output available
3. Input filter not available, output available

with:

typedef struct AVFilterInOut {
    const char *name;
    AVFilterLink *link;

    struct AVFilterInOut *next;
} AVFilterInOut;

Either side could be linked no matter if the other is. What remains is
just a common check "do we have both then remove the AVFilterInOut"

Also name could be moved to AVFilterLink, that way we would still have
the names after the filter graph is build as a sideeffect.


[...]
> /**
>  * Add to a graph a graph described by a string.
>  * @param graph   the filter graph where to link the parsed graph context
>  * @param filters string to be parsed
>  * @param inouts  linked list to the inputs and outputs of the graph

I wonder if it would be cleaner or not to just pass a list of inputs and
get a list of outputs after the function returns.


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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20080424/a13a959c/attachment.pgp>