MemoryLeak for Mesh::Vao in GraphicsStateObject

[KaChen-Dex]

Hi, We found a memory leak in Falcor: The deleted mesh doesn't free Vao object because it is cached in GraphicsStateObject in Graph. This has been a problem for us because we support streaming meshes and the system eventually out of GPU memory after a while.

I attached the patch here. Can you take a look and see if it can be integrated into Github?

thanks.

diff --git a/Framework/Source/API/GraphicsStateObject.cpp b/Framework/Source/API/GraphicsStateObject.cpp
index 27004560..9e9ae621 100644
--- a/Framework/Source/API/GraphicsStateObject.cpp
+++ b/Framework/Source/API/GraphicsStateObject.cpp
@@ -74,7 +74,10 @@ namespace Falcor
         {
             b = b && (other.mpDepthStencilState == nullptr || other.mpDepthStencilState == spDefaultDepthStencilState);
         }
-
+// FixLeak: Check PrimitiveTopology as it is built into PipelineStateObject on Vulkan.
+#ifdef FALCOR_VK
+        b = b && (mVaoTopology == other.mVaoTopology);
+#endif
         return b;
     }
 
diff --git a/Framework/Source/API/GraphicsStateObject.h b/Framework/Source/API/GraphicsStateObject.h
index 76eff4e3..b306a113 100644
--- a/Framework/Source/API/GraphicsStateObject.h
+++ b/Framework/Source/API/GraphicsStateObject.h
@@ -105,12 +105,17 @@ namespace Falcor
 
 #ifdef FALCOR_VK
         public:
-            Desc& setVao(const Vao::SharedConstPtr& pVao) { mpVao = pVao; return *this; }
+            // FixLeak: fix leak  on Mesh::Vao --- Begin
+            // Desc& setVao(const Vao::SharedConstPtr& pVao) { mpVao = pVao; return *this; }
             Desc& setRenderPass(VkRenderPass renderPass) { mRenderPass = renderPass; return *this; }
-            const Vao::SharedConstPtr& getVao() const { return mpVao; }
+            // const Vao::SharedConstPtr& getVao() const { return mpVao; }
             VkRenderPass getRenderPass() const {return mRenderPass;}
+
+            void setVaoTopology(Vao::Topology topology) { mVaoTopology = topology; }
+            Vao::Topology GetVaoTopology() { return mVaoTopology; }
         private:
-            Vao::SharedConstPtr mpVao;
+            Vao::Topology mVaoTopology;
+            // FixLeak: fix leak  on Mesh::Vao --- End
             VkRenderPass mRenderPass;
 #endif
         };
diff --git a/Framework/Source/API/Vulkan/VKGraphicsStateObject.cpp b/Framework/Source/API/Vulkan/VKGraphicsStateObject.cpp
index 7a145e64..d036b63b 100644
--- a/Framework/Source/API/Vulkan/VKGraphicsStateObject.cpp
+++ b/Framework/Source/API/Vulkan/VKGraphicsStateObject.cpp
@@ -46,7 +46,8 @@ namespace Falcor
 
         // Input Assembly State
         VkPipelineInputAssemblyStateCreateInfo inputAssemblyInfo = {};
-        initVkInputAssemblyInfo(mDesc.getVao().get(), inputAssemblyInfo);
+        // FixLeak: passing Vao::Topology instead of shared_ptr of Vao.
+        initVkInputAssemblyInfo(mDesc.GetVaoTopology(), inputAssemblyInfo);
         
         // Viewport State
         // Viewport and Scissors will be dynamic, but the count is still described here in the info struct
diff --git a/Framework/Source/API/Vulkan/VKState.cpp b/Framework/Source/API/Vulkan/VKState.cpp
index 7aaf97fb..1574c2ec 100644
--- a/Framework/Source/API/Vulkan/VKState.cpp
+++ b/Framework/Source/API/Vulkan/VKState.cpp
@@ -509,12 +509,13 @@ namespace Falcor
         }
     }
 
-    void initVkInputAssemblyInfo(const Vao* pVao, VkPipelineInputAssemblyStateCreateInfo& infoOut)
+    // FixLeak: passing Vao::Topology instead of shared_ptr of Vao.
+    void initVkInputAssemblyInfo(Vao::Topology topology, VkPipelineInputAssemblyStateCreateInfo& infoOut)
     {
         infoOut = {};
         infoOut.sType = VK_STRUCTURE_TYPE_PIPELINE_INPUT_ASSEMBLY_STATE_CREATE_INFO;
-        infoOut.topology = getVkPrimitiveTopology(pVao->getPrimitiveTopology());
-        infoOut.primitiveRestartEnable = (pVao->getPrimitiveTopology() == Vao::Topology::TriangleStrip); // Following the DX convention
+        infoOut.topology = getVkPrimitiveTopology(topology);
+        infoOut.primitiveRestartEnable = (topology == Vao::Topology::TriangleStrip); // Following the DX convention
     }
 
     void initVkRenderPassInfo(const Fbo::Desc& fboDesc, RenderPassCreateInfo& infoOut)
diff --git a/Framework/Source/API/Vulkan/VKState.h b/Framework/Source/API/Vulkan/VKState.h
index cc10c25c..dbde4f23 100644
--- a/Framework/Source/API/Vulkan/VKState.h
+++ b/Framework/Source/API/Vulkan/VKState.h
@@ -60,7 +60,8 @@ namespace Falcor
     void initVkVertexLayoutInfo(const VertexLayout* pLayout, VertexInputStateCreateInfo& infoOut, ProgramReflection const* pReflector);
     void initVkSamplerInfo(const Sampler* pSampler, VkSamplerCreateInfo& infoOut);
     void initVkMultiSampleInfo(const BlendState* pState, const Fbo::Desc& fboDesc, const uint32_t& sampleMask, VkPipelineMultisampleStateCreateInfo& infoOut, bool enableSampleFrequency);
-    void initVkInputAssemblyInfo(const Vao* pVao, VkPipelineInputAssemblyStateCreateInfo& infoOut);
+    // FixLeak: passing Vao::Topology instead of shared_ptr of Vao.
+    void initVkInputAssemblyInfo(Vao::Topology topology, VkPipelineInputAssemblyStateCreateInfo& infoOut);
     void initVkRenderPassInfo(const Fbo::Desc& fboDesc, RenderPassCreateInfo& infoOut);
 
     VkFilter getVkFilter(Sampler::Filter filter);
diff --git a/Framework/Source/Graphics/GraphicsState.cpp b/Framework/Source/Graphics/GraphicsState.cpp
index 7cb19024..66730398 100644
--- a/Framework/Source/Graphics/GraphicsState.cpp
+++ b/Framework/Source/Graphics/GraphicsState.cpp
@@ -167,7 +167,8 @@ namespace Falcor
             mpVao = pVao;
 
 #ifdef FALCOR_VK
-            mDesc.setVao(pVao);
+            // FixLeak: passing Vao::Topology instead of shared_ptr of Vao.
+            mDesc.setVaoTopology(pVao->getPrimitiveTopology());
 #endif
 
             mpGsoGraph->walk(pVao ? (void*)pVao->getVertexLayout().get() : nullptr);

[nbentyNV]

Thanks for that.

I suggest a simpler fix - save a weak_ptr to the VAO inside Desc. This will solve the leak with minimal changes while still storing enough information to detect whether the pointer is valid or not.

[KaChen-Dex]

You are right changing to weak_ptr introduce less changes. I actually changed to weak_ptr in the first iteration too. I then realized that PipelineStateObject doesn't need to know the whole information of vertex buffer etc, it only cares about the topology of triangles, not how many vertices the mesh has etc. The actually setting of vertex buffers is outside of the scope for PipelineStateObject. The DirectX code doesn't have Vao.

[nbentyNV]

That's true. I'll add the fix to the next release.

[KaChen-Dex]

thanks!