[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Vitor Sessak
vitor1001
Mon May 5 22:01:36 CEST 2008
Hi
Michael Niedermayer wrote:
> On Wed, Apr 23, 2008 at 11:05:37PM +0200, Vitor Sessak wrote:
>> Hi and thanks for the review
> [...]
>>>> p->filter = filter;
>>>> p->pad_idx = pad;
>>>> p->next = *openLinks;
>>>> *openLinks = p;
>>>> }
>>>> }
>>>>
>>>>
>>>> if(*currInputs) {
>>>> av_log(log_ctx, AV_LOG_ERROR,
>>>> "Too many inputs specified for the \"%s\" filter.\n",
>>>> filter->name);
>>>> return -1;
>>>> }
>>>>
>>>> pad = filter->output_count;
>>>> while(pad) {
>>>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>>> pad--;
>>>> currlinkn->name = NULL;
>>>> currlinkn->type = LinkTypeOut;
>>>> currlinkn->filter = filter;
>>>> currlinkn->pad_idx = pad;
>>>> currlinkn->next = *currInputs;
>>>> *currInputs = currlinkn;
>>> somehow i thik this can be factored with the above as well
>> Maybe, but without worsening a lot readability?
>
> We need to find a way to simplify the whole parser without reducing
> readability.
> Its much more complex than i would like ...
Do you really think it can be so much more simpler? The "complex" part
of it (free_inout(), extract_inout(), link_filter_inouts(),
parse_inputs(), parse_outputs(), avfilter_parse_graph()) make 240 lines
of code.
>>> [...]
>>>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>>>> AVFilterInOut **openLinks, AVClass *log_ctx)
>>>> {
>>>> int pad = 0;
>>>> AVFilterInOut *p;
>>>>
>>>> while (**buf == '[') {
>>>> char *name;
>>>>
>>>> parse_link_name(buf, &name, log_ctx);
>>>>
>>>> if(!name)
>>>> return -1;
>>>>
>>>> /* First check if the label is not in the openLinks list */
>>>> p = extract_inout(name, openLinks);
>>>>
>>>> /* Not in the list, so add it as an input */
>>>> if(!p) {
>>>> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
>>>>
>>>> currlinkn->name = name;
>>>> currlinkn->type = LinkTypeIn;
>>>> currlinkn->filter = NULL;
>>>> currlinkn->pad_idx = pad;
>>>> currlinkn->next = *currInputs;
>>>> *currInputs = currlinkn;
>>> Ive seen similar code a few times already here somewhere ...
>> This code appears three times. But doing something like
>>
>> currlinkn = new_link(NULL, LinkTypeOut, filter, pad, *currInputs);
>>
>> wouldn't make this new_link() function a senseless wrapper?
>
> maybe iam not sure, do what you prefer
I did instead
AVFilterInOut tmp_link = { LinkTypeIn,
name,
NULL,
pad,
*currInputs,
};
AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
*currlinkn = tmp_link;
I think it is more readable.
>
>
> [...]
>> static void consume_whitespace(const char **buf)
>> {
>> *buf += strspn(*buf, " \n\t");
>> }
>
> i think a
>
> static int count_whitespace(const char *buf)
> {
> return strspn(buf, " \n\t");
> }
>
> would be clearer
>
> a(&x)
> vs.
> x += a(x)
>
> second makes it much clearer that the pointer is increased.
I agree.
> [...]
>> static int link_filter_inouts(AVFilterContext *filter,
>> AVFilterInOut **currInputs,
>> AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>
>> int pad = 0;
>>
>> pad = filter->input_count;
>
> hmm
oops
> [...]
>> static int parse_outputs(const char **buf, AVFilterInOut **currInputs,
>> AVFilterInOut **openLinks, AVClass *log_ctx)
>> {
>> int pad = 0;
>>
>> while(**buf == '[') {
>> char *name;
>> AVFilterInOut *match;
>>
>> AVFilterInOut *input = *currInputs;
>> *currInputs = (*currInputs)->next;
>>
>> parse_link_name(buf, &name, log_ctx);
>>
>> if(!name)
>> return -1;
>>
>> /* First check if the label is not in the openLinks list */
>> match = extract_inout(name, openLinks);
>>
>
>> if(match) {
>> /* A label of a open link. Link it. */
>> if(match->type != LinkTypeIn) {
>> av_log(log_ctx, AV_LOG_ERROR,
>> "Label \"%s\" appears twice as output!\n", match->name);
>> return -1;
>> }
>>
>> if(link_filter(input->filter, input->pad_idx,
>> match->filter, match->pad_idx, log_ctx) < 0)
>> return -1;
>> av_free(match);
>> av_free(input);
>> } else {
>> /* Not in the list, so add the first input as a openLink */
>> input->next = *openLinks;
>> input->type = LinkTypeOut;
>> input->name = name;
>> *openLinks = input;
>> }
>
> Maybe some of the code surrounding link_filter() could be simplified if
> avfilter_link wasnt used? I mean now the code needs to handle a few cases
> 1. Input filter available, output not
> 2. Input filter available, output available
> 3. Input filter not available, output available
>
> with:
>
> typedef struct AVFilterInOut {
> const char *name;
> AVFilterLink *link;
>
> struct AVFilterInOut *next;
> } AVFilterInOut;
>
> Either side could be linked no matter if the other is. What remains is
> just a common check "do we have both then remove the AVFilterInOut"
For a moment I thought that could allow merging parse_inputs() and
parse_outputs(). But now I don't really think so, since they do really
different stuff to currLinks.
> Also name could be moved to AVFilterLink, that way we would still have
> the names after the filter graph is build as a sideeffect.
I agree that would be a nice side-effect (that could be done even
without changing AVFilterInOut).
> [...]
>> /**
>> * Add to a graph a graph described by a string.
>> * @param graph the filter graph where to link the parsed graph context
>> * @param filters string to be parsed
>> * @param inouts linked list to the inputs and outputs of the graph
>
> I wonder if it would be cleaner or not to just pass a list of inputs and
> get a list of outputs after the function returns.
As the code is now, I don't think it does. Also it is nice to let
avfilter_parse_graph() do all the linking needed.
New version attached.
-Vitor, who just came back from vacations
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphparser.c
Type: text/x-csrc
Size: 11185 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080505/a811413a/attachment.c>
More information about the ffmpeg-devel
mailing list