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 JACK MIDI dropping events when two events have the same time stamp. #5994

Merged
merged 1 commit into from
Apr 24, 2021

Conversation

softrabbit
Copy link
Member

As reported and debugged on Discord: JACK MIDI input has a tendency to drop events.
Explanation:

  • jack_midi_event_get is called to get a new event at the end of the for loop, if in_event_time == i
  • i gets incremented for the next iteration.
  • If the newly fetched event has the same time as the previous one, the comparison will be false and remain so for the rest of the buffer. Any further events will be ignored.

I suggest turning the if into a while to handle all events with the same timevalue.

@LmmsBot
Copy link

LmmsBot commented Apr 22, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13624-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.102%2Bgaa4989bc5-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13624?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13622-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.102%2Bgaa4989bc5-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13622?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/bnqfbpfih3e514ec/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38829668"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1ywheurvis0ep3si/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38829668"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13621-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.102%2Bgaa4989b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13621?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13625-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.102%2Bgaa4989bc5-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13625?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "cd68ced1db07d94e59a169614fe364437ccdbdbc"}

@VOID404
Copy link

VOID404 commented Apr 22, 2021

As the person reporting the bug, I cannot reproduce it on this version

@Veratil
Copy link
Contributor

Veratil commented Apr 22, 2021

@JohannesLorenz you're the previous committer for this loop fixing another bug, so pinging you for attention (would hate to regress). I see no issue changing if to while though. So approval from me. Would like your thumbs up, too.

@softrabbit
Copy link
Member Author

softrabbit commented Apr 22, 2021

Looking at the function around the fix, I wonder if it could be straightened into a while(jack_midi_event_get(&in_event, port_buf, event_index++) ).

@Veratil
Copy link
Contributor

Veratil commented Apr 22, 2021

Tangential topic: I did some looking around, it's possible we may have another bug in this loop related to SysEx messages.
jackaudio/jack2@b744332

@softrabbit
Copy link
Member Author

Tangential topic: I did some looking around, it's possible we may have another bug in this loop related to SysEx messages.
jackaudio/jack2@b744332

MidiClientRaw::parseData which handles the bytes will most likely just ignore the SysEx, so I think we're safe on that point.

@Veratil Veratil merged commit 89fc6c9 into LMMS:master Apr 24, 2021
@JohannesLorenz
Copy link
Contributor

Sorry, I missed this, but it looks basically good 👍

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
@softrabbit softrabbit deleted the jack-midi-drops-events-fix branch June 26, 2024 08:24
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.

5 participants