1015519 - Creating a new AudioContext causes a brief clicking sound

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Jim Porter (:squib)]

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>.

Comment 1

[Karl Tomlinson (:karlt)]

(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?

Comment 2

[Jim Porter (:squib)]

(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.

Comment 3

[Paul Adenot (:padenot)]

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.

Comment 4

[(no longer active)]

I noticed this yesterday too!

Comment 5

[Jason Smith [:jsmith]]

Jim - Is there a way to reproduce this in an E2E fashion with the ringtones app? STR could help us get a window here.

Comment 6

[Tiffanie Shakespeare [:tif]]

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

Comment 7

[Jason Smith [:jsmith]]

Using the STR in comment 6, check if this reproduces on 1.4.

Comment 9

[Jason Smith [:jsmith]]

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.

Comment 10

[Jason Smith [:jsmith]]

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?

Comment 11

[Jim Porter (:squib)]

Attachment: 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).

Comment 12

[Jim Porter (:squib)]

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.

Comment 13

[Pi Wei Cheng [:piwei] (inactive)]

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

Comment 14

[Jim Porter (:squib)]

(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!

Comment 15

[Jason Smith [:jsmith]]

Maybe bug 982490 broke this?

Comment 16

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #15)
> Maybe bug 982490 broke this?

Paul - What do you think?

Comment 17

[Paul Adenot (:padenot)]

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.

Comment 18

[Maire Reavy [:mreavy]]

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.

Comment 19

[Paul Adenot (:padenot)]

Attachment: Don't write uninitialized buffers to the AudioStream in AudioSegment::WriteTo. r=

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.

Comment 20

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b15160c643b

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b15160c643b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34d3a6ced61d

Can we land a test for this regression?

Comment 23

[Paul Adenot (:padenot)]

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.

Comment 24

[Paul Adenot (:padenot)]

(clearing NEEDINFO)

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Paul, could you fill the uplift request?

Comment 26

[Paul Adenot (:padenot)]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/643d6fce5bee