surge-synthesizer / surge

Synthesizer plug-in (previously released as Vember Audio Surge)
https://surge-synthesizer.github.io/
GNU General Public License v3.0
3.11k stars 397 forks source link

LV2 patch corruption when re-opening GUI #1577

Closed unfa closed 3 years ago

unfa commented 4 years ago

I've been trying to get into Surge for a good while now.

My platform of choice is Linux, and until recently that's the only OS group I've been running Surge on. However recently I had something similar happen to me under Windows 10.

If I close the Surge window (GUI) and later open it up again in my DAW - at that moment the patch gets corrupted, usually in very specific ways.

Most often the oscillators all get reset to the default Sawtooth. Often LFO settings are corrupted too. Sometimes the patch is completely reset to Init state.

If I store my patch first before closing the GUI I can just reload it immediately after it gets corrupted, but it'll break again after I reopen the Surge window for another time.

As you can expect this is a big problem and prevents me from being able to do anything more than fiddling with Surge, and it's a big loss for me, as I see a great potential for amazing sounds in it.

I've had this issue for about a year now (basically since the first time I tried Surge on Linux). This happens to me no matter the host: Ardour, Carla or Zrythm.

However this week I had a similar problem happened on a Windows 10 system where I so far was able to use Sure with success with Ardour 5.12.

one of my patches got reset to Init when loading a project created a few days earlier. I have saved and loaded it before without problems, and other patches were fine. I don't know if this could be related.

Please let me know what can I do to help you fix this problem. I know I'm not the only Linux user suffering from it.

tank-trax commented 4 years ago

It appears to be working but this weird thing happens in Qtractor using LV2

created a patch and saved session reopen and Surge has the INIT playing when the MIDI device is played, however upon opening Surge or displaying its GUI and it plays the correct streamed/saved preset in the DAW

tried with a 2nd preset and it loads the correct sound when qtractor opens whether Surge's GUI is displayed or not,

in both cases the preset browser window displays INIT as it was not a preset that was saved but the session

it does appear to be holding the streamed sounds that are created without needing to save them as a Surge FXP preset

Update: it's not holding in Ardour (on first save occasionally, revisiting the sessions seems to work)

baconpaul commented 4 years ago

so "opening the editor restores the state"? Sigh. This is hard. How do I run QTractor and Ardour?

falkTX commented 4 years ago

Note that qtractor might send a MIDI program message at the beginning of the timeline. Not sure if related or not..

baconpaul commented 4 years ago

So I just started qtractor and loaded a preset and saved and when i restored and opened surge I had the preset name. When I look at the .qtr file it has everything saved properly also it seems.

Lemme see if I can make ardour work in my vm.

@tank-trax you are sure you are using latest, and also using fresh qtr files? Files you made with the old lv2 will not work reliably. (Although I will notice that qtractor streams the id number and name, not the symbol, differently than Carla, to the qtr)

baconpaul commented 4 years ago

So I can't actually test my sound or midi since my ubuntu vm doesn't route to my mac speakers and I don't have any usb midi devices which are set up for linux. And if I restore my std::cout prints in qtractor, it dumps core when I load the LV2 (Ha!).

I tried to make Ardour load the LV2 but couldn't find the LV2 path in any of the dialogs

So I'm not going to be much help. We are at least better off - we work in Carla it seems now - so I still plan to sweep this diff into 1.6.6 tomorrow.

But if I'm going to debug this I will need a lot of help with these tools and also things like "making ubuntu in a VM play out my mac speakers" which I've not needed to do up until now.

tank-trax commented 4 years ago

I will post some examples

baconpaul commented 4 years ago

As I mentioned on slack I can delay 1.6.6 a day or two while we try to pin this down

If you can tell me how to find and load an lv2 in ardour i Can look this weekend

tank-trax commented 4 years ago

At boot up Ardour (and Mixbus) scan for plugins, generally scanning whatever is in LV2_PATH, LADSPA_PATH and VST_PATH

sometime though certain plugins get blacklisted or need rescanning that would be in "Preferences -> Plugins -> Scan for Plugins"

to find paths in Linux for plugins...

echo $VST_PATH
~/.vst:/usr/lib/vst:/usr/local/lib/vst:/usr/lib/lxvst:~/.vst3 (I will have to add /usr/lib/vst3)

echo $LV2_PATH
~/.lv2:/usr/lib/lv2:/usr/local/lib/lv2

echo $LADSPA_PATH
~/.ladspa:/usr/lib/ladspa:/usr/local/lib/ladspa

echo $DSSI_PATH
~/.dssi:/usr/lib/dssi:/usr/local/lib/dssi

How to load LV2.... and an example of created preset saved by DAW, looks to be good, the qtractor thing could be an anomaly

I think it's OK, apart from qtractor needing to display the GUI on one stream to load it JALV.SELECT. Carla, Ardour are loading the saved/streamed preset according to the DAW,

baconpaul commented 4 years ago

OK so it sounds like we are feeling pretty good then with the change? Ardour seems to work?

unfa commented 4 years ago

I've just tested in Ardour and I don't know about re-opening the GUI, but I get patch changing itself when I stop the transport:

https://youtu.be/RTS5PQG70vQ

I've built today from source:

image

I'm not sure if this should be put on a separate issue.

baconpaul commented 4 years ago

It's all the same thing - we can leave it here.

Can you share that ardour file (just zip it and attach it to this issue) so I don't have to figure out how to set up and make midi flow into ardour?

I'm not sure which API point is getting called on transport stop but if I can just run it I will get there.

I really wonder why your build has a version number which says 1.6.2 + git by the way. That git hash is the latest so it is fine but whatever packing mechanism you are using should fix that version number! Totally separate issue though.

Thanks!

baconpaul commented 4 years ago

Actually I got it all working and it seems totally fine to me. Can stop transport, can do automation, can send notes.

I wonder: We had to change the lv2 description to fix this bug. Is there some ardour cache hanging around which you need to blow? I am using basically a fresh vm and a fresh install of everything so I have no lingering stuff.

https://www.youtube.com/watch?v=HlUHcsVI4F0&feature=youtu.be

there's a video of it running fine for me in a simple 'play 3 notes, have a patch saved'. And it all gets retained if I save and restore.

Thoughts on what's different between my experience and yours? The lingering cache is the only one I can really imagine.

baconpaul commented 4 years ago

And specifically from looking at how ardour saves files, if you are using a session that you created with surge.lv2 before 8dc0cf8 it will definitely have incorrect internal state that will lead to undefined behaviors.

unfa commented 4 years ago

That particular session was created long ago and I used it for testing. I should create a new session and test there too I guess.

tank-trax commented 4 years ago

git clone https://aur.archlinux.org/surge-synthesizer.git

will pull the PKGBUILD file

the version in the file says 1.6.5

and they are at Package Details: surge-synthesizer 1.6.5-1

https://aur.archlinux.org/packages/surge-synthesizer/

unfa commented 4 years ago

Here's the old test session I recorded the video with: Surge Test.zip

baconpaul commented 4 years ago

That particular session was created long ago and I used it for testing. I should create a new session and test there too I guess.

Yes the nature of our bug means that we had to change port symbols. Looking at the ardour implementation, they keep their port mappings based on symbols (which is a correct use of the spec) but when we change them it means the old session will be unpredictable.

The double fact that our bug was we had dup symbols means old sessions are double unpredictable

A fresh session is, alas, what's required yes. Sorry!

unfa commented 4 years ago

Ok, I've tested with a fresh session. The transport start/stop doesn't cause the corruption. But re-opening the GUI does!

baconpaul commented 4 years ago

OK great. I should have tested that I'm sorry. Lemme fire up my VM.

baconpaul commented 4 years ago

Alright so I just opened and closed the UI 20 times and didn't have vales change in the UI.

Can you give just a touch more detail?

I'm sorry - I'm so unfamiliar with these tools that debugging is really slow for me. I appreciate your help.

unfa commented 4 years ago

I've closed and re-opened Ardour and created yet another new session to be sure, but I still get this issue. Here's the session: Surge Test 3.zip

unfa commented 4 years ago

My oscillators are reset to Sawtooth and my LFO modes get reset too. I think Send effect returns are also reset to 100%, but haven't tested it right here.

Not all parameters are being reset. I think the filters are intact. I've been testing a bit more and I still get the transport corruption now.

unfa commented 4 years ago

What's interesting - transport corruption occurs eve if the GUI is closed.

That its: when I stop playback in Ardour the oscillator types get reset to Sawtooth (but not other parameters or LFO assignements).

Also my LFO's get switched back to Sine, from Envelope type I've set. Would a debug build be helpful?

baconpaul commented 4 years ago

Fascinating. I am putting in a bunch of log stuff. Seems ardour is calling activate/deacivate when you press stop. Taking a look....

A debug build might help. What I really need to do though is see what ardour is calling when with what values so I'm just using printfs right now tbh.

Stay tuned...

baconpaul commented 4 years ago

Oh boy that LFO comment is super useful. I'm actually able to reproduce that.

Basically open UI, set LFO type to square, close UI, open UI - voila sine again.

That's something I can debug for sure!

baconpaul commented 4 years ago

OK here is exactly the problem

Some parameters (and oscillator type and LFO shape) are not sending automation events to the LV2

Since LV2 (and only LV2) maintains this idea that there is an independently managed version of the state which is authoritative, though, since the LFO shape switch updates the internal state and the patch, but not the port, when you deactivate/reactivate you end up getting reset to the stored value in the port (in this case, sine and sawtooth) even though that value is nonsense.

I don't want to editorialize about the LV2 design decision that much other than to say it is radically different from the VSTs and the AU. But now I know this I think I can make a fix which for 1.6.6 I can conditionalize just to the LV2 to those two control points, and can test the rest.

baconpaul commented 4 years ago

Sigh. Very very difficult but I think I will have the fix soon enough. I'll just wrap it all in an #ifdef LV2 so we don' have to pollute the other hosts this close to a release. My guess is tomorrow or monday before i have something testable.

The three areas are the LFO type, the effects, and the oscillator type

And again the problem is that LV2 wants to pretend that the ports are primary, whereas the other host types know the synth is primary. And so we have to do careful state management. If we miss one message from synth -> host in LV2 we get corrupted, whereas in every other plugin style we have no problem because the host asks the plugin for values and doesn't maintain independent state that needs coordinating. :(

baconpaul commented 4 years ago

OK that LFO tip was exactly what I needed

Three things - FX, LFO and OSC - could get out of sync between the synth and the ports because Surge chose to not notify automation of those in bulk. In the other flavors this wasn't a problem. In the LV2 it is fatal.

So I have forced surge to scream parameter changes to the LV2 when you change those things and now it all works.

Let me let our CI run and then I'll merge the change and you can try it again

Thanks for your patience.

baconpaul commented 4 years ago

Alright @unfa @tank-trax if you can try as of fc6af5b970 that would be really appreciated. I think I got it all

If you do see a corruption (which is more accurate called a 'non-synchronization' now I understand it) I'll need to know which field I missed so I can apply the same fix to it. I tried them to make sure messages get sent and think i got them all but some exhaustive test (which is basally: Change something, close the ui, press plan and stop, open the UI, is it the same) would be helpful.

the always-on sliders are all OK. It's just the switches and dynamic elements which could be broken.

Would still like to do a release on Wednesday so feedback tomorrow or Monday would be super useful if you can.

Thank you for your help and patience.

falkTX commented 4 years ago

Did I understood things wrong, or was the act of opening the UI triggering changes in the DSP? When it Ui opens, it needs to read values from DSP side and display them. It should never ever change values when opened, parameters and state must only change when dealing with an action from the user. So even if the LFO type in the UI got wrong, it still should not do anything regarding the DSP side... right?

baconpaul commented 4 years ago

So @falkTX here’s what was happening exactly

There were UI gestures which changed the internal state of the UI but didn’t broadcast parameter changes to the LV2.

This mean the LV2 ports kept the old copy

When you start and stop transport or in some cases open and close the UI in Ardour it activates and reactivates; or does some other form of reapplying the ports to the synth. There’s a couple of paths but basically if the synth and ports are inconsistent you see it when changing transport or when opening and closing.

Thus the old copy was applied

My fix was to make sure the UI gestures sent the full set of parameter changes to the LV2

This isn’t a problem in the other hosts since they don’t maintain and apply independent copies of the state

I think it is probably wrong that we don’t broadcast the parameter changes from surge but for our upcoming release, I conditioned it to only LV2 to minimize risk. I will revisit in 1.7 if we get confirmation that the latest build fixes.

If I were to make one LV2 feature request it would be to have a “purely virtual” port whose getter doesn’t store a value but rather calls a function every time, and has a notification of change. I don’t know how hard that is but such a port means we could keep the state resident in surge and not have this double state management to deal with.

Appreciate your help. The double copy of the state is a tricky feature of LV2 for sure but I think we have it now.

baconpaul commented 4 years ago

Actually now I think of it we can probably remove all the state caching stuff if we just make the patch reload send an automation message for every parameter in the lv2. I don’t want to make that big a change now especially if we have it right with my tactical change but clearly there’s an opportunity to use what I learned last 12 hours and fix some other bits of state management in a way which makes surge more lv2-y. Separate issue though - what I need now is confirmation or bug report on current branch. Thanks!

baconpaul commented 4 years ago

Actually: I'm going to take a look at doing just this. Since the LV2 is broken now it can't make it any worse and I can conditionalize it all. Really would appreciate tests at head but may be more coming.

Ignore that. I'm sticking with the patch we have. I see why it is complicated - there are situations where we change a patch in surge which are not triggered by a UI action, like midi program changes - and I now see how @jpcima dealt with that.

I think we can, indeed, deal with that better by doing a big collection of UI-like gestures from the synth but it's more restructuring than I want to do

The version at master i the version I want to go with for now. Sorry for the git noise.

unfa commented 4 years ago

I've just built from latest master and I can't reproduce this problem any more! Horray!

I don't want to bless the day already, because it might be too early, but I think you've got it sorted out guys.

Thank you so much for your work!

baconpaul commented 4 years ago

Awesome. Thank you very much!!

I plan to ship this version as 1.6.6 on probably Wednesday.

unfa commented 4 years ago

I'd like to perform some thorough testing to make sure this is done and dusted before the release, but I am afraid I won't manage to do as much. I will try to test as much as I can and provide feedback here if I find any problems st ill.

I am very happy, because I can finally start using Surge in my music and make videos about it! I've been waiting fro so long :D

Thank you!

baconpaul commented 4 years ago

Yeah great! And sorry you had been waiting so long - that's a bummer bug glad we got it fixed once you flagged it here.

Please do flag issue here on the GitHub. It is the only way I'll reliably see them.

unfa commented 4 years ago

@baconpaul Thank you so much for your work on fixing this. I hope to test Surge more thoroughly soon and start using it in my work. I'll report any residual related issues here if they creep up.

baconpaul commented 4 years ago

Wonderful! As you do please remember we’ve worked to keep the manual up to date; so if you find the manual doesn’t match your experience that’s also a bug we would fix. And enjoy making music with surge!

unfa commented 4 years ago

I've found some more places where this is still present.

When I used 4 LFOs (1 - 4) and switched them to envelope mode, they got switched back to sine and also the sustain level of the envelopes was set to 1.

You can see that on the snare patch here: Hardbass Test.zip

(I was unable to get proper amount of pitch modulation fro ma single LFO/envelope so I used 4 together to get the desired effect - BTW am I doing it wrong?)

unfa commented 4 years ago

I will update now and see if I can reproduce.

baconpaul commented 4 years ago

OK I was going to do the 1.6.6. Release tomorrow morning. I’ll peek at this case before I do and perhaps sneak a fix in under the wire. Thanks!

As to extreme pitch modulation, right click on the pitch slider and choose “extend” and both your setting and modulation range go from +/- 12 to +/- 96 notes.

baconpaul commented 4 years ago

I can't load your session; but I was able to have the envelope mode and envelope sustain work all no problem for me with the code at head in ardour

baconpaul commented 4 years ago

Since I can't repro this I'll proceed with the 1.6.6 release this morning - we can always fix it later if there is a problem we find a solution to and can even branch back into the 1.6.6 family if we are well down the 1.7 path.

unfa commented 4 years ago

Hmm, I've done a livestream using Surge last Sunday and it not only worked slowly, but also caused Ardour to crash multiple times, and then kept crashing the session upon load making it impossible to progress: https://youtu.be/hgsQu8xHU2k

I've just updated from Git and I've tried to use it again, but I have the transport top/start resetting my Insert FX dry/wet to 100% and also it crashed Ardour once.

baconpaul commented 3 years ago

Closing this issue, since #4030 is the place we are investing in LV2 going forward. If the JUCE/LV2 port works, it works, and the problems in our hand-port will be gone (or replaced with problems in the JUCE/LV2 port).