Slip mode: consider active (regular) loop #11435

ronso0 · 2023-04-03T09:58:42Z

I was curious why this hasn't been implemented earlier, and couldn't really believe "this is super complicated though so it will be a lot of effort to fix" (2013, #6993).

So, this is POC and it performs nicely with active loops (no change for rolling loops).
I didn't run the tests, yet, only tested manually: play a track, set up a loop, clone that deck, then engage slip mode, mess around a bit with the waveform. Disabled slip mode and both decks were in sync.

I don't intend to finish this (unless just polishing is required for merging), I just want to share my experiment and give a starting point for others, so feel free to pick this up.
I guess the code can be improved and the tests should probably to be extended.

daschuer · 2023-05-06T07:38:24Z

This is a draft PR, right. Please correct me if I am wrong.

ronso0 · 2023-05-06T14:08:01Z

Well, the tests passed, both manually and CI, so as long as no better solution is in sight we might as well merge it.
If so, I'd take another look if it can be polished or some comments be added.

ronso0 · 2023-05-07T23:19:55Z

I'm pretty confident this works as expected and doesn't break anything.
I tested enabling slip mode while inside an active loop and with a catching loop ahead, and did scratch for a while. In both cases, quitting slip mode would restore the play position exactly where it's in the the previously cloned deck playing in parallel.

I thought about moving it to a new function in LoopingControl, but that'd be just for the sake of doing the virtual looping in a looping class while having to pass a few paramters around. No win IMO.

daschuer

Thank you. I have left some comments.

src/engine/enginebuffer.cpp

daschuer · 2023-05-09T07:18:32Z

src/engine/enginebuffer.cpp

+            // simulate looping
+            const double loopStart = m_pLoopingControl->getLoopSamples().start;
+            const double loopEnd = m_pLoopingControl->getLoopSamples().end;
+            while (newPos > loopEnd) {


Suggested change

while (newPos > loopEnd) {

while (newPos >= loopEnd) {

LoopEnd is the trigger point, the first point after the loop.
Not sure if we need also consider the playback direction here, probably not because the while loop works in both directions.

This would move us to loop_in.
newPos <= loopStart below would move us back to loop_end

src/engine/enginebuffer.cpp

ronso0 · 2023-05-09T13:39:16Z

Thanks for your review!

I'll ~~rebase~~ rework on top of 2.4.
Since I did some digging while working on #11532 I think we can simply use LoopingControl::seekInsideAdjustedLoop here, exactly like LoopingControl::process does, and just create a similiar, simplified wrapper to be called from EngineBuffer.

ronso0 · 2023-05-09T20:33:08Z

Seems to work.
Though to fulfil slip mode expectations related to this fix we should NOT deactivate a loop when seeking outside, so the playback can continue inside the loop when quitting slip mode. Fixed by the second commit.

ronso0 · 2023-05-09T20:50:13Z

Oh, I misread this

@daschuer modified milestones 2.3.6, 2.4 ...

If there really will be a 2.3.6 release I'll backport this, just the position types need to be changed back to double.

daschuer · 2023-05-09T21:16:17Z

I have no particular interest in a 2.3.6, but we will have a HEAD of the 2.3 branch just before releasing 2.4.0. with a separate Changelog for a future 2.3.6. It will not be available on our download page, because it is immediately replaces with 2.4.0. The only way around it is to freeze the 2.3 branch now, but is hard to predict if we need a 2.3.6 anyway.

I have picked here the 2.3.6 label, because it was original targeted to 2.3. Since this is not a critical bug I don't mind where it should go.

What is you idea?

ronso0 · 2023-05-09T21:55:17Z

Well, #6993 is from 2013, with very few comments, IMO it's just a incidental bug that affects a handful of people (who might think it's intended behaviour) and (considering the little feedback we got when asking users to test bugfix releases on Zulip) I think it's actually fine to target 2.4
And maybe add a test for it.

daschuer · 2023-05-10T05:58:00Z

src/engine/controls/loopingcontrol.cpp

@@ -1587,6 +1597,18 @@ void LoopingControl::slotLoopMove(double beats) {
    }
 }

+// Must be called from the engine thread only


This comment is no longer true, because m_loopInfo can read at any time and the function has no immediate effect to the loop.

Both functions can make use of a better name because they don't actually seek.
They adjust a given "currentPosition" which is also badly named, because it is not yet current.

Maybe adjustPositionForCurrentLoop(mixxx::audio::FramePos position)

If you want do a bit more, you may move LoopingControl::seekInsideAdjustedLoop() into an anonymous namespace. It is a function without "this" access.

daschuer · 2023-05-10T06:06:30Z

src/engine/enginebuffer.cpp

+            if (adjustedPos == mixxx::audio::kInvalidFramePos) {
+                m_slipPosition = newPos;
+            } else {
+                m_slipPosition = adjustedPos;
+            }


This conditional branch is a workaround for an implementation detail in seekInsideAdjustedLoop()
Please move it into seekInsideCurrentLoop() to make this code here more easy to read.

daschuer · 2023-05-10T06:34:19Z

src/engine/controls/loopingcontrol.cpp

+        return currentPosition;
+    }
+    LoopInfo loopInfo = m_loopInfo.getValue();
+    return seekInsideAdjustedLoop(currentPosition,


If I return here loopInfo.endPosition, the loop is left after disable slip.
This is because the end position is the first position after the loop.
A bug in the first conditions in seekInsideAdjustedLoop.

I am afraid we need also consider the direction, but I have not tested yet reverse playback yet. An open TODO.

If I return here loopInfo.endPosition, the loop is left after disable slip.
...
A bug in the first conditions in seekInsideAdjustedLoop.

True. I fixed that. Though, that only affected the new slip looping. All existing function (EngineBuffer, ReadAheadManager) already handle that appropriately.

This is because the end position is the first position after the loop.

I don't understand. If ReadAheadManager::getNextSamples is exactly at the trigger point (loop_out, or _in if reverse) it'll consider that as "inside the loop, trigger reached" and wrap around.

Yes, this means the play position is never at the loop end after the engine call is done.

If you explicitly seek to this position, the code treats it as being after the loop and continues without looping.

ronso0 · 2023-05-14T22:34:39Z

Oh man, trying to figure a (simple) solution was really a brainfuck, until I noticed that both notifySeek and seekInsideAdjustedLoop must consider the play direction.

Now it's workig as desired. I did some tests as mentioned above, incl. reverse

play a track, set up a loop, clone that deck, then engage slip mode, mess around a bit with the waveform. Disabled slip mode and both decks were in sync.

…tedLoop

ronso0 · 2023-05-14T22:39:47Z

TODO: get rid of RateControl assertions, try EngineBuffer::getRateControl etc.

daschuer · 2023-05-15T15:09:17Z

Thank you. Works good now and is IMHO also a consistent solution.

daschuer marked this pull request as draft May 6, 2023 07:38

ronso0 marked this pull request as ready for review May 6, 2023 09:22

ronso0 marked this pull request as draft May 6, 2023 09:23

ronso0 force-pushed the slip-loop branch 2 times, most recently from 34d2f65 to b40c5d1 Compare May 7, 2023 23:04

ronso0 marked this pull request as ready for review May 7, 2023 23:05

daschuer requested changes May 9, 2023

View reviewed changes

daschuer modified the milestones: 2.3.6, 2.4.0 May 9, 2023

mixxxdj deleted a comment from daschuer May 9, 2023

ronso0 changed the base branch from 2.3 to 2.4 May 9, 2023 20:34

ronso0 force-pushed the slip-loop branch from b40c5d1 to 31e4bba Compare May 9, 2023 20:35

ronso0 requested a review from daschuer May 9, 2023 20:43

ronso0 modified the milestones: 2.3.6, 2.4.0 May 9, 2023

daschuer requested changes May 10, 2023

View reviewed changes

ronso0 mentioned this pull request May 14, 2023

Reloop button remains lit #11553

Open

ronso0 force-pushed the slip-loop branch from 31e4bba to 0b93401 Compare May 14, 2023 22:34

ronso0 added 5 commits May 15, 2023 00:35

LoopingControl: assert RateControl pointer

9eb2773

LoopingControl: make more functions consider reverse (playing backwards)

0c035cf

LoopingControl: seekInsideAdjustedLoop >> adjustedPositionInsideAdjus…

b5f7e89

…tedLoop

Slip mode: simulate looping if there is an active regular loop

c5127eb

Slip mode: keep current loop enabled when seeking outside of it

0ea8a93

ronso0 force-pushed the slip-loop branch from 0b93401 to 0ea8a93 Compare May 14, 2023 22:35

daschuer merged commit c4090e4 into mixxxdj:2.4 May 15, 2023

ronso0 deleted the slip-loop branch May 15, 2023 15:39

This was referenced Aug 19, 2023

Fix wrong visual play position when inside loop #11840

Merged

Slip mode handles loops not correct #11844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slip mode: consider active (regular) loop #11435

Slip mode: consider active (regular) loop #11435

ronso0 commented Apr 3, 2023 •

edited

Loading

daschuer commented May 6, 2023

ronso0 commented May 6, 2023

ronso0 commented May 7, 2023

daschuer left a comment

daschuer May 9, 2023

ronso0 May 9, 2023

ronso0 commented May 9, 2023 •

edited

Loading

ronso0 commented May 9, 2023

ronso0 commented May 9, 2023

daschuer commented May 9, 2023

ronso0 commented May 9, 2023

daschuer May 10, 2023

daschuer May 10, 2023

daschuer May 10, 2023

ronso0 May 10, 2023

daschuer May 10, 2023

ronso0 commented May 14, 2023

ronso0 commented May 14, 2023

daschuer commented May 15, 2023

Slip mode: consider active (regular) loop #11435

Slip mode: consider active (regular) loop #11435

Conversation

ronso0 commented Apr 3, 2023 • edited Loading

daschuer commented May 6, 2023

ronso0 commented May 6, 2023

ronso0 commented May 7, 2023

daschuer left a comment

Choose a reason for hiding this comment

daschuer May 9, 2023

Choose a reason for hiding this comment

ronso0 May 9, 2023

Choose a reason for hiding this comment

ronso0 commented May 9, 2023 • edited Loading

ronso0 commented May 9, 2023

ronso0 commented May 9, 2023

daschuer commented May 9, 2023

ronso0 commented May 9, 2023

daschuer May 10, 2023

Choose a reason for hiding this comment

daschuer May 10, 2023

Choose a reason for hiding this comment

daschuer May 10, 2023

Choose a reason for hiding this comment

ronso0 May 10, 2023

Choose a reason for hiding this comment

daschuer May 10, 2023

Choose a reason for hiding this comment

ronso0 commented May 14, 2023

ronso0 commented May 14, 2023

daschuer commented May 15, 2023

ronso0 commented Apr 3, 2023 •

edited

Loading

ronso0 commented May 9, 2023 •

edited

Loading