Merge pull request mixxxdj#1809 from rryan/qt5-knob-sluggishness

Render WKnob, WKnobComposed and WSliderComposed using the GuiTick timer. Bug #1793015

build/depends.py

@@ -1231,6 +1231,7 @@ def sources(self, build):
                    "util/autohidpi.cpp",
                    "util/screensaver.cpp",
                    "util/indexrange.cpp",
+                   "util/widgetrendertimer.cpp",
 
                    '#res/mixxx.qrc'
                    ]

src/util/timer.cpp

@@ -1,5 +1,7 @@
 #include "util/timer.h"
+
 #include "util/experiment.h"
+#include "util/time.h"
 #include "waveform/guitick.h"
 
 Timer::Timer(const QString& key, Stat::ComputeFlags compute)
@@ -80,32 +82,30 @@ mixxx::Duration SuspendableTimer::elapsed(bool report) {
 
 GuiTickTimer::GuiTickTimer(QObject* pParent)
         : QObject(pParent),
-          m_pGuiTick50ms(new ControlProxy(
-              "[Master]", "guiTick50ms", this)),
+          m_pGuiTick(make_parented<ControlProxy>(
+              "[Master]", "guiTickTime", this)),
           m_bActive(false) {
-    m_pGuiTick50ms->connectValueChanged(SLOT(slotGuiTick50ms(double)));
-}
-
-GuiTickTimer::~GuiTickTimer() {
 }
 
 void GuiTickTimer::start(mixxx::Duration duration) {
+    m_pGuiTick->connectValueChanged(SLOT(slotGuiTick(double)));
     m_interval = duration;
-    m_elapsed = mixxx::Duration::fromSeconds(0);
+    m_lastUpdate = mixxx::Duration::fromSeconds(0);
     m_bActive = true;
 }
 
 void GuiTickTimer::stop() {
+    m_pGuiTick->disconnect();
     m_bActive = false;
     m_interval = mixxx::Duration::fromSeconds(0);
-    m_elapsed = mixxx::Duration::fromSeconds(0);
+    m_lastUpdate = mixxx::Duration::fromSeconds(0);
 }
 
-void GuiTickTimer::slotGuiTick50ms(double) {
+void GuiTickTimer::slotGuiTick(double) {
     if (m_bActive) {
-        m_elapsed += mixxx::Duration::fromMillis(50);
-        if (m_elapsed >= m_interval) {
-            m_elapsed = mixxx::Duration::fromSeconds(0);
+        auto time = mixxx::Time::elapsed();
+        if (time - m_lastUpdate >= m_interval) {
+            m_lastUpdate = time;
             emit(timeout());
         }
     }

src/util/timer.h

@@ -2,13 +2,13 @@
 #define UTIL_TIMER_H
 
 #include <QObject>
-#include <QScopedPointer>
 
 #include "control/controlproxy.h"
-#include "util/stat.h"
-#include "util/performancetimer.h"
 #include "util/cmdlineargs.h"
 #include "util/duration.h"
+#include "util/parented_ptr.h"
+#include "util/performancetimer.h"
+#include "util/stat.h"
 
 const Stat::ComputeFlags kDefaultComputeFlags = Stat::COUNT | Stat::SUM | Stat::AVERAGE |
         Stat::MAX | Stat::MIN | Stat::SAMPLE_VARIANCE;
@@ -110,27 +110,28 @@ class ScopedTimer {
     bool m_cancel;
 };
 
-// A timer that provides a similar API to QTimer but uses the GuiTick 50ms
-// signal provided by the VsyncThread.
+// A timer that provides a similar API to QTimer but uses render events from the
+// VSyncThread as its source of timing events. This means the timer cannot fire
+// at a rate faster than the user's configured waveform FPS.
 class GuiTickTimer : public QObject {
     Q_OBJECT
   public:
     GuiTickTimer(QObject* pParent);
-    virtual ~GuiTickTimer();
 
     void start(mixxx::Duration interval);
+    bool isActive() const { return m_bActive; }
     void stop();
 
   signals:
     void timeout();
 
   private slots:
-    void slotGuiTick50ms(double v);
+    void slotGuiTick(double v);
 
   private:
-    QScopedPointer<ControlProxy> m_pGuiTick50ms;
+    parented_ptr<ControlProxy> m_pGuiTick;
     mixxx::Duration m_interval;
-    mixxx::Duration m_elapsed;
+    mixxx::Duration m_lastUpdate;
     bool m_bActive;
 };
 

src/util/widgetrendertimer.cpp

@@ -0,0 +1,30 @@
+#include "util/widgetrendertimer.h"
+
+#include "util/time.h"
+
+WidgetRenderTimer::WidgetRenderTimer(mixxx::Duration renderFrequency,
+                                     mixxx::Duration inactivityTimeout)
+        : m_renderFrequency(renderFrequency),
+          m_inactivityTimeout(inactivityTimeout),
+          m_guiTickTimer(this) {
+    connect(&m_guiTickTimer, SIGNAL(timeout()),
+            this, SLOT(guiTick()));
+}
+
+void WidgetRenderTimer::guiTick() {
+    mixxx::Duration now = mixxx::Time::elapsed();
+    if (now - m_lastActivity > m_inactivityTimeout) {
+        m_guiTickTimer.stop();
+    }
+    if (m_lastActivity > m_lastRender) {
+        m_lastRender = m_lastActivity;
+        emit(update());
+    }
+}
+
+void WidgetRenderTimer::activity() {
+    m_lastActivity = mixxx::Time::elapsed();
+    if (!m_guiTickTimer.isActive()) {
+        m_guiTickTimer.start(m_renderFrequency);
+    }
+}

src/util/widgetrendertimer.h

@@ -0,0 +1,51 @@
+#pragma once
+
+#include <QObject>
+
+#include "util/duration.h"
+#include "util/timer.h"
+
+// A helper class for rendering widgets on a timer when they need updating.
+// Prevents calling QWidget::update too quickly. Controlled by two parameters:
+// - renderFrequency: The frequency to render the widget at in response
+//   to updates.
+// - inactivityTimeout: The timeout after which the widget's render timer is
+//   deactivated.
+//
+// This class was created in response to Launchpad Bug #1793015. With Qt 4, we
+// would simply call QWidget::update in response to input events that required
+// re-rendering widgets, relying on Qt to batch them together and deliver them
+// at a reasonable frequency. On macOS, the behavior of QWidget::update in Qt 5
+// seems to have changed such that render events happen much more frequently
+// than they used to. To address this, we instead use a downsampling timer
+// attached to the VSyncThread's render ticks for the waveform renderers. The
+// timer invokes guiTick(), which is responsible for actually calling
+// QWidget::update(). When input arrives, we call inputActivity to attach the
+// timer. After 1 second of inactivity, we disconnect the timer.
+class WidgetRenderTimer : public QObject {
+    Q_OBJECT
+  public:
+    WidgetRenderTimer(mixxx::Duration renderFrequency,
+                      mixxx::Duration inactivityTimeout);
+
+    // Call this method whenever the widget's state has changed such that a
+    // re-render is necessary.
+    void activity();
+
+  signals:
+    // Emitted when the widget should actually render. Connect this signal to
+    // QWidget::update or QWidget::repaint.
+    void update();
+
+  private slots:
+    // Called when the internal GuiTickTimer's timeout is elapsed. Decides
+    // whether the widget should render in response to this timer tick.
+    void guiTick();
+
+  private:
+    const mixxx::Duration m_renderFrequency;
+    const mixxx::Duration m_inactivityTimeout;
+    GuiTickTimer m_guiTickTimer;
+    mixxx::Duration m_lastActivity;
+    mixxx::Duration m_lastRender;
+};

src/waveform/guitick.cpp

@@ -1,18 +1,13 @@
 #include <QTimer>
 
-#include "guitick.h"
+#include "waveform/guitick.h"
 #include "control/controlobject.h"
 
 GuiTick::GuiTick(QObject* pParent)
         : QObject(pParent) {
-     m_pCOGuiTickTime = new ControlObject(ConfigKey("[Master]", "guiTickTime"));
-     m_pCOGuiTick50ms = new ControlObject(ConfigKey("[Master]", "guiTick50ms"));
-     m_cpuTimer.start();
-}
-
-GuiTick::~GuiTick() {
-    delete m_pCOGuiTickTime;
-    delete m_pCOGuiTick50ms;
+    m_pCOGuiTickTime = std::make_unique<ControlObject>(ConfigKey("[Master]", "guiTickTime"));
+    m_pCOGuiTick50ms = std::make_unique<ControlObject>(ConfigKey("[Master]", "guiTick50ms"));
+    m_cpuTimer.start();
 }
 
 // this is called from the VSyncThread

src/waveform/guitick.h

@@ -3,21 +3,22 @@
 
 #include <QObject>
 
+#include "control/controlobject.h"
 #include "util/duration.h"
+#include "util/memory.h"
 #include "util/performancetimer.h"
 
-class ControlObject;
-
+// A helper class that manages the "guiTickTime" COs, that drive updates of the
+// GUI from the VsyncThread at the user's configured FPS (possibly downsampled).
 class GuiTick : public QObject {
     Q_OBJECT
   public:
     GuiTick(QObject* pParent = NULL);
-    ~GuiTick();
     void process();
 
   private:
-    ControlObject* m_pCOGuiTickTime;
-    ControlObject* m_pCOGuiTick50ms;
+    std::unique_ptr<ControlObject> m_pCOGuiTickTime;
+    std::unique_ptr<ControlObject> m_pCOGuiTick50ms;
     PerformanceTimer m_cpuTimer;
     mixxx::Duration m_lastUpdateTime;
     mixxx::Duration m_cpuTimeLastTick;

src/widget/knobeventhandler.h

@@ -43,7 +43,7 @@ class KnobEventHandler {
             QCursor::setPos(m_startPos);
             double value = valueFromMouseEvent(pWidget, e);
             pWidget->setControlParameterDown(value);
-            pWidget->update();
+            pWidget->inputActivity();
         }
     }
 
@@ -72,15 +72,14 @@ class KnobEventHandler {
                 QApplication::restoreOverrideCursor();
                 value = valueFromMouseEvent(pWidget, e);
                 pWidget->setControlParameterUp(value);
-                pWidget->update();
+                pWidget->inputActivity();
                 break;
             case Qt::RightButton:
                 m_bRightButtonPressed = false;
                 break;
             default:
                 break;
         }
-        pWidget->update();
     }
 
     void wheelEvent(T* pWidget, QWheelEvent* e) {
@@ -92,7 +91,7 @@ class KnobEventHandler {
         newValue = math_clamp(newValue, 0.0, 1.0);
 
         pWidget->setControlParameter(newValue);
-        pWidget->update();
+        pWidget->inputActivity();
         e->accept();
     }
 

src/widget/slidereventhandler.h

@@ -66,7 +66,7 @@ class SliderEventHandler {
             }
 
             // Update display
-            pWidget->update();
+            pWidget->inputActivity();
         }
     }
 
@@ -113,7 +113,7 @@ class SliderEventHandler {
 
         pWidget->setControlParameter(newParameter);
         onConnectedControlChanged(pWidget, newParameter);
-        pWidget->update();
+        pWidget->inputActivity();
         e->accept();
     }
 
@@ -145,7 +145,7 @@ class SliderEventHandler {
             // parents.
             if (newPos != m_dPos) {
                 m_dPos = newPos;
-                pWidget->update();
+                pWidget->inputActivity();
             }
         }
     }

src/widget/wknob.cpp

@@ -19,10 +19,15 @@
 #include <QMouseEvent>
 #include <QWheelEvent>
 
+#include "util/duration.h"
 #include "widget/wknob.h"
 
 WKnob::WKnob(QWidget* pParent)
-        : WDisplay(pParent) {
+        : WDisplay(pParent),
+          m_renderTimer(mixxx::Duration::fromMillis(20),
+                        mixxx::Duration::fromSeconds(1)) {
+    connect(&m_renderTimer, SIGNAL(update()),
+            this, SLOT(update()));
 }
 
 void WKnob::mouseMoveEvent(QMouseEvent* e) {
@@ -40,3 +45,7 @@ void WKnob::mouseReleaseEvent(QMouseEvent* e) {
 void WKnob::wheelEvent(QWheelEvent* e) {
     m_handler.wheelEvent(this, e);
 }
+
+void WKnob::inputActivity() {
+    m_renderTimer.activity();
+}

src/widget/wknob.h

@@ -7,6 +7,7 @@
 #include <QMouseEvent>
 #include <QWheelEvent>
 
+#include "util/widgetrendertimer.h"
 #include "widget/wdisplay.h"
 #include "widget/knobeventhandler.h"
 
@@ -24,8 +25,11 @@ class WKnob : public WDisplay {
     void mouseMoveEvent(QMouseEvent *e) override;
     void mousePressEvent(QMouseEvent *e) override;
     void mouseReleaseEvent(QMouseEvent *e) override;
+    void inputActivity();
 
   private:
+    WidgetRenderTimer m_renderTimer;
+
     KnobEventHandler<WKnob> m_handler;
     friend class KnobEventHandler<WKnob>;
 };

src/widget/wknobcomposed.cpp

@@ -2,6 +2,7 @@
 #include <QStyleOption>
 #include <QTransform>
 
+#include "util/duration.h"
 #include "widget/wknobcomposed.h"
 
 WKnobComposed::WKnobComposed(QWidget* pParent)
@@ -10,7 +11,11 @@ WKnobComposed::WKnobComposed(QWidget* pParent)
           m_dMinAngle(-230.0),
           m_dMaxAngle(50.0),
           m_dKnobCenterXOffset(0),
-          m_dKnobCenterYOffset(0) {
+          m_dKnobCenterYOffset(0),
+          m_renderTimer(mixxx::Duration::fromMillis(20),
+                        mixxx::Duration::fromSeconds(1)) {
+    connect(&m_renderTimer, SIGNAL(update()),
+            this, SLOT(update()));
 }
 
 void WKnobComposed::setup(const QDomNode& node, const SkinContext& context) {
@@ -79,7 +84,7 @@ void WKnobComposed::onConnectedControlChanged(double dParameter, double dValue)
     // angle range? Right now it's just 1/100th of a degree.
     if (fabs(angle - m_dCurrentAngle) > 0.01) {
         // paintEvent updates m_dCurrentAngle
-        update();
+        inputActivity();
     }
 }
 
@@ -130,3 +135,7 @@ void WKnobComposed::mouseReleaseEvent(QMouseEvent* e) {
 void WKnobComposed::wheelEvent(QWheelEvent* e) {
     m_handler.wheelEvent(this, e);
 }
+
+void WKnobComposed::inputActivity() {
+    m_renderTimer.activity();
+}