939491 - "Acid Defender" WebAudio game audio slows down then stops after switching tabs

Status: VERIFIED FIXED

(Core :: Web Audio, defect, P1)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load http://www.cappel-nord.de/webaudio/acid-defender/
2. Select "Jam Mode"
3. Wait a few seconds for the music to start
4. Switch to a different tab
5. The music was stop with reverb fading out (as expected)
6. Switch back to the "Acid Defender" tab
7. The music will resume
8. Click one of the high hat buttons: "Hat 1", "Hat 2", or "Hat 3"
9. Wait about 10 seconds

RESULT:
A few seconds after the high hat sound starts, the music playback will start slowing down then completely stop. If you reload the tab and start again, the music will play fine.

This bug seems to be a regression in Nightly 27 build 2013-10-25. WebAudio did not play at all in build 10-24 and a few earlier builds. Here is the pushlog from build 10-24 and 10-25:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=19fd3388c372&tochange=9f8233fcce1d

Comment 1

[Chris Peterson [:cpeterson]]

Karl: could this WebAudio bug be a regression from bug 923301? That bug's fix is in the regression range pushlog.

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to Chris Peterson (:cpeterson) from comment #0)
> This bug seems to be a regression in Nightly 27 build 2013-10-25. WebAudio
> did not play at all in build 10-24 and a few earlier builds.

These two sentences seem to be contradictory.
Do you mean a *pro*gression in Nightly 27 build 2013-10-25?

Do you know any earlier build that worked as expected?

(In reply to Chris Peterson (:cpeterson) from comment #1)
> Karl: could this WebAudio bug be a regression from bug 923301? That bug's
> fix is in the regression range pushlog.

Bug 923301 may have been what made this play at all.  Is that what you mean?
That bug may have reduced the cpu load on the MediaStreamGraph thread enough that it could produce some audio.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> (In reply to Chris Peterson (:cpeterson) from comment #0)
> > This bug seems to be a regression in Nightly 27 build 2013-10-25. WebAudio
> > did not play at all in build 10-24 and a few earlier builds.
> 
> These two sentences seem to be contradictory.
> Do you mean a *pro*gression in Nightly 27 build 2013-10-25?
> 
> Do you know any earlier build that worked as expected?

Identifying the regression build is difficult because I think there are two overlapping bugs. This test stopped playing WebAudio between builds 10-10 and 10-20. In build 10-25, this test started playing audio again but it has the "audio slows then stops" bug. So the "slow audio" bug might have been introduced in the "no audio" regression range, but it seems more likely that the "slow audio" bug was a regression from the "no audio" bug fix.

Some results from my bisection of Nightly builds:

* 2013-10-10 = audio played correctly
...
* 2013-10-20 = no audio!
...
* 2013-10-24 = no audio
* 2013-10-25 = audio played correctly but has slow audio bug!
* 2013-10-25 = "
* 2013-10-26 = "
* 2013-10-28 = "
* 2013-10-31 = "
* 2013-11-02 = "
* 2013-11-06 = "

Comment 4

[Chris Peterson [:cpeterson]]

I bisected the mozilla-inbound builds archived on TBPL and identified the following regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a25982be666c&tochange=578c80c21547

This halting audio bug was introduced the same day as the no audio bug was fixed, but that was just a coincidence. Bisecting the mozilla-inbound builds should that the no audio bug was fixed before the halting audio bug was introduced.

Comment 5

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/5a9ac6fed6ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131024214310
Bad:
http://hg.mozilla.org/mozilla-central/rev/186e834d87dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131025011312
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5a9ac6fed6ff&tochange=186e834d87dc


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a25982be666c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131024185909
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/578c80c21547
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131024190609
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a25982be666c&tochange=578c80c21547

Suspected: bug 898291, bug 924288, bug 926619, bug 923301

Comment 6

[Karl Tomlinson (:karlt)]

Thanks.  Confirmed that backing out allows this to continue playing.

I'll check whether the new code is not working as expected, or whether long tails on BiquadFilterNodes are making us continue to process each sound for a long time until there are too many sounds to process.

I spoke with Ralph about the possibility of chopping off the signal and processing when biquad output dropped to a certain level, but he pointed out that there is no practical absolute level with float samples because there may be a gain node downstream.

Comment 7

[Karl Tomlinson (:karlt)]

Confirmed that backing out https://hg.mozilla.org/mozilla-central/rev/d8fe38e7b4f7 allows this to continue playing.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> I'll check whether the new code is not working as expected, or whether long
> tails on BiquadFilterNodes are making us continue to process each sound for
> a long time until there are too many sounds to process.

I don't know if this is a related problem to the long tails, but this test used to exhibit bug 907318 ("'Acid Defender' WebAudio game has much more reverb compared to Chrome").

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> this test
> used to exhibit bug 907318 ("'Acid Defender' WebAudio game has much more
> reverb compared to Chrome").

Thanks, I noticed there was a change in reverb, and narrowed that down to http://hg.mozilla.org/mozilla-central/rev/d53c32505943

Comment 10

[Karl Tomlinson (:karlt)]

A time-based tail cut-off might work better.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/webaudio/BiquadDSPKernel.cpp&l=37

Comment 11

[Karl Tomlinson (:karlt)]

When input is zero the output is defined by a recurrence relation involving
the previous two samples.  For constant coefficients we can predict the decay
time, but the coefficients can vary over time, so we would need to
integrate/sum the predicted decays, and it ends up easier to just watch the
values.

In this example, the frequency of the relevant low-pass filter is changing but
ends up at 200 Hz, at which point
m_a1 = -1.9802982118813859 and m_a2 = 0.98110246009360835.

The single precision output values become subnormal after about 45 blocks of
128 samples.  At 44.1 kHz, that is 130 ms, which is less than the 200ms tail
time used in Blink.

The filter code [1] may look like it flushes out subnormals to zero, but that
fails for two reasons:

1) If HAVE_DENORMAL is defined, then flushDenormalFloatToZero() is a no-op on
   x86(_64) because it is assumed that a DenormalDisabler exists while this
   code is run.  No such object is instantiated in Gecko.  The double
   calculated value is rounded to a denormal float at each tick, and so future
   values no longer decrease and never round to zero. 

2) Even when HAVE_DENORMAL is not defined, though perhaps unlikely, it is
   possible that flushDenormalFloatToZero() may sometimes flush only one of
   the historic output values to zero.  As one of the coefficients is > 1, the
   other value can get amplified, and I expect this can lead to an oscillating
   cycle that never reaches zero.

Flushing subnormal audio sample values to zero is sensible, but making the
hardware flush subnormals to zero for all arithmetic may cause unexpected
results, such as divide-by-zero due to permitting a - b = 0 when a != b.

[1] http://hg.mozilla.org/mozilla-central/annotate/5eb1c89fc2bc/content/media/webaudio/blink/Biquad.cpp#l64

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: avoid producing infinite stream of subnormals in BiquadFilterNode tail

Using double precision for the processing means the main loop is unlikely to
encounter subnormals and provides suitable extra precision to minimize
accumulation of error.

If the tail output values are denormalized when converted to single precision
then flush them to zero to reduce downstream computation cost.

Comment 13

[(no longer active)]

Comment on attachment 8339563 [details] [diff] [review]
avoid producing infinite stream of subnormals in BiquadFilterNode tail

Review of attachment 8339563 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane, but I'd like Paul to take a look at it as well to check the math etc.

Comment 14

[Karl Tomlinson (:karlt)]

I'll replace the "DenormalDisabler.h" include with <float.h>.

Comment 15

[(no longer active)]

(In reply to comment #14)
> I'll replace the "DenormalDisabler.h" include with <float.h>.

Sounds good.  Is this also a problem in other places where we use DenormalDisabler::flushDenormalFloatToZero?  Also, does this bug affect Blink as well?

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> Is this also a problem in other places where we use
> DenormalDisabler::flushDenormalFloatToZero?

Bug 944143.

> Also, does this bug affect Blink as well?

A DenormalDisabler is instantiated at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp&cl=GROK&rcl=1385536680&l=59
That and the tail time limit deals with most of the issues here.

There may be different bugs in Blink.  For example, the clicks when playing this demo in Chromium may be due to truncating the tail too soon - I don't know.

Comment 17

[(no longer active)]

(In reply to comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > Is this also a problem in other places where we use
> > DenormalDisabler::flushDenormalFloatToZero?
> 
> Bug 944143.
> 
> > Also, does this bug affect Blink as well?
> 
> A DenormalDisabler is instantiated at
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp&cl=GROK&rcl=1385536680&l=59
> That and the tail time limit deals with most of the issues here.
> 
> There may be different bugs in Blink.  For example, the clicks when playing
> this demo in Chromium may be due to truncating the tail too soon - I don't
> know.

I see.  In the interest of being a good citizen with our borrowed code, can you please report this to them and include a link to the bug here and let them figure things out?  Thanks!

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8339563 [details] [diff] [review]
avoid producing infinite stream of subnormals in BiquadFilterNode tail

Review of attachment 8339563 [details] [diff] [review]:
-----------------------------------------------------------------

This seems correct to me.

Comment 19

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c904838825a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c904838825a

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8339563 [details] [diff] [review]
avoid producing infinite stream of subnormals in BiquadFilterNode tail

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924288
User impact if declined: perf impact on some webaudio applications, sometimes significant enough to stall the audio graph (and stop the sound or make it jerky).
Testing completed (on m-c, etc.): on m-c, observed the patch behaved as expected in a debugger.
Risk to taking this patch (and alternatives if risky): lowish.
String or IDL/UUID changes made by this patch: none.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5368478c4dde

Comment 23

[ganesh_sahai1]

Tested on :---
User Agent : Mozilla/5.0 (Windows NT 6.2; rv:28.0) Gecko/20100101 Firefox/28.0
Version : Firefox  28.0a1
Platform Tested : x86_64, Win 8

Status : Found not fixed. As compared to Google Chrome the voice in not clear.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to ganesh_sahai1 from comment #23)
> Status : Found not fixed. As compared to Google Chrome the voice in not
> clear.

I hear a kind of muffled buzz that is not in Chrome, but I also hear that in a 2013-10-15 build off febfe3c7732b.
Is that what you are hearing?

That would be a different bug.
This bug is about the game slowing down and stopping.
Does the game slow down and stop?

Comment 25

[ganesh_sahai1]

Test on the build at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b8-candidates/build1/win32/en-US/

The sound problem is there in the above build as well as in the latest nightly build.

In the latest the build the game does not show stops same as in the earlier (release 25 or 25.0b8). It has improved compared to those previous builds. However,in the latest build there are some jerkiness in video which is not there in Chrome.

Comment 26

[u279076]

Paul, can you do some exploratory testing to ensure this is really fixed?

Comment 27

[Paul Silaghi, QA [:pauly]]

I reproduced the problem on nightly 2013-11-17.
The audio doesn't slow down on FF 27b2, 28.0a2 2013-12-19 on Win 7, Mac OS X 10.8.5, Ubuntu 12.04.
Verified fixed.

Comment 28

[Paul Silaghi, QA [:pauly]]

Also found another issue here:
Pressing the X button to close Firefox having 2 tabs opened with the focus on the game tab displays the "close multiple tabs" warning and the song stops as expected. Canceling the warning will make the audio no longer automatically continue playing on FF 28 and up. This is not reproducible on FF 27.
Please say if this worth to be filed separately.

Comment 29

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> Please say if this worth to be filed separately.

Yes, please file that and cc me.

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> > There may be different bugs in Blink.  For example, the clicks when playing
> > this demo in Chromium may be due to truncating the tail too soon - I don't
> > know.
> 
> I see.  In the interest of being a good citizen with our borrowed code, can
> you please report this to them and include a link to the bug here and let
> them figure things out?  Thanks!

I tested truncating the tail in Gecko after to 0.2 seconds to match Chromium's implementation and didn't here the clicks, so they seem to be unrelated to BiquadFilter tail code.

There are other differences in how the demos sound in the two browsers, but I don't know which is more correct.