[MPlayer-dev-eng] libvfilter-0.1.tar.gz

[Fredrik Kuivinen]

On Sun, Apr 07, 2002 at 12:32:48AM +0200, Arpi wrote:
> Hi,
> 
> > > Fredrik implemented the vidoe filter layer (not completed yet) but _IMHO_ 
> > in
> > > a messy way, at least there are ugly things.
> > > But his patch was a very good
> > > startpoint, has some good ideas! First I started to cleanup his patch, the
> > n
> > > switched to something new, but still very similar to his idea.
> > 
> > I had prefered if you told me what you thought was ugly with my code
> > before you rewrote it...
> 
> - the way how you copy mpi
> - placing stuff into libvo
> - changing typedefs to struct xxx in headers
> - mess with globals in libmpcodecs
> - va_args
> - lots of TODO
> - your indenting :)))
>

You are a bit late because you have already rewritten it. But we
have a filter layer in cvs now with a (IMHO) quite good design and
that is a good thing. It just feels kind of stupid to throw away ~2k
lines of working code and replace it with another implementation
which basically has the same design.

> > > the main differences to Fredrik's patch:
> > > - I use linked list of filters instead of array - makes possible to easily
> > >   insert/remove filters runtime, even from a filter plugin (like colorspac
> > e
> > >   conversion)
> > 
> > Actually (I think) this will cause you trouble when you want to
> > insert/remove filters, at least if you don't make a doubly linked
> > list. I have already implemented remove and insert with my design.
> 
> when you lookup the filter to be removed, you also find its parent
>
> > The runtime support is untested but the code is there. Anyway the
> > data structure used to store the filters should be an implementation
> > detail of the filter layer so it should be possible to change it
> > without touching the filters.
> 
> i don't really understand this - why do you need to change filters now?
> 

I tried to say that what kind of data structure we use to store the
filter chain don't matter much as long as the filters aren't aware
of it.

Anyway a linked list looks like a good choice to me now.

> > I think vf_forward_config is unecessary. IMHO it is better to pass
> > pointers to width, height, format etc. Imagine a situation when we
> > want to insert a filter which don't change width, height, format,
> > d_width, d_height or any other of the config parameters. In this
> > situation we don't need to (re)config the filters that are after the
> > filter we are about to insert. If we have vf_forward_config we will
> > always (re)config everything after the filter we insert. In the same
> but what happens, if a given filter needs to call child's config() more
> than once? in your way, you can manipulate parameters but cannot avoid
> calling child's config or call it twice with different params.
>

Why would you need to call config more than once? 
And why would you not want to call it at all?

I am asking out of curiosity, vo_forward_config is not as bad as I
thought it was when I saw it the first time.

> > When reading your code I found something that I think is a bug:
> > In vf_vo.c:put_image you have code which looks like this:
> > if(mpi->flags & MP_IMGFLAG_PLANAR)
> >     draw_slice(mpi ... );
> > else
> >     draw_frame(mpi->planes);
> > 
> > This wont work if stride != width*bpp/8 and mpi->imgfmt is packed.
> 
> if it's a bug (i's a limitation of libvo - but not a bug) then it's not a
> new bug. libvo's draw_frame() doesn't accept stride != width*bpp.
> this is why i've added new libvo control put_frame.
> if a libvo driver implements put_frame, it don't have to implement
> draw_slice/draw_frame. i'll change major drivers soon.
> 

ok. put_frame seems to be a good thing.

/ Fredrik