The attached patch solves this issue but has not been tested on little endian systems.

Comment on attachment 647577 [details] [diff] [review] libcubeb Audio Unit backend big endian patch Review of attachment 647577 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Comments below. ::: media/libcubeb/include/cubeb.h @@ +11,5 @@ > +#include "prcpucfg.h" > + > +#if defined(IS_BIG_ENDIAN) && !defined(WORDS_BIGENDIAN) > +#define WORDS_BIGENDIAN > +#endif cubeb.h is an external library header, we can't include NSPR includes in it in the upstream version. Does the PPC compiler define something we can use directly, like WORDS_BIGENDIAN? If not, we can add -DWORDS_BIGENDIAN to the cubeb cflags in the Mozilla build system. ::: media/libcubeb/src/cubeb_audiounit.c @@ +148,5 @@ > ss.mBitsPerChannel = 32; > + ss.mFormatFlags |= kAudioFormatFlagIsFloat > +#ifdef IS_LITTLE_ENDIAN > + | kAudioFormatFlagIsBigEndian > +#endif These all look fine, except that it needs to use WORDS_BIGENDIAN or whatever rather than relying on NSPR stuff.

__BIG_ENDIAN__ and __LITTLE_ENDIAN__ are guaranteed to be defined correctly on compilers (gcc/clang) for the Mac OS X/Darwin platform. Other compilers might define WORDS_BIGENDIAN directly but I don't know.

I pushed a variation of the header fix upstream: https://github.com/kinetiknz/cubeb/commit/697b5c1279bf723d93b9d5f08e1fec6a6382bb77 The other changes are actually not what we want now that I've looked at them again. They're inverting the sample format to get byte swapping, but they should just provide what they claim to be, and we need to fix the callers directly. We're opening the cubeb stream as FLOAT32LE when the nsAudioStream format is FLOAT32 (i.e. native endian), which is wrong. Fixing that, plus the header fix, should be sufficient.

Tobias, can you please test this patch and let me know if it works?

It does indeed work with your patch. Now wouldn't it be nice to avoid the byte swapping for signed 16 bit integer PCM as well?

Thanks for confirming. We're not byte-swapping anywhere in the nsBufferedAudioStream code now, just passing the correct formats through with the correct parameters. All of the new code is using native endian float32 (or sint16) only. The (existing) byte-swapping in nsNativeAudioStream doesn't cost much since we have to touch every sample to adjust the volume; that code is going away in bug 775319 anyway.

Sorry, I didn't do the test correctly; I applied your changes nsAudioStream.cpp and cubeb.h taking back my changes to those files, but didn't revert my changes to cubeb_audiounit.c . But that already means that your patch didn't change anything - at least for the cases I tested. Probably those cases all use sint16 which isn't affected by your patch at all.

What are you testing? Ignoring nsNativeAudioStream (which is only used if media.use_cubeb is false or the platform doesn't have a cubeb backend), I think sint16 is only used on ARM now, so that code should really be using the NE rather than LE versions... it's just that those code paths have only been tested on LE ARM machines. With a clean tree, and only my patch applied, is audio playback still broken?

Your patch works - and the changes I had applied to cubeb_audiounit.c were ineffective because they depended on WORDS_BIGENDIAN to be defined in cubeb.h which wasn't the case anymore. So everything is working correctly with just your patch applied; using gdb I verified that the audio stream is float 32 native (->big) endian here.

Comment on attachment 649167 [details] [diff] [review] patch v0 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 623444 User impact if declined: some audio formats play back incorrectly (as garbage) on big endian platforms Testing completed (on m-c, etc.): patch baked on m-c and m-a for a month Risk to taking this patch (and alternatives if risky): extremely low, patch changes a single format flag and adds one compile time test to a header String or UUID changes made by this patch: none

Comment on attachment 649167 [details] [diff] [review] patch v0 [Triage Comment] Low risk fix for a FF15 regression, albeit for a small affected population. Approving for Beta 16.