EngineBuffer: Reset sample counter after indicator update

[alajovic]

Playpos indicator updates in EngineBuffer are supposed to happen with a rate of 15 Hz:

mixxx/src/engine/enginebuffer.cpp

Line 53 in 3ebac44

constexpr int kPlaypositionUpdateRate = 15; // updates per second

There is a counter that takes track of this:

mixxx/src/engine/enginebuffer.h

Line 379 in 3ebac44

int m_iSamplesSinceLastIndicatorUpdate;

but it is never reset after an update:

mixxx/src/engine/enginebuffer.cpp

Lines 1477 to 1482 in 3ebac44

	if (m_iSamplesSinceLastIndicatorUpdate >
	(kSamplesPerFrame * m_pSampleRate->get() /
	kPlaypositionUpdateRate)) {
	m_playposSlider->set(fFractionalPlaypos);
	m_pCueControl->updateIndicators();
	}

As a result, playpos updates are issued at a rate much faster than 15 Hz. I noticed this because the DDJ-SX mapping relies on those 15 Hz to blink the jog dial when the end of a track approaches, and the blinking was way too fast and erratic.

It seems that the counter reset was accidentally removed in 5a4444d. I added it back. The issue is present both in 2.5 and 2.6, so the fix targets 2.5.

[alajovic]

Eh. Missed the fact that this member variable had been renamed between 2.5 and 2.6, and I committed the fix with 2.6 name to 2.5. Surprise – it didn't build. Fixed that now.

[ronso0]

Thank you.
I didn't test this or anything, but IMO mappings shouldn't use a custom ed-of-track threshold but connect to [ChannelN],end_of_track (threshold set in Preferences ‣ Waveforms ‣ End of track warning) so the GUI and controller are in sync (not necessarily the blining but the timing)

[alajovic]

@ronso0 Fair point, but that's a separate issue. The mapping itself is in need of a modernization. I have recently gotten a DDJ-SX and mixxx logged warnings becuse the mapping was still using connectControl instead of makeConnection. It has fallen behind in several other aspects too. I have fixed most of that already and I'm testing the updated mapping with the controller. Once I'm reasonably sure that I have not screwed anything up, I'll open a PR. So while I'm at it, I can also try adjusting the blink code so that it will take into account [ChannelN],end_of_track.

That sample counter is a parallel issue — the counter exists, but it starts off uninitialized (it's assigned only in setNewPlaypos) and is never reset. I think there's a fair chance that the respective code was simply lost during refactoring.

[ronso0]

I agree to both.

Will take a look at this soonish.

[ronso0]

Though CI fails (on some targets onl??) because the engine somehow relies on the current behavior?
Can you take a look at that, too?

[ronso0]

Maybe we need to keep playposition with fast update and add a visual_playposition control which updates at 15 Hz?
(just a wild guess..)

[JoergAtGithub]

visual_playposition is updated with the VSYNC of the main screen. This must be always in sync to prevent visual jitter.

[alajovic]

Some CI targets indeed failed, and the pattern seems random. Almost as if it were some kind of a timing issue. 😁 I'll look into this.

[alajovic]

Tests assume that a call to ProcessBuffer() will always update playposition. Before my change, this was indeed the case, and it was a direct consequence of not resetting the counter. The counter just kept incrementing, consequently it was always above the threshold, and playposition updated on every call. But now it doesn't - it updates every couple of calls (~15 Hz). The only thing that's slightly puzzling is why only some CI pipelines failed on the tests - because this behavior looks completely deterministic to me.

Not sure how to proceed. On one hand, it sounds sensible to keep playposition accurate, i.e. to not decimate. On the other hand, if we don't decimate, a mapping that connects to this CO will receive a lot of updates, potentially at a rate that (I imagine) can saturate the MIDI channel if every update is passed onwards. This is outside of my area of expertise. Please let me know your thoughts.

[ronso0]

@alajovic you're right, playposition is updated on every process call¹, as well as CueControl::updateIndicatorsAndModifyPlay()

¹ corresponds to buffer size, or "engine process rate", which may be up to 750 Hz with minimal buffer of 1.33 ms
this may also explain why we didn't get bug reports for latencies used by most users (just my assumption) of 5-20 ms

Btw IIUC this also calls AutoDJProcessor::calculateTransition at engine rate, which may be a bit too much IMO

---

visual_playposition is updated with the VSYNC of the main screen.

VisualPlayposition is a class, there is no CO visual_playposition.
My suggestion was just to keep playposition updates as is (engine rate) and add a CO that updates with the desired rate of 15 Hz.

---

@alajovic as a quick fix for your mapping issue I suggest you move m_playposSlider->set(fFractionalPlaypos); out of the rate limiting if scope.
We need to check which consequences the rapid plapyos updates have, and which units actually rely on this fast update by no -- and which other units might suffer from it. (Woverview for example does not, it repaints only if the resulting pixel pos ahs changed, so fast updates are irrelevant even though the slot is called)

@daschuer maybe you can shed a light on this?