Enable beatloop activation directly after activating a hotcue by goddisignz · Pull Request #2190 · mixxxdj/mixxx

goddisignz · 2019-06-24T20:21:19Z

Fxes https://bugs.launchpad.net/mixxx/+bug/1464003 .
Basically the loopcontroller has to set the loop start position to the queued seek position before activating the loop.
Furthermore, when a hotcue that is at the end of an active loop is activated, also disable the loop instead of jumping back to the loop entry.

uklotzde · 2019-06-24T21:10:20Z

 void LoopingControl::slotBeatLoop(double beats, bool keepStartPoint, bool enable) {
+
+    // if a seek was queued in the engine buffer wait until it is executed
+    while(getEngineBuffer()->isSeekQueued()){}


This doesn't look sane to me. A busy wait on a condition that is controlled by another thread?

I expected something like this but was not sure which other mechanism to use. Just tell me what would me the right one and I will adapt my solution.

A straight forward solution is to predict the effect of the scheduled seek here.

I think you need to make the
LoopSamples loopSamples = m_loopSamples.getValue(); conditional.
If there is a cued seek, you need to use m_queuedSeekPosition.getValue().

By luck you can ignore the quantize flag. If not you need to adjust the loop along with the m_queuedSeekPosition.

* fixes https://bugs.launchpad.net/mixxx/+bug/1464003 * furthermore, when a hotcue that is at the end of an active loop is activated, also disable the loop instead of jumping back to the loop entry

daschuer

Thank you for your update.
I have added some comments.

Does it work as expected now?

It would be great to have a unit test for this case.
Can you had one? Do you know how?

daschuer · 2019-06-29T21:00:58Z

+
+    // if a seek was queued in the engine buffer move the current sample to its position
+    ControlValueAtomic<double> seekPosition;
+    if(getEngineBuffer()->isSeekQueued(seekPosition)){


Please use git-clang-format HEAD~1 to fix some code style issues.
https://www.mixxx.org/wiki/doku.php/coding_guidelines#command_line

daschuer · 2019-06-29T21:07:07Z

+            setNewPlaypos(position, false);
+            adjustingPhase = true;
+        }
+        setNewPlaypos(syncPosition, adjustingPhase);


The double setNewPlaypos() call reads a bit like a hack. Can we add for example a new parameter to identify the situation explicit?

Yep, this is more like a workaround but as I am still new to the project I didn't want to change parameters of already existing methods. We could have a seekedPosition and syncedPosition in setNewPlaypos and notifySeek and let the controller decide what to do with this information. How about that?

daschuer · 2019-06-29T21:09:16Z

    return false;
 }

+bool EngineBuffer::isSeekQueued(ControlValueAtomic<double> &seekPos) {


we use only const references in Mixxx. I think we can use here a "double* pSeekPosition" as second return value.

daschuer · 2019-06-29T21:13:29Z

 }

+bool EngineBuffer::isSeekQueued(ControlValueAtomic<double> &seekPos) {
+    if(!m_iSeekQueued){


Is this check redundant?

daschuer · 2019-06-29T21:15:10Z

 void LoopingControl::slotBeatLoop(double beats, bool keepStartPoint, bool enable) {
+
+    // if a seek was queued in the engine buffer move the current sample to its position
+    ControlValueAtomic<double> seekPosition;


we can use here a plain double, because it lives on the stack and is not used concurrently.

goddisignz · 2019-06-30T09:17:59Z

I will make changes and recommit.
It works as expected except in one case. If the synced position is in front of an active loop the playposition does not end up in front of the end of the loop. I would expect it to be like that.
Not sure if the loopcontroller can take care of that currently.

uklotzde · 2019-06-30T16:49:13Z

    }
+
+    // seek finished after setNewPlaypos
+    m_iSeekQueued.fetchAndStoreRelaxed(SEEK_NONE);


Are you able to justify that the reordering of atomic memory operations still works as expected?

Actually, I couldn't. But it this line won't be necessary anymore.

daschuer

Some more comments. We are getting close. Thank you.

daschuer · 2019-07-01T21:24:16Z


    SeekRequests seekType = static_cast<SeekRequest>(
-            m_iSeekQueued.fetchAndStoreRelease(SEEK_NONE));
+            m_iSeekQueued.fetchAndStoreRelaxed(SEEK_NONE));


here wee need to be sure that m_queuedSeekPosition.getValue() is not accessed before reading and writing m_iSeekQueued so I think fetchAndSubAcquire() would be the right call.

That must be an autocomplete-mistake. I wanted to change it back to the original version.

But it was originally wrong. We need the Aquire memory fence here, to stop the pipeline executing the reading of the seek position too early.

OK, I will change that. But which value should I subtract?
Are you sure fetchAndSubAcquire shouldn't be fetchAndStoreAcquire?

Ups ... of cause.

daschuer · 2019-07-02T06:16:12Z


 // This is called from the engine thread
-void LoopingControl::notifySeek(double dNewPlaypos, bool adjustingPhase) {
+void LoopingControl::notifySeek(double dNewPlaypos, double dSyncedNewPlayPos, bool adjustingPhase) {


Can you clarify and finally ad a code comment why, we need to pass two positions here?

I am unsure, if we still need the adjustingPhase value. Is it only read here? Original it was to prevent jump out of a loop when adjusting phase.
Now we have the original and the adjusted position to detect this.

What was the case, you need this code for?
What are the cases, we need to considder?

Can you rename dNewPlayposition to
something more descriptive?
Like dRequestedPlaypos or such?

Assume an active loop starting exactly at hotcue A. If I activate the hotcue the synced position is very likely in front of the loop. The adjustingPhase is still false, as it is was a SEEK_STANDARD_PHASE. If we set the adjustingPhase to true in that case, the loop would not be disabled for any other seek position outside the loop.
Thus, the loopcontroller should know about the position requested by the user but the remaining controllers should still be notified with the synced position.

I assume that the adjustPhase is still necessary. When there was no sync, both positions are equal. But (I guess) the same holds true if only a sync without any seeking is requested.

I just had a thought on solving this case differently in the processSeek method. Let the loopingcontroller calculate a position inside a loop (if enabled) based on the requested and the synced position. If the requested position is outside the loop return the synced position and behave as currently. If the requested position is inside but the synced position is outside the loop, correct the synced position into the loop.

double syncPosition = position; if (!paused && (seekType & SEEK_PHASE)) { syncPosition = m_pBpmControl->getNearestPositionInPhase(position, true, true); position = m_pLoopingControl->getSyncPositionInsideLoop(position, syncPosition); }

The setNewPos and notifySeek could be left unchanged.

Yes, even better readability. Thank you.

But why do we need syncPosition outside the if block?
I think it would be even more clean to have a requested position as well and not reuse position for this.

…eek"" This reverts commit c8f9a5b.

daschuer

some more comments.

daschuer · 2019-07-03T21:56:23Z

+    LoopSamples loopSamples = m_loopSamples.getValue();
+
+    // if the request itself is outside loop do nothing
+    // loop will be disabled later by notifySeek(...)


or does it make sense to have the loop disable logic here?

daschuer · 2019-07-03T21:58:09Z

+
+    // the synced position is in front of the loop
+    // adjust the synced position to same amount in front of the loop end
+    if (dSyncedPlayPos <= loopSamples.start) {


I think it should be just <

daschuer · 2019-07-03T21:59:29Z

+    // the synced position is in front of the loop
+    // adjust the synced position to same amount in front of the loop end
+    if (dSyncedPlayPos <= loopSamples.start) {
+        return loopSamples.end - (loopSamples.start - dSyncedPlayPos);


this can still be outside the loop in case the loop is only a fraction of a beat.

You're right, but easy to solve.
BTW: This whole method only works on beatloops (N-bar or fraction of N). Should we also care about loops which are non-beatloops or just ignore that case?

I think it is independent from where loop in and out are set.

We may have this condition:
B____I__O
B=beat
I=loop in
O= loop out

We need only make sure that the time between. B and I is rolled of in the loop in a way that the requested point in the loop is reached as if the seek had been done to B.

So yes, we can ignore the case. The user is responsible to use a beat loop if he wishes the loop is in beat.

daschuer · 2019-07-03T22:00:45Z

+    // the synced position is behind the loop
+    // adjust the synced position to same amount behind the loop start
+    if (dSyncedPlayPos >= loopSamples.end) {
+        return loopSamples.start + (dSyncedPlayPos - loopSamples.end);


the same here, this can still be behind the loop in case of small loops.

daschuer · 2019-07-03T22:03:16Z

    }
+
    if (position != m_filepos_play) {
        setNewPlaypos(position, adjustingPhase);


i think we can now get rid of the adjustingPhase flag, because the position is always inside the loop now.

goddisignz · 2019-07-18T19:55:00Z

I will use create a new PR to fix https://bugs.launchpad.net/mixxx/+bug/1778246 in the 2.1 release.
The remaining changes will be put to a new PR

uklotzde reviewed Jun 24, 2019

View reviewed changes

daschuer reviewed Jun 24, 2019

View reviewed changes

Comment thread src/engine/controls/loopingcontrol.cpp

set loop position to queued seek position before activating beatloop

7a418a7

* fixes https://bugs.launchpad.net/mixxx/+bug/1464003 * furthermore, when a hotcue that is at the end of an active loop is activated, also disable the loop instead of jumping back to the loop entry

daschuer requested changes Jun 29, 2019

View reviewed changes

uklotzde reviewed Jun 30, 2019

View reviewed changes

goddisignz added 2 commits June 30, 2019 19:49

implemented most requested changes

255fd69

added parameter for synchronized position in engines "notifySeek"

c8f9a5b

daschuer requested changes Jul 2, 2019

View reviewed changes

goddisignz added 2 commits July 2, 2019 22:02

Revert "added parameter for synchronized position in engines "notifyS…

cc375f6

…eek"" This reverts commit c8f9a5b.

let the loopingcontroller adjust synced seeks if necessary

d441ca8

daschuer requested changes Jul 3, 2019

View reviewed changes

prevent jumping out of loop if loop is smaller than one bar

2fdffa2

Holzhaus mentioned this pull request Jul 9, 2019

Add support for Saved loops #2194

Merged

5 tasks

goddisignz added 2 commits July 14, 2019 15:07

remove obsolete adjustingPhase parameter

7ea5f07

do not unset m_iSeekQueued before controls have been notified

0d1cb49

goddisignz closed this Jul 18, 2019

goddisignz mentioned this pull request Jul 21, 2019

enable beatloop activation directly after activating a hotcue #2213

Merged

This was referenced Aug 22, 2022

cannot jump to hotcue and immediately start a loop #8093

Closed

Pressing Hotcues buttons randomly deactivate loop #9350

Closed

Uh oh!

Conversation

goddisignz commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goddisignz commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goddisignz Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goddisignz Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goddisignz commented Jun 24, 2019 •

edited

Loading

goddisignz commented Jun 30, 2019 •

edited

Loading

goddisignz Jul 2, 2019 •

edited

Loading

goddisignz Jul 5, 2019 •

edited

Loading