Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IIR EQ base class #294

Merged
merged 10 commits into from
Aug 7, 2014
Merged

IIR EQ base class #294

merged 10 commits into from
Aug 7, 2014

Conversation

daschuer
Copy link
Member

This introduces a common base class for all fidlib based Filters.
It makes use of templates to be generic as possible, without introducing CPU overhead.

This allows to create most of fidlib filters.
Added a Butterworth4 filter to test it out.
Repaired ramping (regression from last commit)
@daschuer
Copy link
Member Author

@badescunicu please have a look. Thank you.

@@ -1,69 +1,29 @@
#ifndef ENGINEFILTEBESSEL4_H
#define ENGINEFILTEBESSEL4_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot an "R" : ENGINEFILTERBESSEL4_H

@badescunicu
Copy link
Contributor

LGTM. It is a nice addition because a lot of code was duplicated before.

@daschuer
Copy link
Member Author

Notes addressed. Any one else willing to review?

#define IIR_BP 1
#define IIR_HP 2

template<unsigned int SIZE, unsigned int PASS>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PASS should be an enum or enum class.

@ywwg
Copy link
Member

ywwg commented Jul 24, 2014

do we have any tests so we can prove this is a noop? I would be satisfied with running data through the old version and making "golden data" out of that to test against.

@daschuer
Copy link
Member Author

do we have any tests so we can prove this is a noop? I would be satisfied with running data through the old version and making "golden data" out of that to test against.

Do you think we need a Test in terms of Google test?
Do you want to compare the samples? I think this is not worth the work, because it is obvious that the core process functions are not changed.

@daschuer
Copy link
Member Author

daschuer commented Aug 3, 2014

@ywwg: Is it merge-able now?

double cross_inc = 2.0 / static_cast<double>(iBufferSize);
for (int i = 0; i < iBufferSize; i += 2) {
// Do a linear cross fade between the old samples and the new samples
// Fade input of the new filter, bacause it is settled for In = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change bacause to because

@kain88-de
Copy link
Member

Beside the style nitpicks and the comment about your new fading code LGTM.

A test that the copied processSample function still produce the same result would be nice but I also feel comfortable merging this without

This avoids the gain drop during filter change, and is a good compromise between a setteld filter and latency.
@daschuer
Copy link
Member Author

daschuer commented Aug 7, 2014

Thank you for review. I will merge it now since this is the base of all upcoming EQ PRs and it will be tested a lot during those tests.

daschuer added a commit that referenced this pull request Aug 7, 2014
@daschuer daschuer merged commit db39484 into mixxxdj:master Aug 7, 2014
@daschuer daschuer deleted the iireqs branch May 30, 2015 21:55
m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024
news(release): add 2.3.5 release blog post
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants