[Ffmpeg-devel] [RFC] ffmpeg-windows mailinglist?
Wed Aug 2 07:25:41 CEST 2006
On Tue, Aug 01, 2006 at 09:09:21PM -0400, Augie Fackler wrote:
> Usually the comments boil down to "this is ugly" or something similar
> with no feedback on what could make the patch "pretty" enough to be
> accepted. As an example, taken from <http://svn.cod3r.com/perian/
> - ".balign 16 \n\t"
> + BALIGN_16
> +#if defined(__APPLE__)
> +# define BALIGN_8 ".align 3 \n\t"
> +# define BALIGN_16 ".align 4 \n\t"
> +# define BALIGN_8 ".balign 8 \n\t"
> +# define BALIGN_16 ".balign 16 \n\t"
> Is this really so ugly as to be a scar upon the codebase? If not,
> then what in the linked patch is truly so objectionable?
It's ugly because it makes an assumption about a platform which may or
may not be true in the future. Just look at the source to the "screen"
program which is full of crap like:
#if (defined(sgi) && defined(SVR4)) || defined(__osf__) || ...
(YES THIS IS A REAL LINE FROM THE CODE AND IT GOES ON FOR MUCH
When you start including crap like #ifdef __APPLE__ the above is what
The solution is to make a test in configure to test the behavior of
align. First try assembling a file with .balign or .p2align. If one
fails try the other. If both fail, first try .align 3. This will fail
if .align takes its argument in bytes rather than as the number of low
bits that must be zero; if it does try .align 8. If they all fail,
disable alignment altogether and hope it's not needed. :) Then include
a definition of BALIGN_8 and BALIGN_16 in config.h.
The benefits of this approach:
- all the nastiness is hidden in configure, not in the sources.
- if apple ever does change their assembler, ffmpeg automatically
supports the new syntax.
- you also get (for free!) support for other systems that lack .balign
More information about the ffmpeg-devel