Library with Decimator and Interpolator classes #677
Conversation
Update design_fir.py fix filename
move example
There was a problem hiding this comment.
Besides the notes made inline, the style is not consistent with our style guide. Namely, you should use tabs for indentation of the C++ code.
examples/Audio/resample/render.cpp
Outdated
| #include <libraries/AudioFile/AudioFile.h> | ||
| #include <libraries/Scope/Scope.h> | ||
|
|
||
| #include "Resample.h" |
There was a problem hiding this comment.
shouldn't this be libraries/Resample/Resample.h ?
examples/Audio/resample/render.cpp
Outdated
| return false; | ||
| }; | ||
|
|
||
| x = new float[blockSize](); |
There was a problem hiding this comment.
If you use raw pointers, you should correspondingly use delete [] x and delete [] y in cleanup(). Even better, you should probably use std::vector instead.
There was a problem hiding this comment.
Thanks. Changed the example to use stack initialized arrays (is that the right name?) as in examples/Audio/convolver/render.cpp.
examples/Audio/resample/render.cpp
Outdated
| } | ||
|
|
||
| // upsample | ||
| interpolator.process(x, y); |
There was a problem hiding this comment.
If using std::vector for x and y, you could rewrite this method to take (const) std::vector& as arguments, or if you keep the same interface, pass x.data() and y.data().
There was a problem hiding this comment.
I copied the interface from Convolver.process. The examples now also work the same, see above.
examples/Audio/resample/render.cpp
Outdated
| bool setup(BelaContext* context, void* userData) { | ||
| unsigned int sampleRateResampled = context->audioSampleRate / gDecimationFactor; | ||
|
|
||
| blockSize = context->audioFrames; |
There was a problem hiding this comment.
I'd suggest to use context->audioFrames directly where possible. It normally doesn't make much sense to cache its value in a global variable.
| pass | ||
| file = open(fname, "a") | ||
|
|
||
| file.write("#include <vector>\n\n") |
There was a problem hiding this comment.
When generating a header file, I'd suggest using std::array for this(as the array lengths are known at compile time and immutable).
There was a problem hiding this comment.
Would you then recommend to convert the arrays into vectors during load (in get_fir)?
| file.write(f"// numTaps: {len(tapsmin)}\n") | ||
| file.write(f"// ripple: {ripple[quality]}\n") | ||
| file.write(f"// phase: minimum\n") | ||
| file.write(f"std::vector<float> fir_{down}_{quality}_minphase = "+"{\n") |
There was a problem hiding this comment.
if using std::array, you'll have to add the array size within the template arguments.
|
|
||
| f = w * 0.5*sr/np.pi # Convert w to Hz. | ||
|
|
||
| plt.figure(1) |
There was a problem hiding this comment.
do these plt.xxx commands work fine on Bela? I am wondering if it'll crash because of lack of display. In the latter case they should probably made conditional.
There was a problem hiding this comment.
The script was meant to be run on a system that has scipy installed, see above comment. Is that a problem?
libraries/Resample/Resample.cpp
Outdated
|
|
||
| void Interpolator::process(ne10_float32_t* outBlock, const ne10_float32_t* inBlock) { | ||
| if (factor == 1 && outBlock != inBlock) { | ||
| memcpy(outBlock, inBlock, blockSizeIn * sizeof *outBlock); |
There was a problem hiding this comment.
Could you use parenthesis for sizeof? I find it clearer that way especially in these non-trivial expressions.
libraries/Resample/Resample.cpp
Outdated
| std::vector<float> fir = get_fir(L, quality, phase); | ||
| uint filtSize = fir.size(); | ||
| // numTaps must be a multiple of the interpolation factor | ||
| uint numTaps = ceil((float)filtSize / L) * L; |
There was a problem hiding this comment.
not a fan of aligning the = becuase if you add a variable with a longer name you'll need to move everything else (or risk of leaving a mess as it happens at line 104
std::vector<float> fir = get_fir(L, quality, phase);
|
Thanks a lot for the thorough review, @giuliomoro! I tried to improve the code using your comments, but I am not used to programming in C++ and/or code reviews on GitHub so its a learning process for me. 😸 How should we handle the generation of the filter coefficients? As mentioned above, the python distribution on Bela seems not to include SciPy so running the scripts on Bela seems tricky. |
Pull request related to post at https://forum.bela.io/d/1907-decimation-and-interpolation-library
libraries/Resample/Resample.hoffers classes for Decimation and Interpolation by integer order factors. See example atexamples/Audio/resample/render.cpp. The needed anti-aliasing filters are pre-computed by the included python script.Works fine with my own application at home, but haven't written a thorough test yet.
I am not used to write C++ code or design anti-aliasing filters, so would be happy for any feedback!
Todo: