feat: controller screen renderer #11407

acolombier · 2023-03-26T14:56:24Z

Outdated info

Disclaimer: first time doing QML, so feel free to raise better ways of doing things with it!

This PR is a draft aiming to bring wide support for controllers using onboard screens with delegated rendering, such as the NI Kontrol S4 MK3. However, my effort is to make something as agnostic as possible so it could support other vendors/devices using the same concept.~

Currently, it is based on QT6/QML version. Everyone on Linux can test it locally without a controller using the provided tools/dummy_hid_device.cpp (Instructions in tools/README.md). Here is a demo using that daemon:

There is a fundamental issues which I am unable to solve, so I thought I will create a draft to hopefully pick your brain on them and get some feedback on how this can be resolved: The QML setup relies on QML_SINGLETON which only works in a single QML Engine setup, but this feature requires to use multiple QML engines in multiple threads. We could probably make it work within the same engine as the UI, but I am not sure of the impact it would have on the main UI responsiveness.

Remaining tasks:

Easy screen debugging (command args, XML attribute or dlgpref checkbox)
Screen debugging stability - currently still yield random SEGV - I believe it due to the fact that it using GUI QPainter in a non GUI thread, but if anyone has an other suggestion, it would be greatly appreciated!
Thread safety/memory optimization - couple of deep-copy currently to ensure mutual exclusion but this needs better management
Old UI compatibility - currently waiting on @m0dB 's work to be merged as well as their changes on the CMake to allow Qt6 builds without QML interface.
Unit test for controller definition parsing
Behavior when using the screen preview in the preference pannel

Depends on #12139

JoergAtGithub

Please have a look at the at the CI build fails of the QT6 Linux build.

src/controllers/controllermappinginfo.h

src/controllers/rendering/controllerrenderingengine.cpp

src/controllers/rendering/controllerrenderingtransform.cpp

src/qml/qmlplayermanagerproxy.cpp

acolombier · 2023-03-26T19:15:01Z

Please have a look at the at the CI build fails of the QT6 Linux build.

Looks like I will need to add further dependencies. I also haven't yet built it locally with clazy to tackle all my bad coding practices.

src/controllers/rendering/controllerrenderingtransform.cpp

JoergAtGithub · 2023-03-26T20:02:00Z

Please have a look at the at the CI build fails of the QT6 Linux build.

Looks like I will need to add further dependencies. I also haven't yet built it locally with clazy to tackle all my bad coding practices.

I don't think so. Mixxx is written in C++20 language. In this version of the C++ standard a language feature seems to be removed, which you used here: https://github.com/mixxxdj/mixxx/actions/runs/4525190619/jobs/7969519692?pr=11407#step:21:459

I guess this might be the solution: https://stackoverflow.com/questions/54060011/deprecated-lambda-capture-in-c20

acolombier · 2023-03-26T21:23:22Z

Please have a look at the at the CI build fails of the QT6 Linux build.

Looks like I will need to add further dependencies. I also haven't yet built it locally with clazy to tackle all my bad coding practices.

I don't think so. Mixxx is written in C++20 language. In this version of the C++ standard a language feature seems to be removed, which you used here: https://github.com/mixxxdj/mixxx/actions/runs/4525190619/jobs/7969519692?pr=11407#step:21:459

I guess this might be the solution: https://stackoverflow.com/questions/54060011/deprecated-lambda-capture-in-c20

I was referring to that: I had to install additional headers and dev libs locally.

The lambda capture is part of "my bad coding practices" :)

github-actions · 2023-07-15T00:07:59Z

This PR is marked as stale because it has been open 90 days with no activity.

vollzeitaffe · 2023-09-16T11:25:16Z

@acolombier Are you still working on this? :)
I really appreciate your work for the s4 mk3 and hope we can improve it more and more

acolombier · 2023-09-16T15:44:17Z

Hi @vollzeitaffe, and thanks for the feedback. Sadly, I didn't have much time to make some progress on it lately, but I am hoping to restart working on this soon.

.gitignore

daschuer · 2023-09-25T08:44:25Z

We have now:

 [ 15%] Running moc --collect-json for target mixxx-lib
Error opening /home/runner/work/mixxx/mixxx/build/mixxx-lib_autogen/include/../KAL6L63ZK7/moc_waveformrendermark.cpp.json for reading

acolombier · 2023-09-25T08:51:42Z

We have now:

 [ 15%] Running moc --collect-json for target mixxx-lib
Error opening /home/runner/work/mixxx/mixxx/build/mixxx-lib_autogen/include/../KAL6L63ZK7/moc_waveformrendermark.cpp.json for reading

Yes, that's expected :)

This PR is currently in a completely broken state - I've been scratching my head to get Mixxx to build with the old interface but yet build the QML library and include Mixxx.* modules so these can be used on the controller screen - the most import one is the waveform.

Please if you have any suggestion on how to make this happen, I'm all ears! My experience with CMake is near zero so... I have also an active thread on Zulip if you want to keep the debugging chat off the PR!

JoergAtGithub · 2023-09-25T17:49:38Z

I can compile this without warnings or errors, if I do a Debug build (Windows CMake configuration x64_off). If I use the normal x64_legacy configuration, I get the also the Error opening moc_waveformrendermark.cpp.json for reading as on CI.

acolombier · 2023-09-26T22:04:12Z

Some good progress with the latest changes:

Using OpenGL to perform pixel format
Using the new QML components and proxy with the old UI

Remaining tasks:

Cleanup the implementation from a big hack PoC to something actually maintainable
Stability (better error handling, testing, GC/tear-down)

Bonus, a little teaser:

output.1.mp4

acolombier · 2023-09-30T17:32:53Z

Here is the latest demo with partial screen update support

mixxx_screens.mp4

src/controllers/dlgprefcontroller.cpp

src/controllers/legacycontrollermappingfilehandler.cpp

Swiftb0y · 2024-05-24T11:27:07Z

src/controllers/rendering/controllerrenderingengine.cpp

+ControllerRenderingEngine::~ControllerRenderingEngine() {
+    DEBUG_ASSERT_THIS_QOBJECT_THREAD_ANTI_AFFINITY();
+    m_pThread->wait();
+    VERIFY_OR_DEBUG_ASSERT(!m_fbo) {
+        kLogger.critical() << "The ControllerEngine is being deleted but hasn't been "
+                              "cleaned up. Brace for impact";
+    };
+}


The parent dtor is run after this dtor, this implies that the QObject managing the thread, outlives the thread (since once QThread::wait return once the thread is dead). While probably fine in practice, its definitely not safe. I think we're using QThread wrong. From what I can tell you're supposed to inherit from QThread and then reimplement QThread::run.

This pattern is already followed at other places in Mixxx. Inheriting is one option, but I don't think it is meant to be the only way.

can you point me to them? I want to analyze those for safety as well. I'm not saying that it wouldn't work (I mean it does, you've done extensive testing), but its probably not the right way. It's probably okay-ish to wait in the destructor for a thread to finish, but likely not if the object is living in that thread. After all, that seems the whole reason the QThread itself is not running in the thread it is managing (in the re-implementation case).
https://www.kdab.com/wp-content/uploads/stories/multithreading-with-qt-1.pdf
https://doc.qt.io/qt-6/threads-technologies.html

ControllerManager for example, following exactly the same design, with thread join in dtor. I believe you can find other as well in the analyser components

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

Swiftb0y · 2024-05-24T12:59:50Z

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

+    auto splashOffDeadline = Clock::now() + maxSplashOffDuration;
+    while (splashOffDeadline > Clock::now()) {
+        QCoreApplication::processEvents(QEventLoop::WaitForMoreEvents,
+                std::chrono::duration_cast<std::chrono::milliseconds>(
+                        splashOffDeadline - Clock::now())
+                        .count());
+    }
+
+    m_rootItems.clear();
+    for (const auto& pScreen : std::as_const(m_renderingScreens)) {
+        // When stopping, the rendering engine emits an event which triggers the
+        // shutdown in case it was initiated following a rendering issue. We
+        // need to disconnect first before stopping.
+        pScreen->disconnect(this);
+        VERIFY_OR_DEBUG_ASSERT(!pScreen->isValid() ||
+                !pScreen->isRunning() || pScreen->stop()) {
+            qCWarning(m_logger) << "Unable to stop the screen";
+        };
    }


tbh. I feel like this is not really worth the effort/complexity. If the engine was requested to shutdown, it should shutdown as fast as possible. wdyt think of removing the splash-off feature for now and we can think about how to put it back later in a less-invasive way.

No, splash off is a hard requirement. Without this, your screen remains frozen with the last frame, before the controller stops.

yeah, I'm aware of that... Well this probably is not unique to the screen if I think about it. I mean for the controller we just call shutdown and then immediately continue tearing down the engine, thats not ideal as well... I'll need to think about it.

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

Swiftb0y · 2024-05-24T13:31:05Z

src/test/helpers/log_test.h

+#define SETUP_LOG_CAPTURE() \
+    qInstallMessageHandler(logCapture)
+
+#define ASSERT_ALL_EXPECTED_MSG()                                                                  \
+    if (!logMessagesExpected.isEmpty()) {                                                          \
+        QString errMsg;                                                                            \
+        QDebug strm(&errMsg);                                                                      \
+        strm << logMessagesExpected.size() << "expected log messages didn't occur: \n";            \
+        for (const auto& msg : std::as_const(logMessagesExpected))                                 \
+            strm << "\t" << std::get<QtMsgType>(msg) << std::get<QRegularExpression>(msg) << "\n"; \
+        FAIL() << errMsg.toStdString();                                                            \
+    } else                                                                                         \
+        logMessagesExpected.clear();
+
+#define EXPECT_LOG_MSG(type, exp) \
+    logMessagesExpected.push_back(std::make_tuple(type, QRegularExpression(exp)))
+
+extern QList<std::tuple<QtMsgType, QRegularExpression>> logMessagesExpected;
+void logCapture(QtMsgType msgType, const QMessageLogContext&, const QString& msg);


this is not a good testing strategy IMO (unless you convince me otherwise; I don't have a giant amount of testing experience). From what I can tell, this is mainly used for inspecting the failure handling during parsing. So this is really a codesmell that indicates the sub-par parsing logic. I think we can postpone that for now, but I feel like a proper way of parsing would involve inspecting the generated ScreenInfo struct and checking all the fields to contain the "noValue-value" (so kMaxMsaa for example, or whatever value is the default, or wrap each member in an an optional (we don't have 1000 ScreenInfos, so I don't think the wasted space due to alignment is an issue).

Since logging an error message is the way to inform the user that there was a problem parsing the mapping, yes it is a good strategy IMO.
Please note that the ScreenInfo is already being tested as well, thanks to the addScreenInfo mock

Since logging an error message is the way to inform the user that there was a problem parsing the mapping

Right, but what are we actually testing here, we testing whether the exact wording of the log message is what we expect. The log message is subject to change, so that can get annoying (changing a single string causing dozens of tests to fail). Moreover, intercepting the logmessage is doing mostly just testing logger.
Asserting the logmessages is like testing UI by asserting the pixels have the right color IMO.

Please note that the ScreenInfo is already being tested as well, thanks to the addScreenInfo mock

Ah, I missed that. That is totally sufficient IMO.

If you want to we can keep the logging for now, but I doubt it'll stay around for long.

we testing whether the exact wording of the log message is what we expect

That's not quite right. We are making sure that a message is delivered to the user. Using QRegularExpression, we make sure a specific pattern is present. This way, if during some refactoring the message is somehow not displayed anymore, we failed the test.
In the future, it could be that the log message is replaced by a dialog messages displayed to the user. These test cases will make sure that when this happen, the developer should take care of asserting the message still get delivered to the user. (not in the log this time obviously)

Asserting the logmessages is like testing UI by asserting the pixels have the right color IMO.

That's not true. I'm not testing that an exact array of byte is being written to stdout, I'm checking that a specific text pattern with important information to provide to the user is present in an expected receiver (warning logs). This is similar to what you would do with testing framework like Selenium/Cypress/Playwright, where you would make sure a certain component contains a specific message that the user must be communicated with: you wouldn't test pixels have the right color, but you would select a component and ensure its state or content is matching an expected state

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

acolombier · 2024-05-24T16:22:50Z

I'm now at a stage where navigating through this PR has become quite complicated due to the number of messages and conversations.
How far do we think we are before this is considered ready?

ywwg · 2024-05-24T19:09:52Z

we have a bad habit in mixxx of failing to merge large PRs. We need to maintain stability and quality but also we need to get big new features in. I think we should focus on the core design / correctness issues and be more lax on style / codesmell issues for this first merge. @Swiftb0y I think it is reasonable to ask you to confirm that you've made all the notes you plan to make rather than do successive iterations of further notes. At the end of the day "lgtm" means "it looks good" not "this is the best code ever written." There will be bugs and regressions and perhaps even crashes, but we can find them. We don't want to lose another large, important feature in mixxx because the PR could never get merged.

Let's use the resolve-conversation feature to reduce the clutter and get down to a list of specific notes.

Swiftb0y · 2024-05-24T19:54:08Z

How far do we think we are before this is considered ready?

Not very much to go actually. The last review was the first one where I actually got through everything. I think this is in a decent state already in terms of features. I can try doing a little more cleanup in future refactoring PRs (these are kinda my thing ;) ). Lets work through the the comments from my last review, then I'll do another pass making sure I didn't miss anything obvious and then we can merge.

acolombier · 2024-05-25T16:28:25Z

I can try doing a little more cleanup in future refactoring PRs

This would be very appreciated. I like the fact we could use the original PR's conversation to keep track of it as it would help sharing the knowledge, either of things I didn't know about C++ or some untold Mixxx guidelines. In the meantime, it would appreciated to get some help on these cleanup tasks that sometime get pretty frustrating.

ywwg · 2024-05-28T13:32:49Z

I am a strong believer in separating cleanup / refactor PRs from feature PRs. The best PR is one that says "refactoring / cleanup, no change in behavior". And then all the behavior change can be made against clean code. It is not always possible to do it this way, but it's a great pattern

ywwg · 2024-05-28T18:31:23Z

cleanup / refactor PRs from feature PRs

specifically, I mean if the code author is planning to do refactors in order to support a feature. That is a separate issue from cleanups suggested by reviewers along the way, which are welcome but we don't want to push too hard on.

acolombier · 2024-05-30T18:22:54Z

FYI, some conflicts have developed, but due to the size of the PR, I think it is likely some more appears if this doesn't get merged quickly after review, so I will resolve them once we are ready-ish to merge if that's okay with everyone.

Swiftb0y · 2024-05-30T19:31:52Z

Ignoring all the concessions I made during the previous review rounds, I consider this merge-able. I'd just really like to do a smoke test first but my build env is currently borked so I can't really. Could you give this a smoke test @ywwg (building and ensuring at least the --controller-preview-screens works)?

ywwg

tested on my machine, works!

JoergAtGithub

I didn't review the code, just cross read it. The general architecture looks good and if the code reviewers agree, this can be merged from my point of view!

Swiftb0y · 2024-05-31T20:37:57Z

great. ?

…een-render

acolombier · 2024-05-31T20:42:20Z

I fixed the conflict.

acolombier · 2024-06-03T10:27:36Z

I believe I should have fixed the build now, hopefully this is ready to merge now?

fwcd · 2024-06-06T13:34:40Z

.github/workflows/build.yml

@@ -47,6 +47,7 @@ jobs:
              -DMACOS_BUNDLE=ON
              -DMODPLUG=ON
              -DQT6=ON
+              -DQML=OFF


Hm, this PR seems to have broken the build with QML on macOS (which is the default) and since we've disabled it in CI, some issues seem to have flown under the radar here... I'll have a quick look at how easy that would be to fix

JoergAtGithub reviewed Mar 26, 2023

View reviewed changes

src/controllers/controllermappinginfo.h Outdated Show resolved Hide resolved

src/controllers/rendering/controllerrenderingengine.cpp Outdated Show resolved Hide resolved

acolombier commented Mar 26, 2023

View reviewed changes

src/controllers/rendering/controllerrenderingtransform.cpp Outdated Show resolved Hide resolved

acolombier commented Mar 26, 2023

View reviewed changes

src/qml/qmlplayermanagerproxy.cpp Outdated Show resolved Hide resolved

JoergAtGithub reviewed Mar 26, 2023

View reviewed changes

src/controllers/rendering/controllerrenderingtransform.cpp Outdated Show resolved Hide resolved

jclaveau mentioned this pull request May 23, 2023

Implement a Mixxx controller for the jog-display jclaveau/cdj-retrofit#11

Open

8 tasks

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jul 15, 2023

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Aug 21, 2023

acolombier force-pushed the feat/controller-screen-render branch from 5cffedb to 76375be Compare September 24, 2023 16:53

github-actions bot added controller mappings build labels Sep 24, 2023

daschuer reviewed Sep 25, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

github-actions bot added the ui label Sep 25, 2023

acolombier force-pushed the feat/controller-screen-render branch from 95e4678 to 92690b5 Compare September 25, 2023 21:14

acolombier force-pushed the feat/controller-screen-render branch from 5bb13d1 to 4420f64 Compare September 29, 2023 18:22

github-actions bot removed the ui label Sep 29, 2023

acolombier force-pushed the feat/controller-screen-render branch from 43052e8 to 61b365c Compare October 1, 2023 08:57

github-actions bot added the ui label Oct 1, 2023

acolombier force-pushed the feat/controller-screen-render branch 2 times, most recently from d58f0eb to 89b1a3e Compare October 1, 2023 15:53

Don't use callFunctionOnObjects for QML

5861649

acolombier force-pushed the feat/controller-screen-render branch from dcc9007 to 5861649 Compare May 23, 2024 21:15

acolombier requested review from Swiftb0y and ywwg May 23, 2024 21:15

Swiftb0y reviewed May 24, 2024

View reviewed changes

acolombier and others added 2 commits May 24, 2024 17:11

Nits

b63577c

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Comments and co

cb9a5c2

Use stack allocation for QML component

6197a07

acolombier force-pushed the feat/controller-screen-render branch from 4fb9d22 to 6197a07 Compare May 25, 2024 17:19

ywwg approved these changes May 31, 2024

View reviewed changes

ywwg requested a review from Swiftb0y May 31, 2024 20:07

JoergAtGithub reviewed May 31, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/controller-scr…

df39da0

…een-render

github-actions bot added the controller mappings label May 31, 2024

Swiftb0y approved these changes May 31, 2024

View reviewed changes

Use QStringLiteral on ScopedTimer

d7ce6d8

ywwg merged commit 9b33757 into mixxxdj:main Jun 3, 2024
13 checks passed

fwcd reviewed Jun 6, 2024

View reviewed changes

fwcd mentioned this pull request Jun 6, 2024

Fix QML build regressions on macOS #13334

Merged

feat: controller screen renderer #11407

feat: controller screen renderer #11407

Conversation

acolombier commented Mar 26, 2023 • edited

JoergAtGithub left a comment

Choose a reason for hiding this comment

acolombier commented Mar 26, 2023

JoergAtGithub commented Mar 26, 2023

acolombier commented Mar 26, 2023 • edited

github-actions bot commented Jul 15, 2023

vollzeitaffe commented Sep 16, 2023 • edited

acolombier commented Sep 16, 2023

daschuer commented Sep 25, 2023

acolombier commented Sep 25, 2023

JoergAtGithub commented Sep 25, 2023

acolombier commented Sep 26, 2023

acolombier commented Sep 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y May 24, 2024 • edited

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

acolombier commented May 24, 2024

ywwg commented May 24, 2024

Swiftb0y commented May 24, 2024 • edited

acolombier commented May 25, 2024

ywwg commented May 28, 2024

ywwg commented May 28, 2024

acolombier commented May 30, 2024

Swiftb0y commented May 30, 2024

ywwg left a comment • edited

Choose a reason for hiding this comment

JoergAtGithub left a comment

Choose a reason for hiding this comment

Swiftb0y commented May 31, 2024

acolombier commented May 31, 2024

acolombier commented Jun 3, 2024

Choose a reason for hiding this comment

acolombier commented Mar 26, 2023 •

edited

acolombier commented Mar 26, 2023 •

edited

vollzeitaffe commented Sep 16, 2023 •

edited

Swiftb0y May 24, 2024 •

edited

Swiftb0y commented May 24, 2024 •

edited

ywwg left a comment •

edited