Lv2 core implementation #4899

JohannesLorenz · 2019-03-17T10:20:44Z

This is a first minimal Lv2 plugin, supporting the whole core, except for CV ports.
No features or extensions are supported yet.

What should work:

Generate sound using Lv2 plugins and effects
Any kind of automation
Correct stereo linking behavior
Instrument window's "Plugin" tab resizes to fit all knobs
Loading/Saving

What does not work yet:

After linking models, values are swapped instead of being the same #4904 Unlinking one stereo model, changing its values, then changing
values of a linked one, then clicking "Link Channels" does not really link
the first stereo model (bug imported from Ladspa) -> Will be fixed with next master merge.
~~Known Bug: Triple Oscillator has an orange stripe on its "Plugin" tab~~
~~Known Bug: Many old instruments windows are resized wrong~~
Known Bug: Instrument windows are resized wrong in some cases when using the arrows to switch instruments in the instrument window. This bug does only affect projects with at least one "variable sized instrument".
~~Known Bug: VST/Vestige cannot be instantiated~~

Code Reviewers:

@DomClark (functional)
@Spekular (style)

Feel free to split the review or just look at everything you're interested in.

Review hints:

Lv2Manger.h provides a full overview of the new Lv2 classes. It may be easier
to first look there.
Possible things to check may include memory issues or deviations from the Lv2 specs.

Testers:

@douglasdgi - Linux
@T0NIT0RMX - Windows

Testing hints:

Most CALF and some MDA effects work.
For testing an instrument, you can use the x42 testsignal generator (Instruments with piano are unsupported in the Lv2 core).
You can test scale points/combo boxes using the "MDA combo" effect.
Many plugins will not be recognized. The console logs them and shows reasons.

On advance, many thanks for reviewing and testing!

Add labeled controls for different types with a common base class

Implement a container for multiple equal groups of linked models and suiting views. Such groups are suited for representing mono effects where each Model occurs twice. A group provides Models for one mono processor and is visually represented with a group box. This concept is common for LADSPA and Lv2, and useful for any mono effect.

For an explenation about the new Lv2 classes, see Lv2Manager.h

JohannesLorenz · 2019-03-17T11:51:31Z

Sorry for the CI failure. It seems like it only requires replacing two assert calls with Q_ASSERT. I'll fix this ASAP.

JohannesLorenz · 2019-03-17T14:47:42Z

CI is working again (the left CI errors don't look code related) and I removed a lot of dead code so the code should get more readable.

Spekular

First pass cosmetic review, checked for rule Add a space after if, else if, else, for, do, while and switch.

plugins/Lv2Effect/Lv2Effect.cpp

plugins/Lv2Effect/Lv2FxControlDialog.cpp

src/gui/Lv2ViewBase.cpp

Spekular · 2019-04-04T12:00:00Z

There are some violations of the rule:

Do not add a space after opening brackets or a space before closing brackets.

If you interpret it strictly. Another rule in the conventions gives this example:
if (m_sample > 0) {--m_sample;}

Whereas your code occasionally uses this style:
if (m_sample > 0) { --m_sample; }

However, I can't bring myself to "correct" the latter in review as it looks far more readable to me and I question the original rule. When using explicitly blocked brackets, there should obviously not be spaces before/after. However, for one liners I feel the opposite is true, there should obviously be spaces between the brackets and the code itself.

Spekular

Finished my style review. A fair amount of places that don't use explicit blocking, so I added comments for that. Don't necessarily agree with that change either, feels like vertical space will balloon in some places if it's "fixed".

plugins/Lv2Effect/Lv2Effect.cpp

src/core/lv2/Lv2Ports.cpp

src/gui/Lv2ViewBase.cpp

@Spekular

Apply fixes according to coding conventions, thanks to @Spekular Co-Authored-By: JohannesLorenz <[email protected]>

JohannesLorenz · 2020-05-07T19:36:52Z

All comments solved again. I think the only one that might require a second look is the D/W fix 4c2a52a, but even that should be correct.

@DomClark @PhysSong Please let me know if there are more things to change or whether you are OK with the PR now.

JohannesLorenz · 2020-05-07T20:24:00Z

Thanks. This means all mandatory reviews are passed now.

After there have been so many changes and merges, I'd like to still ask someone to test it through again before we're going to master. Especially loading/saving.

I invited you, @douglasdgi , for now. Some testing hints are in the top post.

JohannesLorenz · 2020-05-07T20:28:25Z

One issue is that there's currently hundreds of lines of debug output coming when you start LMMS, like:

Lv2 plugin "LSP Spektrumanalysator x1" (URI: http://lsp-plug.in/plugins/lv2/spectrum_analyzer_x1 ) can not be loaded:
  -  unknown port type for mandatory port : UI Input
  -  unknown port type for mandatory port : UI Output
  -  required feature not supported : http://lv2plug.in/ns/ext/urid#map

I think I'll disable this unless the user sets an environment variable, like LMMS_LV2_DEBUG.

-> EDIT: Solved in bca38f0.

JohannesLorenz · 2020-05-16T23:34:29Z

Thanks to @douglasdgi for reporting: If you have installed lv2, but not lilv, you get

CMake Error at CMakeLists.txt:191 (IF):
  if given arguments:

    "1" "AND"

  Unknown arguments specified

The code line is IF(${LV2_FOUND} AND ${LILV_FOUND}) . Anyone an idea how to fix this?

LostRobotMusic · 2020-05-16T23:48:19Z

It would be good if the search bar in the effects was unselected upon clicking the top bar of the window (or really just anywhere in the window other than the search bar), so that the user can use their keyboard to play whatever instrument they have open without needing to switch windows.

PhysSong

@JohannesLorenz This will fix the reported issue.

CMakeLists.txt

@PhysSong

Fix CMakeLists.txt issues, thanks to @PhysSong Co-authored-by: Hyunjin Song <[email protected]>

JohannesLorenz · 2020-05-17T08:37:24Z

@douglasdgi I fixed it in 867973c . Can you please check if it's fixed well?

Do you plan to test more or are you finished with testing? (If you're done, you can approve by submitting a PR review)

CMakeLists.txt

Co-authored-by: Hyunjin Song <[email protected]>

This reverts commit 3862929.

This reverts commit 756b84e.

JohannesLorenz · 2020-05-23T17:14:40Z

All reviews successful. I'll try to merge it tomorrow.

JohannesLorenz · 2020-05-24T15:05:09Z

I did some git tricks to get the branch content with a single commit into master (2a66e83) while leaving this branch as it is, so I'm closing this branch now.

PhysSong · 2020-06-11T03:12:27Z

plugins/Lv2Effect/Lv2Effect.h

+
+private:
+	Lv2FxControls m_controls;
+	std::vector<sampleFrame> m_tmpOutputSmps;


This is invalid because sampleFrame is an array type. For some reason, libstdc++ allows it. This fails to build on macOS(reported on Discord), but the CI was not spotting this because you didn't add LV2 packages to macOS CI builds.

Can we redefine sampleFrame from sample_t[DEFAULT_CHANNELS] to std::array<sample_t, DEFAULT_CHANNELS>? The latter is generally recommended over plain arrays in modern C++ as it is much better behaved.

@DomClark That's what's done in #4994.

Will be fixed now in a new PR #5536 .

@Reflexe you will need to merge this branch (or rebase to master) - If you do, watch out for merge conflicts in lines marked "@recording".

... in order to make standard containers be able to store it. Required for #5532 (#4899) and the recording PR. This includes: * removing the `LocklessRingBuffer<sampleFrame>` specialization * passing samplerame in `StereoDelay::tick` as a reference Additional cleanups: * removing already unused typedef `sampleFrameA` * add some `const_cast` to make code more readable

... in order to make standard containers be able to store it. Required for LMMS#5532 (LMMS#4899) and the recording PR. This includes: * removing the `LocklessRingBuffer<sampleFrame>` specialization * passing samplerame in `StereoDelay::tick` as a reference Additional cleanups: * removing already unused typedef `sampleFrameA` * add some `const_cast` to make code more readable

JohannesLorenz added 12 commits March 16, 2019 15:39

Add PluginIssue class

0e516aa

Add Control classes

e43d68e

Add labeled controls for different types with a common base class

Merge branch 'instr-sub-plugins' into lv2-review

d20981c

Merge branch 'model-visitor' into lv2-review

f4d7f94

Merge branch 'linked-model-groups' into lv2-review

8fc9921

Merge branch 'plugin-issue' into lv2-review

577368f

Merge branch 'piano-optional' into lv2-review

3434bee

Merge branch 'variable-tab-widget' into lv2-review

f7b9d33

Merge branch 'misc-lv2-preparation' into lv2-review

d899cf7

Merge branch 'variable-tab-widget' into lv2

c623d2c

Lv2 core implementation

288e83b

For an explenation about the new Lv2 classes, see Lv2Manager.h

JohannesLorenz added the enhancement label Mar 17, 2019

JohannesLorenz added this to the 1.3.0 milestone Mar 17, 2019

Fix CI: assert -> Q_ASSERT

5dc7bf6

JohannesLorenz added 2 commits March 17, 2019 14:13

Fixed warning about uninitialized values

ba70b60

Remove a lot of dead code

ae8473c

JohannesLorenz added 2 commits March 22, 2019 23:04

VST Bugfix, stolen from master

2a28ded

Merge branch 'variable-tab-widget' into lv2-review

9b13377

This was referenced Mar 30, 2019

How to understand LMMS's internal plugins nature? #4916

Closed

Impulse Response Convolution Reverb Request #641

Closed

LV2 support #562

Open

Spekular requested changes Apr 3, 2019

View reviewed changes

Spekular reviewed Apr 4, 2019

View reviewed changes

Spekular and others added 2 commits April 4, 2019 18:33

Apply suggestions from code review

7961b25

Apply fixes according to coding conventions, thanks to @Spekular Co-Authored-By: JohannesLorenz <[email protected]>

Whitespace fixes

71e0951

DomClark approved these changes May 7, 2020

View reviewed changes

JohannesLorenz requested a review from LostRobotMusic May 7, 2020 20:24

Enable turning on/off Lv2 debug output

bca38f0

PhysSong reviewed May 17, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

JohannesLorenz and others added 2 commits May 17, 2020 08:11

Apply suggestions from code review

e443ce1

Fix CMakeLists.txt issues, thanks to @PhysSong Co-authored-by: Hyunjin Song <[email protected]>

Merge branch 'master' into lv2-review

867973c

tresf mentioned this pull request May 18, 2020

Provide a Flatpak bundle #4109

Open

PhysSong reviewed May 19, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

JohannesLorenz and others added 5 commits May 19, 2020 07:21

CMakeLists.txt: Improve WANT_SUIL description

39bfecd

Co-authored-by: Hyunjin Song <[email protected]>

Remove Lv2ViewProc::leaveEvent

3862929

Merge branch 'master' into lv2-review

d282952

Revert "Remove Lv2ViewProc::leaveEvent"

756b84e

This reverts commit 3862929.

Revert "Revert "Remove Lv2ViewProc::leaveEvent""

5602bb9

This reverts commit 756b84e.

LostRobotMusic approved these changes May 23, 2020

View reviewed changes

JohannesLorenz closed this May 24, 2020

PhysSong reviewed Jun 11, 2020

View reviewed changes

JohannesLorenz mentioned this pull request Jun 11, 2020

Add lv2 packages to Mac CI #5532

Merged

JohannesLorenz mentioned this pull request Oct 30, 2020

Support LV2 with MSVC #5730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lv2 core implementation #4899

Lv2 core implementation #4899

JohannesLorenz commented Mar 17, 2019 •

edited

Loading

JohannesLorenz commented Mar 17, 2019

JohannesLorenz commented Mar 17, 2019

Spekular left a comment

Spekular commented Apr 4, 2019

Spekular left a comment

JohannesLorenz commented May 7, 2020

JohannesLorenz commented May 7, 2020

JohannesLorenz commented May 7, 2020 •

edited

Loading

JohannesLorenz commented May 16, 2020

LostRobotMusic commented May 16, 2020

PhysSong left a comment

JohannesLorenz commented May 17, 2020 •

edited

Loading

JohannesLorenz commented May 23, 2020

JohannesLorenz commented May 24, 2020

PhysSong Jun 11, 2020

DomClark Jun 11, 2020

PhysSong Jun 12, 2020

JohannesLorenz Jun 13, 2020

Lv2 core implementation #4899

Lv2 core implementation #4899

Conversation

JohannesLorenz commented Mar 17, 2019 • edited Loading

JohannesLorenz commented Mar 17, 2019

JohannesLorenz commented Mar 17, 2019

Spekular left a comment

Choose a reason for hiding this comment

Spekular commented Apr 4, 2019

Spekular left a comment

Choose a reason for hiding this comment

JohannesLorenz commented May 7, 2020

JohannesLorenz commented May 7, 2020

JohannesLorenz commented May 7, 2020 • edited Loading

JohannesLorenz commented May 16, 2020

LostRobotMusic commented May 16, 2020

PhysSong left a comment

Choose a reason for hiding this comment

JohannesLorenz commented May 17, 2020 • edited Loading

JohannesLorenz commented May 23, 2020

JohannesLorenz commented May 24, 2020

PhysSong Jun 11, 2020

Choose a reason for hiding this comment

DomClark Jun 11, 2020

Choose a reason for hiding this comment

PhysSong Jun 12, 2020

Choose a reason for hiding this comment

JohannesLorenz Jun 13, 2020

Choose a reason for hiding this comment

JohannesLorenz commented Mar 17, 2019 •

edited

Loading

JohannesLorenz commented May 7, 2020 •

edited

Loading

JohannesLorenz commented May 17, 2020 •

edited

Loading