Skip to content

Commit b55fddf

Browse files
JSUYAJSUYA
committed
Fix borrowed GLES external texture ownership
1 parent a00bf20 commit b55fddf

4 files changed

Lines changed: 96 additions & 15 deletions

File tree

‎engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ ReactorGLES::ReactorGLES(std::unique_ptr<ProcTableGLES> gl)
8383
ReactorGLES::~ReactorGLES() {
8484
if (CanReactOnCurrentThread()) {
8585
for (auto& handle : handles_) {
86-
if (handle.second.name.has_value()) {
86+
if (handle.second.name.has_value() && handle.second.owns_gl_handle) {
8787
CollectGLHandle(*proc_table_, handle.first.GetType(),
8888
handle.second.name.value());
8989
}
@@ -211,10 +211,16 @@ HandleGLES ReactorGLES::CreateUntrackedHandle(HandleType type) const {
211211
return new_handle;
212212
}
213213

214-
HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) {
214+
HandleGLES ReactorGLES::CreateHandle(HandleType type,
215+
GLuint external_handle,
216+
HandleOwnership ownership) {
215217
if (type == HandleType::kUnknown) {
216218
return HandleGLES::DeadHandle();
217219
}
220+
if (ownership == HandleOwnership::kBorrowed && external_handle == GL_NONE) {
221+
VALIDATION_LOG << "A borrowed handle requires an external GL handle.";
222+
return HandleGLES::DeadHandle();
223+
}
218224
auto new_handle = HandleGLES::Create(type);
219225
if (new_handle.IsDead()) {
220226
return HandleGLES::DeadHandle();
@@ -228,7 +234,8 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) {
228234
}
229235

230236
WriterLock handles_lock(handles_mutex_);
231-
handles_[new_handle] = LiveHandle{gl_handle};
237+
handles_[new_handle] =
238+
LiveHandle{gl_handle, ownership == HandleOwnership::kOwned};
232239
return new_handle;
233240
}
234241

@@ -297,8 +304,12 @@ bool ReactorGLES::ConsolidateHandles() {
297304
TRACE_EVENT0("impeller", __FUNCTION__);
298305
const auto& gl = GetProcTable();
299306
std::thread::id current_thread = std::this_thread::get_id();
300-
std::vector<std::tuple<HandleGLES, std::optional<GLStorage>>>
301-
handles_to_delete;
307+
struct HandleToDelete {
308+
HandleGLES handle;
309+
std::optional<GLStorage> storage;
310+
bool owns_gl_handle = true;
311+
};
312+
std::vector<HandleToDelete> handles_to_delete;
302313
std::vector<std::tuple<DebugResourceType, GLint, std::string>>
303314
handles_to_name;
304315
{
@@ -320,7 +331,11 @@ bool ReactorGLES::ConsolidateHandles() {
320331
if (!handle.second.callback) {
321332
storage = handle.second.name;
322333
}
323-
handles_to_delete.emplace_back(std::make_tuple(handle.first, storage));
334+
handles_to_delete.push_back(HandleToDelete{
335+
.handle = handle.first,
336+
.storage = storage,
337+
.owns_gl_handle = handle.second.owns_gl_handle,
338+
});
324339
continue;
325340
}
326341
// Create live handles.
@@ -343,7 +358,7 @@ bool ReactorGLES::ConsolidateHandles() {
343358
}
344359
}
345360
for (const auto& handle_to_delete : handles_to_delete) {
346-
handles_.erase(std::get<0>(handle_to_delete));
361+
handles_.erase(handle_to_delete.handle);
347362
}
348363
}
349364

@@ -352,11 +367,11 @@ bool ReactorGLES::ConsolidateHandles() {
352367
std::get<2>(handle));
353368
}
354369
for (const auto& handle : handles_to_delete) {
355-
const std::optional<GLStorage>& storage = std::get<1>(handle);
370+
const std::optional<GLStorage>& storage = handle.storage;
356371
// This could be false if the handle was created and collected without
357372
// use. We still need to get rid of map entry.
358-
if (storage.has_value()) {
359-
CollectGLHandle(gl, std::get<0>(handle).GetType(), storage.value());
373+
if (storage.has_value() && handle.owns_gl_handle) {
374+
CollectGLHandle(gl, handle.handle.GetType(), storage.value());
360375
}
361376
}
362377

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,18 @@ namespace impeller {
5151
/// Creating a handle in the reactor doesn't mean an OpenGL handle
5252
/// is created immediately. OpenGL handles become live before the
5353
/// next reaction. Similarly, dropping the last reference to a
54-
/// reactor handle means that the OpenGL handle will be deleted at
55-
/// some point in the near future.
54+
/// owned reactor handle means that the OpenGL handle will be
55+
/// deleted at some point in the near future.
5656
///
5757
class ReactorGLES {
5858
public:
5959
using WorkerID = UniqueID;
6060

61+
enum class HandleOwnership {
62+
kOwned,
63+
kBorrowed,
64+
};
65+
6166
//----------------------------------------------------------------------------
6267
/// @brief A delegate implemented by a thread on which an OpenGL context
6368
/// is current. There may be multiple workers for the reactor to
@@ -167,10 +172,15 @@ class ReactorGLES {
167172
///
168173
/// @param[in] type The type of handle to create.
169174
/// @param[in] external_handle An already created GL handle if one exists.
175+
/// @param[in] ownership Whether the reactor should collect the GL
176+
/// object when the handle is collected. Borrowed
177+
/// handles require an external handle.
170178
///
171179
/// @return The reactor handle.
172180
///
173-
HandleGLES CreateHandle(HandleType type, GLuint external_handle = GL_NONE);
181+
HandleGLES CreateHandle(HandleType type,
182+
GLuint external_handle = GL_NONE,
183+
HandleOwnership ownership = HandleOwnership::kOwned);
174184

175185
/// @brief Create a handle that is not managed by `ReactorGLES`.
176186
/// @details This behaves just like `CreateHandle` but it doesn't add the
@@ -272,11 +282,14 @@ class ReactorGLES {
272282
std::optional<GLStorage> name;
273283
std::optional<std::string> pending_debug_label;
274284
bool pending_collection = false;
285+
bool owns_gl_handle = true;
275286
fml::ScopedCleanupClosure callback = {};
276287

277288
LiveHandle() = default;
278289

279-
explicit LiveHandle(std::optional<GLStorage> p_name) : name(p_name) {}
290+
explicit LiveHandle(std::optional<GLStorage> p_name,
291+
bool p_owns_gl_handle = true)
292+
: name(p_name), owns_gl_handle(p_owns_gl_handle) {}
280293

281294
constexpr bool IsLive() const { return name.has_value(); }
282295
};

‎engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,58 @@ TEST(ReactorGLES, CanAttachCleanupCallbacksToHandles) {
4949
EXPECT_EQ(value, 1);
5050
}
5151

52+
TEST(ReactorGLES, BorrowedHandleCleanupDoesNotDeleteGLHandle) {
53+
auto mock_gles_impl = std::make_unique<MockGLESImpl>();
54+
55+
EXPECT_CALL(*mock_gles_impl, DeleteTextures(_, _)).Times(0);
56+
57+
std::shared_ptr<MockGLES> mock_gles =
58+
MockGLES::Init(std::move(mock_gles_impl));
59+
ProcTableGLES::Resolver resolver = kMockResolverGLES;
60+
auto proc_table = std::make_unique<ProcTableGLES>(resolver);
61+
auto worker = std::make_shared<TestWorker>();
62+
auto reactor = std::make_shared<ReactorGLES>(std::move(proc_table));
63+
reactor->AddWorker(worker);
64+
65+
int value = 0;
66+
auto handle = reactor->CreateHandle(HandleType::kTexture, 1123,
67+
ReactorGLES::HandleOwnership::kBorrowed);
68+
auto added =
69+
reactor->RegisterCleanupCallback(handle, [&value]() { value++; });
70+
71+
EXPECT_TRUE(added);
72+
EXPECT_TRUE(reactor->React());
73+
74+
reactor->CollectHandle(handle);
75+
EXPECT_TRUE(reactor->AddOperation([](const ReactorGLES& reactor) {}));
76+
EXPECT_TRUE(reactor->React());
77+
EXPECT_EQ(value, 1);
78+
}
79+
80+
TEST(ReactorGLES, BorrowedHandleShutdownDoesNotDeleteGLHandle) {
81+
auto mock_gles_impl = std::make_unique<MockGLESImpl>();
82+
83+
EXPECT_CALL(*mock_gles_impl, DeleteTextures(_, _)).Times(0);
84+
85+
std::shared_ptr<MockGLES> mock_gles =
86+
MockGLES::Init(std::move(mock_gles_impl));
87+
ProcTableGLES::Resolver resolver = kMockResolverGLES;
88+
auto proc_table = std::make_unique<ProcTableGLES>(resolver);
89+
auto worker = std::make_shared<TestWorker>();
90+
auto reactor = std::make_shared<ReactorGLES>(std::move(proc_table));
91+
reactor->AddWorker(worker);
92+
93+
int value = 0;
94+
auto handle = reactor->CreateHandle(HandleType::kTexture, 1123,
95+
ReactorGLES::HandleOwnership::kBorrowed);
96+
auto added =
97+
reactor->RegisterCleanupCallback(handle, [&value]() { value++; });
98+
99+
EXPECT_TRUE(added);
100+
reactor.reset();
101+
EXPECT_EQ(value, 1);
102+
}
103+
52104
TEST(ReactorGLES, DeletesHandlesDuringShutdown) {
53105
auto mock_gles_impl = std::make_unique<MockGLESImpl>();
54106

‎engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,8 @@ EmbedderExternalTextureGL::CreateTextureGLES(
227227
impeller::ContextGLES& context =
228228
impeller::ContextGLES::Cast(*aiks_context->GetContext());
229229
impeller::HandleGLES handle = context.GetReactor()->CreateHandle(
230-
impeller::HandleType::kTexture, texture->name);
230+
impeller::HandleType::kTexture, texture->name,
231+
impeller::ReactorGLES::HandleOwnership::kBorrowed);
231232

232233
auto gles_texture =
233234
impeller::TextureGLES::WrapTexture(context.GetReactor(), desc, handle);

0 commit comments

Comments
 (0)