Fix race condition when executing multiple filterlayerjobs at once

The updating of referenced layer ids for each Entity should only be performed
once and not by multiple jobs. Therefore, this update was moved into a
dedicated job which is now a dependency of the filterentityjob instances.

Change-Id: Ie8ecc49a7c6c7d41a1f1f0d18619b5e142b68204
Task-number: QTBUG-68942
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

11 files changed, 225 insertions, 50 deletions

diff --git a/src/render/jobs/filterlayerentityjob.cpp b/src/render/jobs/filterlayerentityjob.cpp
index 0ed2ed559..e206ef968 100644
--- a/src/render/jobs/filterlayerentityjob.cpp
+++ b/src/render/jobs/filterlayerentityjob.cpp
@@ -52,47 +52,6 @@ namespace Render {
 
 namespace {
 int layerFilterJobCounter = 0;
-
-// TO DO: This will be moved to a dedicated job with smarter
-// heuristics in a later commit
-void addLayerIdToEntityChildren(const QVector<Entity *> &children,
-                                const Qt3DCore::QNodeId layerId)
-{
-    for (Entity *child : children) {
-        child->addRecursiveLayerId(layerId);
-        addLayerIdToEntityChildren(child->children(), layerId);
-    }
-}
-
-void updateEntityLayers(NodeManagers *manager)
-{
-    EntityManager *entityManager = manager->renderNodesManager();
-
-    const QVector<HEntity> handles = entityManager->activeHandles();
-
-    // Clear list of recursive layerIds
-    for (const HEntity &handle : handles) {
-        Entity *entity = entityManager->data(handle);
-        entity->clearRecursiveLayerIds();
-    }
-
-    LayerManager *layerManager = manager->layerManager();
-
-    // Set recursive layerIds on children
-    for (const HEntity &handle : handles) {
-        Entity *entity = entityManager->data(handle);
-        const Qt3DCore::QNodeIdVector entityLayers = entity->componentsUuid<Layer>();
-
-        for (const Qt3DCore::QNodeId layerId : entityLayers) {
-            Layer *layer = layerManager->lookupResource(layerId);
-            if (layer->recursive()) {
-                // Find all children of the entity and add the layers to them
-                addLayerIdToEntityChildren(entity->children(), layerId);
-            }
-        }
-    }
-}
-
 } // anonymous
 
 FilterLayerEntityJob::FilterLayerEntityJob()
@@ -107,12 +66,10 @@ void FilterLayerEntityJob::run()
 {
 
     m_filteredEntities.clear();
-    if (hasLayerFilter()) { // LayerFilter set -> filter
-        updateEntityLayers(m_manager);
+    if (hasLayerFilter()) // LayerFilter set -> filter
         filterLayerAndEntity();
-    } else { // No LayerFilter set -> retrieve all
+    else // No LayerFilter set -> retrieve all
         selectAllEntities();
-    }
 
     // sort needed for set_intersection in RenderViewBuilder
     std::sort(m_filteredEntities.begin(), m_filteredEntities.end());
diff --git a/src/render/jobs/job_common_p.h b/src/render/jobs/job_common_p.h
index 648f07a9b..03e2cc90e 100644
--- a/src/render/jobs/job_common_p.h
+++ b/src/render/jobs/job_common_p.h
@@ -106,6 +106,7 @@ namespace JobTypes {
         ProximityFiltering,
         SyncFilterEntityByLayer,
         SyncMaterialGatherer,
+        UpdateLayerEntity
     };
 
 } // JobTypes
diff --git a/src/render/jobs/jobs.pri b/src/render/jobs/jobs.pri
index abb3f605c..472531681 100644
--- a/src/render/jobs/jobs.pri
+++ b/src/render/jobs/jobs.pri
@@ -30,7 +30,8 @@ HEADERS += \
     $$PWD/updateskinningpalettejob_p.h \
     $$PWD/filterproximitydistancejob_p.h \
     $$PWD/abstractpickingjob_p.h \
-    $$PWD/raycastingjob_p.h
+    $$PWD/raycastingjob_p.h \
+    $$PWD/updateentitylayersjob_p.h
 
 SOURCES += \
     $$PWD/updateworldtransformjob.cpp \
@@ -59,5 +60,6 @@ SOURCES += \
     $$PWD/updateskinningpalettejob.cpp \
     $$PWD/filterproximitydistancejob.cpp \
     $$PWD/abstractpickingjob.cpp \
-    $$PWD/raycastingjob.cpp
+    $$PWD/raycastingjob.cpp \
+    $$PWD/updateentitylayersjob.cpp
 
diff --git a/src/render/jobs/updateentitylayersjob.cpp b/src/render/jobs/updateentitylayersjob.cpp
new file mode 100644
index 000000000..1fa34684f
--- /dev/null
+++ b/src/render/jobs/updateentitylayersjob.cpp
@@ -0,0 +1,106 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 Klaralvdalens Datakonsult AB (KDAB).
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl-3.0.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or (at your option) the GNU General
+** Public license version 3 or any later version approved by the KDE Free
+** Qt Foundation. The licenses are as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-2.0.html and
+** https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include "updateentitylayersjob_p.h"
+#include <Qt3DRender/private/managers_p.h>
+#include <Qt3DRender/private/nodemanagers_p.h>
+#include <Qt3DRender/private/entity_p.h>
+#include <Qt3DRender/private/job_common_p.h>
+
+QT_BEGIN_NAMESPACE
+
+namespace Qt3DRender {
+
+namespace Render {
+
+namespace {
+
+void addLayerIdToEntityChildren(const QVector<Entity *> &children,
+                                const Qt3DCore::QNodeId layerId)
+{
+    for (Entity *child : children) {
+        child->addRecursiveLayerId(layerId);
+        addLayerIdToEntityChildren(child->children(), layerId);
+    }
+}
+
+} // anonymous
+
+UpdateEntityLayersJob::UpdateEntityLayersJob()
+    : m_manager(nullptr)
+{
+    SET_JOB_RUN_STAT_TYPE(this, JobTypes::UpdateLayerEntity, 0);
+
+}
+
+void UpdateEntityLayersJob::run()
+{
+    Q_ASSERT(m_manager);
+    EntityManager *entityManager = m_manager->renderNodesManager();
+
+    const QVector<HEntity> handles = entityManager->activeHandles();
+
+    // Clear list of recursive layerIds
+    for (const HEntity &handle : handles) {
+        Entity *entity = entityManager->data(handle);
+        entity->clearRecursiveLayerIds();
+    }
+
+    LayerManager *layerManager = m_manager->layerManager();
+
+    // Set recursive layerIds on children
+    for (const HEntity &handle : handles) {
+        Entity *entity = entityManager->data(handle);
+        const Qt3DCore::QNodeIdVector entityLayers = entity->componentsUuid<Layer>();
+
+        for (const Qt3DCore::QNodeId layerId : entityLayers) {
+            Layer *layer = layerManager->lookupResource(layerId);
+            if (layer->recursive()) {
+                // Find all children of the entity and add the layers to them
+                addLayerIdToEntityChildren(entity->children(), layerId);
+            }
+        }
+    }
+}
+
+} // Render
+
+} // Qt3DRender
+
+QT_END_NAMESPACE
diff --git a/src/render/jobs/updateentitylayersjob_p.h b/src/render/jobs/updateentitylayersjob_p.h
new file mode 100644
index 000000000..8c73899d9
--- /dev/null
+++ b/src/render/jobs/updateentitylayersjob_p.h
@@ -0,0 +1,92 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 Klaralvdalens Datakonsult AB (KDAB).
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl-3.0.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or (at your option) the GNU General
+** Public license version 3 or any later version approved by the KDE Free
+** Qt Foundation. The licenses are as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-2.0.html and
+** https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+
+#ifndef QT3DRENDER_RENDER_UPDATEENTITYLAYERSJOB_P_H
+#define QT3DRENDER_RENDER_UPDATEENTITYLAYERSJOB_P_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists for the convenience
+// of other Qt classes.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <Qt3DRender/private/qt3drender_global_p.h>
+#include <Qt3DCore/qaspectjob.h>
+
+QT_BEGIN_NAMESPACE
+
+namespace Qt3DRender {
+
+namespace Render {
+
+class Entity;
+class NodeManagers;
+
+
+class QT3DRENDERSHARED_PRIVATE_EXPORT UpdateEntityLayersJob: public Qt3DCore::QAspectJob
+{
+public:
+    UpdateEntityLayersJob();
+
+    inline void setManager(NodeManagers *manager) { m_manager = manager; }
+    inline NodeManagers *manager() const { return m_manager; }
+
+    // QAspectJob interface
+    void run() final;
+
+private:
+    NodeManagers *m_manager;
+};
+
+
+typedef QSharedPointer<UpdateEntityLayersJob> UpdateEntityLayersJobPtr;
+
+} // Render
+
+} // Qt3DRender
+
+QT_END_NAMESPACE
+
+#endif // QT3DRENDER_RENDER_UPDATEENTITYLAYERSJOB_P_H
diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp
index 3df3883b7..f04b864a3 100644
--- a/src/render/renderers/opengl/renderer/renderer.cpp
+++ b/src/render/renderers/opengl/renderer/renderer.cpp
@@ -190,6 +190,7 @@ Renderer::Renderer(QRenderAspect::RenderType type)
     , m_updateLevelOfDetailJob(Render::UpdateLevelOfDetailJobPtr::create())
     , m_updateMeshTriangleListJob(Render::UpdateMeshTriangleListJobPtr::create())
     , m_filterCompatibleTechniqueJob(Render::FilterCompatibleTechniqueJobPtr::create())
+    , m_updateEntityLayersJob(Render::UpdateEntityLayersJobPtr::create())
     , m_bufferGathererJob(Render::GenericLambdaJobPtr<std::function<void ()>>::create([this] { lookForDirtyBuffers(); }, JobTypes::DirtyBufferGathering))
     , m_vaoGathererJob(Render::GenericLambdaJobPtr<std::function<void ()>>::create([this] { lookForAbandonedVaos(); }, JobTypes::DirtyVaoGathering))
     , m_textureGathererJob(Render::GenericLambdaJobPtr<std::function<void ()>>::create([this] { lookForDirtyTextures(); }, JobTypes::DirtyTextureGathering))
@@ -297,6 +298,7 @@ void Renderer::setNodeManagers(NodeManagers *managers)
     m_updateSkinningPaletteJob->setManagers(m_nodesManager);
     m_updateMeshTriangleListJob->setManagers(m_nodesManager);
     m_filterCompatibleTechniqueJob->setManager(m_nodesManager->techniqueManager());
+    m_updateEntityLayersJob->setManager(m_nodesManager);
 }
 
 void Renderer::setServices(QServiceLocator *services)
@@ -1634,6 +1636,10 @@ QVector<Qt3DCore::QAspectJobPtr> Renderer::renderBinJobs()
     const bool layersCacheNeedsToBeRebuilt = layersDirty || entitiesEnabledDirty;
     const bool materialDirty = dirtyBitsForFrame & AbstractRenderer::MaterialDirty;
 
+    // Rebuild Entity Layers list if layers are dirty
+    if (layersDirty)
+        renderBinJobs.push_back(m_updateEntityLayersJob);
+
     QMutexLocker lock(m_renderQueue->mutex());
     if (m_renderQueue->wasReset()) { // Have we rendered yet? (Scene3D case)
         // Traverse the current framegraph. For each leaf node create a
diff --git a/src/render/renderers/opengl/renderer/renderer_p.h b/src/render/renderers/opengl/renderer/renderer_p.h
index bf9554a54..826d069c3 100644
--- a/src/render/renderers/opengl/renderer/renderer_p.h
+++ b/src/render/renderers/opengl/renderer/renderer_p.h
@@ -77,6 +77,7 @@
 #include <Qt3DRender/private/updatemeshtrianglelistjob_p.h>
 #include <Qt3DRender/private/filtercompatibletechniquejob_p.h>
 #include <Qt3DRender/private/updateskinningpalettejob_p.h>
+#include <Qt3DRender/private/updateentitylayersjob_p.h>
 #include <Qt3DRender/private/renderercache_p.h>
 
 #include <QHash>
@@ -215,6 +216,7 @@ public:
     inline IntrospectShadersJobPtr introspectShadersJob() const { return m_introspectShaderJob; }
     inline Qt3DCore::QAspectJobPtr bufferGathererJob() const { return m_bufferGathererJob; }
     inline Qt3DCore::QAspectJobPtr textureGathererJob() const { return m_textureGathererJob; }
+    inline UpdateEntityLayersJobPtr updateEntityLayersJob() const { return m_updateEntityLayersJob; }
 
     Qt3DCore::QAbstractFrameAdvanceService *frameAdvanceService() const override;
 
@@ -355,6 +357,7 @@ private:
     UpdateLevelOfDetailJobPtr m_updateLevelOfDetailJob;
     UpdateMeshTriangleListJobPtr m_updateMeshTriangleListJob;
     FilterCompatibleTechniqueJobPtr m_filterCompatibleTechniqueJob;
+    UpdateEntityLayersJobPtr m_updateEntityLayersJob;
 
     QVector<Qt3DCore::QNodeId> m_pendingRenderCaptureSendRequests;
 
diff --git a/src/render/renderers/opengl/renderer/renderviewbuilder.cpp b/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
index 515767caa..c256337db 100644
--- a/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
+++ b/src/render/renderers/opengl/renderer/renderviewbuilder.cpp
@@ -581,6 +581,7 @@ QVector<Qt3DCore::QAspectJobPtr> RenderViewBuilder::buildJobHierachy() const
     jobs.push_back(m_syncRenderViewInitializationJob); // Step 2
 
     if (m_layerCacheNeedsToBeRebuilt) {
+        m_filterEntityByLayerJob->addDependency(m_renderer->updateEntityLayersJob());
         m_filterEntityByLayerJob->addDependency(m_syncRenderViewInitializationJob);
         m_filterEntityByLayerJob->addDependency(m_renderer->updateTreeEnabledJob());
 
diff --git a/tests/auto/render/layerfiltering/tst_layerfiltering.cpp b/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
index c2651c477..eeffc69b2 100644
--- a/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
+++ b/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
@@ -46,9 +46,11 @@ private Q_SLOTS:
     {
         // GIVEN
         Qt3DRender::Render::FilterLayerEntityJob filterJob;
+        Qt3DRender::Render::UpdateEntityLayersJob updateEntityLayerJob;
         Qt3DRender::QLayer frontendLayer;
 
         // THEN
+        QVERIFY(updateEntityLayerJob.manager() == nullptr);
         QCOMPARE(filterJob.hasLayerFilter(), false);
         QCOMPARE(filterJob.filteredEntities().size(), 0);
         QCOMPARE(filterJob.layerFilters().size(), 0);
@@ -635,6 +637,10 @@ private Q_SLOTS:
         updateTreeEnabledJob.run();
 
         // WHEN
+        Qt3DRender::Render::UpdateEntityLayersJob updateLayerEntityJob;
+        updateLayerEntityJob.setManager(aspect->nodeManagers());
+        updateLayerEntityJob.run();
+
         Qt3DRender::Render::FilterLayerEntityJob filterJob;
         filterJob.setLayerFilters(layerFilterIds);
         filterJob.setManager(aspect->nodeManagers());
diff --git a/tests/auto/render/renderer/tst_renderer.cpp b/tests/auto/render/renderer/tst_renderer.cpp
index d5961e9fd..9f6007181 100644
--- a/tests/auto/render/renderer/tst_renderer.cpp
+++ b/tests/auto/render/renderer/tst_renderer.cpp
@@ -212,8 +212,8 @@ private Q_SLOTS:
                  1 + // VAOGatherer
                  1 + // BufferGathererJob
                  1 + // TexturesGathererJob
-                 1   // SyncTextureLoadingJob
-                 );
+                 1 + // SyncTextureLoadingJob
+                 1); // UpdateEntityLayersJob
 
         renderer.clearDirtyBits(Qt3DRender::Render::AbstractRenderer::AllDirty);
 
diff --git a/tests/auto/render/renderviewbuilder/tst_renderviewbuilder.cpp b/tests/auto/render/renderviewbuilder/tst_renderviewbuilder.cpp
index cb51012c1..81bca0158 100644
--- a/tests/auto/render/renderviewbuilder/tst_renderviewbuilder.cpp
+++ b/tests/auto/render/renderviewbuilder/tst_renderviewbuilder.cpp
@@ -352,7 +352,8 @@ private Q_SLOTS:
             QCOMPARE(renderViewBuilder.syncRenderViewInitializationJob()->dependencies().first().data(), renderViewBuilder.renderViewJob().data());
 
             // Step 3
-            QCOMPARE(renderViewBuilder.filterEntityByLayerJob()->dependencies().size(), 2);
+            QCOMPARE(renderViewBuilder.filterEntityByLayerJob()->dependencies().size(), 3);
+            QVERIFY(renderViewBuilder.filterEntityByLayerJob()->dependencies().contains(testAspect.renderer()->updateEntityLayersJob()));
             QVERIFY(renderViewBuilder.filterEntityByLayerJob()->dependencies().contains(renderViewBuilder.syncRenderViewInitializationJob()));
             QVERIFY(renderViewBuilder.filterEntityByLayerJob()->dependencies().contains(testAspect.renderer()->updateTreeEnabledJob()));