FlutterMetalTexture destruction_callback not called when fetched via FlutterMetalRendererConfig.get_next_drawable_callback #116381
Assignees
Labels
e: embedder
Users of the Embedder API
engine
flutter/engine repository. See also e: labels.
perf: memory
Performance issues related to memory
Comments
cbracken
added
engine
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 1, 2022
This ensures FlutterMetalTexture.destruction_callback gets called. FlutterRendererConfig.get_next_drawable_callback` holds a callback used by the embedder API to request a drawable; in the case of Metal, this drawable is a FlutterMetalTexture. FlutterMetalTexture.destruction_callback should be called when it's safe to release resources associated with the FlutterMetalTexture. This callback is not currently invoked for textures returned via FlutterRendererConfig.get_next_drawable_callback; instead we unpack the returned struct and pass it on. In the compositor codepath, we do create an SkSurface that triggers the destruction callback, here: https://github.com/flutter/engine/blob/303e26e96561d9b76f2344e97a5fc32eb6dfdb9a/shell/platform/embedder/embedder.cc#L868-L881 Issue: flutter/flutter#116381
|
I’ve put together a patch that fixes this, but will need to add an embedder API test before I send it out, |
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 3, 2022
This ensures FlutterMetalTexture.destruction_callback gets called. FlutterRendererConfig.get_next_drawable_callback` holds a callback used by the embedder API to request a drawable; in the case of Metal, this drawable is a FlutterMetalTexture. FlutterMetalTexture.destruction_callback should be called when it's safe to release resources associated with the FlutterMetalTexture. This callback is not currently invoked for textures returned via FlutterRendererConfig.get_next_drawable_callback; instead we unpack the returned struct and pass it on. In the compositor codepath, we do create an SkSurface that triggers the destruction callback, here: https://github.com/flutter/engine/blob/303e26e96561d9b76f2344e97a5fc32eb6dfdb9a/shell/platform/embedder/embedder.cc#L868-L881 Issue: flutter/flutter#116381
cbracken
added a commit
to flutter/engine
that referenced
this issue
Dec 3, 2022
This ensures FlutterMetalTexture.destruction_callback gets called. FlutterRendererConfig.get_next_drawable_callback holds a callback used by the embedder API to request a drawable; in the case of Metal, this drawable is a FlutterMetalTexture. FlutterMetalTexture.destruction_callback should be called when it's safe to release resources associated with the FlutterMetalTexture. This callback is not currently invoked for textures returned via FlutterRendererConfig.get_next_drawable_callback; instead we unpack the returned struct and pass it on. In the compositor codepath, we do create an SkSurface that triggers the destruction callback, here: https://github.com/flutter/engine/blob/303e26e96561d9b76f2344e97a5fc32eb6dfdb9a/shell/platform/embedder/embedder.cc#L868-L881 Issue: flutter/flutter#116381
|
Fixed by flutter/engine#38038. |
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 7, 2022
This is a followup to flutter#38038. In that patch, a destruction callback for textures created via the FlutterRendererConfig callbacks (as opposed to by the FlutterCompositor callbacks) was added and passed through to the texture info attached to the SurfaceFrame generated in GPUSurfaceMetalSkia::AcquireFrameFromMTLTexture (called via GPUSurfaceMetalSkia::GetMTLTexture, which invokes the get_next_drawable_callback), however, in order for the destruction callback to make it to the presented Skia texture, it needs to be passed through to the present callback here: https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL233 which is invoked by the submit callback passed to Skia: https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL239 The present callback is implemented in EmbedderSurface::PresentTexture, which invokes the present callback registered in FlutterMetalRendererConfig.present_drawable_callback. This patch ensures that the destruction callback is passed through to Skia's present callback so destruction occurs in the right place. Issue: flutter/flutter#116381
auto-submit bot
pushed a commit
to flutter/engine
that referenced
this issue
Dec 7, 2022
This is a followup to #38038. In that patch, a destruction callback for textures created via the FlutterRendererConfig callbacks (as opposed to by the FlutterCompositor callbacks) was added and passed through to the texture info attached to the SurfaceFrame generated in GPUSurfaceMetalSkia::AcquireFrameFromMTLTexture (called via GPUSurfaceMetalSkia::GetMTLTexture, which invokes the get_next_drawable_callback), however, in order for the destruction callback to make it to the presented Skia texture, it needs to be passed through to the present callback here: https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL233 which is invoked by the submit callback passed to Skia: https://github.com/flutter/engine/blob/5545ccf8719c9568d8f521f9f18a869ca9e10056/shell/gpu/gpu_surface_metal_skia.mm#LL239 The present callback is implemented in EmbedderSurface::PresentTexture, which invokes the present callback registered in FlutterMetalRendererConfig.present_drawable_callback. This patch ensures that the destruction callback is passed through to Skia's present callback so destruction occurs in the right place. Issue: flutter/flutter#116381
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 8, 2022
This is a minor cleanup that exposes the test metal surface to tests via EmbedderTestContextMeta::GetTestMetalSurface for parity with the GetTestMetalContext method which exposes the test metal context. This eliminates the need for the more specific GetTextureInfo method since the texture info is accessible via the test context. This is a test refactoring with no semantic differences to the engine. Issue: flutter/flutter#116381
cbracken
added a commit
to cbracken/cocoon
that referenced
this issue
Dec 8, 2022
Embedder API tests in the engine are implemented as C++ unit tests and
test infrastructure such as embedder_test_context_metal.{h,cc} (located
under the shell/platform/embedder/tests directory), which execute and
test Dart functions (and associated test assets) stored in the
shell/platform/embedder/fixtures directory.
Uncovered while sending flutter/engine#38133.
Issue: flutter/flutter#116381
8 tasks
cbracken
added a commit
to flutter/engine
that referenced
this issue
Dec 8, 2022
This is a minor cleanup that exposes the test metal surface to tests via EmbedderTestContextMeta::GetTestMetalSurface for parity with the GetTestMetalContext method which exposes the test metal context. This eliminates the need for the more specific GetTextureInfo method since the texture info is accessible via the test context. This is a test refactoring with no semantic differences to the engine. Issue: flutter/flutter#116381
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 8, 2022
While I've sent a patch to mark the shell/platform/embedder/tests and fixtures directories as test exempt (since they are tests), by convention, tests should end in _unittests.* for C++ tests, and _test.* for Dart tests. This renames for consistency with other tests such as embedder_a11y_unittests.cc. Uncovered by flutter#38133 Related: flutter/cocoon#2340 Issue: flutter/flutter#116381
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 8, 2022
While I've sent a patch to mark the shell/platform/embedder/tests and fixtures directories as test exempt (since they are tests), by convention, tests should end in _unittests.* for C++ tests, and _test.* for Dart tests. This renames for consistency with other tests such as embedder_a11y_unittests.cc. Uncovered by flutter#38133 Related: flutter/cocoon#2340 Issue: flutter/flutter#116381
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Dec 8, 2022
While I've sent a patch to mark the shell/platform/embedder/tests and fixtures directories as test exempt (since they are tests), by convention, tests should end in _unittests.* for C++ tests, and _test.* for Dart tests. This renames for consistency with other tests such as embedder_a11y_unittests.cc. Uncovered by flutter#38133 Related: flutter/cocoon#2340 Issue: flutter/flutter#116381
auto-submit bot
pushed a commit
to flutter/cocoon
that referenced
this issue
Dec 8, 2022
Embedder API tests in the engine are implemented as C++ unit tests and
test infrastructure such as embedder_test_context_metal.{h,cc} (located
under the shell/platform/embedder/tests directory), which execute and
test Dart functions (and associated test assets) stored in the
shell/platform/embedder/fixtures directory.
Uncovered while sending flutter/engine#38133.
Issue: flutter/flutter#116381
cbracken
added a commit
to flutter/engine
that referenced
this issue
Dec 8, 2022
While I've sent a patch to mark the shell/platform/embedder/tests and fixtures directories as test exempt (since they are tests), by convention, tests should end in _unittests.* for C++ tests, and _test.* for Dart tests. This renames for consistency with other tests such as embedder_a11y_unittests.cc. Uncovered by #38133 Related: flutter/cocoon#2340 Issue: flutter/flutter#116381
8 tasks
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
FlutterRendererConfig'sget_next_drawable_callbackmember holds a callback used by the embedder API to request a drawable; in the case of Metal, this drawable is aFlutterMetalTexture.FlutterMetalTextureincludes adestruction_callbackmember which should be called when it's safe to release resources associated with theFlutterMetalTexture. This callback is not currently invoked for textures returned viaFlutterRendererConfig.get_next_drawable_callback; instead we unpack the returned struct and pass it on. See here:https://github.com/flutter/engine/blob/303e26e96561d9b76f2344e97a5fc32eb6dfdb9a/shell/platform/embedder/embedder.cc#L468-L481
In the compositor codepath, we do create an SkSurface that triggers the destruction callback, here:
https://github.com/flutter/engine/blob/303e26e96561d9b76f2344e97a5fc32eb6dfdb9a/shell/platform/embedder/embedder.cc#L868-L881
While the primary purpose of
FlutterRendererConfig.get_next_drawable_callbackis to get a hold of a texture, andSkSurfacewill release it, the embedder may need to release other resources allocated as part of texture creation, so it's important we triggerFlutterMetalTexture.destruction_callbackas part of that codepath.Identified in flutter/engine#37789.
The text was updated successfully, but these errors were encountered: