339124 - IntelMac: nsISound::Play() does not work

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Kathleen :Brade]

I added a new feature to my extension to play a sound.  It does not work on my Intel-based iMac.

Steps to reproduce in Firefox 1.5.0.x:
 * about:config
 * change pref accessibility.typeaheadfind.soundURL to:
       http://www.dailywav.com/0506/annoyinganimal.wav
 * restart Firefox
 * load a page and search for text that isn't on page

Results:
 * my TiBook plays the sound
 * my iMac plays nothing (and throws no errors)

Guess: byte order problem?

Comment 1

[Josh Aas]

Attachment: fix v1.0

yup, endian problem

Comment 2

[Mark Mentovai]

Comment on attachment 223219 [details] [diff] [review]
fix v1.0

>     // look for WAVE
>     const char* dataPtr = inData;
>-    if (*(OSType *)dataPtr == 'RIFF')
>+    if (PR_ntohl(*(OSType *)dataPtr) == 'RIFF')
>     {
>       dataPtr += 4;     // skip RIFF
>       dataPtr += 4;     // skip length bytes
>-      if (*(OSType *)dataPtr == 'WAVE')
>+      if (PR_ntohl(*(OSType *)dataPtr) == 'WAVE')
>         return kQTFileTypeWave;
>     }
>     
>     // look for AIFF
>     dataPtr = inData;
>-    if (*(OSType *)dataPtr == 'FORM')
>+    if (PR_ntohl(*(OSType *)dataPtr) == 'FORM')

...but you already converted that value once...

Let's take this opportunity to do a little restructuring.  Call PR_ntohl(*dataPtr) only once for the first four bytes, and remember that value so you can use it again to look for 'FORM' after having looked for 'RIFF'.

>     {
>       dataPtr += 4;     // skip FORM
>       dataPtr += 4;     // skip length bytes
>-      if (*(OSType *)dataPtr == 'AIFF')
>+      if (PR_ntohl(*(OSType *)dataPtr) == 'AIFF')
>         return kQTFileTypeAIFF;
>        
>-      if (*(OSType *)dataPtr == 'AIFC')
>+      if (PR_ntohl(*(OSType *)dataPtr) == 'AIFC')

...and in here, you're looking at the same bytes (at offset 8) as you did in the 'RIFF' case.  Although I won't be a jerk if you don't want to consolidate the WAVE check with the AIFF and AIFC checks, you should at least only convert the value once when looking for AIFF or AIFC.

>-    if (*(OSType *)inData == 'MThd')
>+    if (PR_ntohl(*(OSType *)inData) == 'MThd')

...Ditto here, no need to keep doing the same conversion...

Comment 3

[Josh Aas]

Attachment: fix v1.1

Comment 4

[Mark Mentovai]

Comment on attachment 223231 [details] [diff] [review]
fix v1.1

>+  

You already mentioned this.

>+      const char* dataPtr = inData;
>       dataPtr += 4;     // skip RIFF
>       dataPtr += 4;     // skip length bytes

Initialize dataPtr to inData + 8, as long as you keep some comment that describes what you're skipping.

>+      const char* dataPtr = inData;
>       dataPtr += 4;     // skip FORM
>       dataPtr += 4;     // skip length bytes

Ditto.

Comment 5

[Mark Mentovai]

x86 Mac parity issue, nominating for 1.8.0.5.

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 223231 [details] [diff] [review]
fix v1.1

sr=pink

Comment 7

[Josh Aas]

landed on trunk, needs to land on 1.8.1 and 1.8.0 branches

Comment 8

[Josh Aas]

landed on 1.8.1 branch

Comment 9

[Marcia Knous [:marcia]]

Verified on the 1.8.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1a3) Gecko/20060531 BonEcho/2.0a3. Will verify the 1.8.0 branch fix when it lands.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 223231 [details] [diff] [review]
fix v1.1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[Josh Aas]

landed on 1.8.0 branch

Comment 12

[Marcia Knous [:marcia]]

verified fixed on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1a3) Gecko/20060626 BonEcho/2.0a3.