Support BGRA textures in BlitCopyTextureToBufferCommandGLES::Encode and fix improper mapping of BGRA to RGBA in blit_command_gles and texture_gles

[b-luk]

Updates BlitCopyTextureToBufferCommandGLES::Encode to check for "GL_EXT_texture_format_BGRA8888", and call gl.ReadPixels with GL_BGRA_EXT if appropriate.

Also fixes an improper mapping of BGRA to RGBA in blit_command_gles and texture_gles. It now properly maps BGRA if BGRA is supported. If BGRA is not supported, it falls back to the existing behavior of using RGBA. I think this may be incorrect, but it's existing behavior so I'm not changing it.

This fixes codec_test.dart for opengles, which currently fails on linux.

Fixes #182105

codec_test.dart failure

codec_test.dart has many tests which calls getNextFrame() on a codec, and then calls toByteData on the resulting frame:

    final Uint8List data = File(
      path.join('flutter', 'lib', 'ui', 'fixtures', 'four_frame_with_reuse.gif'),
    ).readAsBytesSync();
    final ui.Codec codec = await ui.instantiateImageCodec(data);

    // Capture the final frame of animation. If we have not composited
    // correctly, it will be clipped strangely.
    late ui.FrameInfo frameInfo;
    for (var i = 0; i < 4; i++) {
      frameInfo = await codec.getNextFrame();
    }
    codec.dispose();

    final ui.Image image = frameInfo.image;
    final ByteData imageData = (await image.toByteData(format: ui.ImageByteFormat.png))!;

This was causing a crash with the following error:

[ERROR:flutter/impeller/renderer/backend/gles/blit_command_gles.cc(343)] Break on 'ImpellerValidationBreak' to inspect point of failure: Only textures with pixel format RGBA are supported yet.
[FATAL:flutter/impeller/renderer/backend/gles/blit_pass_gles.cc(88)] Check failed: result. Must be able to encode GL commands without error.

Cause

getNextFrame() converts an image frame into a bitmap. The bitmap generation is configured with the SkImageInfo here:

flutter/engine/src/flutter/lib/ui/painting/multi_frame_codec.cc

Line 65 in ba9a650

SkImageInfo info = generator_->GetInfo().makeColorType(kN32_SkColorType);

The bitmap uses kN32_SkColorType. This is defined here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/core/SkColorType.h;l=145-149?q=kN32_SkColorType&ss=chromium. It is an alias that can be kRGBA_8888_SkColorType or kBGRA_8888_SkColorType, depending on the platform.

So the generated DlImage created by getNextFrame() is backed by a texture which can have a pixel format of either RGBA or BGRA.

When toByteData() is called on this image, opengles impeller uses BlitCopyTextureToBufferCommandGLES::Encode to convert the DlImage's texture to byte data. If the texture has a BGRA pixel format, this fails:

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

Lines 340 to 345 in ba9a650

	bool BlitCopyTextureToBufferCommandGLES::Encode(
	const ReactorGLES& reactor) const {
	if (source->GetTextureDescriptor().format != PixelFormat::kR8G8B8A8UNormInt) {
	VALIDATION_LOG << "Only textures with pixel format RGBA are supported yet.";
	return false;
	}

On linux, including our CI linux tests, the pixel format is BGRA and codec_test.dart fails.

Fix

This PR changes BlitCopyTextureToBufferCommandGLES::Encode to add a check for "GL_EXT_texture_format_BGRA8888" support. If it is supported, BGRA pixel format textures are accepted. These textures will be read with gl.ReadPixels using a format of GL_BGRA_EXT rather than GL_RGBA.

If "GL_EXT_texture_format_BGRA8888" is not supported, this may still result in a crash. This could potentially be fixed by converting BGRA images to RGBA. But that is outside the scope of this PR.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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 adds support for BGRA textures in BlitCopyTextureToBufferCommandGLES::Encode by checking for the GL_EXT_texture_format_BGRA8888 extension. This resolves a crash in codec_test.dart on Linux. The changes include updating the encoding logic to handle GL_BGRA_EXT format, adding comprehensive unit tests for RGBA, BGRA, and unsupported pixel formats, and refactoring the test setup for better clarity. Additionally, several previously skipped Dart tests related to image handling are now re-enabled, presumably because this fix resolves their underlying issues. My feedback includes a suggestion to refactor the pixel format checking logic for improved readability.

[gemini-code-assist]

The nested if-else structure for checking the pixel format can be complex to follow. For improved readability and maintainability, consider refactoring this logic into a switch statement on source_format. This would make the different cases for pixel formats and extension availability clearer.

  const auto source_format = source->GetTextureDescriptor().format;
  GLenum format;
  switch (source_format) {
    case PixelFormat::kR8G8B8A8UNormInt:
      format = GL_RGBA;
      break;
    case PixelFormat::kB8G8R8A8UNormInt:
      if (gl.GetDescription()->HasExtension("GL_EXT_texture_format_BGRA8888")) {
        format = GL_BGRA_EXT;
        break;
      }
      // Fallthrough intended if extension is not present.
    default:
      if (gl.GetDescription()->HasExtension("GL_EXT_texture_format_BGRA8888")) {
        VALIDATION_LOG << "Only textures with pixel format RGBA or BGRA are "
                          "supported.";
      } else {
        VALIDATION_LOG << "Only textures with pixel format RGBA are supported.";
      }
      return false;
  }

[gaaclarke]

Looks good for the most part except for the change to CreateUntrackedHandle, I have questions there.

[gaaclarke]

I'm not keen to accept this extra overhead. Do you not need this after my PR that eliminates multithreaded tests?

[b-luk]

We can keep the discussion around this to the other PR. This PR contains this change too, but only because this PR is based on top of that one.

[b-luk]

This doesn't pass on linux. codec_test has tests which check specific byte values of the output, which end up different because it's using BGRA. I'll convert this to a draft until I fix this.

[flutter-dashboard]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[b-luk]

I merged with HEAD so we no longer need the FML_DCHECK change from #182329.

I tracked down the linux failures to an improper mapping from BGRA to RGBA in blit_command_gles and texture_gles. I changed it to properly use BGRA if BGRA is supported. If BGRA is not supported, it falls back to the existing behavior of using RGBA. I think this may be incorrect, but it's existing behavior so I'm hesitant to change it. Is there some other code somewhere which might expect BGRA to be mapped to RGBA in these places?

[gemini-code-assist]

Code Review

This pull request adds support for BGRA textures in the GLES backend, which resolves a crash in codec_test.dart. However, a high-severity security issue was found in BlitCopyTextureToBufferCommandGLES::Encode, where missing bounds validation on a destination buffer could lead to a buffer overflow during gl.ReadPixels operations. This should be addressed by adding explicit checks against the buffer length before performing memory writes. Additionally, there is a suggestion to improve the clarity of the format validation logic.

[gemini-code-assist]

The Encode method of BlitCopyTextureToBufferCommandGLES performs a gl.ReadPixels operation on a destination buffer using a raw pointer calculation data + destination_offset. However, it fails to validate that the write operation stays within the bounds of the destination buffer. Specifically, it does not check if destination_offset + (source_region.GetWidth() * source_region.GetHeight() * 4) is less than or equal to the length of the destination buffer provided by the UpdateBufferData lambda. A malicious or incorrectly configured blit command could trigger an out-of-bounds write, leading to memory corruption and potentially arbitrary code execution.

It is recommended to add a validation check within the lambda to ensure the operation fits within the buffer's allocated size.

[gemini-code-assist]

The logic to determine the pixel format and check for supported formats is a bit complex and hard to follow due to the nested if-else structure. This could be simplified to improve readability and maintainability. A flatter structure with more specific error messages would make it clearer which formats are supported and why a failure occurred.

Suggested change

	PixelFormat source_format = source->GetTextureDescriptor().format;
	GLenum format;
	if (source_format == PixelFormat::kR8G8B8A8UNormInt) {
	format = GL_RGBA;
	} else if (gl.GetDescription()->HasExtension(
	"GL_EXT_texture_format_BGRA8888")) {
	if (source_format == PixelFormat::kB8G8R8A8UNormInt) {
	format = GL_BGRA_EXT;
	} else {
	VALIDATION_LOG << "Only textures with pixel format RGBA or BGRA are "
	"supported.";
	return false;
	}
	} else {
	VALIDATION_LOG << "Only textures with pixel format RGBA are supported.";
	return false;
	}
	PixelFormat source_format = source->GetTextureDescriptor().format;
	GLenum format;
	if (source_format == PixelFormat::kR8G8B8A8UNormInt) {
	format = GL_RGBA;
	} else if (source_format == PixelFormat::kB8G8R8A8UNormInt) {
	if (!gl.GetDescription()->HasExtension("GL_EXT_texture_format_BGRA8888")) {
	VALIDATION_LOG
	<< "A BGRA texture was provided, but the GL driver does not "
	"support the GL_EXT_texture_format_BGRA8888 extension.";
	return false;
	}
	format = GL_BGRA_EXT;
	} else {
	VALIDATION_LOG << "Only textures with pixel format RGBA or BGRA are "
	"supported for texture-to-buffer copies. Got format "
	<< PixelFormatToString(source_format) << ".";
	return false;
	}

[gaaclarke]

looks good to me, thanks b-luk!