Closed Bug 1015519 Opened 6 years ago Closed 5 years ago

Creating a new AudioContext causes a brief clicking sound

Categories

(Core :: Web Audio, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox30 --- unaffected
firefox31 + fixed
firefox32 + fixed
firefox33 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: squib, Assigned: padenot)

References

(Depends on 1 open bug)

Details

(Keywords: regression, testcase, Whiteboard: interaction-design)

Attachments

(2 files)

Sorry I don't have a good test case for this, as I'm otherwise occupied at the moment, but I've noticed that when creating a new AudioContext and hooking up an Audio object to it, I get a brief clicking sound. This happens on both B2G and Firefox desktop (at least on nightly).

I noticed this in the past couple of weeks on my branch here: <https://github.com/jimporter/gaia/tree/ringtones-merge>. Here's the relevant block of code: <https://github.com/jimporter/gaia/blob/ringtones-merge/apps/ringtones/js/tone_player.js#L13>.
(In reply to Jim Porter (:squib) from comment #0)
> I've noticed that when creating a new AudioContext and
> hooking up an Audio object to it, I get a brief clicking sound.

Does the sound contain a single click or multiple clicks?

Does this happen even if there is only silence from the newly connected Audio object?

Or is this when the Audio begins playing, like bug 974232?

> I noticed this in the past couple of weeks

Do you know whether this is a regression?
i.e. do you know whether the click was absent some time before these weeks?
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #1)
> (In reply to Jim Porter (:squib) from comment #0)
> > I've noticed that when creating a new AudioContext and
> > hooking up an Audio object to it, I get a brief clicking sound.
> 
> Does the sound contain a single click or multiple clicks?

A single click.

> Does this happen even if there is only silence from the newly connected
> Audio object?

Yes. You get a click immediately when the Audio object is connected to the AudioContext.

> Or is this when the Audio begins playing, like bug 974232?

This can happen if I set the audio to begin playing immediately.

> > I noticed this in the past couple of weeks
> 
> Do you know whether this is a regression?
> i.e. do you know whether the click was absent some time before these weeks?

I am pretty sure this regressed sometime 2-3 weeks ago. I'll try to check.
fwiw, I've seen (heard) this as well, but at thought at the time it was my home audio system acting up. I think this is a regression, because it surprised me at the time.
I noticed this yesterday too!
Whiteboard: interaction-design
Jim - Is there a way to reproduce this in an E2E fashion with the ringtones app? STR could help us get a window here.
Flags: needinfo?(squibblyflabbetydoo)
STR

1. Go to settings -> Sound
2. Ensure volume is turned up - it seems like the click is louder at higher settings
3. Tap on either "Manage Ringtones" or the Ringer settings
Flags: needinfo?(squibblyflabbetydoo)
Using the STR in comment 6, check if this reproduces on 1.4.
blocking-b2g: --- → backlog
Duplicate of this bug: 1026141
The dupe answered the question in comment 7 - this is only present in 2.0 because 1.4 does not have this feature. The video is dupe though proves that the click is obvious though, so we need this fixed.
blocking-b2g: backlog → 2.0?
Keywords: qawanted
Priority: -- → P1
A window is going to be hard to do w/o a reduced test case.

Jim - Can you give QA a test case to use to do a window against?
Flags: needinfo?(squibblyflabbetydoo)
Attached file Test case
Here's a test case. Note: You don't need a device for this, or even B2G! It reproduces just fine on Firefox Desktop (Linux x86_64 nightly, at least).
Flags: needinfo?(squibblyflabbetydoo)
Oh, note that if you can't reproduce it, you might need to close the tab and reopen it, or even restart the browser, but it happens at least 90% of the time for me.
blocking-b2g: 2.0? → 2.0+
QA Contact: pcheng
Using the test tool on comment 11, I was able to find a window EARLIER than when 'Manage Ringtones' feature was introduced.

Mozilla inbound regression window:

Last Working Environmental Variables:
Device: Buri
Build ID: 20140411035728
Gaia: 1368d716072adf308e1b435ac828f97545a045f1
Gecko: 783c5013dbec
Version: 31.0a1 (Master) 
Firmware Version: v1.2device.cfg

First Broken Environmental Variables:
Device: Buri
Build ID: 20140411072825
Gaia: 1368d716072adf308e1b435ac828f97545a045f1
Gecko: 16803b53b8a5
Version: 31.0a1 (Master) 
Firmware Version: v1.2device.cfg

Gaia is the same on both builds, leading this to a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=783c5013dbec&tochange=16803b53b8a5
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
(In reply to Pi Wei Cheng from comment #13)
> Using the test tool on comment 11, I was able to find a window EARLIER than
> when 'Manage Ringtones' feature was introduced.

This is unsurprising, since we first noticed this while I was still working on bug 960329. Thanks for the analysis!
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Maybe bug 982490 broke this?
Flags: needinfo?(paul)
(In reply to Jason Smith [:jsmith] from comment #15)
> Maybe bug 982490 broke this?

Paul - What do you think?
I think that you are right, I came to the same conclusion, and posted in this bug, but I just saw it mid-aired.

This should not be hard to track down, we should just make sure all the data that go into the 
"AudioStream::Write" calls are good (e.g., pure silence when we just get an AudioContext and do nothing). The bug is probably in AudioSegment::WriteTo.
Flags: needinfo?(paul)
Paul -- since this is a 2.0 blocker and likely a regression from earlier your bug, I'm going to assign you as the bug owner.  If you need additional help tracking this down, please let me know.  Thanks.
Assignee: nobody → paul
Depends on: 982490
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
This was caused by the fact that we would write |GetDuration()| frames without
checking how many frame we actually wrote in the buffer, writing uninitialized
data to the AudioStream.
Attachment #8442755 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/2b15160c643b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 982490
No longer depends on: 982490
https://hg.mozilla.org/releases/mozilla-aurora/rev/34d3a6ced61d

Can we land a test for this regression?
Flags: needinfo?(paul)
Flags: in-testsuite?
Depends on: 1029557
This can't be done at the moment, but here is a plan:

Once we have bug 989921 landed and someone write some more code so that a test with the right privileges can access the mixer's output in JavaScript, this is feasible.

I've opened bug 1029554 (to get the ability to have the mixed audio output from JS) and bug 1029557 (to write the test for this bug), and I've set myself as mentor. When (if) things calm down in term of workload, and if it's not done, I'll probably do it myself, as it opens the door for some serious audio testing. I know some webrtc folks are interested as well, we'll see, maybe they'll grab it.
(clearing NEEDINFO)
Flags: needinfo?(paul)
Paul, could you fill the uplift request?
Flags: needinfo?(paul)
Comment on attachment 8442755 [details] [diff] [review]
Don't write uninitialized buffers to the AudioStream in AudioSegment::WriteTo. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 982490
User impact if declined: Short click when creating an AudioContext.
Testing completed (on m-c, etc.): baked on m-c for a while, verified manually on two FxOS devices + Firefox desktop on Linux (I could repro on all those devices/platforms without this patch).
Risk to taking this patch (and alternatives if risky): low risk, the cause is well understood and the patch is not complicated
String or IDL/UUID changes made by this patch: none
Attachment #8442755 - Flags: approval-mozilla-beta?
Flags: needinfo?(paul)
Attachment #8442755 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.