Compute the tail time for an IIRFilter from its coefficients

Compute the tail time for an IIRFilter from its coefficients

For an IIRFilterNode the tail time is currently set to infinity, which is
mathmatically true, but impractical because that keeps the IIRFilter
alive even if the output is very small.

Instead, compute a tail time based on the coefficients of the IIR
filter. The tail time is the estimated time where the impulse response
is sufficiently small that we can say that output is essentially
zero.

By doing so, an IIRFilterNode can disconnect itself from downstreams
nodes, reducing complexity of the graph.  Without this, downstream
nodes stay alive processing silence.

BUG=612215
TEST=iirfilter.html

Review-Url: https://codereview.chromium.org/2851873003
Cr-Commit-Position: refs/heads/master@{#472845}
Committed: https://chromium.googlesource.com/chromium/src/+/7615be681fe875bc5e6e5715e1455bbffa0a5eb3

[Raymond Toy, 2017-05-12 16:06:53]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2017-05-12 16:06:53]

PTAL

[hongchan, 2017-05-12 16:24:05]

Mostly nits and 1 open question.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/IIRDSPKernel.cpp (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/IIRDSPKernel.cpp:14: tail_time_ =
iir_.TailTime(processor->SampleRate());
Is this the only moment that the tail time is computed?

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/IIRFilter.cpp (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:149: const double
kMaxTailTime = 60;
Let's pull these constants out of this function - probably move them to the top
of this file.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:154: const float
kMaxTailAmplitude = 1 / 32768.0;
Ditto.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:202: for (int k =
number_of_blocks - 1; k >= 0; --k) {
|number_of_blocks -1| can be |index| here.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:214: return time;
We don't need |time| here. Right?

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/IIRFilter.h (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.h:34: double TailTime(double
sample_rate);
What's the frequency of this computation?

On a related note, I am aware that IIRFilter does not share its infra with any
other DSP kernel or platform components. Should we actually consider doing that?

It seems like TailTime() and ResetState() methods can be shared with other
similar components.

We can fix them later, but I think we should at least consider.

[Raymond Toy, 2017-05-12 17:33:18]

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/IIRDSPKernel.cpp (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/IIRDSPKernel.cpp:14: tail_time_ =
iir_.TailTime(processor->SampleRate());
On 2017/05/12 16:24:04, hongchan wrote:
> Is this the only moment that the tail time is computed?

Since the filter can't change, we only need to compute the tail time once during
construction of the filter.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/IIRFilter.h (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.h:34: double TailTime(double
sample_rate);
On 2017/05/12 16:24:05, hongchan wrote:
> What's the frequency of this computation?

I don't quite understand this question?  The TailTime is always requested every
render quantum. For the IIR filter, the value is computed once during
construction and this just returns that value.
> 
> On a related note, I am aware that IIRFilter does not share its infra with any
> other DSP kernel or platform components. Should we actually consider doing
that?

I don't understand what is being shared.

> 
> It seems like TailTime() and ResetState() methods can be shared with other
> similar components.

I don't understand this either.  The TailTime() value depends on the node.
> 
> We can fix them later, but I think we should at least consider.

[hongchan, 2017-05-12 17:36:45]

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/IIRFilter.h (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.h:34: double TailTime(double
sample_rate);
On 2017/05/12 17:33:18, Raymond Toy wrote:
> On 2017/05/12 16:24:05, hongchan wrote:
> > What's the frequency of this computation?
> 
> I don't quite understand this question?  The TailTime is always requested
every
> render quantum. For the IIR filter, the value is computed once during
> construction and this just returns that value.

You answered this above. We compute just once.

> > 
> > On a related note, I am aware that IIRFilter does not share its infra with
any
> > other DSP kernel or platform components. Should we actually consider doing
> that?
> 
> I don't understand what is being shared.
> 
> > 
> > It seems like TailTime() and ResetState() methods can be shared with other
> > similar components.
> 
> I don't understand this either.  The TailTime() value depends on the node.
> > 
> > We can fix them later, but I think we should at least consider.
> 

I just looked at the header file. Two methods are shared with other friend
classes. Ignore my comments.

[Raymond Toy, 2017-05-12 19:42:51]

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/IIRFilter.cpp (right):

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:149: const double
kMaxTailTime = 60;
On 2017/05/12 16:24:05, hongchan wrote:
> Let's pull these constants out of this function - probably move them to the
top
> of this file.

But they're not relevant to any other part of the code.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:202: for (int k =
number_of_blocks - 1; k >= 0; --k) {
On 2017/05/12 16:24:04, hongchan wrote:
> |number_of_blocks -1| can be |index| here.

Done.

https://codereview.chromium.org/2851873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/IIRFilter.cpp:214: return time;
On 2017/05/12 16:24:05, hongchan wrote:
> We don't need |time| here. Right?

Done.

[hongchan, 2017-05-16 22:30:02]

lgtm

[Raymond Toy, 2017-05-17 15:32:44]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-05-17 15:33:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:15:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 17:15:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raymond Toy, 2017-05-17 19:24:53]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-05-17 19:26:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 21:40:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 21:40:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raymond Toy, 2017-05-17 22:17:42]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2017-05-17 22:17:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2851873003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-17 22:19:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 00:38:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 00:38:43]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Raymond Toy, 2017-05-18 15:13:29]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-05-18 15:14:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 16:51:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495120409491820,
"parent_rev": "339e6991a01df07f62d2e91ef220c2139d323d04", "commit_rev":
"7615be681fe875bc5e6e5715e1455bbffa0a5eb3"}

[commit-bot: I haz the power, 2017-05-18 16:51:11]

Description was changed from

==========
Compute the tail time for an IIRFilter from its coefficients

For an IIRFilterNode the tail time is currently set to infinity, which is
mathmatically true, but impractical because that keeps the IIRFilter
alive even if the output is very small.

Instead, compute a tail time based on the coefficients of the IIR
filter. The tail time is the estimated time where the impulse response
is sufficiently small that we can say that output is essentially
zero.

By doing so, an IIRFilterNode can disconnect itself from downstreams
nodes, reducing complexity of the graph.  Without this, downstream
nodes stay alive processing silence.

BUG=612215
TEST=iirfilter.html
==========

to

==========
Compute the tail time for an IIRFilter from its coefficients

For an IIRFilterNode the tail time is currently set to infinity, which is
mathmatically true, but impractical because that keeps the IIRFilter
alive even if the output is very small.

Instead, compute a tail time based on the coefficients of the IIR
filter. The tail time is the estimated time where the impulse response
is sufficiently small that we can say that output is essentially
zero.

By doing so, an IIRFilterNode can disconnect itself from downstreams
nodes, reducing complexity of the graph.  Without this, downstream
nodes stay alive processing silence.

BUG=612215
TEST=iirfilter.html

Review-Url: https://codereview.chromium.org/2851873003
Cr-Commit-Position: refs/heads/master@{#472845}
Committed:
https://chromium.googlesource.com/chromium/src/+/7615be681fe875bc5e6e5715e145...
==========

[commit-bot: I haz the power, 2017-05-18 16:51:12]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/7615be681fe875bc5e6e5715e145...