From 4e97f4907a749e547fe291695d67c8d310c14131 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 3 Jul 2024 17:17:38 -0700 Subject: [PATCH 1/4] [Impeller] dont recycle host buffers in between platform view compositing on iOS. --- flow/surface_frame.h | 7 +++++++ shell/gpu/gpu_surface_gl_impeller.cc | 5 +++-- shell/gpu/gpu_surface_metal_impeller.mm | 10 ++++++---- shell/gpu/gpu_surface_vulkan_impeller.cc | 5 ++--- .../ios/framework/Source/FlutterPlatformViews.mm | 6 ++++++ 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/flow/surface_frame.h b/flow/surface_frame.h index d88e9a21b6522..f1c4768bbf7eb 100644 --- a/flow/surface_frame.h +++ b/flow/surface_frame.h @@ -80,6 +80,13 @@ class SurfaceFrame { // Time at which this frame is scheduled to be presented. This is a hint // that can be passed to the platform to drop queued frames. std::optional presentation_time; + + // Whether this surface frame represents the last in a group frames that were + // submitted as part of a platform compositor interop step, such as during iOS + // platform view compositing. + // + // Defaults to true, which is generally a safe value. + bool frame_boundary = true; }; bool Submit(); diff --git a/shell/gpu/gpu_surface_gl_impeller.cc b/shell/gpu/gpu_surface_gl_impeller.cc index 2c45762a512fd..41ff5df3914e9 100644 --- a/shell/gpu/gpu_surface_gl_impeller.cc +++ b/shell/gpu/gpu_surface_gl_impeller.cc @@ -127,14 +127,15 @@ std::unique_ptr GPUSurfaceGLImpeller::AcquireFrame( impeller_dispatcher, SkIRect::MakeWH(cull_rect.width, cull_rect.height)); auto picture = impeller_dispatcher.EndRecordingAsPicture(); + const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; return renderer->Render( std::move(surface), fml::MakeCopyable( - [aiks_context, picture = std::move(picture)]( + [aiks_context, picture = std::move(picture), reset_host_buffer]( impeller::RenderTarget& render_target) -> bool { return aiks_context->Render(picture, render_target, - /*reset_host_buffer=*/true); + reset_host_buffer); })); }); diff --git a/shell/gpu/gpu_surface_metal_impeller.mm b/shell/gpu/gpu_surface_metal_impeller.mm index 67bd3997ef96c..eeff0a9a44160 100644 --- a/shell/gpu/gpu_surface_metal_impeller.mm +++ b/shell/gpu/gpu_surface_metal_impeller.mm @@ -187,12 +187,13 @@ impeller::DlDispatcher impeller_dispatcher(cull_rect); display_list->Dispatch(impeller_dispatcher, sk_cull_rect); auto picture = impeller_dispatcher.EndRecordingAsPicture(); + const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; return renderer->Render( std::move(surface), - fml::MakeCopyable([aiks_context, picture = std::move(picture)]( + fml::MakeCopyable([aiks_context, picture = std::move(picture), reset_host_buffer]( impeller::RenderTarget& render_target) -> bool { - return aiks_context->Render(picture, render_target, /*reset_host_buffer=*/true); + return aiks_context->Render(picture, render_target, reset_host_buffer); })); #endif }); @@ -307,11 +308,12 @@ display_list->Dispatch(impeller_dispatcher, sk_cull_rect); auto picture = impeller_dispatcher.EndRecordingAsPicture(); + const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; bool render_result = renderer->Render( std::move(surface), - fml::MakeCopyable([aiks_context, picture = std::move(picture)]( + fml::MakeCopyable([aiks_context, picture = std::move(picture), reset_host_buffer]( impeller::RenderTarget& render_target) -> bool { - return aiks_context->Render(picture, render_target, /*reset_host_buffer=*/true); + return aiks_context->Render(picture, render_target, reset_host_buffer); })); #endif if (!render_result) { diff --git a/shell/gpu/gpu_surface_vulkan_impeller.cc b/shell/gpu/gpu_surface_vulkan_impeller.cc index 3c0824f854832..3737381db78fa 100644 --- a/shell/gpu/gpu_surface_vulkan_impeller.cc +++ b/shell/gpu/gpu_surface_vulkan_impeller.cc @@ -117,9 +117,8 @@ std::unique_ptr GPUSurfaceVulkanImpeller::AcquireFrame( impeller_dispatcher, SkIRect::MakeWH(cull_rect.width, cull_rect.height)); auto picture = impeller_dispatcher.EndRecordingAsPicture(); - - return aiks_context->Render(picture, render_target, - /*reset_host_buffer=*/true); + const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; + return aiks_context->Render(picture, render_target, reset_host_buffer); #endif })); }); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm index 46aafd83e5efe..f0b90beebc11f 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm @@ -864,6 +864,12 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, slice->render_into(overlay_canvas); overlay_canvas->RestoreToCount(restore_count); + // This flutter view is never the last in a frame, since we always submit the + // underlay view last. + frame->set_submit_info({ + .frame_boundary = false + }); + layer->did_submit_last_frame = frame->Submit(); return layer; } From d22d8aea02d453ac42705e6df47c327adc16e575 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 3 Jul 2024 17:18:03 -0700 Subject: [PATCH 2/4] ++ --- flow/surface_frame.h | 6 +++--- shell/gpu/gpu_surface_gl_impeller.cc | 3 ++- shell/gpu/gpu_surface_metal_impeller.mm | 8 ++++---- shell/gpu/gpu_surface_vulkan_impeller.cc | 6 ++++-- .../darwin/ios/framework/Source/FlutterPlatformViews.mm | 4 +--- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/flow/surface_frame.h b/flow/surface_frame.h index f1c4768bbf7eb..da5516feaacab 100644 --- a/flow/surface_frame.h +++ b/flow/surface_frame.h @@ -81,9 +81,9 @@ class SurfaceFrame { // that can be passed to the platform to drop queued frames. std::optional presentation_time; - // Whether this surface frame represents the last in a group frames that were - // submitted as part of a platform compositor interop step, such as during iOS - // platform view compositing. + // Whether this surface frame represents the last in a group frames that + // were submitted as part of a platform compositor interop step, such as + // during iOS platform view compositing. // // Defaults to true, which is generally a safe value. bool frame_boundary = true; diff --git a/shell/gpu/gpu_surface_gl_impeller.cc b/shell/gpu/gpu_surface_gl_impeller.cc index 41ff5df3914e9..ca2c90da8f4a9 100644 --- a/shell/gpu/gpu_surface_gl_impeller.cc +++ b/shell/gpu/gpu_surface_gl_impeller.cc @@ -127,7 +127,8 @@ std::unique_ptr GPUSurfaceGLImpeller::AcquireFrame( impeller_dispatcher, SkIRect::MakeWH(cull_rect.width, cull_rect.height)); auto picture = impeller_dispatcher.EndRecordingAsPicture(); - const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; + const bool reset_host_buffer = + surface_frame.submit_info().frame_boundary; return renderer->Render( std::move(surface), diff --git a/shell/gpu/gpu_surface_metal_impeller.mm b/shell/gpu/gpu_surface_metal_impeller.mm index eeff0a9a44160..04a48a06c5e19 100644 --- a/shell/gpu/gpu_surface_metal_impeller.mm +++ b/shell/gpu/gpu_surface_metal_impeller.mm @@ -191,8 +191,8 @@ return renderer->Render( std::move(surface), - fml::MakeCopyable([aiks_context, picture = std::move(picture), reset_host_buffer]( - impeller::RenderTarget& render_target) -> bool { + fml::MakeCopyable([aiks_context, picture = std::move(picture), + reset_host_buffer](impeller::RenderTarget& render_target) -> bool { return aiks_context->Render(picture, render_target, reset_host_buffer); })); #endif @@ -311,8 +311,8 @@ const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; bool render_result = renderer->Render( std::move(surface), - fml::MakeCopyable([aiks_context, picture = std::move(picture), reset_host_buffer]( - impeller::RenderTarget& render_target) -> bool { + fml::MakeCopyable([aiks_context, picture = std::move(picture), + reset_host_buffer](impeller::RenderTarget& render_target) -> bool { return aiks_context->Render(picture, render_target, reset_host_buffer); })); #endif diff --git a/shell/gpu/gpu_surface_vulkan_impeller.cc b/shell/gpu/gpu_surface_vulkan_impeller.cc index 3737381db78fa..5da0e5eb511a5 100644 --- a/shell/gpu/gpu_surface_vulkan_impeller.cc +++ b/shell/gpu/gpu_surface_vulkan_impeller.cc @@ -117,8 +117,10 @@ std::unique_ptr GPUSurfaceVulkanImpeller::AcquireFrame( impeller_dispatcher, SkIRect::MakeWH(cull_rect.width, cull_rect.height)); auto picture = impeller_dispatcher.EndRecordingAsPicture(); - const bool reset_host_buffer = surface_frame.submit_info().frame_boundary; - return aiks_context->Render(picture, render_target, reset_host_buffer); + const bool reset_host_buffer = + surface_frame.submit_info().frame_boundary; + return aiks_context->Render(picture, render_target, + reset_host_buffer); #endif })); }); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm index f0b90beebc11f..551be716b15de 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm @@ -866,9 +866,7 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, // This flutter view is never the last in a frame, since we always submit the // underlay view last. - frame->set_submit_info({ - .frame_boundary = false - }); + frame->set_submit_info({.frame_boundary = false}); layer->did_submit_last_frame = frame->Submit(); return layer; From 99647165f3b766f21e79c0dd4bce29caa0f0add7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 3 Jul 2024 19:14:21 -0700 Subject: [PATCH 3/4] add real test (for real.) --- .../gpu_surface_metal_impeller_unittests.mm | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/shell/gpu/gpu_surface_metal_impeller_unittests.mm b/shell/gpu/gpu_surface_metal_impeller_unittests.mm index 383d57b2c666d..3a85ab230f00d 100644 --- a/shell/gpu/gpu_surface_metal_impeller_unittests.mm +++ b/shell/gpu/gpu_surface_metal_impeller_unittests.mm @@ -97,5 +97,37 @@ GPUCAMetalLayerHandle GetCAMetalLayer(const SkISize& frame_info) const override ASSERT_TRUE(frame->Submit()); } +TEST(GPUSurfaceMetalImpeller, ResetHostBufferBasedOnFrameBoundary) { + auto delegate = std::make_shared(); + delegate->SetDevice(); + + auto context = CreateImpellerContext(); + std::unique_ptr surface = + std::make_unique(delegate.get(), CreateImpellerContext()); + + ASSERT_TRUE(surface->IsValid()); + + auto& host_buffer = surface->GetAiksContext()->GetContentContext().GetTransientsBuffer(); + + EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 0u); + + auto frame = surface->AcquireFrame(SkISize::Make(100, 100)); + frame->set_submit_info({ + .frame_boundary = false + }); + + ASSERT_TRUE(frame->Submit()); + EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 0u); + + frame = surface->AcquireFrame(SkISize::Make(100, 100)); + frame->set_submit_info({ + .frame_boundary = true + }); + + + ASSERT_TRUE(frame->Submit()); + EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 1u); +} + } // namespace testing } // namespace flutter From bf1733fc41d620a0fd0815c8a7f3bf59944e65e5 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 3 Jul 2024 19:14:52 -0700 Subject: [PATCH 4/4] ++ --- shell/gpu/gpu_surface_metal_impeller_unittests.mm | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/shell/gpu/gpu_surface_metal_impeller_unittests.mm b/shell/gpu/gpu_surface_metal_impeller_unittests.mm index 3a85ab230f00d..13ccbba3f6906 100644 --- a/shell/gpu/gpu_surface_metal_impeller_unittests.mm +++ b/shell/gpu/gpu_surface_metal_impeller_unittests.mm @@ -112,18 +112,13 @@ GPUCAMetalLayerHandle GetCAMetalLayer(const SkISize& frame_info) const override EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 0u); auto frame = surface->AcquireFrame(SkISize::Make(100, 100)); - frame->set_submit_info({ - .frame_boundary = false - }); + frame->set_submit_info({.frame_boundary = false}); ASSERT_TRUE(frame->Submit()); EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 0u); frame = surface->AcquireFrame(SkISize::Make(100, 100)); - frame->set_submit_info({ - .frame_boundary = true - }); - + frame->set_submit_info({.frame_boundary = true}); ASSERT_TRUE(frame->Submit()); EXPECT_EQ(host_buffer.GetStateForTest().current_frame, 1u);