flac dev - Jun 2014 - FIxed rest of cast-align warnings

Hi all,

In commit 3eb4094b859 I think I have fixed all the cast-align warnings.

I have tested this in amd64/Linux (little endian) and powerpc64/Linux
(big endian) and it passed all tests (including the new MD5 tests).

I also did a little performance testing on amd64/Linux with a one
hour long stereo WAV file and could not find any mesasurable difference
between the old and the new code.

I would appreciate it if people could kick this around, test on other
platforms/architectures and test for performance regressions.

Cheers,
Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> In commit 3eb4094b859 I think I have fixed all the cast-align warnings.
>
> I have tested this in amd64/Linux (little endian) and powerpc64/Linux
> (big endian) and it passed all tests (including the new MD5 tests).
About a commit
http://git.xiph.org/?p=flac.git;a=commitdiff;h=40e817dc526d1d41e900bb0f54d1a4451bda01ed

I think it's more consistent to write

FLAC__int16 s16buffer[FLAC__MAX_BLOCK_SIZE * FLAC__MAX_CHANNELS *
sizeof(FLAC__int32)/sizeof(FLAC__int16)];

instead of

FLAC__int16 s16buffer[FLAC__MAX_BLOCK_SIZE * FLAC__MAX_CHANNELS *
sizeof(FLAC__int16)];

op 29-06-14 15:31, Erik de Castro Lopo schreef:
> I would appreciate it if people could kick this around, test on other
> platforms/architectures and test for performance regressions.
These patches indeed fix the -Wcast-align warnings, but the new 
MD5 tests fail
> +++ libFLAC unit test: md5
>
> testing FLAC__MD5Init ... OK
> testing that FLAC__MD5Final clears the MD5Context ... OK
> testing FLAC__MD5Accumulate (channels=1, bytes_per_sample=1) ...
> FAILED, expected MD5 sum b2bb8775b7d5bf59c36c8637293a4602 but 
> got 419123315a22592e3170b67d0aa26916
However, using the binary on actual FLAC files (with flac -t) 
doesn't give any errors of mismatching md5 sums, so maybe the 
problem is in the test itself? ARM is big endian IIRC, maybe 
that could be the cause?

lvqcl wrote:
> About a commit
http://git.xiph.org/?p=flac.git;a=commitdiff;h=40e817dc526d1d41e900bb0f54d1a4451bda01ed
> 
> I think it's more consistent to write
> 
> FLAC__int16 s16buffer[FLAC__MAX_BLOCK_SIZE * FLAC__MAX_CHANNELS *
sizeof(FLAC__int32)/sizeof(FLAC__int16)];
> 
> instead of
> 
> FLAC__int16 s16buffer[FLAC__MAX_BLOCK_SIZE * FLAC__MAX_CHANNELS *
sizeof(FLAC__int16)];
Really? Would you also write this? :

FLAC__int32 s32buffer[FLAC__MAX_BLOCK_SIZE * FLAC__MAX_CHANNELS *
sizeof(FLAC__int32)/sizeof(FLAC__int32)];

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Martijn van Beurden wrote:
> 
> op 29-06-14 15:31, Erik de Castro Lopo schreef:
> > I would appreciate it if people could kick this around, test on other
> > platforms/architectures and test for performance regressions.
> 
> These patches indeed fix the -Wcast-align warnings, but the new 
> MD5 tests fail
> 
> > +++ libFLAC unit test: md5
> >
> > testing FLAC__MD5Init ... OK
> > testing that FLAC__MD5Final clears the MD5Context ... OK
> > testing FLAC__MD5Accumulate (channels=1, bytes_per_sample=1) ...
> > FAILED, expected MD5 sum b2bb8775b7d5bf59c36c8637293a4602 but 
> > got 419123315a22592e3170b67d0aa26916
> 
> However, using the binary on actual FLAC files (with flac -t) 
> doesn't give any errors of mismatching md5 sums, so maybe the 
> problem is in the test itself?
Possibly. I will investigate. Fortunately, I got the same
error in my QEMU armhf chroot, but I didn't trust QEMU.
> ARM is big endian IIRC, maybe that could be the cause?
ARM is actully bi-endian. The endian-ness is often configured
by an external pin on the chip but some systems can swap
endian-nesses at run time by updating a register but obviously
this is not something we need to support. We just need to support
whatever endian-ness is detected at configure time.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> Hi all,
>
> In commit 3eb4094b859 I think I have fixed all the cast-align warnings.
>
> I have tested this in amd64/Linux (little endian) and powerpc64/Linux
> (big endian) and it passed all tests (including the new MD5 tests).
>
> I also did a little performance testing on amd64/Linux with a one
> hour long stereo WAV file and could not find any mesasurable difference
> between the old and the new code.
>
> I would appreciate it if people could kick this around, test on other
> platforms/architectures and test for performance regressions.
Compiler: GCC 4.9.0,
CPU: Intel Core 2 (Yorkfield),
Platform: IA32 and x86-64,
Source: 44.1/16/stereo;
encoding: WAV to FLAC -8; decoding: FLAC -8 to WAV.

Result: speed differences are within measurement error
(both encoding and decoding).

Martijn van Beurden wrote:
> These patches indeed fix the -Wcast-align warnings, but the new 
> MD5 tests fail
> 
> > +++ libFLAC unit test: md5
> >
> > testing FLAC__MD5Init ... OK
> > testing that FLAC__MD5Final clears the MD5Context ... OK
> > testing FLAC__MD5Accumulate (channels=1, bytes_per_sample=1) ...
> > FAILED, expected MD5 sum b2bb8775b7d5bf59c36c8637293a4602 but 
> > got 419123315a22592e3170b67d0aa26916
> 
> However, using the binary on actual FLAC files (with flac -t) 
> doesn't give any errors of mismatching md5 sums, so maybe the 
> problem is in the test itself?
Yes, the tests were broken and were only passing by replying on
undefined behaviour.

Tests fixed, and pass on amd64/Linux, powerpc64/Linux and armh/Linux.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/