[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Michael Niedermayer
michaelni
Fri Apr 18 15:08:34 CEST 2008
On Sat, Apr 12, 2008 at 04:40:24PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Fri, Apr 11, 2008 at 06:43:10PM +0200, Vitor Sessak wrote:
>>> Hi, and thanks for the review
>>>
>>> Michael Niedermayer wrote:
>>>> On Thu, Apr 10, 2008 at 08:55:24PM +0200, Vitor Sessak wrote:
>>>>> Michael Niedermayer wrote:
>>>>>> On Sun, Apr 06, 2008 at 10:39:00PM +0200, Vitor Sessak wrote:
>>>> [...]
>>>>>> [...]
>>>>>>> /**
>>>>>>> * 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 ...
>>>>> I'm not sure it would be really simpler. For me, it will only save one
>>>>> or two malloc lines...
>>>> Well, i will think more about this, but as is the parser currently looks
>>>> too complex for what it supports ...
>>> I agree it looks kind of complex for what it does, but a good part of the
>>> complexity comes from whitespace handling and giving decent error
>>> messages.
>>>
>>>> [...]
>>>>> Also, are you ok to commit it without svn history? It was rewritten
>>>>> from scratch...
>>>> Iam ok with ommiting unrelated history / what is prior to your rewrite
>>>> ...
>>>> [...]
>>>>> /**
>>>>> * 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 do not like this. This is a hack not the correct way to use av_log()
>>> Options:
>>>
>>> 1- Make AVFilterGraph a context for av_log
>>> a) Good points: Instance specific
>>> b) Bad points: Has nothing specifically to do with the parser
>>>
>>> 2- Make a AVFilterInOut linked list a member of a context
>>> a) Good points: The best candidate for a parser context
>>> b) Bad points: Useless struct
>>>
>>> 4- Malloc'ing a instance of AVClass in the beginning of
>>> avfilter_parse_graph and passing it to every function
>>> a) Good points: Instance specific
>>> b) Bad points: Useless function parameters
>>>
>>> 5- av_log(NULL, ...)
>> 6 - let the user provide a AVClass / NULL
>
> Ok, I'm with 6.
>
>> 7 - return the error message per char ** instead of through av_log()
>> 4 and 5 are unacceptable
>> i do like 3 ... especially because there is no 3 :)
>
> 10l
>
>>>>> static AVFilterContext *create_filter(AVFilterGraph *ctx, int index,
>>>>> char *name, char *args)
>>>> shouldnt name and args be const
>>> Yes
>>>
>>>> [...]
>>>>> /**
>>>>> * 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) + 1);
>>>>> const char *in = *buf;
>>>>> char *ret = out;
>>>>>
>>>>> consume_whitespace(buf);
>>>>>
>>>>> do{
>>>>> char c = *in++;
>>>> you change buf but use in, also why arent you useing just one variable
>>>> ...
>>> My original idea was to avoid constructs like *(*buf)++ = , but I'm not
>>> against it, so changed.
>> if you like sensless abstraction there is bytestream_get_byte()
>> [...]
>>>>> *buf = in-1;
>>>>> return ret;
>>>>> }
>>>>>
>>>>> /**
>>>>> * 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)
>>>>> {
>>>>> const char *start = *buf;
>>>>> (*buf)++;
>>>>>
>>>>> *name = consume_string(buf);
>>>>>
>>>>> if(!*name[0]) {
>>>>> av_log(&log_ctx, AV_LOG_ERROR,
>>>>> "Bad (empty?) label found in the following: \"%s\".\n",
>>>>> start);
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> if(*(*buf)++ != ']') {
>>>>> av_log(&log_ctx, AV_LOG_ERROR,
>>>>> "Mismatched '[' found in the following: \"%s\".\n",
>>>>> start);
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> return;
>>>>>
>>>>> fail:
>>>>> av_freep(name);
>>>>> }
>>>> you can at least move the fail: free to where the second goto is, this
>>>> also
>>>> makes the return redundant.
>>> consume_string() will alloc a string in both cases (an empty one in the
>>> first goto).
>> goto fail;
>> ...
>> goro fail;
>> }
>> return;
>> fail:
>> av_freep(name);
>> }
>> is the same as
>> goto fail;
>> ...
>> fail:
>> av_freep(name);
>> }
>> }
>
> Ok, I got it. Done.
>
>> [...]
>>>> [...]
>>>>> /**
>>>>> * Parse a string describing a filter graph.
>>>>> */
>>>>> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
>>>>> AVFilterContext *in, int inpad,
>>>>> AVFilterContext *out, int outpad)
>>>>> {
>>>>> AVFilterInOut *inout=NULL;
>>>>> AVFilterInOut *head=NULL;
>>>>>
>>>>> int index = 0;
>>>>> char chr = 0;
>>>>> int pad = 0;
>>>>> int has_out = 0;
>>>>>
>>>>> AVFilterContext *last_filt = NULL;
>>>>>
>>>>> consume_whitespace(&filters);
>>>>>
>>>>> do {
>>>>> AVFilterContext *filter;
>>>>> int oldpad = pad;
>>>>> const char *inouts = filters;
>>>>>
>>>>> // We need to parse the inputs of the filter after we create
>>>>> it, so
>>>>> // skip it by now
>>>>> filters = skip_inouts(filters);
>>>>>
>>>>> if(!(filter = parse_filter(&filters, graph, index)))
>>>>> goto fail;
>>>>>
>>>>> pad = parse_inouts(&inouts, &inout, chr == ',', LinkTypeIn,
>>>>> filter);
>>>>>
>>>>> if(pad < 0)
>>>>> goto fail;
>>>>>
>>>>> // 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;
>>>>> }
>>>>>
>>>>> if(chr == ',') {
>>>>> if(link_filter(last_filt, oldpad, filter, 0) < 0)
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> pad = parse_inouts(&filters, &inout, 0, LinkTypeOut, filter);
>>>>> chr = *filters++;
>>>>> index++;
>>>>> last_filt = filter;
>>>>> } while (chr == ',' || chr == ';');
>>>>>
>>>>> head = inout;
>>>>> for (; inout != NULL; inout = inout->next) {
>>>> Why dont you build the graph in 1 pass?
>>> Why? In the 2 pass version, I can be sure to find a match for every
>>> label. Also, it makes two nicely separated blocks of code, the linking
>>> can be cleanly moved to another function, for example.
>> It makes the code more complex ...
>
> It wasn't obvious at first it would be less complex after merging, but I
> think I succeeded in simplifying it a bit by doing so...
>
>>> /*
>>> * filter graphs
>> typo
>> or was there something else wrong hmmmmmmm ;)
>
> Wow, 100l
>
> A final question: should I, instead of having as a prototype
>
> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
> AVFilterContext *in, int inpad,
> AVFilterContext *out, int outpad,
> AVClass *log_ctx);
>
> use
>
> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
> AVFilterContext **in, int *inpad,
> AVFilterContext **out, int *outpad,
> AVClass *log_ctx);
>
> So I could extend it later to have several inputs/outputs without changing
> the public API? Are you ok with a NULL-termined array of (AVFilterContext
> *) for several inputs?
I like neither and i think that using filter list + pad index lists is
not very convenient. This is especially the case in a more complex
parser supporting more than "1 link from the previous filter".
Thus i think the API might change when the parser is improved to support
more of what was discussed.
For the moment the choice between the 2 doesnt matter IMHO.
[...]
> /**
> * Process a link. This funcion looks for a matching label in the *inout
> * linked list. If none is found, it adds this link to the list.
> */
> static int handle_link(char *name, AVFilterInOut **inout, int pad,
> enum LinkType type, AVFilterContext *filter,
> AVClass *log_ctx)
> {
> AVFilterInOut *p = *inout;
>
> for (; p && strcmp(p->name, name); p = p->next);
>
> if(!p) {
> // First label apearence, add it to the linked list
> AVFilterInOut *inoutn = av_malloc(sizeof(AVFilterInOut));
>
> inoutn->name = name;
> inoutn->type = type;
> inoutn->filter = filter;
> inoutn->pad_idx = pad;
> inoutn->next = *inout;
> *inout = inoutn;
> return 0;
> }
>
> if(p->type == LinkTypeIn && type == LinkTypeOut) {
> if(link_filter(filter, pad, p->filter, p->pad_idx, log_ctx) < 0)
> goto fail;
> } else if(p->type == LinkTypeOut && type == LinkTypeIn) {
> if(link_filter(p->filter, p->pad_idx, filter, pad, log_ctx) < 0)
> goto fail;
> } else {
> av_log(log_ctx, AV_LOG_ERROR,
> "Two links named '%s' are either both input or both output\n",
> name);
> goto fail;
> }
>
> p->filter = NULL;
>
> return 0;
> fail:
> return -1;
as return -1 is the only thing done after goto fail, you can as well replace
all goto fail be it.
> }
>
>
> /**
> * Parse "[a1][link2] ... [etc]"
> */
> static int parse_inouts(const char **buf, AVFilterInOut **inout, int pad,
> enum LinkType type, AVFilterContext *filter,
> AVClass *log_ctx)
> {
> while (**buf == '[') {
> char *name;
>
> parse_link_name(buf, &name, log_ctx);
>
> if(!name)
> return -1;
>
> if(handle_link(name, inout, pad++, type, filter, log_ctx) < 0)
> return -1;
handle_link() is such a nice meaningless name, why not inline the code
here, its not used anywhere else anyway?
[...]
> // We need to parse the inputs of the filter after we create it, so
> // skip it by now
> filters = skip_inouts(filters);
>
> if(!(filter = parse_filter(&filters, graph, index, log_ctx)))
> goto fail;
>
> pad = parse_inouts(&inouts, &inout, chr == ',', LinkTypeIn, filter,
> log_ctx);
I do not like this design.
What prevents you from parsing the inouts just once?
A filter has N inputs, these are known before the filter, either
through the explicit inouts or the previous filter(s).
Parsing the inputs would move such links around between the named and
numbered inputs.
I think the mess results from the lack of support of more than 1
input being passed through between filters.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20080418/085890a9/attachment.pgp>
More information about the ffmpeg-devel
mailing list