68804 - Sound output cracks

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ben Bucksch (:BenB)]

Reproduce:
1. Get new mil in Mailnews

Actual result:
A sound is plyed, if new mail "arrived". But before the sound plays, a crack can
be heared in the speakers. Sophisticated speakers don't like that at all, it
also sounds unprofessional.

Expected result:
Clear, sharp output of the soundfile, and only that.

Linux.
(What is the component for GFX?)

Comment 1

[Igor Izyumin]

This is not a mozilla problem, but a linux sound driver problem in general.  On
my system, every time the system sound device is opened, there is a crackle. 
OSS and ALSA do that; maybe other drivers (eg commercial OSS) change that.

Comment 2

[Ben Bucksch (:BenB)]

No, XMMS (WinAMP clone), which uses OSS, doesn't exhibit this behaviour.

Comment 3

[Ben Bucksch (:BenB)]

Well, there are some cracks but
- rarely (only sometimes during skipping tracks)
- minimal (much less volume; shorter)

Comment 4

[Kevin Brosnan]

*** Bug 217803 has been marked as a duplicate of this bug. ***

Comment 5

[Joey Minta]

*** Bug 290953 has been marked as a duplicate of this bug. ***

Comment 6

[Uwe Dannowski]

This is a mozilla problem, not a driver problem.
The GTK(2) widget's nsSound::Play function (actually, its helper function nsSound::OnStreamComplete) sends the whole WAV file - including header and trailing garbage - to ESD, while ESD expects just the samples.

See
  http://lxr.mozilla.org/mozilla/source/widget/src/gtk/nsSound.cpp#130
or
  http://lxr.mozilla.org/mozilla/source/widget/src/gtk2/nsSound.cpp#134

Comment 7

[Ben Bucksch (:BenB)]

Changing component, owner, milestone, to reflect today

Comment 8

[Uwe Dannowski]

Attachment: Send only sample data, not WAV file headers to ESD

Comment 9

[Michael Wu [:mwu]]

Attachment: Do proper WAV file parsing

This works similarly to the previous patch, except it cleans up the code a bit, does more sanity checking and isn't bit rotted. Thanks to Uwe Dannowski for the analysis and initial patch.

Comment 10

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

+                if (i + audio_len >= dataLen)
+                    audio_len = (dataLen - i) - 1;

Why are you dropping the last byte of the file?

+        for (PRUint32 j = 0; j < audio_len - 1; j += 2) {

I'd prefer "j + 2 <= audio_len"

+            } else
+                i++;

I'd prefer braces around the i++. Also, is it really correct to scan through the WAV file looking for the string "fmt "? Aren't there other chunks that could have binary data in them that happens to contain the string "fmt "?

Comment 11

[Michael Wu [:mwu]]

(In reply to comment #10)
> +                if (i + audio_len >= dataLen)
> +                    audio_len = (dataLen - i) - 1;
> 
> Why are you dropping the last byte of the file?
> 
Opps. I knew something was a bit off about this code.. Thanks

> Also, is it really correct to scan through
> the WAV file looking for the string "fmt "? Aren't there other chunks that
> could have binary data in them that happens to contain the string "fmt "?
> 
I've added a check which makes sure we only parse one fmt chunk. It is fairly unlikely that fmt wouldn't the first chunk, so this should work out.

Comment 12

[Michael Wu [:mwu]]

Attachment: Do proper WAV file parsing, v2

Updated w/ roc's comments.

Comment 13

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

Is there no way to do a proper "skip to the next chunk" operation as we search for the "fmt " chunk?

Comment 14

[Michael Wu [:mwu]]

Attachment: Do proper WAV file parsing, v3

Properly skip unknown chunks, eliminate unused block_align variable.

Comment 15

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

Comment on attachment 233669 [details] [diff] [review]
Do proper WAV file parsing, v3

excellent!

Comment 16

[Michael Wu [:mwu]]

Checking in widget/src/gtk2/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.15; previous revision: 1.14
done

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 233669 [details] [diff] [review]
Do proper WAV file parsing, v3

This patch has been baking for a while and there has been no regressions AFAIK. It makes the gtk2 nsSound code actually parse WAV files instead of sending the whole file to the sound card.

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 233669 [details] [diff] [review]
Do proper WAV file parsing, v3

a=mconnor on behalf of drivers for checkin to the 1.8 branch

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Not going to block release on this ... but we did approve the patch.

Comment 20

[Michael Wu [:mwu]]

Checking in widget/src/gtk2/nsSound.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsSound.cpp,v  <--  nsSound.cpp
new revision: 1.12.20.2; previous revision: 1.12.20.1
done

Comment 21

[Adam Guthrie]

*** Bug 354743 has been marked as a duplicate of this bug. ***

Comment 23

[peter]

With Thunderbird 2.0.0.6 in Ubuntu 7.10 b1, the bug still there.

Comment 24

[peter]

Sorry, that was for another bug.