这是indexloc提供的服务，不要输入任何密码
Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit dab2533

Browse files
gaaclarkegaaclarke
authored
[Impeller] introduces DeviceHolder to avoid accessing a dead Device (#41748)
An alternative to #41527 This introduces a new protocol called `DeviceHolder` whose only purpose is to return a pointer to a device. That way people can hold onto weak_ptr's to the `DeviceHolder` and know to abort further work once the Device is deleted (like loading pipelines on a background thread). This PR also changes the return type of ContextVK::GetDevice from returning a reference to a pointer as a hope to better communicate what is happening and discourage copying of the vk::Device. I would actually recommend abolishing holding onto instance variables of the vk::Device since it isn't clear that that is just a raw pointer. That's sort of how we got into this problem. fixes flutter/flutter#125522 fixes flutter/flutter#125519 ~~fixes flutter/flutter#125571 Testing: Fixes flake in existing tests. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
1 parent 5c98a39 commit dab2533

21 files changed

+156
-70
lines changed

‎engine/src/flutter/ci/licenses_golden/licenses_flutter‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,6 +1489,7 @@ ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc
14891489
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h + ../../../flutter/LICENSE
14901490
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/device_buffer_vk.cc + ../../../flutter/LICENSE
14911491
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/device_buffer_vk.h + ../../../flutter/LICENSE
1492+
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/device_holder.h + ../../../flutter/LICENSE
14921493
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc + ../../../flutter/LICENSE
14931494
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.h + ../../../flutter/LICENSE
14941495
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/formats_vk.cc + ../../../flutter/LICENSE
@@ -4090,6 +4091,7 @@ FILE: ../../../flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc
40904091
FILE: ../../../flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h
40914092
FILE: ../../../flutter/impeller/renderer/backend/vulkan/device_buffer_vk.cc
40924093
FILE: ../../../flutter/impeller/renderer/backend/vulkan/device_buffer_vk.h
4094+
FILE: ../../../flutter/impeller/renderer/backend/vulkan/device_holder.h
40934095
FILE: ../../../flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc
40944096
FILE: ../../../flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.h
40954097
FILE: ../../../flutter/impeller/renderer/backend/vulkan/formats_vk.cc

‎engine/src/flutter/impeller/renderer/backend/gles/context_gles.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@
1818
namespace impeller {
1919

2020
class ContextGLES final : public Context,
21-
public BackendCast<ContextGLES, Context> {
21+
public BackendCast<ContextGLES, Context>,
22+
public std::enable_shared_from_this<ContextGLES> {
2223
public:
2324
static std::shared_ptr<ContextGLES> Create(
2425
std::unique_ptr<ProcTableGLES> gl,

‎engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
namespace impeller {
2323

2424
class ContextMTL final : public Context,
25-
public BackendCast<ContextMTL, Context> {
25+
public BackendCast<ContextMTL, Context>,
26+
public std::enable_shared_from_this<ContextMTL> {
2627
public:
2728
static std::shared_ptr<ContextMTL> Create(
2829
const std::vector<std::string>& shader_library_paths);

‎engine/src/flutter/impeller/renderer/backend/metal/surface_mtl.mm‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@
112112
render_target_desc.SetColorAttachment(color0, 0u);
113113

114114
// The constructor is private. So make_unique may not be used.
115-
return std::unique_ptr<SurfaceMTL>(
116-
new SurfaceMTL(context->weak_from_this(), render_target_desc, resolve_tex,
117-
drawable, requires_blit, clip_rect));
115+
return std::unique_ptr<SurfaceMTL>(new SurfaceMTL(context, render_target_desc,
116+
resolve_tex, drawable,
117+
requires_blit, clip_rect));
118118
}
119119

120120
SurfaceMTL::SurfaceMTL(const std::weak_ptr<Context>& context,

‎engine/src/flutter/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ namespace testing {
1313
TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) {
1414
auto context = CreateMockVulkanContext();
1515
auto pool = CommandPoolVK::GetThreadLocal(context.get());
16-
CommandEncoderVK encoder(context->GetDevice(), context->GetGraphicsQueue(),
17-
pool, context->GetFenceWaiter());
16+
CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
17+
context->GetFenceWaiter());
1818
BlitCopyTextureToTextureCommandVK cmd;
1919
cmd.source = context->GetResourceAllocator()->CreateTexture({
2020
.size = ISize(100, 100),
@@ -31,8 +31,8 @@ TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) {
3131
TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) {
3232
auto context = CreateMockVulkanContext();
3333
auto pool = CommandPoolVK::GetThreadLocal(context.get());
34-
CommandEncoderVK encoder(context->GetDevice(), context->GetGraphicsQueue(),
35-
pool, context->GetFenceWaiter());
34+
CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
35+
context->GetFenceWaiter());
3636
BlitCopyTextureToBufferCommandVK cmd;
3737
cmd.source = context->GetResourceAllocator()->CreateTexture({
3838
.size = ISize(100, 100),
@@ -49,8 +49,8 @@ TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) {
4949
TEST(BlitCommandVkTest, BlitGenerateMipmapCommandVK) {
5050
auto context = CreateMockVulkanContext();
5151
auto pool = CommandPoolVK::GetThreadLocal(context.get());
52-
CommandEncoderVK encoder(context->GetDevice(), context->GetGraphicsQueue(),
53-
pool, context->GetFenceWaiter());
52+
CommandEncoderVK encoder(context, context->GetGraphicsQueue(), pool,
53+
context->GetFenceWaiter());
5454
BlitGenerateMipmapCommandVK cmd;
5555
cmd.texture = context->GetResourceAllocator()->CreateTexture({
5656
.size = ISize(100, 100),

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_encoder_vk.cc‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,9 @@ namespace impeller {
1414

1515
class TrackedObjectsVK {
1616
public:
17-
explicit TrackedObjectsVK(const vk::Device& device,
17+
explicit TrackedObjectsVK(std::weak_ptr<const DeviceHolder> device_holder,
1818
const std::shared_ptr<CommandPoolVK>& pool)
19-
: desc_pool_(device) {
19+
: desc_pool_(device_holder) {
2020
if (!pool) {
2121
return;
2222
}
@@ -97,12 +97,14 @@ class TrackedObjectsVK {
9797
FML_DISALLOW_COPY_AND_ASSIGN(TrackedObjectsVK);
9898
};
9999

100-
CommandEncoderVK::CommandEncoderVK(vk::Device device,
101-
const std::shared_ptr<QueueVK>& queue,
102-
const std::shared_ptr<CommandPoolVK>& pool,
103-
std::shared_ptr<FenceWaiterVK> fence_waiter)
100+
CommandEncoderVK::CommandEncoderVK(
101+
std::weak_ptr<const DeviceHolder> device_holder,
102+
const std::shared_ptr<QueueVK>& queue,
103+
const std::shared_ptr<CommandPoolVK>& pool,
104+
std::shared_ptr<FenceWaiterVK> fence_waiter)
104105
: fence_waiter_(std::move(fence_waiter)),
105-
tracked_objects_(std::make_shared<TrackedObjectsVK>(device, pool)) {
106+
tracked_objects_(
107+
std::make_shared<TrackedObjectsVK>(device_holder, pool)) {
106108
if (!fence_waiter_ || !tracked_objects_->IsValid() || !queue) {
107109
return;
108110
}
@@ -113,7 +115,7 @@ CommandEncoderVK::CommandEncoderVK(vk::Device device,
113115
VALIDATION_LOG << "Could not begin command buffer.";
114116
return;
115117
}
116-
device_ = device;
118+
device_holder_ = device_holder;
117119
queue_ = queue;
118120
is_valid_ = true;
119121
}
@@ -139,7 +141,11 @@ bool CommandEncoderVK::Submit() {
139141
if (command_buffer.end() != vk::Result::eSuccess) {
140142
return false;
141143
}
142-
auto [fence_result, fence] = device_.createFenceUnique({});
144+
std::shared_ptr<const DeviceHolder> strong_device = device_holder_.lock();
145+
if (!strong_device) {
146+
return false;
147+
}
148+
auto [fence_result, fence] = strong_device->GetDevice().createFenceUnique({});
143149
if (fence_result != vk::Result::eSuccess) {
144150
return false;
145151
}
@@ -168,7 +174,7 @@ void CommandEncoderVK::Reset() {
168174
tracked_objects_.reset();
169175

170176
queue_ = nullptr;
171-
device_ = nullptr;
177+
device_holder_ = {};
172178
is_valid_ = false;
173179
}
174180

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_encoder_vk.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "flutter/fml/macros.h"
1111
#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
1212
#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
13+
#include "impeller/renderer/backend/vulkan/device_holder.h"
1314
#include "impeller/renderer/backend/vulkan/queue_vk.h"
1415
#include "impeller/renderer/backend/vulkan/shared_object_vk.h"
1516
#include "impeller/renderer/backend/vulkan/vk.h"
@@ -69,13 +70,13 @@ class CommandEncoderVK {
6970
friend class ::impeller::testing::
7071
BlitCommandVkTest_BlitGenerateMipmapCommandVK_Test;
7172

72-
vk::Device device_ = {};
73+
std::weak_ptr<const DeviceHolder> device_holder_;
7374
std::shared_ptr<QueueVK> queue_;
7475
std::shared_ptr<FenceWaiterVK> fence_waiter_;
7576
std::shared_ptr<TrackedObjectsVK> tracked_objects_;
7677
bool is_valid_ = false;
7778

78-
CommandEncoderVK(vk::Device device,
79+
CommandEncoderVK(std::weak_ptr<const DeviceHolder> device_holder,
7980
const std::shared_ptr<QueueVK>& queue,
8081
const std::shared_ptr<CommandPoolVK>& pool,
8182
std::shared_ptr<FenceWaiterVK> fence_waiter);

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ CommandPoolVK::CommandPoolVK(const ContextVK* context)
7979
return;
8080
}
8181

82-
device_ = context->GetDevice();
82+
device_holder_ = context->weak_from_this();
8383
graphics_pool_ = std::move(pool.value);
8484
is_valid_ = true;
8585
}
@@ -102,6 +102,10 @@ vk::CommandPool CommandPoolVK::GetGraphicsCommandPool() const {
102102
}
103103

104104
vk::UniqueCommandBuffer CommandPoolVK::CreateGraphicsCommandBuffer() {
105+
std::shared_ptr<const DeviceHolder> strong_device = device_holder_.lock();
106+
if (!strong_device) {
107+
return {};
108+
}
105109
if (std::this_thread::get_id() != owner_id_) {
106110
return {};
107111
}
@@ -113,7 +117,8 @@ vk::UniqueCommandBuffer CommandPoolVK::CreateGraphicsCommandBuffer() {
113117
alloc_info.commandPool = graphics_pool_.get();
114118
alloc_info.commandBufferCount = 1u;
115119
alloc_info.level = vk::CommandBufferLevel::ePrimary;
116-
auto [result, buffers] = device_.allocateCommandBuffersUnique(alloc_info);
120+
auto [result, buffers] =
121+
strong_device->GetDevice().allocateCommandBuffersUnique(alloc_info);
117122
if (result != vk::Result::eSuccess) {
118123
return {};
119124
}

‎engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "flutter/fml/macros.h"
1212
#include "impeller/base/thread.h"
13+
#include "impeller/renderer/backend/vulkan/device_holder.h"
1314
#include "impeller/renderer/backend/vulkan/shared_object_vk.h"
1415
#include "impeller/renderer/backend/vulkan/vk.h"
1516

@@ -38,7 +39,7 @@ class CommandPoolVK {
3839

3940
private:
4041
const std::thread::id owner_id_;
41-
vk::Device device_ = {};
42+
std::weak_ptr<const DeviceHolder> device_holder_;
4243
vk::UniqueCommandPool graphics_pool_;
4344
Mutex buffers_to_collect_mutex_;
4445
std::set<SharedHandleVK<vk::CommandBuffer>> buffers_to_collect_

‎engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -273,11 +273,12 @@ void ContextVK::Setup(Settings settings) {
273273
device_info.setPEnabledFeatures(&required_features.value());
274274
// Device layers are deprecated and ignored.
275275

276-
auto device = physical_device->createDeviceUnique(device_info);
277-
if (device.result != vk::Result::eSuccess) {
276+
auto device_result = physical_device->createDeviceUnique(device_info);
277+
if (device_result.result != vk::Result::eSuccess) {
278278
VALIDATION_LOG << "Could not create logical device.";
279279
return;
280280
}
281+
vk::UniqueDevice device = std::move(device_result.value);
281282

282283
if (!caps->SetDevice(physical_device.value())) {
283284
VALIDATION_LOG << "Capabilities could not be updated.";
@@ -291,7 +292,7 @@ void ContextVK::Setup(Settings settings) {
291292
weak_from_this(), //
292293
application_info.apiVersion, //
293294
physical_device.value(), //
294-
device.value.get(), //
295+
device.get(), //
295296
instance.value.get(), //
296297
dispatcher.vkGetInstanceProcAddr, //
297298
dispatcher.vkGetDeviceProcAddr //
@@ -306,7 +307,8 @@ void ContextVK::Setup(Settings settings) {
306307
/// Setup the pipeline library.
307308
///
308309
auto pipeline_library = std::shared_ptr<PipelineLibraryVK>(
309-
new PipelineLibraryVK(device.value.get(), //
310+
new PipelineLibraryVK(weak_from_this(), //
311+
device.get(), //
310312
caps, //
311313
std::move(settings.cache_directory), //
312314
settings.worker_task_runner //
@@ -317,11 +319,11 @@ void ContextVK::Setup(Settings settings) {
317319
return;
318320
}
319321

320-
auto sampler_library = std::shared_ptr<SamplerLibraryVK>(
321-
new SamplerLibraryVK(device.value.get()));
322+
auto sampler_library =
323+
std::shared_ptr<SamplerLibraryVK>(new SamplerLibraryVK(device.get()));
322324

323325
auto shader_library = std::shared_ptr<ShaderLibraryVK>(
324-
new ShaderLibraryVK(device.value.get(), //
326+
new ShaderLibraryVK(device.get(), //
325327
settings.shader_libraries_data) //
326328
);
327329

@@ -334,7 +336,7 @@ void ContextVK::Setup(Settings settings) {
334336
/// Create the fence waiter.
335337
///
336338
auto fence_waiter =
337-
std::shared_ptr<FenceWaiterVK>(new FenceWaiterVK(device.value.get()));
339+
std::shared_ptr<FenceWaiterVK>(new FenceWaiterVK(device.get()));
338340
if (!fence_waiter->IsValid()) {
339341
VALIDATION_LOG << "Could not create fence waiter.";
340342
return;
@@ -343,7 +345,7 @@ void ContextVK::Setup(Settings settings) {
343345
//----------------------------------------------------------------------------
344346
/// Fetch the queues.
345347
///
346-
QueuesVK queues(device.value.get(), //
348+
QueuesVK queues(device.get(), //
347349
graphics_queue.value(), //
348350
compute_queue.value(), //
349351
transfer_queue.value() //
@@ -363,7 +365,7 @@ void ContextVK::Setup(Settings settings) {
363365
instance_ = std::move(instance.value);
364366
debug_report_ = std::move(debug_report);
365367
physical_device_ = physical_device.value();
366-
device_ = std::move(device.value);
368+
device_ = std::move(device);
367369
allocator_ = std::move(allocator);
368370
shader_library_ = std::move(shader_library);
369371
sampler_library_ = std::move(sampler_library);
@@ -378,7 +380,7 @@ void ContextVK::Setup(Settings settings) {
378380
/// Label all the relevant objects. This happens after setup so that the debug
379381
/// messengers have had a chance to be setup.
380382
///
381-
SetDebugName(device_.get(), device_.get(), "ImpellerDevice");
383+
SetDebugName(GetDevice(), device_.get(), "ImpellerDevice");
382384
}
383385

384386
// |Context|
@@ -421,8 +423,8 @@ vk::Instance ContextVK::GetInstance() const {
421423
return *instance_;
422424
}
423425

424-
vk::Device ContextVK::GetDevice() const {
425-
return *device_;
426+
const vk::Device& ContextVK::GetDevice() const {
427+
return device_.get();
426428
}
427429

428430
std::unique_ptr<Surface> ContextVK::AcquireNextSurface() {
@@ -489,7 +491,7 @@ std::unique_ptr<CommandEncoderVK> ContextVK::CreateGraphicsCommandEncoder()
489491
return nullptr;
490492
}
491493
auto encoder = std::unique_ptr<CommandEncoderVK>(new CommandEncoderVK(
492-
*device_, //
494+
weak_from_this(), //
493495
queues_.graphics_queue, //
494496
tls_pool, //
495497
fence_waiter_ //

0 commit comments

Comments
 (0)