Cleanup dragon Renderer and ActivatedSurface

Cleanup and small improvements. Some less obvious changes:
- Moving private methods to private.
- Separate checkContextInfoRequested() from doRender()
- m_clearDepth and m_clearStencil were not properly used, they never
cached previous value. So removing them and also m_clearColor, setting
shouldn't cost ~anything but better make sure they are correct before
glClear.
- bindParameters() removed as it was unused and same method is in
renderer/dragondraw.cpp
- isActiveRenderTarget parameter of updateRenderTarget() wasn't really
used for anything.

Task-number: QT3DS-3045
Change-Id: Ica4379fd5f32f1716b87133e151880f660c6c642
Reviewed-by: Miikka Heikkinen <miikka.heikkinen@qt.io>

7 files changed, 124 insertions, 455 deletions

diff --git a/src/runtime/dragon/dragonactivatedsurface.cpp b/src/runtime/dragon/dragonactivatedsurface.cpp
index 2d79a94..9699c5c 100644
--- a/src/runtime/dragon/dragonactivatedsurface.cpp
+++ b/src/runtime/dragon/dragonactivatedsurface.cpp
@@ -65,9 +65,6 @@
 // Qt3DRender::Render
 #include <Qt3DRender/private/platformsurfacefilter_p.h>
 
-#include <Qt3DCore/QNodeId>
-
-#include <QOpenGLContext>
 #include <QSurface>
 #include <QWindow>
 #include <qcullface.h>
@@ -237,41 +234,29 @@ ActivatedSurface::~ActivatedSurface()
     m_glContext->doneCurrent();
 }
 
-bool ActivatedSurface::isValid()
+bool ActivatedSurface::isValid() const
 {
     return m_valid;
 }
 
-void ActivatedSurface::setGlHelper(QSharedPointer<Dragon::GraphicsHelperInterface> helper)
-{
-    m_glHelper = helper;
-}
-
 void ActivatedSurface::clearBackBuffer(const ClearBackBufferInfo &info)
 {
     auto clearBufferTypes = info.buffers;
     if (clearBufferTypes & QClearBuffers::ColorBuffer) {
         const QVector4D vCol = info.color;
         const QColor color = QColor::fromRgbF(vCol.x(), vCol.y(), vCol.z(), vCol.w());
-        if (m_clearColor != color) {
-            m_clearColor = color;
-            m_glContext->functions()->glClearColor(color.redF(),
-                                                   color.greenF(),
-                                                   color.blueF(),
-                                                   color.alphaF());
-        }
+        m_glContext->functions()->glClearColor(color.redF(),
+                                               color.greenF(),
+                                               color.blueF(),
+                                               color.alphaF());
     }
     if (clearBufferTypes & QClearBuffers::DepthBuffer) {
         const float depth = info.depth;
-        if (m_clearDepth != depth) {
-            m_glContext->functions()->glClearDepthf(depth);
-        }
+        m_glContext->functions()->glClearDepthf(depth);
     }
     if (clearBufferTypes & QClearBuffers::StencilBuffer) {
         const int stencil = info.stencil;
-        if (m_clearStencil != stencil) {
-            m_glContext->functions()->glClearStencil(stencil);
-        }
+        m_glContext->functions()->glClearStencil(stencil);
     }
 
     if (clearBufferTypes != QClearBuffers::None) {
@@ -288,24 +273,6 @@ void ActivatedSurface::clearBackBuffer(const ClearBackBufferInfo &info)
     }
 }
 
-bool ActivatedSurface::activateShader(const Immutable<GLShader> &shader)
-{
-    if (shader->dna == m_activeShaderDNA) {
-        return true;
-    }
-
-    // TODO consider m_material
-    //    // Ensure material uniforms are re-applied
-    //    m_material = nullptr;
-
-    // TODO do we need to store the active shader?
-    //    m_activeShader = shader;
-
-    shader->shaderProgram->bind();
-    m_activeShaderDNA = shader->dna;
-    return true;
-}
-
 void ActivatedSurface::applyUniform(const ShaderUniform &description, const UniformValue &v)
 {
     const UniformType type = m_glHelper->uniformTypeFromGLType(description.m_type);
@@ -472,13 +439,13 @@ UniformValue standardUniformValue(StandardUniform standardUniformType,
         return UniformValue(convertToQMatrix4x4(cameraMatrices->viewMatrix * model).normalMatrix());
     case ViewportMatrix: {
         QMatrix4x4 viewportMatrix;
-        // TO DO: Implement on Matrix4x4
+        // TODO: Implement on Matrix4x4
         viewportMatrix.viewport(resolveViewport(renderView->viewport, renderView->surfaceSize));
         return UniformValue(Matrix4x4(viewportMatrix));
     }
     case InverseViewportMatrix: {
         QMatrix4x4 viewportMatrix;
-        // TO DO: Implement on Matrix4x4
+        // TODO: Implement on Matrix4x4
         viewportMatrix.viewport(resolveViewport(renderView->viewport, renderView->surfaceSize));
         return UniformValue(Matrix4x4(viewportMatrix.inverted()));
     }
@@ -506,143 +473,8 @@ UniformValue standardUniformValue(StandardUniform standardUniformType,
     }
     default:
         Q_UNREACHABLE();
-        return UniformValue();
-    }
-}
-
-bool ActivatedSurface::bindParameters(const Immutable<RenderView> &renderView,
-                                      const Immutable<CameraMatrices> &cameraMatrices,
-                                      const MutableContainer<GLBuffer> &glBuffers,
-                                      const MutableContainer<GLTexture> &glTextures,
-                                      int maxTextureImageUnits,
-                                      const Immutable<GLShader> &glShader,
-                                      const Immutable<Matrix4x4> &worldTransform,
-                                      const ParameterNameToParameterMap &parameterMap)
-{
-    // ParameterMap = nameId -> parameter for current command
-
-    // Set standard uniforms
-    // TODO Once we have shader graphs, we should put standard uniforms in a uniform buffer object
-    // and emulate UBOs on hardware that doesn't support it, including unrolling them in the
-    // shaders
-    for (const auto &standardShaderUniform : glShader->standardUniforms) {
-        const auto &shaderUniform = standardShaderUniform.shaderUniform;
-        const auto &setter = standardShaderUniform.standardUniform;
-        UniformValue uniformValue = standardUniformValue(setter, renderView, cameraMatrices, *worldTransform);
-        applyUniform(shaderUniform, uniformValue);
-    }
-
-    // Keep track of the currently bound texture unit
-    int unit = 0;
-    // TODO: In OpenGL 4.4+, there is bindTextures(), which we could add to our graphics helpers:
-    // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glBindTextures.xhtml
-    for (const auto &shaderUniform : glShader->uniforms) { // scalars and textures
-        // TODO consider returning the result of this in cases where the parameters have not changed,
-        // so that we can reuse the same parameter and avoid looking it up every frame. The returned
-        // result can be stored in a map in the Frame
-        const auto &parameterIt = parameterMap.constFind(shaderUniform.m_nameId);
-        if (parameterIt == parameterMap.cend())
-            continue;
-
-        const auto &parameter = parameterIt.value();
-        const auto &uniformValue = parameter->uniformValue();
-
-        if (uniformValue.valueType() != UniformValue::NodeId) {
-            applyUniform(shaderUniform, uniformValue);
-            continue;
-        }
-
-        // At this point a uniform value can only be a scalar type
-        // or a Qt3DCore::QNodeId corresponding to a Texture
-        // ShaderData/Buffers would be handled as UBO/SSBO and would therefore
-        // not be in the default uniform block
-
-        const QNodeId *nodeIds = uniformValue.constData<QNodeId>();
-        const int uniformArraySize = uint(uniformValue.byteSize()) / sizeof(QNodeId);
-        for (int i = 0; i < uniformArraySize; ++i) {
-            const QNodeId texId = nodeIds[i];
-            if (texId.isNull()) {
-                qDebug() << "Got null texId, skipping";
-                // TODO this entire implementation is a bit fragile. Consider making UniformValue
-                // store data in a more type-safe way for QNodeIds
-                continue;
-            }
-
-            Q_ASSERT(glTextures.contains(texId));
-            const auto &texture = glTextures[texId];
-            //            Q_ASSERT(texture->openGLTexture != nullptr);
-
-            if (texture->openGLTexture == nullptr) {
-                // TODO current valid case for this is when the format is automatic, however,
-                // we need to investigate where this is used and why we should skip at this point
-                continue;
-            }
-
-            ShaderParameterPack::NamedTexture namedTex(shaderUniform.m_nameId, texId, i);
-
-            UniformValue textureValue(uniformArraySize * sizeof(int), UniformValue::TextureValue);
-            std::fill(textureValue.data<int>(), textureValue.data<int>() + uniformArraySize, -1);
-
-            if (unit >= maxTextureImageUnits) {
-                // TODO: Check this earlier and skip rendering completely if this is the case.
-                qWarning() << "WARNING: Max number of texture units exhausted,"
-                           << "GL_MAX_TEXTURE_IMAGE_UNITS:" << maxTextureImageUnits
-                           << "Texture cannot be bound.";
-                continue;
-            }
-            // NOTE: Qt3D used to have some logic to keep track of bound textures to avoid
-            // re-binding. However, this introduced additional complexity and caused performance
-            // problems on the CPU side. We should consider adding back some of this logic
-            // if it turns out to be beneficial for performance. However, considering that
-            // glBindTextures no longer uses glActiveTexture, it seems likely that there is
-            // no need for this.
-            texture->openGLTexture->bind(GLuint(unit));
-
-            textureValue.data<int>()[namedTex.uniformArrayIndex] = unit;
-            applyUniform(shaderUniform, textureValue);
-
-            unit += 1;
-        }
-    }
-
-    GLuint uboIndex = 0;
-    for (const auto &shaderUniformBlock : glShader->uniformBlocks) {
-        const auto &parameterIt = parameterMap.constFind(shaderUniformBlock.m_nameId);
-        if (parameterIt == parameterMap.cend())
-            continue;
-
-        const auto &parameter = parameterIt.value();
-        const auto &uniformValue = parameter->uniformValue();
-        if (uniformValue.valueType() != UniformValue::NodeId) {
-            // TODO might be a bit spammy to send this to the output
-            qWarning()
-                << "WARNING: Found UniformBlock with corresponding parameter that was not of UniformValue::NodeId type. This is not supported";
-        }
-
-        const auto &bufferId = *uniformValue.constData<QNodeId>();
-        Q_ASSERT(glBuffers.contains(bufferId));
-        const auto &glBuffer = glBuffers[bufferId];
-
-        // TODO always zero, consider using other locations...
-        // TODO Make sure that there's enough binding points
-        const GLuint blockIndex = GLuint(shaderUniformBlock.m_index);
-        const GLuint programId = glShader->shaderProgram->programId();
-        m_glHelper->bindUniformBlock(programId, blockIndex, uboIndex);
-        // Needed to avoid conflict where the buffer would already be bound as a VertexArray
-        const auto glType = glBufferTypes[GLBuffer::UniformBuffer];
-        m_glContext->functions()->glBindBuffer(glType, glBuffer->bufferId());
-        m_glHelper->bindBufferBase(glType, uboIndex++, glBuffer->bufferId());
-        // Buffer update to GL buffer will be done at render time
-    }
-
-    for (const auto &shaderStorageBlock : glShader->storageBlocks) {
-        // TODO add support for shader storage blocks
-        Q_UNUSED(shaderStorageBlock)
-        qWarning() << "WARNING: Shader storage blocks are not yet supported";
     }
-
-    // if not all data is valid, the next frame will be rendered immediately
-    return true;
+    return UniformValue();
 }
 
 bool ActivatedSurface::bindVertexArrayObject(const Immutable<GLVertexArrayObject> &vao)
@@ -684,11 +516,6 @@ Immutable<GLVertexArrayObject> ActivatedSurface::createVertexArrayObject(
     }
     vao.m_owners = key;
 
-    // TODO actually check if geometry is dirty
-
-    // TODO verify that the shader does not need to be bound to create a vertex array object
-//    activateShader(uploadedShader);
-
     // TODO return bound vao for other commands to use or just move internals here
 
     const auto attributeIds = geometry->attributes();
@@ -698,8 +525,6 @@ Immutable<GLVertexArrayObject> ActivatedSurface::createVertexArrayObject(
         const auto &glBuffer = uploadedBuffers[attribute->bufferId()];
         const auto attributeType = attributeTypeToGLBufferType(attribute->attributeType());
 
-        // TODO get GlBuffer
-
         // Index Attribute
         bool attributeWasDirty = attributes.dirtyOrNew().contains(attributeId);
 
@@ -759,7 +584,6 @@ Immutable<GLVertexArrayObject> ActivatedSurface::createVertexArrayObject(
         }
 
         GLVertexArrayObject::VAOVertexAttribute attr;
-//        attr.bufferHandle = glBuffer;
         attr.attributeType = attributeType;
         attr.dataType = attributeDataType;
         attr.divisor = attribute->divisor();
@@ -812,21 +636,6 @@ void ActivatedSurface::memoryBarrier(const QMemoryBarrier::Operations &operation
 // TODO Review (make more functional?)
 void ActivatedSurface::activateRenderTarget(GLuint fboId, const AttachmentPack &attachments)
 {
-    //    GLuint fboId = lastBoundFBOId; // we will revert to this if we did not get an FBO id
-    //    if (renderTargetNodeId) {
-    //        // New RenderTarget
-    //        if (!m_renderTargets.contains(renderTargetNodeId)) {
-    //            if (m_defaultFBO != 0 && 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);
-    //            } else {
-    //                fboId = createRenderTarget(renderTargetNodeId, attachments, glTextures);
-    //            }
-    //        } else {
-    //            fboId = updateRenderTarget(renderTargetNodeId, attachments, true, glTextures);
-    //        }
-    //    }
     m_activeFBO = fboId;
     m_glHelper->bindFrameBufferObject(m_activeFBO, GraphicsHelperInterface::FBODraw);
     // Set active drawBuffers
@@ -836,12 +645,13 @@ void ActivatedSurface::activateRenderTarget(GLuint fboId, const AttachmentPack &
 // TODO Review (make more functional?)
 void ActivatedSurface::activateDrawBuffers(const AttachmentPack &attachments)
 {
-    const QVector<int> activeDrawBuffers = attachments.drawBuffers;
-
     if (!m_glHelper->checkFrameBufferComplete()) {
         qWarning() << "WARNING: FBO incomplete";
         return;
     }
+
+    const QVector<int> activeDrawBuffers = attachments.drawBuffers;
+
     if (activeDrawBuffers.size() <= 1)
         return;
     // We need MRT if the number is more than 1
@@ -854,15 +664,6 @@ void ActivatedSurface::activateDrawBuffers(const AttachmentPack &attachments)
     m_glHelper->drawBuffers(activeDrawBuffers.size(), activeDrawBuffers.data());
 }
 
-bool ActivatedSurface::bindGLBuffer(const Mutable<GLBuffer> &buffer, GLBuffer::Type type)
-{
-    // TODO DO NOT USE
-    // TODO only bind in functions that need it, and immediately release when finished calling
-    Q_UNUSED(buffer)
-    Q_UNUSED(type)
-    return true;
-}
-
 // TODO Review (make more functional?)
 GLuint ActivatedSurface::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId,
                                             const AttachmentPack &attachments,
@@ -885,7 +686,6 @@ GLuint ActivatedSurface::createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId
 // TODO Review (make more functional?)
 GLuint ActivatedSurface::updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId,
                                             const AttachmentPack &attachments,
-                                            bool isActiveRenderTarget,
                                             const MutableContainer<GLTexture> &glTextures)
 {
     Q_ASSERT(m_renderTargets.contains(renderTargetNodeId));
@@ -907,10 +707,6 @@ GLuint ActivatedSurface::updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId
             }
             const auto &glTexture = glTextures[attachment->textureUuid];
             needsResize |= (glTexture->size() != s);
-            if (isActiveRenderTarget) {
-                if (attachment->point == QRenderTargetOutput::Color0)
-                    m_renderTargetFormat = glTexture->properties.format;
-            }
         }
     }
 
@@ -922,7 +718,7 @@ GLuint ActivatedSurface::updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId
     return fboId;
 }
 
-void ActivatedSurface::viewport(const QRectF &viewport, const QSize &surfaceSize)
+void ActivatedSurface::setViewport(const QRectF &viewport, const QSize &surfaceSize)
 {
     // save for later use; this has nothing to do with the viewport but it is
     // here that we get to know the surfaceSize from the RenderView.
@@ -1061,7 +857,7 @@ QSize ActivatedSurface::renderTargetSize(const QSize &surfaceSize) const
     return renderTargetSize;
 }
 
-GraphicsApiFilterData ActivatedSurface::contextInfo()
+GraphicsApiFilterData ActivatedSurface::contextInfo() const
 {
     GraphicsApiFilterData result;
     QStringList extensions;
@@ -1103,11 +899,12 @@ QSharedPointer<Dragon::GraphicsHelperInterface> ActivatedSurface::resolveHighest
 #ifndef QT_OPENGL_ES_2
     else {
         QAbstractOpenGLFunctions *glFunctions = nullptr;
-        // TODO clean up
-/*        if ((glFunctions = m_glContext->versionFunctions<QOpenGLFunctions_4_3_Core>()) != nullptr) {
-            qCDebug(Render::Backend) << Q_FUNC_INFO << " Building OpenGL 4.3";
+        // TODO: Note that max OpenGL backend is currently GL3_3, check 4.3 behavior
+        /*if ((glFunctions = m_glContext->versionFunctions<QOpenGLFunctions_4_3_Core>())
+                   != nullptr) {
+            //qCDebug(Render::Backend) << Q_FUNC_INFO << " Building OpenGL 4.3";
             glHelper.reset(new Dragon::GraphicsHelperGL4());
-        } else*/ if ((glFunctions = m_glContext->versionFunctions<QOpenGLFunctions_3_3_Core>())
+        } else */ if ((glFunctions = m_glContext->versionFunctions<QOpenGLFunctions_3_3_Core>())
                    != nullptr) {
             //qCDebug(Render::Backend) << Q_FUNC_INFO << " Building OpenGL 3.3";
             glHelper.reset(new Dragon::GraphicsHelperGL3_3());
@@ -1127,9 +924,6 @@ QSharedPointer<Dragon::GraphicsHelperInterface> ActivatedSurface::resolveHighest
     }
 #endif
 
-    // Set Vendor and Extensions of reference GraphicsApiFilter
-    // TO DO: would that vary like the glHelper ?
-
     return glHelper;
 }
 
@@ -1473,7 +1267,7 @@ void ActivatedSurface::applyState(const StateVariant &stateVariant)
 
 void ActivatedSurface::resetMasked(qint64 maskOfStatesToReset)
 {
-    // TO DO -> Call gcHelper methods instead of raw GL
+    // TODO -> Call gcHelper methods instead of raw GL
     // QOpenGLFunctions shouldn't be used here directly
     QOpenGLFunctions *funcs = m_glContext->functions();
 
@@ -1576,7 +1370,7 @@ void ActivatedSurface::blitFramebuffer(const BlitFramebufferInfo &blitFramebuffe
     if (inputTarget.has_value()) {
         const auto &inputRenderTarget = inputTarget.get();
         if (m_renderTargets.contains(inputRenderTarget->peerId()))
-            inputFboId = updateRenderTarget(inputRenderTarget->peerId(), inputAttachments.get(), false, glTextures);
+            inputFboId = updateRenderTarget(inputRenderTarget->peerId(), inputAttachments.get(), glTextures);
         else
             inputFboId = createRenderTarget(inputRenderTarget->peerId(), inputAttachments.get(), glTextures);
         inputBufferIsDefault = false;
@@ -1587,7 +1381,7 @@ void ActivatedSurface::blitFramebuffer(const BlitFramebufferInfo &blitFramebuffe
     if (outputTarget.has_value()) {
         const auto &outputRenderTarget = outputTarget.get();
         if (m_renderTargets.contains(outputRenderTarget->peerId()))
-            outputFboId = updateRenderTarget(outputRenderTarget->peerId(), outputAttachments.get(), false, glTextures);
+            outputFboId = updateRenderTarget(outputRenderTarget->peerId(), outputAttachments.get(), glTextures);
         else
             outputFboId = createRenderTarget(outputRenderTarget->peerId(), outputAttachments.get(), glTextures);
         outputBufferIsDefault = false;
diff --git a/src/runtime/dragon/dragonactivatedsurface_p.h b/src/runtime/dragon/dragonactivatedsurface_p.h
index 20cd3b3..1afaa39 100644
--- a/src/runtime/dragon/dragonactivatedsurface_p.h
+++ b/src/runtime/dragon/dragonactivatedsurface_p.h
@@ -30,8 +30,6 @@
 #ifndef ACTIVATEDSURFACE_P_H
 #define ACTIVATEDSURFACE_P_H
 
-#include <private/dragonoptional_p.h>
-
 #include <Qt3DRender/qabstracttexture.h>
 #include <Qt3DRender/qmemorybarrier.h>
 
@@ -108,69 +106,59 @@ public:
 
     ~ActivatedSurface();
 
-    bool isValid();
-    void setGlHelper(QSharedPointer<Dragon::GraphicsHelperInterface> helper);
+    bool isValid() const;
+
     // TODO this is not really const and needs a better solution
     Dragon::GraphicsHelperInterface *glHelper() const { return m_glHelper.data(); }
-    QSharedPointer<Dragon::GraphicsHelperInterface> resolveHighestOpenGLFunctions() const;
 
     // commands
     void clearBackBuffer(const ClearBackBufferInfo &info);
     void memoryBarrier(const QMemoryBarrier::Operations &operations);
     void activateRenderTarget(GLuint fboId,
                               const AttachmentPack &attachments);
-    void viewport(const QRectF &viewport, const QSize &surfaceSize);
+    void setViewport(const QRectF &viewport, const QSize &surfaceSize);
     Immutable<GLVertexArrayObject> createVertexArrayObject(VAOIdentifier key,
         const Mutable<GLShader> &uploadedShader,
         const Immutable<Geometry> &geometry,
         const ValueContainer<Attribute> &attributes,
         const MutableContainer<GLBuffer> &uploadedBuffers);
-    GraphicsApiFilterData contextInfo();
+    GraphicsApiFilterData contextInfo() const;
 
-    bool activateShader(const Immutable<GLShader> &shader);
-    bool bindVertexArrayObject(const Immutable<GLVertexArrayObject> &vao);
+    QOpenGLContext *openGLContext() const;
 
-    GLBuffer createGLBuffer(const Immutable<LoadedBuffer> &buffer);
-    bool bindGLBuffer(const Mutable<GLBuffer> &buffer, GLBuffer::Type type);
-    bool bindParameters(const Immutable<RenderView> &renderView, const Immutable<CameraMatrices> &cameraMatrices, const MutableContainer<GLBuffer> &glBuffers,
-                        const MutableContainer<GLTexture> &glTextures, int maxTextureImageUnits,
-                        const Immutable<GLShader> &glShader, const Immutable<Matrix4x4> &worldTransform,
-                        const ParameterNameToParameterMap &parameterMap);
+    QSize bindFrameBufferAttachmentHelper(GLuint fboId,
+                                         const AttachmentPack &attachments,
+                                         const MutableContainer<GLTexture> &glTextures);
 
-//    bool performDraw(const RenderCommand &command,
-//                     const Value<GLShader> &glShader,
-//                     const ValueContainer<Attribute> &attributes, const Value<GLVertexArrayObject> &vao);
-    QOpenGLContext *openGLContext() const;
-    // TODO make private
-    void applyState(const StateVariant &stateVariant);
+    DrawContext beginDrawing(bool autoSwapBuffers);
 
-    // TODO consider moving where they are used
+    void blitFramebuffer(const BlitFramebufferInfo &blitFramebufferInfo, uint defaultFboId, const MutableContainer<GLTexture> &glTextures);
+    RenderStateSet applyStateSet(const RenderStateSet &previous, const RenderStateSet &next);
+
+private:
+
+    void applyState(const StateVariant &stateVariant);
+    QSharedPointer<Dragon::GraphicsHelperInterface> resolveHighestOpenGLFunctions() const;
+    bool bindVertexArrayObject(const Immutable<GLVertexArrayObject> &vao);
     GLuint createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId,
                               const AttachmentPack &attachments,
                               const MutableContainer<GLTexture> &glTextures);
     GLuint updateRenderTarget(Qt3DCore::QNodeId renderTargetNodeId,
                               const AttachmentPack &attachments,
-                              bool isActiveRenderTarget,
                               const MutableContainer<GLTexture> &glTextures);
-    QSize bindFrameBufferAttachmentHelper(GLuint fboId,
-                                         const AttachmentPack &attachments,
-                                         const MutableContainer<GLTexture> &glTextures);
     void activateDrawBuffers(const AttachmentPack &attachments);
     // Parameter/uniform functions
     void applyUniform(const ShaderUniform &description, const UniformValue &v);
 
-    DrawContext beginDrawing(bool autoSwapBuffers);
+    QSize renderTargetSize(const QSize &surfaceSize) const;
+
+    template<UniformType>
+    void applyUniformHelper(const ShaderUniform &, const UniformValue &) const
+    {
+        Q_ASSERT_X(false, Q_FUNC_INFO, "Uniform: Didn't provide specialized apply() implementation");
+    }
+    void resetMasked(qint64 maskOfStatesToReset);
 
-    void blitFramebuffer(const BlitFramebufferInfo &blitFramebufferInfo, uint defaultFboId, const MutableContainer<GLTexture> &glTextures);
-    RenderStateSet applyStateSet(const RenderStateSet &previous, const RenderStateSet &next);
-private:
-    // Cached clear data
-    Optional<QColor> m_clearColor;
-    Optional<float> m_clearDepth;
-    Optional<int> m_clearStencil;
-    // Cached shader data
-    ProgramDNA m_activeShaderDNA = 0;
-    // TODO difference between currentSurface and surface is what?
     QSurface *m_surface = nullptr;
     bool m_valid = false;
     QSharedPointer<Dragon::GraphicsHelperInterface> m_glHelper;
@@ -179,35 +167,21 @@ private:
     GLuint m_activeFBO = 0;
 
     // Render targets
-    //    QHash<Qt3DCore::QNodeId, HGLBuffer> m_renderBufferHash;
     QHash<Qt3DCore::QNodeId, GLuint> m_renderTargets;
     QHash<GLuint, QSize> m_renderTargetsSize;
-    QAbstractTexture::TextureFormat m_renderTargetFormat;
 
     // Context
     bool m_supportsVAO = false;
     GLuint m_defaultFBO = 0;
 
-
-    // TODO consider removing
     QSize m_surfaceSize;
     QRectF m_viewport;
 
-    // TODO reconsider active textures strategy
-    Immutable<RenderStateSet> m_currentStateSet;
-
     // Set to be mutable because some functions don't change the state, but still
     // need to access the context. Note that mutable is strictly not necessary
     // because it's a pointer.
     mutable QOpenGLContext *m_glContext = nullptr;
-    QSize renderTargetSize(const QSize &surfaceSize) const;
 
-    template<UniformType>
-    void applyUniformHelper(const ShaderUniform &, const UniformValue &) const
-    {
-        Q_ASSERT_X(false, Q_FUNC_INFO, "Uniform: Didn't provide specialized apply() implementation");
-    }
-    void resetMasked(qint64 maskOfStatesToReset);
 };
 
 // TODO consider a template-only (macro-free) implementation
diff --git a/src/runtime/dragon/dragonoffscreensurfacehelper.cpp b/src/runtime/dragon/dragonoffscreensurfacehelper.cpp
index aef5335..0c68cca 100644
--- a/src/runtime/dragon/dragonoffscreensurfacehelper.cpp
+++ b/src/runtime/dragon/dragonoffscreensurfacehelper.cpp
@@ -45,10 +45,12 @@ OffscreenSurfaceHelper::OffscreenSurfaceHelper(const QSurfaceFormat &format, QOb
 
 void OffscreenSurfaceHelper::createOffscreenSurface()
 {
-    m_offscreenSurface = new QOffscreenSurface;
-    m_offscreenSurface->setParent(this);
-    m_offscreenSurface->setFormat(m_format);
-    m_offscreenSurface->create();
+    if (!m_offscreenSurface) {
+        m_offscreenSurface = new QOffscreenSurface;
+        m_offscreenSurface->setParent(this);
+        m_offscreenSurface->setFormat(m_format);
+        m_offscreenSurface->create();
+    }
 }
 
 QOffscreenSurface *OffscreenSurfaceHelper::offscreenSurface() const
diff --git a/src/runtime/dragon/dragonrenderer.cpp b/src/runtime/dragon/dragonrenderer.cpp
index 5bc99f8..72b7301 100644
--- a/src/runtime/dragon/dragonrenderer.cpp
+++ b/src/runtime/dragon/dragonrenderer.cpp
@@ -80,36 +80,6 @@ GraphicsApiFilterData contextInfo(const QOpenGLContext &m_gl)
     return m_contextInfo;
 }
 
-// TODO consider debug logging
-// void debugLogging() {
-//    // Note: at this point we are certain the context (m_gl) is current with a surface
-//    const QByteArray debugLoggingMode = qgetenv("QT3DRENDER_DEBUG_LOGGING");
-//    const bool enableDebugLogging = !debugLoggingMode.isEmpty();
-
-//    if (enableDebugLogging && !m_debugLogger) {
-//        if (m_gl->hasExtension("GL_KHR_debug")) {
-//            qCDebug(Backend) << "Qt3D: Enabling OpenGL debug logging";
-//            m_debugLogger.reset(new QOpenGLDebugLogger);
-//            if (m_debugLogger->initialize()) {
-//                QObject::connect(m_debugLogger.data(), &QOpenGLDebugLogger::messageLogged,
-//                &logOpenGLDebugMessage); const QString mode =
-//                QString::fromLocal8Bit(debugLoggingMode);
-//                m_debugLogger->startLogging(mode.startsWith(QLatin1String("sync"),
-//                Qt::CaseInsensitive)
-//                                            ? QOpenGLDebugLogger::SynchronousLogging
-//                                            : QOpenGLDebugLogger::AsynchronousLogging);
-
-//                const auto msgs = m_debugLogger->loggedMessages();
-//                for (const QOpenGLDebugMessage &msg : msgs)
-//                    logOpenGLDebugMessage(msg);
-//            }
-//        } else {
-//            qCDebug(Backend) << "Qt3D: OpenGL debug logging requested but GL_KHR_debug not
-//            supported";
-//        }
-//    }
-//}
-
 /*!
  \brief Renderer lives on a separate thread performs the actual rendering of the RenderCommands
 from each RenderView.
@@ -154,6 +124,8 @@ Renderer::~Renderer()
     m_offscreenHelper->deleteLater();
     if (m_ownedContext)
         m_glContext->deleteLater();
+    if (m_ownedShareContext)
+        m_shareContext->deleteLater();
 }
 
 /*!
@@ -178,16 +150,16 @@ void Renderer::initialize(QOpenGLContext *context)
     }
 
     if (!m_glContext->shareContext()) {
-        // TODO don't forget to delete this
         m_shareContext = new QOpenGLContext;
         m_shareContext->setFormat(m_glContext->format());
         m_shareContext->setShareContext(m_glContext);
         m_shareContext->create();
+        m_ownedShareContext = true;
     } else {
         m_shareContext = m_glContext->shareContext();
     }
 
-    m_offscreenHelper = new OffscreenSurfaceHelper(m_shareContext->format());
+    m_offscreenHelper = new OffscreenSurfaceHelper(m_glContext->format());
     m_offscreenHelper->moveToThread(QCoreApplication::instance()->thread());
 
     QMetaObject::invokeMethod(m_offscreenHelper, "createOffscreenSurface");
@@ -195,11 +167,16 @@ void Renderer::initialize(QOpenGLContext *context)
     m_waitForInitializationToBeCompleted.release();
 }
 
-QOpenGLContext *Renderer::openGLContext()
+QOpenGLContext *Renderer::openGLContext() const
 {
     return m_glContext;
 }
 
+QOpenGLContext *Renderer::shareContext() const
+{
+    return m_shareContext;
+}
+
 void Renderer::addLatestData(FrameInput data)
 {
     QMutexLocker lock(&m_latestDataMutex);
@@ -236,86 +213,54 @@ void Renderer::render()
 
     Q_ASSERT(m_glContext != nullptr);
 
-    //    QOffscreenSurface offscreenSurface;
-    //    offscreenSurface.create();
-    {
-        // Scoped to make sure OpenGLResources go out of scope before the offscreen surface
-        Frame frame;
-        while (m_running.load() > 0) {
-            // TODO we probably need to move this inside doRender...
-            // TODO we should wait for render views to appear :)
-            //        RenderCommand dummy;
-            //        RenderView view;
-            //        view.renderCommands.push_back(std::move(dummy));
-            //        renderViews.push_back(std::move(view));
-
-            frame = doRender(std::move(frame));
-        }
-        // Make sure there is a context to use for the cleanup of resources
-        Q_ASSERT(m_offscreenHelper->offscreenSurface());
-        m_glContext->makeCurrent(m_offscreenHelper->offscreenSurface());
+    Frame frame;
+    while (m_running.load() > 0) {
+        // TODO we probably need to move this inside doRender...
+        // TODO we should wait for render views to appear :)
+        frame = doRender(std::move(frame));
     }
+    // Make sure there is a context to use for the cleanup of resources
+    Q_ASSERT(m_offscreenHelper->offscreenSurface());
+    m_glContext->makeCurrent(m_offscreenHelper->offscreenSurface());
 }
 
-class BoundShader
+void Renderer::checkContextInfoRequested()
 {
-public:
-    BoundShader(const std::shared_ptr<ActivatedSurface> &surface, Immutable<GLShader> shader)
-            : m_shader(shader)
-            , m_surface(surface)
-    {
-        m_shader->shaderProgram->bind();
+    QMutexLocker lock(&m_latestDataMutex);
+    if (m_contextInfoRequested) {
+        // This is the only input/output we allow the renderer
+        // to perform with the aspect apart from the render views.
+        // Everything else, such as uploading textures or shaders
+        // needs to be submitted with the views and happens below,
+        // while preparing rendering.
+        m_writeContextInfoSemaphore.acquire();
+        const RenderViews views = m_requestedContextInfoViews;
+        for (const auto &view : views) {
+            auto *surface = view->surface;
+            Render::SurfaceLocker lock(surface);
+            if (!lock.isSurfaceValid()) {
+                continue;
+            }
+            ActivatedSurface gl(surface, m_glContext, &lock);
+            if (!gl.isValid()) {
+                continue;
+            }
+            m_contextInfo = gl.contextInfo();
+            break;
+        }
+        m_contextInfoRequested = false;
+        m_requestedContextInfoViews = {};
+        m_readContextInfoSemaphore.release();
     }
+}
 
-    ~BoundShader() { m_shader->shaderProgram->release(); }
-
-    Immutable<GLShader> m_shader;
-    std::shared_ptr<ActivatedSurface> m_surface;
-};
-
-// TODO rename, this is not only rendering but also running commands
+// Render and run commands of the frame
 Renderer::Frame Renderer::doRender(Renderer::Frame frame)
 {
-    // TODO this function got uglier than expected, clean it up
     Q_ASSERT(m_glContext != nullptr);
-    Q_ASSERT(m_shareContext != nullptr);
-    //    Q_ASSERT(m_state != nullptr);
-    //    QThread::msleep(250); // TODO replace with semaphore
 
-    {
-        // In case someone has requested context info, we don't render,
-        // but only gather context info
-
-        // TODO this is basically implementing QPromise (for a QFuture),
-        // which perhaps is something to consider actually doing...
-        QMutexLocker lock(&m_latestDataMutex);
-        if (m_contextInfoRequested) {
-            // This is the only input/output we allow the renderer
-            // to perform with the aspect apart from the render views.
-            // Everything else, such as uploading textures or shaders
-            // needs to be submitted with the views and happens below,
-            // while preparing rendering.
-            m_writeContextInfoSemaphore.acquire();
-            GraphicsApiFilterData result;
-            RenderViews views = m_requestedContextInfoViews;
-            for (const auto &view : views) {
-                auto *surface = view->surface;
-                Render::SurfaceLocker lock(surface);
-                if (!lock.isSurfaceValid()) {
-                    continue;
-                }
-                ActivatedSurface gl(surface, m_glContext, &lock);
-                if (!gl.isValid()) {
-                    continue;
-                }
-                m_contextInfo = gl.contextInfo();
-                break;
-            }
-            m_contextInfoRequested = false;
-            m_requestedContextInfoViews = {};
-            m_readContextInfoSemaphore.release();
-        }
-    }
+    // In case someone has requested context info, we gather context info
+    checkContextInfoRequested();
 
     // TODO jobs expect us to receive and upload textures, if that fails below,
     // we're in a position where we do not know about said data anymore...
@@ -377,10 +322,15 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
     RenderStateSet stateSet;
 
     bool preparationsComplete = false;
-    // TODO use an iterator for improved performance
-    for (auto *surface : surfaceViews.keys()) {
+
+    QHashIterator<QSurface *, QVector<SurfaceInfo>> surfaceViewsIterator(surfaceViews);
+    while (surfaceViewsIterator.hasNext()) {
+
+        const auto view = surfaceViewsIterator.next();
+        auto *surface = view.key();
+
         // Preparations need an OpenGL context, but it can be any, so let's pick the first valid surface
-        const auto &surfaceInfos = surfaceViews[surface];
+        const auto &surfaceInfos = view.value();
 
         Render::SurfaceLocker lock(surface);
         if (!lock.isSurfaceValid()) {
@@ -406,8 +356,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                 m_glContext->functions()->glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS,
                                                         &maxTextureUnits);
                 frame.maxTextureImageUnits = maxTextureUnits;
-                //qCDebug(Render::Backend)
-                //    << "context supports" << maxTextureUnits << "texture units";
             }
 
             // TODO not nice to have to take this by reference, but what can we do?
@@ -477,7 +425,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                 return GLBuffer(activeSurface, buffer);
             };
 
-            // TODO rename loaded -> generated
             frame.uploadedBuffers = synchronizeNew(std::move(frame.uploadedBuffers),
                                                    loadedBuffers,
                                                    createGLBufferHelper);
@@ -492,8 +439,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                     tmp.reload(loadedBuffer);
                     return tmp;
                 });
-                //                auto result = glBuffer;
-                //                result->reload(loadedBuffer);
                 return result;
             };
 
@@ -501,19 +446,15 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                                                      loadedBuffers,
                                                      reloadGLBufferHelper);
 
-            QHash<VAOIdentifier, bool> updatedTable;
             // Per-view preparations
             for (const auto &surfaceInfo : surfaceInfos) {
                 const auto &view = surfaceInfo.renderView;
                 const auto &leafNodeId = surfaceInfo.leafNodeId;
                 const auto &commands = frameInput.renderCommands[leafNodeId]->commands;
                 for (const auto &acommand : commands) {
-//                    const auto &command = *acommand->renderCommand;
                     const auto &command = *acommand;
                     if (command.m_type == RenderCommand::Compute) {
-                        // TODO implement compute commands
-                        qWarning()
-                                << "WARNING: Compute commands are not implemented in Dragon renderer yet";
+                        qWarning() << "WARNING: Compute commands are not implemented in Dragon renderer yet";
                         continue;
                     }
 
@@ -542,7 +483,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                 // Create render target
                 if (!view->renderTargetId.isNull()) {
                     auto defaultFBO = m_glContext->defaultFramebufferObject();
-                    // TODO bad name, rename
                     // TODO consider why this is here, iOS apparently
                     auto lastBoundFBOId = activeSurface.glHelper()->boundFrameBufferObject();
                     const auto &renderTargetNodeId = view->renderTargetId;
@@ -588,11 +528,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                             }
                             const auto &glTexture = frame.uploadedTextures[attachment->textureUuid];
                             needsResize |= (glTexture->size() != s);
-
-                            // TODO move this to where we use the render target
-                            // it is needed if we need to read back the buffer
-                            //                            if (attachment->point == QRenderTargetOutput::Color0)
-                            //                                m_renderTargetFormat = glTexture->properties.format;
                         }
 
                         if (needsResize) {
@@ -612,24 +547,12 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
             preparationsComplete = true;
         }
 
-        if (!preparationsComplete)
-            return frame;
+        Q_ASSERT(preparationsComplete);
 
         // TODO might be better to just have to call swapBuffers manually
         bool autoSwapBuffers = m_renderType == RenderType::Threaded;
-        DrawContext drawContext = activeSurface.beginDrawing(autoSwapBuffers);
-
-        // TODO consider what m_ownCurrent was used for in SubmissionContext
-
-        // TODO consider caching per surface again
-        //        if (m_glHelpers[surface] == nullptr) {
-        //            // TODO would be nice if we didn't have to pass this as non-const
-        //            // TODO could perhaps live on ActivatedSurface?
-        //            m_glHelpers.insert(surface, gl.resolveHighestOpenGLFunctions());
-        //        }
-        //        gl.setGlHelper(m_glHelpers[surface]);
+        activeSurface.beginDrawing(autoSwapBuffers);
 
-        // Preparation
         // TODO consider doing preparation and submission in one go
 
         // Lock scene2ds as they might be rendering simultaniously otherwise
@@ -656,15 +579,12 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
             activeSurface.clearBackBuffer(view->clearBackBufferInfo);
             // TODO make it return a viewport, used by other functions that need one
 
-            activeSurface.viewport(view->viewport, view->surfaceSize * view->devicePixelRatio);
+            activeSurface.setViewport(view->viewport, view->surfaceSize * view->devicePixelRatio);
 
             for (const auto &acommand : commands) {
-//                const auto &command = *acommand->renderCommand;
                 const auto &command = *acommand;
                 if (command.m_type == RenderCommand::Compute) {
-                    // TODO implement compute commands
-                    qWarning()
-                            << "WARNING: Compute commands are not implemented in Dragon renderer yet";
+                    qWarning() << "WARNING: Compute commands are not implemented in Dragon renderer yet";
                     continue;
                 }
 
@@ -681,7 +601,6 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                 Q_ASSERT(frame.uploadedVaos.contains(vaoKey));
                 const auto &vao = frame.uploadedVaos[vaoKey];
 
-                //                gl->performDraw(command, glShader, frameInput.attributes, vao);
                 Commands::draw(activeSurface,
                                view,
                                cameraMatrices,
diff --git a/src/runtime/dragon/dragonrenderer_p.h b/src/runtime/dragon/dragonrenderer_p.h
index 7dfc062..8a4a8cd 100644
--- a/src/runtime/dragon/dragonrenderer_p.h
+++ b/src/runtime/dragon/dragonrenderer_p.h
@@ -62,21 +62,6 @@ QT_BEGIN_NAMESPACE
 
 namespace Qt3DRender {
 
-namespace Render {
-// TODO SubmissionContext needs to be replaced because it expects to access the Renderer...
-// While we're here: how much of it is needed?
-class SubmissionContext;
-
-// Consider the need for a shader cache with the new jobs - is it persistent across application
-// runs?
-class ShaderCache;
-
-// TODO do we need the VSyncFrameAdvanceService?
-class VSyncFrameAdvanceService;
-
-// TODO do we need the offscreen surface?
-} // namespace Render
-
 namespace Dragon {
 
 // renderer.h
@@ -86,6 +71,7 @@ struct GLShader;
 struct GLTexture;
 class Renderer;
 class GraphicsHelperInterface;
+
 class RenderThread : public QThread
 {
     Q_OBJECT
@@ -171,11 +157,8 @@ public:
 
     QSemaphore nextFrameSemaphore;
 
-    QOpenGLContext *openGLContext();
-    QOpenGLContext *shareContext()
-    {
-        return m_shareContext;
-    }
+    QOpenGLContext *openGLContext() const;
+    QOpenGLContext *shareContext() const;
 
     void beginShutdown();
     void endShutdown();
@@ -188,7 +171,11 @@ public:
     {
         m_snaggedTextures.clear();
     }
+
 private:
+
+    void checkContextInfoRequested();
+
     // Render thread
     QScopedPointer<RenderThread> m_renderThread;
     QAtomicInt m_running;
@@ -197,9 +184,9 @@ private:
     QMutex m_latestDataMutex;
 
     // Context
-    //    QScopedPointer<Render::SubmissionContext> m_submissionContext;
     QMutex m_shareContextMutex;
     bool m_ownedContext = false;
+    bool m_ownedShareContext = false;
 
     // TODO Protect behind a read-only barrier with a mutex, similar to RenderQueue
     QQueue<FrameInput> m_latestData;
@@ -227,12 +214,6 @@ private:
     QHash<Qt3DCore::QNodeId, QSharedPointer<QOpenGLTexture>> m_snaggedTextures;
 
     RenderType m_renderType;
-
-    //    // Shaders
-    //    Render::ShaderCache *m_shaderCache = nullptr;
-
-    //    // Other
-    //    QScopedPointer<Render::VSyncFrameAdvanceService> m_vsyncFrameAdvanceService;
 };
 
 } // namespace Dragon
diff --git a/src/runtime/dragon/materialsystem/dragonshader_p.h b/src/runtime/dragon/materialsystem/dragonshader_p.h
index cb7899e..d1b223c 100644
--- a/src/runtime/dragon/materialsystem/dragonshader_p.h
+++ b/src/runtime/dragon/materialsystem/dragonshader_p.h
@@ -91,6 +91,7 @@ public:
     // TODO makes no sense here, we need to introduce LoadedShader type
     bool isLoaded() const { return m_isLoaded; }
     void setLoaded(bool loaded) { m_isLoaded = loaded; }
+    // TODO: Shader DNA not really used in dragon, consider removing
     ProgramDNA dna() const Q_DECL_NOTHROW { return m_dna; }
 
     inline QVector<ShaderUniform> uniforms() const { return m_uniforms; }
diff --git a/src/runtime/dragon/renderer/dragonopenglvertexarrayobject_p.h b/src/runtime/dragon/renderer/dragonopenglvertexarrayobject_p.h
index 537b52d..072c1b2 100644
--- a/src/runtime/dragon/renderer/dragonopenglvertexarrayobject_p.h
+++ b/src/runtime/dragon/renderer/dragonopenglvertexarrayobject_p.h
@@ -92,8 +92,6 @@ public:
 
     struct VAOVertexAttribute
     {
-        Mutable<GLBuffer> bufferHandle;
-
         // TODO should we just store the Attribute instead of copying this?
         GLBuffer::Type attributeType;
         int location;