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

Fix case where delta is greater than loop length #26

Merged
merged 1 commit into from
Jul 4, 2019

Conversation

ngeiswei
Copy link
Contributor

@ngeiswei ngeiswei commented Jul 1, 2019

This may happen when the loop is very short and the note pitch is very
high. In the absence of that fix the m_phase may go out of bound,
including beyond the allocated memory for the sample, producing
glitches and eventually a crash.

@ngeiswei
Copy link
Contributor Author

ngeiswei commented Jul 1, 2019

I suppose

m_phase -= m_loop_phase1 * std::ceil(delta/m_loop_phase1);

would work as well. It's not clear to me it would be faster because it's a fairly rare case, and even in such rare case it shouldn't loop more than a handful of times. I like that it's slightly shorter however. Let me know what you prefer.

@rncbc
Copy link
Owner

rncbc commented Jul 1, 2019

i'm good on either solution

maybe you can test and make evidence of which one to follow/merge

cheers && thanks again

@ngeiswei
Copy link
Contributor Author

ngeiswei commented Jul 1, 2019

OK, I'll benchmark both, curious to see if I can measure a difference, probably not... (BTW, thank YOU, for your work thus far).

This may happen when the loop is very short and the note pitch is very
high. In the absence of that fix the m_phase may go out of bound,
including beyond the allocated memory for the sample, producing
glitches and eventually a crash.
@ngeiswei
Copy link
Contributor Author

ngeiswei commented Jul 3, 2019

There is no measurable difference in performance between the loop version and the rounded ratio version, so I've pushed the rounded ratio version as I found it slightly more elegant.

@rncbc rncbc merged commit de6c0a7 into rncbc:master Jul 4, 2019
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.

None yet

2 participants