Provide a matrix inverse shim for GLES 2.0. #50545

johnoneil · 2024-02-11T20:44:00Z

We saw a shader compilation error after updating flutter/impeller due to the lack of a matrix inverse implementation on GLSL 1.0 used on GLSL 2.0. This change provides one (for the current used case which is only for mat3 support.

I think this change needs some discussion as gating the change on GLSL < 1.4 would be more effective. Also my knowledge of correctly managing these .glsl based shaders is still superficial.

Pre-launch Checklist

[x ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[ x] I read the Tree Hygiene wiki page, which explains my responsibilities.
[x ] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
[x ] 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 · 2024-02-11T20:44:03Z

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

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.

jonahwilliams · 2024-02-11T21:38:52Z

impeller/compiler/shader_lib/impeller/transform.glsl

+
+// Shim matrix `inverse` for versions that lack it.
+// TODO: This could be gated on GLSL < 1.4.
+mat3 IPMat3Inverse( mat3 m) {


The compiler seems unhappy with this code

jonahwilliams · 2024-02-11T21:40:45Z

impeller/compiler/shader_lib/impeller/transform.glsl

@@ -26,4 +26,32 @@ vec4 IPPositionForGlyphPosition(mat4 mvp,
              unit_position.y * destination_size.y, 0.0, 1.0);
 }

-#endif
+#ifdef IMPELLER_TARGET_OPENGLES


Its not possible to leave a define unevaluated with our shader compiler pipeline. that is, any define must evaluate to a value during the engine build - so its only possible to use these to turn this on/off for an entire backend.

If there is a runtime capabilities check you'd need to use, you'll have to use a specialization constant - which must be defined in the shader entrypoint and provided during initialization in content_context.cc

For an example of that, grep for supports_decal.

In this particular case, that might not be worthwhile.

Thank you Jonah. I'm in a lot of meetings this week but will try to update this when possible.

I don't think adding a second pipeline is worth the reduced flops of the O(1) invocation of this in conical gradient for on GLES only.

inverse isn't supported until GLSL ES 3. This restores GLES 2 support (GLSL ES 1).

In other words: I think this approach seems OK as-is for now?

Yes I agree, one ifdef for GLES

impeller/compiler/shader_lib/impeller/transform.glsl

bdero · 2024-02-13T17:52:26Z

impeller/compiler/shader_lib/impeller/transform.glsl

@@ -26,4 +26,32 @@ vec4 IPPositionForGlyphPosition(mat4 mvp,
              unit_position.y * destination_size.y, 0.0, 1.0);
 }

-#endif
+#ifdef IMPELLER_TARGET_OPENGLES


I don't think adding a second pipeline is worth the reduced flops of the O(1) invocation of this in conical gradient for on GLES only.

inverse isn't supported until GLSL ES 3. This restores GLES 2 support (GLSL ES 1).

bdero · 2024-02-14T00:45:17Z

@johnoneil Thanks for this patch!

@jonahwilliams @matanlurey WDYT about a test exemption for this given it's a shader-only refactor exercised by goldens using the conical gradient?
I don't think there's a way to force the driver into a strict GLES 2 mode (but if there was, it would save us from these kinds of headaches).

jonahwilliams · 2024-02-14T00:47:29Z

Yeah I think this probably qualifies, as its unlikely we'll ever get GLES 2.0 only capable hardware.

johnmccutchan · 2024-02-14T01:07:40Z

test-exempt: shader only change tested via goldens.

bdero · 2024-02-14T19:57:03Z

@johnoneil Looks like you just need to apply the formatter diff in the linux_unopt output and this will be good to go!

git apply <<DONE
diff --git a/impeller/compiler/shader_lib/impeller/gradient.glsl b/impeller/compiler/shader_lib/impeller/gradient.glsl
index a33f4eb3c9..0000000000 100644
--- a/impeller/compiler/shader_lib/impeller/gradient.glsl
+++ b/impeller/compiler/shader_lib/impeller/gradient.glsl
@@ -12,8 +12,8 @@ mat3 IPMapToUnitX(vec2 p0, vec2 p1) {
   // Returns a matrix that maps [p0, p1] to [(0, 0), (1, 0)]. Results are
   // undefined if p0 = p1.
   return mat3(0.0, -1.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0) *
-         IPMat3Inverse(mat3(p1.y - p0.y, p0.x - p1.x, 0.0, p1.x - p0.x, p1.y - p0.y,
-                      0.0, p0.x, p0.y, 1.0));
+         IPMat3Inverse(mat3(p1.y - p0.y, p0.x - p1.x, 0.0, p1.x - p0.x,
+                            p1.y - p0.y, 0.0, p0.x, p0.y, 1.0));
 }
 
 /// Compute the t value for a conical gradient at point `p` between the 2
diff --git a/impeller/compiler/shader_lib/impeller/transform.glsl b/impeller/compiler/shader_lib/impeller/transform.glsl
index 12a140b116..0000000000 100644
--- a/impeller/compiler/shader_lib/impeller/transform.glsl
+++ b/impeller/compiler/shader_lib/impeller/transform.glsl
@@ -30,7 +30,7 @@ vec4 IPPositionForGlyphPosition(mat4 mvp,
 
 // Shim matrix `inverse` for versions that lack it.
 // TODO: This could be gated on GLSL < 1.4.
-mat3 IPMat3Inverse( mat3 m) {
+mat3 IPMat3Inverse(mat3 m) {
   float a00 = m[0][0], a01 = m[0][1], a02 = m[0][2];
   float a10 = m[1][0], a11 = m[1][1], a12 = m[1][2];
   float a20 = m[2][0], a21 = m[2][1], a22 = m[2][2];
@@ -41,17 +41,18 @@ mat3 IPMat3Inverse( mat3 m) {
 
   float det = a00 * b01 + a01 * b11 + a02 * b21;
 
-  return mat3(b01, (-a22 * a01 + a02 * a21), (a12 * a01 - a02 * a11),
-              b11, (a22 * a00 - a02 * a20), (-a12 * a00 + a02 * a10),
-              b21, (-a21 * a00 + a01 * a20), (a11 * a00 - a01 * a10)) / det;
+  return mat3(b01, (-a22 * a01 + a02 * a21), (a12 * a01 - a02 * a11), b11,
+              (a22 * a00 - a02 * a20), (-a12 * a00 + a02 * a10), b21,
+              (-a21 * a00 + a01 * a20), (a11 * a00 - a01 * a10)) /
+         det;
 }
 
-#else // IMPELLER_TARGET_OPENGLES
+#else  // IMPELLER_TARGET_OPENGLES
 
 mat3 IPMat3Inverse(mat3 m) {
   return inverse(m);
 }
 
-#endif // IMPELLER_TARGET_OPENGLES
+#endif  // IMPELLER_TARGET_OPENGLES
 
-#endif // TRANSFORM_GLSL_
+#endif  // TRANSFORM_GLSL_
DONE

johnoneil · 2024-02-14T21:34:45Z

@bdero What's the best way to avoid similar format errors in the future? I don't know the local command to run those checks.

bdero

LGTM!

bdero · 2024-02-15T02:04:15Z

@johnoneil Shader files (.glsl, .frag, .vert) are run through clang-format on CI. So I autoformat shaders and cpp files in vscode using the xavier.clang-format plugin configured to use the formatter that ships with the Fuchsia Clang SDK (which is also used by CI):

    // In local workspace settings
    "clang-format.executable": "/absolute/path/to/my/engine/checkout/src/buildtools/mac-x64/clang/bin/clang-format",

jonahwilliams

LGTM

…#50545)

2024-02-15 30870216+gaaclarke@users.noreply.github.com Added tool to easily check golden diffs locally. (flutter/engine#50654) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 4bbf2060b008 to 12d0b7fac4c3 (2 revisions) (flutter/engine#50689) 2024-02-15 johnoneil@users.noreply.github.com Provide a matrix inverse shim for GLES 2.0. (flutter/engine#50545) 2024-02-15 jonahwilliams@google.com [iOS] Ensure FlutterMetalLayer has correct backpressure. (flutter/engine#50486) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 682f0e1e7e77 to 4bbf2060b008 (3 revisions) (flutter/engine#50686) 2024-02-15 103135467+sealesj@users.noreply.github.com Pin OSV-Scanner reusable workflow (flutter/engine#50649) 2024-02-15 whesse@google.com Add support for dart_src GN variable to flutter_frontend_server build (flutter/engine#50685) 2024-02-15 6718144+renancaraujo@users.noreply.github.com fix: consider array size on canvaskit shader data (flutter/engine#49754) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 1277910beec9 to 682f0e1e7e77 (1 revision) (flutter/engine#50683) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 85ab600a9519 to 1277910beec9 (2 revisions) (flutter/engine#50682)

github-actions bot added the e: impeller label Feb 11, 2024

johnoneil mentioned this pull request Feb 11, 2024

[Impeller] Don't use inverse in shaders targeting the OpenGLES backend. flutter/flutter#141829

Closed

jonahwilliams reviewed Feb 11, 2024

View reviewed changes

bdero reviewed Feb 13, 2024

View reviewed changes

johnoneil force-pushed the wb/johnoneil/glsl-1.0-inverse branch from 625c55b to 580777b Compare February 14, 2024 18:27

johnoneil force-pushed the wb/johnoneil/glsl-1.0-inverse branch from 580777b to 1002801 Compare February 14, 2024 20:19

Provide a matrix inverse shim for GLES 2.0.

3618421

johnoneil force-pushed the wb/johnoneil/glsl-1.0-inverse branch from 1002801 to 3618421 Compare February 15, 2024 00:03

bdero approved these changes Feb 15, 2024

View reviewed changes

jonahwilliams approved these changes Feb 15, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 15, 2024

auto-submit bot merged commit df0dc1f into flutter:main Feb 15, 2024
27 checks passed

engine-flutter-autoroll mentioned this pull request Feb 16, 2024

Roll Flutter Engine from 15a358bbaf71 to 7de84271eb65 (56 revisions) flutter/flutter#143552

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024

df0dc1fc0 Provide a matrix inverse shim for GLES 2.0. (flutter/engine…

31153ce

…#50545)

jason-simmons mentioned this pull request Feb 16, 2024

Manual roll Flutter Engine from d3c71d78f8ef to b7103bc8b3743 flutter/flutter#143563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a matrix inverse shim for GLES 2.0. #50545

Provide a matrix inverse shim for GLES 2.0. #50545

johnoneil commented Feb 11, 2024

flutter-dashboard bot commented Feb 11, 2024

jonahwilliams Feb 11, 2024

jonahwilliams Feb 11, 2024

johnoneil Feb 13, 2024

bdero Feb 13, 2024

bdero Feb 14, 2024

jonahwilliams Feb 14, 2024

bdero Feb 13, 2024

bdero commented Feb 14, 2024

jonahwilliams commented Feb 14, 2024

johnmccutchan commented Feb 14, 2024

bdero commented Feb 14, 2024

johnoneil commented Feb 14, 2024

bdero left a comment

bdero commented Feb 15, 2024

jonahwilliams left a comment

Provide a matrix inverse shim for GLES 2.0. #50545

Provide a matrix inverse shim for GLES 2.0. #50545

Conversation

johnoneil commented Feb 11, 2024

Pre-launch Checklist

flutter-dashboard bot commented Feb 11, 2024

jonahwilliams Feb 11, 2024

Choose a reason for hiding this comment

jonahwilliams Feb 11, 2024

Choose a reason for hiding this comment

johnoneil Feb 13, 2024

Choose a reason for hiding this comment

bdero Feb 13, 2024

Choose a reason for hiding this comment

bdero Feb 14, 2024

Choose a reason for hiding this comment

jonahwilliams Feb 14, 2024

Choose a reason for hiding this comment

bdero Feb 13, 2024

Choose a reason for hiding this comment

bdero commented Feb 14, 2024

jonahwilliams commented Feb 14, 2024

johnmccutchan commented Feb 14, 2024

bdero commented Feb 14, 2024

johnoneil commented Feb 14, 2024

bdero left a comment

Choose a reason for hiding this comment

bdero commented Feb 15, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment