Data race and precision issues in jog control object interaction from controller scripts

[robbert-vdh]

Bug Description

Mentioned in #11199. There are some potential issues with the way the jog control object is interacted with from controller scripts. This value is an accumulator that is read and then set to zero in RateControl::getJogFactor(). There are two issues with the way it's used now:

• Control scripts overwrite the value, rather than adding to it. This means that if the buffer size is large and multiple jog wheel ticks have occurred since the time the last buffer was processed, the jog values from older ticks will have been overwritten by the later ones and are thus discarded. This can be fixed by storing the value in the control object as an atomic double, and using a fetch-and-add to atomically add to it from the control script.
• In the getJogFactor() function the control object is read, checked, and then set to 0 if the old value was nonzero. The control script could have changed the value in between the read and set, which causes that value to get thrown out. This can be solved by using an atomic exchange operation to get the value and reset the control object to 0 in one operation.

Additionally, but this has nothing to do with race conditions, the smoothing on the jog control should probably be modified. The default is a linear-phase box filter with a latency of 12.5 buffers. This causes nudging to feel unresponsive, especially at larger buffer sizes. A simple but effective replacement would be a simple first order low-pass filter/exponential moving average that takes the buffer length into account..

Version

all current versions

OS

No response

[daschuer]

Great, that you have found it. Maybe this is the cause of scratching instability?

For my understanding this comment along with the set to 0 is wrong. Maybe it was a workaround for a broken controller script?

mixxx/src/engine/controls/ratecontrol.cpp

Line 370 in eff27ab

// Since m_pJog is an accumulator, reset it since we've used its value.

The "jog" co should always contain the current rate (velocity) of the jog wheel, not the number of degrees it has tuned since the last callback. When the controller is updated twice, the value must not be changed, when the velocity has not changed.

The bug happens now, when we have two consecutive Audio Callback without a controller callback, because the drops to zero, which is wrong.

The other issue it the jitter in case of changing jogwheel rates here it may be the case that an old value is read just before new value is written, which causes a dip in the rate ramp. I think the smoothing filter aim to deal with it but also lints this bug.

With 64 bit OSs the control objects we have no read and write API. So I think we should make sure that only the controller thread is reading.
Maybe instead of setting the value to zero we may use an 32 int overflow and a compare value to count continuously.

[robbert-vdh]

Great, that you have found it. Maybe this is the cause of scratching instability?

Scratching using engine.scratchTick() is fine. The reason why it sounds a bit wobbly is because of the filter used. You're hearing ringing, or oscillations around the target velocity as the filter settles in. Tuning the alpha and beta parameters can help dial this back a little, but another filter with little to no ringing may be a better choice. I haven't investigated this yet though so my only suggestion would be a boring old first order IIR low-pass filter/an exponential moving average.

Control scripts that directly modify the scratch2 control object will always exhibit erroneous behavior though for the reasons I explained in #11199.

For my understanding this comment along with the set to 0 is wrong. Maybe it was a workaround for a broken controller script?

No that is indeed correct! The jog control object contains an offset for the playhead, so this offset must be read and used only once. The issues are that between the reading and resetting the controller script may write to the control object, throwing away the value, and that multiple subsequent writes without a read (so if the control script's jog handler gets triggered at a significantly higher rate than the audio process function) then those values are thrown away.

This value is then processed through a low-pass filter to obtain a velocity. The currently used low-pass filter is not idea though since it uses a fixed 25 buffer smoothing period (which is 33.3 ms at a buffer size of 64 and a sample rate of 48 kHz, and over half a second at a buffer size of 512 samples).

To avoid the issue with writes getting lost and with a write happening between the read and reset the control object should use atomic operations. An atomic fetch-and-add operation should be used to increment the jog control object's value so duplicate writes don't get lost, and an atomic swap/exchange can be used in the rate control function to get the control object's value and to simultaneously reset it to 0.

[daschuer]

No that is indeed correct! The jog control object contains an offset for the playhead, so this offset must be read and used only once.

You describe the behavior how it is described. But this cannot work as you correctly pointed out and it is also not implemented most of the controller scripts. Like here for instance:

mixxx/res/controllers/Vestax-VCI-300-scripts.js

Line 496 in d8826ca

engine.setValue(this.group, "jog", jogDeltaSign * normalizedAbsJogDelta * VestaxVCI300.MIXXX_JOG_RANGE);

This is not even the case in your script:

mixxx/res/controllers/Traktor-Kontrol-S3-hid-scripts.js

Line 1310 in ce70fa0

engine.setValue(this.activeChannel, "jog", velocity * TraktorS3.JogSpeedMultiplier);

A Quick search only reveals:

mixxx/res/controllers/Denon-MC4000-scripts.js

Line 127 in 52bcaf1

var jogAbsolute = jogDelta + engine.getValue(group, "jog");

mixxx/res/controllers/Hercules-DJ-Console-4-Mx-scripts.js

Line 1435 in 52bcaf1

engine.setValue(group, "jog", direction + engine.getValue(group, "jog"));

I consider them as broken, because it results in a different Jog sensitivity depending on the audio buffer size.
In that case a moved distance between the callbacks is directly translated into a velocity without considering the time.

Some insights:
The controller polling frequency is not synced with the audio. Actually it is only the midi input buffer polled, that is filled in the pace the controller provides updates. So we you see we have two stages that introduces a jitter error, the midi input buffer and the "jog" CO

See also:
#6952
#6951
#6950

We consider to offer that topic as a GSoC Projec to fix the issue at the root:
https://github.com/mixxxdj/mixxx/wiki/GSOC-2023-Ideas#scratch-smoothing
Do you have interest to help?

[robbert-vdh]

Like I said, those are all incorrect. These two you linked:

mixxx/res/controllers/Denon-MC4000-scripts.js

Line 127 in 52bcaf1

var jogAbsolute = jogDelta + engine.getValue(group, "jog");

mixxx/res/controllers/Hercules-DJ-Console-4-Mx-scripts.js

Line 1435 in 52bcaf1

engine.setValue(group, "jog", direction + engine.getValue(group, "jog"));

Are also broken. The control object's value may change between the two calls, which means that this version may move the playhead too much rather than too little.

The fix as I outlined earlier would be to:

1. Store control object values as std::atomic_doubles if they aren't already.
2. Add a get_and_reset() method that performs and atomic swap to set the value to 0 while simultaneously returning the old value.
3. Add an add_value()/increment_value() that performs and atomic fetch-and-add that adds a value to the control object's value atomically.
4. Use the get_and_reset() in RateControl, use add_value() in control scripts.
5. And lastly the filter should probably be changed out for a filter that scales with the buffer's length and that doesn't introduce any latency like the current filter does.

[daschuer]

'jog" takes the velocity, not the distance passed like the last callback. That's why the solution of reading back the jog value in the controller mapping is wrong.

With the solution you have outlined, the jog sensivity depend on the audio buffer size which is not desired.

With the current velocity solution and long audio callbacks we may miss a jog update or read the same value twice. This is the jitter issue we may compensate.

[robbert-vdh]

Sending distance instead of velocity to the control object may result in more consistent timings and be the preferable approach since the filtering is only done on the audio thread, but using instantaneous velocity like it's doing now is not necessarily wrong. The data needs to be treated differently and the final result is slightly different, but the approaches are mostly equivalent. Either approach just moves the derivative step to a different place in the equation.