Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

calf.so shipped with LMMS crashes Ardour and Calf plug-ins #3942

Closed
boomshop opened this issue Nov 5, 2017 · 20 comments
Closed

calf.so shipped with LMMS crashes Ardour and Calf plug-ins #3942

boomshop opened this issue Nov 5, 2017 · 20 comments

Comments

@boomshop
Copy link

boomshop commented Nov 5, 2017

Dear Devs,

the calf.so containing the legacy LADSPA Calf plug-ins shipped with LMMS crashes Ardour in conjunction with the "real" Calf plug-ins as soon as one adds any Calf plug-in to a track or bus in Ardour.

https://linuxmusicians.com/viewtopic.php?f=24&t=17751#p87012
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=870473

Probably renaming of package and plugins could do the trick.

@tresf
Copy link
Member

tresf commented Nov 5, 2017

We aren't the authors of calf-ladspa, so this really is a packaging issue and we don't package LMMS for any Linux platform currently.

Since LADSPA support was removed from calf quite a while ago, a forked version of the library has been bundled with LMMS since. Eventually, we can go back to mainline when LV2 is written, but that's a separate task.

If a packager puts our LADSPA plugins in the same place as upstream is looking, they need to stop. I'm not sure what we can do to help this situation except write LV2 support. Creating new LADSPA IDs and prefixed plugin names sounds like a bigger mess to all parties involved.

Edit: Thanks for your work on this library BTW. 🍺

@tresf
Copy link
Member

tresf commented Nov 5, 2017

To add to the note about upstream cooperation, we do have an active initiative to move all of our plugins to be pulling from and contributing directly to upstream. Our test case is swh (#3931) and we've made good progress on this.

Since all Calf plugins are bundled into one single library, the calf.so we ship could certainly be renamed to something different, but I think it would be better for the distros to just build the version we bundle and place it into a non-standard directory instead. We can add a new searchpath if needed.

@tresf
Copy link
Member

tresf commented Nov 6, 2017

@boomshop I tested it as veal.so and it works fine. I've forked upstream and renamed it as well to be consistent. 2896168#diff-7594a73ab160d0bf3265845d9c8c42a7L5 🐄

@boomshop
Copy link
Author

boomshop commented Nov 6, 2017

Thanks a lot for taking care of this problem.

If a packager puts our LADSPA plugins in the same place as upstream is looking, they need to stop.

True. This is a problem at least on Debain-based package management. KXStudio offers a workaround by having its own (completely empty) calf-ladspa package which is a dependency of calf-plugins now.

What makes me wonder is that on an Arch system the LADSPA calf.so was installed in some LMMS directory but still found by Ardour. Removing it from there made Ardour work again. But it still might be some problem of the packagers, don't know.

If possible a different, maverick path for this .so would be a good way but I fear that this might become a can of worms regarding agreement between LMMS and distribution teams also.

@boomshop
Copy link
Author

boomshop commented Nov 6, 2017

I tested it as veal.so and it works fine. I've forked upstream and renamed it as well to be consistent.

I fear that just renaming the shared object will not solve the problem. Additionally the name you've chosen might make the problem worse since users might be confused what to search for/delete to make their Ardour/Calf work again.

@tresf
Copy link
Member

tresf commented Nov 6, 2017

Additionally the name you've chosen might make the problem worse since users might be confused what to search for/delete to make their Ardour/Calf work again.

This is moot if it fixes the issue.

a different, maverick path for this .so would be a good way but I fear that this might become a can of worms regarding agreement between LMMS and distribution teams also.

It's not a can of worms for us, but I don't know if it would help either. I believe the root of this issue stems from an old decision to start bundling up the LMMS LADSPA plugins in part due to the popular vocoder plugin being blindly (and irresponsibly) patched into swh. Here's the conversation history: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=758888

This has been fixed, but not until after @grejppi (and others) were reminding us about how bad this was.
At the time we couldn't improve it... we needed a capable DSP/LADSPA coder to implement this. Fortunately @jasp00 came along and completed the port to swh upstream, which Steve Harris graciously accepted, so since swh/ladspa#28 vocoder can be made available with swh-plugins now.

I fear that just renaming the shared object will not solve the problem.

From what I'm seeing Calf is the only conflict and it's also the only LADSPA lib that we "bundle" which needed to change the source code drastically to get to compile, so we can dig deeper into the renaming effort to avoid the conflict. Can you help explain what needs to be done to avoid this conflict? Does LV2 use the same plugin ID (if someone uses LADSPA for an Audacity project and then upgrades to an LV2 version, does the project still load?)

If the root of the problem is the underlying plugin IDs, we can try to shim an upgrade into LMMS to keep them playing nice together. I can do the upgrade LMMS-side, but I might need some help with the calf side.

Edit: Looks like we just store the library name. Should be pretty straight-forward for us. I don't think we track the LADSPAv1 ID anywhere although I can't guarantee the same for other software that's been relying on the LMMS ladspa bundle.

Project upgrade:

    <effect ... name="ladspaeffect">
       <key>
-         <attribute value="calf" name="file"/>
+         <attribute value="veal" name="file"/>
          <attribute value="Compressor" name="plugin"/>
       </key>

@tresf
Copy link
Member

tresf commented Nov 10, 2017

@boomshop here's our LADSPA-compatible calf library. We discussed quickly on discord and have decided to keep the name. This will cause issues with software previously using the LADSPA version of the calf plug-in which does not provide an LV2 project-upgrade path.

veal.so.tar.gz (x86_64 only)

I assume this inconvenience of potentially affecting LADSPA on other software is desired over the alternative. By "alternative", I mean "breaking Calf 0.90". We've written an upgrade routine for our software to handle this gracefully. We've also created our official fork of Calf at https://github.com/lmms/veal. If there are projects that rely on the LMMS LADSPA plugins we encourage them to do so as well.

At this time I would like you to validate this fix works and resolves the original issue. Any other bug reports submitted about affecting other software in regards to calf.so we will close as wontfix as a result of this decision.

I'd like to mention one major downside of all of this:

  • We are doing this in our master (bleeding-edge) branch.
  • We have not done this in our stable-1.2 (beta) branch
  • We have not done this in our stable-1.1(stale) branch.

What does this mean? Well, if Debian and friends rely solely on the state of stable for building these packages, this fix won't make it to the mirrors for a long time. If a backport is needed, the package maintainers need to do this themselves or they may coordinate it with us.

Please confirm that this technique works or send a link to this bug report to a person who can.

@boomshop
Copy link
Author

@tresf, thank you very much for the quick response/processing! I'll report back.

@boomshop
Copy link
Author

Hey!

The guy who initially reported the issue was unable to build/test things by now unfortunately due to some package bugs in Arch. The issue is reported, he's waiting for a reply.

Additionally I asked FalkTX (KXStudio Maintainer) if he could test it in his build environment. He wasn't able to reproduce the problem, nonetheless he thinks only renaming the binary isn't enough in this case - let me quote him:

ie, there's still a "calf_plugins" namespace there.

I don't need any testing to know that this will result in exactly the same issues as before, nothing has really changed.
They need to change the C++ namespace from calf_plugins to veal_plugins, or even better rename all mentions of "calf" to "veal".
Only then this has a chance of working properly.

So far, I'll report as soon as I have any more information.

Best
Markus

@tresf
Copy link
Member

tresf commented Nov 15, 2017

They need to change the C++ namespace from calf_plugins to veal_plugins, or even better rename all mentions of "calf" to "veal".

We should be able to handle that.

unable to build/test things
additionally [...] FalkTX wasn't able to reproduce the problem

Ok, we'll need a unit test to prove that this is fixed. We'll keep this bug open until a test build with the namespace changes has occurred, but reproducing the problem is always a good baseline for comparison when determining if a fix has occurred. 🍻

@boomshop
Copy link
Author

True, the user who came up with the bug needs to verify it at least. I'll let you know as soon as he's able to build from git. Thanks!

@tresf
Copy link
Member

tresf commented Nov 16, 2017

Name space change proposed here: LMMS/veal#1.

I understand this may cause confusion when a DAW has LADSPA and LV2 support as some plug-ins will be named similarly however Calf plug-ins append " LADPSA" to the plug-in name descriptor for differentiation.

Library attached. Testing appreciated.
veal.so.tar.gz

@boomshop
Copy link
Author

Awesome! For me it worked without a flaw. I forwarded it to the guy who initially reported the bug for testing, will report back asap.

@boomshop
Copy link
Author

He just dropped me a note that everything is fine on his box, i.e. Ardour sees both LV2 and LADSPA plugins and doesn't crash when adding any of those. Feel free to close this issue whenever you like.

Thank you very much, best
Markus

@tresf
Copy link
Member

tresf commented Nov 16, 2017

Ok, I've merged the namespace changes. We have a few items on our side:

  • Nudge submodule to latest commit
  • Regression test the plugin against old project files

@tresf
Copy link
Member

tresf commented Nov 17, 2017

Tests pass, nudged. Closed via d63cfe0. veal.so will build with the new namespace and the new library name from our master branch.

@tresf tresf closed this as completed Nov 17, 2017
@tresf
Copy link
Member

tresf commented Nov 17, 2017

Slightly off topic, but @JohannesLorenz is working on porting the 0.90 version to LADSPA on our veal fork. Here's some progress he's had.

image

@boomshop
Copy link
Author

Don't get me wrong but wouldn't it be much easier to implement LV2 capabilities into LMMS? I would guess most of the Calf plugins are close to unusable without their generic interface.

@boomshop
Copy link
Author

And thanks for the hint with the copyright information!

@tresf
Copy link
Member

tresf commented Nov 17, 2017

Don't get me wrong but wouldn't it be much easier to implement LV2 capabilities into LMMS? I would guess most of the Calf plugins are close to unusable without their generic interface.

Yes, in the long run, adopting LV2 will make Calf much easier for integration. It's also on the roadmap for offering a standard interface for our instrument plugins since we never added DSSI support.

In the short-term, we have an initiative to remove the hundreds of thousands of lines of code in our project that are provided solely by 3rd parties to 1. Clean up our project and 2. Encourage upstream patches (per #3963).

@JohannesLorenz is the one helping with the LADSPAv1 port of 0.90 so I guess I'd have to defer LMMS interfacing questions to him. It's not an area I'm familiar with. :)

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

No branches or pull requests

2 participants