[Impeller] Optionally support SamplerAddressMode::kDecal on the OpenGLES backend #46650

ColdPaleLight · 2023-10-07T12:32:20Z

fix flutter/flutter#129358

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.

…LES backend

jonahwilliams

Nice!

There are a few places where we ifdef in the shader, such as

engine/impeller/entity/shaders/blending/porter_duff_blend.frag

Line 32 in a18c527

#ifdef IMPELLER_TARGET_OPENGLES

I think we should be able to update the ifdef to check for the extension? or something like that.

jonahwilliams · 2023-10-07T15:59:02Z

impeller/renderer/backend/gles/sampler_gles.cc

 }

-static GLint ToAddressMode(SamplerAddressMode mode) {
+static GLint ToAddressMode(SamplerAddressMode mode,


nit: I might just pass in a bool for whether or not decal is supported.

Just a drive by comment; Is it just me or does anyone else find "decal" mode to be a non-standard and/or overly confusing term? How about just ClampToBorderColor? Thats what the various APIs call it (GL_CLAMP_TO_BORDER_EXT, MTLSamplerAddressModeClampToBorderColor, VK_SAMPLER_ADDRESS_MODE_CLAMP_TO_BORDER).

Perhaps later, we can rename this enum value for consistency.

There is a cost for non-consistency with the underlying graphics API, but there is also a value in matching the framework level enumeration which uses decal as a terminology. I don't feel as strongly about decal as I do about "FilterQuality", which is entirely opaque - at least decal has some semantic meaning

nit: I might just pass in a bool for whether or not decal is supported.

Done

chinmaygarde · 2023-10-07T18:40:57Z

impeller/renderer/backend/gles/test/capabilities_unittests.cc


+TEST(CapabilitiesGLES, SupportsDecalSamplerAddressMode) {
+  auto const extensions = std::vector<unsigned char*>{
+      (unsigned char*)"GL_KHR_debug",                 //


Nit: reinterpret_cast

chinmaygarde · 2023-10-07T18:47:25Z

impeller/renderer/backend/gles/sampler_gles.cc

 }

-static GLint ToAddressMode(SamplerAddressMode mode) {
+static GLint ToAddressMode(SamplerAddressMode mode,


Just a drive by comment; Is it just me or does anyone else find "decal" mode to be a non-standard and/or overly confusing term? How about just ClampToBorderColor? Thats what the various APIs call it (GL_CLAMP_TO_BORDER_EXT, MTLSamplerAddressModeClampToBorderColor, VK_SAMPLER_ADDRESS_MODE_CLAMP_TO_BORDER).

Perhaps later, we can rename this enum value for consistency.

chinmaygarde · 2023-10-07T18:50:15Z

impeller/renderer/backend/gles/sampler_gles.cc

-                   ToAddressMode(desc.width_address_mode));
-  gl.TexParameteri(target.value(), GL_TEXTURE_WRAP_T,
-                   ToAddressMode(desc.height_address_mode));
+  gl.TexParameteri(


How do we set the border color? TEXTURE_BORDER_COLOR_EXT

Maybe this defaults to transparent black so we don't need to worry about it?

according to https://registry.khronos.org/OpenGL-Refpages/es3/html/glTexParameter.xhtml this value defaults to transparent black so we don't need to set anything.

I guess. I took a look at Vulkan and setting the border color is an extension. But setting it to VK_SAMPLER_ADDRESS_MODE_CLAMP_TO_BORDER is not. I guess this is fine. I don't think we set the color on Metal either. Just really odd.

Flutter does not expose an API for setting the border color.

Nevermind. Its an extension that is core in 1.1. And we do set it to transparent black. Let's be explicit and do the same here as well please?

Its is also dealers choice on Metal at the moment. Let's be explicit and set borderColor here too. In fact, a default border color specification in formats.h would be good for consistency.

Flutter does not expose an API for setting the border color.

Yeah, just didn't want to read the spec to see if the default was transparent black. Just being explicit seemed better. There isn't any bug though.

Issue filed: flutter/flutter#136153

ColdPaleLight · 2023-10-08T12:16:52Z

Nice!

There are a few places where we ifdef in the shader, such as

engine/impeller/entity/shaders/blending/porter_duff_blend.frag

Line 32 in a18c527

#ifdef IMPELLER_TARGET_OPENGLES

I think we should be able to update the ifdef to check for the extension? or something like that.

I removed the macro and added a variable in the uniform block to determine if the gpu api supports clip to border. (I originally planned to use different pipelines that support decal and those that do not, but I found that this would double the number of advanced blend pipelines.)

jonahwilliams · 2023-10-08T18:17:00Z

Rather than use a uniform, can we use a combination of defines - if opengles and at least one of the decal extensions not defined? Each extension should have a corresponding define already

ColdPaleLight · 2023-10-09T02:15:01Z

Rather than use a uniform, can we use a combination of defines - if opengles and at least one of the decal extensions not defined? Each extension should have a corresponding define already

I guess this won't work because shaderc will do a preprocessing before compiling glsl, so the macros will be handled at that stage. Therefore, glsl generated by spirv cross no longer contains statements like "#ifdef GL_EXT_texture_border_clamp"

jonahwilliams · 2023-10-09T02:31:36Z

that is extremely unfortunate

jonahwilliams · 2023-10-09T02:42:45Z

Well, we can clean it up later then. There is probably another way to do it by way of tinkering with SPIRV cross.

jonahwilliams · 2023-10-09T02:43:33Z

impeller/entity/shaders/blending/advanced_blend.glsl

-#else
-  return texture(texture_sampler, texture_coords);
-#endif
+  if (blend_info.supports_decal_sampler_address_mode > 0.0) {


These can still technically be ifdefs since vulkan/metal always support this.

Got it, added them back.

jonahwilliams

LGTM

… the OpenGLES backend (flutter/engine#46650)

…136215) flutter/engine@81e5bf2...0966f62 2023-10-09 [email protected] [Impeller] Optionally support SamplerAddressMode::kDecal on the OpenGLES backend (flutter/engine#46650) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#136215) flutter/engine@81e5bf2...0966f62 2023-10-09 [email protected] [Impeller] Optionally support SamplerAddressMode::kDecal on the OpenGLES backend (flutter/engine#46650) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…LES backend (#46650) fix flutter/flutter#129358

[Impeller] Optionally support SamplerAddressMode::kDecal on the OpenG…

dd5b80a

…LES backend

github-actions bot added the e: impeller label Oct 7, 2023

ColdPaleLight requested a review from bdero October 7, 2023 12:40

ColdPaleLight self-assigned this Oct 7, 2023

ColdPaleLight requested a review from jonahwilliams October 7, 2023 13:20

jonahwilliams reviewed Oct 7, 2023

View reviewed changes

chinmaygarde reviewed Oct 7, 2023

View reviewed changes

ColdPaleLight added 4 commits October 8, 2023 15:10

Clean code

e5fd823

Fix issue that essl can't use uint

5367092

Fix compile error

1852799

Update malioc.json

ea78ff1

ColdPaleLight mentioned this pull request Oct 8, 2023

[Impeller] Explicitly set texture border color flutter/flutter#136153

Closed

ColdPaleLight requested review from chinmaygarde and jonahwilliams October 8, 2023 12:17

jonahwilliams reviewed Oct 9, 2023

View reviewed changes

ColdPaleLight added 2 commits October 9, 2023 11:11

Add 'ifdef' back

990d7d2

Update malioc.json

1de6e16

jonahwilliams approved these changes Oct 9, 2023

View reviewed changes

jonahwilliams mentioned this pull request Oct 9, 2023

[Impeller] Delay evaluation of ifdefs for GLES specific feature detection. flutter/flutter#136210

Closed

ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 9, 2023

auto-submit bot merged commit 0966f62 into flutter:main Oct 9, 2023

engine-flutter-autoroll mentioned this pull request Oct 9, 2023

Roll Flutter Engine from 81e5bf29dee4 to 0966f62d7101 (1 revision) flutter/flutter#136215

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 9, 2023

0966f62d7 [Impeller] Optionally support SamplerAddressMode::kDecal on…

c7fa4e7

… the OpenGLES backend (flutter/engine#46650)

harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023

[Impeller] Optionally support SamplerAddressMode::kDecal on the OpenG…

a99fcbd

…LES backend (#46650) fix flutter/flutter#129358

[Impeller] Optionally support SamplerAddressMode::kDecal on the OpenGLES backend #46650

[Impeller] Optionally support SamplerAddressMode::kDecal on the OpenGLES backend #46650

Uh oh!

Conversation

ColdPaleLight commented Oct 7, 2023

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight commented Oct 8, 2023

Uh oh!

jonahwilliams commented Oct 8, 2023

Uh oh!

ColdPaleLight commented Oct 9, 2023

Uh oh!

jonahwilliams commented Oct 9, 2023

Uh oh!

jonahwilliams commented Oct 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants