Add format field to EGL surface backing store

[vially]

Trying to use the GL_BGRA8_EXT format for the EGL surface backing store on desktop OpenGL platforms would fail at:

engine/shell/platform/embedder/embedder.cc

Line 869 in bde314b

FML_LOG(ERROR) << "Could not wrap embedder supplied frame-buffer.";

This seems to be a known issue and both the Linux and Windows embedders have some logic to pick a different format depending on the OpenGL context information:

engine/shell/platform/windows/compositor_opengl.cc

Lines 23 to 34 in 088dcfa

	// Based off Skia's logic:
	// https://github.com/google/skia/blob/4738ed711e03212aceec3cd502a4adb545f38e63/src/gpu/ganesh/gl/GrGLCaps.cpp#L1963-L2116
	int GetSupportedTextureFormat(const impeller::DescriptionGLES* description) {
	if (description->HasExtension("GL_EXT_texture_format_BGRA8888")) {
	return GL_BGRA8_EXT;
	} else if (description->HasExtension("GL_APPLE_texture_format_BGRA8888") &&
	description->GetGlVersion().IsAtLeast(impeller::Version(3, 0))) {
	return GL_BGRA8_EXT;
	} else {
	return GL_RGBA8;
	}
	}

engine/shell/platform/linux/fl_framebuffer.cc

Lines 81 to 104 in 088dcfa

	GLenum fl_framebuffer_get_format(FlFramebuffer* self) {
	// Flutter defines SK_R32_SHIFT=16, so SK_PMCOLOR_BYTE_ORDER should be BGRA.
	// In Linux kN32_SkColorType is assumed to be kBGRA_8888_SkColorType.
	// So we must choose a valid gl format to be compatible with surface format
	// BGRA8.
	// Following logic is copied from Skia GrGLCaps.cpp:
	// https://github.com/google/skia/blob/4738ed711e03212aceec3cd502a4adb545f38e63/src/gpu/ganesh/gl/GrGLCaps.cpp#L1963-L2116
	if (epoxy_is_desktop_gl()) {
	// For OpenGL.
	if (epoxy_gl_version() >= 12 || epoxy_has_gl_extension("GL_EXT_bgra")) {
	return GL_RGBA8;
	}
	} else {
	// For OpenGL ES.
	if (epoxy_has_gl_extension("GL_EXT_texture_format_BGRA8888") ||
	(epoxy_has_gl_extension("GL_APPLE_texture_format_BGRA8888") &&
	epoxy_gl_version() >= 30)) {
	return GL_BGRA8_EXT;
	}
	}
	g_critical("Failed to determine valid GL format for Flutter rendering");
	return GL_RGBA8;
	}

This pull-request gets rid of the hard-coded GL_BGRA8_EXT format and makes it configurable by adding a format field to the FlutterOpenGLSurface struct.

Disclaimer: This has only been tested on desktop Linux (Wayland) using the GR_GL_RGBA8 format which seemed to work as expected.

This change is related to the recently introduced EGL surface backing store: flutter/flutter#58363

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 and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

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

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[loic-sharma]

@vially Thanks for opening this patch! I think it would be better if we followed the other backing stores' pattern and instead introduced a format parameter to the config to allow the embedder to choose its format. See this discussion: https://github.com/flutter/engine/pull/43683/files#r1263989268

FYI, we just recently landed #54329 which moves away from kN32_SkColorType and instead uses FlutterFormatToSkColorType to convert the config's format to an SkColorType. We will likely want to use that here as well.

cc @chinmaygarde

[vially]

@loic-sharma Thanks for the review. I have updated the implementation with your suggestion by adding a format field to the FlutterOpenGLSurface embedder struct (similar to the other backing stores).

Does it make sense to also add some logic to set the default format to GL_BGRA8_EXT (when set to 0/unset), just to maintain compatibility with the previous logic?

Or maybe backwards compatibility is not a real concern in this case, since the EGL surface backing store (#43683) was only merged a few days ago?

/cc @ardera

[ardera]

@loic-sharma Thanks for the review. I have updated the implementation with your suggestion by adding a format field to the FlutterOpenGLSurface embedder struct (similar to the other backing stores).

Does it make sense to also add some logic to set the default format to GL_BGRA8_EXT (when set to 0/unset), just to maintain compatibility with the previous logic?

Or maybe backwards compatibility is not a real concern in this case, since the EGL surface backing store (#43683) was only merged a few days ago?

/cc @ardera

Oof thanks for the fix, I wasn't aware of this.

IMO I think it's also a good idea to document this limitation in the embedder API, e.g.:

typedef struct {
  // . . .

  /// The surface format (example GL_RGBA8).
  ///
  /// Due to limitations in Skia, this needs to be GL_BGRA8_EXT when either:
  ///   - GL_EXT_texture_format_BGRA8888 is present or
  ///   - GL_APPLE_texture_format_BGRA8888 is present and GL version is larger than 3.0
  /// otherwise it should be GL_RGBA8.
  /// (or whatever the correct logic is)
  uint32_t format;
} FlutterOpenGLSurface;

Maybe also with a TODO link for an issue. It's a bit ugly to write that there, but it's even more annoying to run into the assert, and having to dig through engine / other embedder sources to find the fix.

I'd say backwards compatibility is not a concern yet, it hasn't even reached stable yet. We also ignored backwards compatibility for #38847 (and that was when the software pixel formats were already in stable) (though that was API not ABI)

EDIT: Ah, with FlutterFormatToSkColorType there's no limitation anymore, right? In that case it'd still be good to document that only GL_RGBA8 and GL_BGRA8_EXT are supported.

[loic-sharma]

I'd say backwards compatibility is not a concern yet, it hasn't even reached stable yet.

Agreed. The current approach looks good to me, but @chinmaygarde has the final word here :)

IMO I think it's also a good idea to document this limitation in the embedder API, e.g.:

Agreed documenting allowed formats this is a good idea, but I wouldn't prescribe the logic to choose the format as that's likely highly dependent on the embedder and its platform. Perhaps a comment like:

typedef struct {
  ...

  /// The surface format.
  ///
  /// Allowed values:
  ///   - GL_RGBA8
  ///   - GL_BGRA8_EXT
  uint32_t format;
} FlutterOpenGLSurface;

[vially]

Thank you both. I have updated the field comment with your suggestions to list the allowed values. I've also updated the test producer class to set to format in order to make the tests pass.

[loic-sharma]

Are there any tests we could add for this?

[vially]

Are there any tests we could add for this?

I have added a new test for validating that passing an invalid format will result in an invalid Skia surface. The "happy" path of valid format resulting in valid Skia surface is already covered by the original tests from #43683, so I think the common code-paths are now covered.

It would be nice if the tests covered some of the edge-cases too (e.g.: trying to use GL_BGRA8_EXT when it's not available) but I haven't found an easy way to do that.

[jtmcdole]

@loic-sharma - friendly ping

[iamsergio]

I've tested this patch on Linux/X11 and now GL finally works! Before only GLES worked.

[chinmaygarde]

@loic-sharma Can you tell us how to proceed here?

[loic-sharma]

Apologies for the delay, this fell off my radar.

[loic-sharma]

These changes look good to me (modulo this minor comment).

I'm not an expert in this area, please get an approval from @chinmaygarde before merging this.

[jonahwilliams]

LGTM

[loic-sharma]

Re-LGTM with updates - thanks for the wonderful contribution!

[auto-submit]

auto label is removed for flutter/engine/54499, due to - The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.