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

[Vitor Sessak]

Michael Niedermayer wrote:
> On Wed, Mar 19, 2008 at 08:24:41PM +0100, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Wed, Mar 19, 2008 at 07:48:04PM +0100, Vitor Sessak wrote:
>>>> Hi Michael, Vmrsss
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Wed, Mar 19, 2008 at 03:01:41PM +0000, vmrsss wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>> On 19 Mar 2008, at 14:00, Michael Niedermayer wrote:
>>>>> [...]
>>>>>>> [...]
>>>>>>>> As for the extra operator, you are right. However, I claim that in
>>>>>>>> order to allow reusable libraries of graphs in your syntax you will
>>>>>>>> have to provide a renaming operator, eg:
>>>>>>>>
>>>>>>>>  	{tmp1/in} (in)overlay(out) {tmp2/out}  = (tmp1) overlay (tmp2)
>>>>>>> You are inventing some syntax and then complain about it, there is  
>>>>>>> nothing
>>>>>>> in the code nor in any mails from vitor suggesting anything like  
>>>>>>> above.
>>>>>> Hmm, I might not have made myself clear: sure you haven't suggested  
>>>>>> anything like the above, I am arguing (and made a specific example in  
>>>>>> support) that you **have** to if you want to reuse filters.
>>>>> Look, as there is nothing specified about "reuseable libraries" any
>>>>> system/syntax could be used, this clearly would allow to use "your syntax"
>>>>> as well thus this is a proper proof that it cannot be worse than "your
>>>>> syntax".
>>>> One problem about libraries of filter chains is that ideally they should 
>>>> be more flexible than just some graph hunk. Citing an example of what 
>>>> I'd like to be possible:
>>>>
>>>> AVFilter avfilter_vf_rotate =
>>>> {
>>>>      .name = "rotate";
>>>>      .init = init;
>>>>
>>>>      {some magic to make it a pseudofilter}
>>>> }
>>>>
>>>> static void init(char *args, AVFilter *ctx) {
>>>>      int ang= atoi(args);
>>>>      switch (ang) {
>>>>      case 90:
>>>>          ctx->actual_filter = "transpose,vflip";
>>>>          break;
>>>>      case 180:
>>>>          ctx->actual_filter = "hflip,vflip";
>>>>          break;
>>>>      case 270:
>>>>          ctx->actual_filter = "vflip,transpose";
>>>>          break;
>>>>      else
>>>>          snprintf(ctx->actual_filter, "rotate_slow=%d", ang);
>>>>          break;
>>>>      }
>>>> }
>>>>
>>>> But for doing this it would have to be a real filter, not something that 
>>>> pasted some code while parsing the chain.
>>> no, implement 1 filter doing generic rotations/flip/transpose with special
>>> cases for what can be done faster.
>>> And then just replace "transpose" / "vflip" / "hflip" by the appropriate
>>> generic filter string
>>>
>>> no code duplication, simple and fast
>> Well, it's only simpler if you consider the pseudo-filter framework as 
>> part of the complexity of the rotate filter. It's usually simpler to 
>> read/maintain pieces code that do just one operation.
>>
>> Also, I cited that more as an example. One could think in more cases 
>> like using movie_src, set_pts and overlay to make a add_logo filter, 
>> etc. Anyway, if this complexity is ever added, the pseudo-filter code 
>> now in avfiltergraph.c would be only used for those pseudo-filters, and 
>> not always as is in current code.
> 
> If you really want to insert filters, the logic way would be
> 
> av_insert_filter() which directly works on libavfilter/avfilter.h
> structs, i dont see the need for a second layer API

Well, thinking again, its better to discuss all this after a working 
version of lavfi hit SVN (so it won't take forever), but maybe a clean 
way to implement it would be add another callback to the AVFilter struct

/**
  * Callback called after all the input and output pads are
  * linked in the filter chain.
  */
void (*after_linking)(AVFilterLink *link);

The idea would be that the pseudo-filter would unlink itself from the 
filter chain in this function and would add another(s) filters in its place.

-Vitor