[Impeller] Do not delete the GL object in a HandleGLES if the handle has a cleanup callback#183561
Conversation
github-actions
bot
added
engine
github-actions
bot
removed
platform-linux
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies Impeller's OpenGL backend to prevent it from deleting GL objects for which the embedder manages the lifetime. This is achieved by checking for a cleanup callback on HandleGLES; if one is present, ReactorGLES will not delete the texture. The changes are logical and well-implemented, including updates to EmbedderExternalTextureGL to always register a callback (even a no-op) to signify embedder ownership, and improving error handling with ScopedCleanupClosure. The accompanying unit test updates correctly validate the new behavior. I have a couple of minor suggestions to improve code readability and maintainability.
engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc
Outdated
Show resolved
Hide resolved
engine/src/flutter/shell/platform/embedder/tests/embedder_gl_unittests.cc
Outdated
Show resolved
Hide resolved
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I looked through the tests and I don't see one that seems to assert the texture destruction callback is called in the normal case. Maybe I'm wrong, can you please validate that and possibly add a test for that.
Otherwise, code looks great.
| static GLuint gl_texture = 0; | ||
| auto rendered_scene = context.GetNextSceneImage(); | ||
| context.GetRendererConfig().open_gl.gl_external_texture_frame_callback = | ||
| [](void* user_data, int64_t texture_id, size_t width, size_t height, | ||
| FlutterOpenGLTexture* texture) -> bool { |
There was a problem hiding this comment.
no action required: I think this would be more clear if we used c syntax for setting the callback instead because they are c function pointers. I had that comment the other day were I suggested capturing a local.
There was a problem hiding this comment.
Assigning a no-capture lambda to a function pointer is a common idiom. It has the advantage that the lambda can access static variables declared within the enclosing function's scope.
| image->SetCoordinateSystem( | ||
| impeller::TextureCoordinateSystem::kUploadFromHost); | ||
|
|
||
| scoped_cleanup.Release(); |
There was a problem hiding this comment.
Can we add a test that will fail if this isn't present? Looking over the tests, I'm not sure if that is possible. If someone accidentally removed this i guess it would probably be a warning but since it's far away from the comment that explains it, it would be nice to make sure it doesn't get goofed accidentally.
There was a problem hiding this comment.
Added a test that checks invocation of the texture destruction callback.
The callback in the test will delete the GL texture. If the callback is called prematurely, then this will cause a mismatch against the expected image.
…has a cleanup callback This allows embedders to control the lifetime of textures wrapped by HandleGLES. If the embedder calls RegisterCleanupCallback on the handle, then ReactorGLES will not delete the texture when the handle is collected. Fixes flutter#183058
jason-simmons
force-pushed
the
bug_183065_cleanup_callback
branch
from
6e0d8d6 to
d45b366
Compare
jason-simmons
added
CICD
flutter-dashboard
bot
removed
the
autosubmit
…he handle has a cleanup callback (flutter/flutter#183561)
…he handle has a cleanup callback (flutter/flutter#183561)
…he handle has a cleanup callback (flutter/flutter#183561)
…he handle has a cleanup callback (flutter/flutter#183561)
…he handle has a cleanup callback (flutter/flutter#183561)
…he handle has a cleanup callback (flutter/flutter#183561)
flutter/flutter@d117642...dd64978 2026-03-18 [email protected] Roll Skia from 2fb5fa71eb12 to f0a13e5efbad (2 revisions) (flutter/flutter#183830) 2026-03-18 [email protected] Roll Skia from ae3d36cb9e29 to 2fb5fa71eb12 (3 revisions) (flutter/flutter#183823) 2026-03-18 [email protected] Linux reuse sibling (flutter/flutter#183653) 2026-03-18 [email protected] Roll Skia from 84a180a1fa80 to ae3d36cb9e29 (4 revisions) (flutter/flutter#183812) 2026-03-18 [email protected] fix(web): handle asynchronously disposed platform views (flutter/flutter#183666) 2026-03-17 [email protected] (Test cross-imports) Remove legacy Material import from sliver_constraints_test (flutter/flutter#183351) 2026-03-17 [email protected] Fix Android Studio pluginsPath when version is unknown (do not use 0.0) (flutter/flutter#182681) 2026-03-17 [email protected] Fixes animation glitch into bottom sheet (flutter/flutter#183303) 2026-03-17 [email protected] Handle#6537 second grouped test (flutter/flutter#182529) 2026-03-17 [email protected] Add a Clarification for the docs of suggestionsBuilder of SearchAnchor (flutter/flutter#183106) 2026-03-17 [email protected] Remove obsolete null checks from style guide (flutter/flutter#181703) 2026-03-17 [email protected] [Impeller] Do not delete the GL object in a HandleGLES if the handle has a cleanup callback (flutter/flutter#183561) 2026-03-17 [email protected] Encode source file patches as UTF-8 in the code formatter script (flutter/flutter#183761) 2026-03-17 [email protected] Fix widget inspector control layout and add safe area regression test (flutter/flutter#180789) 2026-03-17 [email protected] Reland "[Android] Add mechanism for setting Android engine flags via Android manifest (take 2)" (flutter/flutter#182522) 2026-03-17 [email protected] fix(web): fix crash in Skwasm when transferring non-transferable texture sources (flutter/flutter#183799) 2026-03-17 [email protected] Roll Skia from dba893a44d7a to 84a180a1fa80 (7 revisions) (flutter/flutter#183803) 2026-03-17 [email protected] Framework: Improve DropdownButton selectedItemBuilder assertion (flutter/flutter#183732) 2026-03-17 [email protected] Add mainAxisAlignment to NavigationRail (flutter/flutter#183514) 2026-03-17 [email protected] Update android triage process to not look at unassigned p1s every week (flutter/flutter#183805) 2026-03-17 [email protected] Adds macos impeller new gallery transition perf test. (flutter/flutter#183802) 2026-03-17 [email protected] fix(web_ui): move prepareToDraw after raster to improve concurrency and stability (flutter/flutter#183791) 2026-03-17 [email protected] [build] Generate debug info for assembly. (flutter/flutter#183425) 2026-03-17 [email protected] [web] Fix occasional failure to find Chrome tab (flutter/flutter#183737) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
This allows embedders to control the lifetime of textures wrapped by HandleGLES. If the embedder calls RegisterCleanupCallback on the handle, then ReactorGLES will not delete the texture when the handle is collected.
Fixes #183058