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

[Arpi]

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 :)))

> > 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?

> > TODO:
> > - -vop option parsing
done
> >   note: we should use the subconfig code from configparser, if possible
not done
> > - vf_free_filter (easy)
not yet done
> > - update libmpcodecs to call vf_* functions instead of libvo
done

> > 
> > I'm interested in opinions, ideas, etc.
> > 
you late a bit, i've commited stuff few minutes before this mail :(

> 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.

> situation, but when removing instead of inserting a filter,
> vf_forward_uninit will be the Wrong Thing to do.
hmm. right. i'll fix.

btw i didn't planned runtime removal of filters. adding is only
requires for colorspace conversion, if a config() fails because of
no common csp, it will insert a converter.

> 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.
yes

> I still think vf_forward_uninit is a bad idea though because when we
i still agree :)

> 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.

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.

anyway, back to the problem.
you're right, it would fail if stride != width*bpp. but it never happens!
codecs normally keep stride==width*bpp, unless get_image of next filter
asks to change stride (direct rendering). so, at least with the current
code, draw_frame() is never called with stride != width*bpp.

> 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 :)
argh


A'rpi / Astral & ESP-team

--
Developer of MPlayer, the Movie Player for Linux - http://www.MPlayerHQ.hu