Skip to content
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

No luck sending/receiving MIDI on iOS #85

Closed
JBramEu opened this issue Jul 30, 2024 · 19 comments
Closed

No luck sending/receiving MIDI on iOS #85

JBramEu opened this issue Jul 30, 2024 · 19 comments

Comments

@JBramEu
Copy link

JBramEu commented Jul 30, 2024

Test setup:

  • ktmidi 0.9.0
  • TraditionalCoreMidiAccess
  • iPad Pro (non-M1)
  • iPadOS 17.5.1
  • USB Midi connection to a Sequential Prophet 6 Synthesizer
  • Same application works fine on JVM and Android targets with their respective MidiAccess implementation

What is working:

  • device detection (querying list of IOs, Prophet 6 shows up)

What's not working:

  • Sending midi (nothing received on the synth)
  • Receiving midi (listener not triggered)

Alternate approaches tried:

  • UmpCoreMidiAccess (crashes the app)

Further investigation results:
Both input and output report CLOSED as their connectionState immediately after MidiAccess.openIn/Output

@atsushieno
Copy link
Owner

Hmm, I have no further idea how I can get this working as I have no iOS device and there is no known MIDI device that works on iOSSimulator. Can you try your codebase on macOS native if is also usable there?

I'll be super busy until this weekend so I'll revisit after that. Thanks for your patient experiment.

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

I'm looking at the Apple sample app to compare it to what the library does atm.
Maybe we can get some ideas there.

First find is, that MIDIInputPortCreate is deprecated and we should use MIDIInputPortCreateWithProtocol instead.

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

Found an issue that needs to be addressed:

ISSUE: The ids of all midi devices are 0 (TraditionalCoreMidiAccess)

This results in following lines not selecting the correct port:
outputs.first { it.id == portId }
inputs.first { it.id == portId }

This seems to partially fix my issue, as receiving midi data seems to work now after applying a workaround.
Still no luck sending, though.

I'll keep digging.

@JBramEu JBramEu closed this as completed Aug 6, 2024
@JBramEu JBramEu reopened this Aug 6, 2024
@atsushieno
Copy link
Owner

Thanks for investigating the issue!

MIDIInputPortCreateWithProtocol

I am aware of this API. I kept it to the old API because, use of this newer API would limit the target platform to "iOS 14.0+iPadOS 14.0+Mac Catalyst 14.0+macOS 11.0+visionOS 1.0+" (copypasting from the API reference). And I could not find any decent way to detect the run-time platform version in Kotlin-Native. (IIRC there was another piece of code that gave up run-time detection in the CoreMidi implementation...)

@atsushieno
Copy link
Owner

IIRC there was another piece of code that gave up run-time detection in the CoreMidi implementation..

Found that:

// FIXME: How can I detect if I am on macOS or iOS in general??

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

I assume this is the reason why you're sticking with MIDIPacketList instead of MIDIEventList as well?

Here's another thing I'm currently investigating:

MIDIPacketListAdd(packetList.ptr,
    sizeOf<MIDIPacketList>().toULong(), // <-- should this be the capacity of the list like it is for MIDIEventList?
    curPacket,
    timestampInNanoseconds.toULong(),
    length.toULong(),
    pinned.addressOf(offset).reinterpret()
)

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

And here's the way I'm doing OS detection.

I'm using expect/actual functions to provide this information via the respective source set:

commonMain:

interface Platform {
    val name: String
}

expect fun getPlatform(): Platform // you can call this in common

androidMain:

class AndroidPlatform : Platform {
    override val name: String = "Android ${Build.VERSION.SDK_INT}"
}

actual fun getPlatform(): Platform = AndroidPlatform()

iosMain:

class IOSPlatform: Platform {
    override val name: String = UIDevice.currentDevice.systemName() + " " + UIDevice.currentDevice.systemVersion
}

actual fun getPlatform(): Platform = IOSPlatform()

desktopMain (JVM):

class JVMPlatform: Platform {
    override val name: String = "Java ${System.getProperty("java.version")}"
}

actual fun getPlatform(): Platform = JVMPlatform()

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

I created a separate issue for the device IDs so the scope is a bit more clear. Hope that's OK. This leaves us with working MIDI receive, but non-working MIDI send for this issue.

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

Alright, making progress.

This works for standard MIDI messages, my huge sysex chunks still fail because they are overloading the default buffer size when calling MIDIPacketListAdd. Looking into how to increase this as next step.

MIDIPacketListAdd(
      packetList.ptr,
      sizeOf<MIDIPacketList>().toULong(), // <-- should be the size of the list
      curPacket,
      0u, // <-- Midi timestamps are different from system timestamps
     length.toULong(),
     pinned.addressOf(offset).reinterpret()
)

MIDISend(portRef, coreMidiPortDetails.endpoint, packetList.ptr)

About the timestamp: MIDISend docs say that the OS will schedule messages with future timestamps, which I suspect to be the reason why nothing was sent even after i set the correct size parameter. Now with a zero timestamp messages get sent.

One other thing I noticed is that we're ignoring the result of both MIDIPacketListAdd and MIDISend. We should add proper error handling here.

@JBramEu
Copy link
Author

JBramEu commented Aug 6, 2024

Alright, looks like we're sending now :)

Here's my send function with customizable buffer size and error handling.
I have set SEND_BUFFER_SIZE as a global variable, but I suggest to add it to the constructor of CoreMidiAccess

override fun send(mevent: ByteArray, offset: Int, length: Int, timestampInNanoseconds: Long) {
        mevent.usePinned { pinned ->
            memScoped {
                val packetListSize: ULong
                val packetList: MIDIPacketList

                if (SEND_BUFFER_SIZE != null) {
                    // this is not so pretty, but it seems to be the most popular solution online
                    packetList = alloc(SEND_BUFFER_SIZE, 0) as MIDIPacketList
                    packetListSize = SEND_BUFFER_SIZE.toULong()
                } else {
                    packetList = alloc<MIDIPacketList>()
                    packetListSize = sizeOf<MIDIPacketList>().toULong()
                }

                val curPacket = MIDIPacketListInit(packetList.ptr)

                val addResult = MIDIPacketListAdd(
                    packetList.ptr,
                    packetListSize,
                    curPacket,
                    0u, // <-- Midi timestamps are different from system timestamps,
                    length.toULong(),
                    pinned.addressOf(offset).reinterpret()
                )

                if (addResult == null) {
                    throw Exception("Could not add message to send buffer. Trying increasing the buffer size.")
                }

                checkStatus {
                    MIDISend(portRef, coreMidiPortDetails.endpoint, packetList.ptr)
                }
            }
        }
    }

@atsushieno
Copy link
Owner

Thanks for all those investigation. I noticed that sending MIDI output did not get recognized by CoreMIDI service on macOS either and MIDIPacketListAdd() returned null, but somehow no further luck to get alloc() working well. Now it should be almost the same as what you suggested. Let me know if it is still problematic.

@JBramEu
Copy link
Author

JBramEu commented Aug 7, 2024

Thanks for adding the fixes so quickly!

I would, however, think about reducing the default send buffer size.
iOS default seem to be 256 Bytes and I would argue to not modify the buffer size at all as default case and only use the somewhat hacky memory allocation if we really need it.

As far as I understood the apple docs, the PacketList is meant to compile multiple messages if necessary and then send it as a batch. In our current implementation, a new packetList gets allocated for every message we send. Meaning we're potentially allocating 1024 Bytes for sending a Note On with 3 Bytes of actual payload. It shouldn't really make a difference, since it gets cleared right after sending (memScoped), but it's not the best practice and unnecessarily boosts the memory footprint imho.

I played around with allocating the exactly needed amount of bytes only (we know how big our packets are), but this would require to know the exact internal structure of the MIDIPacketList struct.

And let me add another thought for future reference here:
If people were to send huge chunks (talking > 10MB) of SysEx data, they should split it into chunks, with each chunk being of smaller or equal size than the send buffer. It is not a good idea to set the send buffer to such a big size (I think I read somewhere in the docs that the max supported size is 65535 Byte anyways).

@atsushieno
Copy link
Owner

I think my fix (based on your implementation but modified) answers to your concern on allocation by reusing alloc()-ed memory using Arena. Would that still involve massive allocation?

20f683d#diff-e07e9fbc42e5823c84b08c77e235ecd206c85fdd4788f386f40b7526c509f53bR121

@JBramEu
Copy link
Author

JBramEu commented Aug 7, 2024

Not sure that's the way to go, because we'd need to clear the packetlist before we add a new packet, but then we don't know if it was sent yet. I'd stick with the previoussolution.

In general, I think of the packetList more like a one-time container, which (in a native ios application) just compiles a list of stuff before it gets passed to the send method and then cleaned up.

@atsushieno
Copy link
Owner

Not sure that's the way to go, because we'd need to clear the packetlist before we add a new packet, but then we don't know if it was sent yet.

Doesn't MIDIPacketListInit() do exactly that job?

@JBramEu
Copy link
Author

JBramEu commented Aug 7, 2024

Yeah, you're right, the init function should be sufficient.
Your solution will definitely reduce the memory footprint and - I just ran a quick test - works.

I'm not 100 percent sure if the garbage collection will free up the allocated memory of the arena, though.
I assume it will, but C interoperability is not really my focus in Kotlin.

Not sure how memScoped and arena go together in this case, do you think we still need the memScoped, even if we allocate from the arena via lazy? My guess is that it has no effect with this solution.

@atsushieno
Copy link
Owner

Arena is derived from AutoFreeScope. It is poorly documented but if it does not auto free memory, I am not sure if the entire Kotlin-Native is reliable :p

do you think we still need the memScoped, even if we allocate from the arena via lazy? My guess is that it has no effect with this solution.

Good call, it would not be required anymore.

@atsushieno
Copy link
Owner

Once it's set in stone, I will prepare for v0.9.1 release.

@atsushieno
Copy link
Owner

done ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants