265194 - Mozilla can't play wav file correctly on solaris sparc

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Leon Sha]

If I have mozilla play a custom sound when mail arrives, all I get on my sparc
is a grinding noise. The sound file is in WAV format. 
Steps:
1) Open Edit->Preference->Mail&News Groups->Notifications
2) In the "Custom .wav file" field, select a wav file.
3) Click "Preview"

It will play noise instead of the wav file

Comment 1

[Leon Sha]

Attachment: Patch

This is caused by endian problem.
Sun sparc is little endian cpu while pc is big endian cpu.
So in this patch I just let esd to handle the file.
We do not need to play the stream by mozilla itself.

Comment 2

[Boris Zbarsky [:bzbarsky]]

What if the URL is not a local file?  The nsISound api allows arbitrary URIs to
be passed in.

In any case, converting a URI to a filename via GetPath() is wrong...  You want
to QI to nsIFileURL and get the nsIFile.  Otherwise there are character encoding
issues.

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 162675 [details] [diff] [review]
Patch

minusing, we discussed this patch on IRC.  I don't want gtk to be the
odd-platform-out in not supporting arbitrary URI's for sounds.	Instead, we
should put endian-conversion code here if it's needed.

Comment 4

[Leon Sha]

Attachment: Patch v2

Comment 5

[Leon Sha]

Attachment: Patch v3

Add null check new according to timeless's comments.
I think it is not nesessary to use nsCAutostring. It is a simple use of string.
Also I do not use NS_SWAP16 because it is not the function. I can  add more
comments to explains how to do the conertion.

Comment 6

[Leon Sha]

(In reply to comment #5)
> Created an attachment (id=164266)
> Patch v3
> 
> Add null check new according to timeless's comments.
> I think it is not nesessary to use nsCAutostring. It is a simple use of string.
> Also I do not use NS_SWAP16 because it is not the function. I can  add more
> comments to explains how to do the conertion.
s/conertion/convertion.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

+    PRUint8 *buf = new PRUint8[dataLen - WAV_MINI_LENGTH];
+    if (!buf)
+      return NS_ERROR_FAILURE;

maybe NS_ERROR_OUT_OF_MEMORY?

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 164266 [details] [diff] [review]
Patch v3

Just a few minor comments:

>--- widget/src/gtk/nsSound.cpp	4 May 2004 18:02:35 -0000	1.40
>+++ widget/src/gtk/nsSound.cpp	2 Nov 2004 10:00:20 -0000
>@@ -62,16 +62,18 @@ static PRLibrary *elib = nsnull;
> 
> #define ESD_BITS8  (0x0000)
> #define ESD_BITS16 (0x0001) 
> #define ESD_MONO (0x0010)
> #define ESD_STEREO (0x0020) 
> #define ESD_STREAM (0x0000)
> #define ESD_PLAY (0x1000)
> 
>+#define WAV_MINI_LENGTH 44
>+

This is "minimum", correct?  We usually use MIN as the shortened form of that,
I'd suggest changing it for consistency.

>@@ -164,16 +166,23 @@ NS_IMETHODIMP nsSound::OnStreamComplete(
> 
>   if (memcmp(data, "RIFF", 4)) {
> #ifdef DEBUG
>     printf("We only support WAV files currently.\n");
> #endif
>     return NS_ERROR_FAILURE;
>   }
> 
>+  if (dataLen <= WAV_MINI_LENGTH) {
>+#ifdef DEBUG
>+      printf("WAV files should be longer than 44.\n");

"44 bytes", assuming that's the unit here.

>@@ -229,18 +238,37 @@ NS_IMETHODIMP nsSound::OnStreamComplete(
> 
>   fd = (*EsdPlayStreamFallback)(mask, rate, NULL, "mozillaSound"); 
>   
>   if (fd < 0) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   /* write data out */
>+#ifdef IS_BIG_ENDIAN
>+  if(bits_per_sample == 8)
>+    write(fd, data, dataLen);
>+  else {
>+    PRUint8 *buf = new PRUint8[dataLen - WAV_MINI_LENGTH];

It would probably be good to add a comment about what's going on, such as  " //
Swap the byte order if we're on a big-endian architecture"

>+    if (!buf)
>+      return NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY, as you mentioned on irc.

>+    PRUint32 j = 0;
>+    // We do SWAP16 manually.
>+    while(j < dataLen - WAV_MINI_LENGTH - 1) {
>+      buf[j] = data[j + WAV_MINI_LENGTH + 1];
>+      buf[j + 1] = data[j + WAV_MINI_LENGTH];
>+      j+=2;
>+    }

This might be a bit easier to read as a for loop:

for (PRUint32 j = 0; i < dataLen - WAV_MINI_LENGTH - 1; j += 2) {
  ...
}

I'm not particular either way though.  I am wondering, though, why you skip the
last 45 bytes of the data... what's going on there?

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 164266 [details] [diff] [review]
Patch v3

Ok, sounds like the reason we skip the first few bytes is that it's a header we
don't really need to send.  So r=me with my other previous comments addressed.

Comment 10

[Leon Sha]

Attachment: Patch v4

With bryner's comments.

Comment 11

[Darin Fisher]

Comment on attachment 164345 [details] [diff] [review]
Patch v4

>Index: widget/src/gtk/nsSound.cpp

>+  if (dataLen <= WAV_MIN_LENGTH) {
>+#ifdef DEBUG
>+      printf("WAV files should be longer than 44 bytes.\n");
>+#endif

use NS_WARNING instead:

	NS_WARNING("WAV files should be longer than 44 bytes");


>+#ifdef IS_BIG_ENDIAN
>+  if(bits_per_sample == 8)

nit: please add a space between "if" and the opening paranthesis.  thx!


>Index: widget/src/gtk2/nsSound.cpp

>+    if (dataLen <= WAV_MIN_LENGTH) {
>+#ifdef DEBUG
>+        printf("WAV files should be longer than 44 bytes.\n");
>+#endif

same thing here.. use NS_WARNING.

>+    if(bits_per_sample == 8)

and same nit here.. add an extra space.


sr=darin with those nits fixed.

Comment 12

[Leon Sha]

Attachment: Patch for check in with darin's comments.

Comment 13

[Ginn Chen]

Checking in widget/src/gtk/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in widget/src/gtk2/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.12; previous revision: 1.11
done