Add GLES support for the same pixel formats for copying texture -> buffer as when copying buffer -> texture by b-luk · Pull Request #183428 · flutter/flutter

b-luk · 2026-03-10T00:03:00Z

BlitCopyBufferToTextureCommandGLES (and initializing/setting texture contents in TextureGLES) supports a bunch of different pixel formats, as defined in the TexImage2DData constructors in blit_command_gles.cc and texture_gles.cc.

But BlitCopyTextureToBufferCommandGLES only has support specifically for kR8G8B8A8UNormInt and kB8G8R8A8UNormInt.

This makes BlitCopyTextureToBufferCommandGLES support the same pixel formats as BlitCopyBufferToTextureCommandGLES and TextureGLES.

Fixes #183462

Changes:

Share `ToPixelFormatGLES`

blit_command_gles.cc and texture_gles.cc have almost-duplicate logic to convert impeller::PixelFormat to a GLint internal_format, a GLenum external_format, and a GLenum type. Factor this out into a shared ToPixelFormatGLES function in formats_gles.h/cc.

Where there were differences between the blit_command_gles.cc and texture_gles.cc:

kR32G32B32A32Float:
- blit_command_gles.cc has internal_format = GL_RGBA, external_format = GL_RGBA
- texture_gles.cc has internal_format = GL_RGBA32F, external_format = GL_RGBA
- I used internal_format = GL_RGBA32F, external_format = GL_RGBA
kR16G16B16A16Float:
- blit_command_gles.cc has internal_format = GL_RGBA, external_format = GL_RGBA
- texture_gles.cc has internal_format = GL_RGBA16F, external_format = GL_RGBA
- I used internal_format = GL_RGBA16F, external_format = GL_RGBA
kR32Float:
- blit_command_gles.cc has internal_format = GL_RED, external_format = GL_RED
- texture_gles.cc has internal_format = GL_R32F, external_format = GL_RGBA
- I used internal_format = GL_R32F, external_format = GL_RED

All the other PixelFormat cases were the same between the two files and copied over directly.

Remove `TexImage2DData`

blit_command_gles.cc and texture_gles.cc each define a TexImage2DData struct which just contains the pixel format data and sometimes an optional data buffer. I removed both of these. Instead, we just use the ToPixelFormatGLES return value directly, and use the source data buffer directly.

Improve pixel format support in `BlitCopyTextureToBufferCommandGLES::Encode()`

In BlitCopyTextureToBufferCommandGLES::Encode(), use ToPixelFormatGLES() to determine the format and type used for gl.ReadPixels(). This expands the existing functionality that only supported two specific pixel formats.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request refactors and centralizes pixel format conversion logic for GLES, expanding support for texture-to-buffer copies by introducing ToPixelFormatGLES and removing the duplicated TexImage2DData struct. However, it introduces several heap buffer overflow and over-read vulnerabilities. This is due to a mismatch in how the kS8UInt (Stencil 8) format is handled, where Impeller treats it as 1-byte-per-pixel, but the GLES backend maps it to a 4-byte depth-stencil format (GL_UNSIGNED_INT_24_8). This discrepancy leads to out-of-bounds memory access in BlitCopyTextureToBufferCommandGLES, BlitCopyBufferToTextureCommandGLES, and TextureGLES::OnSetContents. These critical security issues must be addressed by ensuring consistent byte-size handling or performing explicit format conversions in the GLES backend. Additionally, consider improving the new utility function by making it aware of GPU capabilities for better correctness (specifically for SRGB formats) and to enhance debuggability.

gemini-code-assist · 2026-03-10T18:24:23Z

engine/src/flutter/impeller/renderer/backend/gles/blit_command_gles.cc

+            gl.ReadPixels(source_region.GetX(), source_region.GetY(),
+                          source_region.GetWidth(), source_region.GetHeight(),
+                          format, type, data + destination_offset);


There is a mismatch between the pixel format mapping in ToPixelFormatGLES and the byte size calculation in BytesPerPixelForPixelFormat for PixelFormat::kS8UInt.

In formats_gles.cc, kS8UInt is mapped to GL_DEPTH_STENCIL with GL_UNSIGNED_INT_24_8 (4 bytes per pixel). However, BytesPerPixelForPixelFormat in impeller/core/formats.h returns 1 byte per pixel for kS8UInt.

In BlitCopyTextureToBufferCommandGLES::Encode, gl.ReadPixels will write 4 bytes per pixel based on the GLES format, but the destination buffer size and the FlipImage call (line 314) rely on bytes_per_pixel which is 1. This leads to a heap buffer overflow when copying stencil data from a texture to a buffer.

To remediate this, ensure that the GLES format used for gl.ReadPixels matches the expected byte size of the destination buffer, or perform a manual conversion from the 4-byte depth-stencil format to the 1-byte stencil format.

gemini-code-assist · 2026-03-10T18:24:23Z

engine/src/flutter/impeller/renderer/backend/gles/blit_command_gles.cc

+                     gles_format->external_format,    // format
+                     gles_format->type,               // type
+                     tex_data);                       // data


Similar to the issue in BlitCopyTextureToBufferCommandGLES::Encode, there is a mismatch for PixelFormat::kS8UInt that leads to a heap buffer over-read here.

gl.TexSubImage2D will read 4 bytes per pixel from tex_data because gles_format->type is GL_UNSIGNED_INT_24_8. However, the source buffer source may have been allocated based on 1 byte per pixel (the core definition of kS8UInt). This can result in reading past the end of the source buffer.

Ensure the source buffer contains sufficient data for the 4-byte GLES format, or handle the conversion from 1-byte stencil data to the 4-byte depth-stencil format required by GLES.

gemini-code-assist · 2026-03-10T18:24:23Z

engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc

+      gl.TexImage2D(texture_target,          // target
+                    0u,                      // LOD level
+                    format.internal_format,  // internal format
+                    size.width,              // width
+                    size.height,             // height
+                    0u,                      // border
+                    format.external_format,  // format
+                    format.type,             // type
+                    tex_data);               // data


In TextureGLES::OnSetContents, gl.TexImage2D uses the format and type from ToPixelFormatGLES. For PixelFormat::kS8UInt, this results in a 4-byte per pixel read (GL_UNSIGNED_INT_24_8).

However, the validation check on line 234 only ensures that the input mapping buffer is at least GetByteSizeOfBaseMipLevel() bytes. For kS8UInt, this is typically 1 byte per pixel. This leads to a heap buffer over-read when gl.TexImage2D is called, as it will attempt to read 4 bytes per pixel from a buffer that may only be sized for 1 byte per pixel.

Validate that the input buffer size is sufficient for the mapped GLES format and type before calling gl.TexImage2D.

gemini-code-assist · 2026-03-10T18:24:23Z

engine/src/flutter/impeller/renderer/backend/gles/formats_gles.h

+std::optional<PixelFormatGLES> ToPixelFormatGLES(PixelFormat format,
+                                                 bool supports_bgra);


To improve this new utility function, consider changing its signature to accept const CapabilitiesGLES& instead of bool supports_bgra.

This would allow you to check for other capabilities, like SRGB support, which is currently not handled correctly for SRGB pixel formats (e.g., kR8G8B8A8UNormIntSRGB). The supports_bgra flag can then be derived from the capabilities object within the function. This would centralize capability checking and fix a color correctness issue.

Suggested change

std::optional<PixelFormatGLES> ToPixelFormatGLES(PixelFormat format,

bool supports_bgra);

std::optional<PixelFormatGLES> ToPixelFormatGLES(

PixelFormat format,

const CapabilitiesGLES& capabilities);

gemini-code-assist · 2026-03-10T18:24:23Z

engine/src/flutter/impeller/renderer/backend/gles/formats_gles.cc

+    case PixelFormat::kR8G8B8A8UNormInt:
+      format.internal_format = GL_RGBA;
+      format.external_format = GL_RGBA;
+      format.type = GL_UNSIGNED_BYTE;
+      break;
+    case PixelFormat::kR8G8B8A8UNormIntSRGB:
+      format.internal_format = GL_RGBA;
+      format.external_format = GL_RGBA;
+      format.type = GL_UNSIGNED_BYTE;
+      break;


These two case statements for kR8G8B8A8UNormInt and kR8G8B8A8UNormIntSRGB have identical bodies and can be combined to reduce duplication.

Additionally, this is a good opportunity to correctly handle kR8G8B8A8UNormIntSRGB. Its internal_format should be GL_SRGB8_ALPHA8 on platforms that support it to ensure correct gamma conversion. Currently, it's treated as a linear GL_RGBA format, which can lead to incorrect colors. This could be fixed by passing CapabilitiesGLES to this function as suggested in formats_gles.h.

Suggested change

case PixelFormat::kR8G8B8A8UNormInt:

format.internal_format = GL_RGBA;

format.external_format = GL_RGBA;

format.type = GL_UNSIGNED_BYTE;

break;

case PixelFormat::kR8G8B8A8UNormIntSRGB:

format.internal_format = GL_RGBA;

format.external_format = GL_RGBA;

format.type = GL_UNSIGNED_BYTE;

break;

case PixelFormat::kR8G8B8A8UNormInt:

case PixelFormat::kR8G8B8A8UNormIntSRGB:

format.internal_format = GL_RGBA;

format.external_format = GL_RGBA;

format.type = GL_UNSIGNED_BYTE;

break;

The duplication removal is a good suggestion. It also begs the question if the differences between those formats is being respected and if it's important. I think the difference between those is linear space versus gamma space. If this is just used for blitting, i don't think it matters.

Removed the duplication. I'm unsure about the implications of those two formats either. But for the purposes of this PR, I'm just preserving existing behavior here.

engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc

gaaclarke

LGTM, thanks @b-luk !

gaaclarke · 2026-03-10T20:38:19Z

engine/src/flutter/impeller/renderer/backend/gles/formats_gles.cc

+    case PixelFormat::kR8G8B8A8UNormInt:
+      format.internal_format = GL_RGBA;
+      format.external_format = GL_RGBA;
+      format.type = GL_UNSIGNED_BYTE;
+      break;
+    case PixelFormat::kR8G8B8A8UNormIntSRGB:
+      format.internal_format = GL_RGBA;
+      format.external_format = GL_RGBA;
+      format.type = GL_UNSIGNED_BYTE;
+      break;


The duplication removal is a good suggestion. It also begs the question if the differences between those formats is being respected and if it's important. I think the difference between those is linear space versus gamma space. If this is just used for blitting, i don't think it matters.

b-luk · 2026-03-10T20:53:02Z

Thanks for the review, @gaaclarke. I was wrangling with Gemini's comments about PixelFormat::kS8UInt and was planning on dealing with that before actually sending this out for review.

I think Gemini is correct about PixelFormat::kS8UInt having inconsistencies and not returning the correct values with BytesPerPixelForPixelFormat when running on GLES. And this can potentially cause some incorrect behavior in some cases, although I don't know enough about when kS8UInt would actually be used to know if it's a problem real people might see.

Fixing this doesn't seem trivial, and this PR does preserve existing behavior (except for BlitCopyBufferToTextureCommandGLES, where the existing behavior with kS8UInt is to fail because it wasn't supported). So maybe this PR is fine as-is. What do you think?

gaaclarke · 2026-03-10T21:07:07Z

Fixing this doesn't seem trivial, and this PR does preserve existing behavior (except for BlitCopyBufferToTextureCommandGLES, where the existing behavior with kS8UInt is to fail because it wasn't supported). So maybe this PR is fine as-is. What do you think?

Yea, it's fine to maintain the existing behavior. A hypothetical bug is not something we should chase after. We can defer to reported behavior for stuff like this.

That's the interesting thing between supporting something like impeller versus flutter_gpu. In impeller we just have to support what flutter uses, in flutter_gpu we have to support anything anyone could possibly do =)

lgtm

…ture -> buffer as when copying buffer -> texture (flutter/flutter#183428)

flutter/flutter@195ae7b...3f400d7 2026-03-11 [email protected] Roll Clang to 80743bd43fd5b38fedc503308e7a652e23d3ec93 (flutter/flutter#182919) 2026-03-11 [email protected] Roll Fuchsia Linux SDK from 8C_qfgWgoNhkV0_Mn... to QD887D4OanteB7UKM... (flutter/flutter#183492) 2026-03-11 [email protected] Roll Dart SDK from fecf806be5d0 to 8531f7c2bdae (2 revisions) (flutter/flutter#183489) 2026-03-11 [email protected] [impeller] Use the GLES3 shaders in the embedder if supported (flutter/flutter#180072) 2026-03-11 [email protected] Roll Dart SDK from ae2be9700800 to fecf806be5d0 (1 revision) (flutter/flutter#183482) 2026-03-11 [email protected] Add GLES support for the same pixel formats for copying texture -> buffer as when copying buffer -> texture (flutter/flutter#183428) 2026-03-11 [email protected] Roll Skia from 257d04225d0c to 0cab3e4ee34b (8 revisions) (flutter/flutter#183476) 2026-03-10 [email protected] Fix GitHub workflows to use the `flutteractionsbot` mirror for PR branches. (flutter/flutter#183470) 2026-03-10 [email protected] [material/menu_anchor.dart] Ensure positioned menus always begin animating at the target position (flutter/flutter#182932) 2026-03-10 [email protected] Roll pub packages (flutter/flutter#183467) 2026-03-10 [email protected] Roll Dart SDK from ebef6c849489 to ae2be9700800 (4 revisions) (flutter/flutter#183460) 2026-03-10 [email protected] Roll Skia from 4b35832cc7ea to 257d04225d0c (5 revisions) (flutter/flutter#183457) 2026-03-10 [email protected] dev: Use a super-parameter in several missed cases (flutter/flutter#182251) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Shared GLES pixel format conversion

918a9ab

github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Mar 10, 2026

b-luk added 2 commits March 9, 2026 17:39

Enable high_bitrate_texture_test for opengles

8a8f72c

Update unit test

8f2d4df

b-luk changed the title ~~Share GLES pixel format conversion used in blit_command_gles and texture_gles~~ Add GLES support for the same pixel formats for copying texture -> buffer as when copying buffer -> texture Mar 10, 2026

b-luk marked this pull request as ready for review March 10, 2026 18:17

gemini-code-assist bot reviewed Mar 10, 2026

View reviewed changes

gaaclarke approved these changes Mar 10, 2026

View reviewed changes

minor changes

6cf84c4

b-luk requested a review from gaaclarke March 10, 2026 20:53

gaaclarke approved these changes Mar 10, 2026

View reviewed changes

b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 10, 2026

auto-submit bot added this pull request to the merge queue Mar 11, 2026

Merged via the queue into flutter:master with commit b07acf9 Mar 11, 2026
185 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 11, 2026

engine-flutter-autoroll mentioned this pull request Mar 11, 2026

Roll Flutter from 195ae7b3a122 to 3f400d7c69e1 (13 revisions) flutter/packages#11228

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2026

b07acf9bb Add GLES support for the same pixel formats for copying tex…

71541c4

…ture -> buffer as when copying buffer -> texture (flutter/flutter#183428)

b-luk deleted the pixelformat branch March 11, 2026 17:17

		std::optional<PixelFormatGLES> ToPixelFormatGLES(PixelFormat format,
		bool supports_bgra);

Conversation

b-luk commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Share ToPixelFormatGLES

Remove TexImage2DData

Improve pixel format support in BlitCopyTextureToBufferCommandGLES::Encode()

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

gaaclarke Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

b-luk Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

b-luk commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Mar 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

b-luk commented Mar 10, 2026 •

edited

Loading

Share `ToPixelFormatGLES`

Remove `TexImage2DData`

Improve pixel format support in `BlitCopyTextureToBufferCommandGLES::Encode()`

b-luk commented Mar 10, 2026 •

edited

Loading