[FFmpeg-devel] [PATCH] [2/??] [3/3] Filter graphs - Parser for a graph description
Michael Niedermayer
michaelni
Fri May 9 23:58:21 CEST 2008
On Fri, May 09, 2008 at 08:44:23PM +0200, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Mon, May 05, 2008 at 10:01:36PM +0200, Vitor Sessak wrote:
>>> 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.
>> What i really dont like on the code is that it is complex and at the
>> same time only supports a small subset of what we wanted to support.
>> 300 lines would be fine for a complete implemantation with ; * () user
>> defined filters, and all the other things discussed
>> Saying ok to the code to then see it grow to 3 times that to support
>> everything is something i really dont want to see ...
>> Also when i just look at the API i cant help but it feels quite hostile
>> to an implemantation of "user defined filters", and not only that.
>> [...]
>>>>>>> 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.
>> I dont, i prefered the original
>> also
>> currlinkn->next = *currInputs;
>> *currInputs = currlinkn;
>> is duplicated, its a insert_blah()
>
> done
>
>>>> 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).
>> What i was aiming at was to remove the AVFilterInOut struct completely.
>> I just am not sure yet if its a good idea or not, but moving name out is a
>> good idea even on its own.
>> Leaving it in AVFilterInOut while adding it to AVFilterLink would be just
>> a duplicate field.
>
> Yes, but I find ugly to make a linked list of AVFilterLink to use either
> only src and srcpad or only dst and dstpad. And I don't see a non-ugly
> way to use both...
Well it cannot work without using both, as a complete AVFilterLink needs both
set to be a complete link.
Also if it allows some simplificatins then
src/dst* could be changed to filter[2] and filter_pad[2] in AVFilterLink
[...]
>> [...]
>>> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
>>> AVFilterInOut **openLinks, AVClass *log_ctx)
>>> {
>>> int pad = 0;
>>>
>>> while(**buf == '[') {
>>> char *name;
>>> AVFilterInOut *link_to_add;
>>> AVFilterInOut *match;
>>>
>>> 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. Make it one of the inputs of the
>>> next
>>> filter */
>>> if(match->type != LinkTypeOut) {
>>> av_log(log_ctx, AV_LOG_ERROR,
>>> "Label \"%s\" appears twice as input!\n",
>>> match->name);
>>> return -1;
>>> }
>> Just a totally wild idea, what about spliting openLinks into a list of
>> open inputs and a list of open outputs? that would avoid these type checks
>> and possibly the type field as well?
>
> That was a good idea. Done, and it simplified slightly the code.
great, i already started to be depressed by the lack of sucessfull
simplifications.
[...]
> static int link_filter_inouts(AVFilterContext *filter,
> AVFilterInOut **currInputs,
> AVFilterInOut **openInputs, AVClass *log_ctx)
> {
> int pad = filter->input_count;
>
> while(pad--) {
> AVFilterInOut *p = *currInputs;
> if(!p) {
> av_log(log_ctx, AV_LOG_ERROR,
> "Not enough inputs specified for the \"%s\" filter.\n",
> filter->filter->name);
> return -1;
> }
> *currInputs = (*currInputs)->next;
>
> if(p->filter) {
> if(link_filter(p->filter, p->pad_idx, filter, pad, log_ctx))
> return -1;
> av_free(p);
> } else {
> p->filter = filter;
> p->pad_idx = pad;
> p->next = *openInputs;
> *openInputs = p;
duplicate
> }
> }
*currInputs = (*currInputs)->next;
can be moved to the end of the loop
>
>
> if(*currInputs) {
> av_log(log_ctx, AV_LOG_ERROR,
> "Too many inputs specified for the \"%s\" filter.\n",
> filter->filter->name);
> return -1;
> }
>
> pad = filter->output_count;
> while(pad--) {
> AVFilterInOut *currlinkn = av_malloc(sizeof(AVFilterInOut));
> currlinkn->name = NULL;
use mallocz and this becomes unneeded
> currlinkn->filter = filter;
> currlinkn->pad_idx = pad;
> insert_inout(currInputs, currlinkn);
> }
>
> return 0;
> }
>
> static int parse_inputs(const char **buf, AVFilterInOut **currInputs,
> AVFilterInOut **openOutputs, AVClass *log_ctx)
> {
> int pad = 0;
>
> while(**buf == '[') {
> char *name = parse_link_name(buf, log_ctx);
> AVFilterInOut *match;
>
> if(!name)
> return -1;
>
> /* First check if the label is not in the openOutputs list */
> match = extract_inout(name, openOutputs);
>
> if(match) {
> /* A label of a open link. Make it one of the inputs of the next
> filter */
> insert_inout(currInputs, match);
> } else {
> /* Not in the list, so add it as an input */
> AVFilterInOut *link_to_add = av_malloc(sizeof(AVFilterInOut));
>
> link_to_add->name = name;
> link_to_add->filter = NULL;
> link_to_add->pad_idx = pad;
> insert_inout(currInputs, link_to_add);
> }
Try to simplify your code _please_!
if(!match){
match= av_mallocz(sizeof(AVFilterInOut));
match->name = name;
match->pad_idx = pad;
}
insert_inout(currInputs, match);
> *buf += consume_whitespace(*buf);
> pad++;
> }
>
> return pad;
> }
>
> static int parse_outputs(const char **buf, AVFilterInOut **currInputs,
> AVFilterInOut **openInputs,
> AVFilterInOut **openOutputs, AVClass *log_ctx)
> {
> int pad = 0;
>
> while(**buf == '[') {
> char *name = parse_link_name(buf, log_ctx);
> AVFilterInOut *match;
>
> AVFilterInOut *input = *currInputs;
> *currInputs = (*currInputs)->next;
belongs to the end of the loop
[...]
> /**
> * Parse a string describing a filter graph.
> */
> int avfilter_parse_graph(AVFilterGraph *graph, const char *filters,
> AVFilterInOut *openInputs,
> AVFilterInOut *openOutputs, AVClass *log_ctx)
> {
> int index = 0;
> char chr = 0;
> int pad = 0;
>
> AVFilterInOut *currInputs = NULL;
>
> do {
> AVFilterContext *filter;
> filters += consume_whitespace(filters);
>
> pad = parse_inputs(&filters, &currInputs, &openOutputs, log_ctx);
>
> if(pad < 0)
> goto fail;
>
> if(!(filter = parse_filter(&filters, graph, index, log_ctx)))
> goto fail;
inconsistant style
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20080509/09c7b155/attachment.pgp>
More information about the ffmpeg-devel
mailing list