add `hotcue_X_indicator` control, is 1.0 if playposition is at hotcue #12951

ronso0 · 2024-03-11T14:26:17Z

Just a little helper that allows having callbacks for when the playposition crosses a hotcue / loopcue.
For example:

always trigger the fog machine on hotcue5 via MIDI_4_Lights
auto-censor certain spots in all tracks by making hotcue17 trigger a certain sample / engage revere-roll for 1 sec
flash controller hotcue buttons if deck is stopped at a hotcue
...

Description

Basically, it's something like beat_active and cue_indicator: it is activated (1.0) if

the play position is on the hotcue
(when stopped or when the buffer's new position is coincidentally at the hotcue position)
the play position crossed the hotcue since the last update
(while playing forward or backward and while scratching, not when seeking over a hotcue)

beat_active is 1.0 if the playposition is in a certain range around a beat marker
cue_indicator is 1.0 (maybe flashing) if the play position is exactly at the Cue (unlikely when playing)_

Improvement?

Integrating it into hotcue_X_status would allow GUI feedback via WHotcueButton, it'd be just two more states that could be styled rather easily. #9760
For now this is separate because integration/extension seems a bit cumbersome.

Nice to have

tests that covers looping (hotcue slight before and at loop_out, slightly after loop_in)

Proposal for testing

load this mapping to the MIDI Through controller https://gist.github.com/ronso0/2dd322622ddac2590020ce9b2af5d695
load a track to deck1, set hotcues 1-2-3 with a distance of ~10s
load a track to deck2
both decks stoped
Test1:
- play from start
- jump to hotcue 1
- jump to hotcue 3
- == deck2 should not start
Test2:
- play from start
- jump to hotcue 1
- jump to hotcue 2
- == deck2 should start
Test1:
- stopped
- jump to hotcue 3
- scratch backwards over hotcue 2
- == deck2 should start

daschuer

Good idea, the only issue we have that the code does not work in reverse. I think we have similar code for the beat indicator, you may re-use

ronso0 · 2024-03-11T18:49:52Z

Yes, for reverse I'll add a else branch.

beat_active is different though and the code is more complex because it is supposed to light up LEDs or widgets for X milliseconds while the track is playing, i.e. be ON when the playhead is in a certain +- range at/around the actual beat.

This is not required for hotcue_X_indicator which is only supposed to trigger while playing (don't miss it, duration doesn't matter) and when being stopped.

ronso0 · 2024-03-11T23:12:12Z

I adjusted it to

consider both forward and reverse play
ignore seeks

Scratching over a hotcue also activates the indicator.

ronso0 · 2024-03-11T23:24:09Z

For testing I created a controller mapping with a callback connection and added a testing proposal to the PR description.

ronso0 · 2024-03-11T23:30:14Z

🤷‍♂️

The following tests FAILED:
287 - EngineSyncTest.LoadTrackInitializesLeader (Failed)
292 - EngineSyncTest.EjectTrackSyncRemains (Failed)
368 - HotcueControlTest.SavedLoopUnloadTrackWhileActive (Failed)
546 - AdjustReplayGainTest.AdjustReplayGainUpdatesPregain (Failed)

Set 1.0 if playposition is at hotcue or if we passed it since the last update. Ignore seeks over hotcues.

ronso0 · 2024-03-12T03:54:04Z

I simplified it and the tests all pass. Dunno what went wrong and why only these tests failed, couldn't spot a common call that could cause the failure.

JoergAtGithub · 2024-03-16T11:24:07Z

How should this CO behave in case of a saved loop?

JoergAtGithub · 2024-03-16T11:26:24Z

You describe the 3 tests in the description. Why not implementing them as test case?

ronso0 · 2024-03-16T16:17:14Z

How should this CO behave in case of a saved loop?

Exactly like with hotcues (saved loops are just hotcues with an end point).

You describe the 3 tests in the description. Why not implementing them as test case?

Because I was lazy and noticed there are no tests for the cue indicator either ; p

I don't think tests are important as these controls don't affect performance, but of course I can write some.

JoergAtGithub · 2024-03-17T14:47:30Z

How should this CO behave in case of a saved loop?

Exactly like with hotcues (saved loops are just hotcues with an end point).

I wonder if this is what the user expects? I could imagine that he would expect the indicator active, as long as the saved loop is running. Not sure about the use case here.

ronso0 · 2024-03-17T14:59:06Z

What you describe is hotcue_X_status. The indicator added here is just an addition, as equivalent to cue_indicator which is also a point control.

JoergAtGithub · 2024-03-17T15:06:05Z

What you describe is hotcue_X_status. The indicator added here is just an addition, as equivalent to cue_indicator which is also a point control.

Doesn't hotcue_X_status just reports if the saved loop is enabled? I think it doesn't take into account, if and when the playposition reachs the loop.

ronso0 · 2024-03-17T15:13:54Z

What users may expect depends on the documentation.

hotcue_X_status
0 not set
1 set
2 loop active or previewing hotcue

If users want a loop indicator the can combine hotcue_X_status and hotcue_X_indicator.
Or we may extend hotcue_X_status with
3 inside active loop

Let's merge this first step, then extend it if requested.

JoergAtGithub · 2024-03-17T15:19:05Z

src/engine/controls/cuecontrol.cpp

+        if ((cuePos >= prevPos && cuePos <= currPos) ||     // forward
+                (cuePos <= prevPos && cuePos >= currPos)) { // reverse
+            pControl->setIndicator(1.0);
+        } else {
+            pControl->setIndicator(0.0);
+        }


In case of a saved loop and playing reverse, the duration of the loop needs to be added.

Good catch, I did not test looping.
And admittedly that is a tricky one. There is a seek from [loop end - x] to [loop in + y] where x + y = buffer length, so we'd need to get/store loop start/end and check loop_enabled as well as whether the hotcue is inside the loop, and that for both play directions. Uff...

Well - I've made some experience with this topic, in #11840 and #12281

I think I managed to catch all hotcues correctly.

Manual tests passed (with the trace output and the test mapping).
I'll update the Mixxx tests accordingly.

ronso0 · 2024-03-19T02:56:33Z

src/engine/controls/loopingcontrol.h

@@ -98,8 +86,7 @@ class LoopingControl : public EngineControl {

  signals:
    void loopReset();
-    void loopEnabledChanged(bool enabled);
-    void loopUpdated(mixxx::audio::FramePos startPosition, mixxx::audio::FramePos endPosition);
+    void loopUpdated(LoopInfo loopInfo);


Why does clazy insist this should be fully qualified when used in engine controls?
It has no issue with FrameInfo.

Also, the reason clazy-fully-qualified-moc-types warns is

not using fully-qualified type names [..] will break old-style connects and interaction with QML.

No old style here anymore, and is QML supposed to interact with engine controls??

No idea. Does the proposed change work?

Well, it builds (didn't run clazy locally), but I wonder if the reason is relevant here.
I'm not aware of any QML <--> EngineControl interaction, QML uses proxies for the library, players etc.
Control interaction is all done through EngineBuffer and the individual controls interacting with each other.

So I'd rather ignore it and go with a clazy exclude for now
// clazy:exclude=clazy-fully-qualified-moc-types

Though it's annoying that it has to be added everywhere it is used.

…'s signal

daschuer · 2024-03-20T08:33:30Z

src/engine/controls/cuecontrol.cpp

+                cuePos >= loopInfo.startPosition &&
+                cuePos < loopInfo.endPosition &&


Can we put this into a helper isInsideLoop() ? to deduplicate this a bit?

daschuer · 2024-03-20T08:38:05Z

src/engine/controls/cuecontrol.cpp

+                // Hotcue inside loop?
+                cuePos >= loopInfo.startPosition &&
+                cuePos < loopInfo.endPosition &&
+                (cuePos > prevPos || cuePos <= currPos)) {


cuePos > prevPos is clear, because the loop has wrapped around, we passed the cue.
cuePos <= currPos This one is not clear for me, doesn't it apply to all cues is is the loop?

since we already detected that we wrapped around, cuePos <= currPos covers all cues in between loop_in and currPos

daschuer · 2024-03-20T08:39:35Z

src/engine/controls/cuecontrol.cpp

+                // Hotcue inside loop?
+                cuePos >= loopInfo.startPosition &&
+                cuePos < loopInfo.endPosition &&
+                (cuePos < prevPos || cuePos >= currPos)) {


The same here:
cuePos >= currPos What is this condition for?

same as above: when playing reverse, this covers all cues that are after/at currPos and before loop_out

daschuer · 2024-03-20T08:50:51Z

src/engine/controls/cuecontrol.cpp

+            prevPos < loopInfo.endPosition &&
+            currPos < loopInfo.endPosition) {
+        // Check if we wrapped around
+        if (reverse && prevPos < currPos) {


Doesn't the condition apply to any seek, independent of a loop?

I am just considering if it will work more universal we just check if cue position is inside the range of prevPos + the buffer length?

This will not work if the cue is just after the current loop though.
In case of seeks the buffer before the seek is cossfaded with the buffer after the seek. Is it inside a loop?
What is the use case for such a corner case?

Doesn't the condition apply to any seek, independent of a loop?

Seeks are registered in notifySeek() which sets prevPos(), i.e. the loop detection doesn't kick in.

check if cue position is inside the range of prevPos + the buffer length?

Since the buffer is split into two parts if we wrap around, that's exactly what we do here. (besides, I also considered using LoopingControl's nextTrigger() like ReadAheadManager does, or get the trigger already in EngineBuffer so it can pass it to CueControl and RAManager, but that is overkill IMO)

Actually, the original implementation worked just fine (incl. scratching and reverse, NOT for loops),
the current implementation works also for loops, but fails for scratching backwards inside a loop.

Back to square one...

okay, "speed" from EngineBuffer works: it is "effective" reverse, i.e. reverse or scratching backwards.
now everything works as expected.

ronso0 · 2024-03-20T12:39:48Z

I tested

playing forwards & backwards, both with/without looping
scratching forth and back (stopped and playing)

and everything works as expected:

indicator is activated every time the playpos crosses a hotcue
seeks are ignored
hotcues at loop_out are correctly ignored (loop_out is considered outside the loop, i.e. jumping there would disable the loop)

Now it's just the question if there need to be tests for every case.
Or how much trace output we want/need to troubleshoot issues.

daschuer · 2024-03-20T14:13:35Z

Now it's just the question if there need to be tests for every case.

There is no pressing need for testing every single case. It depends more on you interest to contribute them.

Or how much trace output we want/need to troubleshoot issues.

There must be no debug output in the audio thread because it is locking. CueControl::updateIndicators() is inside the audio thread.

ronso0 · 2024-03-20T14:43:01Z

Or how much trace output we want/need to troubleshoot issues.

There must be no debug output in the audio thread because it is locking. CueControl::updateIndicators() is inside the audio thread.

I know, I was more referring to output that can be enabled on demand for troubleshooting.
If this is considered ready for merge I'll remove the TRACE commit.

Re tests, I think the essence of the looping scenario can be covered rather easily.

ronso0 · 2024-03-22T08:49:55Z

For now I have removed looping test commit c73a8f1
if someone can shed a light on the timing in tests, or has a fix handy for the failing (automatic) test we may re-add it.

ronso0 · 2024-04-09T12:15:31Z

I've redone this based on the wrapAround notification from ReadAheadManager I used for #13007.

The relevant commit is 0256bd4, test PR is #13076

daschuer · 2024-04-09T20:43:40Z

Yes, the new implementation looks more robust.

ronso0 · 2024-04-09T21:03:19Z

So I push to this branch?

daschuer · 2024-04-10T05:42:05Z

Which do you like more? I prefer #13076.

ronso0 · 2024-04-10T06:03:51Z

yes, me too, the question was just if you agree to overwrite this one.

daschuer · 2024-04-10T15:51:56Z

Or just close this, I don't mind.

ronso0 · 2024-05-26T13:48:05Z

I'll close this one and refine #13076 sometime

github-actions bot added the engine label Mar 11, 2024

daschuer reviewed Mar 11, 2024

View reviewed changes

ronso0 force-pushed the hotcue-indicator branch from 7310d52 to 319f1cf Compare March 11, 2024 23:10

ronso0 marked this pull request as ready for review March 11, 2024 23:12

ronso0 marked this pull request as draft March 12, 2024 03:03

add hotcue_X_indicator control

bb014d3

Set 1.0 if playposition is at hotcue or if we passed it since the last update. Ignore seeks over hotcues.

ronso0 force-pushed the hotcue-indicator branch from 319f1cf to bb014d3 Compare March 12, 2024 03:15

ronso0 marked this pull request as ready for review March 12, 2024 11:25

ronso0 mentioned this pull request Mar 12, 2024

Beats: allow undoing the last BPM/beats change #12954

Merged

3 tasks

github-actions bot added the code quality label Mar 17, 2024

JoergAtGithub reviewed Mar 17, 2024

View reviewed changes

ronso0 marked this pull request as draft March 18, 2024 01:34

Tests: add basic test for hotcue_X_indicator

b7cffdb

ronso0 force-pushed the hotcue-indicator branch 2 times, most recently from 4423e0f to 5efd118 Compare March 19, 2024 02:19

ronso0 commented Mar 19, 2024

View reviewed changes

ronso0 force-pushed the hotcue-indicator branch 2 times, most recently from a4c6a77 to 978c867 Compare March 19, 2024 15:16

ronso0 added 3 commits March 19, 2024 16:46

make LoopInfo available for CueControl

077d4b1

CueControl: listen to loop_enabled change instead of LoopingControl…

4249924

…'s signal

CueControl: store updated LoopInfo from LoopingControl

780316a

ronso0 force-pushed the hotcue-indicator branch from 978c867 to b3ffc4e Compare March 19, 2024 15:46

daschuer requested changes Mar 20, 2024

View reviewed changes

CueControl: consider looping when updating hotcue_X_indicator

586b1d9

ronso0 force-pushed the hotcue-indicator branch from b3ffc4e to 313609c Compare March 20, 2024 11:26

ronso0 force-pushed the hotcue-indicator branch 4 times, most recently from f9b888a to 2989e99 Compare March 21, 2024 00:16

fixup! TRACE/DEBUG hotcue_X_indicator

76ef49c

ronso0 force-pushed the hotcue-indicator branch from 2989e99 to 76ef49c Compare March 22, 2024 08:48

ronso0 marked this pull request as ready for review March 22, 2024 08:50

ronso0 mentioned this pull request Mar 27, 2024

(fix) Waveform scratch crossing loop boundaries #13007

Merged

ronso0 marked this pull request as draft March 28, 2024 18:47

ronso0 mentioned this pull request Apr 9, 2024

Hotcue indicator alternative #13076

Draft

ronso0 closed this May 26, 2024

ronso0 deleted the hotcue-indicator branch May 26, 2024 13:48

		cuePos >= loopInfo.startPosition &&
		cuePos < loopInfo.endPosition &&

add hotcue_X_indicator control, is 1.0 if playposition is at hotcue #12951

add hotcue_X_indicator control, is 1.0 if playposition is at hotcue #12951

Conversation

ronso0 commented Mar 11, 2024 • edited Loading

Description

Improvement?

Nice to have

Proposal for testing

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Mar 11, 2024

ronso0 commented Mar 11, 2024

ronso0 commented Mar 11, 2024

ronso0 commented Mar 11, 2024

ronso0 commented Mar 12, 2024

JoergAtGithub commented Mar 16, 2024

JoergAtGithub commented Mar 16, 2024

ronso0 commented Mar 16, 2024 • edited Loading

JoergAtGithub commented Mar 17, 2024

ronso0 commented Mar 17, 2024

JoergAtGithub commented Mar 17, 2024

ronso0 commented Mar 17, 2024

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

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 Mar 20, 2024 • edited Loading

daschuer commented Mar 20, 2024

ronso0 commented Mar 20, 2024

ronso0 commented Mar 22, 2024

ronso0 commented Apr 9, 2024

daschuer commented Apr 9, 2024

ronso0 commented Apr 9, 2024

daschuer commented Apr 10, 2024 • edited Loading

ronso0 commented Apr 10, 2024

daschuer commented Apr 10, 2024

ronso0 commented May 26, 2024

add `hotcue_X_indicator` control, is 1.0 if playposition is at hotcue #12951

add `hotcue_X_indicator` control, is 1.0 if playposition is at hotcue #12951

ronso0 commented Mar 11, 2024 •

edited

Loading

ronso0 commented Mar 16, 2024 •

edited

Loading

ronso0 commented Mar 20, 2024 •

edited

Loading

daschuer commented Apr 10, 2024 •

edited

Loading