Add NoteExpression and MIDI event support - #22
Conversation
Allows plugins (especially synths) to react on standard CLAP NoteExpressions and MIDI messages (as e.g. requeired for MPE support). Event capabilities, receiving is gated via compile time consts in Plugin in order to avoid overhead. Also extract raw MIDI-byte parsing into a new shared helper function, used by both the CLAP MIDI event path and the standalone host's MIDI input.
| note: event.note_id, | ||
| semitones: event.value, | ||
| } | ||
| // Covert raw MIDI bytes to CC / channel pressure / pitch bend / poly pressure events. |
| midi_ids.program_change = Some(alloc_block(&mut parameter_infos, &|channel| format!("MIDI Channel {} Program Change", channel + 1))); | ||
| } | ||
|
|
||
| for cc in P::MIDI_CAPABILITIES.enabled_midi_control_changes() { |
There was a problem hiding this comment.
Nitpick, but my feeling is that this name should follow the other capabilities and drop the "enabled" prefix
| }, | ||
| ]; | ||
|
|
||
| fn fill_note_expression_info(note_expressions: NoteExpressions, index: i32, info: &mut NoteExpressionTypeInfo) -> bool { |
There was a problem hiding this comment.
Since this is an internal function, I would rather have the index be a usize and do the checking before calling this function
| kInvalidArgument | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This module is getting quite long so I would move the expression code to a separate module
| unsafe fn getNoteExpressionStringByValue(&self, bus_index: int32, channel: int16, id: NoteExpressionTypeID, value_normalized: NoteExpressionValue, string: *mut String128) -> tresult { | ||
| tracing::trace!("INoteExpressionController::getNoteExpressionStringByValue"); | ||
|
|
||
| if P::NOTE_EXPRESSIONS.is_empty() { |
There was a problem hiding this comment.
Seems to me wrong to be checking for empty here instead of checking below if the specific expression is enabled?
| unsafe fn getNoteExpressionValueByString(&self, bus_index: int32, channel: int16, id: NoteExpressionTypeID, string: *const TChar, value_normalized: *mut NoteExpressionValue) -> tresult { | ||
| tracing::trace!("INoteExpressionController::getNoteExpressionValueByString"); | ||
|
|
||
| if P::NOTE_EXPRESSIONS.is_empty() { |
| } | ||
| } | ||
|
|
||
| fn lookup(&self, note_id: i32) -> Option<(i16, i16)> { |
There was a problem hiding this comment.
get is the more Rustic function name. Should also document what it's returning
| } | ||
|
|
||
| fn lookup(&self, note_id: i32) -> Option<(i16, i16)> { | ||
| for i in 0..self.count { |
There was a problem hiding this comment.
Linear lookup here seems quite wasteful especially as entires are never removed, so in practice this will always go through all the entries, even obsolete ones once 128 notes have been pressed. Why not a HashMap or BTreeMap?
|
|
||
| // Allocate one 16-channel block, pushing hidden ParameterInfos. | ||
| let mut alloc_block = |infos: &mut Vec<ParameterInfo>, name_fn: &dyn Fn(usize) -> String| -> [ParameterId; 16] { | ||
| let mut block = [0u32; 16]; |
There was a problem hiding this comment.
Even though it will never change, would be better to use a constant for number of MIDI channels instead of a magic number
| /// Each block holds 16 hidden parameters, one per MIDI channel. | ||
| #[derive(Default)] | ||
| pub(super) struct MidiParameterIds { | ||
| pub pitch_bend: Option<[ParameterId; 16]>, |
There was a problem hiding this comment.
Even though it will never change, would be better to use a constant for number of MIDI channels instead of a magic number
|
Thanks for the PR! Sorry it took a while to review, I've been quite busy. I like the basic idea but I had a bunch of nitpicks and a slightly larger worry about the way the note ID map is implemented. I would also just rename the |
|
No problem. I've been away and busy with other things as well. I'll completely rework the note_expression impl and move them into a separate file. Also wasn't happy with this. Thanks for checking and the feedback.
The NoteMap does not release its entries on note-offs, because note expressions also need to be handled for stopped but still sounding notes. See https://steinbergmedia.github.io/vst3_dev_portal/pages/Technical+Documentation/Change+History/3.5.0/INoteExpressionController.html?highlight=kNoteExpressionValueEvent#how-does-it-work ( It's not a Further, there are max 128 comparisons only in the worst case. Newest entries are scanned first in the usual case. So in practice, I think this likely will be faster or as fast as generating hashes and the other stuff a hashmap or btree does. Would need to profile that to be sure though. Let me know if you think that's worth evaluating. So I'd keep a flat list as map here, but using an Will add consts for Once all this is done, I'll check how much other code the |
Fair points. I don't see how newest entries would be scannest first though as write index is constantly increased and wraps around but scanning always starts at 0. Performance concerns might be premature but it still feels potentially wasteful. Maybe check FnvIndexMap in heapless since the map is fixed capacity?
This sounds fine.
Also makes sense. |
|
Using
I was talking about the new Alternatively, we could also skip the If we remove the
The passed id, channel, key values in CLAP actually are all optional already. So plugin impls do need to do some kind of lookup anyway. From the CLAP docs: // Clap addresses notes and voices using the 4-value tuple
// (port, channel, key, note_id). Note on/off/end/choke
// events and parameter modulation messages are delivered with
// these values populated.
//
// Values in a note and voice address are either >= 0 if they
// are specified, or -1 to indicate a wildcard. A wildcard
// means a voice with any value in that part of the tuple
// matches the message.
//
// For instance, a (PCKN) of (0, 3, -1, -1) will match all voices
// on channel 3 of port 0. And a PCKN of (-1, 0, 60, -1) will match
// all channel 0 key 60 voices, independent of port or note id.
//
// Especially in the case of note-on note-off pairs, and in the
// absence of voice stacking or polyphonic modulation, a host may
// choose to issue a note id only at note on. So you may see a
// message stream like
//
// CLAP_EVENT_NOTE_ON [0,0,60,184]
// CLAP_EVENT_NOTE_OFF [0,0,60,-1]
//
// and the host will expect the first voice to be released.
// Well constructed plugins will search for voices and notes using
// the entire tuple.
//
// [...]
//
// - A note-on event with a '-1' for port, channel or key is invalid and
// can be rejected or ignored by a plugin or host.
// - A host which does not support note ids should set the note id to -1.
// [Event impls]
//
// target a specific note_id, port, key and channel, with
// -1 meaning wildcard, per the wildcard discussion above
// int32_t note_id;
// int16_t port_index;
// int16_t channel;
// int16_t key;So note ons need In VST3, note-on events do pass |
- Fixed typos - Add constants for MIDI Channel/CC counts and use them where it fits - Move note expression impl into a separate file and return kInvalidArgument for disabled expressions
|
Just checked how my voice impls would deal with the Also checked what the To document all the discussed issues properly, and to make it more Rust idiomatic, I'd also propose turning the wildcards into Rust This way we make super clear that the values can be optional, and force plugin impls to deal with that instead of relying on the -1 magic numbers. With the new options we could also make the types unsigned, and fit them to their specified ranges: channel and key are Here's how #[derive(Clone, Debug)]
#[non_exhaustive]
pub enum Event {
// Note events
//
// Notes are addressed via a `(channel, key, note_id)` tuple, where
// * `Some` values are validated values (0..16 channel, 0..128 key)
// * `None` values are wildcards, matching every voice regardless of that field
/// A note on event.
///
/// `note_id` usually will be `Some` in hosts with note expression support.
NoteOn {
sample_offset: usize,
channel: u8,
note_id: Option<u32>,
key: u8,
velocity: f64,
},
/// A note off event.
///
/// `note_id` likely will be `None` here even when the matching [`Event::NoteOn`] carried one,
/// as hosts may issue a note id at note-on only.
/// `channel` and `key` may be used as wildcards: a host may use this to release a whole
/// MIDI channel or all active voices at once.
NoteOff {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
velocity: f64,
},
// Per-note expression events (VST3/CLAP note expression)
//
// Addressed with the same `(channel, key, note_id)` tuple as note events, but:
// * VST3 uses `note_id` only, so `channel` and `key` always are `None` there.
// * CLAP hosts usually pass `channel` and `key`, and optionally `note_id`.
/// Per-note volume. `gain` is linear in [0, 4], where 1 is 0dB and 0 -INFdB.
///
/// Requires [`crate::NoteExpressions::with_volume`].
PolyVolume {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
gain: f64,
},
/// Polyphonic key pressure (poly aftertouch - VST3's `kPolyPressureEvent` or
/// CLAP's `CLAP_NOTE_EXPRESSION_PRESSURE`).
///
/// `value` is in [0, 1]. The same value delivered as a raw MIDI byte message arrives as
/// [`Event::MidiPolyPressure`].
///
/// Requires [`crate::NoteExpressions::with_pressure`].
PolyPressure {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
value: f64,
},
/// Per-note panning.
///
/// `pan` is in [-1, 1] (left..right).
///
/// Requires [`crate::NoteExpressions::with_pan`].
PolyPan {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
pan: f64,
},
/// Per-note tuning offset in semitones.
///
/// `semitones` is in [-120, +120].
///
/// Requires [`crate::NoteExpressions::with_tuning`].
PolyTuning {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
semitones: f64,
},
/// Per-note vibrato. Rarely (if at all) used by hosts, but part of the CLAP specs.
///
/// `amount` is in [0, 1].
///
/// Requires [`crate::NoteExpressions::with_vibrato`].
PolyVibrato {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
amount: f64,
},
/// Per-note expression (MIDI MPE "slide" / CC 74 equivalent). Rarely (if at all)
/// used by hosts, but part of the CLAP specs. You usually want `PolyBrightness` instead.
///
/// `amount` is in [0, 1].
///
/// Requires [`crate::NoteExpressions::with_expression`].
PolyExpression {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
amount: f64,
},
/// Per-note brightness a.k.a. timbre (CLAP brightness / VST3 brightness).
///
/// `amount` is in [0, 1].
///
/// Requires [`crate::NoteExpressions::with_brightness`].
PolyBrightness {
sample_offset: usize,
channel: Option<u8>,
note_id: Option<u32>,
key: Option<u8>,
amount: f64,
},
// MIDI channel based events
//
// NB: `channel` and `key` come from raw MIDI bytes or from per channel host parameters
// and thus always are valid, never wildcards.
/// Channel-wide MIDI pitch bend.
/// Per-note pitch offset (VST3/CLAP note expression) arrives as [`Event::PolyTuning`].
///
/// `semitones` is the current bend in semitones, using the standard +-2 semitone range.
///
/// Requires [`MidiCapabilities::with_pitch_bend`].
MidiPitchBend {
sample_offset: usize,
channel: u8,
semitones: f64,
},
// [...] Other MIDI events accordingly with channel: u8, value: f64... |
|
Yeah these suggestions sound good, thanks for your design efforts! |
…ment wildcard note matching
…T is enabled Also skip reporting VST3 event busses when Plugin::HAS_NOTE_INPUT or P::HAS_NOTE_OUTPUT is disabled
Per spec it's `Volume, plain range [0 = -oo , 0.25 = 0dB, 0.5 = +6dB, 1 = +12dB]: plain = 20 * log (4 * norm)`
|
Pushed the discussed changes now. This is quite a big change, so I need another day or two to verify things. Two things are still open:
VST3's note-on event also carries a |
|
Those two suggestions also make sense to me |
One maps parameter IDs to MIDI types for the event iterator. The other one MIDI types to parameter IDs for the VST3 `getMidiControllerAssignment` implementation. This speeds up event iteration with MIDI capabilities enabled.
|
Fixed the Let's skip the So the PR seems ready from my side. No rush though. As this is quite a big change I'm also fine with keeping this in my fork for now, as you likely don't need any of those features. |
It's probably ok to add it if/when needed.
Thanks, I'll take a look at the latest changes and merge if it looks ok. I'm working on an update to my plugin and might as well adapt to these changes while I'm at it. |
| let event = unsafe { &*(header as *const clap_event_note) }; | ||
|
|
||
| let (Some(channel), Some(key)) = (note_channel(event.channel), note_key(event.key)) else { | ||
| // CLAP spec requires that valid channels and keys are specified for note-ons. |
There was a problem hiding this comment.
Maybe a warning or info message when an event is skipped?
There was a problem hiding this comment.
Yes, that is a bit spooky. But this is called directly from the host, so such errors are never caused by the plugin. Therefore I'm not sure if logging is really useful here. A debug_assert!(false) maybe doesn't hurt, just to be sure that it never hits under "normal" conditions.
In general, logging in the audio thread should be avoided, so the only options here are debug_assert! and tracing::trace!, right?
There was a problem hiding this comment.
AFAIK tracing calls are no-ops if that level of logging isn't enabled, but might be good to make sure
There was a problem hiding this comment.
Okay. So tracing or debug assert. Which one would you prefer here?
There was a problem hiding this comment.
Logging is a better option IMO but tracing is for very granular debugging so I would maybe opt for a debug-level message
There was a problem hiding this comment.
Okay, I'll add a tracing::debug! then and will check if there are other related places where this could be helpful.
Is Level::DEBUG enabled in tracing in debug builds by default, and skipped in release builds? Haven't checked, but that would be a perfect compromise then.
There was a problem hiding this comment.
It all depends on which tracing client you're using so it's up to the plugin developer what they want to use
| // VST3 always supplies a valid channel and key on a note-on, but a wildcard | ||
| // or out of range value is invalid and should get skipped. | ||
| let (Some(channel), Some(key)) = (note_channel(note_on.channel), note_key(note_on.pitch)) else { | ||
| continue; |
There was a problem hiding this comment.
Again, maybe a warning or info message for skipped events?
| let (channel, key) = self.note_id_map.lookup(note).unwrap_or((-1, -1)); | ||
| // An expression with a missing note id addresses nothing at all, so skip it. | ||
| let note_id = note_id(note_expression.noteId); | ||
| if note_id.is_none() { |
|
|
||
| /// Convert and validate a raw CLAP/VST3 key field. See [`note_channel`] for the wildcard rule. | ||
| pub(crate) fn note_key(raw: i16) -> Option<u8> { | ||
| u8::try_from(raw).ok().filter(|&key| key < 128) |
There was a problem hiding this comment.
Same here, assert instead of ignore
|
I had some small nitpicks about error handling, otherwise looks good! |
| for (channel, pitch_bend_parameter_id) in self.pitch_bend_parameter_ids.borrow_mut().iter_mut().enumerate() { | ||
| while ids.contains(¶meter_id) { | ||
| parameter_id += 1; | ||
| let user_ids = parameters.ids(); |
There was a problem hiding this comment.
This entire block (registering of MIID parameters) maybe should go to plinth-plugin\src\formats\vst3\parameters.rs ?
So the map creation and consuming parts are in one file, making it less likely that they get out of sync when someone changes something here.
| @@ -0,0 +1,299 @@ | |||
| use vst3::Steinberg::Vst::NoteExpressionTypeIDs_::{kBrightnessTypeID, kExpressionTypeID, kInvalidTypeID, kPanTypeID, kTuningTypeID, kVibratoTypeID, kVolumeTypeID}; | |||
There was a problem hiding this comment.
Nitpick: There's now a note_expresion.rs (singular) and note_expressions.rs (plural) - should both be note_expresions.rs?
| 1 | ||
| } | ||
| } | ||
| MediaTypes_::kEvent => { |
There was a problem hiding this comment.
I didn't mention this change in the PR description. Previously the plugin reported an event input bus, even when HAS_MIDI_INPUT was false. Same for HAS_MIDI_OUTPUT. I think the new behavior is correct, but this now changes the bus layout as seen by the host.
Main intention was to add MPE support and note expressions to my synth, but I ended up not dealing with MPE in the plugin-things layer. Instead, I only added the infrastructure to allow plugins to implement MPE via raw MIDI CC message handling.
The MidiCapabilities and NoteExpression gates are quite opinionated. Please let me know if you would prefer using bitflags, runtime configuration or something completely different here. I think options are definitely needed here, because enabling all MIDI events would add 16*128 parameters just for MIDI CCs, which is completely overkill. Runtime options also would work, but compile time options are a nice fit here too IMHO.
This is a breaking change. In order to handle MPE, I need to process raw MIDI Pitch Bend and tuning events separately. So the old Pitch Bend event is now delivered as either PolyTuning or MidiPitchBend.
Note expressions in CLAP are straightforward to deal with. In VST3 this is quite messy. So to unify things across standards I am here only allowing VST3 note expressions to be added from the defined set of CLAP expressions. So totally custom VST3 note expressions cannot be added via this approach.
To document and showcase all this better, I could also add a small synth example with note expressions in addition to the gain example.
BTW: The field
notein events is pretty confusing. It should benote_idinstead to clearly separate it fromkey. I haven't changed that and usednoteinstead as the old PitchBend event and unchanged Note events do.