Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xiaowei-guan wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
46afa78
Revert flutter offical PR and fix wrong parameter issue
xiaowei-guan Jan 7, 2026
56bc83d
Re-implement resolve texture for impeller
xiaowei-guan Jan 7, 2026
4f27663
Minior refactor
xiaowei-guan Jan 8, 2026
c24f208
Revert FlutterOpenGLTexture
xiaowei-guan Jan 8, 2026
76c8de2
Invoked desturction callback when the texture size not right
xiaowei-guan Jan 9, 2026
c2cb797
Minor refactor
xiaowei-guan Jan 9, 2026
35e36be
Remove desc parameter
xiaowei-guan Jan 9, 2026
e3ef7ce
Add null check for texture_image_
xiaowei-guan Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions engine/src/flutter/shell/platform/embedder/embedder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,6 @@ typedef struct {
} FlutterTransformation;

typedef void (*VoidCallback)(void* /* user data */);
typedef bool (*BoolCallback)(void* /* user data */);

typedef enum {
/// Specifies an OpenGL texture target type. Textures are specified using
Expand Down Expand Up @@ -513,13 +512,6 @@ typedef struct {
uint32_t name;
/// The texture format (example GL_RGBA8).
uint32_t format;
/// 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;

/// User data to be returned on the invocation of the destruction callback.
void* user_data;
/// Callback invoked (on an engine managed thread) that asks the embedder to
Expand Down Expand Up @@ -613,6 +605,7 @@ typedef struct {
uint32_t format;
} FlutterOpenGLSurface;

typedef bool (*BoolCallback)(void* /* user data */);
typedef FlutterTransformation (*TransformationCallback)(void* /* user data */);
typedef uint32_t (*UIntCallback)(void* /* user data */);
typedef bool (*SoftwareSurfacePresentCallback)(void* /* user data */,
Expand Down
145 changes: 80 additions & 65 deletions engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,107 +129,122 @@ sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureSkia(
return DlImage::Make(std::move(image));
}

sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpeller(
int64_t texture_id,
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;
Comment on lines +134 to +136
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
auto handle = texture_image_->GetGLHandle();
if (!handle.has_value()) {
return true;
}

if (handle.value() != texture->name) {
return true;
}
return false;
}
Comment on lines +132 to +153
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment thread
xiaowei-guan marked this conversation as resolved.

std::shared_ptr<impeller::TextureGLES>
EmbedderExternalTextureGL::CreateImpellerTexture(
impeller::AiksContext* aiks_context,
const SkISize& size) {
std::unique_ptr<FlutterOpenGLTexture> texture =
external_texture_callback_(texture_id, size.width(), size.height());
FlutterOpenGLTexture* texture) {
// Validate input parameters
if (texture->width <= 0 || texture->height <= 0) {
FML_LOG(ERROR) << "Invalid texture dimensions: " << texture->width << "x"
<< texture->height;
return nullptr;
Comment thread
xiaowei-guan marked this conversation as resolved.
Outdated
}

if (!texture) {
if (texture->name == 0) {
FML_LOG(ERROR) << "Invalid texture name (0)";
return nullptr;
}
Comment thread
xiaowei-guan marked this conversation as resolved.
Outdated

if (texture->bind_callback != nullptr) {
return ResolveTextureImpellerSurface(aiks_context, std::move(texture));
desc_.size = impeller::ISize(texture->width, texture->height);
desc_.storage_mode = impeller::StorageMode::kDevicePrivate;
desc_.format = impeller::PixelFormat::kR8G8B8A8UNormInt;
if (texture->target == GL_TEXTURE_EXTERNAL_OES) {
desc_.type = impeller::TextureType::kTextureExternalOES;
} else {
return ResolveTextureImpellerPixelbuffer(aiks_context, std::move(texture));
desc_.type = impeller::TextureType::kTexture2D;
}
}

sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpellerPixelbuffer(
impeller::AiksContext* aiks_context,
std::unique_ptr<FlutterOpenGLTexture> texture) {
impeller::TextureDescriptor desc;
desc.size = impeller::ISize(texture->width, texture->height);
desc.type = impeller::TextureType::kTexture2D;
desc.storage_mode = impeller::StorageMode::kDevicePrivate;
desc.format = impeller::PixelFormat::kR8G8B8A8UNormInt;
impeller::ContextGLES& context =
impeller::ContextGLES::Cast(*aiks_context->GetContext());
std::shared_ptr<impeller::TextureGLES> image =
std::make_shared<impeller::TextureGLES>(context.GetReactor(), desc);
impeller::HandleGLES handle = context.GetReactor()->CreateHandle(
impeller::HandleType::kTexture, texture->name);

image->MarkContentsInitialized();
if (!image->SetContents(texture->buffer, texture->buffer_size)) {
if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
}
return nullptr;
}
std::shared_ptr<impeller::TextureGLES> texture_image =
impeller::TextureGLES::WrapTexture(context.GetReactor(), desc_, handle);

if (!image) {
if (!texture_image) {
// In case Skia rejects the image, call the release proc so that
// embedders can perform collection of intermediates.
if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
}
FML_LOG(ERROR) << "Could not create external texture";
FML_LOG(ERROR) << "Could not create external texture with name: "
<< texture->name << ", size: " << texture->width << "x"
<< texture->height;
return nullptr;
}

if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
texture_image->SetCoordinateSystem(
impeller::TextureCoordinateSystem::kUploadFromHost);

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;
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

return impeller::DlImageImpeller::Make(image);
return texture_image;
}

sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpellerSurface(
sk_sp<DlImage> EmbedderExternalTextureGL::ResolveTextureImpeller(
int64_t texture_id,
impeller::AiksContext* aiks_context,
std::unique_ptr<FlutterOpenGLTexture> texture) {
impeller::TextureDescriptor desc;
desc.size = impeller::ISize(texture->width, texture->height);
desc.storage_mode = impeller::StorageMode::kDevicePrivate;
desc.format = impeller::PixelFormat::kR8G8B8A8UNormInt;
desc.type = impeller::TextureType::kTextureExternalOES;
impeller::ContextGLES& context =
impeller::ContextGLES::Cast(*aiks_context->GetContext());
std::shared_ptr<impeller::TextureGLES> image =
std::make_shared<impeller::TextureGLES>(context.GetReactor(), desc);
image->MarkContentsInitialized();
image->SetCoordinateSystem(
impeller::TextureCoordinateSystem::kUploadFromHost);
if (!image->Bind()) {
if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
}
FML_LOG(ERROR) << "Could not bind texture";
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 (!image) {
// In case Skia rejects the image, call the release proc so that
// embedders can perform collection of intermediates.
if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
}
FML_LOG(ERROR) << "Could not create external texture";
// Validate texture parameters
if (size.width() <= 0 || size.height() <= 0) {
FML_LOG(ERROR) << "Invalid texture size: " << size.width() << "x"
<< size.height();
return nullptr;
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if (!texture->bind_callback(texture->user_data)) {
if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
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;
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

if (texture->destruction_callback) {
texture->destruction_callback(texture->user_data);
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_);
}
Comment on lines +229 to 262
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// |flutter::Texture|
Expand Down
13 changes: 6 additions & 7 deletions engine/src/flutter/shell/platform/embedder/embedder_external_texture_gl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "flutter/common/graphics/texture.h"
#include "flutter/fml/macros.h"
#include "flutter/shell/platform/embedder/embedder.h"
#include "impeller/renderer/backend/gles/texture_gles.h"
#include "third_party/skia/include/core/SkSize.h"

namespace flutter {
Expand All @@ -25,7 +26,8 @@ class EmbedderExternalTextureGL : public flutter::Texture {
private:
const ExternalTextureCallback& external_texture_callback_;
sk_sp<DlImage> last_image_;

std::shared_ptr<impeller::TextureGLES> texture_image_ = nullptr;
impeller::TextureDescriptor desc_;
sk_sp<DlImage> ResolveTexture(int64_t texture_id,
GrDirectContext* context,
impeller::AiksContext* aiks_context,
Expand All @@ -39,13 +41,10 @@ class EmbedderExternalTextureGL : public flutter::Texture {
impeller::AiksContext* aiks_context,
const SkISize& size);

sk_sp<DlImage> ResolveTextureImpellerPixelbuffer(
impeller::AiksContext* aiks_context,
std::unique_ptr<FlutterOpenGLTexture> texture);

sk_sp<DlImage> ResolveTextureImpellerSurface(
bool IsExternalTextureChanged(FlutterOpenGLTexture* texture);
std::shared_ptr<impeller::TextureGLES> CreateImpellerTexture(
impeller::AiksContext* aiks_context,
std::unique_ptr<FlutterOpenGLTexture> texture);
FlutterOpenGLTexture* texture);

// |flutter::Texture|
void Paint(PaintContext& context,
Expand Down