midir icon indicating copy to clipboard operation
midir copied to clipboard

Jack backend uses non "realtime safe" operations

Open x37v opened this issue 4 years ago • 6 comments

https://github.com/Boddlnagg/midir/blob/85eaa46bf0b3e428621cbdec46af2423229da098/src/backend/jack/mod.rs#L176-L194

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: https://github.com/Boddlnagg/midir/blob/85eaa46bf0b3e428621cbdec46af2423229da098/src/backend/jack/mod.rs#L173

x37v avatar Feb 08 '22 17:02 x37v

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

Be-ing avatar Feb 08 '22 18:02 Be-ing

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

Boddlnagg avatar Feb 08 '22 19:02 Boddlnagg

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

Be-ing avatar Feb 08 '22 19:02 Be-ing

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?

Boddlnagg avatar Feb 12 '22 16:02 Boddlnagg

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.

Be-ing avatar Feb 12 '22 17:02 Be-ing

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.

x37v avatar Feb 12 '22 18:02 x37v