From e52382023b85c435e9c1e3a37eeac65178ee54e0 Mon Sep 17 00:00:00 2001 From: Paul Lemire Date: Fri, 14 Feb 2020 10:20:38 +0100 Subject: Fix and improve FBO handling FBO need to be rebuild when one of the attachments directly or indirectly changes. By direct change we mean one of the FBO attachment texture being resized or the list of attachments changing. By indirect we mean when texture resource is recreated internally by the engine. Failure to handle this cases resulted in FBO referencing invalid attachments. Change-Id: I8dd4c08e464eed7fb0eeefd61a4158304ab4245f Task-number: QTBUG-64757 Reviewed-by: Mike Krus --- src/render/backend/attachmentpack.cpp | 26 +++++++++ src/render/backend/attachmentpack_p.h | 6 +++ .../opengl/graphicshelpers/submissioncontext.cpp | 61 +++++++++++++--------- .../opengl/graphicshelpers/submissioncontext_p.h | 20 +++++-- src/render/renderers/opengl/renderer/renderer.cpp | 11 +++- 5 files changed, 95 insertions(+), 29 deletions(-) diff --git a/src/render/backend/attachmentpack.cpp b/src/render/backend/attachmentpack.cpp index a2ac8c30c..0cd5d8673 100644 --- a/src/render/backend/attachmentpack.cpp +++ b/src/render/backend/attachmentpack.cpp @@ -88,6 +88,32 @@ int AttachmentPack::getDrawBufferIndex(QRenderTargetOutput::AttachmentPoint atta return -1; } +bool operator ==(const Attachment &a, const Attachment &b) +{ + return (a.m_name == b.m_name && + a.m_mipLevel == b.m_mipLevel && + a.m_layer == b.m_layer && + a.m_textureUuid == b.m_textureUuid && + a.m_point == b.m_point && + a.m_face == b.m_face); +} + +bool operator !=(const Attachment &a, const Attachment &b) +{ + return !(a == b); +} + +bool operator ==(const AttachmentPack &packA, const AttachmentPack &packB) +{ + return (packA.attachments() == packB.attachments() && + packA.getGlDrawBuffers() == packB.getGlDrawBuffers()); +} + +bool operator !=(const AttachmentPack &packA, const AttachmentPack &packB) +{ + return !(packA == packB); +} + } // namespace Render } // namespace Qt3DRender diff --git a/src/render/backend/attachmentpack_p.h b/src/render/backend/attachmentpack_p.h index a3a2586dd..d0b65589f 100644 --- a/src/render/backend/attachmentpack_p.h +++ b/src/render/backend/attachmentpack_p.h @@ -99,6 +99,12 @@ private: QVector m_drawBuffers; }; +bool operator ==(const Attachment &a, const Attachment &b); +bool operator !=(const Attachment &a, const Attachment &b); + +bool operator ==(const AttachmentPack &packA, const AttachmentPack &packB); +bool operator !=(const AttachmentPack &packA, const AttachmentPack &packB); + } // namespace Render } // namespace Qt3DRender diff --git a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp index b8229dd9e..1a0558e4b 100644 --- a/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp +++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext.cpp @@ -490,9 +490,8 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI // New RenderTarget if (!m_renderTargets.contains(renderTargetNodeId)) { if (m_defaultFBO && fboId == m_defaultFBO) { - // this is the default fbo that some platforms create (iOS), we just register it - // Insert FBO into hash - m_renderTargets.insert(renderTargetNodeId, fboId); + // this is the default fbo that some platforms create (iOS), we never + // register it } else { fboId = createRenderTarget(renderTargetNodeId, attachments); } @@ -501,6 +500,7 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI } } m_activeFBO = fboId; + m_activeFBONodeId = renderTargetNodeId; m_glHelper->bindFrameBufferObject(m_activeFBO, GraphicsHelperInterface::FBODraw); // Set active drawBuffers activateDrawBuffers(attachments); @@ -509,7 +509,8 @@ void SubmissionContext::activateRenderTarget(Qt3DCore::QNodeId renderTargetNodeI void SubmissionContext::releaseRenderTarget(const Qt3DCore::QNodeId id) { if (m_renderTargets.contains(id)) { - const GLuint fboId = m_renderTargets.take(id); + const RenderTargetInfo targetInfo = m_renderTargets.take(id); + const GLuint fboId = targetInfo.fboId; m_glHelper->releaseFrameBufferObject(fboId); } } @@ -519,11 +520,11 @@ GLuint SubmissionContext::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeI const GLuint fboId = m_glHelper->createFrameBufferObject(); if (fboId) { // The FBO is created and its attachments are set once - // Insert FBO into hash - m_renderTargets.insert(renderTargetNodeId, fboId); // Bind FBO m_glHelper->bindFrameBufferObject(fboId, GraphicsHelperInterface::FBODraw); - bindFrameBufferAttachmentHelper(fboId, attachments); + // Insert FBO into hash + const RenderTargetInfo info = bindFrameBufferAttachmentHelper(fboId, attachments); + m_renderTargets.insert(renderTargetNodeId, info); } else { qCritical("Failed to create FBO"); } @@ -532,31 +533,38 @@ GLuint SubmissionContext::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeI GLuint SubmissionContext::updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId, const AttachmentPack &attachments, bool isActiveRenderTarget) { - const GLuint fboId = m_renderTargets.value(renderTargetNodeId); + const RenderTargetInfo fboInfo = m_renderTargets.value(renderTargetNodeId); + const GLuint fboId =fboInfo.fboId; - // We need to check if one of the attachment was resized - bool needsResize = !m_renderTargetsSize.contains(fboId); // not even initialized yet? - if (!needsResize) { + // We need to check if one of the attachnent have changed QTBUG-64757 + bool needsRebuild = attachments != fboInfo.attachments; + + // Even if the attachment packs are the same, one of the inner texture might have + // been resized or recreated, we need to check for that + if (!needsRebuild) { // render target exists, has attachment been resized? GLTextureManager *glTextureManager = m_renderer->nodeManagers()->glTextureManager(); - const QSize s = m_renderTargetsSize[fboId]; + const QSize s = fboInfo.size; + const auto attachments_ = attachments.attachments(); for (const Attachment &attachment : attachments_) { + const bool textureWasUpdated = m_updateTextureIds.contains(attachment.m_textureUuid); GLTexture *rTex = glTextureManager->lookupResource(attachment.m_textureUuid); - // ### TODO QTBUG-64757 this check is insufficient since the - // texture may have changed to another one with the same size. That - // case is not handled atm. if (rTex) { - needsResize |= rTex->size() != s; + const bool sizeHasChanged = rTex->size() != s; + needsRebuild |= sizeHasChanged; if (isActiveRenderTarget && attachment.m_point == QRenderTargetOutput::Color0) m_renderTargetFormat = rTex->properties().format; } + needsRebuild |= textureWasUpdated; } } - if (needsResize) { + if (needsRebuild) { m_glHelper->bindFrameBufferObject(fboId, GraphicsHelperInterface::FBODraw); - bindFrameBufferAttachmentHelper(fboId, attachments); + const RenderTargetInfo updatedInfo = bindFrameBufferAttachmentHelper(fboId, attachments); + // Update our stored Render Target Info + m_renderTargets.insert(renderTargetNodeId, updatedInfo); } return fboId; @@ -567,8 +575,8 @@ QSize SubmissionContext::renderTargetSize(const QSize &surfaceSize) const QSize renderTargetSize; if (m_activeFBO != m_defaultFBO) { // For external FBOs we may not have a m_renderTargets entry. - if (m_renderTargetsSize.contains(m_activeFBO)) { - renderTargetSize = m_renderTargetsSize[m_activeFBO]; + if (m_renderTargets.contains(m_activeFBONodeId)) { + renderTargetSize = m_renderTargets[m_activeFBONodeId].size; } else if (surfaceSize.isValid()) { renderTargetSize = surfaceSize; } else { @@ -813,7 +821,7 @@ bool SubmissionContext::activateShader(ProgramDNA shaderDNA) return true; } -void SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments) +SubmissionContext::RenderTargetInfo SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments) { // Set FBO attachments. These are normally textures, except that on Open GL // ES <= 3.1 we must use a renderbuffer if a combined depth+stencil is @@ -848,7 +856,7 @@ void SubmissionContext::bindFrameBufferAttachmentHelper(GLuint fboId, const Atta } } } - m_renderTargetsSize.insert(fboId, fboSize); + return {fboId, fboSize, attachments}; } void SubmissionContext::activateDrawBuffers(const AttachmentPack &attachments) @@ -1154,6 +1162,11 @@ void SubmissionContext::deleteSync(GLFence sync) m_glHelper->deleteSync(sync); } +void SubmissionContext::setUpdatedTexture(const Qt3DCore::QNodeIdVector &updatedTextureIds) +{ + m_updateTextureIds = updatedTextureIds; +} + // It will be easier if the QGraphicContext applies the QUniformPack // than the other way around bool SubmissionContext::setParameters(ShaderParameterPack ¶meterPack) @@ -1544,13 +1557,13 @@ void SubmissionContext::blitFramebuffer(Qt3DCore::QNodeId inputRenderTargetId, // Up until this point the input and output rects are normal Qt rectangles. // Convert them to GL rectangles (Y at bottom). - const int inputFboHeight = inputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargetsSize[inputFboId].height(); + const int inputFboHeight = inputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargets[inputRenderTargetId].size.height(); const GLint srcX0 = inputRect.left(); const GLint srcY0 = inputFboHeight - (inputRect.top() + inputRect.height()); const GLint srcX1 = srcX0 + inputRect.width(); const GLint srcY1 = srcY0 + inputRect.height(); - const int outputFboHeight = outputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargetsSize[outputFboId].height(); + const int outputFboHeight = outputFboId == defaultFboId ? m_surfaceSize.height() : m_renderTargets[outputRenderTargetId].size.height(); const GLint dstX0 = outputRect.left(); const GLint dstY0 = outputFboHeight - (outputRect.top() + outputRect.height()); const GLint dstX1 = dstX0 + outputRect.width(); diff --git a/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h index 0f769fa1c..fd42090dd 100644 --- a/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h +++ b/src/render/renderers/opengl/graphicshelpers/submissioncontext_p.h @@ -62,6 +62,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -158,7 +159,16 @@ public: bool wasSyncSignaled(GLFence sync); void deleteSync(GLFence sync); + // Textures + void setUpdatedTexture(const Qt3DCore::QNodeIdVector &updatedTextureIds); + private: + struct RenderTargetInfo { + GLuint fboId; + QSize size; + AttachmentPack attachments; + }; + void initialize(); // Material @@ -166,7 +176,7 @@ private: void setActiveMaterial(Material* rmat); // FBO - void bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments); + RenderTargetInfo bindFrameBufferAttachmentHelper(GLuint fboId, const AttachmentPack &attachments); void activateDrawBuffers(const AttachmentPack &attachments); void resolveRenderTargetFormat(); GLuint createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId, const AttachmentPack &attachments); @@ -187,8 +197,9 @@ private: ProgramDNA m_activeShaderDNA; QHash m_renderBufferHash; - QHash m_renderTargets; - QHash m_renderTargetsSize; + + + QHash m_renderTargets; QAbstractTexture::TextureFormat m_renderTargetFormat; // cache some current state, to make sure we don't issue unnecessary GL calls @@ -199,6 +210,7 @@ private: Material* m_material; QRectF m_viewport; GLuint m_activeFBO; + Qt3DCore::QNodeId m_activeFBONodeId; GLBuffer *m_boundArrayBuffer; RenderStateSet* m_stateSet; @@ -227,6 +239,8 @@ private: using VAOIndexAttribute = HGLBuffer; void enableAttribute(const VAOVertexAttribute &attr); void disableAttribute(const VAOVertexAttribute &attr); + + Qt3DCore::QNodeIdVector m_updateTextureIds; }; } // namespace Render diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp index 3e5bef3cd..aced13fc5 100644 --- a/src/render/renderers/opengl/renderer/renderer.cpp +++ b/src/render/renderers/opengl/renderer/renderer.cpp @@ -1359,13 +1359,15 @@ void Renderer::updateGLResources() if (texture == nullptr) continue; - // Create or Update GLTexture (the GLTexture instance is created if required - // and all things that can take place without a GL context are done here) + // Create or Update GLTexture (the GLTexture instance is created + // (not the underlying GL instance) if required and all things that + // can take place without a GL context are done here) updateTexture(texture); } // We want to upload textures data at this point as the SubmissionThread and // AspectThread are locked ensuring no races between Texture/TextureImage and // GLTexture + QNodeIdVector updatedTexturesForFrame; if (m_submissionContext != nullptr) { GLTextureManager *glTextureManager = m_nodesManager->glTextureManager(); const QVector glTextureHandles = glTextureManager->activeHandles(); @@ -1387,10 +1389,15 @@ void Renderer::updateGLResources() updateInfo.handleType = QAbstractTexture::OpenGLTextureId; updateInfo.handle = info.texture ? QVariant(info.texture->textureId()) : QVariant(); m_updatedTextureProperties.push_back({updateInfo, referenceTextureIds}); + updatedTexturesForFrame += referenceTextureIds; } } } + // If the underlying GL Texture was for whatever reason recreated, we need to make sure + // that if it is used as a color attachment, we rebuild the FBO next time it is used + m_submissionContext->setUpdatedTexture(std::move(updatedTexturesForFrame)); + // Record ids of texture to cleanup while we are still blocking the aspect thread m_textureIdsToCleanup += m_nodesManager->textureManager()->takeTexturesIdsToCleanup(); } -- cgit v1.2.3