Last Comment Bug 779187 - libcubeb AudioUnit backend isn't big endian compatible
: libcubeb AudioUnit backend isn't big endian compatible
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: 16 Branch
: PowerPC Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Matthew Gregan [:kinetik]
: Maire Reavy [:mreavy]
: 787789 (view as bug list)
Depends on:
Blocks: 780708
  Show dependency treegraph
Reported: 2012-07-31 09:55 PDT by Tobias Netzel
Modified: 2012-09-04 17:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

libcubeb Audio Unit backend big endian patch (1.97 KB, patch)
2012-07-31 09:55 PDT, Tobias Netzel
no flags Details | Diff | Splinter Review
libcubeb Audio Unit backend big endian patch (1.96 KB, patch)
2012-08-01 00:07 PDT, Tobias Netzel
no flags Details | Diff | Splinter Review
patch v0 (1.65 KB, patch)
2012-08-05 20:28 PDT, Matthew Gregan [:kinetik]
cajbir.bugzilla: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Tobias Netzel 2012-07-31 09:55:23 PDT
Created attachment 647577 [details] [diff] [review]
libcubeb Audio Unit backend big endian patch

The attached patch solves this issue but has not been tested on little endian systems.
Comment 1 Matthew Gregan [:kinetik] 2012-07-31 19:13:21 PDT
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)
> +#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
> +                       | kAudioFormatFlagIsBigEndian
> +#endif

These all look fine, except that it needs to use WORDS_BIGENDIAN or whatever rather than relying on NSPR stuff.
Comment 2 Tobias Netzel 2012-08-01 00:07:19 PDT
Created attachment 647860 [details] [diff] [review]
libcubeb Audio Unit backend big endian patch

__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.
Comment 3 Matthew Gregan [:kinetik] 2012-08-05 20:27:10 PDT
I pushed a variation of the header fix upstream:

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.
Comment 4 Matthew Gregan [:kinetik] 2012-08-05 20:28:04 PDT
Created attachment 649167 [details] [diff] [review]
patch v0

Tobias, can you please test this patch and let me know if it works?
Comment 5 Matthew Gregan [:kinetik] 2012-08-05 21:36:55 PDT
Comment 6 Tobias Netzel 2012-08-06 00:20:02 PDT
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?
Comment 7 Matthew Gregan [:kinetik] 2012-08-06 00:30:36 PDT
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.
Comment 8 Tobias Netzel 2012-08-06 01:53:14 PDT
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.
Comment 9 Matthew Gregan [:kinetik] 2012-08-06 04:27:32 PDT
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?
Comment 10 Tobias Netzel 2012-08-06 06:23:10 PDT
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 11 Ed Morley [:emorley] 2012-08-06 07:42:40 PDT
Comment 12 Matthew Gregan [:kinetik] 2012-09-03 18:24:19 PDT
*** Bug 787789 has been marked as a duplicate of this bug. ***
Comment 13 Matthew Gregan [:kinetik] 2012-09-03 18:30:20 PDT
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 14 Alex Keybl [:akeybl] 2012-09-04 06:41:45 PDT
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.
Comment 15 Marcus Comstedt 2012-09-04 11:56:00 PDT
*** Bug 787789 has been marked as a duplicate of this bug. ***
Comment 16 Matthew Gregan [:kinetik] 2012-09-04 17:36:49 PDT

Note You need to log in before you can comment on or make changes to this bug.