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

[Fredrik Kuivinen]

On Sat, Apr 06, 2002 at 01:21:04AM +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, then
> 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...

> My implementation is in the middle - it's a new library, instead placing
> stuff into libmpcodecs (as I wanted) or libvo (as Fredrik did).
> It is not finished yet (it compiles itself, and should work, but not yet
> linked to mplayer/mencoder code, i'll finish it today and/or tomorrow).
> I've uploaded the preview edition to mphq incoming, name: libvfilter-0.1.tar.gz
> 
> design ideas:
> - I've moved mpcodecs_get_image to vfilter.
> - vf_instance_t is a struct filled when creating (opening) a new filter. a
> filter can be opened more than once. this struct contains both func pointers
> (some of them can be NULL, and there are fallback functions for the others),
> plugin data (get_image context, priv*) and pointer to the underlaying filter.
> 
> there is a special filter: vf_vo.c
> it is a wrapper to libvo. there will be another special filter: vf_encode,
> for use with mencoder. but maybe each codec will have its own vf_* later...
> 
> 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 colorspace
>   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.
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.

> - There will be a vifeo filter _always_ enabled, the libvo wrapper. it 
>   simplifies code a lot - we don't need libvo stuff, nor 'global' get_image
>   in libmpcodecs.

Good idea.

> - simplified control(), removed the va_args stuff. it just makes us mad
>   and has no real sense, it will never work well with current libvo api :(

This is good. We don't need the va_args stuff anyway.

> TODO:
> - -vop option parsing
>   note: we should use the subconfig code from configparser, if possible
> - vf_free_filter (easy)
> - update libmpcodecs to call vf_* functions instead of libvo
> 
> I'm interested in opinions, ideas, etc.
> 

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
situation, but when removing instead of inserting a filter,
vf_forward_uninit will be the Wrong Thing to do.

Hm When I think a bit more about it... Of course it is possible to
check in vf_forward_config if we need to (re)config the next filter.
I still think vf_forward_uninit is a bad idea though because when we
remove (runtime remove not remove-at-end-of-stream) a filter from the
filter chain we don't, ever, want to uninit the other filters in the
chain. We just want to reconfig some of them.

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.

IMHO your code would be a lot easier to read if you ran it thru
indent(1) but that is just my opinion. Feel free to ignore that
comment :)

/ Fredrik