OpenGL renderer: fix use of outdated VAO when shader is reloaded

In a case where an existing shader was updated, VAO previously associated
with the shader wouldn't be updated properly.

To account for this, we now release all VAOs associated with a shader when
loading a shader. Additionally we also skip VAO creation of commands referencing
a shader that was loaded in the current frame. The reason for that is we know
the command for that VAO is not completed. Qt3D requires a first frame to load a
shader + perform introspection and creates valid command against shader only on
the following frame.

Change-Id: I36f89fdd78e857dc5bc4af3b539d3b32630dfad6
Reviewed-by: Mike Krus <mike.krus@kdab.com>
(cherry picked from commit 7f1bcdb799ad689fc138f976eed7e12f1ce64386)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

3 files changed, 27 insertions, 7 deletions

diff --git a/src/plugins/renderers/opengl/renderer/renderer.cpp b/src/plugins/renderers/opengl/renderer/renderer.cpp
index 91bf77fa9..ede7e2234 100644
--- a/src/plugins/renderers/opengl/renderer/renderer.cpp
+++ b/src/plugins/renderers/opengl/renderer/renderer.cpp
@@ -126,9 +126,6 @@
 
 QT_BEGIN_NAMESPACE
 
-// Crashes on AMD Radeon drivers on Windows. Disable for now.
-//#define SHADER_LOADING_IN_COMMAND_THREAD
-
 using namespace Qt3DCore;
 
 namespace Qt3DRender {
@@ -875,6 +872,13 @@ void Renderer::prepareCommandsSubmission(const std::vector<RenderView *> &render
                 // RenderCommand should have a handle to the corresponding VAO for the Mesh and Shader
                 HVao vaoHandle;
 
+                // If shader was loaded this frame, skip creating VAO for the command
+                // as we have to wait for next frame to make sure command was build against valid shader
+                if (m_lastLoadedShaderIds.contains(command.m_shaderId)) {
+                    command.m_isValid = false;
+                    return;
+                }
+
                 // Create VAO or return already created instance associated with command shader/geometry
                 // (VAO is emulated if not supported)
                 createOrUpdateVAO(&command, &vaoHandle, &vao);
@@ -1232,7 +1236,6 @@ void Renderer::updateGLResources()
         }
     }
 
-#ifndef SHADER_LOADING_IN_COMMAND_THREAD
     {
         Profiling::GLTimeRecorder recorder(Profiling::ShaderUpload, activeProfiler());
         const std::vector<HShader> dirtyShaderHandles = std::move(m_dirtyShaders);
@@ -1246,9 +1249,26 @@ void Renderer::updateGLResources()
 
             // Compile shader
             m_submissionContext->loadShader(shader, shaderManager, m_glResourceManagers->glShaderManager());
+
+            // Release any VAO referencing this shader. When we build VAO, we
+            // rely on the shader introspection to know the active uniforms In
+            // case the shader is reloaded, we might end up having more/less
+            // active uniforms than prior therefore we need to ensure VAO is
+            // rebuilt.
+            VAOManager *vaoManager = m_glResourceManagers->vaoManager();
+            const std::vector<HVao> activeVaos = vaoManager->activeHandles(); // copy
+            for (const HVao &vao : activeVaos) {
+                if (vao.data() && vao->key().second == shader->peerId())
+                    vaoManager->releaseResource(vao->key());
+            }
+
+            // Record shader id in vector of vectors loaded this frame
+            // Given commands need to be built against loaded shader (at next frame)
+            // we can make use of this vector to skip operations that target this
+            // shader for this frame
+            m_lastLoadedShaderIds.push_back(shader->peerId());
         }
     }
-#endif
 
     {
         Profiling::GLTimeRecorder recorder(Profiling::TextureUpload, activeProfiler());
@@ -2239,6 +2259,7 @@ void Renderer::cleanGraphicsResources()
         // We can really release the texture at this point
         m_nodesManager->shaderManager()->releaseResource(shaderCleanedUpId);
     }
+    m_lastLoadedShaderIds.clear();
 }
 
 const GraphicsApiFilterData *Renderer::contextInfo() const
diff --git a/src/plugins/renderers/opengl/renderer/renderer_p.h b/src/plugins/renderers/opengl/renderer/renderer_p.h
index aedd49b49..78dfbe2a5 100644
--- a/src/plugins/renderers/opengl/renderer/renderer_p.h
+++ b/src/plugins/renderers/opengl/renderer/renderer_p.h
@@ -394,6 +394,7 @@ private:
     std::vector<Qt3DCore::QNodeId> m_updatedDisableSubtreeEnablers;
     Qt3DCore::QNodeIdVector m_textureIdsToCleanup;
     std::vector<ShaderBuilderUpdate> m_shaderBuilderUpdates;
+    Qt3DCore::QNodeIdVector m_lastLoadedShaderIds;
 
     bool m_ownedContext;
 
diff --git a/src/plugins/renderers/rhi/renderer/renderer.cpp b/src/plugins/renderers/rhi/renderer/renderer.cpp
index c6190403b..184c94e78 100644
--- a/src/plugins/renderers/rhi/renderer/renderer.cpp
+++ b/src/plugins/renderers/rhi/renderer/renderer.cpp
@@ -129,8 +129,6 @@
 
 QT_BEGIN_NAMESPACE
 
-// Crashes on AMD Radeon drivers on Windows. Disable for now.
-//#define SHADER_LOADING_IN_COMMAND_THREAD
 using namespace Qt3DCore;
 
 namespace Qt3DRender {