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

[Michael Niedermayer]

On Mon, Mar 17, 2008 at 08:12:08PM +0100, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
[...]
> /**
>  * For use in av_log
>  */
> static const char *log_name(void *p)
> {
>     return "Filter parser";
> }
> 
> static const AVClass filter_parser_class = {
>     "Filter parser",
>     log_name
> };
> 
> static const AVClass *log_ctx = &filter_parser_class;

I was more thinking of passing a context as argument into the parser.


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

> /**
>  * get the next non-whitespace char
>  */
> static char consume_char(const char **buf)
> {
>     char out;
>     consume_whitespace(buf);
> 
>     out = **buf;
> 
>     if (out)
>         (*buf)++;
> 
>     return out;
> }

is there some place which needs the extra-complex checks? or would:

consume_whitespace(buf);
return *(*buf)++;

work as well ?



> 
> /**
>  * remove the quotation marks from a string. Ex: "aaa'bb'cc" -> "aaabbcc"
>  */
> static void unquote(char *str)
> {
>     char *p1, *p2;
>     p1=p2=str;
>     while (p1[0] != 0) {
>         if (p1[0] == '\'') p1++;
>         p2[0] = p1[0];
>         p1++;
>         p2++;
>     }
> 
>     p2[0] = 0;
> }

How do we support
drawtext=various special chars:$%&'"=\
?

ignoring that the code could be simplified to:
while(*p1){
    if(*p1 != '\'')
        *p2++ = *p1;
    p1++;
}


> 
> /**
>  * 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)
> {
>     const char *start;
>     char *ret;
>     int size;
> 
>     consume_whitespace(buf);
> 

>     if (!(**buf))
>         return av_mallocz(sizeof(char));

sizeof(char)=1


> 
>     start = *buf;
> 
>     *buf += strcspn(*buf, " ()=,'");
> 
>     if (**buf == '\'') {
>         char *p = strchr(*buf + 1, '\'');
>         if (p)
>             *buf = p + 1;
>         else
>             *buf += strlen(*buf); // Move the pointer to the null end byte
>     }
> 
>     size = *buf - start + 1;
>     ret = av_malloc(size*sizeof(char));

>     memcpy(ret, start, size - 1);
>     ret[size-1] = 0;
> 
>     unquote(ret);

A unquote with src & dst seems more natural.


> 
>     return ret;
> }
> 

> /**
>  * Parse "(linkname)"
>  * @arg name a pointer (that need to be free'd after use) to the name between
>  *           parenthesis
>  */
> static int parse_link_name(const char **buf, char **name)
> {
>     if (consume_char(buf) != '(')
>         goto fail;

This cannot fail, so it should be an assert() if at all


> 
>     *name = consume_string(buf);
> 
>     if (!*name[0])
>         goto fail;
> 
>     if (consume_char(buf) != ')')
>         goto fail;
> 
>     return 0;
> 
>  fail:
>     av_freep(name);
>     av_log(&log_ctx, AV_LOG_ERROR, "Could not parse link name!\n");
>     return AVERROR_INVALIDDATA;
> }

also somehow it seems that it would be more natural to return the link
name or NULL than a error code.


> 
> /**
>  * Parse "filter=params"
>  * @arg name a pointer (that need to be free'd after use) to the name of the
>  *           filter
>  * @arg ars  a pointer (that need to be free'd after use) to the args of the
>  *           filter
>  */
> static int parse_filter(const char **buf, char **name, char **opts)
> {
>     *name = consume_string(buf);
> 
>     if (**buf == '=') {
>         consume_char(buf);
>         *opts = consume_string(buf);
>     } else {
>         *opts = NULL;
>     }
> 
>     return 0;
> }

as this always returns 0, theres no sense in returning that at all


[...]

>         AVFilterInOut *inoutn;
>         inoutn = av_malloc(sizeof(AVFilterInOut));

can be merged


>         parse_link_name(buf, &inoutn->name);
>         inoutn->type = type;
>         inoutn->instance = instance;
>         inoutn->pad_idx = pad++;
>         inoutn->next = *inout;
>         *inout = inoutn;
>     }
>     return pad;
> }
> 

> static AVFilterGraphDesc *parse_chain(const char *filters, int has_in)

Why does the parser work with AVFilterGraphDesc* instead of 
libavfilter/avfilter.h structs ?


[...]
>             for (p=inout->next;
>                  p!=NULL && strcmp(p->name,inout->name); p = p->next);

!=NULL is superflous


[...]
>     while (head) {
>         AVFilterInOut *next;
>         next = head->next;
>         av_free(head);
>         head = next;
>     }
> 
>     if (!has_in) {
>         ret->inputs = av_mallocz(sizeof(AVFilterGraphDescExport));
>         ret->inputs->filter = 0;
>     }
>     if (!has_out) {
>         ret->outputs = av_mallocz(sizeof(AVFilterGraphDescExport));
>         ret->outputs->filter = index-1;
>     }
> 
>     return ret;
> 
>  fail:
>     while (head) {
>         AVFilterInOut *next;
>         next = head->next;
>         av_free(head);
>         head = next;
>     }

dupicate code


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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20080318/9835148f/attachment.pgp>