Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix) Waveform scratching: fix seeking to hotcue (reduced, alt. for #14059) #14357

Merged
merged 8 commits into from
Feb 24, 2025

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 17, 2025

This is #14059 without the member/slot refactoring, i.e. just the fixes + switch to parented_ptr etc.
I hope this lowers the barrier to get the fix into 2.5.1

Original description:


This is a first (still dirty) fix for #13981

TL;DR
it's now possible to seek to hotcues while doing wave scratches.
Scratching at/across loop boundaries (active) still works.

Recently I compared the available methods for scratching with controllers, and while PositionScratchController (developed for waveform scratching) turned out to be the best (for me, no drift, smooth at low rates, nice spinback) I couldn't really use it as I want to because seeking to hotcues while scratching (holding the wheel, or turning very slowly) produced insanely high rates, and didn't stop at the cue pos.

Even though the bug is fixed I still consider this 'dirty' because I don't fully understand what was going wrong with the filters after seeking.

@ronso0 ronso0 force-pushed the waveform-scratching-seek-fix-simple branch from 7fe0fdb to 7d93f59 Compare February 17, 2025 08:46
bool adoptSeekPos = false;
if (!util_isnan(m_seekSamplePos)) {
// If we were notified about a seek, adopt the new position immediately.
m_prevSamplePos = m_seekSamplePos;
Copy link
Member

@daschuer daschuer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the m_seekSamplePos relates to currentSamplePos?
Can you add a source code comment? Are they equal for one call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After seeking, the engine is still using the 'old' playpos, so m_seekSamplePos is different from currentSamplePos, unless rate is 0, of course. (0 or close to 0 is my use case with the controller btw)

Here's the call hierarchy:

EngineBuffer::
processTrackLocked
+ processSeek()
| + setNewPlaypos( seekPosition )
|   + RateControl::notifySeek( seekPosition )
|     + PositionScratchController::notifySeek( seekPosition )
+ RateControl::calculateSpeed
|   + currPos = EngineControl::getFrameInfo().currPos
|   + PositionScratchController::process( currPos )
|   + PositionScratchController::getRate( currPos )
get framesRead with RateControl speed
+ m_playPos = ReadAheadManager::getFilePlaypositionFromLog( m_playPos, framesRead )
+ setFrameInfo()
+ EngineControl::setFrameInfo()
+ RateControl::process

This is obviously not a complete fix (and I'm not sure that can be done in PosScratchController alone) because no matter if we adopt the seek immediatly or delayed in the 2nd call after notifySeek, there's always a visual stop and a dip in rate (though not 0) when moving the mouse with constant speed (as far as that is possible with a track pad with acceleration).

Regardless, I'm in favor of merging this simple fix while we keep debugging.
Given the fact that this bug has been around for very long and we got not reports, I assume not many user seek while wave scratching (and even less use the scratch_position for controllers) and it'd be good to fulfill at least the basic expectation that seeking doesn't cause major glitches.

The perfect implementation would IMO be that the mouse/controller move is computed as if there was no seek, ie. get rid of the rate dip. (btw I'm not entirely sure if that's currently the case with controller scratch methods)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested a bit an it turns out that the new position is applied second process() call so you need to skip one or put the adoption of the seek position at the bottom of the process() call. The adoptSeekPos does also not work right. I think we need to reset the m_samplePosDeltaSum instead. I have tried this but it fails as well. At one version it seeks to the cue point and the immediately back to the original position. So we probably need to tweak the value and just subs tact the seek distance or such. (not tested).

Testing with the maximum audio buffer size was by the way the best to trace what actually happens.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested a bit an it turns out that the new position is applied second process()

FWIW I could never observe that exact the seek position is used for PositionScratchController::process(), there was always a diff (usable minimum was ~100 samples). I figured that with a version where I waited until the currPos and stored seekPos were close and then adopted the seekPos as prevPos 6927fdc

I think we need to reset the m_samplePosDeltaSum instead. I have tried this but it fails as well.

Thought so, too, and tried it as well as some other combinations, but none worked.
Except this one.

At one version it seeks to the cue point and the immediately back to the original position.

Yes, IIUC that is the original issue that causes the high rate.

So we probably need to tweak the value and just subs tact the seek distance or such. (not tested).

If you like to debug further (I don't) and you figure something I'm happy to test it!

@ronso0 ronso0 force-pushed the waveform-scratching-seek-fix-simple branch 2 times, most recently from 9595555 to 2f9d564 Compare February 20, 2025 02:02
@ronso0
Copy link
Member Author

ronso0 commented Feb 20, 2025

In order to keep you review I have decided to close #14059, rebased this one (fix conflicts) and adopt the only different commit from #14059.
Also reworded a commit because the already merged parented_ptr commit already contains the PositionScratchController refactoring.

Will address your review soon.

@ronso0 ronso0 force-pushed the waveform-scratching-seek-fix-simple branch from 2f9d564 to 06da7ea Compare February 22, 2025 13:44
@daschuer
Copy link
Member

Here it is: ronso0#90

@ronso0
Copy link
Member Author

ronso0 commented Feb 23, 2025

there's always a visual stop and a dip in rate

I think that occured simply because I used hotkeys for seeking.
If I use a virt. MIDI script and let that do the timed seeking all is good, the waveform sticks to the pointer even while seeking 👍

@daschuer
Copy link
Member

Here is a final bit: ronso0#91

@ronso0 ronso0 force-pushed the waveform-scratching-seek-fix-simple branch from cd0c66e to 50ad53c Compare February 24, 2025 11:29
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

@daschuer daschuer merged commit 6a9dd27 into mixxxdj:2.5 Feb 24, 2025
13 checks passed
@ronso0 ronso0 deleted the waveform-scratching-seek-fix-simple branch February 24, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pressing hotcue while holding waveform or spinny should seek to hotcue (and stay there)
2 participants