1499597 - Unable to set negative value for Q of BiquadFilterNode when type is "lowpass" or "highpass"

Status: VERIFIED FIXED

(Core :: Web Audio, defect, P2)

Description

[Takamitsu Endo]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

I wrote a code to make Karplus-Strong synthesizer with WebAudio in Firefox.

```javascript
function createKarplusStrongFilter(ctx, pitch, cutoff) {
  var delay = ctx.createDelay(4)
  delay.delayTime.value = 1 / pitch

  var filter = ctx.createBiquadFilter()
  filter.type = "lowpass"
  filter.frequency.value = cutoff
  filter.Q.value = -3 // This isn't working.
  filter.gain.value = 0

  var gain = ctx.createGain()
  gain.gain.value = 0.99

  var input = gain
  var output = gain

  delay.connect(filter)
  filter.connect(gain)
  gain.connect(delay)

  return { input, output, delay, filter, gain }
}

var ctx = new AudioContext()

var ksFilter = createKarplusStrongFilter(ctx, 220, 2000)

// Create impulse.
var buffer = ctx.createBuffer(2, 1, ctx.sampleRate)
buffer.copyToChannel(new Float32Array([1]), 0, 0)
buffer.copyToChannel(new Float32Array([1]), 1, 0)

var source = ctx.createBufferSource()
source.buffer = buffer

// Routing.
source.connect(ksFilter.input)
ksFilter.output.connect(ctx.destination)

source.start()
```


Actual results:

The above code blows up in Firefox while it doesn't in chromium.

Tested version:
- Firefox 62.0.3
- Firefox 64.0b1
- Chromium 69.0.3497.100 (Developer Build) Fedora Project (64-bit)


Expected results:

The above code shouldn't blows up.

Root cause of this issue is Q of BiquadFilterNode doesn't accept negative
value when type is "lowpass" or "highpass".

Removing

```cpp
resonance = std::max(0.0, resonance); // can't go negative
```

from setLowpassParams() and setHighpassParams() in
dom/media/webaudio/blink/Biquad.cpp may fix this issue.

Comment 1

[Daniel Bodea [:danibodea]]

Hi Takamitsu Endo, 

Please provide us with some steps to reproduce, actual and expected results, so that I can confirm and eventually verify it.

Thank you for your contribution!

Comment 2

[Takamitsu Endo]

Attachment: K-S_actual.wav is an actual result of the code in description.

Comment 3

[Takamitsu Endo]

Attachment: K-S_expected.wav is an expected result of the code in description.

Comment 4

[Takamitsu Endo]

Attachment: biquad_demo.zip

Interactive demo of BiquadFilterNode for checking the different behavior of Q.

Comment 5

[Takamitsu Endo]

Hi Daniel.

To reproduce the issue I encountered, copy and paste the javascript code in description to Web Console in developer tools.

Be careful not to execute code on critical sound system. Also turn down the volume. It may hurt your ear and equippment.

I attached the recordings of actual (K-S_actual.wav) and expected (K-S_expected.wav) results.

When I execute the code on firefox, I got K-S_actual.wav. Be careful when playing this.

When I execute the code on chromium, I got K-S_expected.wav.

---

Direct way to check BiquadFilterNode behavior is to use getFrequencyResponse function.

https://webaudio.github.io/web-audio-api/#dom-biquadfilternode-getfrequencyresponse

We can compare a magResponse of firefox to chromium one to confirm this issue.

Example code to get magResponse.

```javascript
var ctx = new AudioContext()

var filter = ctx.createBiquadFilter()
filter.type = "lowpass"
filter.frequency.value = 1000
filter.Q.value = -3 // This isn't working.

var freq = new Float32Array([20, 200, 2000, 20000])
var mag = new Float32Array(freq.length)
var phase = new Float32Array(freq.length)

filter.getFrequencyResponse(freq, mag, phase)

console.log(mag)
```

Output on firefox. (Actual result)

```
[ 1.000199317932129, 1.0197079181671143, 0.27431735396385193, 0.00011016758071491495 ]
```

Output on chromium. (Expected result)

```
[1.0000008344650269, 0.9993003606796265, 0.24034753441810608, 0.00011016154167009518]
```

To check further, You could change values of freq.

---

I also attached biquad_demo.zip for checking the behavior of BiquadFilterNode.

Unzip and open index.html with firefox and chromium, then tweak the Q and check how the black line on the top left canvas (magnitude of frequency response in decibel) is changing.

---

Just in case, this is the current implementation of biquad filter on chromium. As I mentioned in description, there aren't lines that clamping Q (which is named resonance in the code below).

https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/audio/biquad.cc?sq=package:chromium&dr=CSs&g=0


Thanks for taking time to this issue.

Comment 6

[Daniel Bodea [:danibodea]]

I have confirmed the issue on Windows 10 on the 3 main versions of Firefox by pasting the code in comment 0 in the web console and comparing the sounds. Thanks.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1499597 - Don't clamp the Q of a BiquadFilterNode when in low-pass and high-pass mode. r?karlt

Bug 1265395 implemented the new filter equations from
https://github.com/webaudio/web-audio-api/commit/a6842f2f733911a8ac6b330a405eac19878adc15,
but missed removing the clamping.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ecf31593df4
Don't clamp the Q of a BiquadFilterNode when in low-pass and high-pass mode. r=karlt

Comment 9

[Paul Adenot (:padenot)]

Thanks for the report and sorry for the delay!

Comment 10

[Narcis Beleuzu [:NarcisB]]

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

Comment 11

[Daniel Bodea [:danibodea]]

The sound after the fix does not sound identical to the one posted in comment 3, but it is very similar and free of distortion, so I deem it verified.