Bug 44795 - audio engine: add Biquad files
Summary: audio engine: add Biquad files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-27 14:31 PDT by Chris Rogers
Modified: 2010-09-09 17:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.91 KB, patch)
2010-08-27 14:32 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2010-09-08 16:59 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-08-27 14:31:35 PDT
audio engine: add Biquad files
Comment 1 Chris Rogers 2010-08-27 14:32:46 PDT
Created attachment 65768 [details]
Patch
Comment 2 Kenneth Russell 2010-09-07 19:18:53 PDT
Comment on attachment 65768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=65768&action=prettypatch

I trust that the math is correct and is in some generally standard form for signal processing which is why the equations are in certain forms with certain variable names. I have one main question, which we've discussed offline, about a double negative which might imply changing where some negation occurs throughout the code. Also a couple of minor formatting issues. Will wait for your feedback before updating the review status of the patch.

> WebCore/platform/audio/Biquad.cpp:69
> +
Unnecessary newline

> WebCore/platform/audio/Biquad.cpp:165
> +    destP[1] = destP[framesToProcess - 1 + 2];
These are subtle since they would look like out of bounds accesses on arrays simply of length framesToProcess. Could you add a comment referencing the allocation of two extra samples for the input and output buffers in the constructor?

> WebCore/platform/audio/Biquad.cpp:190
> +        resonance = 0.0; // for now can't go negative
resonance = std::max(0.0, resonance);

> WebCore/platform/audio/Biquad.cpp:212
> +        resonance = 0.0; // for now can't go negative
Same std::max as above.

> WebCore/platform/audio/Biquad.cpp:266
> +    m_b1 = - (-2.0 * pole.real());
Why do you have a double negative here? Why not just write "2.0 * pole.real()"?

> WebCore/platform/audio/Biquad.cpp:269
> +    m_b2 = - (poleMag * poleMag);
Extra space between - and (

> WebCore/platform/audio/Biquad.h:90
> +
Unnecessary newline?
Comment 3 Chris Rogers 2010-09-08 16:59:17 PDT
Created attachment 66967 [details]
Patch
Comment 4 Chris Rogers 2010-09-08 17:03:25 PDT
Ken, I've addressed your comments.  I've also fixed an issue where the filter coefficients (m_b1 and m_b2) were being used with negative values from what is normally used in biquad filter difference equations.  I've negated their use both in the processing math and when the coefficient values are calculated, so overall there is no change mathematically, but the code now looks cleaner and is in a more standard form.
Comment 5 Kenneth Russell 2010-09-09 14:15:57 PDT
Comment on attachment 66967 [details]
Patch

Looks good.
Comment 6 WebKit Commit Bot 2010-09-09 17:41:37 PDT
Comment on attachment 66967 [details]
Patch

Clearing flags on attachment: 66967

Committed r67132: <http://trac.webkit.org/changeset/67132>
Comment 7 WebKit Commit Bot 2010-09-09 17:41:43 PDT
All reviewed patches have been landed.  Closing bug.