kmatheussen / radium

A graphical music editor. A next generation tracker.
http://users.notam02.no/~kjetism/radium/
GNU General Public License v2.0
835 stars 36 forks source link

vst3 plugin info is not loaded correctly #1350

Open ghost opened 2 years ago

ghost commented 2 years ago

i noticed that when saving and reloading a project with a vst3 plugin, not all of the information in the plugin's patch is saved, and the plugin sounds different between saving and loading. this doesn't happen with vst2 plugins

i've tested surge and vitalium so far

i'm using 6.9.96

akimaze commented 2 years ago

I confirm the bug, TAL Noise Maker 5.0.4 VST3 is also affected. Tested on Radium official build 6.9.96 on Linux Mint 20.2

kmatheussen commented 2 years ago

I don't know what could cause this, but I'll see if I can upgrade JUCE to the lastest version for the next release. There's been a few fixes for VST3 (see this discussion for instance: https://forum.juce.com/t/hosting-vst3-with-juce-getting-incorrect-parameter-reference-from-specific-vst3-plugins/46441), so maybe that'll fix it.

akimaze commented 2 years ago

@kmatheussen I tried compile Radium with JUCE 6.1.2 (with modified juce_MidiMessage.h and juce_MidiMessage.cpp) saved sound changes a little but still something is not saved.

akimaze commented 2 years ago

@kmatheussen I tried the latest official 6.9.97 version to test is this bug but Surge 1.9.0 VST3 simply crashes (always reproducible) when I try add plugin to track from plugin manager. I sent the crash report.

kmatheussen commented 2 years ago

I tried latest version of Surge now, 1.9.09xxxx from 2021-04-21. And it seems to work. I can't reproduce the crash. But the crash you reported happened inside JUCE, so it doesn't look like a bug in Surge. I'll look into it. Thanks for reporting.

akimaze commented 2 years ago

Yes I use the same version as you (1.9.091069f8 2021-04-21) I also tested it (Surge) in other DAWS and it works OK. Vital VST3 (VST3 seems more stable than VST2) and Odin2 is working OK. Another plugin that crashes like Surge is CHOW Kick.

akimaze commented 2 years ago

I can't remember what preset in Tal-Noise Maker was incorrectly saved in the project. But if you have a working Surge you can easily check it by selecting the preset Leads-> Bee from Surge. In the track insert a note eg C4. Then save and load the Radium project, the sound will be completely different. When you open Surge UI for example Amplitude of LFO 1 parameter was not saved, Filter EG envelope, and a lot of parameters in FX block.

kmatheussen commented 2 years ago

The VST3 crashes should be fixed in https://github.com/kmatheussen/radium/commit/70176fd11cb0c5c339e587921db66cd78b1eab5c It was caused by juce throwing an assertion because bus configurations was not supported. Assertions shouldn't be enabled in release mode.

akimaze commented 2 years ago

@kmatheussen Thank you Radium is not crashing now but still something is not saved from plugin state.

I had an idea how to check if the problem is JUCE (6.1.4) or some where in Radium. I compiled Juce Audio Plugin Host (from extras folder, simply run make), added Surge (preset Leads-> Bee) and saved filtergraph to file. After reading the file, the plug-in state is fully restored. From this I conclude that the problem is in saving (or reading) to the file in Radium. And JUCE is OK.

image

kmatheussen commented 2 years ago

I don't know exactly what's causing this, but it seems related to loading state twice, which of course isn't necessary, but shouldn't cause any problems either in my opinion.

As a quick fix, you can remove the call to 'recreate_from_state' in audio/Juce_plugins.cpp, around line 2365:

@@ -2353,7 +2361,7 @@ static void *create_plugin_data(const SoundPluginType *plugin_type, SoundPlugin

     run_on_message_thread([audio_instance_pointer, plugin, type_data, state2, is_loading]() {
       plugin->data = new Data(audio_instance_pointer, plugin, type_data);
-      recreate_from_state(plugin, state2, is_loading);
+      //recreate_from_state(plugin, state2, is_loading);
       });     
akimaze commented 2 years ago

I confirm that your quick fix fixes that issue. Thank you!

kmatheussen commented 2 years ago

Good. A word of caution though, I haven't tested, but it's possible that removing that line causes loading of .rec or .mrec files to fail, and/or that changing A/B settings will fail. In that case, this patch might work instead:

@@ -2353,7 +2361,8 @@ static void *create_plugin_data(const SoundPluginType *plugin_type, SoundPlugin

     run_on_message_thread([audio_instance_pointer, plugin, type_data, state2, is_loading]() {
       plugin->data = new Data(audio_instance_pointer, plugin, type_data);
-      recreate_from_state(plugin, state2, is_loading);
+      if (!is_loading)
+        recreate_from_state(plugin, state2, is_loading);
       });  
akimaze commented 2 years ago

From my tests: loading VST3 plugin with A,B,C,D - only A works OK with the first and second quick fix. But loading .rec preset works better in first solution than the second. With the first solution first (A) settings are OK but others was incorrect so something is not going right. With the second solution loading rec file (if (!is_loading)) was totally broken - all A,B,C,D plugin states was incorrect.

So second loading is definitely broken - maybe something is freed after first loading? Or maybe this assertion that you disabled should be fixed? Or maybe this is some threading issue? I don't know the Radium code so I'm just guessing ;)

akimaze commented 2 years ago

After #1367 I started to think that it could be the same with loading VST3 plugins. I did more tests with versions 96 and 97 of Radium. After testing it turns out that Vital (Chorusy Keys preset) which did not work in 96 in 97 works well. Similarly I found Tal-Noise Maker preset (ARP 303 Like FN) which did not work in 96 also works well in 97. Only this Bee preset from Surge still not working in 97. Today, however, the new Surge XT 1.0 came out and the Bee preset works in my build without your quick fix (I can't test it in official 97 because of this assertion that crashes Radium).

So it looks like the JUCE update has fixed the problem. But double loading makes Radium much more sensitive to bugs in plugins, so it's probably worth removing the second loading if it's possible :)

Notice that we still need disabling assertion for new Surge XT also so don't remove it.

akimaze commented 2 years ago

I also found surge bug report about loading presets https://github.com/surge-synthesizer/surge/issues/5524

kmatheussen commented 2 years ago

Yeah, I'm pretty sure there's no threading bugs or anything like that. If there were, we would probably have noticed earlier since the same code is used for all types of plugins, not just VST3 plugins on Linux.

But I wonder which of the two state-loading calls that should be moved. Currently it works like this:

  1. Load plugin state
  2. Set plugin effects
  3. Load plugin state (again)

Should the plugin state be set after, or before, setting plugin effects?

kmatheussen commented 2 years ago

Oh, and my tip to fix the problem, by removing the call to 'recreate_from_state', was by removing step 1. So it's not that step 3 is the one causing the problem, which would have been a natural thing to think. It's a really strange bug, Most likely it's a bug in either the VST3 plugins themselves, or in JUCE, but it's really strange.

akimaze commented 2 years ago

For the last two months I have been still using Radium without quick fix (but with an assertion fix that you made for Surge) and I haven't noticed any problems with saving/loading projects (I use Surge XT instead of the old Surge). Since I'm using Radium as my default DAW now, it has been hundreds of loads and saves. So I would assume the problems were plug-in problems. And this quick fix is not needed. I will write a comment here if I notice a problem with loading the state of some plugins.

BTW. New JUCE version is released with some interesting fixes for VST3 and VST3Hosts: https://github.com/juce-framework/JUCE/commit/ec867690b7fb4a8dde011ec5a929e2ed22fda74b https://github.com/juce-framework/JUCE/commit/1bf9ebb4b113bf2dc3c4537974cd81f1cca67a97 https://github.com/juce-framework/JUCE/commit/a3c55a967f5d5811ef451c48dd095ebfd64f74ff https://github.com/juce-framework/JUCE/commit/bd0ca90952c95aeface9dcb4219d746f24f4601d https://github.com/juce-framework/JUCE/commit/a7811661c5d8d0bfc3ee8c1fdef5c5078640ab9c https://github.com/juce-framework/JUCE/commit/06db7f074e595f22be53a0cfc448011b88559ec4 https://github.com/juce-framework/JUCE/commit/bd52350c00ee96dc0a3175c7c7191a8fdf3f0c36

BTW2. Maybe is worth to add JUCE version to about dialog?

kmatheussen commented 2 years ago

Thanks for the update. I will try to update JUCE for the next release and include JUCE version in the about dialog.

On Thu, Mar 17, 2022 at 10:25 AM akimaze @.***> wrote:

For the last two months I have been still using Radium without quick fix (but with an assertion fix that you made for Surge) and I haven't noticed any problems with saving/loading projects (I use Surge XT instead of the old Surge). Since I'm using Radium as my default DAW now, it has been hundreds of loads and saves. So I would assume the problems were plug-in problems. And this quick fix is not needed. I will write a comment here if I notice a problem with loading the state of some plugins.

BTW. New JUCE version is released with some interesting fixes for VST3 and VST3Hosts: @. https://github.com/juce-framework/JUCE/commit/ec867690b7fb4a8dde011ec5a929e2ed22fda74b @. https://github.com/juce-framework/JUCE/commit/1bf9ebb4b113bf2dc3c4537974cd81f1cca67a97 @. https://github.com/juce-framework/JUCE/commit/a3c55a967f5d5811ef451c48dd095ebfd64f74ff @. https://github.com/juce-framework/JUCE/commit/bd0ca90952c95aeface9dcb4219d746f24f4601d @. https://github.com/juce-framework/JUCE/commit/a7811661c5d8d0bfc3ee8c1fdef5c5078640ab9c @. https://github.com/juce-framework/JUCE/commit/06db7f074e595f22be53a0cfc448011b88559ec4 @.*** https://github.com/juce-framework/JUCE/commit/bd52350c00ee96dc0a3175c7c7191a8fdf3f0c36

BTW2. Maybe is worth to add JUCE version to about dialog?

— Reply to this email directly, view it on GitHub https://github.com/kmatheussen/radium/issues/1350#issuecomment-1070613107, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIX3J7RDV3UXOYKM2ZAG6DVAL263ANCNFSM5FQRBN5A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>