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

[Vitor Sessak]

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

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

> [...]
>>>>     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?
> 
> currently you memcpy into dst and then
> unquote(dst);
> what i meant was
> unquote(dst, src); which would avoid the memcpy and be cleaner IMHO

I agree, I didn't see the memcpy.

> 
> 
> [...]
>>>>         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?
> 
> Hmm, this is moving the review in a somewhat entangled situation.
> I really do not like avfiltergraph.*. So if this
> depends on it then the parser will have to wait for a clear demonstration of
> the need of avfiltergraph.
> 
> The way i imagined it was that the parser would just create filters and links
> as it parses them with no intermediate APIs.
Well, even if avfilter_vf_graph goes away (and together with it 3/4 of 
avfiltergraph.c), there are some code in avfiltergraph.c that will stay, 
like the code that inserts the "scale" filter if there is no compatible 
format or the code that actually init and link the filter chain... Only 
after I get rid of avfilter_vf_graph that it would be clear if keeping 
the AVFilterGraphDesc struct makes the thing more or less clean.

-Vitor