elieserdejesus / JamTaba

Jamtaba is a software to play online music jam sessions.
http://www.jamtaba.com
244 stars 50 forks source link

Add MIDI clock synchronization for external hardware or other software #1354

Closed ndonald2 closed 4 years ago

ndonald2 commented 4 years ago

Overview

These changes implement basic MIDI clock output synchronization functionality to the standalone client. Electronic musicians jamming with sequencers or other hardware/software that has a concept of tempo and timeline would previously need to manually tempo and phase match to the metronome in Jamtaba. These changes add the ability to have Jamtaba send MIDI clock messages to available MIDI output devices to automate this synchronization.

I tested this with a friend (@dewb) in a private server with one client on macOS and one on Windows. The clock sync was "good enough" to keep our respective hardware sequencers aligned to the server tempo and interval for the duration of our ~1 hour jam.

This is my first ever contribution to a Qt project, and my first experience with the GUI libraries. As much as possible I tried to mirror existing conventions and architectural patterns in the codebase. The sync preferences and output device management code is essentially identical to the corresponding code dealing with inputs.

Change Summary

Guidance

In addition to code review I could use help from other contributors to test this on all platforms. I have tested solely on macOS Catalina as the only platform for which I currently have access to development tools. @dewb was instrumental in getting the project to build in Windows with these changes, as well as some testing on Windows. However, these changes have not been tested on Linux or older versions of macOS/Windows at all.

Also looking for feedback on VST/AU components - @dewb tested building the VST with no-ops for the abstract MIDI driver methods that were added, AU should be the same but I was unable to build the AU plugin successfully. The MIDI clock sync output functionality is irrelevant and should not be present in the plugin versions.

Link to BETA version release with installer for macOS

Known Issues

Potential Future Enhancements

Screenshots (macOS)

Screen Shot 2020-07-25 at 13 00 46 Screen Shot 2020-07-25 at 13 01 19

ndonald2 commented 4 years ago

I don't think I thoroughly tested previously existing MIDI input functionality after making these changes, so it will be important to verify no regressions there as well.

elieserdejesus commented 4 years ago

@ndonald2 , this is a very good contribution, really apreciate it. I need some time to compile and test in all platforms.

I see you created a new tab to "Sync". What you think about move these Sync widgets to the tab MIDI? For me this is more easy to use, since the Sync is actually "MIDI Sync", so I suppose make sense nest the "Sync" widgets inside the MIDI tab.

I will review the code soon too.

ndonald2 commented 4 years ago

Thanks @elieserdejesus!

I see you created a new tab to "Sync". What you think about move these Sync widgets to the tab MIDI? For me this is more easy to use, since the Sync is actually "MIDI Sync", so I suppose make sense nest the "Sync" widgets inside the MIDI tab.

This is a great suggestion. I have never before worked with the Qt GUI library so I mostly just copied what was already there for the code in the MIDI tab. I think it crossed my mind to add another section in the existing MIDI tab, but one concern would be that people with a lot of MIDI gear will have long lists of devices (including me) and it could make the two lists of checkboxes go taller than a reasonable size for the preferences window. So maybe it would need to scroll?

I can experiment with this over the next week or so

jonjamcam commented 4 years ago

Hi @ndonald2 and @elieserdejesus. Just compiled your branch and tested in win7 x64 over USB2 connection to an Octapad SPD-30. Some comments on functionality:

  1. Overall it works as expected. The octapad recognizes incoming midi correctly as well as starting sync, but I got some issues when the second BPI interval starts (or is it maybe when the interval ends). At this point the spd-30 freezes and all I can do is unplug in from power. I think the problem could be the "stop"/"continue" msg (followed by an immediate start again) used to sync . For reference I use the octapad as slave unit synced via midi clock from an electribe sampler (Master clock) with no problems for some months now. I suggest making this stop/start feature optional for further compatibility.

  2. As you pointed out before the sync is not perfect, so this could lead to problems when syncing to units that force quantize. One solution some software use to compensate is an option to add/reduce manually introducing a value +-expected latency.

Cheers!

ndonald2 commented 4 years ago

Thanks for the testing and feedback @jonjamcam!

I think the problem could be the "stop"/"continue" msg (followed by an immediate start again) used to sync

I wondered about that, I think you are probably correct. It does not cause problems on any of my gear, but I'm also not sure if it's strictly necessary or actually even helps with anything. That said, there should be no extra "start" message after the first one - there is a check that start is only sent once when MIDI sync is started again https://github.com/elieserdejesus/JamTaba/blob/1efd671b08bc3acb97406ac04422d720692881be/src/Common/audio/MidiSyncTrackNode.cpp#L74

Since you can compile from source, if you remove the stop/continue code in MidiSynctrackNode.cpp (comment out or delete lines 78-81), does it fix the issue? And if so, does your Octapad stay aligned to the metronome over time? I understand there may be latency/jitter but hoping it does not drift out of sync over time without those extra messages. I can spend some time testing this soon as well.

One solution some software use to compensate is an option to add/reduce manually introducing a value +-expected latency.

This is a great suggestion, I called it out in the "potential enhancements" in the PR description. I can also take a stab at that soon, will require some new UI in the preferences as well as new settings persistence. I did not want my lack of Qt GUI experience to block opening the PR, at least, so I opted not to try to do that before submitting it. If I'm not able to make it work, assuming everything else in the PR looks good, I might suggest considering that an enhancement for another day - and maybe the community of contributors could even help with it?

Thanks again, excited this has been well received so far :)

jonjamcam commented 4 years ago

if you remove the stop/continue code in MidiSynctrackNode.cpp (comment out or delete lines 78-81), does it fix the issue?

Perfect! Solved in my system. I keep testing with other gear to see if something else comes around. Atm it's working fine.

And if so, does your Octapad stay aligned to the metronome over time?

Further testing will tell on long jams. ATM the sync is consistent even when changing bpi several times, but I think there may be a problem some times when changing BPI to a higher value (i.e. from 16 to 32) sync is lost, but it's not always. It could be my system, we'll see.

Great job @ndonald2 !

elieserdejesus commented 4 years ago

@ndonald2 , please let me know if you will add more commits fo tix the issue mentioned by @jonjamcam .

ndonald2 commented 4 years ago

Thanks for the review! I will have time this weekend to address the syntax comments and remove the stop/continue messages (issue raised by @jonjamcam) before this is ready for merge. I want to be sure to test it without those messages on my setup to verify everything is still stable in the sync.

@elieserdejesus what about your suggestion to move the contents of the new sync tab into the existing MIDI tab? Is that something that could be done later on without causing too much confusion?

elieserdejesus commented 4 years ago

@elieserdejesus what about your suggestion to move the contents of the new sync tab into the existing MIDI tab? Is that something that could be done later on without causing too much confusion?

I can do later, no problem.

ndonald2 commented 4 years ago

@elieserdejesus @jonjamcam this is ready for another review. Changes in my most recent commits:

I tested this on my gear and it's working great. I also plan to start working on latency offset adjustment and a minor improvement to the clock jitter stability but I will start separate branches for that and open separate PRs when ready.

Thanks again for the great support from the community of maintaners 🙏

elieserdejesus commented 4 years ago

very nice @ndonald2 , I will "Squash and merge" your pull request. This will appear as just "one" commit. Please keep your fork updated with the master branch here to avoid future git conflicts.

jonjamcam commented 4 years ago

Hi @ndonald2 and @elieserdejesus . I just compiled jamtaba vst plugin for windows and I see the checkbox appears in my system:

image

It does nothing as expected. Can you confirm?

elieserdejesus commented 4 years ago

@jonjamcam , I added a commit to hide the midi sync checkbox in VST/AU plugin

ndonald2 commented 4 years ago

Hey @elieserdejesus now that this is merged I would love to be able to use it with my friends on multiple platforms. Are there any other changes or enhancements you'd like to see before making a new release available to the public that includes the MIDI clock functionality?

uncoolcentral commented 4 years ago

Happy to test on Win 10 if needed. Just need a compiled download.

elieserdejesus commented 4 years ago

Hi guys, I released a beta including the MIDI clock sync feature. It`s windows only at moment, I have some issues with Mac compilation at moment: https://github.com/elieserdejesus/JamTaba/releases/tag/v2.1.16-beta

ndonald2 commented 3 years ago

@elieserdejesus Thanks, Windows build is helpful. I am setup to compile on macOS but with a newer version of Qt than I think this project is using otherwise. If I can generate a .dmg using the script in the repo would that help in the short term?

elieserdejesus commented 3 years ago

yes @ndonald2 , I dmg will be very helpfull.

jonjamcam commented 3 years ago

+1 for a new release (it's been a while) :D

ndonald2 commented 3 years ago

Here is a link to a macOS dmg built from current master branch. I bumped the version to 2.1.16 locally before building.

https://www.dropbox.com/s/jp7dg5p9b4tre71/Jamtaba_2_Installer.dmg?dl=0

EDIT 2: The original build I posted in this message was failing with a DYLD error on other people's machines. I managed to fix it by running macdeployqt on the .app bundle as part of the the installer script

Please note that I built this with Qt 5.14.2 on macOS 10.15.6 which I believe is much newer than the expected Qt version for this project. Part of the challenge myself and fellow interested contributor @Dewb have run into with this project is the outdated toolchain, so it would be great to know if you have ideas on what the community of contributors can do to help modernize the tooling.

uncoolcentral commented 3 years ago

Beta is working fine. Midi sync is good.

Win 10 64


On Aug 30, 2020, at 11:58 AM, Elieser Ademir de Jesus notifications@github.com wrote:

 Hi guys, I released a beta including the MIDI clock sync feature. It`s windows only at moment, I have some issues with Mac compilation at moment: https://github.com/elieserdejesus/JamTaba/releases/tag/v2.1.16-beta

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.