Why not use MIDIPacketListAdd?

[Boddlnagg]

Is there a reason why you did not use MIDIPacketListAdd and instead built your own PacketBuffer with quite a bit of unsafe code, which might not really be necessary?

Maybe I'm missing something, but right now I don't understand it. If there is a good reason, it should at least be documented in the source code.

[Boddlnagg]

Furthermore, I don't see a reason why PacketBuffer::from_data and PacketBuffer::with_data take data as Vec<u8> instead of &[u8] ... the data is copied into an internal buffer anyway.

[chris-zen]

@Boddlnagg it has been long since I wrote it and don't remember exactly, but my guess is that I was trying to provide some kind of PacketList builder that did not require to know a size for the data buffer beforehand.
About Vec<u8> or &[u8] I guess it was a convenience thing, not sure.
I don't see any reason not to use MIDIPacketListAdd, but given that it just copies the data into the right position (assuming a fixed size packet list), I guess I preferred to do it by using the already provided methods by Vec.
To be honest, while reviewing the code after all this time, I got the impression that it could be cleaned/refactored a bit (may be by using the builder pattern strictly where it keeps the added packets in a Vec<Vec<u8>> and it builds the actual CoreMIDI packet list only when the .build() method is called. Not sure what it would be better). Any suggestion ???
Bear in mind that this was my first time coding Rust in the unsafe world ;-)

[Boddlnagg]

Okay, I tried to read the doucmentation and find examples of CoreMIDI API usage, but unfortunately I still don't understand everything here.

A few findings and ideas in no particular order:

• According to https://stackoverflow.com/questions/15923236/proper-use-of-midipacketlistadd-coremidi "If you call MIDIPacketListAdd with the same timestamp as the previous call, then it just concatenates your data to the previous packet. You will see packetList->numPackets and currentPacket stay the same, but currentPacket->length will increase." This is an optimization that is currently not being done here.
• Some consumers of CoreMIDI use a fixed-size stack-allocated buffer for small messages (e.g. PacketLists that only contain a single small MIDI message, i.e. 1–3 bytes, which is probably quite a common case), and for larger messages they only allocate a buffer once and reuse it. I guess as long as the buffer is valid at the point where MIDISend is called, this should be no problem. Maybe we can make use of a similar optimization. A MIDIPacketList allocated on the stack already contains space for 1 MIDIPacket, which in turn contains space for 256 data bytes, which should be enough for most messages.
• In midir, building a "packet buffer" and sending a message is a single operation which receives a &[u8] from the user. I want to be able to send such a message without any additional heap allocation if possible, at least for short, non-sysex messages. This is not possible with the current API. A builder with a Vec<Vec<u8>> internally would make this situation even worse (at least if it's the only way to construct a PackageList), because it would need to allocate at least twice.
• The official documentation notes that "On ARM, MIDIPacket must be 4-byte aligned.". You probably didn't try to support iOS yet, but I think this is currently wrong in your implementation.
• You are currently ensuring that the size of single packet is not larger than MAX_PACKET_DATA_LENGTH. However, the official documentation states "The maximum size of a packet list [emphasis mine] is 65536 bytes. Large sysex messages must be sent in smaller packet lists." RtMidi, on the other hand, splits longer messages into packets of 65536 bytes each and adds them all to a single PacketList, which contradicts the official documentation. Or maybe the documentation is just wrong? http://comelearncocoawithme.blogspot.de/2011/08/reading-from-external-controllers-with.html also claims that SysEx messages always come in a single PacketList and are never split across multiple PacketLists.
• The official documentation also says "In the case of system-exclusive messages, a packet may only contain a single message, or portion of one, with no other MIDI events. The MIDI messages in the packet must always be complete, except for system-exclusive." This allows multiple non-sysex messages in the same Packet (used by the optimization in my first bullet point).
• The definition of Struct_MIDIPacket at https://github.com/chris-zen/coremidi/blob/master/src/coremidi_sys_ext.rs#L24-L28 seems to be wrong. The size of the static data buffer does not match the original definition at https://github.com/chris-zen/coremidi/blob/master/src/coremidi_sys_ext.rs#L24-L28. Where did you get the constant MAX_PACKET_DATA_LENGTH from? It does not seem to be part of any official API. Additionally, I'm not sure if a Packet is allowed to contain less than the statically allocated 256 bytes (see documentation here).

I don't know how critical all these "optimizations" really are for MIDI, but in general I feel like a library wrapper should at least allow one to use the API in the most efficient way possible, as long as it's memory safe.

The bugs (which might even lead to memory unsafety), however, should definitely be fixed.

I have to think again about an API redesign that makes this as simple, yet efficient, as possible.

[Boddlnagg]

I'm working on a PR that enables all of this and fixes several bugs in the current version. It also involves upgrading to the latest version of coremidi-sys.

[chris-zen]

Great explanations, thanks. I think your suggestion makes a lot of sense. I will look into the PR.