Jack backend uses non "realtime safe" operations

[x37v]

midir/src/backend/jack/mod.rs

Lines 176 to 194 in 85eaa46

	let mut message = MidiMessage::new(); // TODO: create MidiMessage once and reuse its buffer for every handle_input call
	// We have midi events in buffer
	let evcount = buff.get_event_count();
	let mut event = mem::MaybeUninit::uninit();
	for j in 0..evcount {
	message.bytes.clear();
	unsafe { buff.get_event(event.as_mut_ptr(), j) };
	let event = unsafe { event.assume_init() };
	for i in 0..event.size {
	message.bytes.push(unsafe { *event.buffer.offset(i as isize) });
	}
	message.timestamp = Client::get_time(); // this is in microseconds
	(data.callback)(message.timestamp, &message.bytes, data.user_data.as_mut().unwrap());
	}
	}

The process callback is called in the jack process graph which should only be executing "realtime safe" operations but midir creates MidiMessage, which uses a Vec for its bytes, and pushes to that Vec, which allocates on the heap and is therefore not generally considered "realtime safe."

Another thing
Option<T> only implements https://doc.rust-lang.org/std/option/enum.Option.html#impl-Sync if T is Sync and in the case of MidiPort it isn't. So this isn't technically safe either:

midir/src/backend/jack/mod.rs

Line 173 in 85eaa46

if let Some(ref port) = data.port {

[Be-ing]

Is this fixable within midir's design? If not, I suggest to remove the JACK backend.

[Boddlnagg]

And if it's not, what kind of design changes would be required (just hypothetically)?

[Be-ing]

Zero chance of heap allocation
http://www.rossbencina.com/code/real-time-audio-programming-101-time-waits-for-nothing

[Boddlnagg]

So looking at this again, I think there's two things here:

1. Allocation in Vec::push. For small-ish messages we could probably use a fixed-size buffer. But in principle we support arbitrarily large Sysex messages. How is this handled in Jack internally, as it also needs a way to deal with this somehow ...?
2. We actually call the user input handler (data.callback) within this code. So any restrictions would apply there too. Since I can't think of a way to enforce this in Rust, this gets tricky ... it might be possible to use another thread and a channel to send incoming messages to that thread and call the user input handler there. But this really doesn't seem to fit in midir's design (we already recommend to send incoming messages through a channel to a separate thread, but I guess most channel implementations are unbounded and can allocate, so this doesn't really help).

@Be-ing Would Pipewire (#100) have the same kind of issues?

[Be-ing]

Are sysex messages actually arbitrarily sized or is there a defined upper limit?

We actually call the user input handler (data.callback) within this code. So any restrictions would apply there too.

Yikes. You're right, this is a major problem and doesn't really fit in midir's design. I think it may be best to remove the JACK backend. I also notice that it is using jack-sys directly instead of the safe Rust API for the JACK bindings, so it should probably be rewritten even if it is kept.

The JACK implementation (JACK1/JACK2/Pipewire) doesn't matter. In any case the JACK process callback must not heap allocate, block, or invoke any system calls not explicitly defined as being nonblocking or you risk causing audible glitches. This is especially bad in midir's context because an application just trying to get MIDI data could cause glitching in a separate application using JACK for audio.

[x37v]

My approach for similar problems is usually to have a poll() method that the API exposes that the user must call, poll() reads from a channel that the realtime thread pushes to and calls callbacks as needed.
This makes it very explicit about which thread callbacks happen in, but also lets you push basic data from the realtime thread (chunked sysex for instance) and reconstruct it as needed in the poll/callback thread.

If you want to support processing in realtime threads, you could add a separate API for that, which would provide the chunked sysex data and maybe some sort of state indicating in sysex message

So yeah, I guess I'm just suggesting what you're talking about in 2.. if that doesn't fit into your vision if the library, maybe its best to just remove jack.