KeyControl: fix keylock/unlock bugs, reset pitch_adjust #4710

ronso0 · 2022-04-03T19:19:20Z

Fixes unintented pitch shift when un/locking repeatedly with "lock original key" and "unlock, keep current key", see #4710 (comment) by resetting pitch_adjust when unlocking to linear pitch and locking to original key.

adjust pitchRoundtrip test
add test for "Unlock, keep current key" (or include it in pitchRoundtrip test)

ronso0 · 2022-04-03T19:32:12Z

back to draft because I am also looking into another issue with key un/lock and out-of-bounds rate changes (pitch/key can not be reset easily)

src/engine/controls/keycontrol.cpp

ronso0 · 2022-04-05T00:13:16Z

The other issue I'm attempting to fix is this:
When I lock and unlock the key often, with rate changes in between, during a set my pitch != 0.0 indicator flashes even though I just restored the original key. Now I finally managed to reproduce this with a script, and also manually with the GUI +/- buttons.
Appearantly the crucial part is to move the rate slider outside the GUI/controller range
Edit and use reversed pitch slider.
MIDI-Through test script: https://gist.github.com/ronso0/7cc9e421ded49e19e1c47192cfe7d523

Preferences:
- Keylock: lock current key
- Keyunlock: keep current key
- rate range: 8%, slider down increases rate
Procedure:
- deck1 keylock OFF, rate centered, no prior rate or key adjustments on that deck!
- load track
- move rate slider out of bounds (via Sync or scripting engine.setValue(group, "rate", 1.1))
- lock key
- reset key to original with reset_key (pitch = 0.0)
- move rate slider a bit
- unlock key (pitch should still be 0.0)
- pitch is something like 1,735221e-17

See 0a6d00d
I don't know how to avoid the calculation round-trip of m_pitchRateInfo members in the first place without making updateRate() even more complex. Hope this solution is okay.

ronso0 · 2022-04-23T21:28:46Z

ping. Any more comments?

ronso0 · 2022-04-23T21:47:46Z

I've just split the last commit into two to make it easier to review.

I'll also manual testing again with all 4 keylock/unlock combinations.

Swiftb0y · 2022-04-24T14:40:07Z

src/engine/controls/keycontrol.cpp

+#define ENABLE_DEBUG_OUTPUT false
+


since C++17, conditional compilation is usually better done using if constexpr instead

Suggested change

#define ENABLE_DEBUG_OUTPUT false

constexpr bool kEnableDebugOutput = true;

The difference is that the code in #if is ignored by the compiler, the code in "if constexpr" is checked and optimized away later.

True, but it doesn't make much of a difference in terms of what ends up in the binary... Metaprogramming using the C++ preprocessor is discouraged from (in C++) AFAIK.

Alright, thanks!

The preprocessor variant is not type safe. But the preprocessor has the benefit, that you can define these macros from outside (compiler commandline call). But here the define is in the source file.

src/engine/controls/keycontrol.cpp

Swiftb0y · 2022-04-24T15:23:52Z

src/engine/controls/keycontrol.cpp

+    // If we just unlocked the original key with speed != +-1.0 we may encounter wrong
+    // decimals (e.g. 1 - 1.73123e-09) after pitchRatio calculation round-trip.
+    // Round to 1.0 to avoid false positive pitch offset (pitchOctaves != 0).
+    double pitchRatioDiffTo1 = fabs(1.0 - m_pitchRateInfo.pitchRatio);
+    if (0.0 < pitchRatioDiffTo1 && pitchRatioDiffTo1 < pow(10, -9)) { // 0.000000001
+        m_pitchRateInfo.pitchRatio = 1.0;
+    }


I'm struggling to understand the reasoning behind this because I struggle to understand the surrounding code. m_pitchRateInfo.pitchRatio is basically imprecise because of rounding errors? But only around 1.0?

Basically roundtrip rounding erros, yes.
Tbh I didn't manage to understand how all the ratios relate in all situations either because I'm still confused by the naming, but with debug output enabled it's at least easy to trace how the m_pitchRateInfo values change. In #4710 (comment) I outlined the circumstances and the procedure to reproduce the imprecision.

The issue is that the new statement in line 216 is designed to make m_pitchRateInfo.pitchRatio = 1, but it is done indirect via other variables. It would be straight forward and more easy to understand to set it 1 explicit. This would require more changes in the code though.

src/engine/controls/keycontrol.cpp

daschuer · 2022-04-24T20:20:12Z

src/engine/controls/keycontrol.cpp

+    // If we just unlocked the original key with speed != +-1.0 we may encounter wrong
+    // decimals (e.g. 1 - 1.73123e-09) after pitchRatio calculation round-trip.
+    // Round to 1.0 to avoid false positive pitch offset (pitchOctaves != 0).
+    double pitchRatioDiffTo1 = fabs(1.0 - m_pitchRateInfo.pitchRatio);
+    if (0.0 < pitchRatioDiffTo1 && pitchRatioDiffTo1 < pow(10, -9)) { // 0.000000001
+        m_pitchRateInfo.pitchRatio = 1.0;
+    }


The issue is that the new statement in line 216 is designed to make m_pitchRateInfo.pitchRatio = 1, but it is done indirect via other variables. It would be straight forward and more easy to understand to set it 1 explicit. This would require more changes in the code though.

ronso0 · 2022-04-26T19:13:46Z

Uff, there is another bug:
(best experienced with src/test/sine-30.wav : )

keylock: lock original key
keyunlock: keep current key
keylock disabled, enable repeat for continous stream

play, change rate to like +4%, no manual key change
lock key => key reset to default, pitch = 0.0
unlock key => no key change, as expected with keep current key
lock key => key lowered, should reset to original key + pitch slider offset
repeating 3. and 4. will lower the key step by step

I'll go with

It would be straight forward and more easy to understand to set it 1 explicit. This would require more changes in the code though.

in order to rebase this whole thing.

ronso0 · 2022-04-28T22:02:34Z

Uff, there is another bug: (best experienced with src/test/sine-30.wav : )

keylock: lock original key

keyunlock: keep current key

keylock disabled, enable repeat for continous stream

play, change rate to like +4%, no manual key change

lock key => key reset to default, pitch = 0.0

unlock key => no key change, as expected with keep current key

lock key => key lowered, should reset to original key + pitch slider offset

repeating 3. and 4. will lower the key step by step

When I fix that bug by setting both pitchRatio and pitchTweakRatio manually for each keylock/unlock and locked/unlocked case...
all manual tests succeed, Mixxx behaves as expected.
BUT the Pitchroundtrip test fails here:

Note: Google Test filter = EngineBufferTest.PitchRoundtrip
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from EngineBufferTest
[ RUN      ] EngineBufferTest.PitchRoundtrip
/home/ronso/Downloads/mixxx_source/src/master/src/test/enginebuffertest.cpp:75: Failure
Expected equality of these values:
  0.5
  ControlObject::get(ConfigKey(m_sGroup1, "pitch"))
    Which is: 0

The test assumes that pitch_adjust remains at x.y even if the original key is locked.

@daschuer Unfortunately some rules aren't clear to me from the code comments / manual (pitch_adjust) and I'd appreciate if you could clarify:
How is the pitch_adjust knob supposed to behave?
If we assume that user have that mapped to a rotary knob, should the hardware knob always stay in sync with the internal state even when magic buttons are pressed?
Unfotunately the current behaviour is inconsistent:

the knob is static when I "lock at original pitch" after pitch_adjustments (though it doesn't reset to original key, for that I'd need to reset the adjust knob, too)
the knob moves when I "unlock, keep current key"
the knob moves when I press "match key" or "reset key"

This is very confusing.
Should keylock/unlock be treated differently than match/reset?

To be clear, in this stable banch I don't necessarily want to refactor updateRate to fix that bug and make the test still pass / meet established user expectations.

I don't want to open a can of worms and refactor the stable branch just to fix that bug (and make tests still pass).

hardware knob always stay in sync with the internal state

Edit
We could say the knob should move if there is an audible change (pitch changes):

lock original pitch
unlock, reset to original pitch
match key
reset key

Edit 2
If we assume someone has pitch_adjust mapped to a knob softTakeover should be activated because pitch_adjust can jump when using official controls.
The only official mapping in 2.3 that uses pitch_adjust (Traktor Kontrol S3) has softTakeover activated. There is also the Roland DJ-505 mapping which has a Pitch Play mode, but that doesn't take the actual pitch_adjust value into account anyway.
Therefore I consider it low-risk to change the behaviour, even if that could affects users with custom mappings.

daschuer · 2022-05-04T06:48:06Z

I have just tested with the 2.3 and the control is implemented on top of keylock. It is not changed if the pitch is adjusted by keylock. For my understanding this behavior is desired, because you will touch the control likely to adjust the pitch from what you have currently.
Scenario:

Adjust the tempo from matching speed
Hit Keylock to avoid chipmunk voices
Adjust the pitch for a matching key.

First snap the control from soft-takeover and then notice that the range of +-3 Semitones has already been reached would feel cumbersome. Soft-Takover is required in mapping though, because the control is reset after loading new tracks.

The behavior is described here:

mixxx/src/engine/enginebuffer.cpp

Line 937 in 74ec345

// The Pitch_Adjust knob does not reflect the speedSliderPitchRatio.

and here

mixxx/src/engine/controls/keycontrol.cpp

Line 37 in eff27ab

// pitch_adjust is the distance to the linear pitch in semitones

The manual is not clear and can make use of an update:

Adjust the pitch in addition to the :term:tempo slider pitch.

daschuer · 2022-05-04T07:04:25Z

Here is the manual update: mixxxdj/manual#480

daschuer · 2022-05-04T12:18:58Z

Unfotunately the current behaviour is inconsistent:

the knob is static when I "lock at original pitch" after pitch_adjustments (though it doesn't reset to original key, for that I'd need to reset the adjust knob, too)

That can be expected, since the pitch_adjust is applied to all other. Did you expect that the knob is reset and goes out of sync, in such situation? I can imagine to change that behavior, since the adjusted pitch becomes pointless in that case.

the knob moves when I "unlock, keep current key"

We need an instance that holds the remaining offset. I can confirm that this is confusing, because the know goes out of sync and there is no indicator at all (except in Shade) that we are in such a blended situation

the knob moves when I press "match key" or "reset key"

That the knob is moving after match key can be expected in the same way the rate slider moves when using sync.
I think the same is true for "reset key", but I agree that this is inconsistent with the unlock key-lock behavior.

The "unlock, keep current key" mode is part of an "allow toggling keylock during playback" use case. It allows to adjust the tempo without the pitch, and without a pitch jump during enabling. Not sure if this is used at all in the wild.

During keylock enabled, the the rate slider becomes temporary a tempo only slider. I think the user user expects that the pitch_adjust control is an independent pitch control for that state. If we use pitch_adjust purely for the distance to the linear pitch, it will move out of sync whenever you move the rate slider. I think this was the main reason to apply it on top of keylock to avoid this.

ronso0 · 2022-05-06T20:06:11Z

the knob is static when I "lock at original pitch" after pitch_adjustments (though it doesn't reset to original key, for that I'd need to reset the adjust knob, too)

That can be expected, since the pitch_adjust is applied to all other. Did you expect that the knob is reset and goes out of sync, in such situation?

Yes, I'd expect that that the pitch is reset to default, exactly as labeled in the preferences: Keylock mode "Original key". That would be an easy way to reset the key if you don't have reset_key mapped, for example after having matched the key.
If you have pitch_adjust mapped you'd then have access to the full +-3 semitones range.

Also, this fixes the key shift bug with "lock original" + "unlock keep current".

The "unlock, keep current key" mode is part of an "allow toggling keylock during playback" use case. It allows to adjust the tempo without the pitch, and without a pitch jump during enabling. Not sure if this is used at all in the wild.

I use it : ) The motivation to implement it was that I have keylock enabled pretty much all the time, but sometimes I want to produce smooth pitchbend effects just with the jog wheel (or with the tempo slider). Then lock again, if necessary re-sync the beats with the jogwheel.
In conjunction with "Lock current key" this prevents key jumps when toggling the lock. I have reset_key mapped, as well as pitch_adjustup/down and never ran into issues (except the rounding issue I'm trying to fix here).

daschuer · 2022-05-06T20:28:45Z

Yes, I'd expect that that the pitch is reset to default,

OK, I have no objection to this change. It may actually feel more natural, even though it is a point release of a mature branch. Can we still consider it as part of a bugfix?

I use it : )

Great to hear. It this an unique selling point of Mixxx? Maybe a topic of a future news post ;-)

ronso0 · 2022-05-06T21:43:00Z

Great to hear. It this an unique selling point of Mixxx? Maybe a topic of a future news post ;-)

I started DJing on Linux so don't and didn't use any other soft than Mixxx : )
I just found Traktor users asking to revert the "Engaging Key Lock, resets a potential key offset" "fix"
Disable "Engaging Key Lock, resets a potential key offset"
Serato, Rekordbox, idk
Traktor Pro 3 appears to have separate MIDI controls: Key Lock and Key Lock (Preserve Pitch), though no global preferences option for that 🤷 So at least Mixxx rules out Trakor in that regard since we have "Lock original" and "Keep current key" since #1222 : )

Can we still consider it as part of a bugfix?

It fixes the continuous key shift when toggling keylock repeatedly with "Lock original" and "Unlock, keep current".
So I'd say yes.

even though it is a point release of a mature branch.

I think only users who have pitch_adjust knob mapped to a regular rotary knob¹ get a minor benefit from the current behaviour as that knob is still in sync even after a sudden key change. Those who don't have it will have to resort to key_reset.
¹there is only one official mapping with that

So I think it's the same situation as with the crossfader reset after AutoDJ runs out:
users may take it as it is, but for others the current behaviour is an issue.

ronso0 · 2022-05-06T22:08:56Z

I noticed another bug (and I have no idea why we/I implemented it that way in #1222):
key unlocked, rate off center, apply some pitch_adjust (match_key), disable keylock "Reset to linear pitch"
= pitchTweakRatio is reset but not applied to pitch_adjust
= next time touching that knob pitch would jump

ronso0 · 2022-05-06T22:11:37Z

Furthermore I noticed the pitch/keylock tests don't consider the unlock mode, thus they are partially pointless (assuming the default unlock mode).
I will adjust the PitchRoundtrip test and write a new one to test the "Keep current" unlock mode.

ronso0 · 2022-05-06T22:12:57Z

Considering I started this to fix a small initialization bug and minor anoyance with my use case this is growing rather big...

ronso0 · 2022-05-11T23:07:25Z

I think I will split this up:

the first few commits (init keylock from config, restore pitchRatio precision)
the other bugs incl. adjusted tests

So the simple fixes can go to the next bugfix release asap.

daschuer · 2022-05-12T06:57:02Z

Yes, I did also consider this. This way we can get out 2.3.2 in de upcoming week or such.

ronso0 · 2022-05-12T13:23:21Z

2.3.3, yes.

#4756

github-actions · 2023-09-19T00:06:54Z

This PR is marked as stale because it has been open 90 days with no activity.

daschuer · 2023-09-19T05:23:18Z

Oh, sorry this has stalled. Can you resolve it the conflicts?
I will test and review it again.

daschuer · 2023-09-19T06:28:42Z

The conflicts are only in the testing skin.
I have just given this a short spin and I did not discover any issues. So you may want to put this is not a merge able state.

daschuer · 2023-09-19T06:43:27Z

Is the track load behavior still in the desired state?
I am unsue what is desired in the middle ground of not putting sliders out if sync but also start with a new track without supprises. Please verify everything matches your use case.

ronso0 · 2023-09-19T23:04:58Z

Everything still looks okay when testing with my script.
I'll give this a more thorough spin next weekend with my controller.

I added a simple test for KeepLockedKey mode and removed the skin commit.

ronso0 · 2023-09-19T23:07:51Z

I am unsue what is desired in the middle ground of not putting sliders out if sync but also start with a new track without supprises.

I'm not that much into what I did here after my last edit months ago X), please give my some steps to reproduce / understand what you mean.

ronso0 · 2023-09-19T23:42:57Z

/home/runner/work/mixxx/mixxx/src/test/enginebuffertest.cpp:62: Failure
Expected equality of these values:
  0.5
  ControlObject::get(ConfigKey(m_sGroup1, "pitch"))
    Which is: 0.5

??
Test passes only on macOS ARM

ronso0 · 2023-09-19T23:51:28Z

Everything still looks okay when testing with my script.

In a manual test though I ended up with pitch_adjust != 0 again (couldn't reproduce until now, but I didn't do many runs):

Lock: keep current
Unlock: keep locked key

raise pitch to 10% (outside pitch slider range)
lock, unlock, do manual key ajustments etc.
reset rate
reset key
= pitch_adjust != 0, just slightly off (like the rounding trick didn't work)
reset pitch_adjust slider = pitch slightly off
reset pitch slider = pitch_adjust slightly off
etc.
= no way to completely reset the pitch controls (except LOCK, reset key, UN-lock)

🤷

Will see if I can reproduce with my controller.

ronso0 · 2023-09-27T21:10:55Z

/home/runner/work/mixxx/mixxx/src/test/enginebuffertest.cpp:62: Failure
Expected equality of these values:
  0.5
  ControlObject::get(ConfigKey(m_sGroup1, "pitch"))
    Which is: 0.5

?? Test passes only on macOS ARM

Anyone has a clue what's going on?

src/test/enginebuffertest.cpp

ronso0 · 2023-09-28T11:04:35Z

Alright, ASSERT_NEAR makes the tests succeed.
Ready to roll!

@daschuer wanna take a final look?

don't have time for a detailed review, but after taking a quick glance, I have not found any obvious issues.

ywwg · 2023-12-07T01:09:35Z

src/engine/controls/keycontrol.cpp

-                // If 'current' aka 'not original' key was locked
-                // TODO(ronso0) Why does it depend on keylock mode?
-                if (m_keylockMode->get() == kLockCurrentKey) {
-                    // reset to linear pitch


~~it's not that we reset to linear pitch, it's that we allow rate slider changes to affect the musical pitch. But 0.0 rate adjustment may no longer the original key.~~

EDIT: nevermind I misread the switch-case

ywwg · 2023-12-07T01:15:43Z

I wrote a lot of text that was all wrong -- I think this fix is correct! thank you <3

ronso0 · 2023-12-07T16:28:30Z

🎉

ronso0 marked this pull request as draft April 3, 2022 19:30

ywwg reviewed Apr 4, 2022

View reviewed changes

src/engine/controls/keycontrol.cpp Outdated Show resolved Hide resolved

daschuer requested changes Apr 4, 2022

View reviewed changes

src/engine/controls/keycontrol.cpp Outdated Show resolved Hide resolved

ronso0 force-pushed the keylock-fix branch from 458d2d3 to 0a6d00d Compare April 4, 2022 23:59

ronso0 added minor bug engine labels Apr 8, 2022

ronso0 added this to the 2.3.3 milestone Apr 8, 2022

ronso0 force-pushed the keylock-fix branch from 3a8f785 to 57562f4 Compare April 23, 2022 21:46

ronso0 marked this pull request as ready for review April 23, 2022 21:47

Swiftb0y previously requested changes Apr 24, 2022

View reviewed changes

daschuer reviewed Apr 24, 2022

View reviewed changes

ronso0 mentioned this pull request May 12, 2022

KeyControl: init keylock from config, fix pitchRatio rounding issue #4756

Merged

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Sep 19, 2023

daschuer modified the milestones: 2.3.3, 2.4.0 Sep 19, 2023

Merge remote-tracking branch 'mixxx/2.4' into keylock-fix

b99e29c

ronso0 force-pushed the keylock-fix branch from 2966f46 to 0c2a75e Compare September 19, 2023 23:01

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Sep 20, 2023

ronso0 mentioned this pull request Sep 25, 2023

reset_key behaviour depends on keylock #12027

Open

Holzhaus requested changes Sep 27, 2023

View reviewed changes

src/test/enginebuffertest.cpp Outdated Show resolved Hide resolved

Keylock: add test for KeyunlockMode::KeepLockedKey

2067b8b

ronso0 force-pushed the keylock-fix branch from 3992fbe to 2067b8b Compare September 28, 2023 07:20

DEBUG (Remove!): add pitch_adjust slider to LateNight

4138858

KeyControl: std::unique_ptr, parented_ptr

cbe31d4

ywwg reviewed Dec 7, 2023

View reviewed changes

ywwg approved these changes Dec 7, 2023

View reviewed changes

ywwg merged commit 446cee4 into mixxxdj:2.4 Dec 7, 2023

ronso0 deleted the keylock-fix branch December 7, 2023 16:28

ronso0 mentioned this pull request Dec 8, 2023

svg viewbox fix #12407

Merged

	#define ENABLE_DEBUG_OUTPUT false
	constexpr bool kEnableDebugOutput = true;

KeyControl: fix keylock/unlock bugs, reset pitch_adjust #4710

KeyControl: fix keylock/unlock bugs, reset pitch_adjust #4710

Conversation

ronso0 commented Apr 3, 2022 • edited Loading

ronso0 commented Apr 3, 2022

ronso0 commented Apr 5, 2022 • edited by Swiftb0y Loading

ronso0 commented Apr 23, 2022

ronso0 commented Apr 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Apr 26, 2022

ronso0 commented Apr 28, 2022 • edited Loading

daschuer commented May 4, 2022 • edited Loading

daschuer commented May 4, 2022

daschuer commented May 4, 2022

ronso0 commented May 6, 2022 • edited Loading

daschuer commented May 6, 2022

ronso0 commented May 6, 2022 • edited Loading

ronso0 commented May 6, 2022

ronso0 commented May 6, 2022

ronso0 commented May 6, 2022

ronso0 commented May 11, 2022

daschuer commented May 12, 2022

ronso0 commented May 12, 2022

github-actions bot commented Sep 19, 2023

daschuer commented Sep 19, 2023

daschuer commented Sep 19, 2023

daschuer commented Sep 19, 2023

ronso0 commented Sep 19, 2023

ronso0 commented Sep 19, 2023

ronso0 commented Sep 19, 2023 • edited Loading

ronso0 commented Sep 19, 2023 • edited Loading

ronso0 commented Sep 27, 2023

ronso0 commented Sep 28, 2023

ywwg Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

ywwg commented Dec 7, 2023

ronso0 commented Dec 7, 2023

ronso0 commented Apr 3, 2022 •

edited

Loading

ronso0 commented Apr 5, 2022 •

edited by Swiftb0y

Loading

ronso0 commented Apr 28, 2022 •

edited

Loading

daschuer commented May 4, 2022 •

edited

Loading

ronso0 commented May 6, 2022 •

edited

Loading

ronso0 commented May 6, 2022 •

edited

Loading

ronso0 commented Sep 19, 2023 •

edited

Loading

ronso0 commented Sep 19, 2023 •

edited

Loading

ywwg Dec 7, 2023 •

edited

Loading