-
Notifications
You must be signed in to change notification settings - Fork 269
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
Midi support #110
Midi support #110
Conversation
Thanks so much @robclouth! This looks like a great starting point - I'll take some time to review the changes in more detail soon. Very glad to see that it's possible to add basic MIDI message support in under 200 lines changed. |
@@ -71,28 +71,28 @@ PYBIND11_MODULE(pedalboard_native, m) { | |||
m.def("process", process<float>, | |||
"Run a 32-bit floating point audio buffer through a list of Pedalboard " | |||
"plugins.", | |||
py::arg("input_array"), py::arg("sample_rate"), py::arg("plugins"), | |||
py::arg("input_array"), py::arg("midi_messages"), py::arg("sample_rate"), py::arg("plugins"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think we can change this API like this; lots of user code passes sample_rate
as the second argument to process
(or __call__
) without specifying its keyword argument name. If we merge this, all of that user code will break as it will interpret the second argument as midi_messages
.
This change also makes it mandatory to pass MIDI messages every time a plugin is invoked. If we move this to the last argument, we can also make it optional and provide None
or []
as a default value, to avoid adding complexity for those who don't use this feature.
numpy | ||
midi==1.2.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- I don't think this import is actually used (we seem to
import mido
instead ofimport midi
elsewhere) - I'm pretty hesitant to add another runtime dependency if we can avoid it; MIDI messages are fairly simple to represent (as a five-tuple of
(type, note, channel, velocity, time)
) and we could use duck typing to accept objects from themido
package (or themidi
package) without adding them as a runtime dependency.
namespace Pedalboard { | ||
|
||
juce::MidiMessageSequence | ||
copyPyArrayIntoJuceMidiMessageSequence(const py::array_t<float, py::array::c_style> midiMessages) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper seems to re-interpret floating-point data into integer data, which seems a bit risky and confusing. Given that MIDI messages are probably going to be sparse (i.e.: not nearly as large as audio data) we could probably just use a std::vector<std::tuple<int, int, int, float>>
or could create our own thin wrapper class and use that (i.e.: std::vector<pedalboard.MIDIMessage>
).
|
||
midiSequence.addEvent(juce::MidiMessage(byte1, byte2, byte3), timeSeconds); | ||
|
||
DBG( byte1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBG( byte1 ); |
|
||
if (pluginInstance) { | ||
juce::MidiBuffer emptyMidiBuffer; | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
; |
@psobot Hey there, whats the current status on this PR? |
Hi @rhelsing! The ball is in @robclouth's court at the moment - although anybody else is also welcome to create a similar branch (optionally based on this one) and make a new pull request. I'm not directly working on adding MIDI at the moment, but as usual, would be happy to review others' contributions. |
Hey. I won't be continuing on this PR. I got it to where I needed it and put it here in case anyone else found it useful. If anyone wants to continue feel free. |
hi @psobot et al. i'm trying to get @robclouth's MIDI branch working with the example rob posted. Seems like a holy grail for people like me who want to render midi to audio faster than real time. ( I'm not a pybind programmer so don't get ur hopes up. ) Sadly, i'm getting an error on this branch at the load_plugin phase (upstream of the MIDI input).
This... would make sense given Vital is a VST MIDI instrument, not an audio effect VST. But I also get the same error for loading a reverb: load_plugin("/Library/Audio/Plug-Ins/VST3/ValhallaShimmer.vst3") The issue is similar to #118 , but I'm on Mac OS 10.15, not Windows. Anyway, these are the steps I used to build Rob's branch on my machine. Maybe I'm doing something wrong?? #basic build, but with the midi branch
git clone --recurse-submodules --shallow-submodules https://github.com/robclouth/pedalboard.git
cd pedalboard
pip3 install pybind11
pip3 install .
# required for this implementation
pip3 install mido
# not sure if needed
git fetch
# necessary to get to the branch with midi support
git switch midi-support
any ideas or workarounds? |
@TylerMclaughlin Note that the issue you mentioned (#118) was fixed on the I'd recommend either merging from |
Closing in favour of #225, which has been merged into Pedalboard v0.7.4. |
Summary: I've started a basic implementation of MIDI support to get things started
Problem
Pedalboard doesn't currently support plugin instruments because there is no way of sending MIDI to the plugins
Solution
I've added very basic MIDI support using juce::MidiMessageSequence and juce::MidiBuffer.
Result
This is mostly to get the ball rolling with MIDI support, it's definitely not ready for prime-time and haven't rigorously followed the contribution guide. It works for single processors, but not chains because the note off doesn't seem to work for chains. I haven't written any tests because I'm reckless. It's got to where I need it so I'll give it a rest for while. If someone wants to pick up the baton feel free!
For now I've done the following:
E.g.