From 900110ddd56c1f4277d87fc324f9c6e0da066fc6 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Sat, 1 Feb 2025 03:53:58 +0100 Subject: [PATCH 1/8] RateControl: remove unused notifySeek() from ReadAheadManager::getFilePlayposFromLog() is a dupe? received redundant seeks in PositionScratchController --- src/engine/readaheadmanager.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/engine/readaheadmanager.cpp b/src/engine/readaheadmanager.cpp index d26a9bbdc93..6b38e7ee6f8 100644 --- a/src/engine/readaheadmanager.cpp +++ b/src/engine/readaheadmanager.cpp @@ -283,22 +283,8 @@ double ReadAheadManager::getFilePlaypositionFromLog( } double filePlayposition = 0; - bool shouldNotifySeek = false; while (m_readAheadLog.size() > 0 && numConsumedSamples > 0) { ReadLogEntry& entry = m_readAheadLog.front(); - - // Notify EngineControls that we have taken a seek. - // Every new entry start with a seek - // (Not looping control) - if (shouldNotifySeek) { - if (m_pRateControl) { - const auto seekPosition = - mixxx::audio::FramePos::fromEngineSamplePos( - entry.virtualPlaypositionStart); - m_pRateControl->notifySeek(seekPosition); - } - } - // Advance our idea of the current virtual playposition to this // ReadLogEntry's start position. filePlayposition = entry.advancePlayposition(&numConsumedSamples); @@ -307,7 +293,6 @@ double ReadAheadManager::getFilePlaypositionFromLog( // This entry is empty now. m_readAheadLog.pop_front(); } - shouldNotifySeek = true; } return filePlayposition; From f81147c0242f61051c82b54e16fa96e2884479e1 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Sat, 1 Feb 2025 05:17:37 +0100 Subject: [PATCH 2/8] PositionScratchController: make callsToStop a member, adjust comments --- src/engine/positionscratchcontroller.cpp | 28 ++++++++++++------------ src/engine/positionscratchcontroller.h | 1 + 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index 077f788752d..eb365798653 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -69,6 +69,7 @@ namespace { constexpr double kDefaultSampleInterval = 0.016; // The max wait time when no new position has been set +// TODO Make threshold configurable for controller use? constexpr double kMoveDelayMax = 0.04; // The rate threshold above which disabling position scratching will enable // an 'inertia' mode. @@ -76,6 +77,7 @@ constexpr double kThrowThreshold = 2.5; // Max velocity we would like to stop in a given time period. constexpr double kMaxVelocity = 100; // Seconds to stop a throw at the max velocity. +// TODO make configurable, eg. to customize spinbacks with controllers constexpr double kTimeToStop = 1.0; } // anonymous namespace @@ -99,9 +101,10 @@ PositionScratchController::PositionScratchController(const QString& group) m_rate(0), m_moveDelay(0), m_mouseSampleTime(0), - m_bufferSize(-1), // ? + m_bufferSize(-1), m_dt(1), m_callsPerDt(1), + m_callsToStop(1), m_p(1), m_d(1), m_f(0.4) { @@ -123,6 +126,8 @@ void PositionScratchController::slotUpdateFilterParameters(double sampleRate) { // lowpass filter m_callsPerDt = static_cast(ceil(kDefaultSampleInterval / m_dt)); + m_callsToStop = m_dt / kTimeToStop; + // Tweak PD controller for different latencies m_p = 0.3; m_d = m_p / -2; @@ -179,10 +184,10 @@ void PositionScratchController::process(double currentSamplePos, // constants. Roughly we backsolve what the decay should be if we want to // stop a throw of max velocity kMaxVelocity in kTimeToStop seconds. Here is // the derivation: - // kMaxVelocity * alpha ^ (# callbacks to stop in) = decayThreshold + // decayThreshold = kMaxVelocity * alpha ^ (# callbacks to stop in) // # callbacks = kTimeToStop / m_dt // alpha = (decayThreshold / kMaxVelocity) ^ (m_dt / kTimeToStop) - const double kExponentialDecay = pow(decayThreshold / kMaxVelocity, m_dt / kTimeToStop); + const double kExponentialDecay = pow(decayThreshold / kMaxVelocity, m_callsToStop); m_rate *= kExponentialDecay; @@ -239,10 +244,9 @@ void PositionScratchController::process(double currentSamplePos, bool calcRate = true; if (m_scratchTargetDelta == scratchTargetDelta) { - // We get here if the next mouse position is delayed, the mouse - // is stopped or moves very slowly. Since we don't know the case - // we assume delayed mouse updates for 40 ms. - // TODO Make threshold configurable for controller use? + // We get here if the next mouse position is delayed, the + // mouse is stopped or moves very slowly. Since we don't + // know the case we assume delayed mouse updates for 40 ms. m_moveDelay += m_dt * m_callsPerDt; if (m_moveDelay < kMoveDelayMax) { // Assume a missing Mouse Update and continue with the @@ -269,11 +273,7 @@ void PositionScratchController::process(double currentSamplePos, double ctrlError = m_pRateIIFilter->filter( scratchTargetDelta - m_samplePosDeltaSum); m_rate = m_pVelocityController->observation(ctrlError); - m_rate /= ceil(kDefaultSampleInterval / m_dt); - // Note: The following SoundTouch changes the also rate by a ramp - // This looks like average of the new and the old rate independent - // from m_dt. Ramping is disabled when direction changes or rate = 0; - // (determined experimentally) + m_rate /= m_callsPerDt; if (fabs(m_rate) < MIN_SEEK_SPEED) { // we cannot get closer m_rate = 0; @@ -301,8 +301,8 @@ void PositionScratchController::process(double currentSamplePos, m_moveDelay = 0; // Set up initial values, in a way that the system is settled m_rate = releaseRate; - m_samplePosDeltaSum = -(releaseRate / m_p) * - m_callsPerDt; // Set to the remaining error of a p controller + // Set to the remaining error of a p controller + m_samplePosDeltaSum = -(releaseRate / m_p) * m_callsPerDt; m_pVelocityController->reset(-m_samplePosDeltaSum); m_pRateIIFilter->reset(-m_samplePosDeltaSum); m_scratchStartPos = scratchPosition; diff --git a/src/engine/positionscratchcontroller.h b/src/engine/positionscratchcontroller.h index 0f2acc5ac2a..942db8f8d53 100644 --- a/src/engine/positionscratchcontroller.h +++ b/src/engine/positionscratchcontroller.h @@ -58,6 +58,7 @@ class PositionScratchController : public QObject { double m_dt; double m_callsPerDt; + double m_callsToStop; double m_p; double m_d; From 06da7ea7e5cabcbdb4d2625f8c016a8255499b0d Mon Sep 17 00:00:00 2001 From: ronso0 Date: Thu, 20 Feb 2025 02:30:34 +0100 Subject: [PATCH 3/8] (fix) PositionScratchController: avoid false high rate when seeking to hotcue --- src/engine/positionscratchcontroller.cpp | 32 +++++++++++++++++++++--- src/engine/positionscratchcontroller.h | 1 + 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index eb365798653..c82cb974579 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -95,6 +95,9 @@ PositionScratchController::PositionScratchController(const QString& group) m_isScratching(false), m_inertiaEnabled(false), m_prevSamplePos(0), + // TODO we might as well use FramePos in order to use more convenient + // mixxx::audio::kInvalidFramePos, then convert to sample pos on the fly + m_seekSamplePos(std::numeric_limits::quiet_NaN()), m_samplePosDeltaSum(0), m_scratchTargetDelta(0), m_scratchStartPos(0), @@ -167,6 +170,15 @@ void PositionScratchController::process(double currentSamplePos, m_mouseSampleTime = 0; } + bool adoptSeekPos = false; + if (!util_isnan(m_seekSamplePos)) { + // If we were notified about a seek, adopt the new position immediately. + m_prevSamplePos = m_seekSamplePos; + m_seekSamplePos = std::numeric_limits::quiet_NaN(); + + adoptSeekPos = true; + } + if (m_isScratching) { if (m_inertiaEnabled) { // If we got here then we're not scratching and we're in inertia @@ -269,7 +281,11 @@ void PositionScratchController::process(double currentSamplePos, m_scratchTargetDelta = scratchTargetDelta; } - if (calcRate) { + // If we just adopted the seek position we need to avoid false + // high rate and simply report the previous rate. + // It'll adapt to the scratch speed in the next run. + // Setting rate to 0 has the same effect apparently. + if (calcRate && !adoptSeekPos) { double ctrlError = m_pRateIIFilter->filter( scratchTargetDelta - m_samplePosDeltaSum); m_rate = m_pVelocityController->observation(ctrlError); @@ -313,7 +329,15 @@ void PositionScratchController::process(double currentSamplePos, void PositionScratchController::notifySeek(mixxx::audio::FramePos position) { DEBUG_ASSERT(position.isValid()); - // scratching continues after seek due to calculating the relative distance traveled - // in m_samplePosDeltaSum - m_prevSamplePos = position.toEngineSamplePos(); + const double newPos = position.toEngineSamplePos(); + if (!isEnabled()) { + // not scratching, ignore"; + return; + } else if (m_prevSamplePos == newPos) { + // no-op + return; + } + // Scratching continues after seek due to calculating the relative + // distance traveled in m_samplePosDeltaSum + m_seekSamplePos = newPos; } diff --git a/src/engine/positionscratchcontroller.h b/src/engine/positionscratchcontroller.h index 942db8f8d53..ba66bf94907 100644 --- a/src/engine/positionscratchcontroller.h +++ b/src/engine/positionscratchcontroller.h @@ -47,6 +47,7 @@ class PositionScratchController : public QObject { bool m_isScratching; bool m_inertiaEnabled; double m_prevSamplePos; + double m_seekSamplePos; double m_samplePosDeltaSum; double m_scratchTargetDelta; double m_scratchStartPos; From 34fbd10aa3a6760f1566492b7528b43c692f5f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 23 Feb 2025 12:43:32 +0100 Subject: [PATCH 4/8] Rename m_scratchPosSampleTime, move closer to usage and improve comments --- src/engine/positionscratchcontroller.cpp | 24 ++++++++++++------------ src/engine/positionscratchcontroller.h | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index c82cb974579..67f0ac50374 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -103,7 +103,7 @@ PositionScratchController::PositionScratchController(const QString& group) m_scratchStartPos(0), m_rate(0), m_moveDelay(0), - m_mouseSampleTime(0), + m_scratchPosSampleTime(0), m_bufferSize(-1), m_dt(1), m_callsPerDt(1), @@ -163,13 +163,6 @@ void PositionScratchController::process(double currentSamplePos, slotUpdateFilterParameters(m_pMainSampleRate->get()); } - double scratchPosition = 0; - m_mouseSampleTime += m_dt; - if (m_mouseSampleTime >= kDefaultSampleInterval || !m_isScratching) { - scratchPosition = m_pScratchPos->get(); - m_mouseSampleTime = 0; - } - bool adoptSeekPos = false; if (!util_isnan(m_seekSamplePos)) { // If we were notified about a seek, adopt the new position immediately. @@ -244,13 +237,16 @@ void PositionScratchController::process(double currentSamplePos, // This is required to scratch within loop boundaries. m_samplePosDeltaSum += (sampleDelta) / (bufferSize * baseSampleRate); - // If we may have a new position, calculate scratch parameters and + m_scratchPosSampleTime += m_dt; + // If the kDefaultSampleInterval has expired, calculate scratch parameters and // eventually the new rate. // Else, continue with the last rate. - if (m_mouseSampleTime == 0) { + if (m_scratchPosSampleTime >= kDefaultSampleInterval) { + m_scratchPosSampleTime = 0; + // Set the scratch target to the current set position // and normalize to one buffer - double scratchTargetDelta = (scratchPosition - m_scratchStartPos) / + double scratchTargetDelta = (m_pScratchPos->get() - m_scratchStartPos) / (bufferSize * baseSampleRate); bool calcRate = true; @@ -321,7 +317,11 @@ void PositionScratchController::process(double currentSamplePos, m_samplePosDeltaSum = -(releaseRate / m_p) * m_callsPerDt; m_pVelocityController->reset(-m_samplePosDeltaSum); m_pRateIIFilter->reset(-m_samplePosDeltaSum); - m_scratchStartPos = scratchPosition; + // The "scratch_position" CO contains relative traveled audio frames * 2 + // Not necessarily the actual position in the track. We use this to calculate + // the traveled distance of the mouse compared to m_scratchStartPos. + m_scratchStartPos = m_pScratchPos->get(); + m_scratchPosSampleTime = 0; // qDebug() << "scratchEnable()" << currentSamplePos; } m_prevSamplePos = currentSamplePos; diff --git a/src/engine/positionscratchcontroller.h b/src/engine/positionscratchcontroller.h index ba66bf94907..21135d3f461 100644 --- a/src/engine/positionscratchcontroller.h +++ b/src/engine/positionscratchcontroller.h @@ -53,7 +53,7 @@ class PositionScratchController : public QObject { double m_scratchStartPos; double m_rate; double m_moveDelay; - double m_mouseSampleTime; + double m_scratchPosSampleTime; int m_bufferSize; From 943a3890f048af76116cfb329d434dda27d1ab0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 23 Feb 2025 12:43:32 +0100 Subject: [PATCH 5/8] Simplify adoption of the seeked value --- src/engine/positionscratchcontroller.cpp | 25 ++++++++++-------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index 67f0ac50374..3b6e5738798 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -163,15 +163,6 @@ void PositionScratchController::process(double currentSamplePos, slotUpdateFilterParameters(m_pMainSampleRate->get()); } - bool adoptSeekPos = false; - if (!util_isnan(m_seekSamplePos)) { - // If we were notified about a seek, adopt the new position immediately. - m_prevSamplePos = m_seekSamplePos; - m_seekSamplePos = std::numeric_limits::quiet_NaN(); - - adoptSeekPos = true; - } - if (m_isScratching) { if (m_inertiaEnabled) { // If we got here then we're not scratching and we're in inertia @@ -281,7 +272,7 @@ void PositionScratchController::process(double currentSamplePos, // high rate and simply report the previous rate. // It'll adapt to the scratch speed in the next run. // Setting rate to 0 has the same effect apparently. - if (calcRate && !adoptSeekPos) { + if (calcRate) { double ctrlError = m_pRateIIFilter->filter( scratchTargetDelta - m_samplePosDeltaSum); m_rate = m_pVelocityController->observation(ctrlError); @@ -324,17 +315,21 @@ void PositionScratchController::process(double currentSamplePos, m_scratchPosSampleTime = 0; // qDebug() << "scratchEnable()" << currentSamplePos; } + + if (!util_isnan(m_seekSamplePos)) { + // We need to transpose all buffers to compensate the seek + // in a way that the next process call does not even notice anything + currentSamplePos = m_seekSamplePos; + m_seekSamplePos = std::numeric_limits::quiet_NaN(); + } m_prevSamplePos = currentSamplePos; } void PositionScratchController::notifySeek(mixxx::audio::FramePos position) { - DEBUG_ASSERT(position.isValid()); const double newPos = position.toEngineSamplePos(); if (!isEnabled()) { - // not scratching, ignore"; - return; - } else if (m_prevSamplePos == newPos) { - // no-op + // Not scratching, ignore + // Such a queued seek would cause a position jump when touching the waveform. return; } // Scratching continues after seek due to calculating the relative From aca67c549e517c3dd7957db59ffae43714e03a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 24 Feb 2025 09:41:14 +0100 Subject: [PATCH 6/8] Fix a race codition that can happen when scratching is enabled and a hotcue is called --- src/engine/positionscratchcontroller.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index 3b6e5738798..3d9a7da317a 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -152,12 +152,6 @@ void PositionScratchController::process(double currentSamplePos, mixxx::audio::FramePos target) { bool scratchEnable = m_pScratchEnable->get() != 0; - if (!m_isScratching && !scratchEnable) { - // We were not previously in scratch mode are still not in scratch - // mode. Do nothing - return; - } - if (bufferSize != m_bufferSize) { m_bufferSize = bufferSize; slotUpdateFilterParameters(m_pMainSampleRate->get()); @@ -327,11 +321,6 @@ void PositionScratchController::process(double currentSamplePos, void PositionScratchController::notifySeek(mixxx::audio::FramePos position) { const double newPos = position.toEngineSamplePos(); - if (!isEnabled()) { - // Not scratching, ignore - // Such a queued seek would cause a position jump when touching the waveform. - return; - } // Scratching continues after seek due to calculating the relative // distance traveled in m_samplePosDeltaSum m_seekSamplePos = newPos; From 2c9edf40e01199b815425bc8baef978965095c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Mon, 24 Feb 2025 09:52:50 +0100 Subject: [PATCH 7/8] Use toBool() when checking CO "scratch_position_enable" --- src/engine/positionscratchcontroller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index 3d9a7da317a..d944a380693 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -150,7 +150,7 @@ void PositionScratchController::process(double currentSamplePos, int wrappedAround, mixxx::audio::FramePos trigger, mixxx::audio::FramePos target) { - bool scratchEnable = m_pScratchEnable->get() != 0; + bool scratchEnable = m_pScratchEnable->toBool(); if (bufferSize != m_bufferSize) { m_bufferSize = bufferSize; From 50ad53c41852ac5f17ea3d8e5c2ff3a09ac31901 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Mon, 24 Feb 2025 12:14:00 +0100 Subject: [PATCH 8/8] improve "scratch_position" comments --- src/engine/positionscratchcontroller.cpp | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/engine/positionscratchcontroller.cpp b/src/engine/positionscratchcontroller.cpp index d944a380693..46a0eb0918a 100644 --- a/src/engine/positionscratchcontroller.cpp +++ b/src/engine/positionscratchcontroller.cpp @@ -280,8 +280,8 @@ void PositionScratchController::process(double currentSamplePos, // qDebug() << m_rate << scratchTargetDelta << m_samplePosDeltaSum << m_dt; } } else { - // We were previously in scratch mode and are no longer in scratch - // mode. Disable everything, or optionally enable inertia mode if + // We quit scratch mode. + // Disable everything, or optionally enable inertia mode if // the previous rate was high enough to count as a 'throw' if (fabs(m_rate) > kThrowThreshold) { m_inertiaEnabled = true; @@ -291,8 +291,8 @@ void PositionScratchController::process(double currentSamplePos, //qDebug() << "disable"; } } else if (scratchEnable) { - // We were not previously in scratch mode but now are in scratch - // mode. Enable scratching. + // We were not previously in scratch mode but now we are. + // Enable scratching. m_isScratching = true; m_inertiaEnabled = false; m_moveDelay = 0; @@ -302,9 +302,15 @@ void PositionScratchController::process(double currentSamplePos, m_samplePosDeltaSum = -(releaseRate / m_p) * m_callsPerDt; m_pVelocityController->reset(-m_samplePosDeltaSum); m_pRateIIFilter->reset(-m_samplePosDeltaSum); - // The "scratch_position" CO contains relative traveled audio frames * 2 - // Not necessarily the actual position in the track. We use this to calculate - // the traveled distance of the mouse compared to m_scratchStartPos. + // The "scratch_position" CO contains the distance traveled. We use this + // to calculate the traveled distance of the mouse compared to m_scratchStartPos. + // When it's set by WWaveformViewer::mousePressEvent or mouseMoveEvent, + // this is equal to traveled frames * 2 but usually unrelated to the + // the actual position in the track. + // Note that "scratch_position" can also be set by anyone, eg. by wheel + // functions of controller mappings. In such cases its value depends on + // the scaling of the original wheel position / wheel tick values and + // may be entirely unrelated to audio frames. m_scratchStartPos = m_pScratchPos->get(); m_scratchPosSampleTime = 0; // qDebug() << "scratchEnable()" << currentSamplePos;