Re-add ability to switch render modes under Qt 6 #917

Cuperino · 2024-01-12T18:57:20Z

Fixes #880, where QSG_VISUALIZE overdraw, and other modes, were not working under Plasma. It does this by re-enabling RenderModeRequest::apply() when using the OpenGL variants or the Software renderer. Other modes, such as Vulkan, are yet to be implemented.

Testing was done under Plasma Wayland, by combining:

setting QSG_RHI_BACKEND to opengl, vulkan, and software
setting QSG_RHI_PREFER_SOFTWARE_RENDERER to 0 and 1

Cuperino · 2024-01-15T01:39:59Z

quickinspector test is failing on Windows because of the following reasons:

The overlay that states "DirectX is not supported yet, please use OpenGL or Software backend" dims color values, making color compare fail. A solution for this is to make this overlay a little smaller, so we can get a glimpse of the original colors from the corners we pick colors from in this test.
The output image's canvas is scaled according to display scaling, but the contents of the image itself remain at its original, non-scaled, resolution. There's a slim chance, that I roughly estimate to approximate 1/100, that the image is scaled to fit the scaled canvas, and the test passes. Most of the time however, the test will fail with display scaling on.

Qt 5 out: note resolution of 232x100, with the image staying at 100x00

Qt 6 out: note resolution of 232x200, and how the image stays at 100x00

By solving the first problem, the one with the presence of an overlay, we get the following image:

For the test to pass on a system with display scaling, the image should instead produce this output that only happens rarely:

Cuperino · 2024-01-15T01:52:39Z

From inspecting quickscreengrabber.cpp:810, I suspect this could potentially be a bug with the grabWindow() function in Qt 6.

Edit: It could also be a race condition between the image signaling being captured and it being scaled by Qt.

Waqar144 · 2024-01-15T06:02:36Z

A solution for this is to make this overlay a little smaller, so we can get a glimpse of the original colors

Yeah, but we don't support DirectX/Vulkan so making the test pass is meaningless and misguiding. I would rather skip it.

Cuperino · 2024-01-15T07:34:06Z

A solution for this is to make this overlay a little smaller, so we can get a glimpse of the original colors

Yeah, but we don't support DirectX/Vulkan so making the test pass is meaningless and misguiding. I would rather skip it.

Here the image behind the overlay wasn't loading at the correct scale, so I would leave the scale fix and remove the change to the overlay.

See:

0e9cfa3
grabbedFrame.image is sized according to display scaling, but
the contents of the image itself remain at their original,
non-scaled, resolution. There's a slim chance that the image is
scaled to fit the scaled canvas, and the quickinspectortest
passes. Most of the times, however, the image won't be scaled
to fit the canvas.

In this change we manually scale the image according to
devicePixelRatio to fit the image's canvas.

Alternatively, we could leave both, and use the test to catch when the image from the window grab loads with 4x scale instead of 2x. In other words, we could use the test to find out when the bug with the scaling factor and window.grabImage gets fixed in a future Qt version.

Right now the only test failing appears to be quickinspectortest when OpenGL is used Windows.
@Waqar144, Is OpenGL use supported on Windows?

Waqar144 · 2024-01-15T10:32:41Z

@Waqar144, Is OpenGL use supported on Windows?

Afaik, yes. It should work on windows

milianw · 2025-01-03T12:17:38Z

plugins/quickinspector/quickscreengrabber.cpp

@@ -806,8 +806,14 @@ UnsupportedScreenGrabber::~UnsupportedScreenGrabber()

 void UnsupportedScreenGrabber::requestGrabWindow(const QRectF & /*userViewport*/)
 {
+    const qreal ratio = m_window->effectiveDevicePixelRatio();


I don't think this is correct, we want to transfer the original image, with the proper scaling.

this patch is bogus, can you explain what test is failing, where (i.e. with what DPR values?)

quickinspectortest fails after re-enabling the ability to switch render modes in Qt 6, which is what addresses issue #880. A couple of QCompares in quickinspectortest fail for different reasons. Preconditions to fail depend on the QSG_RHI_BACKEND being used and whether the window being drawn is being up-scaled. Any DPR value greater than 1 triggers the issue. I tested with 1.5 and 2.0.

The first failing QTest is QCOMPARE(QColor(img.pixel(1 * view()->devicePixelRatio(), 1 * view()->devicePixelRatio())), QColor(255, 0, 0));. The pre-condition to fail is to use a QSG_RHI_BACKEND that results in UnsupportedScreenGrabber being used. Any of the recently introduced ones will do. The first compare fails because of the semi-transparent overlay that is drawn over the captured image. The overlay has text stating "$backend is not supported yet, please use the OpenGL (QSG_RHI_BACKEND=opengl) or Software backend (QT_QUICK_BACKEND=software)".

It makes sense for that compare to pass despite this being an unsuported backend, because the UnsupportedScreenGrabber provides a fallback that does, in fact, display the window's contents, it just does so with very poor perfromance, hence why we only render it once.

One of my commits adds margins to the overlay so the compare can evaluate the right colors, allowing it to pass. Waqar proposed to me via chat that we prevent the test from running on unsuported platforms. That's another path we could take. I haven't gone that route yet because my fix reveals another problem with the compare that follows that one.

In UnsupportedScreenGrabber::requestGrabWindow, if windows are instantiated with an upscaling factor other than 1, quickinspectortest will also fail at QCOMPARE(QColor(img.pixel(99 * view()->devicePixelRatio(), 1 * view()->devicePixelRatio())), QColor(0, 255, 0));. It fails because the image captured from the upscaled window isn't upscaled despite setDevicePixelRatio having executed. The expected behavior appears to have changed between Qt 5 and Qt 6, hence my most non-triumphat workaround.

The original implementation correctly sets QImage's devicePixelRatio. If we save that image to disk, we see the canvas has been upscaled, while the contents remain unscaled. That's the kind of output QCompare is working with; we know because it evaluates to white instead of the anticipated green, causing it to fail.

This discrepancy could be addressed in a few ways. I didn't realize this at the time but my solution was indeed bogus, resulting in a double up-scale in GammaRay, outside the context of the test; which means the program works as intended, and the issue appears to lie with the tests.

I've yet to determine the true cause for the lack of content up-scaling. My current suspicion is it may have to do with QTransfrom, because removing QTransform transform = frame.transform(); and img = img.transformed(transform); from testFetchingPreview changes nothing.

The test is only run with QT_QUICK_BACKEND=rhi;QSG_RHI_BACKEND=opengl so anything else is ignored anyways, no? Or do you mean when you run it locally with something else? That's on you then, but not something the CI will find, or?

Please document how you run the test, i.e. the exact command including env vars you set to scale / set the DPI.

That said, the comment hinting at crashes which is removed by that patch is easily reproduced locally after all when I run the test (see following valgrind report). I fear we need to find a way to halt the QSG renderer or the like before we apply the mode or find some other way to cleanup the scene graph 🤷 Maybe this also requires patching Qt upstream?

==74949== Invalid read of size 8 ==74949== at 0x5811EFB: UnknownInlinedFun (qvarlengtharray.h:74) ==74949== by 0x5811EFB: UnknownInlinedFun (qrhigles2.cpp:4055) ==74949== by 0x5811EFB: QRhiGles2::executeCommandBuffer(QRhiCommandBuffer*) (qrhigles2.cpp:3306) ==74949== by 0x5807CB6: QRhiGles2::endFrame(QRhiSwapChain*, QFlags<QRhi::EndFrameFlag>) (qrhigles2.cpp:2163) ==74949== by 0x56A672D: QRhi::endFrame(QRhiSwapChain*, QFlags<QRhi::EndFrameFlag>) (qrhi.cpp:10878) ==74949== by 0x4F85FD9: UnknownInlinedFun (qsgthreadedrenderloop.cpp:771) ==74949== by 0x4F85FD9: QSGRenderThread::run() (qsgthreadedrenderloop.cpp:975) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:354) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:290) ==74949== by 0x67B90CC: QThreadPrivate::start(void*) (qthread_unix.cpp:318) ==74949== by 0x6EE639C: start_thread (pthread_create.c:447) ==74949== by 0x6F6B2A3: clone (clone.S:100) ==74949== Address 0xf916088 is 56 bytes inside a block of size 3,472 free'd ==74949== at 0x48488DD: operator delete(void*, unsigned long) (vg_replace_malloc.c:1181) ==74949== by 0x4DEFBA1: QSGBatchRenderer::Renderer::releaseElement(QSGBatchRenderer::Element*, bool) [clone .part.0] (qsgbatchrenderer.cpp:3648) ==74949== by 0x4DDA553: UnknownInlinedFun (qsgbatchrenderer.cpp:960) ==74949== by 0x4DDA553: QSGBatchRenderer::Renderer::~Renderer() (qsgbatchrenderer.cpp:961) ==74949== by 0x4DDAD74: QSGBatchRenderer::Renderer::~Renderer() (qsgbatchrenderer.cpp:966) ==74949== by 0x4DA993B: QQuickWindow::cleanupSceneGraph() (qquickwindow.cpp:2340) ==74949== by 0x664BA24: QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (qmetaobject.cpp:2758) ==74949== by 0x664BE3D: QMetaObject::invokeMethodImpl(QObject*, char const*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (qmetaobject.cpp:1607) ==74949== by 0x23EB5A63: invokeMethod<void> (qobjectdefs.h:376) ==74949== by 0x23EB5A63: invokeMethod<> (qobjectdefs.h:389) ==74949== by 0x23EB5A63: GammaRay::RenderModeRequest::apply() (quickinspector.cpp:311) ==74949== by 0x6691180: UnknownInlinedFun (qobjectdefs_impl.h:486) ==74949== by 0x6691180: void doActivate<true>(QObject*, int, void**) (qobject.cpp:4124) ==74949== by 0x4D9FA67: UnknownInlinedFun (moc_qquickwindow.cpp:631) ==74949== by 0x4D9FA67: QQuickWindowPrivate::renderSceneGraph() (qquickwindow.cpp:696) ==74949== by 0x4F85FA4: UnknownInlinedFun (qsgthreadedrenderloop.cpp:762) ==74949== by 0x4F85FA4: QSGRenderThread::run() (qsgthreadedrenderloop.cpp:975) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:354) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:290) ==74949== by 0x67B90CC: QThreadPrivate::start(void*) (qthread_unix.cpp:318) ==74949== Block was alloc'd at ==74949== at 0x4844F93: operator new(unsigned long) (vg_replace_malloc.c:487) ==74949== by 0x5801858: QRhiGles2::createShaderResourceBindings() (qrhigles2.cpp:1721) ==74949== by 0x4DDF412: QSGBatchRenderer::Renderer::updateMaterialDynamicData(QSGBatchRenderer::ShaderManagerShader*, QSGMaterialShader::RenderState&, QSGMaterial*, QSGBatchRenderer::Batch const*, QSGBatchRenderer::Element*, int, int, char*) (qsgbatchrenderer.cpp:3105) ==74949== by 0x4DE3EC8: QSGBatchRenderer::Renderer::prepareRenderMergedBatch(QSGBatchRenderer::Batch*, QSGBatchRenderer::Renderer::PreparedRenderBatch*) (qsgbatchrenderer.cpp:3269) ==74949== by 0x4DE6644: QSGBatchRenderer::Renderer::prepareRenderPass(QSGBatchRenderer::Renderer::RenderPassContext*) (qsgbatchrenderer.cpp:3895) ==74949== by 0x4DE8016: UnknownInlinedFun (qsgbatchrenderer.cpp:3685) ==74949== by 0x4DE8016: QSGBatchRenderer::Renderer::render() (qsgbatchrenderer.cpp:3678) ==74949== by 0x4DFAA32: QSGRenderer::renderScene() [clone .part.0] (qsgrenderer.cpp:145) ==74949== by 0x4D9FA54: QQuickWindowPrivate::renderSceneGraph() (qquickwindow.cpp:694) ==74949== by 0x4F85FA4: UnknownInlinedFun (qsgthreadedrenderloop.cpp:762) ==74949== by 0x4F85FA4: QSGRenderThread::run() (qsgthreadedrenderloop.cpp:975) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:354) ==74949== by 0x67B90CC: UnknownInlinedFun (qthread_unix.cpp:290) ==74949== by 0x67B90CC: QThreadPrivate::start(void*) (qthread_unix.cpp:318) ==74949== by 0x6EE639C: start_thread (pthread_create.c:447) ==74949== by 0x6F6B2A3: clone (clone.S:100)

To test I use environment variables, like so:

QT_QUICK_BACKEND=rhi QSG_RHI_BACKEND=vulkan

And run the quickinspectortest target from Qt Creator to perform the test.

To set DPI scaling factor, I set the desired scaling factor on my display before running the test. My displays usually have a scaling factor (currently) set to them, so I haven't used environment variables to scale Qt software in a while.

please paste env | grep QT - esp. whether QT_SCREEN_SCALE_FACTORS or QT_AUTO_SCREEN_SCALE_FACTOR is set and if so to which value.

and again: other RHI backends should not be the main concern for now - let's try to ensure it works with OpenGL first and then take it from there

On Plasma Wayland, QT_AUTO_SCREEN_SCALE_FACTOR=0 is set.
QT_SCREEN_SCALE_FACTORS is not set.

On Plasma X11, QT_AUTO_SCREEN_SCALE_FACTOR=0 is set
and QT_SCREEN_SCALE_FACTORS=eDP-1=1.5;HDMI-1=1.5;DP-1=1.5;DP-2=1.5;DP-3=1.5;DP-4=1.5;DP-1-0=1.5;DP-1-1=1.5;HDMI-1-0=1.5;DP-1-2=1.5;DP-1-3=1.5; is also set. Values listed refer to the internal display (eDP-1), physical HDMI (HDMI-1), and both physical and virtual display ports (all other HDMI and remaining DP).

@milianw, After limiting my changes to only cover OpenGL, and fixing the crash by only messing with the window while the GUI thread is blocked, I got all tests to pass and OpenGL.

I'd say this is ready if it wasn't for the fact that under Wayland (haven't tested X11 yet) the image shown in GammaRay is getting cropped at the top. Any ideas what might cause this? @milianw @Waqar144. See top ~180px missing:

Update: I can't replicate the issue after restarting the program I had injected. The issue must've been a momentary thing, probably unrelated.

milianw

lgtm in principle, great find with the beforeSynchronizing step!

I'll mark as approve, but please make sure to fix the commit history, or squash and take care of a clean git history

milianw · 2025-01-09T15:28:58Z

plugins/quickinspector/quickinspector.cpp

@@ -308,12 +308,11 @@ void RenderModeRequest::apply()
        const QByteArray mode = renderModeToString(RenderModeRequest::mode);
        QQuickWindowPrivate *winPriv = QQuickWindowPrivate::get(window);
 #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
-        QObject::connect(window.get(), &QQuickWindow::beforeSynchronizing, this, [this, winPriv, mode](){
+        QObject::connect(window.get(), &QQuickWindow::beforeSynchronizing, this, [this, winPriv, mode]() {


squash this commit bot patch locally please (and consider setting up pre-commit locally)

Fixes #880, where QSG_VISUALIZE modes were not working. Also fixed race condition crashes around use of OpenGL RHI and scene graph events.

Cuperino added the bug label Jan 12, 2024

Cuperino added this to the 3.1 milestone Jan 12, 2024

Cuperino requested a review from Waqar144 January 12, 2024 18:57

Cuperino self-assigned this Jan 12, 2024

Cuperino marked this pull request as draft January 15, 2024 04:41

Cuperino force-pushed the fix880 branch 2 times, most recently from c52e592 to 1b89069 Compare March 20, 2024 21:06

Cuperino force-pushed the fix880 branch from 1b89069 to eb99e92 Compare April 25, 2024 13:08

Cuperino force-pushed the fix880 branch from eb99e92 to 17ad975 Compare May 2, 2024 21:05

milianw requested changes Jan 3, 2025

View reviewed changes

Cuperino force-pushed the fix880 branch 4 times, most recently from d7e505e to 01fe879 Compare January 4, 2025 06:21

Cuperino requested a review from milianw January 5, 2025 15:10

Cuperino force-pushed the fix880 branch 3 times, most recently from 7573ee6 to 855eec6 Compare January 9, 2025 13:44

milianw approved these changes Jan 9, 2025

View reviewed changes

milianw marked this pull request as ready for review January 9, 2025 15:30

Re-enable ability to switch render modes under Qt 6

836386b

Fixes #880, where QSG_VISUALIZE modes were not working. Also fixed race condition crashes around use of OpenGL RHI and scene graph events.

Cuperino force-pushed the fix880 branch from 3d538a9 to 836386b Compare January 10, 2025 17:32

Cuperino removed the request for review from Waqar144 January 10, 2025 17:32

Cuperino merged commit 9ced559 into master Jan 11, 2025
18 checks passed

Cuperino deleted the fix880 branch January 11, 2025 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add ability to switch render modes under Qt 6 #917

Re-add ability to switch render modes under Qt 6 #917

Cuperino commented Jan 12, 2024

Cuperino commented Jan 15, 2024 •

edited

Loading

Cuperino commented Jan 15, 2024 •

edited

Loading

Waqar144 commented Jan 15, 2024

Cuperino commented Jan 15, 2024

Waqar144 commented Jan 15, 2024

milianw Jan 3, 2025

Cuperino Jan 4, 2025

milianw Jan 8, 2025

Cuperino Jan 8, 2025

milianw Jan 9, 2025

Cuperino Jan 9, 2025

Cuperino Jan 9, 2025 •

edited

Loading

milianw left a comment

milianw Jan 9, 2025

Cuperino Jan 11, 2025

Re-add ability to switch render modes under Qt 6 #917

Re-add ability to switch render modes under Qt 6 #917

Conversation

Cuperino commented Jan 12, 2024

Cuperino commented Jan 15, 2024 • edited Loading

Cuperino commented Jan 15, 2024 • edited Loading

Waqar144 commented Jan 15, 2024

Cuperino commented Jan 15, 2024

Waqar144 commented Jan 15, 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

Cuperino Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

milianw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cuperino commented Jan 15, 2024 •

edited

Loading

Cuperino commented Jan 15, 2024 •

edited

Loading

Cuperino Jan 9, 2025 •

edited

Loading