1240054 - Entirely Web Audio generated sound cuts out after a little while, or emits random tap tap tap sounds then silence

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Soledad Penades [:sole] [:spenades]]

This should play the entire THX sound for 26 seconds, but it either cuts out too early or doesn't play anything at all in latest Nightly (46 2016-01-15). The "speaker" icon on the tab is active throughout even if nothing or just some weird tap tap tap noise can be heard.

For a demo: http://sole.github.io/test_cases/web_audio/thx_cutting_out/
Source: https://github.com/sole/test_cases/tree/gh-pages/web_audio/thx_cutting_out

I have extracted the "offending code" out of a bigger piece of code (my slide deck at http://soledadpenades.com/files/t/2015_howa/#1 ), but I don't really know what is the reason for this cutting out or what else to extract out-sounds like perhaps something related to the filters and their automation? as the rest of slides seem to work all fine. I tried changing the amount of oscillators but the end result is always the same, i.e. sound cuts out too early, does tap tap tap or doesn't play at all.

I tried this in DevEdition 44 and it was fine, then it updated and stopped working. Nightly hasn't been working at all in the whole week (since I was told about the bug by someone in twitter on Monday, and it's Friday).

Comment 1

[Paul Adenot (:padenot)]

mozregression [0] told me the regression was caused by bug 1222405. Dan, care to have a look ?


[0] https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=74166e0168385eba427642965b93c7ad010662ca&tochange=3785489fa210d38b890ab73f21202b94c5528c1f

Comment 2

[Dan Minor [:dminor]]

Paul, thanks for tracking that down for me.

The band limited tables are rebuilt when the newly requested fundamental frequency is less than the old one because we may have more partials. In this case, we're decreasing the fundamental frequency at each step, so we end up rebuilding the tables over and over again.

I think we can be smarter about rebuilding the tables; worse comes to worst, I can revert the changes in [1].

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/3785489fa210

Comment 3

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1240054 - Only rebuild BandLimitedTables if more partials are required r?padenot

We currently rebuild the BandLimitedTables whenever we encounter a lower
fundamental frequency but it is only necessary to rebuild the tables if we
can fit more partials below the Nyquist frequency. Rebuilding the tables
unnecessarily can cause performance problems, particularly in the case where
the frequency is continually lowered.

Review commit: https://reviewboard.mozilla.org/r/31229/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31229/

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8709014 [details]
MozReview Request: Bug 1240054 - Only rebuild BandLimitedTables if more partials are required r?padenot

https://reviewboard.mozilla.org/r/31229/#review27999

Thanks. I take it this fixes Sole's problem ? Because this is a performance issue, it's a bit hard to test, but we should ask for uplift regardless.

Comment 5

[Dan Minor [:dminor]]

> Thanks. I take it this fixes Sole's problem ? Because this is a performance
> issue, it's a bit hard to test, but we should ask for uplift regardless.

I compared a build with this patch applied to an unaffected version of firefox ("iceweasel 38.5.0") and the playback sounded the same to me. It definitely fixes the audio cut out / tap tap tap problems.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f704088e0566

Comment 7

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
Regression caused by Bug 122405, landed in Fx45.

Comment 8

[Maire Reavy [:mreavy]]

Typo in comment 7.  Should say Bug 1222405.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f704088e0566

Comment 10

[Maire Reavy [:mreavy]]

Sole-- Can you test against the latest Nightly?  We want to verify that this fix resolves your reported problem.  We're planning to ask for uplift to Fx45 (and we'd love to get the fix in before we cut the first Beta Fx45).  Thanks!

Comment 11

[Soledad Penades [:sole] [:spenades]]

It's working! \o/

Comment 12

[Maire Reavy [:mreavy]]

Excellent!  Thanks, Sole -- especially for the quick reply!  

Hi Dan - Sole verifies this fixes her problem. \o/  Can you ask for uplift to Fx45 today?  And thanks for taking this one!! I really appreciate it.

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8709014 [details]
MozReview Request: Bug 1240054 - Only rebuild BandLimitedTables if more partials are required r?padenot

Approval Request Comment
[Feature/regressing bug #]: Bug 1222405
[User impact if declined]: Significant performance problems for any oscillator with a downward sweeping frequency which can result in unplayable audio.
[Describe test coverage new/current, TreeHerder]: Not currently tested because it is a performance problem, I plan to add a benchmark in [1] to catch further regressions of this type.
[Risks and why]: If there is a bug in the changed code we may not include as many partials as we should which could result in reduced audio quality. This should be caught by the current unit tests, so this is unlikely.
[String/UUID change made/needed]: None

[1] https://github.com/padenot/webaudio-benchmark

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8709014 [details]
MozReview Request: Bug 1240054 - Only rebuild BandLimitedTables if more partials are required r?padenot

Fix a regression, taking it

Comment 15

[Carsten Book [:Tomcat]]

has problems uplifting

grafting 324344:f704088e0566 "Bug 1240054 - Only rebuild BandLimitedTables if more partials are required r=padenot"
merging dom/media/webaudio/blink/PeriodicWave.cpp
warning: conflicts while merging dom/media/webaudio/blink/PeriodicWave.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take look, thanks!

Comment 16

[Carsten Book [:Tomcat]]

thanks :) 

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e13efb1f0b8