izzyreal / vmpc-juce

JUCE implementation of VMPC2000XL
GNU General Public License v3.0
86 stars 7 forks source link

MIDI Clock Output severe jitter #59

Closed deviant-syndrome closed 1 year ago

deviant-syndrome commented 1 year ago

Environment: macOS Catalina Built from sources using XCode toolchain Standalone Application Issue

Seems, like MIDI ticks sent from sequencer are transported to target MIDI out with some irregularities.

Steps to reproduce:

  1. Run standalone application
  2. Select some MIDI-bridge for MIDI output (IAM driver bus works on MacOS)
  3. Make sure Sync Out is set to MIDI Clock
  4. Create a 1 bar sequence
  5. Add 16 copies of this sequence to a song, thus have 16 bars of signal
  6. Run any DAW, make it sync to external clock. Use the MIDI out of VMPC as MIDI Clock source
  7. Run the song on MPC
  8. The sequence will play 16 bar and stop. VMPC sequence will stop at 017.01.00
  9. Notice, that DAW bar counter would stop at around 017.01.30, meaning that the clock was lagging

This jitter is clearly audible, when you, for example sync some VST arpeggiator or drum machine to Standalone VMPC. After about 6 bars, they start to drift.

To eliminate the possible influence of third-party MIDI bridge to clock jitter, I also tried to sync, using a DAW (Reason) as a clock source through the same pipeline. It was rock solid.

According to the code, MIDI out is produced via an advanced concurrent non-blocking queue, which is "block-free", but not "wait free" or something. So, probably, MIDI Clock should be sent via separate high-priority pipeline.

P.S. I completely understand, that

I can try my hand at cobbling something together on this topic, if you could give me any hints, where I should start digging. Or we can just close this issue as "won't fix" if this is too overwhelming.

Thank you.

izzyreal commented 1 year ago

Could you try with this diff in mpc?

diff --git a/src/main/sequencer/FrameSeq.cpp b/src/main/sequencer/FrameSeq.cpp
index 976e57a9..78bc1b49 100644
--- a/src/main/sequencer/FrameSeq.cpp
+++ b/src/main/sequencer/FrameSeq.cpp
@@ -337,6 +337,8 @@ void FrameSeq::sendMidiSyncMsg(unsigned char status)
 {
     auto midiSyncMsg = status == ShortMessage::TIMING_CLOCK ? midiSyncClockMsg : midiSyncStartStopContinueMsg;

+    midiSyncMsg->setMessage(status);
+
     midiSyncMsg->bufferPos = tickFrameOffset;

     if (syncScreen->getModeOut() > 0)
izzyreal commented 1 year ago

And to be sure, are you running the Release build?

izzyreal commented 1 year ago

Ah never mind, I tried it and it doesn't help. I'll work on it. But please take a look if you can. I think it may be related to how there is no synchronization between the continuously emitted clocks in MidiClockEmitter running on the audio thread, and the moment you start playing a sequence due to some keyboard or mouse input off the main thread.

izzyreal commented 1 year ago

Update: Looked into this a bit, will require quite a bit more thought and effort. I hope to get to a proper fix soon.

deviant-syndrome commented 1 year ago

Hey. Thank you for looking into this.

Not sure, if this is any help, but I decided to provide a more accurate picture of MIDI Clock jitters here.

MIDI Clock is 24 ppq, meaning that for 120BPM tempo, the duration between pulses should be around 20,8(3) ms. I quickly put together a simple MaxMSP patch to measure the actual pulse lengths produced by VMPC. This paints a bit of disturbing picture. I have no idea where did those 10ms intervals came from.

MIDI Clock VMPC Original

For comparison, on the same setup I used Reason DAW to send MIDI clock, and I got much more stable results.

MIDI Clock Reason DAW

Also, what I've noticed from the code, it seems, that there are actually 2 separate places, from where we are sending MIDI clock events. One is in EventHandler, where, I guess, those are generated from the sequencer's clock events (did not find the place, where they are put into the sequence, though) Another is MidiClockEmitter, that is also sends MIDI event directly to outputs, but it is driven by internal clock which is ticking at 96 ppq!

izzyreal commented 1 year ago

Awesome that you've made those charts! It visualizes the problem very well.

Regarding EventHandler, good catch. The else if (mce) branch is actually dead. It should have been removed as part of implementing MIDI clock via MidiClockEmitter.

I'm now going to try to follow this: http://midi.teragonaudio.com/tech/midispec/seq.htm. I suspect at least one problem in the current implementation to be that it's not allowing 1ms or more between the MIDI start and the next clock. I have some ideas how to do this:

MidiClockEmitter and FrameSeq will be integrated, and FrameSeq will be continuously running. When a user input message on the message thread should result in a "sequencer starts playing", FrameSeq will enqueue a MIDI start event 1ms before the next MIDI clock will be enqueued by it. If at the start of a buffer the next MIDI clock comes less than 1ms later, the timing of the MIDI start event shifts 4 ticks ahead, so 1ms before the MIDI clock after the next one.

What do you think?

Do you also experience that it takes a while for VMPC to sync up with a DAW's metronome, but it slowly becomes steady? My hope is that the extreme deviations in your charts are caused by not allowing 1ms, but I haven't measured anything yet.

I will also verify that the MIDI events that JUCE emits carry the correct buffer offset.

deviant-syndrome commented 1 year ago

Hey! Ok, at least, MIDI clock handling process within the VMPC is a bit more clear to me now.

Regarding enqueuing MIDI start before next MIDI tick makes sense to me, as it seems to be compliant with the standard. So, that should be the right thing to do.

In terms of my experience with syncs, I had quite the opposite, it starts steady, but then they deviate. To explore even further, I also plotted the graph that shows VMPC's MIDI clock pulse length over time. What I see, is that this clock drop-outs to ~10ms happen over equal periods of time. Strange.

image
izzyreal commented 1 year ago

I think I found the main culprit for the jitter. JUCE is by default not configured to use the highest precision that it can for MIDI out in standalone.

You can call startBackgroundThread() against a MIDI-output to enable a high-precision timer thread for it. The easiest way to hack it into JUCE for experimentation is by adding defaultMidiOutput.startBackgroundThread() after this line: https://github.com/juce-framework/JUCE/blob/e49fb38d44622435f39f486530e84f84df20c8af/modules/juce_audio_devices/audio_io/juce_AudioDeviceManager.cpp#L1204

You will see that without this background thread the precision is tied to the buffer size. Smaller buffer sizes give higher precision than bigger ones. By enabling the background thread, I found the precision becomes independent from buffer size.

Additionally I made this "reference implementation": https://github.com/izzyreal/juce-sync-test It only plays a metronome (press any key to start/stop) and sends MIDI start/stop and clock.

If we can get the reference implementation to perform as desired, after that we can make VMPC2000XL congruent with that performance.

izzyreal commented 1 year ago

When you have time, can you create a histogram for https://github.com/izzyreal/juce-sync-test like you did earlier for VMPC2000XL? I'm curious if it matches the distribution you got from Reason.

deviant-syndrome commented 1 year ago

Yes, sure. I'll try to do this in the following couple of days.

deviant-syndrome commented 1 year ago

Good news. I used the reference implementation to measure the pulse length of MIDI clock, here is what I concluded:

  1. With the juce_AudioDeviceManager.cpp left intact, I observed pretty much the same picture with regular dropouts to 10 ms. However...
  2. If I add the startBackgroundThread() as you recommended, it shows quite a remarkable pulse length stability.

image

So far, even better than Reason, probably because the reference implementation is minimal and does not have any DAW-overheads.

I also tested MIDI transport functionality along with the clock on a VST sequencer. VST sequence plays in sync with the reference implementation's metronome without any noticeable problems. Transport works correctly as well.

Thank you!

izzyreal commented 1 year ago

Very cool 😃 I'll try to wrap up a PR soon to get VMPC2000XL to perform the same. Most likely it will be a combination of changes to mpc in tandem with vmpc-juce. Thanks again!

izzyreal commented 1 year ago

Please check the midi-sync branch on mpc and vmpc-juce, where I've integrated the changes that should make it perform like the reference implementation. I'm getting not so terrible results syncing to Ableton Live, see the video.

https://user-images.githubusercontent.com/3707432/215333450-573216dc-704b-4d83-9c5a-8e3fb1d4ec5c.mov

There are 2 pull requests: https://github.com/izzyreal/vmpc-juce/pull/61 https://github.com/izzyreal/mpc/pull/124

The diff is not very hygienic, sorry for that. I came across some slightly related clean-up.

deviant-syndrome commented 1 year ago

Aaaaalright.... So, MIDI clock is quite solid now.

Screenshot 2023-01-31 at 9 12 11

Also, MIDI transport and arpeggiator lock seems fine to me as well. Please, don't pay attention to arpeggiator position moving out of sync with the sound, this is just a desktop recording issue.

https://user-images.githubusercontent.com/86923754/215698511-39454a79-4103-43c7-8423-235811de3ac6.mov

I skimmed through the PRs code, I generally agree with the elaborate logic of the new FrameSeq. Do not know, if I can provide you with a full code-review, though, as my C/C++ audio DSP expertise is quite limited.

Seems, once those PRs are whipped into shape and merged, this issue can be closed Thank you very much for all the work you've done on this topic.

izzyreal commented 1 year ago

Funky jam in your video 🤘 Thanks for skimming the PR and taking more measurements! What do you mean by "whipped into shape"? I was going to merge them as is, so if you noticed anything odd let me know.

deviant-syndrome commented 1 year ago

Heh. Thank you. Yeah, by "whipping into shape" I mean some purely cosmetic improvements, I added them as comments to the PRs, but feel free to disregard, of course, really minor things.

Again, not really eligible to comment on the code quality here, so I would assume, it's good. Thanks, again

izzyreal commented 1 year ago

Good catches in the comments I've seen. I'll whip it into shape and merge!

izzyreal commented 1 year ago

Resolved in https://github.com/izzyreal/mpc/pull/125 and https://github.com/izzyreal/vmpc-juce/pull/61