[FFmpeg-devel] XVMC Deathmatch

Sun Feb 15 13:19:34 CET 2009

On Sunday, 15 February 2009 at 02:22, Michael Niedermayer wrote:
> On Sun, Feb 15, 2009 at 02:34:59AM +0200, Ivan Kalvachev wrote:
> > On 2/15/09, Diego Biurrun <diego at biurrun.de> wrote:
> > > On Sun, Feb 15, 2009 at 01:01:36AM +0100, Diego Biurrun wrote:
> > >> On Sun, Feb 15, 2009 at 01:56:49AM +0200, Ivan Kalvachev wrote:
> > >> > On 2/15/09, Diego Biurrun <diego at biurrun.de> wrote:
> > >> > > On Sun, Feb 15, 2009 at 12:29:18AM +0100, Michael Niedermayer wrote:
> > >> > >> On Sat, Feb 14, 2009 at 11:40:35PM +0100, Diego Biurrun wrote:
> > >> > >> > On Sat, Feb 14, 2009 at 11:30:02PM +0100, Michael Niedermayer
> > >> > >> > wrote:
> > >> > >> > > On Sat, Feb 14, 2009 at 10:38:39PM +0100, Diego Biurrun wrote:
> > >> > >> > > > On Sat, Feb 14, 2009 at 04:53:01PM +0100, Michael Niedermayer
> > >> > >> > > > wrote:
> > >> > >> > > > >
> > >> > >> > > > > >    //these are not changed by the decoder!
> > >> > >> > > > > >    int  magic;
> > >> > >> > > > >
> > >> > >> > > > > 3 points
> > >> > >> > > > > find and get rid of or rename fields to "ununsed123" all
> > >> > >> > > > > unused
> > >> > >> > > > > fields in this
> > >> > >> > > > > struct, dont break ABI!
> > >> > >> > > >
> > >> > >> > > > All fields are used either in FFmpeg or MPlayer.
> > >> > >> > > >
> > >> > >> > > > The following are unused in FFmpeg:
> > >> > >> > > >
> > >> > >> > > >   int             mc_type;
> > >> > >> > > >   int             chroma_format;
> > >> > >> > > >   unsigned int    display_flags;
> > >> > >> > > >   int             state;
> > >> > >> > > >   void*           p_osd_target_surface_render;
> > >> > >> > > >
> > >> > >> > > > But they are all used in libvo/vo_xvmc.c in MPlayer as can be
> > >> > >> > > > seen
> > >> > >> > > > by
> > >> > >> > > > trying to compile against an xvmc.h without those fields:
> > >> > >> > > >
> > >> > >> > > > libvo/vo_xvmc.c: In function 'xvmc_draw_image':
> > >> > >> > > > libvo/vo_xvmc.c:378: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'state'
> > >> > >> > > > libvo/vo_xvmc.c: In function 'config':
> > >> > >> > > > libvo/vo_xvmc.c:536: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'mc_type'
> > >> > >> > > > libvo/vo_xvmc.c:538: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'chroma_format'
> > >> > >> > > > libvo/vo_xvmc.c: In function 'draw_osd':
> > >> > >> > > > libvo/vo_xvmc.c:899: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'display_flags'
> > >> > >> > > > libvo/vo_xvmc.c:899: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'display_flags'
> > >> > >> > > > libvo/vo_xvmc.c:902: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'state'
> > >> > >> > > > libvo/vo_xvmc.c:903: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'state'
> > >> > >> > > > libvo/vo_xvmc.c:904: error: 'struct xvmc_render_state' has no
> > >> > >> > > > member
> > >> > >> > > > named 'p_osd_target_surface_render'
> > >> > >> > > > [...]
> > >> > >> > > >
> > >> > >> > > > Does research give points? :)
> > >> > >> > >
> > >> > >> > > no, you have to remove or rename these fields to unused* to get
> > >> > >> > > the
> > >> > >> > > points.
> > >> > >> >
> > >> > >> > Well, I did do
> > >> > >> >
> > >> > >> >    int             mc_type;
> > >> > >> >
> > >> > >> > -->
> > >> > >> >
> > >> > >> >    int             unused1;
> > >> > >> >
> > >> > >> > and FFmpeg compiles fine afterwards, but when I put that xvmc.h in
> > >> > >> > my
> > >> > >> > MPlayer tree, I get the above compilation failure.
> > >> > >> >
> > >> > >> > Thus my conclusion is that the field is not, as you claim, unused.
> > >> > >> > Am I missing something?
> > >> > >> >
> > >> > >> > > am i allowed to give tips on how to solve such things?
> > >> > >> >
> > >> > >> > This will likely result in more fixes :)
> > >> > >>
> > >> > >> well
> > >> > >> first as was said the header was not public thus no rename in the
> > >> > >> header
> > >> > >> really could break an external APP, removial of a field of course
> > >> > >> could
> > >> > >> so renaming these to unused123 and placing them under #if
> > >> > >> no_major_bump_yet
> > >> > >> should not break anything (in theory)
> > >> > >
> > >> > > I had all the fields I mentioned above under
> > >> > >
> > >> > > #if LIBAVCODEC_VERSION_MAJOR < 53
> > >> > >
> > >> > >> yes it means mplayer has to be updated before the next major bump and
> > >> > >> before it could use our installed header instead of its private copy
> > >> > >>
> > >> > >> but thats mplayers problem to say it harshly ...
> > >> > >
> > >> > > Well...
> > >> > > Ivan designed this incestuously.  It's kind of hard to tell if this is
> > >> > > a
> > >> > > public or a private header...
> > >> >
> > >> > Well, don't tell me I didn't had reasons to hold your
> > >> > plans on making it public header.
> > >>
> > >> You held that plan back for many years.  Without this deathmatch, you
> > >> would not have made those changes to MPlayer.
> > >>
> > >> > > I suggest that we assume it is becoming a public header just now and
> > >> > > thus
> > >> > > now that Ivan updated MPlayer to not use them anymore, the following
> > >> > > two
> > >> > > fields could be safely be removed:
> > >> > >
> > >> > >    int mc_type;
> > >> > >    int chroma_format;
> > >> > >
> > >> > > OK to remove them?
> > >> >
> > >> > You were the one who insisted on installing it.
> > >> > Deal with the consequences.
> > >>
> > >> I take that as a "I don't care", which I will now interpret to mean I
> > >> have a free ticket to go ahead.
> > >
> > > Note to Michael: I know this breaks ABI and API.  I'm operating under the
> > > assumption that the API is still in flux and the only user is MPlayer,
> > > which has already been updated.
> > >
> > > So don't go subtracting points from me :)
> > 
> > Michael,
> > Is breaking ABI OK for you.
> > I can make some general changes if this is so!
> 
> i dont give a damn about the lavc XVMC ABI, if it can be improved by breaking
> ABI ...
> just think about what you will tell the distro maintainers when their
> lavc suddenly starts acting up with mplayer and xvmc ... with some versions
> being combined ... maybe one should tell them so that they can take some
> precautions

It is currently not possible to compile MPlayer without using private FFmpeg
headers, so I wouldn't worry about modifying those parts of FFmpeg<->MPlayer
interfaces that are used ONLY by MPlayer. For the record, my packages of MPlayer
use a static copy of FFmpeg's libraries for that very reason. I did post
a proof-of-concept patch that allowed MPlayer to be built without libav*
and libswscale present in its tree, but it wasn't nowhere near being OK to
committing. I never had any time to work on this since then.

Regards,
R.

-- 
MPlayer http://mplayerhq.hu | RPMFusion http://rpmfusion.org
There should be a science of discontent. People need hard times and
oppression to develop psychic muscles.
	-- from "Collected Sayings of Muad'Dib" by the Princess Irulan