From 784b9163068838335f6933dcb2f5821b11dc65b5 Mon Sep 17 00:00:00 2001 From: "Eunki, Hong" Date: Fri, 12 May 2023 13:15:13 +0900 Subject: [PATCH] Fix invalidated PipelineCacheL2 pointer problem Let we keep std::list::iterator instead of PipelineCacheL2*. So, the pointer of pipeline what render-renderer has should be valid even if cache container size changed. And also, for safe issue, let we keep the pipeline container as std::list. Change-Id: I5a0572f040ff3304876bdb2483af85ff26fbccbb Signed-off-by: Eunki, Hong --- .../utc-Dali-Internal-PipelineCache.cpp | 2 +- dali/internal/render/renderers/pipeline-cache.cpp | 87 ++++++++++------------ dali/internal/render/renderers/pipeline-cache.h | 40 ++++++---- dali/internal/render/renderers/render-renderer.cpp | 18 ++++- dali/internal/render/renderers/render-renderer.h | 7 +- 5 files changed, 87 insertions(+), 67 deletions(-) diff --git a/automated-tests/src/dali-internal/utc-Dali-Internal-PipelineCache.cpp b/automated-tests/src/dali-internal/utc-Dali-Internal-PipelineCache.cpp index 352bce9..0d3e9f6 100644 --- a/automated-tests/src/dali-internal/utc-Dali-Internal-PipelineCache.cpp +++ b/automated-tests/src/dali-internal/utc-Dali-Internal-PipelineCache.cpp @@ -153,7 +153,7 @@ int UtcDaliCorePipelineCacheTest(void) { for(auto& iterLevel1 : iterLevel0.level1nodes) { - if(iterLevel1.noBlend.pipeline != nullptr) + if(!iterLevel1.noBlends.empty() && iterLevel1.noBlends.begin()->pipeline != nullptr) { noBlendFoundCount++; } diff --git a/dali/internal/render/renderers/pipeline-cache.cpp b/dali/internal/render/renderers/pipeline-cache.cpp index 92fe22b..9839d95 100644 --- a/dali/internal/render/renderers/pipeline-cache.cpp +++ b/dali/internal/render/renderers/pipeline-cache.cpp @@ -192,11 +192,9 @@ constexpr Graphics::BlendOp ConvertBlendEquation(DevelBlendEquation::Type blendE } } // namespace -PipelineCacheL0* PipelineCache::GetPipelineCacheL0(std::size_t hash, Program* program, Render::Geometry* geometry) +PipelineCacheL0Ptr PipelineCache::GetPipelineCacheL0(std::size_t hash, Program* program, Render::Geometry* geometry) { - auto it = std::find_if(level0nodes.begin(), level0nodes.end(), [hash, program, geometry](PipelineCacheL0& item) { - return ((item.hash == hash && item.program == program && item.geometry == geometry)); - }); + auto it = std::find_if(level0nodes.begin(), level0nodes.end(), [hash, program, geometry](PipelineCacheL0& item) { return ((item.hash == hash && item.program == program && item.geometry == geometry)); }); // Add new node to cache if(it == level0nodes.end()) @@ -249,14 +247,14 @@ PipelineCacheL0* PipelineCache::GetPipelineCacheL0(std::size_t hash, Program* pr level0.program = program; level0.geometry = geometry; level0.inputState = vertexInputState; - level0nodes.emplace_back(std::move(level0)); - it = level0nodes.end() - 1; + + it = level0nodes.insert(level0nodes.end(), std::move(level0)); } - return &*it; + return it; } -PipelineCacheL1* PipelineCacheL0::GetPipelineCacheL1(Render::Renderer* renderer, bool usingReflection) +PipelineCacheL1Ptr PipelineCacheL0::GetPipelineCacheL1(Render::Renderer* renderer, bool usingReflection) { // hash must be collision free uint32_t hash = 0; @@ -301,11 +299,8 @@ PipelineCacheL1* PipelineCacheL0::GetPipelineCacheL1(Render::Renderer* renderer, hash = (topo & 0xffu) | ((cull << 8u) & 0xff00u) | ((uint32_t(poly) << 16u) & 0xff0000u); // If L1 not found by hash, create rasterization state describing pipeline and store it - auto it = std::find_if(level1nodes.begin(), level1nodes.end(), [hash](PipelineCacheL1& item) { - return item.hashCode == hash; - }); + auto it = std::find_if(level1nodes.begin(), level1nodes.end(), [hash](PipelineCacheL1& item) { return item.hashCode == hash; }); - PipelineCacheL1* retval = nullptr; if(it == level1nodes.end()) { PipelineCacheL1 item; @@ -314,14 +309,11 @@ PipelineCacheL1* PipelineCacheL0::GetPipelineCacheL1(Render::Renderer* renderer, item.rs.frontFace = Graphics::FrontFace::COUNTER_CLOCKWISE; item.rs.polygonMode = poly; // not in use item.ia.topology = geometry->GetTopology(); - level1nodes.emplace_back(std::move(item)); - retval = &level1nodes.back(); - } - else - { - retval = &*it; + + it = level1nodes.insert(level1nodes.end(), std::move(item)); } - return retval; + + return it; } void PipelineCacheL0::ClearUnusedCache() @@ -339,29 +331,34 @@ void PipelineCacheL0::ClearUnusedCache() } } -PipelineCacheL2* PipelineCacheL1::GetPipelineCacheL2(bool blend, bool premul, BlendingOptions& blendingOptions) +PipelineCacheL2Ptr PipelineCacheL1::GetPipelineCacheL2(bool blend, bool premul, BlendingOptions& blendingOptions) { // early out if(!blend) { + if(DALI_UNLIKELY(noBlends.empty())) + { + noBlends.emplace_back(PipelineCacheL2{}); + } + + auto& noBlend = *noBlends.begin(); + if(noBlend.pipeline == nullptr) { // reset all before returning if pipeline has never been created for that case noBlend.hash = 0; memset(&noBlend.colorBlendState, 0, sizeof(Graphics::ColorBlendState)); } - return &noBlend; + return noBlends.begin(); } auto bitmask = uint32_t(blendingOptions.GetBitmask()); // Find by bitmask (L2 entries must be sorted by bitmask) - auto it = std::find_if(level2nodes.begin(), level2nodes.end(), [bitmask](PipelineCacheL2& item) { - return item.hash == bitmask; - }); + auto it = std::find_if(level2nodes.begin(), level2nodes.end(), [bitmask](PipelineCacheL2& item) { return item.hash == bitmask; }); // TODO: find better way of blend constants lookup - PipelineCacheL2* retval = nullptr; + PipelineCacheL2Ptr retval = level2nodes.end(); if(it != level2nodes.end()) { bool hasBlendColor = blendingOptions.GetBlendColor(); @@ -370,20 +367,20 @@ PipelineCacheL2* PipelineCacheL1::GetPipelineCacheL2(bool blend, bool premul, Bl Vector4 v(it->colorBlendState.blendConstants); if(v == *blendingOptions.GetBlendColor()) { - retval = &*it; + retval = it; } ++it; } if(!hasBlendColor) { - retval = &*it; + retval = it; } } - if(!retval) + if(retval == level2nodes.end()) { // create new entry and return it with null pipeline - PipelineCacheL2 l2; + PipelineCacheL2 l2{}; l2.pipeline = nullptr; auto& colorBlendState = l2.colorBlendState; colorBlendState.SetBlendEnable(true); @@ -414,11 +411,10 @@ PipelineCacheL2* PipelineCacheL1::GetPipelineCacheL2(bool blend, bool premul, Bl } l2.hash = blendingOptions.GetBitmask(); - level2nodes.emplace_back(std::move(l2)); - std::sort(level2nodes.begin(), level2nodes.end(), [](PipelineCacheL2& lhs, PipelineCacheL2& rhs) { - return lhs.hash < rhs.hash; - }); + auto upperBound = std::upper_bound(level2nodes.begin(), level2nodes.end(), l2, [](const PipelineCacheL2& lhs, const PipelineCacheL2& rhs) { return lhs.hash < rhs.hash; }); + + level2nodes.insert(upperBound, std::move(l2)); // run same function to retrieve retval retval = GetPipelineCacheL2(blend, premul, blendingOptions); @@ -429,11 +425,6 @@ PipelineCacheL2* PipelineCacheL1::GetPipelineCacheL2(bool blend, bool premul, Bl bool PipelineCacheL1::ClearUnusedCache() { - if(noBlend.referenceCount > 0) - { - return false; - } - for(auto iter = level2nodes.begin(); iter != level2nodes.end();) { if(iter->referenceCount == 0) @@ -446,6 +437,11 @@ bool PipelineCacheL1::ClearUnusedCache() } } + if(!noBlends.empty() && noBlends.begin()->referenceCount > 0) + { + return false; + } + return level2nodes.empty(); } @@ -502,9 +498,10 @@ PipelineResult PipelineCache::GetPipeline(const PipelineCacheQueryInfo& queryInf return mLatestResult[latestUsedCacheIndex]; } - auto* level0 = GetPipelineCacheL0(queryInfo.hash, queryInfo.program, queryInfo.geometry); - auto* level1 = level0->GetPipelineCacheL1(queryInfo.renderer, queryInfo.cameraUsingReflection); - auto* level2 = level1->GetPipelineCacheL2(queryInfo.blendingEnabled, queryInfo.alphaPremultiplied, *queryInfo.blendingOptions); + auto level0 = GetPipelineCacheL0(queryInfo.hash, queryInfo.program, queryInfo.geometry); + auto level1 = level0->GetPipelineCacheL1(queryInfo.renderer, queryInfo.cameraUsingReflection); + + PipelineCachePtr level2 = level1->GetPipelineCacheL2(queryInfo.blendingEnabled, queryInfo.alphaPremultiplied, *queryInfo.blendingOptions); // Create new pipeline at level2 if requested if(level2->pipeline == nullptr && createNewIfNotFound) @@ -573,12 +570,10 @@ void PipelineCache::ClearUnusedCache() } } -void PipelineCache::ResetPipeline(PipelineCacheL2* pipelineCache) +void PipelineCache::ResetPipeline(PipelineCachePtr pipelineCache) { - if(pipelineCache) - { - pipelineCache->referenceCount--; - } + // TODO : Can we always assume that pipelineCache input is valid iterator? + pipelineCache->referenceCount--; } } // namespace Dali::Internal::Render diff --git a/dali/internal/render/renderers/pipeline-cache.h b/dali/internal/render/renderers/pipeline-cache.h index 659cecc..c153716 100644 --- a/dali/internal/render/renderers/pipeline-cache.h +++ b/dali/internal/render/renderers/pipeline-cache.h @@ -22,9 +22,7 @@ #include #include #include - -// EXTERNAL INCLUDES -#include +#include namespace Dali::Internal { @@ -34,6 +32,18 @@ namespace Render class Renderer; class Geometry; +struct PipelineCacheL2; +struct PipelineCacheL1; +struct PipelineCacheL0; +using PipelineCacheL2Container = std::list; +using PipelineCacheL1Container = std::list; +using PipelineCacheL0Container = std::list; +using PipelineCacheL2Ptr = PipelineCacheL2Container::iterator; +using PipelineCacheL1Ptr = PipelineCacheL1Container::iterator; +using PipelineCacheL0Ptr = PipelineCacheL0Container::iterator; + +using PipelineCachePtr = PipelineCacheL2Ptr; + /** * Cache Level 2 : Last level of cache, stores actual pipeline */ @@ -41,8 +51,8 @@ struct PipelineCacheL2 { uint32_t hash{}; uint32_t referenceCount{0u}; - Graphics::ColorBlendState colorBlendState; - Graphics::UniquePtr pipeline; + Graphics::ColorBlendState colorBlendState{}; + Graphics::UniquePtr pipeline{}; }; /** @@ -50,7 +60,7 @@ struct PipelineCacheL2 */ struct PipelineCacheL1 { - PipelineCacheL2* GetPipelineCacheL2(bool blend, bool premul, BlendingOptions& blendingOptions); + PipelineCacheL2Ptr GetPipelineCacheL2(bool blend, bool premul, BlendingOptions& blendingOptions); /** * @brief Clear unused caches. @@ -61,8 +71,8 @@ struct PipelineCacheL1 Graphics::RasterizationState rs{}; Graphics::InputAssemblyState ia{}; - PipelineCacheL2 noBlend; // special case - std::vector level2nodes; + PipelineCacheL2Container noBlends; // special case + PipelineCacheL2Container level2nodes; }; /** @@ -70,7 +80,7 @@ struct PipelineCacheL1 */ struct PipelineCacheL0 // L0 cache { - PipelineCacheL1* GetPipelineCacheL1(Render::Renderer* renderer, bool usingReflection); + PipelineCacheL1Ptr GetPipelineCacheL1(Render::Renderer* renderer, bool usingReflection); /** * @brief Clear unused caches. @@ -82,7 +92,7 @@ struct PipelineCacheL0 // L0 cache Program* program{}; Graphics::VertexInputState inputState; - std::vector level1nodes; + PipelineCacheL1Container level1nodes; }; struct PipelineCacheQueryInfo @@ -115,7 +125,7 @@ struct PipelineCacheQueryInfo struct PipelineResult { Graphics::Pipeline* pipeline; - PipelineCacheL2* level2; + PipelineCachePtr level2; }; /** @@ -133,7 +143,7 @@ public: /** * Retrieves next cache level */ - PipelineCacheL0* GetPipelineCacheL0(std::size_t hash, Program* program, Render::Geometry* geometry); + PipelineCacheL0Ptr GetPipelineCacheL0(std::size_t hash, Program* program, Render::Geometry* geometry); /** * Retrieves pipeline matching queryInfo struct @@ -162,7 +172,7 @@ public: * @brief Decrease the reference count of the pipeline cache. * @param pipelineCache The pipeline cache to decrease the reference count */ - void ResetPipeline(PipelineCacheL2* pipelineCache); + void ResetPipeline(PipelineCachePtr pipelineCache); private: /** @@ -181,8 +191,8 @@ private: void ClearUnusedCache(); private: - Graphics::Controller* graphicsController{nullptr}; - std::vector level0nodes; + Graphics::Controller* graphicsController{nullptr}; + PipelineCacheL0Container level0nodes; // Cache latest queries whether blend enabled or not. // (Since most UI case (like Text and Image) enable blend, and most 3D case disable blend.) diff --git a/dali/internal/render/renderers/render-renderer.cpp b/dali/internal/render/renderers/render-renderer.cpp index 486eb2f..1ccfc43 100644 --- a/dali/internal/render/renderers/render-renderer.cpp +++ b/dali/internal/render/renderers/render-renderer.cpp @@ -226,7 +226,8 @@ Renderer::Renderer(SceneGraph::RenderDataProvider* dataProvider, mDepthWriteMode(depthWriteMode), mDepthTestMode(depthTestMode), mPremultipliedAlphaEnabled(preMultipliedAlphaEnabled), - mShaderChanged(false) + mShaderChanged(false), + mPipelineCached(false) { if(blendingBitmask != 0u) { @@ -248,7 +249,11 @@ void Renderer::Initialize(Graphics::Controller& graphicsController, ProgramCache Renderer::~Renderer() { // Reset old pipeline - mPipelineCache->ResetPipeline(mPipeline); + if(DALI_LIKELY(mPipelineCached)) + { + mPipelineCache->ResetPipeline(mPipeline); + mPipelineCached = false; + } } void Renderer::operator delete(void* ptr) @@ -972,12 +977,17 @@ Graphics::Pipeline& Renderer::PrepareGraphicsPipeline( queryInfo.GenerateHash(); // Reset old pipeline - mPipelineCache->ResetPipeline(mPipeline); + if(DALI_LIKELY(mPipelineCached)) + { + mPipelineCache->ResetPipeline(mPipeline); + mPipelineCached = false; + } // Find or generate new pipeline. auto pipelineResult = mPipelineCache->GetPipeline(queryInfo, true); - mPipeline = pipelineResult.level2; + mPipeline = pipelineResult.level2; + mPipelineCached = true; // should be never null? return *pipelineResult.pipeline; diff --git a/dali/internal/render/renderers/render-renderer.h b/dali/internal/render/renderers/render-renderer.h index 202277f..2c6b806 100644 --- a/dali/internal/render/renderers/render-renderer.h +++ b/dali/internal/render/renderers/render-renderer.h @@ -19,6 +19,7 @@ */ // INTERNAL INCLUDES +#include #include #include #include @@ -61,6 +62,9 @@ class PipelineCacheL2; class UniformBufferManager; class Renderer; +using PipelineCacheL2Container = std::list; +using PipelineCachePtr = PipelineCacheL2Container::iterator; + using RendererKey = MemoryPoolKey; } //namespace Render } //namespace Internal @@ -596,7 +600,7 @@ private: std::vector mUniformBufferBindings{}; Render::PipelineCache* mPipelineCache{nullptr}; - PipelineCacheL2* mPipeline{nullptr}; + PipelineCachePtr mPipeline{}; using Hash = std::size_t; @@ -641,6 +645,7 @@ private: DepthTestMode::Type mDepthTestMode : 3; ///< The depth test mode bool mPremultipliedAlphaEnabled : 1; ///< Flag indicating whether the Pre-multiplied Alpha Blending is required bool mShaderChanged : 1; ///< Flag indicating the shader changed and uniform maps have to be updated + bool mPipelineCached : 1; ///< Flag indicating whether renderer cache valid pipeline or not. std::vector mDrawCommands; // Devel stuff RenderCallback* mRenderCallback{nullptr}; -- 2.7.4