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

[Vitor Sessak]

Hi

Michael Niedermayer wrote:
> 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.

Well, for example ffmpeg.c, which context will it pass to the parser? 
But maybe a good idea would be passing a context and, if NULL is passed, 
using log_ctx...

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

The idea is to have a safe parser. That way, you can call consume_char 
as much as you like, it'll never go past the end of the string and 
consume_string will never return NULL (but maybe an empty string).

>> /**
>>  * 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:$%&'"=\
> ?

$ ffmpeg -i in.avi -vfilters "drawtext='various special chars:$%&\"=\\'" 
out.avi

But there are two things that is complicated: the "'" char that can't be 
escaped by now (and it's the only one) and the ":" (that is not an 
especial character to this parser, but is parsed independently by each 
filter).

Maybe parsing the different args in the parser and instead of passing to 
the filter something like

static int init(AVFilterContext *ctx, const char *args, void *opaque)

passing instead

static int init(AVFilterContext *ctx, int argc, const char **argv, void 
*opaque)

?

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

Agree.

> 
> 
>> /**
>>  * 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

ok

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

Well, but should I allocate and free memory just for that?

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

agreed.

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

Agree.

> 
> 
>> /**
>>  * 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

Agree.

> 
> 
> [...]
> 
>>         AVFilterInOut *inoutn;
>>         inoutn = av_malloc(sizeof(AVFilterInOut));
> 
> can be merged

Agree.

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

Good question. I didn't start yet to work with avfiltergraph.c, but I'd 
say Bobby's idea was that a third party app using the library would 
prefer to assemble its graphs using such a struct. Anyway, do you mind 
if I leave this for when I send avfiltergraph.c for review?

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

Agree.

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

ok.

New version attached.

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avfiltergraphdesc.c
Type: text/x-csrc
Size: 8990 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080318/d62d8357/attachment.c>