Closed Bug 469698 Opened 13 years ago Closed 13 years ago

Wave audio playback is scrambled on PPC Mac

Categories

(Core :: Audio/Video, defect, P2)

1.9.1 Branch
PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: gatellie, Assigned: kinetik)

References

()

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081215 Shiretoko/3.1b3pre (like Firefox/3.0)
Build Identifier: 

http://www.nch.com.au/acm/8k16bitpcm.wav
This 16-bit wave file plays OK on Windows but it's scrambled on PPC Mac.

Only 8-bit wave files seem to play correctly on PPC Mac.

Reproducible: Always
I can confirm problems playing this file
(http://www.nch.com.au/acm/8k16bitpcm.wav) on a PPC Mac (running OS X
10.4.11) -- but not using Firefox 3.0.4.  I only get the nasty hiss on
the trunk (Minefield) and the 1.9.1 branch (Shiretoko), using today's
nightlies.

I don't have any problems playing the file on Intel Macs (running OS X
10.4.11 or 10.5.6), or on Windows XP, even using today's Minefield and
Shiretoko nightlies.

For completeness, you should probably also attach or link an 8-bit
wave file.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch fix? (obsolete) — Splinter Review
This is probably what we need to do.  I don't have a PPC machine handy to test, so it'd be great if someone else can give this a spin.
I just tried and failed to do a tryserver build ... presumably due to tryserver flakiness.  Please do one yourself when you get a chance.  I'll test it on my PPC box.
Flags: blocking1.9.1?
If someone spins up a tryserver build I can test this on the PPC machine in the lab. Thanks.
NM - I uploaded the patch and I am spinning a tryserver build now.

(In reply to comment #4)
> If someone spins up a tryserver build I can test this on the PPC machine in the
> lab. Thanks.
Here is the index for tryserver builds: https://build.mozilla.org/tryserver-builds/2008-12-24_07:27-marcia@mozilla.com-Bug469698/

Testing the Mac build on a Leopard PPC mac running 10.5.6, the audio file plays fine. I will next test using 10.4, but it will take me a moment to restart.
I can also confirm the audio file plays fine running  Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20081224 Minefield/3.2a1pre, which is the tryserver build from Comment 6. So from the looks of it this patch fixes the issue.
Version: unspecified → 1.9.1 Branch
Seems to me that that conversion should happen in nsAudioStream::Write --- since we've specified the stream format as FORMAT_S16_LE, we should be passing in data in that format. (And libsydney_audio should specify that sa_stream_write takes shorts in native-endian format, if it doesn't say that already.)

It also happens to be slightly more efficient to do it there since we're already rewriting the buffer.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
I'm a bit confused here.  You're saying we should do byte-order conversion in Write, but then say that since the stream is opened as S16_LE, it should be able to expect data in that format (suggesting that the caller should convert it, like my test patch does).

As you say, it'd be more efficient to do the conversion in Write rather than in the caller (to avoid extra copies), but we'd need to rename FORMAT_S16_LE to FORMAT_S16 to make it clear the caller is passing the data with the host's byte ordering.  That sounds good to me, and is probably what you're suggesting anyway, but I thought I had better clarify before going ahead and doing that.
Currently, nsAudioStream::Write, when the format is FORMAT_S16_LE, is actually expecting bytes written in host format. But the Wave decoder is currently passing in data that's actually little-endian (it's raw Wave samples). So the bug is in nsAudioStream::Write, which needs to convert from little-endian to host format before multiplying by the volume.
Attached patch patch v0 (obsolete) — Splinter Review
Yeah, you're completely right.  I was just totally confused.  Here's the correct fix.  It depends on a fix to libsydneyaudio, which is filed as bug #447 (http://trac.annodex.net/ticket/447).
Assignee: nobody → kinetik
Attachment #353122 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #357304 - Flags: superreview?(roc)
Attachment #357304 - Flags: review?(chris.double)
Attachment #357304 - Flags: superreview?(roc) → superreview+
Attachment #357304 - Flags: review?(chris.double) → review+
Matthew, assuming you read this on your travels, can you roll up a patch that locally patches libsydneyaudio, so we can get this fixed on our trunk/1.9.1 branch ASAP?
Whiteboard: [needs landing]
Attached patch patch v1Splinter Review
Patch v0 plus the patch from Annodex trac ticket #427 applied.  I also fixed the SVN revision number in the readme to reflect reality, since it wasn't updated last time we pulled libsydneyaudio into our tree.
Attachment #357304 - Attachment is obsolete: true
Attachment #359376 - Flags: superreview?(roc)
Attachment #359376 - Flags: review?(chris.double)
Attachment #359376 - Flags: review?(chris.double) → review+
Comment on attachment 359376 [details] [diff] [review]
patch v1

+          s = (s & 0x00ff) << 8 | (s & 0xff00) >> 8;

I'd put extra parens in here to avoid anxiety (mine). Someone can do that when they check in.
Attachment #359376 - Flags: superreview?(roc) → superreview+
Pushed http://hg.mozilla.org/mozilla-central/rev/3a6adf4cc1c8 with that change.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Verified fixed on the 1.9.1 branch using  Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090205 Shiretoko/3.1b3pre. I verified using the URL in the bug. Adding keyword.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090205 Minefield/3.2a1pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.