Lv2 support2#1240
Conversation
|
This is very exciting, can't wait to use my CALF reverb plugin live. |
|
Nice! Looks like a pretty small set of changes on the whole. lilv support is waiting for a contributor to pick it up -- there's nothing blocked on me now that buildserver development is automated by pull request: Maybe @sblaisot would be interested in the windows side of that? |
|
This does not build with #1092 merged: |
|
Thank you for working on this, but I am doubtful it should be rushed for 2.1. Maybe we can hide it behind a compile option that is disabled by default like the OpenGL ES waveform renderer. There's still quite a lot of polishing to do:
|
|
I will try working on lilv on the windows build server in the next weeks. |
|
Now the parameter slot assignment is refreshed for loaded effects as well. @be: What could be a solution? We need a way, to store the values of all parameters in the effect.xml file. In addition we need to store the blacklistings. It is also a question what should happen with the hidden parameters. Do we need to store a default, or is the value per effect slot in effect.xml sufficient? All this is probably to much to be part of 2.1, but I just want to be sure the effect.xml format is settled. |
|
Hmm, interesting question. I'm not sure. The way I was thinking blacklisting parameters would be used would be to hide parameters whenever that effect is loaded into any effect slot. In that case, effects.xml would not be an appropriate place to store the blacklist. On the other hand, it may be desirable to have some parameters blacklisted when exporting/importing chains. |
There was a problem hiding this comment.
There were a few more changes in f6403a2 to revert.
| lilv_instance_connect_port(m_handle, audioPortIndices[2], m_outputL); | ||
| lilv_instance_connect_port(m_handle, audioPortIndices[3], m_outputR); | ||
|
|
||
| lilv_instance_activate(m_handle); |
There was a problem hiding this comment.
This is not the correct place to call this function. The LV2 plugin's activate() function needs to be called before LV2EffectProcessor::process whenever the effect is enabled. We could hack this into the existing LV2EffectProcessor::process function by using the EnableState like native effects, but I think it would be a better idea to add an activate() method to the EffectProcessor API and refactor the native effects to use it. Then we might be able to get rid of the intermediate ENABLING and DISABLING EffectProcessor::EnableStates.
|
|
||
| // We assume the audio ports are in the following order: | ||
| // input_left, input_right, output_left, output_right | ||
| lilv_instance_connect_port(m_handle, audioPortIndices[0], m_inputL); |
There was a problem hiding this comment.
Can this handle effects with mono inputs and outputs?
| if (lilv_plugin_is_replaced(plug)) { | ||
| continue; | ||
| } | ||
| LV2Manifest* lv2Manifest = new LV2Manifest(plug, m_properties); |
There was a problem hiding this comment.
This should use lilv_plugin_has_latency to filter out plugins that add latency. We cannot compensate for latency in effects units with switchable input paths because we cannot suddenly switch which channels are being delayed during playback.
There was a problem hiding this comment.
lilv_plugin_has_latency just tells us if the Plug-In reports latency, not if it introduces it.
There is nothing wrong to use a Plug-In that introduces Latency if the Mix knob sticks on fully wet. This is also the case for all our fidlib based effects, which introduce ad group delay.
There was a problem hiding this comment.
Mixxx can't do anything about plugins that add latency but do not report it. Plugins that add latency are problematic because activating them will create an audible gap in the signal. Even if the user is careful to only (de)activate such a plugin when the volume fader is down, it would create an odd situation to manipulate different signal paths with different latencies and throw off the alignment of waveforms.
There was a problem hiding this comment.
It depend on the amount of latency it will add. Currently we fade from the wet to the dry signal by one buffer size. This should cover all known delays. Do you know a plug-In that has a longer latency?
| QList<int> controlPortIndices) { | ||
| m_sampleRate = ControlObject::get(ConfigKey("[Master]", "samplerate")); | ||
|
|
||
| m_handle = lilv_plugin_instantiate(plugin, m_sampleRate, NULL); |
There was a problem hiding this comment.
A separate instance of the plugin for each potential input channel is necessary. When multiple inputs are routed to an effect unit with an LV2 plugin loaded, the state is shared between the two channels which produces distorted sounds.
…1 already existing). Removed 1 obsolete entries
…ware/mixxxdj/github-release-210/. Compile QM files out of TS files that are used by the localized app
Hercules P32: add option to toggle effect units with decks
… add <decription> tags for mixer, FX, looping sections etc.
…date Reloop TerminalMix 2/4 :: mapping update
Tango: change name of skin menu "Extras" to "Misc"
Latenight fix: group FX Buttons in deck
Conflicts: .travis.yml src/effects/effect.cpp src/effects/effect.h src/effects/effectchainmanager.h src/effects/effectinstantiator.h src/effects/effectmanifest.h src/effects/effectrack.cpp src/effects/native/autopaneffect.cpp src/effects/native/balanceeffect.cpp src/effects/native/biquadfullkilleqeffect.h src/effects/native/bitcrushereffect.cpp src/effects/native/echoeffect.cpp src/effects/native/echoeffect.h src/effects/native/filtereffect.cpp src/effects/native/flangereffect.cpp src/effects/native/loudnesscontoureffect.cpp src/effects/native/phasereffect.cpp src/effects/native/reverbeffect.cpp src/effects/native/threebandbiquadeqeffect.h src/engine/effects/engineeffect.cpp src/engine/effects/engineeffect.h src/mixxx.cpp src/preferences/dialog/dlgpreferences.cpp src/preferences/dialog/dlgpreferences.h src/test/metaknob_link_test.cpp src/test/nativeeffects_test.cpp
|
This one is head up with master again. It was a lot of work to put it back to this state. It is odd to let it rod in a PR again. It works well, but has issues in presenting the LV2 effects in the GUI. |
There was a problem hiding this comment.
Thanks for resolving the numerous merge conflicts. I have looked briefly at the code but not done a very thorough review yet. There are still many UX quirks that make this not ready to expose to users yet:
- There needs to be a way to hide effects. Without this, the list of effects to select can easily grow overwhelmingly big. Also, some detected LV2 effects may be useless. For example, I loaded Gx-DelayStereo and it just made a horrible buzzing noise.
- There is no way to see plugins' GUIs. Metering and other analysis plugins are useless without being able to see the plugin's GUI. For some plugins, it is not clear how to use their parameters without seeing the plugin's GUI.
- Our skins currently only support binary effect button parameters but LV2 parameters can have more than 2 states. I am not sure these should be button parameters. It may work better to create a new parameter type that is shown with a new QComboBox subclass.
- Blacklisted parameters are coupled for every instance of an effect. This would be awkward with saving & loading chain presets. Instead, hidden parameters and parameter order should be independent for every instance of an effect. The default parameters shown for an effect and their order should be stored as part of the user's stored default snapshot for the effect along with parameter values and metaknob linkings. This does not need to be done for the initial merge of this branch. @kshitij98 could refactor how parameter hiding is implemented as part of his GSOC project.
- The LV2 and Effects preference pages are redundant.
I think we could merge this for 2.2 if you implement at least blacklisting effects.
|
|
||
| // TODO: implement this for all subclasses and call it when the buffer | ||
| // size and sample rate are configured by SoundManager | ||
| virtual void engineParametersChanged(const mixxx::EngineParameters& bufferParameters) { |
There was a problem hiding this comment.
It was unused.
We cannot change the buffer parameter on the fly all over the code. And I am in doubt if we ever need this.
If we, we can implement it with a fresh eye on this.
There was a problem hiding this comment.
Of course it was unused, that's why it was marked with a TODO comment.
We cannot change the buffer parameter on the fly all over the code.
This is a requirement for efficient memory use. Mixxx currently wastes memory allocating buffer sizes larger than the vast majority of users will use and there are lots of buffers in Mixxx. Let's work towards fixing this before we run into another scaling issue like we did in #1254.
Please bring back this deleted code with the TODO comment.
There was a problem hiding this comment.
It is not a good practice to keep unused code just as a reminder for a TODO.
I have just filed a bug https://bugs.launchpad.net/mixxx/+bug/1770186
| EngineEffect effect(pManifest, registeredChannels, pInstantiator); | ||
| ======= | ||
| EngineEffect effect(manifest, activeInputChannels, pEffectsManager, pInstantiator); | ||
| >>>>>>> upstream/master |
| } | ||
|
|
||
| if (!pState) { | ||
| SampleUtil::copyWithGain(pOutput, pInput, 1.0, bufferParameters.samplesPerBuffer()); |
There was a problem hiding this comment.
Isn't this redundant with the above VERIFY_OR_DEBUG_ASSERT?
There was a problem hiding this comment.
No, pState ist written inside the block above.
|
@rryan are the build servers set up with lilv? |
| LV2Manifest* lv2manifest = m_registeredEffects[effectId]; | ||
|
|
||
| return EffectPointer( | ||
| new Effect( |
There was a problem hiding this comment.
Please replace tabs with spaces.
| #include "control/controlobject.h" | ||
| #include "util/sample.h" | ||
|
|
||
| #define MAX_BUFFER_LEN 160000 |
There was a problem hiding this comment.
Replace this with #include "util/defs.h"
| LV2Backend* pLV2Backend = new LV2Backend(m_pEffectsManager); | ||
| m_pEffectsManager->addEffectsBackend(pLV2Backend); | ||
| #else | ||
| LV2Backend* pLV2Backend = 0; |
| // Initialize preference dialog | ||
| m_pPrefDlg = new DlgPreferences(this, m_pSkinLoader, m_pSoundManager, m_pPlayerManager, | ||
| m_pControllerManager, m_pVCManager, m_pEffectsManager, | ||
| m_pControllerManager, m_pVCManager, pLV2Backend, m_pEffectsManager, |
There was a problem hiding this comment.
extra space after m_pVCManager,
|
Nope: mixxxdj/buildserver#15
…On Sat, May 12, 2018, 9:15 AM Be ***@***.***> wrote:
@rryan <https://github.com/rryan> are the build servers set up with lilv?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1240 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABnn5viU-O7kmYb2fWHR8S8AGz7v30vks5txwqXgaJpZM4M_8Rr>
.
|
| if not conf.CheckForPKG('lilv-0', '0.5'): | ||
| raise Exception('Missing liblilv-0 (needs at least 0.5)') | ||
|
|
||
| build.env.Append(CPPDEFINES='__LILV__') |
There was a problem hiding this comment.
It looks like there is no way to compile with LV2 support on Windows?
There was a problem hiding this comment.
Why do you think that?
Ardour has LV2 support under windows.
|
|
|
Compilation warning: |
|
I am merging this to the upstream lv2_support2 branch and will be following up with another PR targeted at that branch. |
17 participants
This PR adds basic LV2 support to Mixxx. It integrates seamlessly with our current Effects Framework. Just install an LV2 plugin and it will show up when you cycle through the effects.
Currently it is only supporting plugins which handle stereo input/output audio samples and do not require any additional features.
This is the successor of
#316
IMHO this is in a mergeable state, because if a user has no LV2 plug-in installed, he will not noticed this branch at all. There are some pending shortcomings when we look at the visual integration and customize the interface of some plug-Ins but that are no stopper for me.
Lets merge it for now to stop rotting of this already useful branch. If it tuns out that it introduces issue for some regular users, we can opt it out from our release builds.
What do you think?
@rryan: What is required to add lilv to our buildservers?