Fix missing InputEventMIDI from MIDI with multiple messages eg chord

[markofjohnson]

Fix missing InputEventMidi from midi with multiple messages eg chord

MIDIDriver::receive_input_packet can be called to process an incoming midi packet that contains multiple messages. Eg playing a chord (several notes at the same time) may arrive using midi 'running status' and contain several NoteOn in one packet. This commit loops through all the messages in the midi packet creating an InputEventMidi for each one.

Fixes #77035

Simple fix but quite a serious MIDI bug that can be reproduced with eg Roland keyboards connected to macOS by bluetooth. Could effect Windows & Linux too, reproducibility will depend on hardware and OS midi configuration. Suggest consideration for Godot 4.2 and 3.x as their midi_driver.cpp is almost the same and has the same bug. Thanks.

[lawnjelly]

You will need to squash commits:
https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

There will probably be a few nitpicks but the principle looks sound, well done for spotting this.

[AThousandShips]

Also please make your one commit have a clear message, like that of the PR title

[AThousandShips]

You merged instead of pushing, you need to:

git reset --hard 8d4d345
git push -f

[markofjohnson]

Hey @AThousandShips I'm struggling with git. Is it in the correct state now?

Also, there is another change I will propose to this method to protect against a hang if a malformed midi packet comes in (which given it could come from any MIDI device is possible). How should I proceed? I am thinking I will make another commit and ask for feedback?

[AThousandShips]

You can add it as a separate commit to make it easy to remove if it's not desired :) the commit history looks good now

[lawnjelly]

It's actually decades since I did any MIDI but afaik some things to be sure here is:

• There are no infinite loops
• It handles gracefully any unknown MIDI messages
• Ignores system exclusive messages

[markofjohnson]

A malformed MIDI packet can cause an infinite loop hang in this code, due to my bug fix. I will make an additional commit to address this for consideration.

…

On Mar 25, 2024, at 10:44, lawnjelly ***@***.***> wrote: It's actually decades since I did any MIDI but afaik some things to be sure here is: There are no infinite loops It handles gracefully any unknown MIDI messages Ignores system exclusive messages — Reply to this email directly, view it on GitHub <#89871 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADGCIXA7F77QIKUQX2HCTLY2BPBVAVCNFSM6AAAAABFGLMJUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYGU2TSMJWGE>. You are receiving this because you authored the thread.

[markofjohnson]

@lawnjelly and @AThousandShips are we OK with commit d33ee9d re stopping the infinite loop. Adding this is necessary because I added the while loop in order to fix the bug #77035 . Its not perfect because I didn't extend the code to support every possible MIDI message, but I consider that outside the scope of fixing the original defect I ran into.

If you approve I suppose I have to squash commits again?

[AThousandShips]

Will take a look later today

[markofjohnson]

Hey I got really lost in git and decided to start my branch over. I think I now have clean 1 commit with the fixes we discussed. Thanks.

[lawnjelly]

Looks ok to me and seems to have had some testing. I don't know if we have any specific maintainers for MIDI (reduz will probably be familiar, but may be busy) but anyone else welcome to take a look. 👍

I don't know how many users use MIDI input so might need some testing in the wild through betas.

[markofjohnson]

@AThousandShips this MIDI PR ready for review if you want a look. Thanks.

[AThousandShips]

Style and general function looks good, can't verify the validity in regards to MIDI

[akien-mga]

CC @fbcosentino @alcomposer @voidshine @ibrahn who contributed to MIDI support in the past year or so.

[fbcosentino]

Will try to both reproduce the issue and test the commit on both windows and linux this weekend

[markofjohnson]

Bug has only been reproduced on Mac as far as I know. My understanding is that windows ‘unpacks’ MIDI running status into separate MIDI messages before it passes along to user processes. So the bug can’t happen on windows. Mark JOn Apr 4, 2024, at 5:06 AM, fbcosentino ***@***.***> wrote: Will try to both reproduce the issue and test the commit on both windows and linux this weekend —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[ibrahn]

(Sorry I'm late)

On a read through, I see three possible issues:

• Realtime messages (0xf8 to 0xff) will interrupt running status.
• If a packet contains an "unsupported" message type, the remainder of the packet will be discarded.
• Messages from one device can interrupt running status on another. The status bit cache last_received_message should be per connection, rather than global.

I think these problems will already have existed for MacOS builds, so this PR still improves things.

It looks like #77035 affects only Core MIDI (MacOS).
We currently only have two other MIDI drivers:

• on Windows, the platform API handles running status for us and drops SysEx.
• on Linux, our ALSA MIDI driver does this parsing, before passing single complete messages to receive_input_packet.

So the argument could be made to fix this at the Core MIDI driver.
However, looking forward it probably does makes sense to have a parser at the platform independent level, making it less work to add MIDI input drivers for iOS, Android etc.
Perhaps we should move the existing implementation up from the ALSA class to the platform independent. I can look at that this afternoon/tomorrow.

[markofjohnson]

I think these problems will already have existed for MacOS builds, so this PR still improves things.

Yes, and cited are advanced cases of MIDI usage that would require more Godot development to support anyway. Without a fix, MIDI on Mac only works for single note at a time. BTW I tested MIDI in with 2 devices and indeed it's possible to corrupt running status data. Result was bad data through to the gdscript code but no crash.

So the argument could be made to fix this at the [Core MIDI driver]

Would require duplicate parsing code in midi_driver_coremidi.cpp, and it would have to allocate memory and copy data in order to turn running status into individual messages? Not ideal but it would isolate this fix from Windows.

Looking forward it probably does makes sense to have a parser at the platform independent level, making it less work to add MIDI input drivers for iOS, Android etc.

Good idea, especially working towards MIDI in for mobile, but out of scope for this bugfix PR.

thanks.

[ibrahn]

I've done the MIDI parser move, over at #90485. Could you run your chord test against it? I'm afraid I don't have access to a Mac at the moment

[markofjohnson]

I've done the MIDI parser move, over at #90485. Could you run your chord test against it? I'm afraid I don't have access to a Mac at the moment

I tested chord playing on Mac. This fixes the bug :-) . I logged data going in and confirmed that packets with more than one MIDI message work correctly. I noticed that I was not seeing MIDI running status, but packets with multiple messages. (So the previous diagnosis of the problem may not have been due to running status, but due to more than 1 MIDI message in 1 packet.)

So should I withdraw this PR? How - just close it?

[ibrahn]

I'm not sure what the process is at the moment, but I'd guess leave it open for now, at least until we know if #90485 passes review. As a fallback to keep MacOS MIDI on the cards for 4.3

[akien-mga]

I believe this is superseded by #90485, but I'm not too familiar with the details. Are there still changes from this PR which would be worth merge after rebasing / redoing the changes, or should this be closed now?

[markofjohnson]

withdrawing pr because #90485

[markofjohnson]

Superseded by #90485

[akien-mga]

Thanks for the contribution anyway, which led to finding the better cross-platform solution.