libcubeb AudioUnit backend isn't big endian compatible

RESOLVED FIXED in Firefox 16

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Tobias Netzel, Assigned: kinetik)

Tracking

16 Branch
mozilla17
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox16 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Attachment #647577 - Attachment is patch: true
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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.
Attachment #647577 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 649167 [details] [diff] [review]
patch v0

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)

Updated

5 years ago
Attachment #649167 - Flags: review?(chris.double) → review+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/422dde19e8d1
Target Milestone: --- → mozilla17
(Reporter)

Comment 6

5 years ago
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?
(Assignee)

Comment 7

5 years ago
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.
(Reporter)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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?
(Reporter)

Comment 10

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 780708
(Assignee)

Updated

5 years ago
Duplicate of this bug: 787789
(Assignee)

Comment 13

5 years ago
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+

Updated

5 years ago
Duplicate of this bug: 787789
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/451629b4e52f
status-firefox16: --- → fixed
You need to log in before you can comment on or make changes to this bug.