From a0946753b6b0754084620ba5080e75e24e07d630 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 15 Nov 2024 19:18:13 -0800 Subject: [PATCH] EmbedderTest: Extract backend-specific user_data Replace the top-level public EmbedderBackingStoreProducer::UserData struct with backend-specific UserData classes, and make them private to those backends. UserData internals shouldn't be visible to/manipulated by unit tests and compositors, but instead constrained to the backing store producer. Issue: https://github.com/flutter/flutter/issues/158998 --- .../embedder_test_backingstore_producer.h | 29 +++---------- .../embedder_test_backingstore_producer_gl.cc | 43 ++++++++++++++++--- .../embedder_test_backingstore_producer_gl.h | 10 ++++- ...mbedder_test_backingstore_producer_metal.h | 10 ++++- ...bedder_test_backingstore_producer_metal.mm | 14 ++++++ ...der_test_backingstore_producer_software.cc | 24 +++++++++-- ...dder_test_backingstore_producer_software.h | 10 ++++- ...edder_test_backingstore_producer_vulkan.cc | 21 ++++++++- ...bedder_test_backingstore_producer_vulkan.h | 10 ++++- .../tests/embedder_test_compositor.cc | 5 +++ .../embedder/tests/embedder_test_compositor.h | 2 + .../tests/embedder_test_compositor_gl.cc | 20 +-------- .../embedder_test_compositor_software.cc | 8 ++-- .../tests/embedder_test_compositor_vulkan.cc | 4 +- .../embedder/tests/embedder_unittests.cc | 14 +++--- 15 files changed, 148 insertions(+), 76 deletions(-) diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer.h b/shell/platform/embedder/tests/embedder_test_backingstore_producer.h index 410fa608c0943..22be3176d79b2 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer.h +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer.h @@ -21,29 +21,6 @@ namespace flutter::testing { class EmbedderTestBackingStoreProducer { public: - struct UserData { - UserData() : surface(nullptr), image(nullptr){}; - - explicit UserData(sk_sp surface) - : surface(std::move(surface)), image(nullptr){}; - - UserData(sk_sp surface, FlutterVulkanImage* vk_image) - : surface(std::move(surface)), image(vk_image){}; - - sk_sp surface; - FlutterVulkanImage* image; -#ifdef SHELL_ENABLE_GL - UserData(sk_sp surface, - FlutterVulkanImage* vk_image, - std::unique_ptr gl_surface) - : surface(std::move(surface)), - image(vk_image), - gl_surface(std::move(gl_surface)){}; - - std::unique_ptr gl_surface; -#endif - }; - enum class RenderTargetType { kSoftwareBuffer, kSoftwareBuffer2, @@ -61,6 +38,12 @@ class EmbedderTestBackingStoreProducer { virtual bool Create(const FlutterBackingStoreConfig* config, FlutterBackingStore* backing_store_out) = 0; + virtual sk_sp GetSurface( + const FlutterBackingStore* backing_store) const = 0; + + virtual sk_sp MakeImageSnapshot( + const FlutterBackingStore* backing_store) const = 0; + protected: sk_sp context_; RenderTargetType type_; diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.cc b/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.cc index 5a088c61d3997..aae057f282e00 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.cc +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.cc @@ -14,6 +14,13 @@ namespace flutter::testing { +namespace { +struct UserData { + sk_sp surface; + std::unique_ptr gl_surface; +}; +} // namespace + EmbedderTestBackingStoreProducerGL::EmbedderTestBackingStoreProducerGL( sk_sp context, RenderTargetType type, @@ -39,6 +46,30 @@ bool EmbedderTestBackingStoreProducerGL::Create( }; } +sk_sp EmbedderTestBackingStoreProducerGL::GetSurface( + const FlutterBackingStore* backing_store) const { + UserData* user_data = reinterpret_cast(backing_store->user_data); + return user_data->surface; +} + +sk_sp EmbedderTestBackingStoreProducerGL::MakeImageSnapshot( + const FlutterBackingStore* backing_store) const { + UserData* user_data = reinterpret_cast(backing_store->user_data); + if (user_data->gl_surface != nullptr) { + // This backing store is an OpenGL Surface. + // We need to make it current so we can snapshot it. + user_data->gl_surface->MakeCurrent(); + + // GetRasterSurfaceSnapshot() does two + // gl_surface->makeImageSnapshot()'s. Doing a single + // ->makeImageSnapshot() will not work. + return user_data->gl_surface->GetRasterSurfaceSnapshot(); + } + + // Otherwise, it's a GL Texture or FrameBuffer. + return user_data->surface->makeImageSnapshot(); +} + bool EmbedderTestBackingStoreProducerGL::CreateFramebuffer( const FlutterBackingStoreConfig* config, FlutterBackingStore* backing_store_out) { @@ -75,8 +106,7 @@ bool EmbedderTestBackingStoreProducerGL::CreateFramebuffer( return false; } - auto user_data = new UserData(surface); - + auto user_data = new UserData{.surface = surface}; backing_store_out->type = kFlutterBackingStoreTypeOpenGL; backing_store_out->user_data = user_data; backing_store_out->open_gl.type = kFlutterOpenGLTargetTypeFramebuffer; @@ -124,8 +154,7 @@ bool EmbedderTestBackingStoreProducerGL::CreateTexture( return false; } - auto user_data = new UserData(surface); - + auto user_data = new UserData{.surface = surface}; backing_store_out->type = kFlutterBackingStoreTypeOpenGL; backing_store_out->user_data = user_data; backing_store_out->open_gl.type = kFlutterOpenGLTargetTypeTexture; @@ -165,8 +194,10 @@ bool EmbedderTestBackingStoreProducerGL::CreateSurface( auto sk_surface = surface->GetOnscreenSurface(); - auto user_data = new UserData(sk_surface, nullptr, std::move(surface)); - + auto user_data = new UserData{ + .surface = sk_surface, + .gl_surface = std::move(surface), + }; backing_store_out->type = kFlutterBackingStoreTypeOpenGL; backing_store_out->user_data = user_data; backing_store_out->open_gl.type = kFlutterOpenGLTargetTypeSurface; diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.h b/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.h index bf6bb09b5129f..83f564da03774 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.h +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_gl.h @@ -23,8 +23,14 @@ class EmbedderTestBackingStoreProducerGL virtual ~EmbedderTestBackingStoreProducerGL(); - virtual bool Create(const FlutterBackingStoreConfig* config, - FlutterBackingStore* backing_store_out); + bool Create(const FlutterBackingStoreConfig* config, + FlutterBackingStore* backing_store_out) override; + + sk_sp GetSurface( + const FlutterBackingStore* backing_store) const override; + + sk_sp MakeImageSnapshot( + const FlutterBackingStore* backing_store) const override; private: bool CreateFramebuffer(const FlutterBackingStoreConfig* config, diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.h b/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.h index e7d959c535cb4..1d89d06f64963 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.h +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.h @@ -18,8 +18,14 @@ class EmbedderTestBackingStoreProducerMetal virtual ~EmbedderTestBackingStoreProducerMetal(); - virtual bool Create(const FlutterBackingStoreConfig* config, - FlutterBackingStore* backing_store_out); + bool Create(const FlutterBackingStoreConfig* config, + FlutterBackingStore* backing_store_out) override; + + sk_sp GetSurface( + const FlutterBackingStore* backing_store) const override; + + sk_sp MakeImageSnapshot( + const FlutterBackingStore* backing_store) const override; private: std::unique_ptr test_metal_context_; diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.mm b/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.mm index fb920025ff998..42595e92f6a68 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.mm +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.mm @@ -4,6 +4,8 @@ #include "flutter/shell/platform/embedder/tests/embedder_test_backingstore_producer_metal.h" +#include + #include "flutter/fml/logging.h" #include "third_party/skia/include/core/SkColorSpace.h" #include "third_party/skia/include/gpu/ganesh/GrBackendSurface.h" @@ -56,4 +58,16 @@ return true; } +sk_sp EmbedderTestBackingStoreProducerMetal::GetSurface( + const FlutterBackingStore* backing_store) const { + FML_LOG(FATAL) << "Unimplemented."; + std::terminate(); +} + +sk_sp EmbedderTestBackingStoreProducerMetal::MakeImageSnapshot( + const FlutterBackingStore* backing_store) const { + FML_LOG(FATAL) << "Unimplemented."; + std::terminate(); +} + } // namespace flutter::testing diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.cc b/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.cc index 5c3c3ded58d90..5b753d8277261 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.cc +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.cc @@ -12,6 +12,12 @@ namespace flutter::testing { +namespace { +struct UserData { + sk_sp surface; +}; +} // namespace + EmbedderTestBackingStoreProducerSoftware:: EmbedderTestBackingStoreProducerSoftware( sk_sp context, @@ -46,6 +52,18 @@ bool EmbedderTestBackingStoreProducerSoftware::Create( } } +sk_sp EmbedderTestBackingStoreProducerSoftware::GetSurface( + const FlutterBackingStore* backing_store) const { + UserData* user_data = reinterpret_cast(backing_store->user_data); + return user_data->surface; +} + +sk_sp EmbedderTestBackingStoreProducerSoftware::MakeImageSnapshot( + const FlutterBackingStore* backing_store) const { + auto user_data = reinterpret_cast(backing_store->user_data); + return user_data->surface->makeImageSnapshot(); +} + bool EmbedderTestBackingStoreProducerSoftware::CreateSoftware( const FlutterBackingStoreConfig* config, FlutterBackingStore* backing_store_out) { @@ -64,8 +82,7 @@ bool EmbedderTestBackingStoreProducerSoftware::CreateSoftware( return false; } - auto user_data = new UserData(surface); - + auto user_data = new UserData{.surface = surface}; backing_store_out->type = kFlutterBackingStoreTypeSoftware; backing_store_out->user_data = user_data; backing_store_out->software.allocation = pixmap.addr(); @@ -101,8 +118,7 @@ bool EmbedderTestBackingStoreProducerSoftware::CreateSoftware2( return false; } - auto user_data = new UserData(surface); - + auto user_data = new UserData{.surface = surface}; backing_store_out->type = kFlutterBackingStoreTypeSoftware2; backing_store_out->user_data = user_data; backing_store_out->software2.struct_size = diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.h b/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.h index 52cde2487ad97..0fdce10136f2e 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.h +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_software.h @@ -20,8 +20,14 @@ class EmbedderTestBackingStoreProducerSoftware virtual ~EmbedderTestBackingStoreProducerSoftware(); - virtual bool Create(const FlutterBackingStoreConfig* config, - FlutterBackingStore* backing_store_out); + bool Create(const FlutterBackingStoreConfig* config, + FlutterBackingStore* backing_store_out) override; + + sk_sp GetSurface( + const FlutterBackingStore* backing_store) const override; + + sk_sp MakeImageSnapshot( + const FlutterBackingStore* backing_store) const override; private: bool CreateSoftware(const FlutterBackingStoreConfig* config, diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.cc b/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.cc index 154f507bc536a..195b68be4d0d8 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.cc +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.cc @@ -13,6 +13,13 @@ namespace flutter::testing { +namespace { +struct UserData { + sk_sp surface; + FlutterVulkanImage* image; +}; +} // namespace + EmbedderTestBackingStoreProducerVulkan::EmbedderTestBackingStoreProducerVulkan( sk_sp context, RenderTargetType type) @@ -83,7 +90,7 @@ bool EmbedderTestBackingStoreProducerVulkan::Create( // Collect all allocated resources in the destruction_callback. { - auto user_data = new UserData(surface, image); + auto user_data = new UserData{.surface = surface, .image = image}; backing_store_out->user_data = user_data; backing_store_out->vulkan.user_data = user_data; backing_store_out->vulkan.destruction_callback = [](void* user_data) { @@ -96,4 +103,16 @@ bool EmbedderTestBackingStoreProducerVulkan::Create( return true; } +sk_sp EmbedderTestBackingStoreProducerVulkan::GetSurface( + const FlutterBackingStore* backing_store) const { + UserData* user_data = reinterpret_cast(backing_store->user_data); + return user_data->surface; +} + +sk_sp EmbedderTestBackingStoreProducerVulkan::MakeImageSnapshot( + const FlutterBackingStore* backing_store) const { + UserData* user_data = reinterpret_cast(backing_store->user_data); + return user_data->surface->makeImageSnapshot(); +} + } // namespace flutter::testing diff --git a/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.h b/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.h index 5fb50875299c6..08a11282eb17e 100644 --- a/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.h +++ b/shell/platform/embedder/tests/embedder_test_backingstore_producer_vulkan.h @@ -19,8 +19,14 @@ class EmbedderTestBackingStoreProducerVulkan virtual ~EmbedderTestBackingStoreProducerVulkan(); - virtual bool Create(const FlutterBackingStoreConfig* config, - FlutterBackingStore* backing_store_out); + bool Create(const FlutterBackingStoreConfig* config, + FlutterBackingStore* backing_store_out) override; + + sk_sp GetSurface( + const FlutterBackingStore* backing_store) const override; + + sk_sp MakeImageSnapshot( + const FlutterBackingStore* backing_store) const override; private: fml::RefPtr test_vulkan_context_; diff --git a/shell/platform/embedder/tests/embedder_test_compositor.cc b/shell/platform/embedder/tests/embedder_test_compositor.cc index 38b5a6358d568..ab6963b44e722 100644 --- a/shell/platform/embedder/tests/embedder_test_compositor.cc +++ b/shell/platform/embedder/tests/embedder_test_compositor.cc @@ -51,6 +51,11 @@ bool EmbedderTestCompositor::CollectBackingStore( return true; } +sk_sp EmbedderTestCompositor::GetSurface( + const FlutterBackingStore* backing_store) const { + return backingstore_producer_->GetSurface(backing_store); +} + sk_sp EmbedderTestCompositor::GetLastComposition() { return last_composition_; } diff --git a/shell/platform/embedder/tests/embedder_test_compositor.h b/shell/platform/embedder/tests/embedder_test_compositor.h index 4e0b0f16464ba..67992deed1a22 100644 --- a/shell/platform/embedder/tests/embedder_test_compositor.h +++ b/shell/platform/embedder/tests/embedder_test_compositor.h @@ -45,6 +45,8 @@ class EmbedderTestCompositor { void SetPlatformViewRendererCallback( const PlatformViewRendererCallback& callback); + sk_sp GetSurface(const FlutterBackingStore* backing_store) const; + //---------------------------------------------------------------------------- /// @brief Allows tests to install a callback to notify them when the /// entire render tree has been finalized so they can run their diff --git a/shell/platform/embedder/tests/embedder_test_compositor_gl.cc b/shell/platform/embedder/tests/embedder_test_compositor_gl.cc index 92026955df5e7..1404b653dddfc 100644 --- a/shell/platform/embedder/tests/embedder_test_compositor_gl.cc +++ b/shell/platform/embedder/tests/embedder_test_compositor_gl.cc @@ -89,24 +89,8 @@ bool EmbedderTestCompositorGL::UpdateOffscrenComposition( switch (layer->type) { case kFlutterLayerContentTypeBackingStore: { - auto gl_user_data = - reinterpret_cast( - layer->backing_store->user_data); - - if (gl_user_data->gl_surface != nullptr) { - // This backing store is a OpenGL Surface. - // We need to make it current so we can snapshot it. - - gl_user_data->gl_surface->MakeCurrent(); - - // GetRasterSurfaceSnapshot() does two - // gl_surface->makeImageSnapshot()'s. Doing a single - // ->makeImageSnapshot() will not work. - layer_image = gl_user_data->gl_surface->GetRasterSurfaceSnapshot(); - } else { - layer_image = gl_user_data->surface->makeImageSnapshot(); - } - + layer_image = + backingstore_producer_->MakeImageSnapshot(layer->backing_store); // We don't clear the current surface here because we need the // EGL context to be current for surface->makeImageSnapshot() below. break; diff --git a/shell/platform/embedder/tests/embedder_test_compositor_software.cc b/shell/platform/embedder/tests/embedder_test_compositor_software.cc index 8469542d49c03..369a4f89e676f 100644 --- a/shell/platform/embedder/tests/embedder_test_compositor_software.cc +++ b/shell/platform/embedder/tests/embedder_test_compositor_software.cc @@ -68,13 +68,11 @@ bool EmbedderTestCompositorSoftware::UpdateOffscrenComposition( SkIPoint canvas_offset = SkIPoint::Make(0, 0); switch (layer->type) { - case kFlutterLayerContentTypeBackingStore: + case kFlutterLayerContentTypeBackingStore: { layer_image = - reinterpret_cast( - layer->backing_store->user_data) - ->surface->makeImageSnapshot(); - + backingstore_producer_->MakeImageSnapshot(layer->backing_store); break; + } case kFlutterLayerContentTypePlatformView: layer_image = platform_view_renderer_callback_ ? platform_view_renderer_callback_(*layer, nullptr) diff --git a/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc b/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc index d487483b4b34a..480c8c88b7575 100644 --- a/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc +++ b/shell/platform/embedder/tests/embedder_test_compositor_vulkan.cc @@ -82,9 +82,7 @@ bool EmbedderTestCompositorVulkan::UpdateOffscrenComposition( switch (layer->type) { case kFlutterLayerContentTypeBackingStore: layer_image = - reinterpret_cast( - layer->backing_store->user_data) - ->surface->makeImageSnapshot(); + backingstore_producer_->MakeImageSnapshot(layer->backing_store); break; case kFlutterLayerContentTypePlatformView: layer_image = diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc index 120dbb8202eab..4acdcde9aa170 100644 --- a/shell/platform/embedder/tests/embedder_unittests.cc +++ b/shell/platform/embedder/tests/embedder_unittests.cc @@ -2735,17 +2735,15 @@ static void expectSoftwareRenderingOutputMatches( ASSERT_TRUE(engine.is_valid()); context.GetCompositor().SetNextPresentCallback( - [&matches, &bytes, &latch](FlutterViewId view_id, - const FlutterLayer** layers, - size_t layers_count) { + [&context, &matches, &bytes, &latch](FlutterViewId view_id, + const FlutterLayer** layers, + size_t layers_count) { ASSERT_EQ(layers[0]->type, kFlutterLayerContentTypeBackingStore); ASSERT_EQ(layers[0]->backing_store->type, kFlutterBackingStoreTypeSoftware2); - matches = SurfacePixelDataMatchesBytes( - reinterpret_cast( - layers[0]->backing_store->software2.user_data) - ->surface.get(), - bytes); + sk_sp surface = + context.GetCompositor().GetSurface(layers[0]->backing_store); + matches = SurfacePixelDataMatchesBytes(surface.get(), bytes); latch.Signal(); });