[Impeller] Removed last ivars to raw vk::Device's. (#42215)

Removes the last ivars that hold onto a `Device` directly.  This have recently been the cause of crashes, so the safer thing is to remove them all.

For the Allocator and the Sampler library, removing the device ivars is likely just a precaution since they should have only been accessed from the raster thread and killed before the Device.  The fence waiter however was potentially accessing the device from a separate thread, so that was much more risky and likely to cause crashes.

Testing: The existing `impeller_unittests` verify this is setup correctly.

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

Author: gaaclarke

impeller/renderer/backend/vulkan/allocator_vk.cc

@@ -17,19 +17,20 @@ namespace impeller {
 AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
                          uint32_t vulkan_api_version,
                          const vk::PhysicalDevice& physical_device,
-                         const vk::Device& logical_device,
+                         const std::shared_ptr<DeviceHolder>& device_holder,
                          const vk::Instance& instance,
                          PFN_vkGetInstanceProcAddr get_instance_proc_address,
                          PFN_vkGetDeviceProcAddr get_device_proc_address)
-    : context_(std::move(context)), device_(logical_device) {
+    : context_(std::move(context)), device_holder_(device_holder) {
   vk_ = fml::MakeRefCounted<vulkan::VulkanProcTable>(get_instance_proc_address);
 
   auto instance_handle = vulkan::VulkanHandle<VkInstance>(instance);
   if (!vk_->SetupInstanceProcAddresses(instance_handle)) {
     return;
   }
 
-  auto device_handle = vulkan::VulkanHandle<VkDevice>(logical_device);
+  auto device_handle =
+      vulkan::VulkanHandle<VkDevice>(device_holder->GetDevice());
   if (!vk_->SetupDeviceProcAddresses(device_handle)) {
     return;
   }
@@ -78,7 +79,7 @@ AllocatorVK::AllocatorVK(std::weak_ptr<Context> context,
   VmaAllocatorCreateInfo allocator_info = {};
   allocator_info.vulkanApiVersion = vulkan_api_version;
   allocator_info.physicalDevice = physical_device;
-  allocator_info.device = logical_device;
+  allocator_info.device = device_holder->GetDevice();
   allocator_info.instance = instance;
   allocator_info.pVulkanFunctions = &proc_table;
 
@@ -333,10 +334,15 @@ std::shared_ptr<Texture> AllocatorVK::OnCreateTexture(
   if (!IsValid()) {
     return nullptr;
   }
-  auto source = std::make_shared<AllocatedTextureSourceVK>(desc,        //
-                                                           allocator_,  //
-                                                           device_      //
-  );
+  auto device_holder = device_holder_.lock();
+  if (!device_holder) {
+    return nullptr;
+  }
+  auto source =
+      std::make_shared<AllocatedTextureSourceVK>(desc,                       //
+                                                 allocator_,                 //
+                                                 device_holder->GetDevice()  //
+      );
   if (!source->IsValid()) {
     return nullptr;
   }

impeller/renderer/backend/vulkan/allocator_vk.h

@@ -10,6 +10,7 @@
 #include "impeller/core/allocator.h"
 #include "impeller/renderer/backend/vulkan/context_vk.h"
 #include "impeller/renderer/backend/vulkan/device_buffer_vk.h"
+#include "impeller/renderer/backend/vulkan/device_holder.h"
 #include "impeller/renderer/backend/vulkan/vk.h"
 
 #include <memory>
@@ -27,14 +28,14 @@ class AllocatorVK final : public Allocator {
   fml::RefPtr<vulkan::VulkanProcTable> vk_;
   VmaAllocator allocator_ = {};
   std::weak_ptr<Context> context_;
-  vk::Device device_;
+  std::weak_ptr<DeviceHolder> device_holder_;
   ISize max_texture_size_;
   bool is_valid_ = false;
 
   AllocatorVK(std::weak_ptr<Context> context,
               uint32_t vulkan_api_version,
               const vk::PhysicalDevice& physical_device,
-              const vk::Device& logical_device,
+              const std::shared_ptr<DeviceHolder>& device_holder,
               const vk::Instance& instance,
               PFN_vkGetInstanceProcAddr get_instance_proc_address,
               PFN_vkGetDeviceProcAddr get_device_proc_address);

impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc

@@ -13,7 +13,8 @@ namespace testing {
 TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) {
   auto context = CreateMockVulkanContext();
   auto pool = CommandPoolVK::GetThreadLocal(context.get());
-  CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
+  CommandEncoderVK encoder(context->GetDeviceHolder(),
+                           context->GetGraphicsQueue(), pool,
                            context->GetFenceWaiter());
   BlitCopyTextureToTextureCommandVK cmd;
   cmd.source = context->GetResourceAllocator()->CreateTexture({
@@ -31,7 +32,8 @@ TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) {
 TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) {
   auto context = CreateMockVulkanContext();
   auto pool = CommandPoolVK::GetThreadLocal(context.get());
-  CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
+  CommandEncoderVK encoder(context->GetDeviceHolder(),
+                           context->GetGraphicsQueue(), pool,
                            context->GetFenceWaiter());
   BlitCopyTextureToBufferCommandVK cmd;
   cmd.source = context->GetResourceAllocator()->CreateTexture({
@@ -49,7 +51,8 @@ TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) {
 TEST(BlitCommandVkTest, BlitGenerateMipmapCommandVK) {
   auto context = CreateMockVulkanContext();
   auto pool = CommandPoolVK::GetThreadLocal(context.get());
-  CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
+  CommandEncoderVK encoder(context->GetDeviceHolder(),
+                           context->GetGraphicsQueue(), pool,
                            context->GetFenceWaiter());
   BlitGenerateMipmapCommandVK cmd;
   cmd.texture = context->GetResourceAllocator()->CreateTexture({

impeller/renderer/backend/vulkan/command_pool_vk.cc

@@ -78,7 +78,7 @@ CommandPoolVK::CommandPoolVK(const ContextVK* context)
     return;
   }
 
-  device_holder_ = context->weak_from_this();
+  device_holder_ = context->GetDeviceHolder();
   graphics_pool_ = std::move(pool.value);
   is_valid_ = true;
 }

impeller/renderer/backend/vulkan/context_vk.cc

@@ -107,8 +107,8 @@ uint64_t CalculateHash(void* ptr) {
 ContextVK::ContextVK() : hash_(CalculateHash(this)) {}
 
 ContextVK::~ContextVK() {
-  if (device_) {
-    [[maybe_unused]] auto result = device_->waitIdle();
+  if (device_holder_->device) {
+    [[maybe_unused]] auto result = device_holder_->device->waitIdle();
   }
   CommandPoolVK::ClearAllPools(this);
 }
@@ -191,14 +191,17 @@ void ContextVK::Setup(Settings settings) {
   instance_info.setPApplicationInfo(&application_info);
   instance_info.setFlags(instance_flags);
 
-  auto instance = vk::createInstanceUnique(instance_info);
-  if (instance.result != vk::Result::eSuccess) {
-    VALIDATION_LOG << "Could not create Vulkan instance: "
-                   << vk::to_string(instance.result);
-    return;
+  auto device_holder = std::make_shared<DeviceHolderImpl>();
+  {
+    auto instance = vk::createInstanceUnique(instance_info);
+    if (instance.result != vk::Result::eSuccess) {
+      VALIDATION_LOG << "Could not create Vulkan instance: "
+                     << vk::to_string(instance.result);
+      return;
+    }
+    device_holder->instance = std::move(instance.value);
   }
-
-  dispatcher.init(instance.value.get());
+  dispatcher.init(device_holder->instance.get());
 
   //----------------------------------------------------------------------------
   /// Setup the debug report.
@@ -207,7 +210,7 @@ void ContextVK::Setup(Settings settings) {
   /// initialization issues.
   ///
   auto debug_report =
-      std::make_unique<DebugReportVK>(*caps, instance.value.get());
+      std::make_unique<DebugReportVK>(*caps, device_holder->instance.get());
 
   if (!debug_report->IsValid()) {
     VALIDATION_LOG << "Could not setup debug report.";
@@ -217,21 +220,25 @@ void ContextVK::Setup(Settings settings) {
   //----------------------------------------------------------------------------
   /// Pick the physical device.
   ///
-  auto physical_device = PickPhysicalDevice(*caps, instance.value.get());
-  if (!physical_device.has_value()) {
-    VALIDATION_LOG << "No valid Vulkan device found.";
-    return;
+  {
+    auto physical_device =
+        PickPhysicalDevice(*caps, device_holder->instance.get());
+    if (!physical_device.has_value()) {
+      VALIDATION_LOG << "No valid Vulkan device found.";
+      return;
+    }
+    device_holder->physical_device = std::move(physical_device.value());
   }
 
   //----------------------------------------------------------------------------
   /// Pick device queues.
   ///
   auto graphics_queue =
-      PickQueue(physical_device.value(), vk::QueueFlagBits::eGraphics);
+      PickQueue(device_holder->physical_device, vk::QueueFlagBits::eGraphics);
   auto transfer_queue =
-      PickQueue(physical_device.value(), vk::QueueFlagBits::eTransfer);
+      PickQueue(device_holder->physical_device, vk::QueueFlagBits::eTransfer);
   auto compute_queue =
-      PickQueue(physical_device.value(), vk::QueueFlagBits::eCompute);
+      PickQueue(device_holder->physical_device, vk::QueueFlagBits::eCompute);
 
   if (!graphics_queue.has_value()) {
     VALIDATION_LOG << "Could not pick graphics queue.";
@@ -250,7 +257,7 @@ void ContextVK::Setup(Settings settings) {
   /// Create the logical device.
   ///
   auto enabled_device_extensions =
-      caps->GetRequiredDeviceExtensions(physical_device.value());
+      caps->GetRequiredDeviceExtensions(device_holder->physical_device);
   if (!enabled_device_extensions.has_value()) {
     // This shouldn't happen since we already did device selection. But doesn't
     // hurt to check again.
@@ -266,7 +273,7 @@ void ContextVK::Setup(Settings settings) {
       {graphics_queue.value(), compute_queue.value(), transfer_queue.value()});
 
   const auto required_features =
-      caps->GetRequiredDeviceFeatures(physical_device.value());
+      caps->GetRequiredDeviceFeatures(device_holder->physical_device);
   if (!required_features.has_value()) {
     // This shouldn't happen since the device can't be picked if this was not
     // true. But doesn't hurt to check.
@@ -280,17 +287,17 @@ void ContextVK::Setup(Settings settings) {
   device_info.setPEnabledFeatures(&required_features.value());
   // Device layers are deprecated and ignored.
 
-  auto device_result = physical_device->createDeviceUnique(device_info);
-  if (device_result.result != vk::Result::eSuccess) {
-    VALIDATION_LOG << "Could not create logical device.";
-    return;
+  {
+    auto device_result =
+        device_holder->physical_device.createDeviceUnique(device_info);
+    if (device_result.result != vk::Result::eSuccess) {
+      VALIDATION_LOG << "Could not create logical device.";
+      return;
+    }
+    device_holder->device = std::move(device_result.value);
   }
-  device_ = std::move(device_result.value);
-  // This makes sure that the device is deleted at the proper time if there is
-  // an error.
-  fml::ScopedCleanupClosure device_resetter([this]() { device_.reset(); });
 
-  if (!caps->SetDevice(physical_device.value())) {
+  if (!caps->SetDevice(device_holder->physical_device)) {
     VALIDATION_LOG << "Capabilities could not be updated.";
     return;
   }
@@ -301,9 +308,9 @@ void ContextVK::Setup(Settings settings) {
   auto allocator = std::shared_ptr<AllocatorVK>(new AllocatorVK(
       weak_from_this(),                  //
       application_info.apiVersion,       //
-      physical_device.value(),           //
-      device_.get(),                     //
-      instance.value.get(),              //
+      device_holder->physical_device,    //
+      device_holder,                     //
+      device_holder->instance.get(),     //
       dispatcher.vkGetInstanceProcAddr,  //
       dispatcher.vkGetDeviceProcAddr     //
       ));
@@ -317,7 +324,7 @@ void ContextVK::Setup(Settings settings) {
   /// Setup the pipeline library.
   ///
   auto pipeline_library = std::shared_ptr<PipelineLibraryVK>(
-      new PipelineLibraryVK(shared_from_this(),                   //
+      new PipelineLibraryVK(device_holder,                        //
                             caps,                                 //
                             std::move(settings.cache_directory),  //
                             worker_task_runner_                   //
@@ -329,10 +336,10 @@ void ContextVK::Setup(Settings settings) {
   }
 
   auto sampler_library =
-      std::shared_ptr<SamplerLibraryVK>(new SamplerLibraryVK(device_.get()));
+      std::shared_ptr<SamplerLibraryVK>(new SamplerLibraryVK(device_holder));
 
   auto shader_library = std::shared_ptr<ShaderLibraryVK>(
-      new ShaderLibraryVK(weak_from_this(),                //
+      new ShaderLibraryVK(device_holder,                   //
                           settings.shader_libraries_data)  //
   );
 
@@ -345,7 +352,7 @@ void ContextVK::Setup(Settings settings) {
   /// Create the fence waiter.
   ///
   auto fence_waiter =
-      std::shared_ptr<FenceWaiterVK>(new FenceWaiterVK(device_.get()));
+      std::shared_ptr<FenceWaiterVK>(new FenceWaiterVK(device_holder));
   if (!fence_waiter->IsValid()) {
     VALIDATION_LOG << "Could not create fence waiter.";
     return;
@@ -354,27 +361,25 @@ void ContextVK::Setup(Settings settings) {
   //----------------------------------------------------------------------------
   /// Fetch the queues.
   ///
-  QueuesVK queues(device_.get(),           //
-                  graphics_queue.value(),  //
-                  compute_queue.value(),   //
-                  transfer_queue.value()   //
+  QueuesVK queues(device_holder->device.get(),  //
+                  graphics_queue.value(),       //
+                  compute_queue.value(),        //
+                  transfer_queue.value()        //
   );
   if (!queues.IsValid()) {
     VALIDATION_LOG << "Could not fetch device queues.";
     return;
   }
 
   VkPhysicalDeviceProperties physical_device_properties;
-  dispatcher.vkGetPhysicalDeviceProperties(physical_device.value(),
+  dispatcher.vkGetPhysicalDeviceProperties(device_holder->physical_device,
                                            &physical_device_properties);
 
   //----------------------------------------------------------------------------
   /// All done!
   ///
-  instance_ = std::move(instance.value);
+  device_holder_ = std::move(device_holder);
   debug_report_ = std::move(debug_report);
-  physical_device_ = physical_device.value();
-  device_resetter.Release();
   allocator_ = std::move(allocator);
   shader_library_ = std::move(shader_library);
   sampler_library_ = std::move(sampler_library);
@@ -389,7 +394,7 @@ void ContextVK::Setup(Settings settings) {
   /// Label all the relevant objects. This happens after setup so that the debug
   /// messengers have had a chance to be setup.
   ///
-  SetDebugName(GetDevice(), device_.get(), "ImpellerDevice");
+  SetDebugName(GetDevice(), device_holder_->device.get(), "ImpellerDevice");
 }
 
 // |Context|
@@ -429,11 +434,11 @@ std::shared_ptr<CommandBuffer> ContextVK::CreateCommandBuffer() const {
 }
 
 vk::Instance ContextVK::GetInstance() const {
-  return *instance_;
+  return *device_holder_->instance;
 }
 
 const vk::Device& ContextVK::GetDevice() const {
-  return device_.get();
+  return device_holder_->device.get();
 }
 
 const std::shared_ptr<fml::ConcurrentTaskRunner>
@@ -454,12 +459,13 @@ std::unique_ptr<Surface> ContextVK::AcquireNextSurface() {
 
 vk::UniqueSurfaceKHR ContextVK::CreateAndroidSurface(
     ANativeWindow* window) const {
-  if (!instance_) {
+  if (!device_holder_->instance) {
     return vk::UniqueSurfaceKHR{VK_NULL_HANDLE};
   }
 
   auto create_info = vk::AndroidSurfaceCreateInfoKHR().setWindow(window);
-  auto surface_res = instance_->createAndroidSurfaceKHRUnique(create_info);
+  auto surface_res =
+      device_holder_->instance->createAndroidSurfaceKHRUnique(create_info);
 
   if (surface_res.result != vk::Result::eSuccess) {
     VALIDATION_LOG << "Could not create Android surface, error: "
@@ -491,7 +497,7 @@ const std::shared_ptr<QueueVK>& ContextVK::GetGraphicsQueue() const {
 }
 
 vk::PhysicalDevice ContextVK::GetPhysicalDevice() const {
-  return physical_device_;
+  return device_holder_->physical_device;
 }
 
 std::shared_ptr<FenceWaiterVK> ContextVK::GetFenceWaiter() const {
@@ -505,7 +511,7 @@ std::unique_ptr<CommandEncoderVK> ContextVK::CreateGraphicsCommandEncoder()
     return nullptr;
   }
   auto encoder = std::unique_ptr<CommandEncoderVK>(new CommandEncoderVK(
-      weak_from_this(),        //
+      device_holder_,          //
       queues_.graphics_queue,  //
       tls_pool,                //
       fence_waiter_            //