1424906 - PeriodicWave disableNormalization false is incorrect

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P3)

Description

[toy.raymond]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce:

Run the following in a dev console:

c = new OfflineAudioContext(1, 1000, 48000)
w = new PeriodicWave(c, {imag:[0, 1], disableNormalization: true})
o = new OscillatorNode(c, {frequency: 12000, periodicWave: w});
o.connect(c.destination);
o.start();
c.startRendering().then(buffer => console.log(buffer.getChannelData(0)));



Actual results:

Examine the output:

Float32Array [ 0, 2, 0, -2, 0, 2, 0, -2, 0, 2, … ]


Expected results:

The output should actually be [0,1,0,-1,0,1...]

The PeriodicWave object should generate a single sine tone of amplitude one.  But the actual amplitude is two.  If disableNormalization is set to false, the expected result is produced.

Comment 1

[Alex Chronopoulos [:achronop]]

Paul can you confirm or not this one?

Comment 2

[Alice0775 White]

i can reproduce the issue on Nightly59.0a1 x64 win10, but not on Chrome.

Comment 3

[Paul Adenot (:padenot)]

Dan, can you have a look ?

Comment 4

[Dan Minor [:dminor]]

Of course!

Looking at the Chromium version of this function I see the following:

  // TODO(rtoy): Figure out why this needs to be 0.5 when normalization is
  // disabled.
  float normalization_scale = 0.5;

Since the code we are using to create the bandlimited tables was originally imported from Chromium, it seems quite likely that we also need to apply a scaling factor even if normalization is disabled.

I should have realized this when I originally added the option, see Bug 1266737 and the comment I left in the test here: https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/media/webaudio/test/test_periodicWaveDisableNormalization.html#84 where I multiplied the expected results by two rather than fixing the scaling here.

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1424906 - Fix PeriodicWave disableNormalization false behaviour;

This adds a scaling factor of 0.5 even when normalization is
disabled, which is required for correct results.

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

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8936869 [details]
Bug 1424906 - Fix PeriodicWave disableNormalization false behaviour;

https://reviewboard.mozilla.org/r/207578/#review213508

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d950eec8c65
Fix PeriodicWave disableNormalization false behaviour; r=padenot

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1d950eec8c65