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

[Ivan Kalvachev]

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.


The bug appeared first on fedora because they have made some of the
standard headers include byteswap.h and mplayer didn't had detection
of HAVE_BYTESWAP.