Skip to content

Conversation

@Quant1um
Copy link

I found that when validating the example plugin with ASAN on the wrapper leaks about 2mb of memory. This PR fixes it by:

  • Using std::unique_ptr where applicable, and
  • Removing result->addRef(); in Vst3Parameter::create. Not sure if it is correct, or what the original purpose of this was, but it seems to work in validator under ASAN, Bitwig and Reaper. Review needed.

Side note: it would probably be a good idea to turn on address sanitizer while validating the plugin in CI

@baconpaul
Copy link
Collaborator

I'm pretty sure removing the addRef is wrong, and instead we should add a removeRef when we are done with our parameters explicitly. ASAN in the validator wouldn't necessarily catch this since the validator holds a reference to the param while using it and doesn't try to get it again, as far as I know.

@Quant1um
Copy link
Author

I'm pretty sure removing the addRef is wrong, and instead we should add a removeRef when we are done with our parameters explicitly. ASAN in the validator wouldn't necessarily catch this since the validator holds a reference to the param while using it and doesn't try to get it again, as far as I know.

Hmm, it is my understanding that ParameterContainer::addParameter takes ownership of the passed Parameter (it uses the IPtr constructor that doesnt add to refcount, so it stays 1 after addParameter and it gets released in ParameterContainer::removeAllParams and the destructor). The official again example seems to be consistent with this.

Also I am not sure that hosts even interact with the parameter reference (instead they use the getParameterInfo/getParamNormalized/etc API which uses parameters IDs to refer to them)

Correct me if I'm wrong

@baconpaul
Copy link
Collaborator

Right so you are saying the code *and * the comment are wrong! Got it.

@defiantnerd - you have git attribution for that addRef. Thoughts?

The unique ptr changes all look great to me! It's just that addRef I want to have someone else eye up.

Thanks!

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.

2 participants