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

[Michael Niedermayer]

On Sun, Apr 06, 2008 at 10:39:00PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Tue, Apr 01, 2008 at 10:49:39PM +0200, Vitor Sessak wrote:
>> [...]
>>>>
>>>> [...]
>>>>>     char tmp[20];
>>>>>
>>>>>     snprintf(tmp, 20, "%d", index);
>>>>                     ^^
>>>> sizeof
>>> Agreed. But are you ok with the instance name/instance lookup logic here?
>> Well as you mention it now, i realize what you are actually doing there.
>> No i do not like this design at all.
>> What i dont understand is why dont you just use pointers to 
>> AVFilterContext
>> instead of these numbers in char* identifers? I mean without looking
>> at the code common sense tells me whereever you store the numbers you 
>> could
>> store pointers to AVFilterContext as well.
>
> Well, it made more sense before the new parser was written. Changed that.
>
>>>>
>>>>>     if(!(filterdef = avfilter_get_by_name(name)) ||
>>>>>        !(filt = avfilter_open(filterdef, tmp))) {
>>>>>         av_log(&log_ctx, AV_LOG_ERROR,
>>>>>                "error creating filter '%s'\n", name);
>>>> The purpose of the context is so the user can associate it with some
>>>> specific instance of something. using a global context defeats this.
>>> Yes. Any suggestion?
>> Its not very important ...
>> The solution is to put that AVClass in some context, something related to
>> the parser ...
>
> I don't see any candidates...
>
> Another change is that I changed the notation of labels to '[in]' to allow 
> implementing parenthesis later.

[...]

>     if (avfilter_graph_add_filter(ctx, filt) < 0)
>         return NULL;
> 
>     if(avfilter_init_filter(filt, args, NULL)) {

the whitespace betweem if and ( looks inconsistant


[...]
> /**
>  * Consumes a string from *buf.
>  * @return a copy of the consumed string, which should be free'd after use
>  */
> static char *consume_string(const char **buf)
> {
>     char *out = av_malloc(strlen(*buf));

strlen+1 (for the zero at the end) ?



>     const char *in = *buf;
>     char *ret = out;
> 
>     consume_whitespace(buf);
> 
>     do{
>         char c = *in++;
>         switch (c) {
>         case '\\':
>             *out++= *in++;
>             break;
>         case '\'':
>             while(*in && *in != '\'')
>                 *out++= *in++;
>             if(*in) in++;
>             break;
>         case 0:
>         case ']':
>         case '[':
>         case '=':
>         case ',':

>             *out++= 0;
>             break;
>         default:
>             *out++= c;

i wonder if 
    c=0;
defailt:
    *out++= c;

would be too hackish ...


[...]
> /**
>  * 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)
> {
>     (*buf)++;
> 
>     *name = consume_string(buf);
> 
>     if (!*name[0])
>         goto fail;
> 
>     if (*(*buf)++ != ']')
>         goto fail;
> 
>     return;
>  fail:
>     av_freep(name);

>     av_log(&log_ctx, AV_LOG_ERROR, "Could not parse link name!\n");
> }

These are the kind of error messages people love in long complicated commands
id suggest to also print buf or something.
And possibly this could be done more generically, that is always print an
error message with the remaining string. Of course only if this is simpler
than custom av_log calls with specific arguments for each case.


[...]
> /**
>  * A linked-list of the inputs/outputs of the filter chain.
>  */
> typedef struct AVFilterInOut {
>     enum LinkType type;
>     char *name;
>     AVFilterContext *filter;
>     int pad_idx;
> 
>     struct AVFilterInOut *next;
> } AVFilterInOut;

I wonder if an array would be simpler than a linked list ...


> 
> static void free_inout(AVFilterInOut *head)
> {
>     while (head) {

>         AVFilterInOut *next;
>         next = head->next;

declaration and statement can be merged


>         av_free(head);
>         head = next;
>     }
> }
> 

> /**
>  * Parse "(a1)(link2) ... (etc)"

hmm [] () ?


>  */
> static int parse_inouts(const char **buf, AVFilterInOut **inout, int firstpad,
>                         enum LinkType type, AVFilterContext *filter)
> {

>     int pad = firstpad;

redundant


>     while (**buf == '[') {
>         AVFilterInOut *inoutn = av_malloc(sizeof(AVFilterInOut));
>         parse_link_name(buf, &inoutn->name);
>         inoutn->type = type;
>         inoutn->filter = filter;
>         inoutn->pad_idx = pad++;
>         inoutn->next = *inout;
>         *inout = inoutn;
>     }

what about [a b c] and such?


>     return pad;
> }
> 

> static const char *skip_inouts(const char *buf)
> {
>     while (*buf == '[') {

>         buf += strcspn(buf, "]");
>         buf++;

buf += strcspn(buf, "]") + 1;


>     }

superflous {}


[...]
>         // If the first filter has an input and none was given, it is
>         // implicitly the input of the whole graph.
>         if (pad == 0 && filter->input_count == 1) {
>             if(link_filter(in, inpad, filter, 0))
>                 goto fail;
>         }

so ',' links only one pad and ';' links nothing ?


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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20080410/ea07cb2b/attachment.pgp>