[FFmpeg-devel] [PATCH] libmpcodecs support

[Aurelien Jacobs]

On Sat, Jan 15, 2011 at 12:44:26AM +0100, Michael Niedermayer wrote:
> On Fri, Jan 14, 2011 at 11:34:28PM +0100, Aurelien Jacobs wrote:
> > On Fri, Jan 14, 2011 at 05:37:45AM +0100, Michael Niedermayer wrote:
> > > Hi
> > > 
> > > Attached patchset makes libavfilter support most libmpcodecs filters
> > > The libmpcodecs filters are practically unmodified and for keeping it
> > > maintainable id like to keep them unmodified.
> > > 
> > > I will commit this soon but wont enable it yet. That way others can help
> > > cleanup the wraper (minus code that for maintainability should stay identical
> > > to the original), fix bugs, rename all global functions so they wont conflict
> > > with mplayers if they ever include this and generally help.
> > > 
> > > Please dont bikeshed this to death.
> > > 
> > > Ahh before i can commit this, libmpcodecs and subdirectories must not get
> > > blocked by the tab & trailing whitespace checkin scripts.
> > > 
> > 
> > 
> > > From 34072e13989ebed5e76a425b886b5b17756e6d5d Mon Sep 17 00:00:00 2001
> > > From: Michael Niedermayer <michaelni at gmx.at>
> > > Date: Fri, 14 Jan 2011 05:07:39 +0100
> > > Subject: [PATCH 7/7] Enable libmpcodecs support.
> > > 
> > > ---
> > >  libavfilter/Makefile     |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  libavfilter/allfilters.c |    1 +
> > >  2 files changed, 66 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> > > index fdb181e..0db4750 100644
> > > --- a/libavfilter/Makefile
> > > +++ b/libavfilter/Makefile
> > > @@ -30,6 +30,7 @@ OBJS-$(CONFIG_FREI0R_FILTER)                 += vf_frei0r.o
> > >  OBJS-$(CONFIG_GRADFUN_FILTER)                += vf_gradfun.o
> > >  OBJS-$(CONFIG_HFLIP_FILTER)                  += vf_hflip.o
> > >  OBJS-$(CONFIG_HQDN3D_FILTER)                 += vf_hqdn3d.o
> > > +OBJS-$(CONFIG_MP_FILTER)                     += vf_mp.o
> > >  OBJS-$(CONFIG_NOFORMAT_FILTER)               += vf_format.o
> > >  OBJS-$(CONFIG_NULL_FILTER)                   += vf_null.o
> > >  OBJS-$(CONFIG_OCV_FILTER)                    += vf_libopencv.o
> > > @@ -54,6 +55,70 @@ OBJS-$(CONFIG_NULLSRC_FILTER)                += vsrc_nullsrc.o
> > >  
> > >  OBJS-$(CONFIG_NULLSINK_FILTER)               += vsink_nullsink.o
> > >  
> > > +
> > > +OBJS += libmpcodecs/mp_image.o
> > > +OBJS += libmpcodecs/img_format.o
> > > +OBJS [...]
> > 
> > I guess all those libmpcodecs files should also be added to
> > OBJS-$(CONFIG_MP_FILTER). I don't think we want them compiled
> > unconditionally.
> 
> i had hoped other people would help but ok fixed locally
> 
> 
> [...]
> > IIUC you said that the ultimate goal is to have all those filters
> > converted to native libavfilter so that we can drop all the libmpcodecs
> > filters. If so, why does this libmpcodecs filter list includes some
> > filters which have already a native libavfilter version ?? Namely:
> >   vf_blackframe.o
> >   vf_cropdetect.o
> >   vf_dsize.o
> >   vf_mirror.o
> >   vf_rectangle.o
> >   vf_rotate.o
> 
> they can be droped after a quick speed & feature comparission

It is usually the opposite. Code can *get in* after a speed & feature
comparison.
But as you want, as long as those duplicate filters don't stay in the
repository for months.

> > Also vf_yvu9 is documented as "Deprecated in favor of the software
> > scaler". So I doubt it is a good idea to keep it.
> 
> why is it not droped from mplayer?

Probably because libmpcodec is mostly un-maintained since years, and
that the few person touching it fear removing anything, just in case
somebody might still be using it.
And maybe also to avoid breaking old scripts using "mplayer -vf yvu9".

While we are on the subject, this libmpcodecs warper will probably
also get used in all kinds of scripts. Maybe we should make it clear
from the begining in the user documentation that those filters are
bound to disapear, so people should be prepared to adapt their scripts
later on.

> > I also suspect vf_palette might already be handled by libavfilter.
> > It might also be worth double checking vf_yuvcsp but I guess this one
> > is not yet supported by libavfilter.
> > 
> > I didn't check the code carefully yet, but I wonder how did you handle
> > filters like vf_screenshot or vf_hue which require some external
> > control/command to trigger some actions ?
> 
> vf_hue worked when i did a quick test

Yes, I guess you can set hue at the begining with a parameter and then
you can't change it dynamically. This is certainly OK for now.

> vf_screensot needs some API amendments on our side, so its WIP

So it's not ready to go in ffmpeg's repository...

> > I don't think there is any clean API to handle this in libavfilter.
> > And I don't think vf_screenshot can have any kind of usefullness inside
> > libavfilter.
> 
> taking screenshoots is useful

Yes, it is definitely useful !
But it has nothing to do inside libavfilter !
An application which want to take a screenshot will just grab the
current picture out of the desired sink of the filter graph and encode
it to png (or any codec) using lavc/lavf.
This is much more flexible.

> > And how did you handle filters like vf_remove_logo which fopen()
> > an image file and decode it internally, totally ingnoring several layers
> > of abstraction ?
> 
> i dont know what you mean by handle, the filter does what it does. ive not
> changed that. Changeing this is completely outside the scope of this patchset

Hum... OK. Just wanted to note that such a filter (or
codec/demuxer/whatever) trying to access directly to a file, bypassing
the avio/protocole layer would never be accepted in any other part of
FFmpeg. But I guess this patchset is a bit special.

> > I guess it should be handled by simply having a second input pad
> > allowing the application to feed it with any kind of decoded picture.
> 
> Not only would it be alot of work to implement this, it would also make
> the use of the filter much more complex.

Probably a bit more complex (but not that much IMHO). But it would also
be much more flexible, allowing any image format (instead of just the
uncommon PGM/PPM), and allowing not only static image but also video
to suppress animated logo.
But I guess it is out of the scope of this patchset and shoud instead be
a goal when porting this filter to native libavfilter.

> > Overall I don't think we should import a whole bunch of filters like
> > this, without even trying all of them to check if they actually work
> > and they actally make any sens at all in the libavfilter context.
> 
> I tried all before submitting this patchset, i did not spend an hour per filter
> for testing.
> What i can say is none crashed, none failed assert

Great ! That's already quite impressive.

> and filters produced the
> expected effect where one was expected which was for many filters without
> finding specific input material and and and, no effect so its possible some
> do not work completely

It would be better to really test if all those filters are working, but
I know it's a huge task. So I guess it is OK to leave to work to
FFmpeg's users...

> What i had hoped was that other people would help me with this and be happy that
> i implemented one of the most asked for and important things for libavfilter.

I'm not sure who asked so much for this ?
I guess there is not many ffmpeg developpers excited about the inclusion
of thousands ugly lines of code from MPlayer, with the long term goal
(and huge task) of removing them.
So you may find more happy people about this on FFmpeg-user than on
FFmpeg-devel.

I honestly don't like this patchset much, but I understand the reason
for it, and I have to admit that overall it is quite reasonable (as long
as we make sure that nobody looses his time working on cleaning/fixing
libmpcodecs instead of implementing native libavfilters).

Aurel