Closed Bug 779187 Opened 8 years ago Closed 8 years ago

libcubeb AudioUnit backend isn't big endian compatible

Categories

(Core :: Audio/Video, defect)

16 Branch
PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox16 --- fixed

People

(Reporter: tobias.netzel, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

The attached patch solves this issue but has not been tested on little endian systems.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #647577 - Attachment is patch: true
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.
Attachment #647577 - Attachment is obsolete: true
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.
Attached patch patch v0Splinter Review
Tobias, can you please test this patch and let me know if it works?
Assignee: nobody → kinetik
Attachment #647860 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #649167 - Flags: review?(chris.double)
Attachment #649167 - Flags: review?(chris.double) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/422dde19e8d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 780708
Duplicate of this bug: 787789
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
Attachment #649167 - Flags: approval-mozilla-beta?
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.
Attachment #649167 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 787789
You need to log in before you can comment on or make changes to this bug.