rhi: Attempt to fix up the logic around beginExternal()

Mainly for Vulkan where it lacked the recording of the still queued
commands. Uncovered by Qt Quick examples that integrate custom Vulkan
rendering.

This still has an issue that needs to be tackled separately. (we probably
will switch to using a dedicated secondary command buffer with
RENDER_PASS_CONTINUE_BIT for the external commands, and then just have
a vkCmdExecuteCommands in our own queue instead of recording everything in
beginExternal).

The possibility of losing glMemoryBarrier() calls due to begin/endExternal()
with the OpenGL backend is fixed too. The logic here mirrors Vulkan
to some extent except that we do not have a concept of (and so the trouble
with) renderpass instances.

Clean up around the implementations of finish() as well. Attempting to share
code via a "flushCommandBuffer" function is admirable but is not worth it
since some semantics are different. (finish() cannot be called within a
begin/endPass, unlike begin/endExternal).

Change-Id: I5137db598d6a40d484e53678f5c919abf750d9ed
Reviewed-by: Andy Nichols <andy.nichols@qt.io>

6 files changed, 102 insertions, 67 deletions

diff --git a/src/gui/rhi/qrhid3d11.cpp b/src/gui/rhi/qrhid3d11.cpp
index 9b411c5021..e978536dd7 100644
--- a/src/gui/rhi/qrhid3d11.cpp
+++ b/src/gui/rhi/qrhid3d11.cpp
@@ -873,8 +873,10 @@ const QRhiNativeHandles *QRhiD3D11::nativeHandles(QRhiCommandBuffer *cb)
 
 void QRhiD3D11::beginExternal(QRhiCommandBuffer *cb)
 {
-    Q_UNUSED(cb);
-    flushCommandBuffer();
+    QD3D11CommandBuffer *cbD = QRHI_RES(QD3D11CommandBuffer, cb);
+    // no timestampSwapChain, in order to avoid timestamp mess
+    executeCommandBuffer(cbD);
+    cbD->resetCommands();
 }
 
 void QRhiD3D11::endExternal(QRhiCommandBuffer *cb)
@@ -1135,27 +1137,25 @@ static inline bool isDepthTextureFormat(QRhiTexture::Format format)
 
 QRhi::FrameOpResult QRhiD3D11::finish()
 {
-    if (inFrame)
-        flushCommandBuffer();
+    if (inFrame) {
+        if (ofr.active) {
+            Q_ASSERT(!contextState.currentSwapChain);
+            Q_ASSERT(ofr.cbWrapper.recordingPass == QD3D11CommandBuffer::NoPass);
+            executeCommandBuffer(&ofr.cbWrapper);
+            ofr.cbWrapper.resetCommands();
+        } else {
+            Q_ASSERT(contextState.currentSwapChain);
+            Q_ASSERT(contextState.currentSwapChain->cb.recordingPass == QD3D11CommandBuffer::NoPass);
+            executeCommandBuffer(&contextState.currentSwapChain->cb); // no timestampSwapChain, in order to avoid timestamp mess
+            contextState.currentSwapChain->cb.resetCommands();
+        }
+    }
 
     finishActiveReadbacks();
 
     return QRhi::FrameOpSuccess;
 }
 
-void QRhiD3D11::flushCommandBuffer()
-{
-    if (ofr.active) {
-        Q_ASSERT(!contextState.currentSwapChain);
-        executeCommandBuffer(&ofr.cbWrapper);
-        ofr.cbWrapper.resetCommands();
-    } else {
-        Q_ASSERT(contextState.currentSwapChain);
-        executeCommandBuffer(&contextState.currentSwapChain->cb); // no timestampSwapChain, in order to avoid timestamp mess
-        contextState.currentSwapChain->cb.resetCommands();
-    }
-}
-
 void QRhiD3D11::enqueueSubresUpload(QD3D11Texture *texD, QD3D11CommandBuffer *cbD,
                                     int layer, int level, const QRhiTextureSubresourceUploadDescription &subresDesc)
 {
diff --git a/src/gui/rhi/qrhid3d11_p_p.h b/src/gui/rhi/qrhid3d11_p_p.h
index 3ceb055b27..096c192183 100644
--- a/src/gui/rhi/qrhid3d11_p_p.h
+++ b/src/gui/rhi/qrhid3d11_p_p.h
@@ -473,9 +473,9 @@ struct QD3D11CommandBuffer : public QRhiCommandBuffer
         imageRetainPool.clear();
     }
     void resetState() {
-        resetCommands();
         recordingPass = NoPass;
         currentTarget = nullptr;
+        resetCommands();
         resetCachedState();
     }
     void resetCachedState() {
@@ -633,7 +633,6 @@ public:
     void sendVMemStatsToProfiler() override;
     void makeThreadLocalNativeContextCurrent() override;
 
-    void flushCommandBuffer();
     void enqueueSubresUpload(QD3D11Texture *texD, QD3D11CommandBuffer *cbD,
                              int layer, int level, const QRhiTextureSubresourceUploadDescription &subresDesc);
     void enqueueResourceUpdates(QRhiCommandBuffer *cb, QRhiResourceUpdateBatch *resourceUpdates);
diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp
index 26e7fa2759..2cc489ea8b 100644
--- a/src/gui/rhi/qrhigles2.cpp
+++ b/src/gui/rhi/qrhigles2.cpp
@@ -1094,15 +1094,36 @@ const QRhiNativeHandles *QRhiGles2::nativeHandles(QRhiCommandBuffer *cb)
 
 void QRhiGles2::beginExternal(QRhiCommandBuffer *cb)
 {
-    Q_UNUSED(cb);
-    flushCommandBuffer(); // also ensures the context is current
+    if (ofr.active) {
+        Q_ASSERT(!currentSwapChain);
+        if (!ensureContext())
+            return;
+    } else {
+        Q_ASSERT(currentSwapChain);
+        if (!ensureContext(currentSwapChain->surface))
+            return;
+    }
+
+    QGles2CommandBuffer *cbD = QRHI_RES(QGles2CommandBuffer, cb);
+    executeCommandBuffer(cbD);
+    cbD->resetCommands();
 }
 
 void QRhiGles2::endExternal(QRhiCommandBuffer *cb)
 {
     QGles2CommandBuffer *cbD = QRHI_RES(QGles2CommandBuffer, cb);
-    Q_ASSERT(cbD->commands.isEmpty());
+    Q_ASSERT(cbD->commands.isEmpty() && cbD->currentPassResTrackerIndex == -1);
+
     cbD->resetCachedState();
+
+    if (cbD->recordingPass != QGles2CommandBuffer::NoPass) {
+        // Commands that come after this point need a resource tracker and also
+        // a BarriersForPass command enqueued. (the ones we had from
+        // beginPass() are now gone since beginExternal() processed all that
+        // due to calling executeCommandBuffer()).
+        enqueueBarriersForPass(cbD);
+    }
+
     if (cbD->currentTarget)
         enqueueBindFramebuffer(cbD->currentTarget, cbD);
 }
@@ -1196,23 +1217,22 @@ QRhi::FrameOpResult QRhiGles2::endOffscreenFrame()
 
 QRhi::FrameOpResult QRhiGles2::finish()
 {
-    return inFrame ? flushCommandBuffer() : QRhi::FrameOpSuccess;
-}
-
-QRhi::FrameOpResult QRhiGles2::flushCommandBuffer()
-{
-    if (ofr.active) {
-        Q_ASSERT(!currentSwapChain);
-        if (!ensureContext())
-            return QRhi::FrameOpError;
-        executeCommandBuffer(&ofr.cbWrapper);
-        ofr.cbWrapper.resetCommands();
-    } else {
-        Q_ASSERT(currentSwapChain);
-        if (!ensureContext(currentSwapChain->surface))
-            return QRhi::FrameOpError;
-        executeCommandBuffer(&currentSwapChain->cb);
-        currentSwapChain->cb.resetCommands();
+    if (inFrame) {
+        if (ofr.active) {
+            Q_ASSERT(!currentSwapChain);
+            Q_ASSERT(ofr.cbWrapper.recordingPass == QGles2CommandBuffer::NoPass);
+            if (!ensureContext())
+                return QRhi::FrameOpError;
+            executeCommandBuffer(&ofr.cbWrapper);
+            ofr.cbWrapper.resetCommands();
+        } else {
+            Q_ASSERT(currentSwapChain);
+            Q_ASSERT(currentSwapChain->cb.recordingPass == QGles2CommandBuffer::NoPass);
+            if (!ensureContext(currentSwapChain->surface))
+                return QRhi::FrameOpError;
+            executeCommandBuffer(&currentSwapChain->cb);
+            currentSwapChain->cb.resetCommands();
+        }
     }
     return QRhi::FrameOpSuccess;
 }
@@ -2483,6 +2503,16 @@ QGles2RenderTargetData *QRhiGles2::enqueueBindFramebuffer(QRhiRenderTarget *rt,
     return rtD;
 }
 
+void QRhiGles2::enqueueBarriersForPass(QGles2CommandBuffer *cbD)
+{
+    cbD->passResTrackers.append(QRhiPassResourceTracker());
+    cbD->currentPassResTrackerIndex = cbD->passResTrackers.count() - 1;
+    QGles2CommandBuffer::Command cmd;
+    cmd.cmd = QGles2CommandBuffer::Command::BarriersForPass;
+    cmd.args.barriersForPass.trackerIndex = cbD->currentPassResTrackerIndex;
+    cbD->commands.append(cmd);
+}
+
 void QRhiGles2::beginPass(QRhiCommandBuffer *cb,
                           QRhiRenderTarget *rt,
                           const QColor &colorClearValue,
@@ -2497,12 +2527,7 @@ void QRhiGles2::beginPass(QRhiCommandBuffer *cb,
 
     // Get a new resource tracker. Then add a command that will generate
     // glMemoryBarrier() calls based on that tracker when submitted.
-    cbD->passResTrackers.append(QRhiPassResourceTracker());
-    cbD->currentPassResTrackerIndex = cbD->passResTrackers.count() - 1;
-    QGles2CommandBuffer::Command cmd;
-    cmd.cmd = QGles2CommandBuffer::Command::BarriersForPass;
-    cmd.args.barriersForPass.trackerIndex = cbD->currentPassResTrackerIndex;
-    cbD->commands.append(cmd);
+    enqueueBarriersForPass(cbD);
 
     bool wantsColorClear, wantsDsClear;
     QGles2RenderTargetData *rtD = enqueueBindFramebuffer(rt, cbD, &wantsColorClear, &wantsDsClear);
@@ -2578,12 +2603,7 @@ void QRhiGles2::beginComputePass(QRhiCommandBuffer *cb, QRhiResourceUpdateBatch
     if (resourceUpdates)
         enqueueResourceUpdates(cb, resourceUpdates);
 
-    cbD->passResTrackers.append(QRhiPassResourceTracker());
-    cbD->currentPassResTrackerIndex = cbD->passResTrackers.count() - 1;
-    QGles2CommandBuffer::Command cmd;
-    cmd.cmd = QGles2CommandBuffer::Command::BarriersForPass;
-    cmd.args.barriersForPass.trackerIndex = cbD->currentPassResTrackerIndex;
-    cbD->commands.append(cmd);
+    enqueueBarriersForPass(cbD);
 
     cbD->recordingPass = QGles2CommandBuffer::ComputePass;
 
diff --git a/src/gui/rhi/qrhigles2_p_p.h b/src/gui/rhi/qrhigles2_p_p.h
index 462ce8032c..3602d62122 100644
--- a/src/gui/rhi/qrhigles2_p_p.h
+++ b/src/gui/rhi/qrhigles2_p_p.h
@@ -525,19 +525,16 @@ struct QGles2CommandBuffer : public QRhiCommandBuffer
     }
     void resetCommands() {
         commands.clear();
-        // beginExternal() can lead to calling resetCommands() inside a pass,
-        // hence the condition
-        if (recordingPass == NoPass) {
-            passResTrackers.clear();
-            currentPassResTrackerIndex = -1;
-        }
         dataRetainPool.clear();
         imageRetainPool.clear();
+
+        passResTrackers.clear();
+        currentPassResTrackerIndex = -1;
     }
     void resetState() {
-        resetCommands();
         recordingPass = NoPass;
         currentTarget = nullptr;
+        resetCommands();
         resetCachedState();
     }
     void resetCachedState() {
@@ -671,7 +668,6 @@ public:
 
     bool ensureContext(QSurface *surface = nullptr) const;
     void executeDeferredReleases();
-    QRhi::FrameOpResult flushCommandBuffer();
     void trackedBufferBarrier(QGles2CommandBuffer *cbD, QGles2Buffer *bufD, QGles2Buffer::Access access);
     void trackedImageBarrier(QGles2CommandBuffer *cbD, QGles2Texture *texD, QGles2Texture::Access access);
     void enqueueSubresUpload(QGles2Texture *texD, QGles2CommandBuffer *cbD,
@@ -692,6 +688,7 @@ public:
                              const uint *dynOfsPairs, int dynOfsCount);
     QGles2RenderTargetData *enqueueBindFramebuffer(QRhiRenderTarget *rt, QGles2CommandBuffer *cbD,
                                                    bool *wantsColorClear = nullptr, bool *wantsDsClear = nullptr);
+    void enqueueBarriersForPass(QGles2CommandBuffer *cbD);
     int effectiveSampleCount(int sampleCount) const;
     bool compileShader(GLuint program, const QRhiShaderStage &shaderStage,
                        QShaderDescription *desc, int *glslVersionUsed);
diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp
index a0f32252cf..0381def9f1 100644
--- a/src/gui/rhi/qrhivulkan.cpp
+++ b/src/gui/rhi/qrhivulkan.cpp
@@ -1868,12 +1868,16 @@ QRhi::FrameOpResult QRhiVulkan::finish()
         VkCommandBuffer cb;
         if (ofr.active) {
             Q_ASSERT(!currentSwapChain);
+            Q_ASSERT(ofr.cbWrapper.recordingPass == QVkCommandBuffer::NoPass);
             recordCommandBuffer(&ofr.cbWrapper);
+            ofr.cbWrapper.resetCommands();
             cb = ofr.cbWrapper.cb;
         } else {
             Q_ASSERT(currentSwapChain);
+            Q_ASSERT(currentSwapChain->cbWrapper.recordingPass == QVkCommandBuffer::NoPass);
             swapChainD = currentSwapChain;
             recordCommandBuffer(&swapChainD->cbWrapper);
+            swapChainD->cbWrapper.resetCommands();
             cb = swapChainD->cbWrapper.cb;
         }
         QRhi::FrameOpResult submitres = endAndSubmitCommandBuffer(cb, VK_NULL_HANDLE, nullptr, nullptr);
@@ -3120,17 +3124,16 @@ VkSampleCountFlagBits QRhiVulkan::effectiveSampleCount(int sampleCount)
 void QRhiVulkan::enqueueTransitionPassResources(QVkCommandBuffer *cbD)
 {
     cbD->passResTrackers.append(QRhiPassResourceTracker());
+    cbD->currentPassResTrackerIndex = cbD->passResTrackers.count() - 1;
+
     QVkCommandBuffer::Command cmd;
     cmd.cmd = QVkCommandBuffer::Command::TransitionPassResources;
     cmd.args.transitionResources.trackerIndex = cbD->passResTrackers.count() - 1;
     cbD->commands.append(cmd);
-    cbD->currentPassResTrackerIndex = cbD->passResTrackers.count() - 1;
 }
 
 void QRhiVulkan::recordCommandBuffer(QVkCommandBuffer *cbD)
 {
-    Q_ASSERT(cbD->recordingPass == QVkCommandBuffer::NoPass);
-
     for (QVkCommandBuffer::Command &cmd : cbD->commands) {
         switch (cmd.cmd) {
         case QVkCommandBuffer::Command::CopyBuffer:
@@ -3244,8 +3247,6 @@ void QRhiVulkan::recordCommandBuffer(QVkCommandBuffer *cbD)
             break;
         }
     }
-
-    cbD->resetCommands();
 }
 
 static inline VkAccessFlags toVkAccess(QRhiPassResourceTracker::BufferAccess access)
@@ -4138,13 +4139,30 @@ const QRhiNativeHandles *QRhiVulkan::nativeHandles(QRhiCommandBuffer *cb)
 
 void QRhiVulkan::beginExternal(QRhiCommandBuffer *cb)
 {
-    Q_UNUSED(cb);
+    QVkCommandBuffer *cbD = QRHI_RES(QVkCommandBuffer, cb);
+    recordCommandBuffer(cbD);
+    cbD->resetCommands();
 }
 
 void QRhiVulkan::endExternal(QRhiCommandBuffer *cb)
 {
     QVkCommandBuffer *cbD = QRHI_RES(QVkCommandBuffer, cb);
+    Q_ASSERT(cbD->commands.isEmpty() && cbD->currentPassResTrackerIndex == -1);
+
     cbD->resetCachedState();
+
+    // ### FIXME this is all broken (leads to barriers within a renderpass
+    // instance which (1) would need a self-dependency for the subpass and (2) is
+    // not what we want anyway since it then has a different sychronization scope).
+    //
+    // To be replaced with a secondary command buffer for the external content.
+    //
+    if (cbD->recordingPass != QVkCommandBuffer::NoPass) {
+        // Commands that come after this point need a resource tracker and the
+        // corresponding barriers. Note that we are inside a renderpass
+        // instance here and that needs a self-dependency for the subpass.
+        enqueueTransitionPassResources(cbD);
+    }
 }
 
 void QRhiVulkan::setObjectName(uint64_t object, VkDebugReportObjectTypeEXT type, const QByteArray &name, int slot)
diff --git a/src/gui/rhi/qrhivulkan_p_p.h b/src/gui/rhi/qrhivulkan_p_p.h
index e2ea8fec2a..dd9a7d4216 100644
--- a/src/gui/rhi/qrhivulkan_p_p.h
+++ b/src/gui/rhi/qrhivulkan_p_p.h
@@ -324,9 +324,9 @@ struct QVkCommandBuffer : public QRhiCommandBuffer
     };
 
     void resetState() {
-        resetCommands();
         recordingPass = NoPass;
         currentTarget = nullptr;
+        resetCommands();
         resetCachedState();
     }
 
@@ -510,9 +510,10 @@ struct QVkCommandBuffer : public QRhiCommandBuffer
 
     void resetCommands() {
         commands.clear();
+        resetPools();
+
         passResTrackers.clear();
         currentPassResTrackerIndex = -1;
-        resetPools();
     }
 
     void resetPools() {