LV2 sometimes crashes when re-opening the gui, breaking the Ardour project.

[magnetophon]

Describe the bug
In Ardour, the LV2 sometimes crashes when re-opening the gui, taking down Ardour with it.
Sometimes those crashes then leave the Ardour project in an un-openable state: when I try it crashes again with the message:

[1]    14121 segmentation fault (core dumped)  ardour5 ~/ardour/SynthyTetst/SynthyTetst.ardour

Please let us know your surge version

• Surge Version: 1.6.6
• Plugin type LV2
• Bits 64

To Reproduce
I have no sure-fire way of reproducing this.
If wanted, I can attach the Ardour file.

Desktop (please complete the following information):

• OS: NixOS
• Host: Ardour
• Version: 5.12

[baconpaul]

Thanks for the report. A lot going on right now but ill see if I can repro when I'm next over in linux. Also tagging @jpcima in case he wants to take a shot at it.

[jpcima]

Yes sure, I did not have a lot of luck at trying to reproduce the Ardour crashes reported lately when I tried, but also I don't really manipulate Ardour beyond the very basics.
@magnetophon please share the session file.

[magnetophon]

@jpcima Here you go:
SynthyTetst.ardour.txt

I had to change the file extention to please github, just remove the ".txt" again.

[jpcima]

Thanks, I'm attaching a backtrace of the crash.
Backtrace.txt

The problem is as follows:

• In LV2, the wrapper would assume the caller of setParameterAutomated is the UI.
  While not true in general case, in LV2, the DSP-side is not allowed to modify its own parameters.
  (without doubt it would be best to make this part more robust, I'm not yet sure how)

There were a few TARGET_LV2 conditionals that check against this inside SurgeSynthesizer.

Now there is also this part, from where arises the problem, found in SurgeSynthesizer::loadOscalgos.

surge/src/common/SurgeSynthesizer.cpp

Lines 1644 to 1652 in a7da44c

	#if TARGET_LV2
	auto tp = &(storage.getPatch().scene[s].osc[i].type);
	sendParameterAutomation(tp->id, getParameter01(tp->id) );
	for (int k = 0; k < n_osc_params; k++)
	{
	auto pp = &(storage.getPatch().scene[s].osc[i].p[k]);
	sendParameterAutomation(pp->id, getParameter01(pp->id) );
	}
	#endif

From Ardour, we're not in UI thread at this point, we're DSP, _editor is null and we crash.
The call chain is process → processControl → loadOscalgos.

[baconpaul]

Right but without that, the UI doesn’t update and the state doesn’t save.

Let me explain what is happening. The short version is “the DSP changes the parameters”

But when you switch oscillator types in surge (through whatever mechanism - loading a patch, automation in your DAW, or a variety of other things) the values of a collection of parameters change.

Or in other words if you get the automation message (from the UI or the DAW) “set oscillator 2 type to sin”, it means that the 10 subordinate parameters also update.

So how do you do that in LV2?

Without the above code, the parameters don’t update and you see the original problem @unfa had. With the above problem you get a crash.

I gotta say this insistence that the parameters are external, independent, and not governed by the engine is, I think, a pretty big problem for surge in LV2! I am curious how other synths with any sort of dynamism possible react.

[baconpaul]

Another way of thinking of this is imagine the following general problem

You modify parameter 1 in your synth
That means the synth has new values for parameter 2-7

So, for instance, you change an effect type from delay to chorus. That means FX param 1 has its meaning change from chorus depth to delay rate, and its value change from 12hz to 4 seconds.

Or you send a midi program change message to your synth, which changes the value of every parameter, because the program has changed.

In AU/VST2/VST3 you simply send param automated or update your value and let the DAW poll.

How does an LV2 solve these problem.

[jpcima]

This mode of doing things is not adaptable as such into LV2.
By that, I mean parameters that can change value and repurpose themselves by whatever mechanism as mentioned in the second post above.

Such manipulations are only possible in presence of a UI actor.
LV2 is strict regarding this (unfortunately maybe), as it wants to put the most control into the host instead of the plugin.

Hence, no immediate solution. With this said, I don't have all the knowledge regarding LV2 and will like to be proven wrong.

[baconpaul]

Who would be able to help? Any ideas?

Is it really the case that no LV2 plugin supports a midi program change message? Maybe we start there!

If what you say is strictly true, i don’t see a way for surge to work as an LV2, and that seems like it is probably not the conclusion we want to draw.

[jpcima]

I was mentioned the patch extensions before, so perhaps something to research there, especially patch:Set. I'm not sure if it can serve the purpose we would like, which is for DSP to send parameter change to Host.

cc @falkTX, do you know about this? I noted Carla contains some mentions of patch:Set URID.

[baconpaul]

Yeah I don’t understand lv2 well enough to understand that document alas.

But our use case is super simple. We have interdependent parameters which are exposed for automation. So in the absence of a ui, a valid automation message coming from the daw on one parameter implies a change on another 3-4.

If we know how to crack that in a valid lv2 I can help us figure out what to do with surge.

[falkTX]

// in run, event iterator
if (event->body.type == fURIDs.atomObject)
{
const LV2_Atom_Object* const object = (const LV2_Atom_Object*)&event->body;
const LV2_Atom* property = nullptr;
const LV2_Atom* value = nullptr;
lv2_atom_object_get(object, fURIDs.patchProperty, &property, fURIDs.patchValue, &value, nullptr);

if (property != nullptr && property->type == fURIDs.atomURID &&
    value != nullptr && value->type == fURIDs.atomFloat)
{
    const LV2_URID urid = ((const LV2_Atom_URID*)property)->body;
    const float value = ((const LV2_Atom_Float*)property)->body;
    // compare urid to previously mapped values, set value
}

}

[falkTX]

BEH that was so stupid, deleted my comment contents by mistake while editing... :(
hopefully you get the details over email...

[jpcima]

@falkTX that's immensely useful, thanks a ton
I'll try putting that to practice to check how that works.

[baconpaul]

Ooh and yeah we can try that instead of the setParameterAutomateds I put in place perhaps? They are all LV2 conditionalized anyway. Check out the diff I did right before 1.6.6 for the LV2 for the places I had to fix to keep us consistent.

[jpcima]

I think it can substitute the implementation of setParameterAutomated entirely if I understand right.
As soon as I have time, I will try.

[baconpaul]

wonderful.

[magnetophon]

Thanks a lot everybody!

Looking forward to making music with this wonderful synth, without constant fear of loosing work!

[baconpaul]

Hey @jpcima and @falkTX

Hope all is well wherever you are in the world.

@tank-trax and I were chatting about this today. Any chance that the mail from @falkTX which explains how to fix this and which got half pasted into this issue could be at least fully pasted into the issue? Would love to fix this. We've been doing a bunch of other infrastructure friendly things (move to cmake; add a skin engine) and quite a bit of dsp stuff too (expose a new reverb; start work on a flanger) and I think we will do a 1.7 this summer sometime. Would love to have LV2 resolved in that release!

Thanks, and again, hope all is safe and well by you both.

[baconpaul]

Hey guys. Still sending my best wishes in crazy times. wondering if you can share a bit more of the intent of the truncated messages above?

making progress on linux vst3 and would love the lv2 to work. right now one of the primary reason that surge isn't all FOSS is the linux music community. Our LV2 has bugs and the community is still catching up in vst3 land so I can't credibly tell people to not use vst2.

but with juce6 finally supporting linux vst3 and our lv2 so tantalizingly close i see a path to a properly FOSS surge.... so anything you could share would be appreciated.

[baconpaul]

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).