From 5bc3707a4a8839dc7fcb29fc5e77280da979faa5 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Nov 2024 12:03:22 -0800 Subject: [PATCH] [Impeller] Wrap provided FBO instead of defaulting to FBO0 and cleanup/document the texture API. 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. Not, 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 +++++++++++----- impeller/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 impeller/renderer/backend/gles/test/surface_gles_unittests.cc diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index 3562eae59b82c..b3e54aefae8b7 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/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/impeller/renderer/backend/gles/surface_gles.cc b/impeller/renderer/backend/gles/surface_gles.cc index d225831d0896d..494351913fde4 100644 --- a/impeller/renderer/backend/gles/surface_gles.cc +++ b/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/impeller/renderer/backend/gles/test/surface_gles_unittests.cc b/impeller/renderer/backend/gles/test/surface_gles_unittests.cc new file mode 100644 index 0000000000000..dbef03075a088 --- /dev/null +++ b/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/impeller/renderer/backend/gles/texture_gles.cc b/impeller/renderer/backend/gles/texture_gles.cc index f2167de515c08..1902fd22ae7c7 100644 --- a/impeller/renderer/backend/gles/texture_gles.cc +++ b/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/impeller/renderer/backend/gles/texture_gles.h b/impeller/renderer/backend/gles/texture_gles.h index 05ad1d952c11b..2f5a9d176104e 100644 --- a/impeller/renderer/backend/gles/texture_gles.h +++ b/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/impeller/toolkit/interop/impeller.cc b/impeller/toolkit/interop/impeller.cc index 03999398268ad..7681efe515f9c 100644 --- a/impeller/toolkit/interop/impeller.cc +++ b/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/shell/platform/android/image_external_texture_gl_impeller.cc b/shell/platform/android/image_external_texture_gl_impeller.cc index f14fea8882beb..7b2e5ce706016 100644 --- a/shell/platform/android/image_external_texture_gl_impeller.cc +++ b/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/shell/platform/android/surface_texture_external_texture_gl_impeller.cc b/shell/platform/android/surface_texture_external_texture_gl_impeller.cc index 227610a524889..69f4b5e2c6906 100644 --- a/shell/platform/android/surface_texture_external_texture_gl_impeller.cc +++ b/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/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index 05fed0e016d29..8bec5367a87b4 100644 --- a/shell/platform/embedder/embedder.cc +++ b/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/shell/platform/embedder/embedder_external_texture_gl.cc b/shell/platform/embedder/embedder_external_texture_gl.cc index 892b064af080f..852e7390a25ae 100644 --- a/shell/platform/embedder/embedder_external_texture_gl.cc +++ b/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