1203616 - Noisy/distorted audio on Web Audio demo (Ring Modulator from BBC)

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Loic]

Bug found when triaging bug 1203111.

STR:
1) Open http://webaudio.prototyping.bbc.co.uk/ring-modulator/#demo
2) Click on the speech tooltips.

Result: audio is altered and noisy.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7a68ee30a57450602d7e445a5ba61c2cdbb0c07e&tochange=6ce98e37b52e63b3d1cb452e9bb61edc164f6ddb

Yanis Sellami — Bug 1118372 - Properly apply volume in WaveShaperNodeEngine. r=padenot

Comment 1

[Karl Tomlinson (:karlt)]

Thanks, Loic.  I think this may be an effect of bug 1190215.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 42+. It is probably too late to take this in 41. 
Karl, how bad is this regression and how widespread are its effects?

Comment 3

[Karl Tomlinson (:karlt)]

It's a bad bug, but things, including the url here, were bad before due to bug 1118372.  Basically WaveShaperNode only works in particular situations.

There are some situations where things may have worked before bug 1118372 was fixed, but don't now.  However, I think it works in more situations now than before, so I think backing out changes from bug 1118372 would not be an improvement.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to get a fix in 41. While this is a valid bug, I do not believe it is a release blocker for 41. It would be nice if we can get a fix soon, for 42.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

No use tracking it for 41.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Maire, can you help find an owner for this bug? Thanks!

Comment 7

[Maire Reavy [:mreavy]]

Hi Liz, Paul is going to look at this and see how much worse we made things.  Depending on the fix and the risks/rewards, we'll see how far we want to uplift.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Properly scale the input buffer before processing it with the curve. r=

You were right in comment 1, Karl, simply doing this fixes it. I'm using this
bug and duping the other one against it because this one is tracked by releng.

I'll be adding a test tomorrow.

Comment 9

[Paul Adenot (:padenot)]

This is an easy, not risky patch, that will have tests. We'll hopefully be able to uplift it.

Comment 10

[Paul Adenot (:padenot)]

Attachment: Properly scale the input buffer before processing it with the curve. r=

Hrm, I forgot to qref.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Test that waveshaper nodes don't corrupt their input buffer. r=

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

>+      if (aInput.mVolume != 1.0) {

1.0f

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8670688 [details] [diff] [review]
Test that waveshaper nodes don't corrupt their input buffer. r=

>+++ b/dom/media/webaudio/test/test_mediaElementAudioSourceNodeFidelity.html

>+++ b/dom/media/webaudio/test/webaudio.js

I guess changes to these files were accidentally included in this patch, but
please remove those as they are unrelated.  binIndexForFrequency() can be
moved if and when required.

The new test is fine, but some suggestions to make the test a bit
stronger below:

>+osc.connect(gain);
>+gain.connect(ws);
>+gain.connect(sp);

Whether this test detects the bug depends on the order of processing, which is
not defined, but may happen to depend on the order of connections here.

To be sure of detecting this failure, two waveshapers can be connected to the
same gain node and their outputs tested.  See test_analyserNodeWithGain.html, for example.

>+  if (max > 0.49) {
>+    ok(true, "Volume was handled properly.");
>+    sp.onaudioprocess = null;
>+    SimpleTest.finish();
>+  }

Something like ok(Math.abs(max - gain.gain.value) < 0.01, "[...]") would
tighten up the test a bit.

If the frequency of the oscillator were chosen appropriately for script
processor buffer size and sample rate, then this could be tested in the first
audioprocess event, without needing to wait until sufficiently large max is
detected.  An offline context is an option so that sample rate is
configurable, which may make the frequency choice easier.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a61652a27e8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/240a3f4f4346

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a61652a27e8c
https://hg.mozilla.org/mozilla-central/rev/240a3f4f4346

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Paul, could you fill the uplift request to aurora (and beta, if you care enough) :) ?
merci

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

Approval Request Comment
[Feature/regressing bug #]: bug 1118372
[User impact if declined]: Bad behavior of some part of the Web Audio API (audio distortion), break content out there
[Describe test coverage new/current, TreeHerder]: Test added, been on inbound/central for some time
[Risks and why]: no risk, this is very easy and has a test 
[String/UUID change made/needed]: none

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8670688 [details] [diff] [review]
Test that waveshaper nodes don't corrupt their input buffer. r=

(See the comment above).

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

OK, thanks. Should be in 42 beta 6.
If there is any potential regressions, I will steal your new bike and use it as ransom until they are fixed! ;)

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b02ead76b603
https://hg.mozilla.org/releases/mozilla-beta/rev/ccb29d307ee6

Comment 22

[Carsten Book [:Tomcat]]

the second patch didn't apply cleanly to aurora:

grafting 307840:240a3f4f4346 "Bug 1203616 - Test that waveshaper nodes don't corrupt their input buffer. r=karlt"
merging dom/media/webaudio/test/mochitest.ini
warning: conflicts during merge.
merging dom/media/webaudio/test/mochitest.ini incomplete! (edit conflicts, then use 'hg resolve --mark')

paul, could you take a look, thanks!

Comment 23

[Paul Adenot (:padenot)]

Attachment: Test that waveshaper nodes don't corrupt their input buffer. (Aurora rebase)

Comment 24

[Paul Adenot (:padenot)]

Carsten, here the rebased patch.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d77a1a7bc476
https://hg.mozilla.org/releases/mozilla-aurora/rev/304c71a047eb

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d77a1a7bc476
https://hg.mozilla.org/releases/mozilla-aurora/rev/304c71a047eb