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

Michael Niedermayer michaelni
Tue Apr 1 22:28:43 CEST 2008


On Sun, Mar 30, 2008 at 01:50:51PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Wed, Mar 19, 2008 at 08:03:54PM +0100, Vitor Sessak wrote:
>>> Michael Niedermayer wrote:
>>>> On Tue, Mar 18, 2008 at 09:04:18PM +0100, Vitor Sessak wrote:
>>>>> 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...
>>>> hmm, well, leave log_ctx for now this is pretty much irrelevant we dont 
>>>> really
>>>> need a user supplied ctx it was just an idea ...
>>>>
>>>>
>>>>>>> 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).
>>>> In which case of consume_char() use can the removed check cause it
>>>> to go over the end of the buffer without a goto fail or similar first?
>>> Well, it always will have a goto fail, but sometimes in "fail:" the 
>>> function just returns an empty string and then the code flux continues, 
>> Does that mean the parser silently accepts invalid nonsense? This is not
>> good.
>>> and can try to consume one more char. One option would be making every 
>>> function that uses have a return value to say if it failed or not (which 
>>> would need to be checked). In my opinion it'll result in more complex 
>>> code.
>>>
>>>>
>>>>>>> /**
>>>>>>>  * 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) 
>>>> yes, thats why i brought it up ...
>>> If you think it is important, I can try to implement a "\" escape for 
>>> those cases. Or it can just be added when someone sees that it is needed.
>> its just
>> do{
>>     int c= *in++;
>>     if(c == '\\')
>>         *out++= *in++;
>>     else if(c== '\''){
>>         while(*in && *in != '\'')
>>             *out++= *in++;
>>         if(*in) in++;
>>     }else
>>         *out++= c;
>> }while(out[-1]);
>>>>> 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)
>>>>>
>>>>> ?
>>>> no, IMHO just pass the single string, its very easy for the filter to 
>>>> feed
>>>> this to sscanf()
>>> Unless in this case:
>>>
>>> (in) drawtext='special char!@:#!':arial:12 (out)
>>>
>>> (*args will have "special char!@:#!:arial:12")
>> put the string to print last and theres no problem with special chars
>
> Next version attached. I hope I didn't forgot any comment.

[...]
> /* TODO: insert in sorted order */
> void avfilter_graph_add_filter(AVFilterGraph *graph, AVFilterContext *filter)
> {
>     graph->filters = av_realloc(graph->filters,
>                                 sizeof(AVFilterContext*) * ++graph->filter_count);
>     graph->filters[graph->filter_count - 1] = filter;
> }

This looks a little risky if one considers that graph->filters can be a NULL
pointer.


> 
> /* search intelligently, once we insert in order */
> AVFilterContext *avfilter_graph_get_filter(AVFilterGraph *graph, char *name)

doxygen @todo/@fixme or something ...


> {
>     int i;
> 
>     if(!name)
>         return NULL;

where is this function called with NULL ?

> 
>     for(i = 0; i < graph->filter_count; i ++)
>         if(graph->filters[i]->name && !strcmp(name, graph->filters[i]->name))
>             return graph->filters[i];
> 
>     return NULL;
> }
> 

> static int query_formats(AVFilterGraph *graph)
> {

Now i dont think this belongs in the parser code ...
There should be IMHO
A. code for a filter graph (a single flat filter graph that is a filter graph
   is not a filter) If we ever see the need for having filter graphs be
   filters this can be added later as a filter easily. Not weirdly
   intermingled with everything else.
B. the parser


[...]
>     char tmp[20];
> 
>     snprintf(tmp, 20, "%d", index);
                    ^^
sizeof


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


[...]
> /**
>  * Copy the first size bytes of input string to a null-terminated string,
>  * removing any control character. Ex: "aaa'bb'c\'c\\" -> "aaabbc'c\"
>  */
> static void copy_unquoted(char *out, const char *in, int size)
> {
>     int i;
>     for (i=0; i < size; i++) {
>         if (in[i] == '\'')
>             continue;
>         else if (in[i] == '\\') {
>             if (i+1 == size) {
>                 *out = 0;
>                 return;
>             }
>             i++;
>         }
>         *out++ = in[i];
>     }
>     *out=0;
> }

This behaves very strange, that is
'\' -> '


[...]
>     while(1) {
>         *buf += strcspn(*buf, " ()=,'\\");
>         if (**buf == '\\')
>             *buf+=2;
>         else
>             break;
>     }
> 
>     if (**buf == '\'') {
>         const char *p = *buf;
>         do {
>             p++;
>             p = strchr(p, '\'');
>         } while (p && p[-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);
>     copy_unquoted(ret, start, size-1);

The code above is really ugly.

You are doing the unquoting twice and not even 1/3 of that code is needed

The following should be doing this slightly more sanely.

do{
    int c= *in++;
    if(c == '\\')
        *out++= *in++;
    else if(c== '\''){
        while(*in && *in != '\'')
            *out++= *in++;
        if(*in) in++;
    }else if(*in == end_chr)    or is_special(*in)
        *out++= 0;
    else
        *out++= c;
}while(out[-1])


[...]
>         } else if (!strcmp(inout->name, "out")) {
>             has_out = 1;
>             snprintf(tmp, 20, "%d", inout->instance);
>             if(!(filt = avfilter_graph_get_filter(graph, tmp))) {
>                 av_log(&log_ctx, AV_LOG_ERROR, "filter owning exported pad does not exist\n");
>                 goto fail;
>             }
> 
>             if(avfilter_link(filt, inout->pad_idx, out, outpad)) {
>                 av_log(&log_ctx, AV_LOG_ERROR, "cannot create link between source and destination filters\n");
>                 goto fail;

>         }
> 
>         } else {

these dont look correctly indented


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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20080401/f4ec8dcd/attachment.pgp>



More information about the ffmpeg-devel mailing list