Closed Bug 1424906 Opened 2 years ago Closed 2 years ago

PeriodicWave disableNormalization false is incorrect

Categories

(Core :: Web Audio, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: toy.raymond, Assigned: dminor)

References

Details

Attachments

(1 file)

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.
Component: Untriaged → Audio/Video
Product: Firefox → Core
Component: Audio/Video → Web Audio
Paul can you confirm or not this one?
Flags: needinfo?(padenot)
Whiteboard: [need info padenot 2017-12-12]
i can reproduce the issue on Nightly59.0a1 x64 win10, but not on Chrome.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Dan, can you have a look ?
Flags: needinfo?(padenot) → needinfo?(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.
Assignee: nobody → dminor
Flags: needinfo?(dminor)
Duplicate of this bug: 1266737
Status: NEW → ASSIGNED
Rank: 25
Priority: -- → P3
Whiteboard: [need info padenot 2017-12-12]
Comment on attachment 8936869 [details]
Bug 1424906 - Fix PeriodicWave disableNormalization false behaviour;

https://reviewboard.mozilla.org/r/207578/#review213508
Attachment #8936869 - Flags: review?(padenot) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d950eec8c65
Fix PeriodicWave disableNormalization false behaviour; r=padenot
https://hg.mozilla.org/mozilla-central/rev/1d950eec8c65
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.