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

Lanczos read index error #4465

Closed
baconpaul opened this issue May 3, 2021 · 4 comments
Closed

Lanczos read index error #4465

baconpaul opened this issue May 3, 2021 · 4 comments
Labels
Bug Report Item submitted using the Bug Report template DSP Issues and feature requests related to sound generation in the synth
Milestone

Comments

@baconpaul
Copy link
Collaborator

@jatinchowdhury18 reports:

By the way @baconpaul, I was studying the Lanczos resampling code today, and might have found a weird edge case: basically at this line (https://github.com/surge-synthesizer/surge/blob/main/src/common/dsp/LanczosResampler.h#L132) tidx can end up being equal to LanczosResampler::tableObs which means that all of the table accesses further down in that function are uninitialized. I noticed the issue when the input and output sample rates are the same, but I could imagine getting the same bug (but less often) if the sample rates are an integer multiple.

and I think he's right (even though it would only blow up twist when running at 24k sample rate I think). Write a test and check and fix for sure.

@baconpaul baconpaul added DSP Issues and feature requests related to sound generation in the synth Bug Report Item submitted using the Bug Report template labels May 3, 2021
@baconpaul baconpaul added this to the Surge XT 1.0 milestone May 3, 2021
@jatinchowdhury18
Copy link
Collaborator

Yeah, the solution I came up with was to make the tables longer by 1 entry

float lanczosTable[tableObs+1][filterSize];

But a better option might be to wrap (but after setting fidx):

int tidx = (int)(off0byto);
double fidx = (off0byto - tidx);
tidx = (idx0 + tableObs) & (tableObs - 1);

Another weird behaviour I found was that if you push() more samples than the BUFFER_SZ in between calls to populateNext() then the buffer will overflow, and weird stuff starts to happen. It probably doesn't matter for our purposes since the block sizes are small, and I'm not sure if there's anything we can do about it anyway, but it might be worth putting a comment in case we try to re-use that code elsewhere.

Anyway, just wanted to write down the stuff I was thinking last night before I forget it all :). I can help with writing test cases and so on later today or tomorrow.

@baconpaul
Copy link
Collaborator Author

I was thinking fo the extend-by-one fix this morning and I think it is the right one.
The wrap goes to the wrong "side" - basically we need to be up-to-and-including one not up-to one.

@baconpaul
Copy link
Collaborator Author

yeah the buffer overflow is real but practically not a problem. I thought about making the buffer size a template.

I'll push a little +1 fix in a minute.

baconpaul added a commit to baconpaul/surge that referenced this issue May 3, 2021
@baconpaul
Copy link
Collaborator Author

Anyway put in a fix and tested twist - which is still fine at 44.1 and 48
let me know if it fixes things for your test case also?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Report Item submitted using the Bug Report template DSP Issues and feature requests related to sound generation in the synth
Projects
None yet
Development

No branches or pull requests

2 participants