adamnemecek / WebMIDIKit

Simplest MIDI Swift library
MIT License
146 stars 27 forks source link

PacketlistAPI from swift #16

Closed jrmaxdev closed 4 years ago

jrmaxdev commented 6 years ago

Hello Adam, I was looking for a swift MIDI solution and found your code. I agree that the PacketList API cannot be used from swift right now, but I found a simple way to work around it using some small inlined c-functions. With these functions you can use the CoreMIDI PacketList API from swift without restrictions. Feel free to check out the repository jrmadev/SwiftMIDI

I took a look at your packet iterating extension:

extension MIDIPacketList: Sequence {
    public typealias Element = MIDIEvent

    public func makeIterator() -> AnyIterator<Element> {

       // the packet struct is copied here:
        var p: MIDIPacket = packet
        var i = (0..<numPackets).makeIterator()

        return AnyIterator {
            defer {

           // the packet struct is copied here:
                p = MIDIPacketNext(&p).pointee
            }

            return i.next().map { _ in .init(packet: &p) }
        }
    }
}

The iterated packets are copied from the original list.

In MIDIServices.h MIDIPacketNext is documented like this:

/*!
 @function MIDIPacketNext

 @abstract Advances a MIDIPacket pointer to the MIDIPacket which
      immediately follows it in memory if it is part of a MIDIPacketList.

 @param pkt A pointer to a MIDIPacket in a MIDIPacketList.

 @result The subsequent packet in the MIDIPacketList.
*/

When You assign the packet struct to a variable (struct are value types in swift) it is no longer part of a packet list. As a side effect you loose bytes in case the data is longer than 256 bytes. MIDIPacketNext calculates pointer offsets i.e. when calling MIDIPacketNext(&p) the returned pointer will point eventually into memory outside of of the current packet.

In my repository I have a small code sample showing exactly this. IterateOverCopiedPackets

Greetings, Jan

adamnemecek commented 6 years ago

Thanks for the info. I used to handle this case.

https://github.com/adamnemecek/WebMIDIKit/blob/a042248820c930fb83a2866e1e9475a9ae01484a/Sources/AXMIDI/AXMIDI.c

I think that at one point, my code was similar to yours but eventually decided to drop the C dependency for easiness when I realized that I think that your case mostly likely wont actually happen.

The reason for that is that I dont think that people are sending anything but notes or other short messages with their keyboardds. No one actually uses the dynamically sized messages. They might do it in MIDI files but definitely not for midi recording.

Lemme know what you think.

jrmaxdev commented 6 years ago

Thanks for the reply. I agree, most people would use it that way.

I stumbled into this pointer issue when I tried to read data out of MusicEventUserData delivered by a sequence callback. I could not find a decent way to get a pointer to the data field of UnsafePointer< MusicEventUserData >. The only thing that worked in swift was calculating the offset of the data field in the struct but that indeed looked very ugly.

Using a simple c-function to return the pointer from the struct seemed much safer to me. So I am dependent on c anyway (like anybody using CoreMIDI). The only effort to use this function is to include a bridging header.

The same problem would apply when reading meta events or sysex data from a sequence.

The only thing that would matter to me in reading packet lists would be performance. I don't think that a c-function call will cause much overhead. When using CoreMIDI in swift you call c-functions all the time.

adamnemecek commented 6 years ago

It was not a performance decision. You can always get a UnsafePointer to the data, or use the Data (aka NSData) class.

The overhead was due to the package structure not performance. Including C in an SPM package is annoying. I used to have it but removed it.

jrmaxdev commented 6 years ago

Thank for your reply. Your remark 'you can always get a UnsafePointer to the data' made me think about it again. With a little effort and some awkward lookin swift expressions I managed to remove all c-functions.

adamnemecek commented 6 years ago

Are you aware of the audiokit project? https://github.com/AudioKit/AudioKit/ I'll invite you to the slack group, you'll fit right in.

adamnemecek commented 6 years ago

What's your email?