[Ffmpeg-devel] [RFC] ffmpeg-windows mailinglist?

[Rich Felker]

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/ 
> trunk/ffmpeg-svn-mactel.patch>:
> -        ".balign 16                     \n\t"
> +        BALIGN_16
> and
> +#if defined(__APPLE__)
> +#  define BALIGN_8  ".align 3 \n\t"
> +#  define BALIGN_16 ".align 4 \n\t"
> +#else
> +#  define BALIGN_8  ".balign 8 \n\t"
> +#  define BALIGN_16 ".balign 16 \n\t"
> +#endif
> 
> 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
LONGER!)
When you start including crap like #ifdef __APPLE__ the above is what
eventually happens.

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

Rich