atsushieno / ktmidi

Kotlin multiplatform library for MIDI access abstraction and data processing for MIDI 1.0, MIDI 2.0, SMF, SMF2 (MIDI Clip File), and MIDI-CI.
MIT License
70 stars 7 forks source link

SYSEX troubles on nearly every platform #81

Closed JBramEu closed 2 months ago

JBramEu commented 3 months ago

As stated in the title, I'm experiencing lots of troubles with SYSEX messages. Here's a summary:

Windows JVM Works by default

MacOS JVM MacOS Bug, their Java Midi implementation ignores SYSEX messages. I could work around this by adding CoreMidi4J as a dependency and cloning JvmMidiAccess to use the library components. May I suggest adding this workaround to the library?

Android The Android platform splits up midi data in quite small chunks - and since the data I'm handling is a SysEx bulk of more than 1k Bytes, I need to check for the Sysex start byte - and if there's no Sysex End, keep and combine the received data bytes until I finally receive a sysex end byte. This is quite annoying. Not sure if this could happen on all platforms and android is the only one chunking the data stream in such small pieces or if the other platforms make sure they don't split SysEx messages in half.

iOS Both, TraditionalMidiAccess and UmpMidiAccess just crash instantly upon trying to send a message. No solution/workaround yet, need to do some more debugging.

I haven't tried Linux or any of the native targets yet.

atsushieno commented 3 months ago

Thanks for trying it out on various platforms!

I would not recommend JvmMidiAccess on MacOS (I use RtMidiAccess which also supports virtual MIDI ports). I am aware of CoreMidi4J project, but never thought that it is still under development (my impression is especially because there is no updates while there has been MIDI 2.0 support in CoreMIDI). Is there any better points in CoreMidi4J than RtMidi? I am all for having any better option.

Android: sounds like we need something like #53 for AndroidMidiAccess. I don't have a lot of sysex messaging experience on Android but haven't seen such an issue when I was trying ktmidi-ci-tool (in this repo) on Android. What were the MIDI devices you tried?

iOS/macOS (Native): I have some fixes to get CoreMidiAccess outputs working.

I don't have iOS devices, and I thought I don't have any MIDI output device, so all what I thought I can do is to examine input-sample with my MIDI keyboard on macOS, which has been working fine. I noticed my Seaboard BLOCK could also function as a MIDI out device, so I used it to confirm the crasher.

JBramEu commented 3 months ago

Hi!

I'm using a couple of analog synthesizers, which send their program data via SysEx. Sequential Prophet 6 being the one I'm doing the most testing with. One program of the prophet is 1076 bytes (including the sysex start, headers and sysex stop). AndroidMidiAccess never delivers this message in one piece, so I have to check if the incoming byteArray contains sysex start AND sysex end. If the latter is missing, I am appending the incoming bytes in a buffer until I finally get a sysex end. This seems to work well so far, i do need to test on more devices, though.

Looking forward to your iOS fixes!

Side note: Android offers a nice way to test midi: You can connect your device via USB and then select the MIDI usb mode, so you can send midi from your pc to the android device.

atsushieno commented 3 months ago

Side note: Android offers a nice way to test midi: You can connect your device via USB and then select the MIDI usb mode, so you can send midi from your pc to the android device.

I thought you are talking about MIDI input devices, but now it seems you are talking about reproducible tests. How can I run reproducible tests without specific MIDI input devices that I don't have?

Nevertheless, if there is any way to connect arbitrary MIDI devices on GitHub Actions servers then I would resort to that.

atsushieno commented 3 months ago

I noticed that we should NOT handle this in MidiAccess. Here is a use case:

Some devices receive farmware updates via SysEx messages, and the binary can be huge (like hundreds of megabytes). The sender and receiver do NOT want us (any intermediate MIDI access ports) to handle them buffered everywhere but handle the stream chunk only by themselves at the initiator and the destination. That means, it is up to the app's design decision on how to handle those chunked SysEx messages.

What we would provide there instead is a consolidated SysEx chunk manager that can buffer any incomplete inputs and return "completed" chunks as new inputs appear, so that app developers like you don't have to deal with them by themselves. My current idea is like this:

package dev.atsushieno.ktmidi

class Midi1SysExChunkProcessor {
    val remaining = mutableListOf<Byte>()
    // Returns a sequence of "event list" that are "complete" i.e. not pending.
    // Any incomplete sysex buffer is stored in `remaining`.
    // If there is no buffer and the input is an incomplete SysEx buffer, then only an empty sequence is returned.
    fun process(input: List<Byte>): Sequence<List<Byte>> = sequence {
        if (remaining.isNotEmpty()) {
            val f7Pos = input.indexOf(0xF7.toByte())
            if (f7Pos < 0)
                remaining.addAll(input)
            else {
                yield(remaining + input.take(f7Pos + 1))
                // process the remaining recursively
                yieldAll(process(input.drop(f7Pos + 1)))
            }
        } else {
            // If sysex is found then check if it is incomplete.
            // F0 must occur only as the beginning of SysEx, so simply check it by indexOf().
            val f0Pos = input.indexOf(0xF0.toByte())
            if (f0Pos < 0)
                yield(input)
            else {
                yield(input.take(f0Pos))
                val f7Pos = input.indexOf(0xF7.toByte())
                if (f7Pos < 0)
                    remaining.addAll(input)
                else {
                    yield(input.take(f7Pos + 1))
                    // process the remaining recursively
                    yieldAll(process(input.drop(f7Pos + 1)))
                }
            }
        }
    }
}

Thoughts?

JBramEu commented 3 months ago

I agree that this kind of processing is better done on the application level. Your Processor implementation is basically what I'm doing atm. Not sure about the best way to have the processor interact with Midi1Message.convert() yet, though.

atsushieno commented 3 months ago

Not sure about the best way to have the processor interact with Midi1Message.convert() yet, though.

That's a good catch. Maybe this would suffice?

// In `Midi1Message` companion object 
        @Deprecated("Use convert(bytes, index, size, sysExChunkProcessor. It's better if you supply Midi1SysExChunkProcessor() (it is null by default for backward compatibility).", ReplaceWith("convert(bytes, index, size, null)"))
        fun convert(bytes: ByteArray, index: Int, size: Int): Sequence<Midi1Message> = convert(bytes, index, size, null)

        fun convert(bytes: ByteArray, index: Int, size: Int,
                    sysExChunkProcessor: Midi1SysExChunkProcessor? = Midi1SysExChunkProcessor()
        ): Sequence<Midi1Message> = convert(bytes.drop(index).take(size), sysExChunkProcessor)

        fun convert(bytes: List<Byte>,
                    sysExChunkProcessor: Midi1SysExChunkProcessor? = Midi1SysExChunkProcessor()
        ): Sequence<Midi1Message> = sequence {
            if (sysExChunkProcessor == null)
                yieldAll(convertInternal(bytes))
            else
                sysExChunkProcessor.process(bytes)
                    .map { convertInternal(it) }
                    .forEach { yieldAll(it) }
        }

        private fun convertInternal(bytes: List<Byte>): Sequence<Midi1Message> = sequence {
            var i = 0
            val size = bytes.size
            val end = bytes.size
            while (i < end) {
                if (bytes[i].toUnsigned() == 0xF0) {
                    yield(Midi1CompoundMessage(0xF0, 0, 0, bytes.drop(i).take(size).toByteArray()))

                   <snip>

You'd only need convert(bytes, index, size, Midi1SysExChunkProcessor()) (the last explicit argument would be needed in the meantime, until the deprecated function is removed after some version bumps).

atsushieno commented 3 months ago

v0.9.0 is released. It contains those changes (CoreMidiAccess and Midi1SysExChunkManager).

Considering that all those reported issues are addressed, I will close this issue in a week or so, unless further issues are found.

atsushieno commented 2 months ago

Closing as ^. Thanks for the report.

JBramEu commented 2 months ago

Sorry for the delay, i was on vacation. I just tested the Midi1SysExChunkProcessor and found a minor bug. We need to clear the Buffer (remaining) after receiving 0xF7, atm the processor is only working once ;) Other than that, the chunk processor works fine and handles the fragmented packages on android.

atsushieno commented 2 months ago

I should have added some dedicated tests for that...! Now it's done. Thanks for the catch!