search

DISTRHO / Cardinal

Virtual modular synthesizer plugin
https://cardinal.kx.studio/
GNU General Public License v3.0
2.22k stars 153 forks source link

reinit random in case plugin was initialized on another thread #515

Closed ctsexton closed 1 year ago

ctsexton commented 1 year ago

Hey there DISTRHO folks,

The random module uses a thread-local RNG state, making an assumption that the initialization will always occur on the same thread that the random functions are called from. I think this is not a safe assumption to make in a plugin context because the plugin might not be instantiated on the same thread as the UI code that calls these functions. I guess most plugins assume the host is instantiating plugins on the UI thread.

Ran into this when trying to use Cardinal in my host, and adding a new module or cable would cause that while loop to spin forever since random::u64() was returning 0. I know this is just a band-aid fix, it would be easy to forget to add this in other places where the random module is used (I didn't go looking outside this Engine.cpp file). Maybe you have a better way, but this fixed the issue for me at least.

Thanks for Cardinal!

falkTX commented 1 year ago

Rack 2.3 (or 2.2 not sure) has changes related to this, it is best to follow what they do for this.

ctsexton commented 1 year ago

Yep makes sense. I found the commit in Rack here: https://github.com/VCVRack/Rack/commit/bf675ada612c514ad1a18a0a89d804ea265dca8b

How would you prefer I apply the patch? Should I add the random files to the override directory? I could try updating the git submodule but that might be a lot more work if there are other unrelated changes that need to be addressed.

falkTX commented 1 year ago

Best to do is to have a PR with the changes ready to match Rack 2.3 Engine.cpp side, assuming the update to latest Rack at some point soon.

I have been too busy, not able to give Cardinal proper attention lately, but even if I had time I think it is better to wait until a 2.3.1 pops out, or at least things stabilize on their API changes.. There have been quite some noise in their forums in regards to an API breaking change, so now is not the time to update. We have SurgeXT and MindMeld in Cardinal, 2 big modules affected by the change, lets wait until dust settles.

falkTX commented 1 year ago

I am lacking time in April, and want to get a release out on the 15th to be part of the cool kids group (see https://libreav.org/article/quarterly-release-pact)

So for now I think I will merge your changes as-is. When we update the Rack version used in Cardinal, we can clean things up and go the same way as VCV. But personally I dont see many reasons to update just yet, considering it would take a little time to adjust to it and has potential breakage on random places.