[MPlayer-cvslog] r26219 - trunk/mpbswap.h

[Aurelien Jacobs]

Ivan Kalvachev wrote:

> On Tue, Mar 11, 2008 at 8:30 PM, Aurelien Jacobs <aurel at gnuage.org>
> wrote:
> > Ivan Kalvachev wrote:
> >
> >  > On Tue, Mar 11, 2008 at 1:29 PM, Reimar Döffinger
> >  > <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> >  > >
> >  > > On Tue, Mar 11, 2008 at 01:20:31PM +0200, Ivan Kalvachev wrote:
> >  > >  > On Tue, Mar 11, 2008 at 12:42 PM, Reimar Döffinger
> >  > >  > <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> >  > >  > > On Mon, Mar 10, 2008 at 09:20:08PM +0100, diego wrote:
> >  > >  > >  > Author: diego
> >  > >  > >  > Date: Mon Mar 10 21:20:08 2008
> >  > >  > >  > New Revision: 26219
> >  > >  > >  >
> >  > >  > >  > Log:
> >  > >  > >  > Add missing header #include.
> >  > >  > >  >
> >  > >  > >  >
> >  > >  > >  > Modified:
> >  > >  > >  >    trunk/mpbswap.h
> >  > >  > >  >
> >  > >  > >  > Modified: trunk/mpbswap.h
> >  > >  > >  > ==============================================================================
> >  > >  > >  > --- trunk/mpbswap.h   (original)
> >  > >  > >  > +++ trunk/mpbswap.h   Mon Mar 10 21:20:08 2008
> >  > >  > >  > @@ -3,6 +3,8 @@
> >  > >  > >  >
> >  > >  > >  >  #include <sys/types.h>
> >  > >  > >  >  #include "libavutil/bswap.h"
> >  > >  > >  > +#include "config.h"
> >  > >  > >  > +
> >  > >  > >
> >  > >  > >  Uh, libavutil/bswap.h almost certainly requires config.h,
> >  > >  > > this seems like a stupid place to put it?
> >
> >  Uh, libavutil/bswap.h almost certainly doesn't require anything as
> >  it is supposed to be self-contained.
> 
> Your statement is right, but you are wrong.
> 
> >  > >  > I think I should save you some time and give you the
> >  > >  > previous answer we got from diego about same kind of bug:
> >  > >  >
> >  > >  > On Sat, Mar 8, 2008 at 4:36 PM, Diego Biurrun
> >  > >  > <diego at biurrun.de> wrote:
> >  > >  > >  Nonsense.
> >  > >  > >  Diego
> >  > >
> >  > >  I don't claim it to be a bug, I just say it does not make much
> >  > > sense to me. Also previously the policy I think was if
> >  > > config.h is included it should be included directly after the
> >  > > system headers.
> >  >
> >  > Yes, and there is very good reason for that rule.
> >  >
> >  > This is not bug only because remaining proper config.h
> >  > inclusions are still in the .c files, so this include have no
> >  > actual meaning.
> >  >
> >  > If the some .c file for some reason doesn't do that, this commit
> >  > would ensure very rare and obscure breaking (for example on
> >  > fedora ).
> >
> >  Whether .c files include config.h or not won't change anything.
> >  All ffmpeg headers are supposed to be self-contained !
> >  Feel free to show us how is this supposed to fail on fedora (or
> > whatever).
> 
> The bug is actually collision with system header byteswap.h that have
> absolutely the same defines as bswap.h function. Because of this the
> inclusion of bswap.h dies with error __extension__ something.. (bswap
> function names got replaces with byteswap defines).
> 
> The fix is trivial if you look in bswap.h. You have to define
> HAVE_BYTESWAP and then the system header would be used. The problem is
> that HAVE_BYTESWAP is defined in config.h and to be included by ffmpeg
> header it needs another define... and that define should be passed
> through the command line.

Good. You've just understood why libavutil/bswap.h is not part of
ffmpeg public API. And as such, it is not designed to be used
outside ffmpeg build system (configure/Makefile/...).
As MPlayer insists to fiddle with ffmpeg internal headers, it has to
reproduce ffmpeg build system. Especially defines such as HAVE_BYTESWAP
and HAVE_AV_CONFIG_H.

But all of this has nothing to do with header inclusion order.

Aurel