Reduce and clean up makeThreadLocalContext calls

Move the calls to explicitly make the OpenGL context current (if there
is one at all) to be within a beginFrame-endFrame block whenever
possible.

Once a frame is being recorded, makeThreadLocalNativeContextCurrent
means (*) calling context->makeCurrent with the target window. Outside,
a QOffscreenSurface is used because no particular window is targeted at
that point from QRhi's perspective.

(*) this will no longer be true once the OpenGL backend of QRhi in
qtbase improves its logic and avoids changing the surface when not
necessary. Regardless, the cleanups here are just as applicable.

Within both render loops there are places where we do a
makeThreadLocalNativeContextCurrent unnecessarily early, before
calling beginFrame. This serves no practical purpose, but on the other
hand it gives calls like the following on the QOpenGLContext:

makeCurrent(offscreen_surface) // why is this not window already?
makeCurrent(window)
swapBuffers(window)

The purpose of this all is to ensure that applications' direct OpenGL
rendering connected to a QQuickWindow signal or issued during an
updatePaintNode have a context current on the thread. This is what Qt
5 guaranteed and Qt 6 must continue to uphold the contract, even if in
Qt 6 all the context and OpenGL stuff is hidden behind an additional
layer, and a QRhi API call gives no guarantees what it does regarding
the current context and what will be current when it returns. Hence
the need for an explicit semi-internal QRhi API call. However, this
has to follow the semantics from other aspects of the API, and so
cannot assume that it is valid to do makeCurrent with a window outside
a begin/endFrame - it does not even know the window before a
beginFrame is issued with a swapchain (which in turn targets a
window).

User code with direct OpenGL usage is invoked at earliest in the
syncSceneGraph() call, and that is of course inside a
begin/endFrame. So doing a makeCurrent(offscreen_surface) early on has
no purpose at all, it is rather an artifact of the original RHI port
of Qt Quick and the Qt 5 structure of the code. So we can safely just
move the makeThreadLocalNativeContextCurrent() calls a bit closer to
the sync.

In addition, this allows us to clean up some leftovers from the original
6.0 rhi port. (variables like 'current' are the legacy of old OpenGL
code current = makeCurrent() and is not quite applicable the same way,
massage these as appropriate)

Pick-to: 6.2
Change-Id: I62e0e0c211650b69ba1228058f9b3e182e968a9d
Reviewed-by: Andy Nichols <andy.nichols@qt.io>

1 files changed, 15 insertions, 21 deletions

diff --git a/src/quick/scenegraph/qsgthreadedrenderloop.cpp b/src/quick/scenegraph/qsgthreadedrenderloop.cpp
index f6df6566aa..07ce4806f1 100644
--- a/src/quick/scenegraph/qsgthreadedrenderloop.cpp
+++ b/src/quick/scenegraph/qsgthreadedrenderloop.cpp
@@ -446,11 +446,11 @@ bool QSGRenderThread::event(QEvent *e)
         if (ce->window) {
             if (rhi) {
                 QQuickWindowPrivate *cd = QQuickWindowPrivate::get(ce->window);
-                cd->rhi->makeThreadLocalNativeContextCurrent();
                 // The assumption is that the swapchain is usable, because on
                 // expose the thread starts up and renders a frame so one cannot
                 // get here without having done at least one on-screen frame.
                 cd->rhi->beginFrame(cd->swapchain);
+                cd->rhi->makeThreadLocalNativeContextCurrent(); // for custom GL rendering before/during/after sync
                 cd->syncSceneGraph();
                 sgrc->endSync();
                 cd->renderSceneGraph(ce->window->size());
@@ -513,17 +513,10 @@ void QSGRenderThread::invalidateGraphics(QQuickWindow *window, bool inDestructor
         return;
     }
 
-
     bool wipeSG = inDestructor || !window->isPersistentSceneGraph();
     bool wipeGraphics = inDestructor || (wipeSG && !window->isPersistentGraphics());
 
-    bool current = true;
-    if (rhi)
-        rhi->makeThreadLocalNativeContextCurrent();
-
-    if (Q_UNLIKELY(!current)) {
-        qCDebug(QSG_LOG_RENDERLOOP, QSG_RT_PAD, "- cleanup without an OpenGL context");
-    }
+    rhi->makeThreadLocalNativeContextCurrent();
 
     QQuickWindowPrivate *dd = QQuickWindowPrivate::get(window);
 
@@ -577,7 +570,7 @@ void QSGRenderThread::sync(bool inExpose)
 
     Q_ASSERT_X(wm->m_lockedForSync, "QSGRenderThread::sync()", "sync triggered on bad terms as gui is not already locked...");
 
-    bool current = true;
+    bool canSync = true;
     if (rhi) {
         if (windowSize.width() > 0 && windowSize.height() > 0) {
             // With the rhi making the (OpenGL) context current serves only one
@@ -589,12 +582,12 @@ void QSGRenderThread::sync(bool inExpose)
         } else {
             // Zero size windows do not initialize a swapchain and
             // rendercontext. So no sync or render can be done then.
-            current = false;
+            canSync = false;
         }
     } else {
-        current = false;
+        canSync = false;
     }
-    if (current) {
+    if (canSync) {
         QQuickWindowPrivate *d = QQuickWindowPrivate::get(window);
         bool hadRenderer = d->renderer != nullptr;
         // If the scene graph was touched since the last sync() make sure it sends the
@@ -777,16 +770,15 @@ void QSGRenderThread::syncAndRender()
         d->animationController->unlock();
     }
 
-    bool current = true;
     // Zero size windows do not initialize a swapchain and
     // rendercontext. So no sync or render can be done then.
-    if (d->renderer && windowSize.width() > 0 && windowSize.height() > 0 && rhi)
-        rhi->makeThreadLocalNativeContextCurrent();
-    else
-        current = false;
+    const bool canRender = d->renderer && cd->swapchain && windowSize.width() > 0 && windowSize.height() > 0;
+
+    if (canRender) {
+        if (!syncRequested) // else this was already done in sync()
+            rhi->makeThreadLocalNativeContextCurrent();
 
-    if (current) {
-        d->renderSceneGraph(windowSize, rhi ? cd->swapchain->currentPixelSize() : QSize());
+        d->renderSceneGraph(windowSize, cd->swapchain->currentPixelSize());
 
         if (profileFrames)
             renderTime = threadTimer.nsecsElapsed();
@@ -824,7 +816,7 @@ void QSGRenderThread::syncAndRender()
 
     // beforeFrameBegin - afterFrameEnd must always come in pairs; if there was
     // no before due to 0 size then there shouldn't be an after either
-   if (current)
+   if (canRender)
         emit window->afterFrameEnd();
 
     // Though it would be more correct to put this block directly after
@@ -920,6 +912,8 @@ void QSGRenderThread::ensureRhi()
         }
     }
     if (!sgrc->rhi() && windowSize.width() > 0 && windowSize.height() > 0) {
+        // We need to guarantee that sceneGraphInitialized is emitted
+        // with a context current, if running with OpenGL.
         rhi->makeThreadLocalNativeContextCurrent();
         QSGDefaultRenderContext::InitParams rcParams;
         rcParams.rhi = rhi;