[Impeller] Wrap provided FBO instead of defaulting to FBO0 and cleanup/document the texture API. (#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
diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn
index 3562eae..b3e54ae 100644
--- a/impeller/renderer/backend/gles/BUILD.gn
+++ b/impeller/renderer/backend/gles/BUILD.gn
@@ -24,6 +24,7 @@
"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 d225831..4943519 100644
--- a/impeller/renderer/backend/gles/surface_gles.cc
+++ b/impeller/renderer/backend/gles/surface_gles.cc
@@ -34,8 +34,8 @@
color0_tex.storage_mode = StorageMode::kDevicePrivate;
ColorAttachment color0;
- color0.texture = std::make_shared<TextureGLES>(
- 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 @@
depth_stencil_texture_desc.usage = TextureUsage::kRenderTarget;
depth_stencil_texture_desc.sample_count = SampleCount::kCount1;
- auto depth_stencil_tex = std::make_shared<TextureGLES>(
- 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 0000000..dbef030
--- /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 f2167de..1902fd2 100644
--- a/impeller/renderer/backend/gles/texture_gles.cc
+++ b/impeller/renderer/backend/gles/texture_gles.cc
@@ -140,34 +140,52 @@
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> TextureGLES::WrapFBO(ReactorGLES::Ref reactor,
TextureDescriptor desc,
GLuint fbo) {
- return std::shared_ptr<TextureGLES>(
- new TextureGLES(std::move(reactor), desc, true, fbo, std::nullopt));
+ auto texture = std::shared_ptr<TextureGLES>(
+ new TextureGLES(std::move(reactor), desc, fbo, std::nullopt));
+ if (!texture->IsValid()) {
+ return nullptr;
+ }
+ return texture;
}
+std::shared_ptr<TextureGLES> 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<TextureGLES>(
+ new TextureGLES(std::move(reactor), desc, std::nullopt, external_handle));
+ if (!texture->IsValid()) {
+ return nullptr;
+ }
+ return texture;
+}
+
+std::shared_ptr<TextureGLES> 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<ReactorGLES> reactor,
TextureDescriptor desc,
- bool is_wrapped,
std::optional<GLuint> fbo,
std::optional<HandleGLES> external_handle)
: Texture(desc),
@@ -176,7 +194,7 @@
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 @@
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 05ad1d9..2f5a9d1 100644
--- a/impeller/renderer/backend/gles/texture_gles.h
+++ b/impeller/renderer/backend/gles/texture_gles.h
@@ -24,24 +24,56 @@
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<TextureGLES> 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<TextureGLES> 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<TextureGLES> CreatePlaceholder(
+ ReactorGLES::Ref reactor,
+ TextureDescriptor desc);
+
+ TextureGLES(ReactorGLES::Ref reactor, TextureDescriptor desc);
+
// |Texture|
~TextureGLES() override;
@@ -69,9 +101,22 @@
std::optional<GLuint> 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 @@
TextureGLES(std::shared_ptr<ReactorGLES> reactor,
TextureDescriptor desc,
- bool is_wrapped,
std::optional<GLuint> fbo,
std::optional<HandleGLES> external_handle);
diff --git a/impeller/toolkit/interop/impeller.cc b/impeller/toolkit/interop/impeller.cc
index 0399939..7681efe 100644
--- a/impeller/toolkit/interop/impeller.cc
+++ b/impeller/toolkit/interop/impeller.cc
@@ -528,13 +528,6 @@
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 @@
desc.mip_count = std::min(descriptor->mip_count, 1u);
desc.usage = TextureUsage::kShaderRead;
desc.compression_type = CompressionType::kLossless;
- auto texture = std::make_shared<TextureGLES>(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 f14fea8..7b2e5ce 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 @@
static_cast<int>(bounds.height())};
desc.mip_count = 1;
auto texture = std::make_shared<impeller::TextureGLES>(
- 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 227610a..69f4b5e 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 @@
static_cast<int>(bounds.height())};
desc.mip_count = 1;
texture_ = std::make_shared<impeller::TextureGLES>(
- 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 05fed0e..8bec536 100644
--- a/shell/platform/embedder/embedder.cc
+++ b/shell/platform/embedder/embedder.cc
@@ -1196,9 +1196,8 @@
depth_stencil_texture_desc.sample_count = impeller::SampleCount::kCount1;
}
- auto depth_stencil_tex = std::make_shared<impeller::TextureGLES>(
- 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 892b064..852e739 100644
--- a/shell/platform/embedder/embedder_external_texture_gl.cc
+++ b/shell/platform/embedder/embedder_external_texture_gl.cc
@@ -147,8 +147,7 @@
impeller::HandleGLES handle = context.GetReactor()->CreateHandle(
impeller::HandleType::kTexture, texture->target);
std::shared_ptr<impeller::TextureGLES> image =
- std::make_shared<impeller::TextureGLES>(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
