469698 - Wave audio playback is scrambled on PPC Mac

Status: VERIFIED FIXED

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

Description

[J. Gatellier]

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

Comment 1

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: fix?

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.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 4

[Marcia Knous [:marcia]]

If someone spins up a tryserver build I can test this on the PPC machine in the lab. Thanks.

Comment 5

[Marcia Knous [:marcia]]

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.

Comment 6

[Marcia Knous [:marcia]]

Here is the index for tryserver builds: https://build.mozilla.org/tryserver-builds/2008-12-24_07:[e-mail]-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.

Comment 7

[Marcia Knous [:marcia]]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Matthew Gregan [:kinetik]]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v0

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).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: patch v1

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/3a6adf4cc1c8 with that change.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed for FF3.1 beta 3!

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d4fe4feaa8c4

Comment 17

[Marcia Knous [:marcia]]

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.