From 3d874d893a1c00f7d2a1984533a50aac0fe95e7a Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Fri, 19 Aug 2022 10:43:24 -0400 Subject: [PATCH 1/3] Do not override partial repaint support globally This needs to happen at a delegate level. fixes: https://github.com/flutter/flutter/issues/109858 --- flow/surface_frame.h | 2 +- shell/gpu/gpu_surface_gl_delegate.h | 2 +- shell/gpu/gpu_surface_gl_skia.cc | 8 +++----- shell/gpu/gpu_surface_gl_skia.h | 8 ++++---- shell/platform/android/android_surface_gl_skia.cc | 2 ++ shell/platform/embedder/embedder_surface_gl.cc | 10 ++++++++++ shell/platform/embedder/embedder_surface_gl.h | 3 +++ 7 files changed, 24 insertions(+), 11 deletions(-) diff --git a/flow/surface_frame.h b/flow/surface_frame.h index 961bfe5965241..d630ab331be40 100644 --- a/flow/surface_frame.h +++ b/flow/surface_frame.h @@ -51,7 +51,7 @@ class SurfaceFrame { // If existing damage is unspecified (nullopt), entire frame will be // rasterized (no partial redraw). To signal that there is no existing // damage use an empty SkIRect. - std::optional existing_damage; + std::optional existing_damage = std::nullopt; }; SurfaceFrame(sk_sp surface, diff --git a/shell/gpu/gpu_surface_gl_delegate.h b/shell/gpu/gpu_surface_gl_delegate.h index d1e31b96559f5..f097cba2d0266 100644 --- a/shell/gpu/gpu_surface_gl_delegate.h +++ b/shell/gpu/gpu_surface_gl_delegate.h @@ -30,7 +30,7 @@ struct GLFBOInfo { // This boolean flags whether the returned FBO supports partial repaint. const bool partial_repaint_enabled; // The frame buffer's existing damage (i.e. damage since it was last used). - const SkIRect existing_damage; + const std::optional existing_damage; }; // Information passed during presentation of a frame. diff --git a/shell/gpu/gpu_surface_gl_skia.cc b/shell/gpu/gpu_surface_gl_skia.cc index eafb6120b3774..06b3d5d3c5748 100644 --- a/shell/gpu/gpu_surface_gl_skia.cc +++ b/shell/gpu/gpu_surface_gl_skia.cc @@ -196,7 +196,6 @@ bool GPUSurfaceGLSkia::CreateOrUpdateSurfaces(const SkISize& size) { onscreen_surface_ = std::move(onscreen_surface); fbo_id_ = fbo_info.fbo_id; - supports_partial_repaint_ = fbo_info.partial_repaint_enabled; existing_damage_ = fbo_info.existing_damage; return true; @@ -250,9 +249,9 @@ std::unique_ptr GPUSurfaceGLSkia::AcquireFrame( }; framebuffer_info = delegate_->GLContextFramebufferInfo(); - // Partial repaint is enabled by default - framebuffer_info.supports_partial_repaint = supports_partial_repaint_; - framebuffer_info.existing_damage = existing_damage_; + if (!framebuffer_info.existing_damage.has_value()) { + framebuffer_info.existing_damage = existing_damage_; + } return std::make_unique(surface, std::move(framebuffer_info), submit_callback, std::move(context_switch)); @@ -303,7 +302,6 @@ bool GPUSurfaceGLSkia::PresentSurface(const SurfaceFrame& frame, onscreen_surface_ = std::move(new_onscreen_surface); fbo_id_ = fbo_info.fbo_id; - supports_partial_repaint_ = fbo_info.partial_repaint_enabled; existing_damage_ = fbo_info.existing_damage; } diff --git a/shell/gpu/gpu_surface_gl_skia.h b/shell/gpu/gpu_surface_gl_skia.h index 39f6e3f607dfe..35caee497eb6f 100644 --- a/shell/gpu/gpu_surface_gl_skia.h +++ b/shell/gpu/gpu_surface_gl_skia.h @@ -67,8 +67,10 @@ class GPUSurfaceGLSkia : public Surface { sk_sp onscreen_surface_; /// FBO backing the current `onscreen_surface_`. uint32_t fbo_id_ = 0; - // Private variable used to keep track of the current FBO's existing damage. - SkIRect existing_damage_ = SkIRect::MakeEmpty(); + // The current FBO's existing damage, as tracked by the GPU surface, delegates + // still have an option of overriding this damage with their own in + // `GLContextFrameBufferInfo`. + std::optional existing_damage_ = std::nullopt; bool context_owner_ = false; // TODO(38466): Refactor GPU surface APIs take into account the fact that an // external view embedder may want to render to the root surface. This is a @@ -76,8 +78,6 @@ class GPUSurfaceGLSkia : public Surface { // external view embedder is present. const bool render_to_surface_ = true; bool valid_ = false; - // Partial repaint is on by default. - bool supports_partial_repaint_ = true; // WeakPtrFactory must be the last member. fml::TaskRunnerAffineWeakPtrFactory weak_factory_; diff --git a/shell/platform/android/android_surface_gl_skia.cc b/shell/platform/android/android_surface_gl_skia.cc index 50ac30835bb1f..f4e3d5acd24c3 100644 --- a/shell/platform/android/android_surface_gl_skia.cc +++ b/shell/platform/android/android_surface_gl_skia.cc @@ -167,6 +167,8 @@ GLFBOInfo AndroidSurfaceGLSkia::GLContextFBO(GLFrameInfo frame_info) const { // The default window bound framebuffer on Android. return GLFBOInfo{ .fbo_id = 0, + .partial_repaint_enabled = onscreen_surface_->SupportsPartialRepaint(), + .existing_damage = onscreen_surface_->InitialDamage(), }; } diff --git a/shell/platform/embedder/embedder_surface_gl.cc b/shell/platform/embedder/embedder_surface_gl.cc index 88cdb316c5e5b..be74aa4852b7e 100644 --- a/shell/platform/embedder/embedder_surface_gl.cc +++ b/shell/platform/embedder/embedder_surface_gl.cc @@ -80,6 +80,16 @@ EmbedderSurfaceGL::GLProcResolver EmbedderSurfaceGL::GetGLProcResolver() const { return gl_dispatch_table_.gl_proc_resolver; } +// |GPUSurfaceGLDelegate| +SurfaceFrame::FramebufferInfo EmbedderSurfaceGL::GLContextFramebufferInfo() + const { + // Enable partial repaint by default on the embedders. + auto info = SurfaceFrame::FramebufferInfo{}; + info.supports_readback = true; + info.supports_partial_repaint = true; + return info; +} + // |EmbedderSurface| std::unique_ptr EmbedderSurfaceGL::CreateGPUSurface() { const bool render_to_surface = !external_view_embedder_; diff --git a/shell/platform/embedder/embedder_surface_gl.h b/shell/platform/embedder/embedder_surface_gl.h index 583f0f469a0e7..695aeea84b860 100644 --- a/shell/platform/embedder/embedder_surface_gl.h +++ b/shell/platform/embedder/embedder_surface_gl.h @@ -71,6 +71,9 @@ class EmbedderSurfaceGL final : public EmbedderSurface, // |GPUSurfaceGLDelegate| GLProcResolver GetGLProcResolver() const override; + // |GPUSurfaceGLDelegate| + SurfaceFrame::FramebufferInfo GLContextFramebufferInfo() const override; + FML_DISALLOW_COPY_AND_ASSIGN(EmbedderSurfaceGL); }; From 670076298fe458506423e9bd636a1212f452576c Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Fri, 19 Aug 2022 12:13:42 -0400 Subject: [PATCH 2/3] only enable partial repaint if embedders provide the callback --- shell/platform/embedder/embedder_surface_gl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shell/platform/embedder/embedder_surface_gl.cc b/shell/platform/embedder/embedder_surface_gl.cc index be74aa4852b7e..f08793429c4d5 100644 --- a/shell/platform/embedder/embedder_surface_gl.cc +++ b/shell/platform/embedder/embedder_surface_gl.cc @@ -86,7 +86,8 @@ SurfaceFrame::FramebufferInfo EmbedderSurfaceGL::GLContextFramebufferInfo() // Enable partial repaint by default on the embedders. auto info = SurfaceFrame::FramebufferInfo{}; info.supports_readback = true; - info.supports_partial_repaint = true; + info.supports_partial_repaint = + gl_dispatch_table_.gl_populate_existing_damage != nullptr; return info; } From b16d087ed8c979582f6ebb57d59d11f3f787b08b Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Fri, 19 Aug 2022 13:30:53 -0400 Subject: [PATCH 3/3] add test --- .../embedder/tests/embedder_unittests_gl.cc | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/shell/platform/embedder/tests/embedder_unittests_gl.cc b/shell/platform/embedder/tests/embedder_unittests_gl.cc index cd12376085349..08f653495f379 100644 --- a/shell/platform/embedder/tests/embedder_unittests_gl.cc +++ b/shell/platform/embedder/tests/embedder_unittests_gl.cc @@ -4065,6 +4065,26 @@ TEST_F(EmbedderTest, ExternalTextureGLRefreshedTooOften) { EXPECT_TRUE(resolve_called); } +TEST_F(EmbedderTest, + PresentInfoReceivesNoDamageWhenPopulateExistingDamageIsUndefined) { + auto& context = GetEmbedderContext(EmbedderTestContextType::kOpenGLContext); + + EmbedderConfigBuilder builder(context); + builder.SetOpenGLRendererConfig(SkISize::Make(800, 600)); + builder.SetDartEntrypoint("render_gradient"); + builder.GetRendererConfig().open_gl.populate_existing_damage = nullptr; + + auto engine = builder.LaunchEngine(); + ASSERT_TRUE(engine.is_valid()); + + // No damage should be passed. + static_cast(context).SetGLPresentCallback( + [](FlutterPresentInfo present_info) { + ASSERT_EQ(present_info.frame_damage.damage, nullptr); + ASSERT_EQ(present_info.buffer_damage.damage, nullptr); + }); +} + INSTANTIATE_TEST_SUITE_P( EmbedderTestGlVk, EmbedderTestMultiBackend,