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

Fix sysex processing in Midi1 #54

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Conversation

AlecJY
Copy link
Contributor

@AlecJY AlecJY commented Dec 8, 2023

According to SMF 1.0 spec, sysex messages are stored in the format:

F0 <length> <bytes to be transmitted after F0>

There is another format to store sysex messages:

F7 <length> <all bytes to be transmitted>

The second format is usually used together with the first format to transmit sysex messages as little packets.

This patch fixed the processing of F0 sysex messages, and added supports for F7 sysex messages. In addition, the buffer increment policy in Midi1Player is adjust to prevent buffer overflow in some conditions. It's important to note that because we cannot know if there are more data after a F0 sysex message, the ending F7 filling feature for sysex messages is removed with the patch.

Some platforms may not support sending incomplete MIDI messages. Here is my test about the compatibilies:

API Platform Supported Comment
AndroidMidiAccess Android ✔️
AndroidMidiAccess ChromeOS (ARCVM) Without any error messages
AlsaMidiAccess Linux ✔️
RtMidiAccess Windows With error message "MidiOutWinMM::sendMessage: error sending MIDI message." or "MidiOutWinMM::sendMessage: message size is greater than 3 bytes (and not sysex)!"
RtMidiAccess Linux ✔️ With a warning message "MidiOutAlsa::sendMessage: incomplete message!"
JvmMidiAccess Windows/Linux See #53
JzzMidiAccess Chrome/Firefox Web MIDI API says MIDIOutput.send() requires "one or more valid, complete MIDI messages"

@atsushieno
Copy link
Owner

Thanks, it is a great summary on how incomplete sysex messages are handled in those platforms. The code change looks good overall.

I guess you were working on your changes based on some old code, but I have been working on the new RtMidiAccess implementation which is based on JavaCPP (#32), which may give different results than what you posted ^. This also means that next time you try the latest code it could give different results. And in case my latest it does not work appropriately, I may have to revert back to JNAerator-based implementation. That's the latest status so far.

@atsushieno atsushieno merged commit 85df35b into atsushieno:main Dec 9, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

2 participants