CoreServices: Add ControlIndicatorTimer to remove GuiTick dependency #4157

Holzhaus · 2021-07-28T17:04:07Z

Previously, the ControlIndicator objects relied on the
[Master],guiTickTime and [Master],guiTick50ms that are managed by
the GuiTick class and processed in the vsync thread during waveform
processing. But the blinking of indicator controls is unrelated to
Waveforms and should still work when we get rid of the legacy skin
system and waveforms widgets.

This simple change introduces two new control objects:

[Master],indicator_250millis
[Master],indicator_500millis

Both switch between 0.0 and 1.0 every 250/500ms. ControlIndicator has
been adapted to use these new controls instead.

These controls may also be used by controller mappings that want to
implement custom blinking controls. Before, this was already possible
using a timer in JavaScript, but these were not synchronized with the
other blinking controls, such as [ChannelN],play_indicator or
[ChannelN],cue_indicator which made them more annoying than necessary.

uklotzde

How about using a dedicated thread? The UI event loop where this object is supposed to live causes latency. Probably over-engineering, could be refactored later.

src/control/controlindicator.cpp

src/control/controlindicatortimer.cpp

src/control/controlindicator.h

coveralls · 2021-07-28T20:12:40Z

Pull Request Test Coverage Report for Build 1080639362

23 of 58 (39.66%) changed or added relevant lines in 3 files are covered.
10 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.02%) to 28.607%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/control/controlindicator.h	0	3	0.0%
src/control/controlindicator.cpp	14	21	66.67%
src/control/controlindicatortimer.cpp	9	34	26.47%

Files with Coverage Reduction	New Missed Lines	%
src/control/controlindicator.cpp	1	77.55%
src/engine/cachingreader/cachingreader.cpp	9	71.38%

Totals
Change from base Build 1080508055:	-0.02%
Covered Lines:	20090
Relevant Lines:	70227

💛 - Coveralls

daschuer · 2021-07-28T21:06:37Z

src/control/controlindicatortimer.cpp

+    m_pCOIndicator250millis->setReadOnly();
+    m_pCOIndicator500millis->setReadOnly();
+    connect(&m_timer, &QTimer::timeout, this, &ControlIndicatorTimer::slotTimeout);
+    m_timer.start(250);


The QTimer has special code that ensures to deliver the signal as close as possible to the given time.
That sounds appealing on the fist sight, unfortunately this introduces kind of priority for the connected events that interfere with the vsync code that has a hard deadline to met to avoid jerking. If a blink event happens during vsync, it is delayed together with the whole GUI until the next buffer is swapped. If there is pending waveform work to do, it is shiftet to the next buffer which is causing a jerk.

That was the original reason to schedule the repainting of blinking widgets with the vsync thread.

This is probably not relevant for QML widgets where the vsync is probably done later, but it still applies for CPU rendered widgets. Can we make the time source conditionally depending on the skin type?

I assume that we need some kind of vsync interval for any waveform solution.
So maybe it is best to provide an generic interface for such syncing.

What do you think?

Would running the timer in its own thread solve this?

The QTimer has special code that ensures to deliver the signal as close as possible to the given time.

The default timer type is Qt::CoarseTimer, that "try to keep accuracy within 5% of the desired interval". It's not a Qt::PreciseTimer which would try to keep millisecond accuracy.

That sounds appealing on the fist sight, unfortunately this introduces kind of priority for the connected events that interfere with the vsync code that has a hard deadline to met to avoid jerking. If a blink event happens during vsync, it is delayed together with the whole GUI until the next buffer is swapped. If there is pending waveform work to do, it is shiftet to the next buffer which is causing a jerk.

If that was a problem, wouldn't that apply to all QTimers in the GUI thread? If so, why didn't we notice issues yet? We use QTimers often:

$ grep -Hrn "\bQTimer\b" ../src ../src/control/controlbehavior.h:3:#include <QTimer> ../src/control/controlbehavior.h:151: QTimer* getTimer() { ../src/control/controlbehavior.h:153: m_pushTimer.reset(new QTimer()); ../src/control/controlbehavior.h:159: QScopedPointer<QTimer> m_pushTimer; ../src/control/controlindicatortimer.h:4:#include <QTimer> ../src/control/controlindicatortimer.h:27: QTimer m_timer; ../src/control/controlindicatortimer.cpp:17: connect(&m_timer, &QTimer::timeout, this, &ControlIndicatorTimer::slotTimeout); ../src/widget/wtime.cpp:14: m_pTimer = new QTimer(this); ../src/widget/wtime.cpp:25: connect(m_pTimer, &QTimer::timeout, this, &WTime::refreshTime); ../src/widget/wsearchlineedit.h:6:#include <QTimer> ../src/widget/wsearchlineedit.h:95: QTimer m_debouncingTimer; ../src/widget/wsearchlineedit.h:96: QTimer m_saveTimer; ../src/widget/wtime.h:6:#include <QTimer> ../src/widget/wtime.h:25: QTimer* m_pTimer; ../src/widget/wsearchlinee$ grep -HrnF QTimer ../src ../src/control/controlbehavior.h:3:#include <QTimer> ../src/control/controlbehavior.h:151: QTimer* getTimer() { ../src/control/controlbehavior.h:153: m_pushTimer.reset(new QTimer()); ../src/control/controlbehavior.h:159: QScopedPointer<QTimer> m_pushTimer; ../src/control/controlindicatortimer.h:4:#include <QTimer> ../src/control/controlindicatortimer.h:27: QTimer m_timer; ../src/control/controlindicatortimer.cpp:17: connect(&m_timer, &QTimer::timeout, this, &ControlIndicatorTimer::slotTimeout); ../src/vinylcontrol/vinylcontrolmanager.cpp:169:void VinylControlManager::timerEvent(QTimerEvent*) { ../src/vinylcontrol/vinylcontrolsignalwidget.h:13:#include <QTimerEvent> ../src/vinylcontrol/vinylcontrolmanager.h:5:#include <QTimerEvent> ../src/vinylcontrol/vinylcontrolmanager.h:43: void timerEvent(QTimerEvent* pEvent); ../src/mixer/playerinfo.h:8:#include <QTimerEvent> ../src/mixer/playerinfo.h:49: void timerEvent(QTimerEvent* pTimerEvent) override; ../src/mixer/playerinfo.cpp:112:void PlayerInfo::timerEvent(QTimerEvent* pTimerEvent) { ../src/widget/wtime.cpp:14: m_pTimer = new QTimer(this); ../src/widget/wtime.cpp:25: connect(m_pTimer, &QTimer::timeout, this, &WTime::refreshTime); ../src/widget/wsearchlineedit.h:6:#include <QTimer> ../src/widget/wsearchlineedit.h:95: QTimer m_debouncingTimer; ../src/widget/wsearchlineedit.h:96: QTimer m_saveTimer; ../src/widget/wtime.h:6:#include <QTimer> ../src/widget/wtime.h:25: QTimer* m_pTimer; ../src/widget/wsearchlineedit.cpp:116: &QTimer::timeout, ../src/widget/wsearchlineedit.cpp:120: &QTimer::timeout, ../src/widget/wcoverart.h:7:#include <QTimer> ../src/widget/wcoverart.h:76: QTimer m_clickTimer; ../src/widget/wpushbutton.h:8:#include <QTimer> ../src/widget/wpushbutton.h:110: QTimer m_clickTimer; ../src/widget/wlibrarysidebar.cpp:110:void WLibrarySidebar::timerEvent(QTimerEvent *event) { ../src/widget/wlibrarysidebar.h:10:#include <QTimerEvent> ../src/widget/wlibrarysidebar.h:26: void timerEvent(QTimerEvent* event) override; ../src/library/dlgcoverartfullsize.h:5:#include <QTimer> ../src/library/dlgcoverartfullsize.h:50: QTimer m_clickTimer; ../src/library/sidebarmodel.h:6:#include <QTimer> ../src/library/sidebarmodel.h:90: QTimer* const m_pressedUntilClickedTimer; ../src/library/sidebarmodel.cpp:34: m_pressedUntilClickedTimer(new QTimer(this)) { ../src/library/sidebarmodel.cpp:37: &QTimer::timeout, ../src/waveform/guitick.cpp:1:#include <QTimer> ../src/dialog/dlgdevelopertools.cpp:92:void DlgDeveloperTools::timerEvent(QTimerEvent* pEvent) { ../src/dialog/dlgdevelopertools.h:6:#include <QTimerEvent> ../src/dialog/dlgdevelopertools.h:20: void timerEvent(QTimerEvent* pTimerEvent) override; ../src/util/timer.h:112:// A timer that provides a similar API to QTimer but uses render events from the ../src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp:471:void ControllerScriptInterfaceLegacy::timerEvent(QTimerEvent* event) { ../src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h:64: virtual void timerEvent(QTimerEvent* event); ../src/controllers/controllermanager.h:4:#include <QTimer> ../src/controllers/controllermanager.h:77: QTimer m_pollTimer; ../src/controllers/dlgcontrollerlearning.h:6:#include <QTimer> ../src/controllers/dlgcontrollerlearning.h:83: QTimer m_firstMessageTimer; ../src/controllers/dlgcontrollerlearning.h:84: QTimer m_lastMessageTimer; ../src/controllers/dlgcontrollerlearning.cpp:137: connect(&m_lastMessageTimer, &QTimer::timeout, this, &DlgControllerLearning::slotTimerExpired); ../src/controllers/dlgcontrollerlearning.cpp:142: &QTimer::timeout, ../src/controllers/controller.h:4:#include <QTimerEvent> ../src/controllers/controllermanager.cpp:101: connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::pollDevices); ../src/network/jsonwebtask.cpp:6:#include <QTimerEvent> ../src/network/webtask.cpp:4:#include <QTimerEvent> ../src/network/webtask.cpp:300:void WebTask::timerEvent(QTimerEvent* event) { ../src/network/webtask.h:146: void timerEvent(QTimerEvent* event) final; ```dit.cpp:116: &QTimer::timeout, ../src/widget/wsearchlineedit.cpp:120: &QTimer::timeout, ../src/widget/wcoverart.h:7:#include <QTimer> ../src/widget/wcoverart.h:76: QTimer m_clickTimer; ../src/widget/wpushbutton.h:8:#include <QTimer> ../src/widget/wpushbutton.h:110: QTimer m_clickTimer; ../src/library/dlgcoverartfullsize.h:5:#include <QTimer> ../src/library/dlgcoverartfullsize.h:50: QTimer m_clickTimer; ../src/library/sidebarmodel.h:6:#include <QTimer> ../src/library/sidebarmodel.h:90: QTimer* const m_pressedUntilClickedTimer; ../src/library/sidebarmodel.cpp:34: m_pressedUntilClickedTimer(new QTimer(this)) { ../src/library/sidebarmodel.cpp:37: &QTimer::timeout, ../src/waveform/guitick.cpp:1:#include <QTimer> ../src/util/timer.h:112:// A timer that provides a similar API to QTimer but uses render events from the ../src/controllers/controllermanager.h:4:#include <QTimer> ../src/controllers/controllermanager.h:77: QTimer m_pollTimer; ../src/controllers/dlgcontrollerlearning.h:6:#include <QTimer> ../src/controllers/dlgcontrollerlearning.h:83: QTimer m_firstMessageTimer; ../src/controllers/dlgcontrollerlearning.h:84: QTimer m_lastMessageTimer; ../src/controllers/dlgcontrollerlearning.cpp:137: connect(&m_lastMessageTimer, &QTimer::timeout, this, &DlgControllerLearning::slotTimerExpired); ../src/controllers/dlgcontrollerlearning.cpp:142: &QTimer::timeout, ../src/controllers/controllermanager.cpp:101: connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::pollDevices);

I think accuracy within 5% should be fine for blinking these COs.

If a blink event happens during vsync, it is delayed together with the whole GUI until the next buffer is swapped. If there is pending waveform work to do, it is shiftet to the next buffer which is causing a jerk.

This seems to be a misunderstanding based on unverified assumptions as @Holzhaus explained above. Testing this branch, I do not see any regression in waveform jerking and the indicator controls continue to work.

I switched between this branch and main several times playing 2 decks with 2 decks paused using the Pioneer cue mode which has (annoyingly) frequent blinking and did not notice a difference in waveform scrolling.

Be-ing · 2021-07-28T22:45:37Z

src/control/controlindicatortimer.h

+/// This class provides two generic indicator controls that change their value
+/// in intervals of 250 milliseconds and 500 milliseconds. These controls are
+/// used by all `ControlIndicator` COs and may also be used to implement custom
+/// blinking buttons (e.g. for Explicit Sync Leader) on controllers. This


huh? This seems off topic.

Do you have a suggestion what to change? I think it makes sense to explain what these COs are used for and that they are not "internal", i.e. that they are useful for third party code, too. I was planning to update the Roland DJ-505 to use them, because the blinking of the SYNC button not being in sync (hah! ;-) with the CUE button is annoying.

I don't think controller scripts should be implementing custom blinking in general.

In general I agree that we should avoid blinking. But in some cases it can be useful. My sync button only has a single color, and in the rare cases I deem it necessary to set a sync leader explicitly, I want to know which deck is sync leader.

active loop, playing sampler, key set off etc. all deserve blinking if the user considers that state unusual/remarkable

src/coreservices.h

Be-ing · 2021-07-29T03:00:04Z

src/control/controlindicatortimer.cpp

+
+void ControlIndicatorTimer::slotTimeout() {
+    // The timeout uses an interval of 250ms, so the 250ms indicator is always updated.
+    m_pCOIndicator250millis->forceSet(1 - m_pCOIndicator250millis->get());


This is an odd way to toggle a boolean value.

Suggested change

m_pCOIndicator250millis->forceSet(1 - m_pCOIndicator250millis->get());

m_pCOIndicator250millis->forceSet(!m_pCOIndicator250millis->toBool());

...because storing a boolean as double is odd ;) But I agree, for the sake of readability use this less efficient variant.

lol, we went full circle with this :D I think I prefer the original version then. That makes it clear that we're storing doubles.

Suggested change

m_pCOIndicator250millis->forceSet(1 - m_pCOIndicator250millis->get());

m_pCOIndicator250millis->forceSet(m_pCOIndicator250millis->toBool() ? 0.0 : 1.0);

sure that works

Be-ing · 2021-07-29T03:35:40Z

How about using a dedicated thread? The UI event loop where this object is supposed to live causes latency. Probably over-engineering, could be refactored later.

The current implementation seems to work fine, so I think a separate thread would be overengineering.

Be-ing · 2021-07-29T03:38:27Z

Thank you for working on this. This is much better than entangling CoreServices with the QWidgets GUI.

Be-ing · 2021-07-29T04:23:24Z

DeckVisuals is still coupled to WaveformWidgetFactory. I suspect that can be refactored to use a similar architecture driven by a QTimer in the main thread, maybe even using this same ControlIndicatorTimer. That can be resolved in another PR.

Be-ing · 2021-07-29T04:37:29Z

Do we still need GuiTick and GuiTickTimer now? I am not sure, but I am inclined to say let's not fix what isn't broken. I don't want to reopen the can of worms that led to implementing WidgetRenderTimer.

Holzhaus · 2021-07-29T08:31:49Z

Do we still need GuiTick and GuiTickTimer now? I am not sure, but I am inclined to say let's not fix what isn't broken. I don't want to reopen the can of worms that led to implementing WidgetRenderTimer.

GuiTick/GuiTickTimer are still used by WidgetRenderTimer, WTrackTable and the battery widget. I wasn't planning to touch that code, so let's just leave it in for legacy skins and remove GuiTick/GuiTickTimer when we remove support for these skins.

daschuer · 2021-07-29T08:49:43Z

Would running the timer in its own thread solve this?

No, the timer source is the issue, CPU clock vs GPU clock.

If that was a problem, wouldn't that apply to all QTimers in the GUI thread? If so, why didn't we notice issues yet? We use QTimers often:

In general we should avoid timers in the GUI thread, that triggers a repainting, not v-synced. The issue is that every timer makes the situation a tiny bit worse. You can argue case by case that it is not worth the extra effort but the sume of all will finally make the difference.

I think accuracy within 5% should be fine for blinking these COs.

The 5% is used to do similar timed actions in one callback. That is not related to the issue with v-sync, because that is independent. The current solution in main considers the v-sync interval as well. If this where also a QTimer it would work well, but it is not, unfortunately.

testing this branch, I do not see any regression in waveform jerking and the indicator controls continue to work.

@Be-ing It is hard to test. Please describe you test setup in details.

daschuer

All the performance discussion is kind of mood compared to the view lines of code that are required to use the v-sync thread as alternative timing source for these new objects.
So I would prefer to add these lines and continue migrating to a new waveform rendering without introducing regressions into the old in the meantime.

@Holzhaus What do you think?

Holzhaus · 2021-07-29T09:09:16Z

All the performance discussion is kind of mood compared to the view lines of code that are required to use the v-sync thread as alternative timing source for these new objects.
So I would prefer to add these lines and continue migrating to a new waveform rendering without introducing regressions into the old in the meantime.

I honestly don't know how to implement this. I could of course add a controlproxy for the guiTick COs and use that if the CO exists, otherwise use a qtimer. But this won't work, because the guiTick CO would be leaked and stop being updated when we switch to a QML skin.

Also, are you really sure that changing the appearance of a widget depending on the CO value actually takes immediate effect and may cause issues with VSync? In my understanding, something like QAbstractButton::setIcon() will probably call QWidget::update() internally (it just looked it up, it does):

This function does not cause an immediate repaint; instead it schedules a paint event for processing when Qt returns to the main event loop. This permits Qt to optimize for more speed and less flicker than a call to repaint() does.

So we don't actually control the exact moment when the widget is redrawn. And if that causes conflicts with vsync, we have a much bigger issue at hand (or maybe don't, because we didn't notice any issues yet).

daschuer · 2021-07-29T10:07:26Z

I honestly don't know how to implement this.

The new object can be either connected to the vsync thread If exist or to the QTimer. Something like: if (old skin) start vsync thread; else start QTimer

daschuer · 2021-07-29T10:23:54Z

Also, are you really sure that changing the appearance of a widget depending on the CO value actually takes immediate effect and may cause issues with VSync?

My knowledge is based on extensive tests and code review of older QT versions, so I am not certain if all of this still applies, but the basic principles should still exists.

Qt has its own vsync code that avoid tearing.
In a risk of tearing graphic updates are delated after the buffer swap. The main thread is suspended unable to do anything else then.

Our vsync thread does the same in a different thread to avoid these extra suspends. If we sync all GUI actions with our v-sync thread, we can avoid many of these extra suspends, that leads to a single delayed waveform update.

In my understanding, something like QAbstractButton::setIcon() will probably call QWidget::update() internally (it just looked it up, it does):

Yes that's correct. That causes extra jerking when interact with the widgets by not synced user input. Fortunately less notable than jerking during Mixxx ideling around where the user may whatch the waveform.

The cited doc explains how qt does it by default.
Our issue is that we have the need for our own, because the Qt way is optimized for desktop Apps not die Video Playback.

Both interfere each other. Syncing the timer with our own lints the issue, because it avoid suspends in the Qt implementation.

daschuer · 2021-07-29T10:29:17Z

The best way to solve the issue for both would be to receive a v-synced timer out of the Qt anti tearing code. To find out that is on my to do list.
Unfortunately that is buried deep inside target depending code.
Any help is Welcome.

uklotzde · 2021-07-29T11:36:39Z

@daschuer I suggest that we start with this simple solution unless you are able to demonstrate that it causes severe issues.

We could revert this decision at any time, no need to block progress. Let's follow YAGNI. The issues might only affect outdated Qt versions. No one knows and it has not been documented properly.

Be-ing · 2021-07-29T11:38:08Z

@Be-ing It is hard to test. Please describe you test setup in details.

I did. What more details do you want?

uklotzde · 2021-07-29T11:40:12Z

@Be-ing It is hard to test. Please describe you test setup in details.

I did. What more details do you want?

If users or even developers are not noticing issues it is not worth fixing ;)

Be-ing · 2021-07-29T11:52:26Z

In general we should avoid timers in the GUI thread, that triggers a repainting, not v-synced. The issue is that every timer makes the situation a tiny bit worse. You can argue case by case that it is not worth the extra effort but the sume of all will finally make the difference.

The purpose of this PR is to work towards replacing the entire QWidgets GUI system so I don't think such long term concerns matter. That is assuming your understanding of the issue is correct, which I am not sure is the case, and this concern might not be a problem at all.

The best way to solve the issue for both would be to receive a v-synced timer out of the Qt anti tearing code. To find out that is on my to do list.

This seems like more overengineering and I suspect it is not relevant to Qt Quick. The legacy waveform rendering is working okay enough; let's not fix what isn't broken.

Be-ing · 2021-07-29T11:56:18Z

@uklotzde @Holzhaus @ronso0 can you try testing as I described and confirm you do not notice any regression in waveform jerking?

It would be good to confirm there is no regression on macOS too where we have had lots of mysterious performance problems. @foss- can you test the GitHub Actions artifact from this PR with the setup I described and confirm that waveforms do not jerk anymore than before?

Due to concerns that a regular QTimer that fires during vsync could cause jerking legacy waveforms, this implements a way to use the legacy vsync thread/GuiTick infrastructure instead of a `QTimer` in the `ControlIndicatorTimer` (although I'm still not convinced that this actually makes a difference). This can be toggled explicitly using `setLegacyVsyncEnabled` when switching between a legacy and a QML skin (the latter has no vsync thread, so a regular timer needs to be used). Relevant discussion can be found here: mixxxdj#4157 (comment)

Holzhaus · 2021-07-29T12:02:40Z

Whoops, I didn't notice the discussion here and already implemented the legacy vsync mode (although I still doubt that it's needed): fb86a27

We can later simply revert that commit when we drop legacy skins. Or should I remove it now because of YAGNI and only apply it if someone actually notices waveform jerking?

src/control/controlindicatortimer.cpp

Be-ing · 2021-07-29T12:25:55Z

should I remove it now because of YAGNI and only apply it if someone actually notices waveform jerking?

It seems to work the same either way. It is not much code so I do not mind keeping it.

daschuer · 2021-07-30T00:33:38Z

I have tested this with the VSyncTest (GL) waveform at 60 Hz and Pioneer mode enabled on Ubuntu Groovy.
A constantly pink waveform is an indicator for perfect vsync rendering.

I can confirm that this branch now has no regression compared to main.
I experience some rare jerks in the rhythm of the button blinking here and in main. As far as I remember there was no such dependency originally. So there must be something changes under the hood with the qt anti tearing code that is now less optimal for us.

With the QTimer version the situation is worse. I see some kind of interference interval. A flawlessly pink wavefront for some seconds is cycling with heavy jerking also for some seconds.

Unfortunately I am not able to record this, because the recording software "SimpleScreenRecoder" is not able to record every frame vsynced.

@Holzhaus Thank you for the legacy fix.

Holzhaus · 2021-07-30T00:37:52Z

OK, ready to merge then?

daschuer

There is one remaining copy and paste issue to fix.

daschuer · 2021-07-30T00:34:32Z

src/control/controlindicatortimer.cpp

+
+void ControlIndicatorTimer::slotTimeout() {
+    // The timeout uses an interval of 250ms, so the 250ms indicator is always updated.
+    m_pCOIndicator250millis->forceSet(m_pCOIndicator500millis->toBool() ? 0.0 : 1.0);


Suggested change

m_pCOIndicator250millis->forceSet(m_pCOIndicator500millis->toBool() ? 0.0 : 1.0);

m_pCOIndicator250millis->forceSet(m_pCOIndicator250millis->toBool() ? 0.0 : 1.0);

daschuer · 2021-07-30T00:36:49Z

src/control/controlindicatortimer.cpp

+    if (m_toggleIndicator500millisOnNextTimeout) {
+        m_pCOIndicator500millis->forceSet(m_pCOIndicator500millis->toBool() ? 0.0 : 1.0);
+    }
+    m_toggleIndicator500millisOnNextTimeout = !m_toggleIndicator500millisOnNextTimeout;


Idea: you can get rid of this extra bool by using a local with the result from m_pCOIndicator250millis->toBool() instead.

Previously, the `ControlIndicator` objects relied on the `[Master],guiTickTime` and `[Master],guiTick50ms` that are managed by the `GuiTick` class and processed in the vsync thread during waveform processing. But the blinking of indicator controls is unrelated to Waveforms and should still work when we get rid of the legacy skin system and waveforms widgets. This simple change introduces two new control objects: - `[Master],indicator_250millis` - `[Master],indicator_500millis` Both switch between 0.0 and 1.0 every 250/500ms. `ControlIndicator` has been adapted to use these new controls instead. These controls may also be used by controller mappings that want to implement custom blinking controls. Before, this was already possible using a timer in JavaScript, but these were not synchronized with the other blinking controls, such as `[ChannelN],play_indicator` or `[ChannelN],cue_indicator` which made them more annoying than necessary.

Due to concerns that a regular QTimer that fires during vsync could cause jerking legacy waveforms, this implements a way to use the legacy vsync thread/GuiTick infrastructure instead of a `QTimer` in the `ControlIndicatorTimer` (although I'm still not convinced that this actually makes a difference). This can be toggled explicitly using `setLegacyVsyncEnabled` when switching between a legacy and a QML skin (the latter has no vsync thread, so a regular timer needs to be used). Relevant discussion can be found here: mixxxdj#4157 (comment)

The `m_toggleIndicator500millisOnNextTimeout` can be removed, because the 500ms indicator just needs to be toggled on every second toggle of the 250ms indicator.

Due to concerns that a regular QTimer that fires during vsync could cause jerking legacy waveforms, this implements a way to use the legacy vsync thread/GuiTick infrastructure instead of a `QTimer` in the `ControlIndicatorTimer` (although I'm still not convinced that this actually makes a difference). This can be toggled explicitly using `setLegacyVsyncEnabled` when switching between a legacy and a QML skin (the latter has no vsync thread, so a regular timer needs to be used). Relevant discussion can be found here: mixxxdj#4157 (comment)

Be-ing · 2021-07-30T02:22:23Z

@daschuer merge?

daschuer

LGTM

uklotzde · 2021-07-30T07:47:15Z

Good job, everyone!

These controls were added in mixxxdj/mixxx#4157.

Holzhaus added the engine label Jul 28, 2021

github-actions bot added the build label Jul 28, 2021

uklotzde reviewed Jul 28, 2021

View reviewed changes

src/control/controlindicator.cpp Outdated Show resolved Hide resolved

src/control/controlindicatortimer.cpp Outdated Show resolved Hide resolved

src/control/controlindicatortimer.cpp Show resolved Hide resolved

Holzhaus force-pushed the controlindicatortimer branch from baa91e5 to da1e3d5 Compare July 28, 2021 19:30

github-actions bot added the code quality label Jul 28, 2021

Holzhaus mentioned this pull request Jul 28, 2021

Start to untangle CoreServices/MixxxMainWindow initialization #4110

Merged

uklotzde reviewed Jul 28, 2021

View reviewed changes

src/control/controlindicator.h Outdated Show resolved Hide resolved

uklotzde reviewed Jul 28, 2021

View reviewed changes

src/control/controlindicator.h Outdated Show resolved Hide resolved

uklotzde suggested changes Jul 28, 2021

View reviewed changes

src/control/controlindicator.h Outdated Show resolved Hide resolved

daschuer reviewed Jul 28, 2021

View reviewed changes

Holzhaus force-pushed the controlindicatortimer branch from da1e3d5 to 8e38e87 Compare July 28, 2021 22:40

Be-ing reviewed Jul 28, 2021

View reviewed changes

Be-ing reviewed Jul 29, 2021

View reviewed changes

src/coreservices.h Show resolved Hide resolved

Be-ing reviewed Jul 29, 2021

View reviewed changes

daschuer requested changes Jul 29, 2021

View reviewed changes

uklotzde approved these changes Jul 29, 2021

View reviewed changes

Holzhaus force-pushed the controlindicatortimer branch from 8e38e87 to fb86a27 Compare July 29, 2021 12:00

Be-ing reviewed Jul 29, 2021

View reviewed changes

src/control/controlindicatortimer.cpp Show resolved Hide resolved

Holzhaus requested a review from daschuer July 29, 2021 12:36

Holzhaus mentioned this pull request Jul 29, 2021

Roland DJ-505: Use new ControlIndicator COs for blinking lights #4159

Merged

daschuer requested changes Jul 30, 2021

View reviewed changes

Holzhaus force-pushed the controlindicatortimer branch from fb86a27 to 770077f Compare July 30, 2021 00:42

Holzhaus and others added 2 commits July 30, 2021 03:00

ControlIndicatorTimer: Remove unnecessary member variable

b891a96

The `m_toggleIndicator500millisOnNextTimeout` can be removed, because the 500ms indicator just needs to be toggled on every second toggle of the 250ms indicator.

Holzhaus force-pushed the controlindicatortimer branch from 770077f to 1c44c3a Compare July 30, 2021 01:10

Be-ing approved these changes Jul 30, 2021

View reviewed changes

daschuer approved these changes Jul 30, 2021

View reviewed changes

daschuer merged commit 528fcc9 into mixxxdj:main Jul 30, 2021

Holzhaus added a commit to Holzhaus/manual that referenced this pull request Aug 2, 2021

mixxx_controls: Document new indicator controls

cbd7d89

These controls were added in mixxxdj/mixxx#4157.

Holzhaus mentioned this pull request Aug 2, 2021

mixxx_controls: Document new indicator controls mixxxdj/manual#421

Merged

	m_pCOIndicator250millis->forceSet(1 - m_pCOIndicator250millis->get());
	m_pCOIndicator250millis->forceSet(!m_pCOIndicator250millis->toBool());

	m_pCOIndicator250millis->forceSet(1 - m_pCOIndicator250millis->get());
	m_pCOIndicator250millis->forceSet(m_pCOIndicator250millis->toBool() ? 0.0 : 1.0);

CoreServices: Add ControlIndicatorTimer to remove GuiTick dependency #4157

CoreServices: Add ControlIndicatorTimer to remove GuiTick dependency #4157

Conversation

Holzhaus commented Jul 28, 2021

uklotzde left a comment

Choose a reason for hiding this comment

coveralls commented Jul 28, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1080639362

💛 - Coveralls

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

Be-ing Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Holzhaus Jul 28, 2021 • edited Loading

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

Be-ing commented Jul 29, 2021

Be-ing commented Jul 29, 2021

Be-ing commented Jul 29, 2021 • edited Loading

Be-ing commented Jul 29, 2021

Holzhaus commented Jul 29, 2021 • edited Loading

daschuer commented Jul 29, 2021

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Jul 29, 2021 • edited Loading

daschuer commented Jul 29, 2021

daschuer commented Jul 29, 2021

daschuer commented Jul 29, 2021

uklotzde commented Jul 29, 2021

Be-ing commented Jul 29, 2021

uklotzde commented Jul 29, 2021

Be-ing commented Jul 29, 2021

Be-ing commented Jul 29, 2021

Holzhaus commented Jul 29, 2021 • edited Loading

Be-ing commented Jul 29, 2021

daschuer commented Jul 30, 2021

Holzhaus commented Jul 30, 2021

daschuer left a 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

Be-ing commented Jul 30, 2021

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Jul 30, 2021

coveralls commented Jul 28, 2021 •

edited

Loading

Be-ing Jul 29, 2021 •

edited

Loading

Be-ing Jul 28, 2021 •

edited

Loading

Holzhaus Jul 28, 2021 •

edited

Loading

Be-ing commented Jul 29, 2021 •

edited

Loading

Holzhaus commented Jul 29, 2021 •

edited

Loading

Holzhaus commented Jul 29, 2021 •

edited

Loading

Holzhaus commented Jul 29, 2021 •

edited

Loading