RenderViewBuilder: fix leak with EntityRenderCommandData

EntityRenderCommandData would only be released in a separate job than the one
it was allocated in. However this would only happen if there was at least a
single renderable objects after filtering. In case there was no renderable
following filtering, the EntityRenderCommandData was leaked.

To fix the issue and make it less error prone, we now switch to using shared
pointers to avoid having to handle all possible leak cases with raw pointers.

Change-Id: I842d50d2b35ebba8303f6d6c4e72a2427ce31da3
Reviewed-by: Mike Krus <mike.krus@kdab.com>

4 files changed, 14 insertions, 26 deletions

diff --git a/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob.cpp b/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob.cpp
index 6d6ae7853..6cc2105dd 100644
--- a/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob.cpp
+++ b/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob.cpp
@@ -58,7 +58,7 @@ RenderViewCommandUpdaterJob::RenderViewCommandUpdaterJob()
     , m_count(0)
     , m_renderView(nullptr)
     , m_renderer(nullptr)
-    , m_renderables(nullptr)
+    , m_renderables()
 {
     SET_JOB_RUN_STAT_TYPE(this, JobTypes::RenderCommandUpdater, renderViewInstanceCounter++);
 }
@@ -71,7 +71,7 @@ void RenderViewCommandUpdaterJob::run()
         if (m_count == 0)
             return;
         // Update Render Commands (Uniform Change, Depth Change)
-        m_renderView->updateRenderCommand(m_renderables, m_offset, m_count);
+        m_renderView->updateRenderCommand(m_renderables.data(), m_offset, m_count);
     }
 }
 
diff --git a/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob_p.h b/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob_p.h
index e6df3f3b3..d7f424966 100644
--- a/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob_p.h
+++ b/src/render/renderers/opengl/jobs/renderviewcommandupdaterjob_p.h
@@ -71,13 +71,13 @@ public:
 
     inline void setRenderView(RenderView *rv) Q_DECL_NOTHROW { m_renderView = rv; }
     inline void setRenderer(Renderer *renderer) Q_DECL_NOTHROW { m_renderer = renderer; }
-    inline void setRenderables(EntityRenderCommandData *renderables, int offset, int count) Q_DECL_NOTHROW
+    inline void setRenderables(const EntityRenderCommandDataPtr &renderables, int offset, int count) Q_DECL_NOTHROW
     {
         m_offset = offset;
         m_count = count;
         m_renderables = renderables;
     }
-    EntityRenderCommandData *renderables() const { return m_renderables; }
+    EntityRenderCommandDataPtr renderables() const { return m_renderables; }
 
     QVector<RenderCommand> &commands() Q_DECL_NOTHROW { return m_commands; }
 
@@ -88,7 +88,7 @@ private:
     int m_count;
     RenderView *m_renderView;
     Renderer *m_renderer;
-    EntityRenderCommandData *m_renderables;
+    EntityRenderCommandDataPtr m_renderables;
     QVector<RenderCommand> m_commands;
 };
 
diff --git a/src/render/renderers/opengl/renderer/rendercommand_p.h b/src/render/renderers/opengl/renderer/rendercommand_p.h
index 0180d6996..be00fb753 100644
--- a/src/render/renderers/opengl/renderer/rendercommand_p.h
+++ b/src/render/renderers/opengl/renderer/rendercommand_p.h
@@ -158,14 +158,6 @@ struct EntityRenderCommandData
         passesData.push_back(std::move(p));
     }
 
-    EntityRenderCommandData &operator+=(const EntityRenderCommandData &t)
-    {
-        entities += t.entities;
-        commands += t.commands;
-        passesData += t.passesData;
-        return *this;
-    }
-
     EntityRenderCommandData &operator+=(EntityRenderCommandData &&t)
     {
         entities += std::move(t.entities);
@@ -174,14 +166,10 @@ struct EntityRenderCommandData
         return *this;
     }
 
-    EntityRenderCommandData mid(int idx, int len) const
-    {
-        return { entities.mid(idx, len), commands.mid(idx, len), passesData.mid(idx, len) };
-    }
-
-
 };
 
+using EntityRenderCommandDataPtr = QSharedPointer<EntityRenderCommandData>;
+
 
 } // namespace Render
 
diff --git a/src/render/renderers/opengl/renderer/renderviewbuilder.cpp b/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
index 9f7589ecc..8f1b17119 100644
--- a/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
+++ b/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
@@ -125,14 +125,10 @@ public:
         // Append all the commands and sort them
         RenderView *rv = m_renderViewJob->renderView();
 
-        const EntityRenderCommandData *commandData = m_renderViewCommandUpdaterJobs.first()->renderables();
+        const EntityRenderCommandDataPtr commandData = m_renderViewCommandUpdaterJobs.first()->renderables();
 
-        if (commandData != nullptr) {
+        if (commandData) {
             const QVector<RenderCommand> commands = std::move(commandData->commands);
-
-            // TO DO: Cache the EntityRenderCommandData so that it can be reused if nothing in the scene has changed
-            delete commandData;
-
             rv->setCommands(commands);
 
             // TO DO: Find way to store commands once or at least only when required
@@ -320,9 +316,13 @@ public:
                     renderableEntities = RenderViewBuilder::entitiesInSubset(renderableEntities, m_filterProximityJob->filteredEntities());
             }
 
+            // Early return in case we have nothing to filter
+            if (renderableEntities.size() == 0)
+                return;
+
             // Filter out Render commands for which the Entity wasn't selected because
             // of frustum, proximity or layer filtering
-            EntityRenderCommandData *filteredCommandData = new EntityRenderCommandData();
+            EntityRenderCommandDataPtr filteredCommandData = EntityRenderCommandDataPtr::create();
             filteredCommandData->reserve(renderableEntities.size());
             // Because dataCacheForLeaf.renderableEntities or computeEntities are sorted
             // What we get out of EntityRenderCommandData is also sorted by Entity