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

Enable LADSPA plugins on MSVC #6758

Closed
wants to merge 0 commits into from
Closed

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Jul 2, 2023

Title says it. I started from #6240 worked on top of it.

note: From my research, I found that some of these plugins have enabled msvc support upstream, so I would suggest to downstream some of these changes. (thanks to ghost (who apparently deleted his acct) for the upstream changes).

Todo:

  • Get plugins to build
    • calf
    • caps
    • cmt
    • swh
    • tap

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the sys/time.h replacements at all? I can't see anything from that header being used anywhere outside some commented-out code in plugins/LadspaEffect/caps/interface.cc.

plugins/LadspaEffect/calf/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/CMakeLists.txt Outdated Show resolved Hide resolved
include/sys/Time.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/interface.cc Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/Eq.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/Eq.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/SVF.h Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
include/sys/Time.h Outdated Show resolved Hide resolved
@dan-giddins dan-giddins mentioned this pull request Jul 2, 2023
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jul 2, 2023

Thanks for your review Dom.

Do you need the sys/time.h replacements at all? I can't see anything from that header being used anywhere outside some commented-out code in plugins/LadspaEffect/caps/interface.cc.

Apparently, after removing the entire thing, it's still building fine (had to comment out the #include though).

I also took a look at most of your concerns and fixed most of those. I'll look at the rest later.

Another problem with this pr is that the plugins build but don't load in the selecter menu (does this got anything to do with the __attribute__ changes?)

@DomClark
Copy link
Member

DomClark commented Jul 2, 2023

had to comment out the #include though

I'd just remove it entirely, along with the commented out seed function. It's not adding any value sitting there, and it'll be in the Git history if we ever need to look back at it.

Another problem with this pr is that the plugins build but don't load in the selecter menu (does this got anything to do with the __attribute__ changes?)

That would be my guess too. Looking at the code in caps_so_init, it appears to be populating an array with information about the plugins inside. If that function doesn't run, the plugin information won't be available.

@Rossmaxx Rossmaxx mentioned this pull request Jul 5, 2023
2 tasks
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jul 5, 2023

For calf and tap, fixes already implemented in the master branches but need to update submodules. I'll go into more detail in #6765

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jul 7, 2023

opened a pr upstream in swh/ladspa#84

update : it's merged

This was referenced Jul 7, 2023
@theGreatWhiteShark
Copy link

@Rossmaxx awesome work! Have you thought about bundling those MSVC powered LADSPA plugins separately? Other Linux-based applications targeting Windows would definitely profit as well. (Availability of LADSPA plugins on Windows is one of main recurring requests at Hydrogen).

In addition, your list of plugins is currently missing lsp plugins. It's a more or less recent collection and maybe the best out there yet. It mainly targets LV2 but a lot of those plugins are available as LADSPA as well.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jul 9, 2023

@Rossmaxx awesome work!

Thanks.

Have you thought about bundling those MSVC powered LADSPA plugins separately?

It's upto the plugin maintainers to choose. I am upstreaming the changes. Swh has merged my changes too. I'll consider later if more requests come.

In addition, your list of plugins is currently missing lsp plugins.

It's not bundled within lmms either. I am targeting only the bundled ones. I might work on it seperately in the future but can't say anything now.

@theGreatWhiteShark
Copy link

It's upto the plugin maintainers to choose.

Well, depends on their license. But most - if not all - of them are GPL 2(+) licensed which means you do not have to ask for permission to distribute (or modify) the sources and artifacts.

It's not bundled within lmms either. I am targeting only the bundled ones. I might work on it seperately in the future but can't say anything now.

You do not ship them? Anyway. Maybe I will bundle them at some point when I have more time at hand.

@Rossmaxx Rossmaxx marked this pull request as ready for review July 12, 2023 13:46
@Rossmaxx
Copy link
Contributor Author

This pr is now ready.

@Rossmaxx Rossmaxx mentioned this pull request Jul 12, 2023
15 tasks
@DomClark DomClark self-requested a review July 12, 2023 15:33
@tresf tresf mentioned this pull request Jul 13, 2023
plugins/LadspaEffect/caps/interface.cc Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/interface.cc Outdated Show resolved Hide resolved
@tresf
Copy link
Member

tresf commented Jul 16, 2023

On MacOS Ventura, I'm getting the following compilation error:

In file included from /Users/owner/lmms/plugins/LadspaEffect/calf/veal/src/organ.cpp:24:
- /Users/owner/lmms/plugins/LadspaEffect/calf/veal/src/calf/organ.h:160:19: error: no matching member function for call to 'lerp_table_lookup_float_mask'
-        return ph.lerp_table_lookup_float_mask(data, ORGAN_BIG_WAVE_SIZE - 1);

@PhysSong
Copy link
Member

@tresf I also see this:

<redacted>/lmms/plugins/LadspaEffect/calf/veal/src/calf/fixed_point.h:238:14: note: candidate template ignored: substitution failure [with U = float]: array size is not a constant expression
    inline U lerp_table_lookup_float_mask(U data[(1U<<IntBits)+1], unsigned int mask) const {

I found the faulty change: https://github.com/calf-studio-gear/calf/pull/212/files#diff-a1c8f62ee19cef03296fa3410166e427bd1b151344594675902c2652846d31eaL232-R232
Reverting those changes fixes the error. I'll check if it breaks MSVC.

@PhysSong
Copy link
Member

Reverting those changes fixes the error. I'll check if it breaks MSVC.

Seems like it doesn't break 64bit builds, but 32bit builds fail with this error message:

error C2148: total size of array must not exceed 0x7fffffff bytes

plugins/LadspaEffect/tap/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/swh/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/Eq.h Outdated Show resolved Hide resolved
tresf added a commit to tresf/lmms that referenced this pull request Jul 23, 2023
@tresf tresf mentioned this pull request Jul 23, 2023
tresf added a commit to tresf/lmms that referenced this pull request Jul 31, 2023
tresf added a commit that referenced this pull request Aug 3, 2023
@DomClark DomClark linked an issue Aug 20, 2023 that may be closed by this pull request
michaelgregorius added a commit to michaelgregorius/lmms that referenced this pull request Sep 11, 2023
Cherry-pick some changes from PR LMMS#6758 to keep MacOS building.
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build fails unless Perl is available because plugins/LadspaEffect/CMakeLists.txt checks the WANT_* variables instead of the LMMS_HAVE_* variables. While that bug wasn't introduced here, it would be good if you could fix it.

CMakeLists.txt Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

The build fails unless Perl is available because plugins/LadspaEffect/CMakeLists.txt checks the WANT_* variables instead of the LMMS_HAVE_* variables. While that bug wasn't introduced here, it would be good if you could fix it.

Check line 284 and 285 of root cmakelists.

@Rossmaxx
Copy link
Contributor Author

Did a minor fix to address the missing perl problem.

CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/Sine.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/SVF.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/OnePole.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/dsp/Eq.h Outdated Show resolved Hide resolved
plugins/LadspaEffect/tap/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/LadspaEffect/caps/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file supposed to be here? It looks like it may be a local configuration file that accidentally was added. If not, it seems like it will constantly be outdated with paths such as C:/Qt/5.15.2/msvc2019_64, C:/Strawberry/perl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dan-giddins that's a visual studio ide config file. Also, the commits you pushed now look ambiguous.

@tresf
Copy link
Member

tresf commented Nov 6, 2023

I see that the submodule for src/3rdparty/mingw-std-threads was updated. Is that intended? Does MSVC use this?

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Nov 6, 2023

@tresf that change came after the push by dan. That's not supposed to be there.

@dan-giddins
Copy link

Sorry, I did not mean to push commits to this branch! I'm not even sure why I have rights to do this, or how I did it...

@tresf
Copy link
Member

tresf commented Nov 6, 2023

I'm not even sure why I have rights to do this, or how I did it...

It's because GitHub -- by default -- allows project members to to push to PR'd branches -- and you're a project member.

Sorry, I did not mean to push commits to this branch!

Stuff happens. :). The more eyes the better! 🍻

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Nov 6, 2023

wait what, why did this get closed now?

@tresf
Copy link
Member

tresf commented Nov 6, 2023

Bad merge. It's OK, you'll just have to open a new PR once fixed.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Nov 6, 2023

ahh, i messed up the force push

@Rossmaxx Rossmaxx deleted the enable-ladspa branch November 6, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get all plugins to be compataible with MSVC
8 participants