From 655a5d594743dde980f44720f2bea714cc67c886 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Nov 2024 16:59:18 -0800 Subject: [PATCH] [Impeller] Wrap provided FBO instead of defaulting to FBO0 and cleanup/document the texture API. (flutter/engine#56543) Previously, the FBO argument was dropped on the floor. The API was also confusing as the Android subsystems were using the IsWrapped call to sidestep texture contents initialization without actually performing any wrapping. Now, there are separate and documented calls to wrap a texture, wrap a framebuffer (as a texture), and to create a placeholder texture. Callers can also mark any texture as being initialized out of band instead of depending on overloading the meaning of IsWrapped. Fixes https://github.com/flutter/flutter/issues/158486 --- .../impeller/renderer/backend/gles/BUILD.gn | 1 + .../renderer/backend/gles/surface_gles.cc | 11 +-- .../gles/test/surface_gles_unittests.cc | 32 ++++++++ .../renderer/backend/gles/texture_gles.cc | 68 +++++++++++----- .../renderer/backend/gles/texture_gles.h | 80 ++++++++++++++----- .../impeller/toolkit/interop/impeller.cc | 15 ++-- .../image_external_texture_gl_impeller.cc | 6 +- ...ce_texture_external_texture_gl_impeller.cc | 6 +- .../shell/platform/embedder/embedder.cc | 5 +- .../embedder/embedder_external_texture_gl.cc | 3 +- 10 files changed, 163 insertions(+), 64 deletions(-) create mode 100644 engine/src/flutter/impeller/renderer/backend/gles/test/surface_gles_unittests.cc diff --git a/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn b/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn index 3562eae59b8..b3e54aefae8 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn +++ b/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn @@ -24,6 +24,7 @@ impeller_component("gles_unittests") { "test/proc_table_gles_unittests.cc", "test/reactor_unittests.cc", "test/specialization_constants_unittests.cc", + "test/surface_gles_unittests.cc", ] deps = [ ":gles", diff --git a/engine/src/flutter/impeller/renderer/backend/gles/surface_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/surface_gles.cc index d225831d089..494351913fd 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/surface_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/surface_gles.cc @@ -34,8 +34,8 @@ std::unique_ptr SurfaceGLES::WrapFBO( color0_tex.storage_mode = StorageMode::kDevicePrivate; ColorAttachment color0; - color0.texture = std::make_shared( - gl_context.GetReactor(), color0_tex, TextureGLES::IsWrapped::kWrapped); + color0.texture = + TextureGLES::WrapFBO(gl_context.GetReactor(), color0_tex, fbo); color0.clear_color = Color::DarkSlateGray(); color0.load_action = LoadAction::kClear; color0.store_action = StoreAction::kStore; @@ -47,9 +47,10 @@ std::unique_ptr SurfaceGLES::WrapFBO( depth_stencil_texture_desc.usage = TextureUsage::kRenderTarget; depth_stencil_texture_desc.sample_count = SampleCount::kCount1; - auto depth_stencil_tex = std::make_shared( - gl_context.GetReactor(), depth_stencil_texture_desc, - TextureGLES::IsWrapped::kWrapped); + auto depth_stencil_tex = + TextureGLES::CreatePlaceholder(gl_context.GetReactor(), // + depth_stencil_texture_desc // + ); DepthAttachment depth0; depth0.clear_depth = 0; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/surface_gles_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/surface_gles_unittests.cc new file mode 100644 index 00000000000..dbef03075a0 --- /dev/null +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/surface_gles_unittests.cc @@ -0,0 +1,32 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/impeller/playground/playground_test.h" +#include "flutter/impeller/renderer/backend/gles/context_gles.h" +#include "flutter/impeller/renderer/backend/gles/surface_gles.h" +#include "flutter/impeller/renderer/backend/gles/texture_gles.h" +#include "flutter/testing/testing.h" + +namespace impeller::testing { + +using SurfaceGLESTest = PlaygroundTest; +INSTANTIATE_OPENGLES_PLAYGROUND_SUITE(SurfaceGLESTest); + +TEST_P(SurfaceGLESTest, CanWrapNonZeroFBO) { + const GLuint fbo = 1988; + auto surface = + SurfaceGLES::WrapFBO(GetContext(), []() { return true; }, fbo, + PixelFormat::kR8G8B8A8UNormInt, {100, 100}); + ASSERT_TRUE(!!surface); + ASSERT_TRUE(surface->IsValid()); + ASSERT_TRUE(surface->GetRenderTarget().HasColorAttachment(0)); + const auto& texture = TextureGLES::Cast( + *(surface->GetRenderTarget().GetColorAttachments().at(0).texture)); + auto wrapped = texture.GetFBO(); + ASSERT_TRUE(wrapped.has_value()); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) + ASSERT_EQ(wrapped.value(), fbo); +} + +} // namespace impeller::testing diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc index f2167de515c..1902fd22ae7 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc @@ -140,34 +140,52 @@ HandleType ToHandleType(TextureGLES::Type type) { FML_UNREACHABLE(); } -TextureGLES::TextureGLES(ReactorGLES::Ref reactor, TextureDescriptor desc) - : TextureGLES(std::move(reactor), desc, false, std::nullopt, std::nullopt) { -} - -TextureGLES::TextureGLES(ReactorGLES::Ref reactor, - TextureDescriptor desc, - enum IsWrapped wrapped) - : TextureGLES(std::move(reactor), desc, true, std::nullopt, std::nullopt) {} - -TextureGLES::TextureGLES(ReactorGLES::Ref reactor, - TextureDescriptor desc, - HandleGLES external_handle) - : TextureGLES(std::move(reactor), - desc, - true, - std::nullopt, - external_handle) {} - std::shared_ptr TextureGLES::WrapFBO(ReactorGLES::Ref reactor, TextureDescriptor desc, GLuint fbo) { - return std::shared_ptr( - new TextureGLES(std::move(reactor), desc, true, fbo, std::nullopt)); + auto texture = std::shared_ptr( + new TextureGLES(std::move(reactor), desc, fbo, std::nullopt)); + if (!texture->IsValid()) { + return nullptr; + } + return texture; } +std::shared_ptr TextureGLES::WrapTexture( + ReactorGLES::Ref reactor, + TextureDescriptor desc, + HandleGLES external_handle) { + if (external_handle.IsDead()) { + VALIDATION_LOG << "Cannot wrap a dead handle."; + return nullptr; + } + if (external_handle.type != HandleType::kTexture) { + VALIDATION_LOG << "Cannot wrap a non-texture handle."; + return nullptr; + } + auto texture = std::shared_ptr( + new TextureGLES(std::move(reactor), desc, std::nullopt, external_handle)); + if (!texture->IsValid()) { + return nullptr; + } + return texture; +} + +std::shared_ptr TextureGLES::CreatePlaceholder( + ReactorGLES::Ref reactor, + TextureDescriptor desc) { + return TextureGLES::WrapFBO(std::move(reactor), desc, 0u); +} + +TextureGLES::TextureGLES(ReactorGLES::Ref reactor, TextureDescriptor desc) + : TextureGLES(std::move(reactor), // + desc, // + std::nullopt, // + std::nullopt // + ) {} + TextureGLES::TextureGLES(std::shared_ptr reactor, TextureDescriptor desc, - bool is_wrapped, std::optional fbo, std::optional external_handle) : Texture(desc), @@ -176,7 +194,7 @@ TextureGLES::TextureGLES(std::shared_ptr reactor, handle_(external_handle.has_value() ? external_handle.value() : reactor_->CreateHandle(ToHandleType(type_))), - is_wrapped_(is_wrapped), + is_wrapped_(fbo.has_value() || external_handle.has_value()), wrapped_fbo_(fbo) { // Ensure the texture descriptor itself is valid. if (!GetTextureDescriptor().IsValid()) { @@ -479,6 +497,12 @@ bool TextureGLES::Bind() const { return true; } +void TextureGLES::MarkContentsInitialized() { + for (size_t i = 0; i < slices_initialized_.size(); i++) { + slices_initialized_[i] = true; + } +} + void TextureGLES::MarkSliceInitialized(size_t slice) const { slices_initialized_[slice] = true; } diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h index 05ad1d952c1..2f5a9d17610 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h @@ -24,24 +24,56 @@ class TextureGLES final : public Texture, kRenderBufferMultisampled, }; - enum class IsWrapped { - kWrapped, - }; - - TextureGLES(ReactorGLES::Ref reactor, TextureDescriptor desc); - - TextureGLES(ReactorGLES::Ref reactor, - TextureDescriptor desc, - IsWrapped wrapped); - - TextureGLES(ReactorGLES::Ref reactor, - TextureDescriptor desc, - HandleGLES external_handle); - + //---------------------------------------------------------------------------- + /// @brief Create a texture by wrapping an external framebuffer object + /// whose lifecycle is owned by the caller. + /// + /// This is useful for creating a render target for the default + /// window managed framebuffer. + /// + /// @param[in] reactor The reactor + /// @param[in] desc The description + /// @param[in] fbo The fbo + /// + /// @return If a texture representation of the framebuffer could be + /// created. + /// static std::shared_ptr WrapFBO(ReactorGLES::Ref reactor, TextureDescriptor desc, GLuint fbo); + //---------------------------------------------------------------------------- + /// @brief Create a texture by wrapping an external OpenGL texture + /// handle. Ownership of the texture handle is assumed by the + /// reactor. + /// + /// @param[in] reactor The reactor + /// @param[in] desc The description + /// @param[in] external_handle The external handle + /// + /// @return If a texture representation of the framebuffer could be + /// created. + /// + static std::shared_ptr WrapTexture(ReactorGLES::Ref reactor, + TextureDescriptor desc, + HandleGLES external_handle); + + //---------------------------------------------------------------------------- + /// @brief Create a "texture" that is never expected to be bound/unbound + /// explicitly or initialized in any way. It only exists to setup + /// a render pass description. + /// + /// @param[in] reactor The reactor + /// @param[in] desc The description + /// + /// @return If a texture placeholder could be created. + /// + static std::shared_ptr CreatePlaceholder( + ReactorGLES::Ref reactor, + TextureDescriptor desc); + + TextureGLES(ReactorGLES::Ref reactor, TextureDescriptor desc); + // |Texture| ~TextureGLES() override; @@ -69,9 +101,22 @@ class TextureGLES final : public Texture, std::optional GetFBO() const; - // For non cubemap textures, 0 indicates uninitialized and 1 indicates - // initialized. For cubemap textures, each face is initialized separately with - // each bit tracking the initialization of the corresponding slice. + //---------------------------------------------------------------------------- + /// @brief Indicates that all texture storage has already been allocated + /// and contents initialized. + /// + /// This is similar to calling `MarkSliceInitialized` with all + /// slices. + /// + /// @see MarkSliceInitialized. + /// + void MarkContentsInitialized(); + + //---------------------------------------------------------------------------- + /// @brief Indicates that a specific texture slice has been initialized. + /// + /// @param[in] slice The slice to mark as being initialized. + /// void MarkSliceInitialized(size_t slice) const; bool IsSliceInitialized(size_t slice) const; @@ -87,7 +132,6 @@ class TextureGLES final : public Texture, TextureGLES(std::shared_ptr reactor, TextureDescriptor desc, - bool is_wrapped, std::optional fbo, std::optional external_handle); diff --git a/engine/src/flutter/impeller/toolkit/interop/impeller.cc b/engine/src/flutter/impeller/toolkit/interop/impeller.cc index 03999398268..7681efe515f 100644 --- a/engine/src/flutter/impeller/toolkit/interop/impeller.cc +++ b/engine/src/flutter/impeller/toolkit/interop/impeller.cc @@ -528,13 +528,6 @@ ImpellerTexture ImpellerTextureCreateWithOpenGLTextureHandleNew( const auto& impeller_context_gl = ContextGLES::Cast(*impeller_context); const auto& reactor = impeller_context_gl.GetReactor(); - auto wrapped_external_gl_handle = - reactor->CreateHandle(HandleType::kTexture, external_gl_handle); - if (wrapped_external_gl_handle.IsDead()) { - VALIDATION_LOG << "Could not wrap external handle."; - return nullptr; - } - TextureDescriptor desc; desc.storage_mode = StorageMode::kDevicePrivate; desc.type = TextureType::kTexture2D; @@ -543,9 +536,11 @@ ImpellerTexture ImpellerTextureCreateWithOpenGLTextureHandleNew( desc.mip_count = std::min(descriptor->mip_count, 1u); desc.usage = TextureUsage::kShaderRead; desc.compression_type = CompressionType::kLossless; - auto texture = std::make_shared(reactor, // - desc, // - wrapped_external_gl_handle // + + auto texture = TextureGLES::WrapTexture( + reactor, // + desc, // + reactor->CreateHandle(HandleType::kTexture, external_gl_handle) // ); if (!texture || !texture->IsValid()) { VALIDATION_LOG << "Could not wrap external texture."; diff --git a/engine/src/flutter/shell/platform/android/image_external_texture_gl_impeller.cc b/engine/src/flutter/shell/platform/android/image_external_texture_gl_impeller.cc index f14fea8882b..7b2e5ce7060 100644 --- a/engine/src/flutter/shell/platform/android/image_external_texture_gl_impeller.cc +++ b/engine/src/flutter/shell/platform/android/image_external_texture_gl_impeller.cc @@ -38,8 +38,10 @@ sk_sp ImageExternalTextureGLImpeller::CreateDlImage( static_cast(bounds.height())}; desc.mip_count = 1; auto texture = std::make_shared( - impeller_context_->GetReactor(), desc, - impeller::TextureGLES::IsWrapped::kWrapped); + impeller_context_->GetReactor(), desc); + // The contents will be initialized later in the call to + // `glEGLImageTargetTexture2DOES` instead of by Impeller. + texture->MarkContentsInitialized(); texture->SetCoordinateSystem( impeller::TextureCoordinateSystem::kUploadFromHost); if (!texture->Bind()) { diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc index 227610a5248..69f4b5e2c69 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc +++ b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc @@ -33,8 +33,10 @@ void SurfaceTextureExternalTextureGLImpeller::ProcessFrame( static_cast(bounds.height())}; desc.mip_count = 1; texture_ = std::make_shared( - impeller_context_->GetReactor(), desc, - impeller::TextureGLES::IsWrapped::kWrapped); + impeller_context_->GetReactor(), desc); + // The contents will be initialized later in the call to `Attach` instead of + // by Impeller. + texture_->MarkContentsInitialized(); texture_->SetCoordinateSystem( impeller::TextureCoordinateSystem::kUploadFromHost); auto maybe_handle = texture_->GetGLHandle(); diff --git a/engine/src/flutter/shell/platform/embedder/embedder.cc b/engine/src/flutter/shell/platform/embedder/embedder.cc index 05fed0e016d..8bec5367a87 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder.cc +++ b/engine/src/flutter/shell/platform/embedder/embedder.cc @@ -1196,9 +1196,8 @@ MakeRenderTargetFromBackingStoreImpeller( depth_stencil_texture_desc.sample_count = impeller::SampleCount::kCount1; } - auto depth_stencil_tex = std::make_shared( - gl_context.GetReactor(), depth_stencil_texture_desc, - impeller::TextureGLES::IsWrapped::kWrapped); + auto depth_stencil_tex = impeller::TextureGLES::CreatePlaceholder( + gl_context.GetReactor(), depth_stencil_texture_desc); impeller::DepthAttachment depth0; depth0.clear_depth = 0; diff --git a/engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc b/engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc index 892b064af08..852e7390a25 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc +++ b/engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc @@ -147,8 +147,7 @@ sk_sp EmbedderExternalTextureGL::ResolveTextureImpeller( impeller::HandleGLES handle = context.GetReactor()->CreateHandle( impeller::HandleType::kTexture, texture->target); std::shared_ptr image = - std::make_shared(context.GetReactor(), desc, - handle); + impeller::TextureGLES::WrapTexture(context.GetReactor(), desc, handle); if (!image) { // In case Skia rejects the image, call the release proc so that