Closed Bug 68804 Opened 20 years ago Closed 14 years ago

Sound output cracks

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: BenB, Assigned: mwu)

References

Details

(Keywords: fixed1.8.1, platform-parity)

Attachments

(1 file, 3 obsolete files)

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?)
Keywords: mozilla1.0
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.
No, XMMS (WinAMP clone), which uses OSS, doesn't exhibit this behaviour.
Well, there are some cracks but
- rarely (only sometimes during skipping tracks)
- minimal (much less volume; shorter)
Keywords: pp
Target Milestone: --- → M1
Target Milestone: M1 → Future
*** Bug 217803 has been marked as a duplicate of this bug. ***
*** Bug 290953 has been marked as a duplicate of this bug. ***
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
Changing component, owner, milestone, to reflect today
Assignee: slogan → nobody
Component: XPCOM → Widget: Gtk
QA Contact: kandrot → gtk
Target Milestone: Future → ---
Assignee: nobody → michael.wu
Attached patch Do proper WAV file parsing (obsolete) — Splinter Review
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.
Attachment #215756 - Attachment is obsolete: true
Attachment #233004 - Flags: superreview?(roc)
Attachment #233004 - Flags: review?(roc)
+                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 "?
(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.
Attached patch Do proper WAV file parsing, v2 (obsolete) — Splinter Review
Updated w/ roc's comments.
Attachment #233004 - Attachment is obsolete: true
Attachment #233589 - Flags: superreview?(roc)
Attachment #233589 - Flags: review?(roc)
Attachment #233004 - Flags: superreview?(roc)
Attachment #233004 - Flags: review?(roc)
Is there no way to do a proper "skip to the next chunk" operation as we search for the "fmt " chunk?
Properly skip unknown chunks, eliminate unused block_align variable.
Attachment #233589 - Attachment is obsolete: true
Attachment #233669 - Flags: superreview?(roc)
Attachment #233669 - Flags: review?(roc)
Attachment #233589 - Flags: superreview?(roc)
Attachment #233589 - Flags: review?(roc)
Comment on attachment 233669 [details] [diff] [review]
Do proper WAV file parsing, v3

excellent!
Attachment #233669 - Flags: superreview?(roc)
Attachment #233669 - Flags: superreview+
Attachment #233669 - Flags: review?(roc)
Attachment #233669 - Flags: review+
Whiteboard: [checkin needed]
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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.
Attachment #233669 - Flags: approval1.8.1?
Flags: blocking1.8.1?
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
Attachment #233669 - Flags: approval1.8.1? → approval1.8.1+
Not going to block release on this ... but we did approve the patch.
Flags: blocking1.8.1? → blocking1.8.1-
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
Keywords: fixed1.8.1
*** Bug 354743 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 377145
With Thunderbird 2.0.0.6 in Ubuntu 7.10 b1, the bug still there.
Sorry, that was for another bug.
You need to log in before you can comment on or make changes to this bug.