diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 512261ff9e50b..6eafa7868a51c 100644 --- a/ci/licenses_golden/excluded_files +++ b/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 diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index d7abddb096d74..ae2c71be87ca8 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/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", diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index de7e82b28ff21..9b187896a4f6d 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/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 pool_; + // `shared_ptr` since command buffers have a link to the command pool. + std::shared_ptr pool_; vk::UniqueCommandBuffer buffer_; std::set> tracked_objects_; std::set> tracked_buffers_; diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc new file mode 100644 index 0000000000000..0fa38fdb82985 --- /dev/null +++ b/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 + +#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> called_functions; + { + auto context = CreateMockVulkanContext(); + called_functions = GetMockVulkanFunctions(context->GetDevice()); + std::shared_ptr 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> 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 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 diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index f68c9ef0bd366..9ccf34ad95916 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -75,12 +75,13 @@ class ResourceManagerVK final using Reclaimables = std::vector>; 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. diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index a6f3d08e16dc1..c2ee56e309ff2 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/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(device); + mock_device->called_functions_->push_back("vkFreeCommandBuffers"); +} + +void vkDestroyCommandPool(VkDevice device, + VkCommandPool commandPool, + const VkAllocationCallbacks* pAllocator) { + MockDevice* mock_device = reinterpret_cast(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(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; }