[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 flutter/flutter#158486

Author: chinmaygarde

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",

impeller/renderer/backend/gles/surface_gles.cc

@@ -34,8 +34,8 @@ std::unique_ptr<Surface> SurfaceGLES::WrapFBO(
   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 @@ std::unique_ptr<Surface> SurfaceGLES::WrapFBO(
   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;

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

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> 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 @@ TextureGLES::TextureGLES(std::shared_ptr<ReactorGLES> 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;
 }

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<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 @@ class TextureGLES final : public Texture,
 
   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 @@ class TextureGLES final : public Texture,
 
   TextureGLES(std::shared_ptr<ReactorGLES> reactor,
               TextureDescriptor desc,
-              bool is_wrapped,
               std::optional<GLuint> fbo,
               std::optional<HandleGLES> external_handle);
 

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<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.";

shell/platform/android/image_external_texture_gl_impeller.cc

@@ -38,8 +38,10 @@ sk_sp<flutter::DlImage> ImageExternalTextureGLImpeller::CreateDlImage(
                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()) {

shell/platform/android/surface_texture_external_texture_gl_impeller.cc

@@ -33,8 +33,10 @@ void SurfaceTextureExternalTextureGLImpeller::ProcessFrame(
                  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();

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<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;

shell/platform/embedder/embedder_external_texture_gl.cc

@@ -147,8 +147,7 @@ sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpeller(
   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