[Impeller] Started tracking the pool with the command buffer. (#45298)

issue: flutter/flutter#133506

This fixes one of the issues seen in flutter/flutter#133506.  There were multiple ones though.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: gaaclarke

ci/licenses_golden/excluded_files

@@ -151,6 +151,7 @@
 ../../../flutter/impeller/image/README.md
 ../../../flutter/impeller/playground
 ../../../flutter/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc
+../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc
 ../../../flutter/impeller/renderer/backend/vulkan/context_vk_unittests.cc
 ../../../flutter/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc
 ../../../flutter/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc

impeller/renderer/backend/vulkan/BUILD.gn

@@ -9,6 +9,7 @@ impeller_component("vulkan_unittests") {
   testonly = true
   sources = [
     "blit_command_vk_unittests.cc",
+    "command_encoder_vk_unittests.cc",
     "context_vk_unittests.cc",
     "pass_bindings_cache_unittests.cc",
     "resource_manager_vk_unittests.cc",

impeller/renderer/backend/vulkan/command_encoder_vk.cc

@@ -34,13 +34,7 @@ class TrackedObjectsVK {
     if (!buffer_) {
       return;
     }
-    auto pool = pool_.lock();
-    if (!pool) {
-      // The buffer can not be freed if its command pool has been destroyed.
-      buffer_.release();
-      return;
-    }
-    pool->CollectGraphicsCommandBuffer(std::move(buffer_));
+    pool_->CollectGraphicsCommandBuffer(std::move(buffer_));
   }
 
   bool IsValid() const { return is_valid_; }
@@ -86,7 +80,8 @@ class TrackedObjectsVK {
 
  private:
   DescriptorPoolVK desc_pool_;
-  std::weak_ptr<CommandPoolVK> pool_;
+  // `shared_ptr` since command buffers have a link to the command pool.
+  std::shared_ptr<CommandPoolVK> pool_;
   vk::UniqueCommandBuffer buffer_;
   std::set<std::shared_ptr<SharedObjectVK>> tracked_objects_;
   std::set<std::shared_ptr<const Buffer>> tracked_buffers_;

impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc

@@ -0,0 +1,76 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <thread>
+
+#include "flutter/fml/synchronization/count_down_latch.h"
+#include "flutter/testing/testing.h"
+#include "impeller/renderer/backend/vulkan/command_encoder_vk.h"
+#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h"
+#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
+
+namespace impeller {
+namespace testing {
+
+TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) {
+  // Tests that when a CommandEncoderVK is deleted that it will clean up its
+  // command buffers before it cleans up its command pool.
+  std::shared_ptr<std::vector<std::string>> called_functions;
+  {
+    auto context = CreateMockVulkanContext();
+    called_functions = GetMockVulkanFunctions(context->GetDevice());
+    std::shared_ptr<CommandEncoderVK> encoder;
+    std::thread thread([&] {
+      CommandEncoderFactoryVK factory(context);
+      encoder = factory.Create();
+    });
+    thread.join();
+  }
+  auto destroy_pool =
+      std::find(called_functions->begin(), called_functions->end(),
+                "vkDestroyCommandPool");
+  auto free_buffers =
+      std::find(called_functions->begin(), called_functions->end(),
+                "vkFreeCommandBuffers");
+  EXPECT_TRUE(destroy_pool != called_functions->end());
+  EXPECT_TRUE(free_buffers != called_functions->end());
+  EXPECT_TRUE(free_buffers < destroy_pool);
+}
+
+TEST(CommandEncoderVKTest, CleanupAfterSubmit) {
+  // This tests deleting the TrackedObjects where the thread is killed before
+  // the fence waiter has disposed of them, making sure the command buffer and
+  // its pools are deleted in that order.
+  std::shared_ptr<std::vector<std::string>> called_functions;
+  {
+    fml::AutoResetWaitableEvent wait_for_submit;
+    fml::AutoResetWaitableEvent wait_for_thread_join;
+    auto context = CreateMockVulkanContext();
+    std::thread thread([&] {
+      CommandEncoderFactoryVK factory(context);
+      std::shared_ptr<CommandEncoderVK> encoder = factory.Create();
+      encoder->Submit([&](bool success) {
+        ASSERT_TRUE(success);
+        wait_for_thread_join.Wait();
+        wait_for_submit.Signal();
+      });
+    });
+    thread.join();
+    wait_for_thread_join.Signal();
+    wait_for_submit.Wait();
+    called_functions = GetMockVulkanFunctions(context->GetDevice());
+  }
+  auto destroy_pool =
+      std::find(called_functions->begin(), called_functions->end(),
+                "vkDestroyCommandPool");
+  auto free_buffers =
+      std::find(called_functions->begin(), called_functions->end(),
+                "vkFreeCommandBuffers");
+  EXPECT_TRUE(destroy_pool != called_functions->end());
+  EXPECT_TRUE(free_buffers != called_functions->end());
+  EXPECT_TRUE(free_buffers < destroy_pool);
+}
+
+}  // namespace testing
+}  // namespace impeller

impeller/renderer/backend/vulkan/resource_manager_vk.h

@@ -75,12 +75,13 @@ class ResourceManagerVK final
   using Reclaimables = std::vector<std::unique_ptr<ResourceVK>>;
 
   ResourceManagerVK();
-
-  std::thread waiter_;
   std::mutex reclaimables_mutex_;
   std::condition_variable reclaimables_cv_;
   Reclaimables reclaimables_;
   bool should_exit_ = false;
+  // This should be initialized last since it references the other instance
+  // variables.
+  std::thread waiter_;
 
   //----------------------------------------------------------------------------
   /// @brief      Starts the resource manager thread.

impeller/renderer/backend/vulkan/test/mock_vulkan.cc

@@ -346,6 +346,52 @@ void vkCmdSetViewport(VkCommandBuffer commandBuffer,
   mock_command_buffer->called_functions_->push_back("vkCmdSetViewport");
 }
 
+void vkFreeCommandBuffers(VkDevice device,
+                          VkCommandPool commandPool,
+                          uint32_t commandBufferCount,
+                          const VkCommandBuffer* pCommandBuffers) {
+  MockDevice* mock_device = reinterpret_cast<MockDevice*>(device);
+  mock_device->called_functions_->push_back("vkFreeCommandBuffers");
+}
+
+void vkDestroyCommandPool(VkDevice device,
+                          VkCommandPool commandPool,
+                          const VkAllocationCallbacks* pAllocator) {
+  MockDevice* mock_device = reinterpret_cast<MockDevice*>(device);
+  mock_device->called_functions_->push_back("vkDestroyCommandPool");
+}
+
+VkResult vkEndCommandBuffer(VkCommandBuffer commandBuffer) {
+  return VK_SUCCESS;
+}
+
+VkResult vkCreateFence(VkDevice device,
+                       const VkFenceCreateInfo* pCreateInfo,
+                       const VkAllocationCallbacks* pAllocator,
+                       VkFence* pFence) {
+  *pFence = reinterpret_cast<VkFence>(0xfe0ce);
+  return VK_SUCCESS;
+}
+
+VkResult vkQueueSubmit(VkQueue queue,
+                       uint32_t submitCount,
+                       const VkSubmitInfo* pSubmits,
+                       VkFence fence) {
+  return VK_SUCCESS;
+}
+
+VkResult vkWaitForFences(VkDevice device,
+                         uint32_t fenceCount,
+                         const VkFence* pFences,
+                         VkBool32 waitAll,
+                         uint64_t timeout) {
+  return VK_SUCCESS;
+}
+
+VkResult vkGetFenceStatus(VkDevice device, VkFence fence) {
+  return VK_SUCCESS;
+}
+
 PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance,
                                             const char* pName) {
   if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) {
@@ -424,6 +470,20 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance,
     return (PFN_vkVoidFunction)vkCmdSetScissor;
   } else if (strcmp("vkCmdSetViewport", pName) == 0) {
     return (PFN_vkVoidFunction)vkCmdSetViewport;
+  } else if (strcmp("vkDestroyCommandPool", pName) == 0) {
+    return (PFN_vkVoidFunction)vkDestroyCommandPool;
+  } else if (strcmp("vkFreeCommandBuffers", pName) == 0) {
+    return (PFN_vkVoidFunction)vkFreeCommandBuffers;
+  } else if (strcmp("vkEndCommandBuffer", pName) == 0) {
+    return (PFN_vkVoidFunction)vkEndCommandBuffer;
+  } else if (strcmp("vkCreateFence", pName) == 0) {
+    return (PFN_vkVoidFunction)vkCreateFence;
+  } else if (strcmp("vkQueueSubmit", pName) == 0) {
+    return (PFN_vkVoidFunction)vkQueueSubmit;
+  } else if (strcmp("vkWaitForFences", pName) == 0) {
+    return (PFN_vkVoidFunction)vkWaitForFences;
+  } else if (strcmp("vkGetFenceStatus", pName) == 0) {
+    return (PFN_vkVoidFunction)vkGetFenceStatus;
   }
   return noop;
 }