search

PortMidi / portmidi

portmidi is a cross-platform MIDI input/output library
Other
130 stars 35 forks source link

pm_mac: handle eox even when multiple small messages are part of one packet #67

Closed te-johan closed 1 week ago

te-johan commented 9 months ago

during a midi stress tests i noticed that sometimes sysex messages got stuck in PortMidi. what was specific to the error cases was that there was always a real time event just before the last byte of a sysex message. ie . [0xf0, 0x1d, 0x51, 0xf8, 0xf7]

coremidi sends one packet containing 0xf8 and 0xf7 and the old code did not handle that case correctly.

rbdannenberg commented 9 months ago

Wow, this sounds significant -- great find! This stuff is tricky and amazing this hasn't come up after so many years of use, but maybe sysex is pretty unusual so this is definitely an rare case. I want to look at this carefully and have some other things to merge that I put off due to travel, but I think I can do this within a week. Your contribution, especially taking time to track down the problem, is greatly appreciated.

te-johan commented 1 week ago

I saw that there has been some updates on master. Is this issues resolved now?

rbdannenberg commented 1 week ago

Good question - I think so, but it looks like the addition of support for virtual devices led to some code reorganization. I think that all data is now passed to pm_read_bytes() rather than trying to rely on CoreMIDI to parse some messages. I should take a closer look at the code, and will, but I'm tied up for a few days. If you have a way to rerun whatever stress tests uncovered the problem in the first place, that would be great. I don't have anything set up to create the conditions where the old code failed.

rbdannenberg commented 1 week ago

After some investigation, I found dba8357, so it would still be great to specifically test the fix, but I believe the eox problem has been fixed: "... The interface between system-dependent and system-independent code was originally based on the idea that the system did a lot of work to parse out messages, so it should be simple to push these messages to the PortMidi input queue. This should have avoided re-parsing MIDI bytes and enabled fast word-at-a-time handling of sysex data. However, MacOS MIDI handling is largely undocumented and has many complexities: packets can contain multiple messages or partial (sysex) messages, messages can have embedded 1-byte real-time messages. To handle these special cases, code became messy and to add to the complexity, the mess was split between system-dependent (mmmacosxcm.c) and system-independent (portmidi.c) code. Even after years, a new bug was found by te-johan, and there may be other problems with embedded real-time messages. Rather than continuing to patch the code, this commit greatly simplifies code by completely reparsing MIDI in system-independent code, but retains the option of passing in fully formed MIDI short messages."