[Tizen] Re-implemented render external texture gl for impeller to avoid change FlutterOpenGLTexture and use deprecated api #22

xiaowei-guan · 2026-01-08T06:06:00Z

We have supported render external texture gl for Impeller
flutter-tizen/engine#368
But this PR has two issues:

It modify FlutterOpenGLTexture, it added additional variables, not a good design.

  /// The pixel data buffer.
  const uint8_t* buffer;
  /// The size of pixel buffer.
  size_t buffer_size;
  /// Callback invoked that the gpu surface texture start binding.
  BoolCallback bind_callback;
  /// The type of the texture.
  FlutterGLImpellerTextureType impeller_texture_type;

It uses Deprecated API

  // Deprecated: use BlitPass::AddCopy instead.
  [[nodiscard]] bool SetContents(const uint8_t* contents,
                                 size_t length,
                                 size_t slice = 0,
                                 bool is_opaque = false);

So I tried to revert offical PR to render external gl for impeller
flutter/engine#56277
But this PR doesn't work on Tizen platform, I think This PR may not have been verified.
To avoid change FlutterOpenGLTexture and use deprecated api, so I re-implemented render external texture gl for impeller based on this offical PR.
Fix flutter-tizen/embedder#131

The offical PR doesn't work

Copilot

Pull request overview

This PR re-implements external texture rendering for Impeller on Tizen to address two critical design issues: (1) avoiding modifications to the public FlutterOpenGLTexture API by removing added fields (buffer, buffer_size, bind_callback), and (2) eliminating the use of deprecated SetContents API in favor of TextureGLES::WrapTexture.

Key changes:

Removed buffer management fields from FlutterOpenGLTexture struct
Replaced ResolveTextureImpellerPixelbuffer and ResolveTextureImpellerSurface with a unified CreateImpellerTexture approach using WrapTexture
Added texture change detection via IsExternalTextureChanged to cache and reuse texture objects

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
embedder.h	Removed buffer-related fields and bind_callback from FlutterOpenGLTexture; moved BoolCallback typedef after FlutterOpenGLSurface definition
embedder_external_texture_gl.h	Added texture_image_ and desc_ members for caching; replaced two Impeller resolution methods with IsExternalTextureChanged and CreateImpellerTexture
embedder_external_texture_gl.cc	Implemented texture caching with change detection; replaced deprecated SetContents with WrapTexture API; added cleanup callback registration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T06:29:43Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+  if (texture_image_ == nullptr) {
+    texture_image_ = CreateImpellerTexture(aiks_context, texture.get());
+  } else {
+    if (IsExternalTextureChanged(texture.get())) {
+      texture_image_.reset();
+      texture_image_ = CreateImpellerTexture(aiks_context, texture.get());
    }
-    return nullptr;
  }


When the texture hasn't changed (IsExternalTextureChanged returns false at line 235), the code doesn't call the destruction_callback for the newly obtained texture object. The external_texture_callback_ returns a new FlutterOpenGLTexture object each time it's called (line 216-217), and each of these objects may have associated embedder resources that need to be cleaned up via the destruction_callback.

Without calling the destruction_callback for unchanged textures, embedder resources associated with these texture objects will leak on every frame where the texture hasn't changed.

Copilot · 2026-01-08T06:29:43Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+bool EmbedderExternalTextureGL::IsExternalTextureChanged(
+    FlutterOpenGLTexture* texture) {
+  if (static_cast<int64_t>(texture->width) != desc_.size.width ||
+      static_cast<int64_t>(texture->height) != desc_.size.height) {
+    return true;
+  }
+  auto handle = texture_image_->GetGLHandle();
+  if (!handle.has_value()) {
+    return true;
+  }
+
+  if (handle.value() != texture->name) {
+    return true;
+  }
+  return false;
+}


The IsExternalTextureChanged function assumes texture_image_ is non-null, but it's called inside ResolveTextureImpeller after checking if texture_image_ is nullptr on line 232. However, this check could fail if texture_image_ is not null but GetGLHandle() returns no value. In such cases, line 138 accesses texture_image_->GetGLHandle() which could be called on a null or invalid texture_image_.

Additionally, this function is called at line 235 where texture_image_ is guaranteed to be non-null due to the else clause, but there's no guard against texture_image_ being in an invalid state.

Copilot · 2026-01-08T06:29:43Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+  if (static_cast<int64_t>(texture->width) != desc_.size.width ||
+      static_cast<int64_t>(texture->height) != desc_.size.height) {
+    return true;


The member variable desc_ is being used to store texture state for comparison in IsExternalTextureChanged, but it's not properly initialized before the first call. On the first invocation where texture_image_ is nullptr (line 232), CreateImpellerTexture is called which sets desc_ at line 165. However, if IsExternalTextureChanged is called when texture_image_ is non-null but desc_ hasn't been initialized from a previous successful CreateImpellerTexture call, the comparison will use uninitialized values.

While this may work in practice because desc_ is initialized during CreateImpellerTexture, the logic is fragile and relies on implicit ordering assumptions.

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

Copilot · 2026-01-08T06:29:44Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+  if (texture->destruction_callback &&
+      !context.GetReactor()->RegisterCleanupCallback(
+          handle,
+          [callback = texture->destruction_callback,
+           user_data = texture->user_data]() { callback(user_data); })) {
+    FML_LOG(ERROR) << "Could not register destruction callback for texture: "
+                   << texture->name;
+    // Clean up the texture since we couldn't register the callback
+    texture_image.reset();
+    return nullptr;
  }


When RegisterCleanupCallback fails (line 198), the code resets texture_image but doesn't explicitly call the destruction_callback for the current texture. The destruction_callback needs to be invoked to allow the embedder to clean up resources immediately when the cleanup callback registration fails. Without this, embedder resources associated with the texture parameter may leak.

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

Copilot · 2026-01-08T06:29:44Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+  // Validate texture parameters
+  if (size.width() <= 0 || size.height() <= 0) {
+    FML_LOG(ERROR) << "Invalid texture size: " << size.width() << "x"
+                   << size.height();
    return nullptr;
  }


The validation of texture size occurs after fetching the texture from external_texture_callback_ (line 216), but before using it. If this validation fails, the destruction_callback for the texture object won't be called, leading to a resource leak. The texture object should have its destruction_callback invoked before returning.

1.Add validation for texture dimensions(width/height > 0) 2.Add validation for texture name. 3.Add datailed error logging.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.h

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

Copilot · 2026-01-09T11:58:29Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpeller(
+    int64_t texture_id,
+    impeller::AiksContext* aiks_context,
+    const SkISize& size) {
+  std::unique_ptr<FlutterOpenGLTexture> texture =
+      external_texture_callback_(texture_id, size.width(), size.height());
+
+  if (!texture) {
+    FML_LOG(ERROR) << "External texture callback returned null for texture_id: "
+                   << texture_id;
    return nullptr;
  }

-  if (!texture->bind_callback(texture->user_data)) {
-    if (texture->destruction_callback) {
-      texture->destruction_callback(texture->user_data);
-    }
+  if (!ValidateTextureParameters(texture.get(), size)) {
    return nullptr;
  }

-  if (texture->destruction_callback) {
-    texture->destruction_callback(texture->user_data);
+  if (!texture_image_) {
+    texture_image_ = CreateImpellerTexture(aiks_context, texture.get());
+  } else {
+    if (IsExternalTextureChanged(texture.get(), desc_)) {
+      texture_image_.reset();
+      texture_image_ = CreateImpellerTexture(aiks_context, texture.get());
+    }
+  }
+
+  if (texture_image_ == nullptr) {
+    FML_LOG(ERROR) << "Failed to create Impeller texture for texture_id: "
+                   << texture_id;
+    return nullptr;
  }

-  return impeller::DlImageImpeller::Make(image);
+  return impeller::DlImageImpeller::Make(texture_image_);
 }


There's a lifecycle management issue with the destruction callbacks. Every call to ResolveTextureImpeller invokes external_texture_callback_ which returns a new FlutterOpenGLTexture object (line 230-231). If the texture hasn't changed and texture_image_ is reused (lines 245-250), the destruction_callback from the newly returned texture is never invoked - it goes out of scope when the unique_ptr is destroyed at the end of the function. This means the embedder's cleanup code won't be called for that texture instance. The destruction callback should be invoked for each FlutterOpenGLTexture returned by the callback, regardless of whether texture_image_ is reused or recreated.

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

Copilot · 2026-01-09T11:58:29Z

engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc

+bool EmbedderExternalTextureGL::IsExternalTextureChanged(
+    FlutterOpenGLTexture* texture,
+    impeller::TextureDescriptor& desc) {
+  if (static_cast<int64_t>(texture->width) != desc.size.width ||
+      static_cast<int64_t>(texture->height) != desc.size.height) {
+    return true;
+  }

-  if (!texture) {
-    return nullptr;
+  auto handle = texture_image_->GetGLHandle();
+  if (!handle.has_value()) {
+    return true;
  }

-  if (texture->bind_callback != nullptr) {
-    return ResolveTextureImpellerSurface(aiks_context, std::move(texture));
-  } else {
-    return ResolveTextureImpellerPixelbuffer(aiks_context, std::move(texture));
+  if (handle.value() != texture->name) {
+    return true;
  }
+


The IsExternalTextureChanged method takes a desc parameter but this appears to be the stored desc_ member variable based on the call at line 246. The method signature would be clearer if it didn't take the desc parameter since it's using the member variable desc_ stored from previous calls. Alternatively, if the intent is to compare against a provided descriptor, the parameter name should be different (e.g., previous_desc or stored_desc) to clarify what's being compared.

1.Add validateTextureParmeters help method to reduce code dulication.

xiaowei-guan added 2 commits January 7, 2026 19:02

Revert flutter offical PR and fix wrong parameter issue

46afa78

Re-implement resolve texture for impeller

56bc83d

The offical PR doesn't work

xiaowei-guan mentioned this pull request Jan 8, 2026

Revert impeller render texture code for gles flutter-tizen/embedder#140

Open

xiaowei-guan requested review from JSUYA, Copilot, gin7773 and seungsoo47 January 8, 2026 06:11

Copilot started reviewing on behalf of xiaowei-guan January 8, 2026 06:24 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

xiaowei-guan added 2 commits January 8, 2026 17:13

Minior refactor

4f27663

1.Add validation for texture dimensions(width/height > 0) 2.Add validation for texture name. 3.Add datailed error logging.

Revert FlutterOpenGLTexture

c24f208

xiaowei-guan requested a review from Copilot January 9, 2026 11:53

Copilot started reviewing on behalf of xiaowei-guan January 9, 2026 11:54 View session

Copilot AI reviewed Jan 9, 2026

View reviewed changes

xiaowei-guan force-pushed the flutter-3.38.3 branch from 7e690e0 to e3ef7ce Compare January 9, 2026 12:10

xiaowei-guan added 4 commits January 9, 2026 23:29

Invoked desturction callback when the texture size not right

76c8de2

Minor refactor

c2cb797

1.Add validateTextureParmeters help method to reduce code dulication.

Remove desc parameter

35e36be

Add null check for texture_image_

e3ef7ce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Tizen] Re-implemented render external texture gl for impeller to avoid change FlutterOpenGLTexture and use deprecated api #22

[Tizen] Re-implemented render external texture gl for impeller to avoid change FlutterOpenGLTexture and use deprecated api #22

Uh oh!

xiaowei-guan commented Jan 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

[Tizen] Re-implemented render external texture gl for impeller to avoid change FlutterOpenGLTexture and use deprecated api #22

Are you sure you want to change the base?

[Tizen] Re-implemented render external texture gl for impeller to avoid change FlutterOpenGLTexture and use deprecated api #22

Uh oh!

Conversation

xiaowei-guan commented Jan 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant