1265408 - Implement IIRFilterNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

This adds a new IIRFilterNode that implements a general IIR filter.

Spec changes at https://github.com/webaudio/web-audio-api/commit/3b452b1de953fbee41a7e30657e21dfb7b7110ff.

Comment 1

[Karl Tomlinson (:karlt)]

Intent to implement and ship discussion:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/XoJ7Ikj3Www

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Add webidl for IIRFilterNode;

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

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Import IIRFilter from blink;

Imported from git revision 57f70919a0a3da5ba002b896778b580986343e08.

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

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Use IIRFilter from blink;

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

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Implement IIRFilterNode;

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

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Add test for IIRFilterNode pass through;

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

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Avoid complex division in getFrequencyResponse;

Using the division operator on std::complex values fails on our linux64
AWS test machines, yielding infinities and NaNs for valid inputs. Presumably
this could affect machines in the wild as well. This patch removes the use
of the division operator and replaces it with real operations.

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

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Import blink IIRFilterNode tests;

Imported from git revision 57f70919a0a3da5ba002b896778b580986343e08.

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

Comment 9

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Use blink IIRFilterNode tests;

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8750373 [details]
Bug 1265408 - Add webidl for IIRFilterNode;

https://reviewboard.mozilla.org/r/51369/#review48127

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8750374 [details]
Bug 1265408 - Import IIRFilter from blink;

https://reviewboard.mozilla.org/r/51371/#review48363

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8750375 [details]
Bug 1265408 - Use IIRFilter from blink;

https://reviewboard.mozilla.org/r/51373/#review48365

::: dom/media/webaudio/blink/IIRFilter.h:51
(Diff revision 1)
>      // yBuffer[bufferIndex] is where y[n], the current output value.
>      int m_bufferIndex;
>  
> -    // Coefficients of the IIR filter.  To minimize storage, these point to the arrays given in the
> -    // constructor.
> -    const AudioDoubleArray* m_feedback;
> +    // Coefficients of the IIR filter.
> +    const AudioDoubleArray *m_feedback;
> +    const AudioDoubleArray *m_feedforward;

* near the type in Gecko.

Comment 13

[Paul Adenot (:padenot)]

https://reviewboard.mozilla.org/r/51373/#review48367

::: dom/media/webaudio/blink/IIRFilter.h:51
(Diff revision 1)
>      // yBuffer[bufferIndex] is where y[n], the current output value.
>      int m_bufferIndex;
>  
> -    // Coefficients of the IIR filter.  To minimize storage, these point to the arrays given in the
> -    // constructor.
> -    const AudioDoubleArray* m_feedback;
> +    // Coefficients of the IIR filter.
> +    const AudioDoubleArray *m_feedback;
> +    const AudioDoubleArray *m_feedforward;

* I meant, it got converted to a bullet list in markdown.

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

https://reviewboard.mozilla.org/r/51375/#review48119

::: dom/media/webaudio/IIRFilterNode.cpp:42
(Diff revision 1)
> +    float* alignedInputBuffer = ALIGNED16(inputBuffer);
> +    ASSERT_ALIGNED16(alignedInputBuffer);
> +
> +    if (aInput.IsNull()) {
> +      if (!mIIRFilters.IsEmpty()) {
> +        mIIRFilters.Clear();

IIR filters can have a tail, so this is going to cut off the end of the input.

It's a bit hard to compute a meaningful number for the length of the tail from the coefficients, but if the coefficients are zero, and the output is zero, then it's going to be silent.

::: dom/media/webaudio/IIRFilterNode.cpp:61
(Diff revision 1)
> +        RefPtr<PlayingRefChangeHandler> refchanged =
> +          new PlayingRefChangeHandler(aStream, PlayingRefChangeHandler::ADDREF);
> +        aStream->Graph()->
> +          DispatchToMainThreadAfterStreamStateUpdate(refchanged.forget());
> +      } else {
> +        NS_WARNING("IIRFilterNode channel count changes may produce audio glitches");

It's probably better to log in the developer console, but only once, there is a utility function to do that.

::: dom/media/webaudio/IIRFilterNode.cpp:128
(Diff revision 1)
> +              2,
> +              ChannelCountMode::Max,
> +              ChannelInterpretation::Speakers)
> +{
> +  mFeedforward.SetLength(aFeedforward.Length());
> +  memcpy(mFeedforward.Elements(), aFeedforward.Elements(), aFeedforward.Length()*sizeof(double));

Surely we can swap out the storage here. If we can't, we can at least use PodCopy.

::: dom/media/webaudio/IIRFilterNode.cpp:179
(Diff revision 1)
> +}
> +
> +void
> +IIRFilterNode::GetFrequencyResponse(const Float32Array& aFrequencyHz,
> +                                       const Float32Array& aMagResponse,
> +                                       const Float32Array& aPhaseResponse)

nit: indent is off.

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

https://reviewboard.mozilla.org/r/51375/#review48369

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

https://reviewboard.mozilla.org/r/51377/#review48121

::: dom/media/webaudio/test/test_iirFilterNode.html:13
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();

It would be cool to have this a web platform test.

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

https://reviewboard.mozilla.org/r/51379/#review48359

::: dom/media/webaudio/blink/Biquad.cpp:456
(Diff revision 1)
>      double a1 = m_a1;
>      double a2 = m_a2;
> -    
> +
>      for (int k = 0; k < nFrequencies; ++k) {
>          double omega = -M_PI * frequency[k];
>          Complex z = Complex(cos(omega), sin(omega));

What is the difference between `Complex` and `std::complex` ?

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

https://reviewboard.mozilla.org/r/51381/#review48373

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

https://reviewboard.mozilla.org/r/51383/#review48377

Comment 20

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/51379/#review48359

> What is the difference between `Complex` and `std::complex` ?

It's a typedef for std::complex<double> in Biquad.h. I found that confusing when I was looking at the Biquad code as std::complex is used in the IIRFilter code. I think the code would be more readable if we just removed the typedef, please let me know if you would like me to change that as part of this patch.

Comment 21

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/51375/#review48119

> Surely we can swap out the storage here. If we can't, we can at least use PodCopy.

By making a copy here we can normalize the coefficients once rather than having to keep a normalized copy inside of each IIRFilter instance.

What might be unnecessary is the copy of the coefficients made inside IIRFilterNodeEngine. I was playing it safe because I'm not familiar enough with the code yet to be sure of the object lifetimes. Please let me know if I can get away with keeping const pointers inside of IIRFilterNodeEngine rather than making a copy.

Comment 22

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/51375/#review48119

> IIR filters can have a tail, so this is going to cut off the end of the input.
> 
> It's a bit hard to compute a meaningful number for the length of the tail from the coefficients, but if the coefficients are zero, and the output is zero, then it's going to be silent.

BiquadFilterNode handles the tail at
https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/dom/media/webaudio/blink/Biquad.h#71

Also ensure that subnormals are handled, as they are almost ensured with infinite decay, as is common with these filters.
I think there might be a bug tracking subnormals.  We can probably flush to zero in the MSG iteration, if you like.  That would be sufficient if all platforms support it.

https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/dom/media/webaudio/blink/Biquad.cpp#77

Comment 23

[Paul Adenot (:padenot)]

How about we finally put an RAII denormal disabler when entering the graph, to deal with the denormals ? At least on x86 it's easy enough.

Comment 24

[Dan Minor [:dminor]]

(In reply to Paul Adenot (:padenot) from comment #23)
> How about we finally put an RAII denormal disabler when entering the graph,
> to deal with the denormals ? At least on x86 it's easy enough.

Bug 1157635 tracks dealing with denormals.

In the meantime, I'll add code to flush them to IIRFilter.cpp so we don't block on fixing Bug 1157635.

Comment 25

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Add buffersAreZero to IIRFilter;

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

Comment 26

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Avoid subnormals in IIRFilter;

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

Comment 27

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

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

Comment 28

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Add webplatform-test for IIRFilterNode;

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

Comment 29

[Dan Minor [:dminor]]

Comment on attachment 8750373 [details]
Bug 1265408 - Add webidl for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51369/diff/1-2/

Comment 30

[Dan Minor [:dminor]]

Comment on attachment 8750374 [details]
Bug 1265408 - Import IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51371/diff/1-2/

Comment 31

[Dan Minor [:dminor]]

Comment on attachment 8750375 [details]
Bug 1265408 - Use IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51373/diff/1-2/

Comment 32

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/1-2/

Comment 33

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/1-2/

Comment 34

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/1-2/

Comment 35

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/1-2/

Comment 36

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/1-2/

Comment 37

[Dan Minor [:dminor]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a24a8d33347fec83abe0ae3e92b1d8d204ff912

Comment 38

[James Graham [:jgraham]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

https://reviewboard.mozilla.org/r/52491/#review49936

Use of the testharness looks OK; I didn't check the test semantics.

::: testing/web-platform/mozilla/meta/MANIFEST.json:11
(Diff revision 1)
>      "testharness": [],
>      "wdspec": []
>    },
>    "local_changes": {
>      "deleted": [],
> +    "deleted_reftests": {},

This change should disappear when you rebase.

::: testing/web-platform/tests/webaudio/the-audio-api/the-iirfilternode-interface/test-iirfilternode.html:10
(Diff revision 1)
> +<script src=/resources/testharnessreport.js></script>
> +<script>
> +test(function(t) {
> +  var ac = new AudioContext();
> +
> +  assert_throws('NotSupportedError', function() {

Note that if any of these asserts fail the others won't be run. That's possibly OK but an alternate style here would be to do something like

```
fn check_args(arg1, arg2, err, desc) {
    test(function() {
        assert_throws(err, function() {
            ac.createIRRFilter(arg1, arg2)
        })
    }, desc)
}

check_args([], [1.0], 'NotSupportedError', 'feedforward coefficients can not be empty, first arg');

check_args([1.0], [], 'NotSupportedError', 'feedforward coefficients can not be empty, second arg');

[…]
```

Comment 39

[Dan Minor [:dminor]]

Comment on attachment 8750373 [details]
Bug 1265408 - Add webidl for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51369/diff/2-3/

Comment 40

[Dan Minor [:dminor]]

Comment on attachment 8750374 [details]
Bug 1265408 - Import IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51371/diff/2-3/

Comment 41

[Dan Minor [:dminor]]

Comment on attachment 8750375 [details]
Bug 1265408 - Use IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51373/diff/2-3/

Comment 42

[Dan Minor [:dminor]]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52485/diff/1-2/

Comment 43

[Dan Minor [:dminor]]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52487/diff/1-2/

Comment 44

[Dan Minor [:dminor]]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52489/diff/1-2/

Comment 45

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/2-3/

Comment 46

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52491/diff/1-2/

Comment 47

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/2-3/

Comment 48

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/2-3/

Comment 49

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/2-3/

Comment 50

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/2-3/

Comment 51

[Karl Tomlinson (:karlt)]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

https://reviewboard.mozilla.org/r/52485/#review51118

::: dom/media/webaudio/blink/IIRFilter.cpp:141
(Diff revision 1)
> +{
> +    double* xBuffer = m_xBuffer.Elements();
> +    double* yBuffer = m_yBuffer.Elements();
> +
> +    for (size_t k = 0; k < m_feedforward->Length(); ++k) {
> +        if (!FuzzyEqual(xBuffer[(m_bufferIndex - k) & (kBufferLength - 1)], 0.0)) {

I don't think there is any need to compare these or the feedback values with 1e-7.

Web Audio supports "High dynamic range, using 32bits floats for internal processing."
There is the possibility of a gain node making small values audible, or the small values could be used as AudioParam inputs or read from javascript.

Testing equality with zero should be fine and that avoids the risk of surprises when values cross the magic threshold.

Comparing against FLT_MIN may be reasonable for the feedback values, but I guess process() will flush them anyway.  The feedforward values shouldn't need this as they come from AudioNode input.

Another option to consider is setting a boolean member variable from process(), if that ends up checking all these values anyway.

Comment 52

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/52487/#review51124

::: dom/media/webaudio/blink/IIRFilter.cpp:98
(Diff revision 1)
> +        // Avoid introducing a stream of subnormals
> +        // TODO: Remove this code when Bug 1157635 is fixed.
> +        if (yn != 0.0 && fabs(yn) < FLT_MIN) {
> +            yn = 0.0;
> +        }

|yn| has extra precision to avoid problems with roundoff being magnified in the output.  I don't think it should be rounded to zero here at single precision in general.

When at least feedforwardLength of input values are zero and feedbackLength of output values are < FLT_MIN, then I think we can assume it is safe to reset the filter.

The main goal here is to prevent a long stream of subnormals flowing through the rest of audio graph.  The internal calculations are at double precision and so their performance is not likely to be affected by subnormals until values are much smaller.  That means there is the option of performing the check for subnormals only at the end of processing the block.

Comment 53

[Dan Minor [:dminor]]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52485/diff/2-3/

Comment 54

[Dan Minor [:dminor]]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52487/diff/2-3/

Comment 55

[Dan Minor [:dminor]]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52489/diff/2-3/

Comment 56

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/3-4/

Comment 57

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52491/diff/2-3/

Comment 58

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/3-4/

Comment 59

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/3-4/

Comment 60

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/3-4/

Comment 61

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/3-4/

Comment 62

[Karl Tomlinson (:karlt)]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

https://reviewboard.mozilla.org/r/52487/#review52076

::: dom/media/webaudio/blink/IIRFilter.cpp:88
(Diff revision 3)
> +        if (yn == 0.0) {
> +            inputZerosCount++;

Can you test sourceP[n] instead of yn here, please?

Conceivably, a feedforward coefficient may be zero, hiding a non-zero input value.

::: dom/media/webaudio/blink/IIRFilter.cpp:117
(Diff revision 3)
> +        if (inputZerosCount >= feedforwardLength && outputZeroOrSubnormalCount >= feedbackLength) {
> +            yn = 0.0;

I don't know that this is stable enough to be sure that the output values will reach zero (even when the feedback coefficients are stable).

The coefficients are often essentially calculating derivatives through
differences.  While the output is slowly approaching zero, these differences
can be slowly approaching zero, but by setting only one of the retained output
values to zero, the differences will detect a jump and AFAIK the feedback
coefficients can be such that this will make the next output value > FLT_MIN.  The output may decay towards zero again, but then the cycle can
repeat.

If all feedbackLength retained output values are flushed to zero together,
then this problem is avoided.

I expect it would be safe to individually flush any output value that is subnormal, but the values retained for the next iteration need to
be handled more carefully so as not to introduce artificial instability.

Comment 63

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/52487/#review52076

> I don't know that this is stable enough to be sure that the output values will reach zero (even when the feedback coefficients are stable).
> 
> The coefficients are often essentially calculating derivatives through
> differences.  While the output is slowly approaching zero, these differences
> can be slowly approaching zero, but by setting only one of the retained output
> values to zero, the differences will detect a jump and AFAIK the feedback
> coefficients can be such that this will make the next output value > FLT_MIN.  The output may decay towards zero again, but then the cycle can
> repeat.
> 
> If all feedbackLength retained output values are flushed to zero together,
> then this problem is avoided.
> 
> I expect it would be safe to individually flush any output value that is subnormal, but the values retained for the next iteration need to
> be handled more carefully so as not to introduce artificial instability.

That makes sense. I'm going to just flush the output values then, as that seems the simplest and safest way ahead here.

Comment 64

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/52487/#review52076

> That makes sense. I'm going to just flush the output values then, as that seems the simplest and safest way ahead here.

That's probably OK, but then buffersAreZero() needs to compare yBuffer values against FLT_MIN, because those values may never reach zero.

Comment 65

[Dan Minor [:dminor]]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52485/diff/3-4/

Comment 66

[Dan Minor [:dminor]]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52487/diff/3-4/

Comment 67

[Dan Minor [:dminor]]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52489/diff/3-4/

Comment 68

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/4-5/

Comment 69

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52491/diff/3-4/

Comment 70

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/4-5/

Comment 71

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/4-5/

Comment 72

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/4-5/

Comment 73

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/4-5/

Comment 74

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/52485/#review52782

::: dom/media/webaudio/blink/IIRFilter.cpp:104
(Diff revisions 3 - 4)
>          m_xBuffer[m_bufferIndex] = sourceP[n];
>          m_yBuffer[m_bufferIndex] = yn;
>  
>          m_bufferIndex = (m_bufferIndex + 1) & (kBufferLength - 1);
>  
> +        // Avoid introducing a stream of subnormals

Update the commit message to include this change, if you are intentionally including that change in this commit.

::: dom/media/webaudio/blink/IIRFilter.cpp:153
(Diff revisions 3 - 4)
>              return false;
>          }
>      }
>  
>      for (size_t k = 0; k < m_feedback->Length(); ++k) {
> -        if (yBuffer[(m_bufferIndex - k) & (kBufferLength - 1)] != 0.0) {
> +        if (yBuffer[(m_bufferIndex - k) & (kBufferLength - 1)] >= FLT_MIN) {

fabs(yBuffer[...])

::: dom/media/webaudio/blink/IIRFilter.cpp:6
(Diff revision 4)
> +#include "WebAudioUtils.h"
>  
>  #include <complex>
>  
> +using mozilla::dom::WebAudioUtils::FuzzyEqual;

No need for this include or the using statement now.

Comment 75

[Karl Tomlinson (:karlt)]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

https://reviewboard.mozilla.org/r/52487/#review52784

I assume you no longer intend to include this change.

Comment 76

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/52485/#review52782

> Update the commit message to include this change, if you are intentionally including that change in this commit.

Sigh, looks like I histedited the fix into the wrong commit. I'll try to fix things up.

Comment 77

[Dan Minor [:dminor]]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52485/diff/4-5/

Comment 78

[Dan Minor [:dminor]]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52487/diff/4-5/

Comment 79

[Dan Minor [:dminor]]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52489/diff/4-5/

Comment 80

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/5-6/

Comment 81

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52491/diff/4-5/

Comment 82

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/5-6/

Comment 83

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/5-6/

Comment 84

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/5-6/

Comment 85

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/5-6/

Comment 86

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/52485/#review52978

::: dom/media/webaudio/blink/IIRFilter.cpp
(Diff revision 5)
> -
> -        destP[n] = yn;

This part is in the wrong commit.

Comment 87

[Karl Tomlinson (:karlt)]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

https://reviewboard.mozilla.org/r/52487/#review52980

Comment 88

[Paul Adenot (:padenot)]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

https://reviewboard.mozilla.org/r/52491/#review53560

Comment 89

[Paul Adenot (:padenot)]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

https://reviewboard.mozilla.org/r/51377/#review53566

Comment 90

[Paul Adenot (:padenot)]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

https://reviewboard.mozilla.org/r/51379/#review53568

Comment 91

[Paul Adenot (:padenot)]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

https://reviewboard.mozilla.org/r/52489/#review53564

::: dom/media/webaudio/WebAudioUtils.cpp:97
(Diff revision 5)
>  }
>  
> +void
> +WebAudioUtils::LogToBrowserConsole(const nsAString& aMsg)
> +{
> +  // This implementation is taken from dom/media/VideoUtils.cpp

How is this different from nsContentUtils::ReportToConsole ?

Comment 92

[Paul Adenot (:padenot)]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

https://reviewboard.mozilla.org/r/51375/#review53170

::: dom/media/webaudio/IIRFilterNode.cpp:71
(Diff revision 6)
> +        RefPtr<PlayingRefChangeHandler> refchanged =
> +          new PlayingRefChangeHandler(aStream, PlayingRefChangeHandler::ADDREF);
> +        aStream->Graph()->
> +          DispatchToMainThreadAfterStreamStateUpdate(refchanged.forget());
> +      } else {
> +        WebAudioUtils::LogToBrowserConsole(NS_LITERAL_STRING("IIRFilterNode channel count changes may produce audio glitches"));

Can we do this in a way that the string is localizable ?

::: dom/media/webaudio/IIRFilterNode.cpp:145
(Diff revision 6)
> +  PodCopy(mFeedback.Elements(), aFeedback.Elements(), aFeedback.Length());
> +
> +  // Scale coefficients -- we guarantee that mFeedback != 0 when creating
> +  // the IIRFilterNode.
> +  double scale = mFeedback[0];
> +  double *elements = mFeedforward.Elements();

nit: * near the type.

::: dom/media/webaudio/IIRFilterNode.cpp:187
(Diff revision 6)
> +{
> +  return IIRFilterNodeBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +IIRFilterNode::GetFrequencyResponse(const Float32Array& aFrequencyHz,

nit: indent is a bit off here.

Comment 93

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/52489/#review53564

> How is this different from nsContentUtils::ReportToConsole ?

nsContentUtils::ReportToConsole only works on the main thread, while this handles both on and off main thread logging. Potentially we could use the same trick of dispatching to the main thread to call ReportToConsole, which would mean our string could be localized. I'll investigate.

Comment 94

[Dan Minor [:dminor]]

Attachment: Bug 1265408 - Log biquad channel change warning to console;

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

Comment 95

[Dan Minor [:dminor]]

Comment on attachment 8750373 [details]
Bug 1265408 - Add webidl for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51369/diff/3-4/

Comment 96

[Dan Minor [:dminor]]

Comment on attachment 8750374 [details]
Bug 1265408 - Import IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51371/diff/3-4/

Comment 97

[Dan Minor [:dminor]]

Comment on attachment 8750375 [details]
Bug 1265408 - Use IIRFilter from blink;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51373/diff/3-4/

Comment 98

[Dan Minor [:dminor]]

Comment on attachment 8752254 [details]
Bug 1265408 - Add buffersAreZero to IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52485/diff/5-6/

Comment 99

[Dan Minor [:dminor]]

Comment on attachment 8752255 [details]
Bug 1265408 - Avoid subnormals in IIRFilter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52487/diff/5-6/

Comment 100

[Dan Minor [:dminor]]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52489/diff/5-6/

Comment 101

[Dan Minor [:dminor]]

Comment on attachment 8750376 [details]
Bug 1265408 - Implement IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51375/diff/6-7/

Comment 102

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52491/diff/5-6/

Comment 103

[Dan Minor [:dminor]]

Comment on attachment 8750377 [details]
Bug 1265408 - Add test for IIRFilterNode pass through;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51377/diff/6-7/

Comment 104

[Dan Minor [:dminor]]

Comment on attachment 8750378 [details]
Bug 1265408 - Avoid complex division in getFrequencyResponse;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51379/diff/6-7/

Comment 105

[Dan Minor [:dminor]]

Comment on attachment 8750379 [details]
Bug 1265408 - Import blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51381/diff/6-7/

Comment 106

[Dan Minor [:dminor]]

Comment on attachment 8750380 [details]
Bug 1265408 - Use blink IIRFilterNode tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51383/diff/6-7/

Comment 107

[Paul Adenot (:padenot)]

Comment on attachment 8759775 [details]
Bug 1265408 - Log biquad channel change warning to console;

https://reviewboard.mozilla.org/r/57644/#review54688

Comment 108

[Paul Adenot (:padenot)]

Comment on attachment 8752256 [details]
Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;

https://reviewboard.mozilla.org/r/52489/#review54690

Comment 109

[Dan Minor [:dminor]]

Comment on attachment 8752257 [details]
Bug 1265408 - Add webplatform-test for IIRFilterNode;

Sorry, I didn't intend to ask for another review on this commit, maybe something went strange with the MozReview -> Bugzilla integration.

Comment 110

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fab8755ac9
Add webidl for IIRFilterNode; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d59146a43fe
Import IIRFilter from blink; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/388a95b05202
Use IIRFilter from blink; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab327850c5f
Add buffersAreZero to IIRFilter; r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/784521a9cc94
Avoid subnormals in IIRFilter; r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0ec5cf4d30
Add LogToDeveloperConsole to WebAudioUtils; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd3f7f5a431
Implement IIRFilterNode; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d602e3a9e6
Add webplatform-test for IIRFilterNode; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d271e05ae2
Add test for IIRFilterNode pass through; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd3a54d766d
Avoid complex division in getFrequencyResponse; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d5bbd33e0e
Import blink IIRFilterNode tests; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/465412cecc51
Use blink IIRFilterNode tests; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa770304f2a
Log biquad channel change warning to console; r=padenot

Comment 111

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

Backed out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=29590791&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9870ee9198c8

Comment 112

[Dan Minor [:dminor]]

Attachment: Add IIRFilterNode to test_interfaces.html

I'll roll this up with the commit that adds the IIRFilterNode webidl prior to landing, but as far as I know, MozReview still doesn't handle requesting a new review gracefully so I thought it best to upload it as a separate patch.

Try run here (hopefully there are no other traps for the unwary): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c849a6bef3fe

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c6e7ba321a
Add webidl for IIRFilterNode; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/679f42969fed
Import IIRFilter from blink; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/08ed713fd7bf
Use IIRFilter from blink; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/d73d8db78774
Add buffersAreZero to IIRFilter; r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2d66845bba
Avoid subnormals in IIRFilter; r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/814c1a3fd2e1
Add LogToDeveloperConsole to WebAudioUtils; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/78eb3b1b93ce
Implement IIRFilterNode; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ac527f4d47
Add webplatform-test for IIRFilterNode; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/555f1884bd94
Add test for IIRFilterNode pass through; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae75b48fdcc
Avoid complex division in getFrequencyResponse; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/49a685db1535
Import blink IIRFilterNode tests; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/492d7b059416
Use blink IIRFilterNode tests; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/021ffc7c02e6
Log biquad channel change warning to console; r=padenot

Comment 114

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2c6e7ba321a
https://hg.mozilla.org/mozilla-central/rev/679f42969fed
https://hg.mozilla.org/mozilla-central/rev/08ed713fd7bf
https://hg.mozilla.org/mozilla-central/rev/d73d8db78774
https://hg.mozilla.org/mozilla-central/rev/4d2d66845bba
https://hg.mozilla.org/mozilla-central/rev/814c1a3fd2e1
https://hg.mozilla.org/mozilla-central/rev/78eb3b1b93ce
https://hg.mozilla.org/mozilla-central/rev/e8ac527f4d47
https://hg.mozilla.org/mozilla-central/rev/555f1884bd94
https://hg.mozilla.org/mozilla-central/rev/8ae75b48fdcc
https://hg.mozilla.org/mozilla-central/rev/49a685db1535
https://hg.mozilla.org/mozilla-central/rev/492d7b059416
https://hg.mozilla.org/mozilla-central/rev/021ffc7c02e6

Comment 115

[Eric Shepherd [:sheppy]]

Much of this was already documented by JP for Chrome; I’ve reviewed and updated the content.

This page has been added:

https://developer.mozilla.org/en-US/docs/Web/API/IIRFilterNode/getFrequencyResponse

This page has been updated heavily:

https://developer.mozilla.org/en-US/docs/Web/API/IIRFilterNode

While at it, I did some little stuff on these:

https://developer.mozilla.org/en-US/docs/Web/API/BiquadFilterNode
https://developer.mozilla.org/en-US/docs/Web/API/BiquadFilterNode/getFrequencyResponse

Finally, this has been added to https://developer.mozilla.org/en-US/Firefox/Releases/50.

Comment 116

[Eric Shepherd [:sheppy]]

Forgot to mention that I also updated this:

https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/createIIRFilter

Comment 117

[Boris Zbarsky [:bzbarsky]]

Fwiw, this bit:

  CreateIIRFilter(const mozilla::dom::binding_detail::AutoSequence<double>& aFeedforward,

should just be:

  CreateIIRFilter(const Sequence<double>& aFeedforward,

and similar for the other uses of binding_detail in these patches.  In general, binding_detail types should not be used outside binding code.

Comment 118

[Dan Minor [:dminor]]

Thanks for letting me know. I filed bug 1328144 to clean this up.