From 7af0c70261d05c69c1af8d344e4fba5cf0847a50 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Thu, 1 Dec 2022 13:53:03 -0800 Subject: [PATCH 1/2] [embedder] Ensure FlutterMetalTexture cleanup call 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: https://github.com/flutter/flutter/issues/116381 --- shell/gpu/gpu_surface_metal_delegate.h | 4 ++++ shell/gpu/gpu_surface_metal_skia.mm | 21 +++++++++++++-------- shell/platform/embedder/embedder.cc | 2 ++ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/shell/gpu/gpu_surface_metal_delegate.h b/shell/gpu/gpu_surface_metal_delegate.h index cb3900e392c1f..a5dadedd9a429 100644 --- a/shell/gpu/gpu_surface_metal_delegate.h +++ b/shell/gpu/gpu_surface_metal_delegate.h @@ -26,9 +26,13 @@ typedef void* GPUCAMetalLayerHandle; // expected to be id typedef const void* GPUMTLTextureHandle; +typedef void (*GPUMTLDestructionCallback)(void* /* destruction_context */); + struct GPUMTLTextureInfo { int64_t texture_id; GPUMTLTextureHandle texture; + GPUMTLDestructionCallback destruction_callback; + void* destruction_context; }; enum class MTLRenderTargetType { kMTLTexture, kCAMetalLayer }; diff --git a/shell/gpu/gpu_surface_metal_skia.mm b/shell/gpu/gpu_surface_metal_skia.mm index 8e92d81ae27c8..d61931ce0688f 100644 --- a/shell/gpu/gpu_surface_metal_skia.mm +++ b/shell/gpu/gpu_surface_metal_skia.mm @@ -38,13 +38,15 @@ MsaaSampleCount sample_cnt, SkColorType color_type, sk_sp color_space, - const SkSurfaceProps* props) { + const SkSurfaceProps* props, + SkSurface::TextureReleaseProc release_proc, + SkSurface::ReleaseContext release_context) { GrMtlTextureInfo info; info.fTexture.reset([texture retain]); GrBackendTexture backend_texture(texture.width, texture.height, GrMipmapped::kNo, info); - return SkSurface::MakeFromBackendTexture(context, backend_texture, origin, - static_cast(sample_cnt), color_type, - std::move(color_space), props); + return SkSurface::MakeFromBackendTexture( + context, backend_texture, origin, static_cast(sample_cnt), color_type, + std::move(color_space), props, release_proc, release_context); } } // namespace @@ -137,7 +139,9 @@ msaa_samples_, // sample count kBGRA_8888_SkColorType, // color type nullptr, // colorspace - nullptr // surface properties + nullptr, // surface properties + nullptr, // release proc + nullptr // release context ); if (!surface) { @@ -203,9 +207,10 @@ return nullptr; } - sk_sp surface = - CreateSurfaceFromMetalTexture(context_.get(), mtl_texture, kTopLeft_GrSurfaceOrigin, - msaa_samples_, kBGRA_8888_SkColorType, nullptr, nullptr); + sk_sp surface = CreateSurfaceFromMetalTexture( + context_.get(), mtl_texture, kTopLeft_GrSurfaceOrigin, msaa_samples_, kBGRA_8888_SkColorType, + nullptr, nullptr, static_cast(texture.destruction_callback), + texture.destruction_context); if (!surface) { FML_LOG(ERROR) << "Could not create the SkSurface from the metal texture."; diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index 6240bc9efcf80..e0f3d47e1f05c 100644 --- a/shell/platform/embedder/embedder.cc +++ b/shell/platform/embedder/embedder.cc @@ -477,6 +477,8 @@ InferMetalPlatformViewCreationCallback( FlutterMetalTexture metal_texture = ptr(user_data, &frame_info); texture_info.texture_id = metal_texture.texture_id; texture_info.texture = metal_texture.texture; + texture_info.destruction_callback = metal_texture.destruction_callback; + texture_info.destruction_context = metal_texture.user_data; return texture_info; }; From 1e6ac95acc85afd7cb261557947000427ff938f7 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 2 Dec 2022 18:24:55 -0800 Subject: [PATCH 2/2] Add destruction test --- shell/platform/embedder/fixtures/main.dart | 19 ++++++++ .../tests/embedder_test_context_metal.cc | 14 +++++- .../tests/embedder_test_context_metal.h | 10 +++++ .../tests/embedder_unittests_metal.mm | 43 +++++++++++++++++++ 4 files changed, 85 insertions(+), 1 deletion(-) diff --git a/shell/platform/embedder/fixtures/main.dart b/shell/platform/embedder/fixtures/main.dart index 1726c5637389b..1049d5aea6f71 100644 --- a/shell/platform/embedder/fixtures/main.dart +++ b/shell/platform/embedder/fixtures/main.dart @@ -498,6 +498,25 @@ void can_composite_platform_views_with_platform_layer_on_bottom() { PlatformDispatcher.instance.scheduleFrame(); } +@pragma('vm:external-name', 'SignalBeginFrame') +external void signalBeginFrame(); + +@pragma('vm:entry-point') +void texture_destruction_callback_called_without_custom_compositor() async { + PlatformDispatcher.instance.onBeginFrame = (Duration duration) { + Color red = Color.fromARGB(127, 255, 0, 0); + Size size = Size(50.0, 150.0); + SceneBuilder builder = SceneBuilder(); + builder.pushOffset(0.0, 0.0); + builder.addPicture( + Offset(10.0, 10.0), CreateColoredBox(red, size)); // red - flutter + builder.pop(); + PlatformDispatcher.instance.views.first.render(builder.build()); + }; + PlatformDispatcher.instance.scheduleFrame(); +} + + @pragma('vm:entry-point') void can_render_scene_without_custom_compositor() { PlatformDispatcher.instance.onBeginFrame = (Duration duration) { diff --git a/shell/platform/embedder/tests/embedder_test_context_metal.cc b/shell/platform/embedder/tests/embedder_test_context_metal.cc index c649227b54800..7554a9278a5af 100644 --- a/shell/platform/embedder/tests/embedder_test_context_metal.cc +++ b/shell/platform/embedder/tests/embedder_test_context_metal.cc @@ -71,10 +71,22 @@ bool EmbedderTestContextMetal::PopulateExternalTexture( } } +TestMetalContext::TextureInfo EmbedderTestContextMetal::GetTextureInfo() { + return metal_surface_->GetTextureInfo(); +} + +void EmbedderTestContextMetal::SetNextDrawableCallback( + NextDrawableCallback next_drawable_callback) { + next_drawable_callback_ = std::move(next_drawable_callback); +} + FlutterMetalTexture EmbedderTestContextMetal::GetNextDrawable( const FlutterFrameInfo* frame_info) { - auto texture_info = metal_surface_->GetTextureInfo(); + if (next_drawable_callback_ != nullptr) { + return next_drawable_callback_(frame_info); + } + auto texture_info = metal_surface_->GetTextureInfo(); FlutterMetalTexture texture; texture.struct_size = sizeof(FlutterMetalTexture); texture.texture_id = texture_info.texture_id; diff --git a/shell/platform/embedder/tests/embedder_test_context_metal.h b/shell/platform/embedder/tests/embedder_test_context_metal.h index eae23eb252d7e..d4b5914e9ae01 100644 --- a/shell/platform/embedder/tests/embedder_test_context_metal.h +++ b/shell/platform/embedder/tests/embedder_test_context_metal.h @@ -20,6 +20,9 @@ class EmbedderTestContextMetal : public EmbedderTestContext { size_t h, FlutterMetalExternalTexture* output)>; + using NextDrawableCallback = + std::function; + explicit EmbedderTestContextMetal(std::string assets_path = ""); ~EmbedderTestContextMetal() override; @@ -45,6 +48,12 @@ class EmbedderTestContextMetal : public EmbedderTestContext { TestMetalContext* GetTestMetalContext(); + // Returns the TextureInfo for the test Metal surface. + TestMetalContext::TextureInfo GetTextureInfo(); + + // Override the default handling for GetNextDrawable. + void SetNextDrawableCallback(NextDrawableCallback next_drawable_callback); + FlutterMetalTexture GetNextDrawable(const FlutterFrameInfo* frame_info); private: @@ -56,6 +65,7 @@ class EmbedderTestContextMetal : public EmbedderTestContext { std::unique_ptr metal_context_; std::unique_ptr metal_surface_; size_t present_count_ = 0; + NextDrawableCallback next_drawable_callback_ = nullptr; void SetupSurface(SkISize surface_size) override; diff --git a/shell/platform/embedder/tests/embedder_unittests_metal.mm b/shell/platform/embedder/tests/embedder_unittests_metal.mm index 296edf7b5723e..351b9ea428d38 100644 --- a/shell/platform/embedder/tests/embedder_unittests_metal.mm +++ b/shell/platform/embedder/tests/embedder_unittests_metal.mm @@ -227,6 +227,49 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr ASSERT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); } +TEST_F(EmbedderTest, TextureDestructionCallbackCalledWithoutCustomCompositorMetal) { + EmbedderTestContextMetal& context = reinterpret_cast( + GetEmbedderContext(EmbedderTestContextType::kMetalContext)); + EmbedderConfigBuilder builder(context); + builder.SetMetalRendererConfig(SkISize::Make(800, 600)); + builder.SetDartEntrypoint("texture_destruction_callback_called_without_custom_compositor"); + + struct CollectContext { + int collect_count = 0; + fml::AutoResetWaitableEvent latch; + }; + + auto collect_context = std::make_unique(); + context.SetNextDrawableCallback([&context, &collect_context](const FlutterFrameInfo* frame_info) { + auto texture_info = context.GetTextureInfo(); + FlutterMetalTexture texture; + texture.struct_size = sizeof(FlutterMetalTexture); + texture.texture_id = texture_info.texture_id; + texture.texture = reinterpret_cast(texture_info.texture); + texture.user_data = collect_context.get(); + texture.destruction_callback = [](void* user_data) { + CollectContext* callback_collect_context = reinterpret_cast(user_data); + callback_collect_context->collect_count++; + callback_collect_context->latch.Signal(); + }; + return texture; + }); + + auto engine = builder.LaunchEngine(); + + // Send a window metrics events so frames may be scheduled. + FlutterWindowMetricsEvent event = {}; + event.struct_size = sizeof(event); + event.width = 800; + event.height = 600; + event.pixel_ratio = 1.0; + ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); + ASSERT_TRUE(engine.is_valid()); + + collect_context->latch.Wait(); + EXPECT_EQ(collect_context->collect_count, 1); +} + TEST_F(EmbedderTest, CompositorMustBeAbleToRenderKnownSceneMetal) { auto& context = GetEmbedderContext(EmbedderTestContextType::kMetalContext);