The synchronization based on BBT (bar, beat, tick) information provided by the JACK server was broken again (in soo many ways). To fix this (hopefully once and for all), I wrote a large integration test using which two Hydrogen instances interact with each other via the JACK server.
Unfortunately, this has some really bad UX consequences for Hydrogen: clicking a position on the ruler is not guaranteed anymore to make Hydrogen relocate to this position.
The reason is as follows: Hydrogen calculates the frame for the desired position based on the current tempo and measure information provided by the JACK Timebase master - which will lead to wrong results in case of tempo and measure changes within the master. But that is all we can base our work on. Hydrogen sends this frame to the JACK server, which will broadcast it without any BBT info. We relocate to the desired location. But in the next processing cycle the Timebase master fills the BBT information corresponding to the frame and broadcasts it. The frame position corresponding to the BBT information we will relocate next might not correspond to the frame we specified (e.g. the master did already pass at least one tempo change).
We could disable the position ruler in the presence of an external Timebase master. But I think this is no good. When e.g. taking care of having the same tempo markers in both Hydrogen and the master, relocation would still work fine.
UX changes
Two allow for the integration test to work properly, a couple of new CLI options had to be introduced
-O/--osc-port to use a custom OSC port in both hydrogen and h2cli.
-L/--log-file to provide a path to an alternative log file.
-T/--log-timestamps to add timestamps to all log messages.
Also, the CLI handling was unified between hydrogen and h2cli as well as simplified:
CLI option -d understand driver names regardless of capitalization.
h2cli option -V is now able to handle whitespaces between flag and argument.
h2cli long option for -k is renamed --drumkit -> --kit in order to align the naming with the one used in hydrogen CLI options.
Preferences option JackBBTSyncMethod was dropped
Previously we had two different approaches to react on BBT (bar, beat, tick) information provided by the JACK server: 1) const measure and 2) identical bars.
In 1) we convert BBT into a tick position and relocate to this very point. If patterns in Hydrogen have a different length than the measure used by the Timebase master, bar: 2 will not necessarily result in Hydrogen relocating into pattern 2. But on the upside, transport is consistent. There are no sudden jumps and it does not matter whether the Timebase master relocates to BBT X or lets transport roll from an arbitrary prior point till that position. Hydrogen will be in the exact same position in both cases.
In 2) the bar information would be mapped to our columns. This means a mismatch between pattern length and measure in the master would either result to jumps during playback or inconsistent position when either rolling or relocating to position X. That is no good UX. That is why we suggested in the manual to ensure pattern lengths always match measures in the master. But when doing so this approach yields the same results as 1). So, I just drop it to make things more simple.
Internal changes
The test is not designed to be run in the AppVeyor pipeline (due to missing JACK support in there) but can be run locally on a Linux machine using build.sh i. This sets the additional cmake flag WANT_INTEGRATION_TESTS, due to which the test won't be compiled in our build pipelines.
Debug messages in JackAudioDriver, AudioEngine, and TransportPosition are now hard-coded and can be activated by setting the e.g. JACK_DEBUG macro to 1 at the top of JackAudioDriver.cpp. Also, all three use distinct colors, which makes audio engine-based problems much faster and easier to debug.
The DiskWriterDriver is no longer using EVENT_PROGRESS to indicate success or failure (but only to indicate progress). This had several downsides. Our events are designed to be one-way messages between core and GUI. There is no way the core knows whether export was completed or failed. To complicate things, events are poped from the EventQueue. So, reading them at several places is off the table. Instead, a bool member of the DiskWriterDriver class is now used to indicated success/failure.
Log messages on level Locks do now include the thread ID.
TODO
[x] - Integration tests for JACK BBT synchronization
[x] - Rework Timebase state handling to avoid glitches
[x] - Send BBT information during repositioning in case we are JACK Timebase master
[x] - Honor all BBT constraints when validating BBT information
[x] - Fix JACK relocation tick rounding to not get trapped in a loop of relocations
[x] - Merge BBT synchronization methods
[x] - Properly handle relocations beyond the end of song with loop mode disabled
[x] - Calculate BBT information (inkl. current playing patterns) in JackTimebaseCallback for frame provided by JACK server as input (instead of the current transport position in the audio engine)
[x] - Fix relocation detection. Only changes in frame information apart from the expected increment are considered
[x] - Handle all potential errors in our JACK API calls
[x] - Fix BBT <-> transport position conversion
[x] - Define default resolution to allow for JackTimebaseCallback to work in case no song is present
[x] - JackAudioDriver::m_nTimebaseFrameOffset inconsistency not caught in integration test
[x] - Workaround for glitching frame bug in JackTimebaseCallback (when encountering an XRun during processing - can be provoked by too many log messages - the frame member in the jack_position_t member provided by the JACK server as input to JackTimebaseCallback does change during the function call. This results in us providing invalid BBT information tailored to the previous frame)
[x] Test transport consistency on song resizing -> in general this is not a problem as the difference in frames is already absorbed in TransportPosition::m_nFrameOffsetTempo, which is part of the relocation check. But since the rescaling is tick-based we get more out of sync and glichtes can not be avoided.
[x] Relocation using fast forward and backward buttons in Hydrogen in the presence of an external Timebase master is erratic
[x] Echos / audio engine inconsistencies on relocation in pattern mode in the presence of an external Timebase master
[x] Use tempo provided by Timebase master in Pattern Mode
The synchronization based on BBT (bar, beat, tick) information provided by the JACK server was broken again (in soo many ways). To fix this (hopefully once and for all), I wrote a large integration test using which two Hydrogen instances interact with each other via the JACK server.
Limitations
I'll do my best to provide a smooth JACK integration. But it turns out JACK does not even properly support tempo changes. Even though it encourage using them in the BBT synchronization.
Unfortunately, this has some really bad UX consequences for Hydrogen: clicking a position on the ruler is not guaranteed anymore to make Hydrogen relocate to this position.
The reason is as follows: Hydrogen calculates the frame for the desired position based on the current tempo and measure information provided by the JACK Timebase master - which will lead to wrong results in case of tempo and measure changes within the master. But that is all we can base our work on. Hydrogen sends this frame to the JACK server, which will broadcast it without any BBT info. We relocate to the desired location. But in the next processing cycle the Timebase master fills the BBT information corresponding to the frame and broadcasts it. The frame position corresponding to the BBT information we will relocate next might not correspond to the frame we specified (e.g. the master did already pass at least one tempo change).
We could disable the position ruler in the presence of an external Timebase master. But I think this is no good. When e.g. taking care of having the same tempo markers in both Hydrogen and the master, relocation would still work fine.
UX changes
Two allow for the integration test to work properly, a couple of new CLI options had to be introduced
-O/--osc-portto use a custom OSC port in bothhydrogenandh2cli.-L/--log-fileto provide a path to an alternative log file.-T/--log-timestampsto add timestamps to all log messages.Also, the CLI handling was unified between
hydrogenandh2clias well as simplified:-dunderstand driver names regardless of capitalization.h2clioption-Vis now able to handle whitespaces between flag and argument.h2clilong option for-kis renamed--drumkit->--kitin order to align the naming with the one used inhydrogenCLI options.Preferences option
JackBBTSyncMethodwas droppedPreviously we had two different approaches to react on BBT (bar, beat, tick) information provided by the JACK server: 1) const measure and 2) identical bars.
In 1) we convert BBT into a tick position and relocate to this very point. If patterns in Hydrogen have a different length than the measure used by the Timebase master, bar: 2 will not necessarily result in Hydrogen relocating into pattern 2. But on the upside, transport is consistent. There are no sudden jumps and it does not matter whether the Timebase master relocates to BBT X or lets transport roll from an arbitrary prior point till that position. Hydrogen will be in the exact same position in both cases.
In 2) the bar information would be mapped to our columns. This means a mismatch between pattern length and measure in the master would either result to jumps during playback or inconsistent position when either rolling or relocating to position X. That is no good UX. That is why we suggested in the manual to ensure pattern lengths always match measures in the master. But when doing so this approach yields the same results as 1). So, I just drop it to make things more simple.
Internal changes
The test is not designed to be run in the
AppVeyorpipeline (due to missing JACK support in there) but can be run locally on a Linux machine usingbuild.sh i. This sets the additionalcmakeflagWANT_INTEGRATION_TESTS, due to which the test won't be compiled in our build pipelines.Debug messages in
JackAudioDriver,AudioEngine, andTransportPositionare now hard-coded and can be activated by setting the e.g.JACK_DEBUGmacro to1at the top ofJackAudioDriver.cpp. Also, all three use distinct colors, which makes audio engine-based problems much faster and easier to debug.The
DiskWriterDriveris no longer usingEVENT_PROGRESSto indicate success or failure (but only to indicate progress). This had several downsides. Our events are designed to be one-way messages between core and GUI. There is no way the core knows whether export was completed or failed. To complicate things, events are poped from theEventQueue. So, reading them at several places is off the table. Instead, a bool member of theDiskWriterDriverclass is now used to indicated success/failure.Log messages on level
Locksdo now include the thread ID.TODO
JackTimebaseCallbackforframeprovided by JACK server as input (instead of the current transport position in the audio engine)frameinformation apart from the expected increment are consideredJackTimebaseCallbackto work in case no song is presentJackAudioDriver::m_nTimebaseFrameOffsetinconsistency not caught in integration testJackTimebaseCallback(when encountering an XRun during processing - can be provoked by too many log messages - theframemember in thejack_position_tmember provided by the JACK server as input toJackTimebaseCallbackdoes change during the function call. This results in us providing invalid BBT information tailored to the previous frame)TransportPosition::m_nFrameOffsetTempo, which is part of the relocation check. But since the rescaling is tick-based we get more out of sync and glichtes can not be avoided.MusEinstanceFixes #1953