Implement IIRFilterNode

RESOLVED FIXED in Firefox 50

Status

()

Core
Web Audio
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: padenot, Assigned: dminor)

Tracking

({dev-doc-complete})

Trunk
mozilla50
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(14 attachments)

58 bytes, text/x-review-board-request
smaug
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
karlt
: review+
Details | Review
58 bytes, text/x-review-board-request
karlt
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
58 bytes, text/x-review-board-request
padenot
: review+
Details | Review
1.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
This adds a new IIRFilterNode that implements a general IIR filter.

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

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → dminor
Keywords: dev-doc-needed
Intent to implement and ship discussion:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/XoJ7Ikj3Www
(Assignee)

Comment 2

2 years ago
Created attachment 8750373 [details]
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/
Attachment #8750373 - Flags: review?(bugs)
Attachment #8750374 - Flags: review?(padenot)
Attachment #8750375 - Flags: review?(padenot)
Attachment #8750376 - Flags: review?(padenot)
Attachment #8750377 - Flags: review?(padenot)
Attachment #8750378 - Flags: review?(padenot)
Attachment #8750379 - Flags: review?(padenot)
Attachment #8750380 - Flags: review?(padenot)
(Assignee)

Comment 3

2 years ago
Created attachment 8750374 [details]
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/
(Assignee)

Comment 6

2 years ago
Created attachment 8750377 [details]
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/
(Assignee)

Comment 7

2 years ago
Created attachment 8750378 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8750379 [details]
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/
(Assignee)

Comment 9

2 years ago
Created attachment 8750380 [details]
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/
(Reporter)

Comment 11

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

https://reviewboard.mozilla.org/r/51371/#review48363
Attachment #8750374 - Flags: review?(padenot) → review+
(Reporter)

Comment 12

2 years ago
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.
Attachment #8750375 - Flags: review?(padenot) → review+
(Reporter)

Comment 13

2 years ago
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.
(Reporter)

Comment 14

2 years ago
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.
Attachment #8750376 - Flags: review?(padenot) → review+
(Reporter)

Updated

2 years ago
Attachment #8750376 - Flags: review+
(Reporter)

Comment 16

2 years ago
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.
Attachment #8750377 - Flags: review?(padenot)
(Reporter)

Updated

2 years ago
Attachment #8750378 - Flags: review?(padenot)
(Reporter)

Comment 17

2 years ago
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` ?
(Reporter)

Comment 18

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

https://reviewboard.mozilla.org/r/51381/#review48373
Attachment #8750379 - Flags: review?(padenot) → review+
(Reporter)

Comment 19

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

https://reviewboard.mozilla.org/r/51383/#review48377
Attachment #8750380 - Flags: review?(padenot) → review+
(Assignee)

Comment 20

2 years ago
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.
(Assignee)

Comment 21

2 years ago
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.
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
(Reporter)

Comment 23

2 years ago
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.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 24

2 years ago
(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.
(Assignee)

Comment 25

2 years ago
Created attachment 8752254 [details]
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/
Attachment #8750373 - Attachment description: MozReview Request: Bug 1265408 - Add webidl for IIRFilterNode; r?smaug → MozReview Request: Bug 1265408 - Add webidl for IIRFilterNode; r=smaug
Attachment #8750374 - Attachment description: MozReview Request: Bug 1265408 - Import IIRFilter from blink; r?padenot → MozReview Request: Bug 1265408 - Import IIRFilter from blink; r=padenot
Attachment #8750375 - Attachment description: MozReview Request: Bug 1265408 - Use IIRFilter from blink; r?padenot → MozReview Request: Bug 1265408 - Use IIRFilter from blink; r=padenot
Attachment #8750376 - Attachment description: MozReview Request: Bug 1265408 - Implement IIRFilterNode; r?padenot → MozReview Request: Bug 1265408 - Implement IIRFilterNode; r?karlt
Attachment #8750377 - Attachment description: MozReview Request: Bug 1265408 - Add tests for IIRFilterNode; r?padenot → MozReview Request: Bug 1265408 - Add test for IIRFilterNode pass through; r?karlt
Attachment #8750378 - Attachment description: MozReview Request: Bug 1265408 - Avoid complex division in getFrequencyResponse; r?padenot → MozReview Request: Bug 1265408 - Avoid complex division in getFrequencyResponse; r?karlt
Attachment #8750379 - Attachment description: MozReview Request: Bug 1265408 - Import blink IIRFilterNode tests; r?padenot → MozReview Request: Bug 1265408 - Import blink IIRFilterNode tests; r=padenot
Attachment #8750380 - Attachment description: MozReview Request: Bug 1265408 - Use blink IIRFilterNode tests; r?padenot → MozReview Request: Bug 1265408 - Use blink IIRFilterNode tests; r=padenot
Attachment #8752254 - Flags: review?(karlt)
Attachment #8752255 - Flags: review?(karlt)
Attachment #8752256 - Flags: review?(karlt)
Attachment #8752257 - Flags: review?(karlt)
Attachment #8752257 - Flags: review?(james)
Attachment #8750376 - Flags: review?(karlt)
Attachment #8750377 - Flags: review?(karlt)
Attachment #8750378 - Flags: review?(karlt)
(Assignee)

Comment 27

2 years ago
Created attachment 8752256 [details]
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/
(Assignee)

Comment 28

2 years ago
Created attachment 8752257 [details]
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/
(Assignee)

Comment 29

2 years ago
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/
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Comment 32

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

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

Comment 33

2 years ago
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/
(Assignee)

Comment 34

2 years ago
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/
(Assignee)

Comment 35

2 years ago
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/
(Assignee)

Comment 36

2 years ago
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 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');

[…]
```
Attachment #8752257 - Flags: review?(james)
(Assignee)

Comment 39

2 years ago
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/
Attachment #8752254 - Attachment description: MozReview Request: Bug 1265408 - Add buffersAreZero to IIRFilter; r?karlt → MozReview Request: Bug 1265408 - Add buffersAreZero to IIRFilter; r?padenot
Attachment #8752255 - Attachment description: MozReview Request: Bug 1265408 - Avoid subnormals in IIRFilter; r?karlt → MozReview Request: Bug 1265408 - Avoid subnormals in IIRFilter; r?padenot
Attachment #8752256 - Attachment description: MozReview Request: Bug 1265408 - Add LogToBrowserConsole to WebAudioUtils; r?karlt → MozReview Request: Bug 1265408 - Add LogToBrowserConsole to WebAudioUtils; r?padenot
Attachment #8750376 - Attachment description: MozReview Request: Bug 1265408 - Implement IIRFilterNode; r?karlt → MozReview Request: Bug 1265408 - Implement IIRFilterNode; r?padenot
Attachment #8752257 - Attachment description: MozReview Request: Bug 1265408 - Add webplatform-test for IIRFilterNode; r?karlt,jgraham → MozReview Request: Bug 1265408 - Add webplatform-test for IIRFilterNode; r?padenot
Attachment #8750377 - Attachment description: MozReview Request: Bug 1265408 - Add test for IIRFilterNode pass through; r?karlt → MozReview Request: Bug 1265408 - Add test for IIRFilterNode pass through; r?padenot
Attachment #8750378 - Attachment description: MozReview Request: Bug 1265408 - Avoid complex division in getFrequencyResponse; r?karlt → MozReview Request: Bug 1265408 - Avoid complex division in getFrequencyResponse; r?padenot
Attachment #8752254 - Flags: review?(karlt) → review?(padenot)
Attachment #8752255 - Flags: review?(karlt) → review?(padenot)
Attachment #8752256 - Flags: review?(karlt) → review?(padenot)
Attachment #8750376 - Flags: review?(karlt) → review?(padenot)
Attachment #8752257 - Flags: review?(karlt) → review?(padenot)
Attachment #8750377 - Flags: review?(karlt) → review?(padenot)
Attachment #8750378 - Flags: review?(karlt) → review?(padenot)
(Assignee)

Comment 40

2 years ago
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/
(Assignee)

Comment 41

2 years ago
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/
(Assignee)

Comment 42

2 years ago
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/
(Assignee)

Comment 43

2 years ago
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/
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Comment 45

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

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

Comment 46

2 years ago
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/
(Assignee)

Comment 47

2 years ago
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/
(Assignee)

Comment 48

2 years ago
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/
(Assignee)

Comment 49

2 years ago
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/
(Assignee)

Comment 50

2 years ago
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 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.
Attachment #8752254 - Flags: review+
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.
(Assignee)

Comment 53

2 years ago
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/
Attachment #8752254 - Attachment description: MozReview Request: Bug 1265408 - Add buffersAreZero to IIRFilter; r?padenot → MozReview Request: Bug 1265408 - Add buffersAreZero to IIRFilter; r=karlt
Attachment #8752255 - Attachment description: MozReview Request: Bug 1265408 - Avoid subnormals in IIRFilter; r?padenot → MozReview Request: Bug 1265408 - Avoid subnormals in IIRFilter; r?karlt
Attachment #8752254 - Flags: review?(padenot)
Attachment #8752255 - Flags: review?(padenot) → review?(karlt)
(Assignee)

Comment 54

2 years ago
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/
(Assignee)

Comment 55

2 years ago
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/
(Assignee)

Comment 56

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

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

Comment 57

2 years ago
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/
(Assignee)

Comment 58

2 years ago
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/
(Assignee)

Comment 59

2 years ago
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/
(Assignee)

Comment 60

2 years ago
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/
(Assignee)

Comment 61

2 years ago
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 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.
Attachment #8752255 - Flags: review?(karlt)
(Assignee)

Comment 63

2 years ago
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.
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.
(Assignee)

Comment 65

2 years ago
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/
Attachment #8752255 - Flags: review?(karlt)
(Assignee)

Comment 66

2 years ago
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/
(Assignee)

Comment 67

2 years ago
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/
(Assignee)

Comment 68

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

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

Comment 69

2 years ago
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/
(Assignee)

Comment 70

2 years ago
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/
(Assignee)

Comment 71

2 years ago
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/
(Assignee)

Comment 72

2 years ago
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/
(Assignee)

Comment 73

2 years ago
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/
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 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.
Attachment #8752255 - Flags: review?(karlt)
(Assignee)

Comment 76

2 years ago
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.
(Assignee)

Comment 77

2 years ago
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/
Attachment #8752255 - Flags: review?(karlt)
(Assignee)

Comment 78

2 years ago
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/
(Assignee)

Comment 79

2 years ago
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/
(Assignee)

Comment 80

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

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

Comment 81

2 years ago
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/
(Assignee)

Comment 82

2 years ago
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/
(Assignee)

Comment 83

2 years ago
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/
(Assignee)

Comment 84

2 years ago
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/
(Assignee)

Comment 85

2 years ago
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/
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.
Attachment #8752255 - Flags: review?(karlt) → review+
(Reporter)

Comment 88

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

https://reviewboard.mozilla.org/r/52491/#review53560
Attachment #8752257 - Flags: review?(padenot) → review+
(Reporter)

Comment 89

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

https://reviewboard.mozilla.org/r/51377/#review53566
Attachment #8750377 - Flags: review?(padenot) → review+
(Reporter)

Comment 90

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

https://reviewboard.mozilla.org/r/51379/#review53568
Attachment #8750378 - Flags: review?(padenot) → review+
(Reporter)

Comment 91

2 years ago
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 ?
Attachment #8752256 - Flags: review?(padenot)
(Reporter)

Comment 92

2 years ago
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.
Attachment #8750376 - Flags: review?(padenot) → review+
(Assignee)

Comment 93

2 years ago
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.
(Assignee)

Comment 94

2 years ago
Created attachment 8759775 [details]
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/
Attachment #8750373 - Attachment description: MozReview Request: Bug 1265408 - Add webidl for IIRFilterNode; r=smaug → Bug 1265408 - Add webidl for IIRFilterNode;
Attachment #8750374 - Attachment description: MozReview Request: Bug 1265408 - Import IIRFilter from blink; r=padenot → Bug 1265408 - Import IIRFilter from blink;
Attachment #8750375 - Attachment description: MozReview Request: Bug 1265408 - Use IIRFilter from blink; r=padenot → Bug 1265408 - Use IIRFilter from blink;
Attachment #8752254 - Attachment description: MozReview Request: Bug 1265408 - Add buffersAreZero to IIRFilter; r=karlt → Bug 1265408 - Add buffersAreZero to IIRFilter;
Attachment #8752255 - Attachment description: MozReview Request: Bug 1265408 - Avoid subnormals in IIRFilter; r?karlt → Bug 1265408 - Avoid subnormals in IIRFilter;
Attachment #8752256 - Attachment description: MozReview Request: Bug 1265408 - Add LogToBrowserConsole to WebAudioUtils; r?padenot → Bug 1265408 - Add LogToDeveloperConsole to WebAudioUtils;
Attachment #8750376 - Attachment description: MozReview Request: Bug 1265408 - Implement IIRFilterNode; r?padenot → Bug 1265408 - Implement IIRFilterNode;
Attachment #8752257 - Attachment description: MozReview Request: Bug 1265408 - Add webplatform-test for IIRFilterNode; r?padenot → Bug 1265408 - Add webplatform-test for IIRFilterNode;
Attachment #8750377 - Attachment description: MozReview Request: Bug 1265408 - Add test for IIRFilterNode pass through; r?padenot → Bug 1265408 - Add test for IIRFilterNode pass through;
Attachment #8750378 - Attachment description: MozReview Request: Bug 1265408 - Avoid complex division in getFrequencyResponse; r?padenot → Bug 1265408 - Avoid complex division in getFrequencyResponse;
Attachment #8750379 - Attachment description: MozReview Request: Bug 1265408 - Import blink IIRFilterNode tests; r=padenot → Bug 1265408 - Import blink IIRFilterNode tests;
Attachment #8750380 - Attachment description: MozReview Request: Bug 1265408 - Use blink IIRFilterNode tests; r=padenot → Bug 1265408 - Use blink IIRFilterNode tests;
Attachment #8759775 - Flags: review?(padenot)
Attachment #8752256 - Flags: review?(padenot)
Attachment #8752257 - Flags: review?(james)
(Assignee)

Comment 95

2 years ago
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/
(Assignee)

Comment 96

2 years ago
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/
(Assignee)

Comment 97

2 years ago
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/
(Assignee)

Comment 98

2 years ago
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/
(Assignee)

Comment 99

2 years ago
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/
(Assignee)

Comment 100

2 years ago
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/
(Assignee)

Comment 101

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

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

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
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/
(Assignee)

Comment 104

2 years ago
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/
(Assignee)

Comment 105

2 years ago
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/
(Assignee)

Comment 106

2 years ago
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/
(Reporter)

Comment 107

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

https://reviewboard.mozilla.org/r/57644/#review54688
Attachment #8759775 - Flags: review?(padenot) → review+
(Reporter)

Comment 108

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

https://reviewboard.mozilla.org/r/52489/#review54690
Attachment #8752256 - Flags: review?(padenot) → review+
(Assignee)

Comment 109

2 years ago
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.
Attachment #8752257 - Flags: review?(james)

Comment 110

2 years ago
Pushed by dminor@mozilla.com:
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
(Assignee)

Comment 112

2 years ago
Created attachment 8760663 [details] [diff] [review]
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
Flags: needinfo?(dminor)
Attachment #8760663 - Flags: review?(bugs)
Attachment #8760663 - Flags: review?(bugs) → review+

Comment 113

2 years ago
Pushed by dminor@mozilla.com:
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
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.
Keywords: dev-doc-needed → dev-doc-complete
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.
Flags: needinfo?(dminor)
(Assignee)

Comment 118

2 years ago
Thanks for letting me know. I filed bug 1328144 to clean this up.
Depends on: 1328144
Flags: needinfo?(dminor)
You need to log in before you can comment on or make changes to this bug.