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 419123315a22592e3170b67d0aa26916However, 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/