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

Refactor Effect processing #7484

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Sep 2, 2024

All LMMS effects share some identical boilerplate code at the beginning and end of their process loop, such as an early return if the effect is not enabled and calling the checkGate method, but these tasks should not be the plugin developer's responsibility. So I created a wrapper method that handles these tasks and simplifies the process method for all effect plugins.

And along the way, I also made a few other minor changes.

Changes made:

  • Introduce processImpl and processBypassedImpl methods, and adapt each effect
    plugin to use them
  • Move plugin state checks, RMS out sum calculations, and checkGate calls to the processAudioBuffer wrapper method
    • Compressor and LOMM now use double instead of float for RMS out sum
    • checkGate now runs for GranularPitchShifterEffect
  • Minor changes to LadspaEffect
  • Remove dynamic allocations and VLAs from VstEffect's process method
  • Some minor style/formatting fixes

- Introduce `processImpl` and `sleepImpl` methods, and adapt each effect
plugin to use them
- Use double for RMS out sum in Compressor and LOMM
- Run `checkGate` for GranularPitchShifterEffect
- Minor changes to LadspaEffect
- Remove dynamic allocations and VLAs from VstEffect's process method
- Some minor style/formatting fixes
Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

We should probably remove the gate feature since it seems to generally be problematic for many plugins (as @LostRobotMusic mentioned), though I'm not sure what that would mean for backwards compatibility (maybe the risk is low enough since its rarely used I'm assuming?)

include/Effect.h Outdated Show resolved Hide resolved
include/Effect.h Outdated Show resolved Hide resolved
plugins/DualFilter/DualFilter.cpp Outdated Show resolved Hide resolved
plugins/DualFilter/DualFilter.cpp Outdated Show resolved Hide resolved
plugins/LadspaEffect/LadspaEffect.cpp Outdated Show resolved Hide resolved
plugins/StereoMatrix/StereoMatrix.cpp Outdated Show resolved Hide resolved
plugins/VstEffect/VstEffect.cpp Outdated Show resolved Hide resolved
plugins/VstEffect/VstEffect.cpp Outdated Show resolved Hide resolved
plugins/WaveShaper/WaveShaper.cpp Outdated Show resolved Hide resolved
plugins/DynamicsProcessor/DynamicsProcessor.cpp Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 3, 2024

I am planning to remove gate knob as a seperate PR. Informed it because there's chance of conflicts

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Functional review finished.

Outstanding reviews:

  • Style review (at least, I did not do it)
  • Testing review

When testing, different things should be tested:

  • Testing GranularPitchShifterEffect
  • Testing a LadspaEffect
  • Testing a Lv2Effect (if the respective comment will be changed)
  • Trying VstEffect with different (very high?) sampleRates

include/Effect.h Show resolved Hide resolved
plugins/Lv2Effect/Lv2Effect.cpp Outdated Show resolved Hide resolved
plugins/LadspaEffect/LadspaEffect.cpp Show resolved Hide resolved
plugins/LadspaEffect/LadspaEffect.cpp Show resolved Hide resolved
plugins/VstEffect/VstEffect.cpp Show resolved Hide resolved
include/AudioEngine.h Show resolved Hide resolved
@PhysSong
Copy link
Member

PhysSong commented Sep 5, 2024

I think there was a suggestion to handle W/D mix and check output level in single place, rather than letting each plugin handles them. This might be in the scope of this PR as well.

@PhysSong
Copy link
Member

PhysSong commented Sep 5, 2024

Also, for sleepImpl: I think something like processBypassedImpl will match what the operation is commonly called.

@messmerd
Copy link
Member Author

I think there was a suggestion to handle W/D mix and check output level in single place, rather than letting each plugin handles them. This might be in the scope of this PR as well.

I like that idea, though I'm not sure if it can be easily done while keeping in-place processing of the buf audio buffer. So that could be left to another PR if it is feasible.

@messmerd
Copy link
Member Author

I think all the reviews have been addressed now.

I was able to move the outSum calculations into the wrapper method, and introduced the ProcessStatus enum to allow plugins to specify what they want to happen after the process loop (continue, continue if not quiet, or sleep). I took this idea from the CLAP API. It makes the interface really clean and avoids the -1.0 magic value.

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Rework checked, OK from that point of view.
Style checked.
Testing still to be done.

include/Effect.h Outdated Show resolved Hide resolved
plugins/Eq/EqEffect.cpp Show resolved Hide resolved
plugins/StereoEnhancer/StereoEnhancer.cpp Outdated Show resolved Hide resolved
@bratpeki
Copy link
Contributor

I see this needs testing. I would be interested. How exactly does one test this?

@messmerd
Copy link
Member Author

@bratpeki This PR shouldn't change anything from a user's perspective, so testing would just involve using effects plugins (LMMS, VST, Ladspa, Lv2) and verifying that they still work as expected. If there are any regressions, it would probably affect the auto-off/gate

@JohannesLorenz
Copy link
Contributor

I see this needs testing. I would be interested. How exactly does one test this?

See also #7484 (review)

@bratpeki
Copy link
Contributor

Alright, I'll clone it, build it, and use it for a few days, reporting back on any mishaps, if they occur!

@bratpeki
Copy link
Contributor

LB302 seems to not produce sound.

@messmerd
Copy link
Member Author

LB302 seems to not produce sound.

It looks like it doesn't work on master either... strange. Not a bug from this PR fortunately

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

I'm currently looking into the regression reported by @bratpeki that seems to not be from this PR. This PR however looks good to me.

plugins/PeakControllerEffect/PeakControllerEffect.cpp Outdated Show resolved Hide resolved
plugins/StereoEnhancer/StereoEnhancer.cpp Outdated Show resolved Hide resolved
@bratpeki
Copy link
Contributor

bratpeki commented Sep 18, 2024

Also, please check if TripleOsc is louder than before.
This could just be my mind playing games with me!

It could also be unrelated to this PR, of course!

@messmerd
Copy link
Member Author

@bratpeki This PR doesn't affect any of the instrument plugins so you don't need to test them

@bratpeki
Copy link
Contributor

LADSPA works great, LV2 has also been tested, I'll look at some LinuxVSTs as well!

@bratpeki
Copy link
Contributor

I have done tested a few AirWindows LinuxVST plugins, and they sound perfectly fine! I've tested the audio with Air, Chorus, StereoChorus, ADClip, and a few other plugins from the suite.

I've left a somewhat detailed report and question on the Discord server.

In any case, the audio sounds just fine, so this LGTM, but I'll leave it to you. Please let me know if you would like me to conduct any more testing on this!

@messmerd
Copy link
Member Author

@bratpeki Thank you very much for testing! I'll look into what you mentioned on Discord, though I imagine it won't be an issue.

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

Successfully merging this pull request may close these issues.

6 participants