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

Allow Pedalboard instances to be used as plugins. #68

Merged
merged 16 commits into from
Feb 9, 2022

Conversation

psobot
Copy link
Member

@psobot psobot commented Jan 26, 2022

Oh boy, this is a big one. This PR:

  • Adds the concept of a PluginContainer on the C++ side to represent a plugin that internally delegates to zero or more Plugin instances.
  • Adds two subclasses of PluginContainer:
    • Mix, which mixes the outputs of multiple Plugin instances in parallel
    • Chain, which runs plugins in series (as the Pedalboard class does now)
  • Makes the Pedalboard class a subclass of Chain, allowing it to be used as a plugin itself
  • Adds new examples to the README
  • Removes the sample_rate parameter from Pedalboard instances, which may be a breaking API change for some users
  • Changes the Python interface to use std::shared_ptr under the hood (to allow for better alignment with Python's reference counting)

@psobot psobot added the enhancement New feature or request label Jan 26, 2022
@psobot psobot mentioned this pull request Jan 26, 2022
Base automatically changed from psobot/delay-plugin to master January 28, 2022 16:51
@psobot psobot force-pushed the psobot/nested-plugin-chains branch from 12afd91 to 83a5bbe Compare February 3, 2022 22:57
}

protected:
std::vector<std::shared_ptr<Plugin>> plugins;
Copy link
Contributor

Choose a reason for hiding this comment

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

So the use of shared_ptr here may turn into a performance penalty since shared_ptrs come with thread locks. If you can pass around Plugin& and initialize from your PluginRegistry, you may have a better time (pun intended)

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd definitely remove the need for a shared_ptr here, but we don't actually have a PluginRegistry or similar - plugins' lifetimes are managed in Python, and the switch to shared_ptr here was in order to bridge more cleanly with Python's reference counting semantics.

My understanding is that shared_ptr only uses a lock when changing the reference count, though; and in our case, that should only ever happen in three places:

  • in the constructor of each plugin (implicitly via pybind11 when returning from py::init)
  • in Python (when objects fall out of scope)
  • when modifying the list of plugins in a PluginContainer (which should only be triggered by a Python operation to begin with)

We do release the GIL to allow for multithreaded processing when actually rendering audio, but at that point, we're not going to trigger any of the three situations above; hence this shouldn't cause any locks to get hit, and we should end up with no performance penalty.

@psobot psobot merged commit 57c8351 into master Feb 9, 2022
@psobot psobot deleted the psobot/nested-plugin-chains branch February 9, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants