Skip to content

GitLab

T
tangled
Commit 74de50be authored by Raymond Toy's avatar Raymond Toy Committed by Commit Bot
Browse files
Options

Handle real root case better in Biquad tailtime computation 

When computing the tail time for a BiquadFilterNode, we forgot to take
the absolute value of the largest root when computing the tail.  For
the test case from the bug, the root with the larger magnitude was
negative.

Also, the computation of the larger root was incorrect; fix that.  And
the comments for the real root case was incorrect and wrote "r" when
it should have been "r1".

Manually verified that the test file causes a DCHECK failure in a
debug build without this CL.

Bug: 829349
Test: webaudio/BiquadFilter/biquad-829349.html
Change-Id: I028331670a61da22a6f42bb7213869dc4cacf8ad
Reviewed-on: https://chromium-review.googlesource.com/998518
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Reviewed-by: default avatarHongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549195}
parent 186074b9
Hide whitespace changes
Inline Side-by-side
Showing with 62 additions and 13 deletions
third_party/WebKit/LayoutTests/webaudio/BiquadFilter/biquad-829349.html 0 → 100644
View file @ 74de50be
<!doctype html>
<html>
<head>
<title>
Biquad Tailtime Computation (crbug.com/829349)
</title>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="../resources/audit-util.js"></script>
<script src="../resources/audit.js"></script>
</head>
<body>
<script>
let audit = Audit.createTaskRunner();
// Sample rate used in testing crbug.com/829349. Use the same rate to
// make sure the coefficients have the same values. (The actual
// coefficients depend on the sample rate, so keep it the same for
// consistency with the repro case.)
let sampleRate = 44100;
audit.define('Peaking filter tailtime', (task, should) => {
// Short length
let context =
new OfflineAudioContext({length: 1024, sampleRate: sampleRate});
let src = new OscillatorNode(context);
// This particular filter is one of the filters used in the repro case
// from crbug.com/829349. In a debug build, this causes a DCHECK crash
// because a value as NaN. Just use this filter and verify that it
// works and doesn't crash in a debug build.
let f = new BiquadFilterNode(
context,
{type: 'peaking', frequency: 15000, Q: 0.2, gain: -5, detune: 0});
src.connect(f).connect(context.destination);
src.start();
context.startRendering()
.then(audioBuffer => {
// Just verify that the output is not all zeroes. The actual test
// is that we don't have a DCHECK failure in a debug build.
should(audioBuffer.getChannelData(0), 'Filter output')
.notBeConstantValueOf(0);
})
.then(() => task.done());
});
audit.run();
</script>
</body>
</html>
third_party/blink/renderer/platform/audio/biquad.cc
View file @ 74de50be
......@@ -719,14 +719,14 @@ double Biquad::TailFrame(int coef_index, double max_frame) {
//
// so
//
// |h(n)| = |C1|*|r|^(n-1)*|1+C2/C1*(r2/r1)^(n-1)|
// <= |C1|*|r|^(n-1)*[1 + |C2/C1|*|r2/r1|^(n-1)]
// <= |C1|*|r|^(n-1)*[1 + |C2/C1|]
// |h(n)| = |C1|*|r1|^(n-1)*|1+C2/C1*(r2/r1)^(n-1)|
// <= |C1|*|r1|^(n-1)*[1 + |C2/C1|*|r2/r1|^(n-1)]
// <= |C1|*|r1|^(n-1)*[1 + |C2/C1|]
//
// by using the triangle inequality and the fact that |r2|<=|r1|.
// And we want |h(n)|<=eps which is true if
//
// |C1|*|r|^(n-1)*[1 + |C2/C1|] <= eps
// |C1|*|r1|^(n-1)*[1 + |C2/C1|] <= eps
//
// or
//
......@@ -800,14 +800,11 @@ double Biquad::TailFrame(int coef_index, double max_frame) {
if (discrim > 0) {
// Compute the real roots so that r1 has the largest magnitude.
double r1;
double r2;
if (a1 < 0) {
r1 = (-a1 + sqrt(discrim)) / 2;
} else {
r1 = (-a1 - sqrt(discrim)) / 2;
}
r2 = a2 / r1;
double rplus = (-a1 + sqrt(discrim)) / 2;
double rminus = (-a1 - sqrt(discrim)) / 2;
double r1 = fabs(rplus) >= fabs(rminus) ? rplus : rminus;
// Use the fact that a2 = r1*r2
double r2 = a2 / r1;
double c1 = (b0 * r1 * r1 + b1 * r1 + b2) / (r2 - r1);
double c2 = (b0 * r2 * r2 + b1 * r2 + b2) / (r2 - r1);
......@@ -821,7 +818,7 @@ double Biquad::TailFrame(int coef_index, double max_frame) {
// This may produce a negative tail frame. Just clamp the tail
// frame to 0.
tail_frame = clampTo(
1 + log(kMaxTailAmplitude / (fabs(c1) + fabs(c2))) / log(r1), 0);
1 + log(kMaxTailAmplitude / (fabs(c1) + fabs(c2))) / log(fabs(r1)), 0);
DCHECK(std::isfinite(tail_frame));
} else if (discrim < 0) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment