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

Vibe -- fix undefined behavior, gargabe output #37

Merged
merged 2 commits into from Sep 10, 2019

Conversation

@x42
Copy link
Contributor

commented Sep 8, 2019

Ideally apply on top of #36

This fixes various issues with rkr-vibe due to uninitialized variables in the filters.
Depending on the initial state, it may produce constant garbage output and even NaN or Inf.

x42 added 2 commits Sep 8, 2019
@x42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

PS. Other effects may have similar issue, but before updating those, there'd ideally be a consistent API to ::activate() a plugin. That should called only once when leaving bypass to clear and reset stat, separate from ::cleanup().

@@ -311,7 +319,7 @@ Vibe::vibefilter(float data, fparams *ftype, int stage)
};

float
Vibe::bjt_shape(float data)
Vibe::bjt_shape(float data) const

This comment has been minimized.

Copy link
@ssj71

ssj71 Sep 9, 2019

Owner

for my own education, is this const making it return a const (immutable) float? what is the motivation for this change?

This comment has been minimized.

Copy link
@x42

x42 Sep 9, 2019

Author Contributor

it means that a method does not modify any class member variables.

I've added this while debugging, to ensure that vibefilter() and bjt_shape() don't change the state. Both methods could actually be static, since they don't use any member vars.

@ssj71

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

so you are saying ::cleanup should not be called when bypassing, and instead there should be a proper ::activate? I think I just implemented the same logic as was in the standalone rakarrack code to call cleanup on bypass. Doesn't make it right, I'm just trying to understand.

I'm fine with merging this, but I'd like to understand it a bit better.

@x42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

so you are saying ::cleanup should not be called when bypassing.

Ideally it would called only once. Right now every run_*() call periodically calls the cleanup method when a plugin is bypassed if(*plug->bypass_p && plug->prev_bypass) is true after the x-fade completed.

@x42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

The main part of the fix is the fparams() constructor in src/Vibe.h that initializes x1 = y1 = 0;
Previously those were uninitialized variables, producing garbage audio output.

After a bypass/re-enable those variable should be cleared, as well. The current patch clears them every cycle. It's not expensive to do that, so it's fine for the time being.

so you are saying ::cleanup should not be called when bypassing, and instead there should be a proper ::activate?

Yes. Long term it would be nice to only reset the state once.

@x42

This comment has been minimized.

@ssj71

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

ok, I think I should go through and fix the logic to only call ::cleanup excatly once after the xfade out. That was the intended result but I can see that its not correct. I can hopefully find enough time to do that in the next few days, that shouldn't necessarily change this PR though IIUC. Sound alright?

@x42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Ideally it'd be called once before fade_in: When activating a plugin, including the first run.

There's usually no need to reset state during cleanup.

@ssj71

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

I'm thinking like #38

It should get the job done, only that it will run an uncessary call to cleanup() when the plugin is bypassed then removed. Changing it to run the other way (cleanup on activate) is just a bit more work that I'm not sure is worth it. Am I missing something?

@ssj71 ssj71 merged commit ade5b34 into ssj71:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.