[Impeller] order metal samplers according to declared order and not usage order #38115

jonahwilliams · 2022-12-07T03:14:26Z

Currently when we generate metal shaders for developer authored fragment shaders, we rely on the declaration order to corespond with the index of each float/sampler. By default samplers seem to get ordered by usage, so we need to add some resource mappings.

…se order

flutter-dashboard · 2022-12-07T03:14:28Z

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

chinmaygarde · 2022-12-07T05:04:13Z

impeller/compiler/compiler.cc

+
+  // Set metal resource mappings to be consistent with location based mapping
+  // used on other backends when creating fragment shaders.
+  if (source_options.remap_samplers) {


Can this just be the default?

There are still some compilation errors when using this on the flutter/engine shaders. I need to spend some more time tracking down why that is

SGTM. I'm just catching up on the issue and the conversation in chat. Will take a closer look in the morning.

I think this works if I also set enable_decoration_binding to true. that also fixes the issue with floats being out of order, but I need to verify that our bindings are still correct.

zanderso · 2022-12-07T16:42:29Z

impeller/compiler/compiler.cc

@@ -20,6 +20,24 @@
 namespace impeller {
 namespace compiler {

+static std::optional<spv::ExecutionModel> SourceTypeToExecutionModel(
+    const SourceType& type) {


nit: SourceType is just an enum, so we'd usually pass it by value.

Suggested change

const SourceType& type) {

SourceType type) {

jonahwilliams · 2022-12-07T17:13:38Z

Setting enable_decoration_binding is causing the compute shader headers to generate bogus code. the ext_res_0 field is ending up with a value of 4294967295 instead of 0, 1, 2.

We may need to map all resource locations and not just samplers

This reverts commit 2fffb05.

jonahwilliams · 2022-12-07T18:50:04Z

I moved this back to an opt-in flag, there were some confusing issues with compute shaders.

jonahwilliams · 2022-12-07T19:17:59Z

I don't really want to do a rendering test for this. Since this requires a change in the framework to opt in, I could add a test there that scans the output of the generated glowing text shader and verifies the order is as expected. Thoughts?

jonahwilliams · 2022-12-07T19:20:16Z

or I guess I could do that here actually

zanderso · 2022-12-07T19:38:06Z

Could you do something like this https://github.com/flutter/engine/blob/main/testing/dart/fragment_shader_test.dart#L275 ?

jonahwilliams · 2022-12-07T19:42:43Z

No, because it needs to use a metal shader on iOS

jonahwilliams · 2022-12-07T19:49:37Z

impeller/fixtures/shader_with_samplers.frag

+out vec4 frag_color;
+
+void main() {
+  vec4 sample_1 = texture(textureB, vec2(1.0));


By default textureB ends up with offset 0 and textureA ends up with offset 1

jonahwilliams · 2022-12-07T20:09:56Z

Failing on CI is a bad sign...

zanderso · 2022-12-07T21:02:50Z

testing/dart/fragment_shader_test.dart

+  // metal shader on iOS. instead parse the source code.
+  test('impellerc orders samplers in metal shader according to declaration and not usage', () async {
+    final Directory directory = shaderDirectory('iplr-remap');
+    final String data = File(path.join(directory.path, 'shader_with_samplers.frag.iplr')).readAsStringSync();


The .iplr file is flatbuffer encoded.

ah right. I need to either parse the flatbuffer or scan the raw bytes...

When generating an .iplr, the plain text platform specific shader might also be generated nearby. Sorry, I'm blanking on the details, but impellerc should be able to generate both at the same time, somehow. Then you can check the bytes of the plain text shader.

strings are encoded as utf8 strings in flatbuffer format, so there are a number of ways to pull that out of the file even without decoding it

zanderso

LGTM

… and not usage order (flutter/engine#38115)

…116802) * bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef8585 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21d4 Remove autoninja. (flutter/engine#38136) * 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (#38127)" (flutter/engine#38137) * b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016) * aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

…sage order (flutter#38115) * [Impeller] order metal samplers according to declared order and not use order * ++ * always enabl remapping * Revert "always enabl remapping" This reverts commit 2fffb05. * ++ * add test * ++ * ++ * only run on mac

* [Impeller] order metal samplers according to declared order and not usage order (#38115) * [Impeller] order metal samplers according to declared order and not use order * ++ * always enabl remapping * Revert "always enabl remapping" This reverts commit 2fffb05. * ++ * add test * ++ * ++ * only run on mac * Fix sampler offsets (#38170) * [impeller] dont remap floats * ++ * Update fragment_shader_test.dart * ++: * ++

…lutter#116802) * bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef8585 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21d4 Remove autoninja. (flutter/engine#38136) * 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (flutter#38127)" (flutter/engine#38137) * b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016) * aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

jonahwilliams added 3 commits December 6, 2022 19:10

[Impeller] order metal samplers according to declared order and not u…

7d7aa3b

…se order

++

3d8d5f8

Merge branch 'master' of github.com:flutter/engine into shader_lib_build

8f41b7d

flutter-dashboard bot added the needs tests label Dec 7, 2022

jonahwilliams changed the title ~~[Impeller] order metal samplers according to declared order and not use order~~ [Impeller] order metal samplers according to declared order and not usage order Dec 7, 2022

jonahwilliams requested a review from zanderso December 7, 2022 04:35

chinmaygarde added the e: impeller label Dec 7, 2022

chinmaygarde reviewed Dec 7, 2022

View reviewed changes

always enabl remapping

2fffb05

jonahwilliams force-pushed the shader_lib_build branch from 531c2d8 to 2fffb05 Compare December 7, 2022 06:25

zanderso reviewed Dec 7, 2022

View reviewed changes

jonahwilliams added 2 commits December 7, 2022 10:43

Revert "always enabl remapping"

6ce0ca3

This reverts commit 2fffb05.

++

d089ef1

add test

eb267f4

jonahwilliams commented Dec 7, 2022

View reviewed changes

jonahwilliams requested a review from zanderso December 7, 2022 19:49

jonahwilliams removed the needs tests label Dec 7, 2022

++

5f576e6

zanderso reviewed Dec 7, 2022

View reviewed changes

jonahwilliams added 2 commits December 7, 2022 13:15

++

b487d0d

only run on mac

939f10e

jonahwilliams requested a review from zanderso December 8, 2022 16:54

zanderso approved these changes Dec 8, 2022

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 8, 2022

auto-submit bot merged commit 3140ad9 into flutter:main Dec 8, 2022

jonahwilliams deleted the shader_lib_build branch December 8, 2022 17:56

engine-flutter-autoroll mentioned this pull request Dec 9, 2022

Roll Flutter Engine from 8d83b98c55b3 to 030950f3070c (29 revisions) flutter/flutter#116802

Merged

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

3140ad924 [Impeller] order metal samplers according to declared order…

f2046f6

… and not usage order (flutter/engine#38115)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] order metal samplers according to declared order and not usage order #38115

[Impeller] order metal samplers according to declared order and not usage order #38115

jonahwilliams commented Dec 7, 2022 •

edited

Loading

flutter-dashboard bot commented Dec 7, 2022

chinmaygarde Dec 7, 2022

jonahwilliams Dec 7, 2022

chinmaygarde Dec 7, 2022

jonahwilliams Dec 7, 2022

zanderso Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

zanderso commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams Dec 7, 2022

jonahwilliams commented Dec 7, 2022

zanderso Dec 7, 2022

jonahwilliams Dec 7, 2022

zanderso Dec 7, 2022

jonahwilliams Dec 7, 2022

zanderso left a comment

[Impeller] order metal samplers according to declared order and not usage order #38115

[Impeller] order metal samplers according to declared order and not usage order #38115

Conversation

jonahwilliams commented Dec 7, 2022 • edited Loading

flutter-dashboard bot commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

zanderso commented Dec 7, 2022

jonahwilliams commented Dec 7, 2022

Choose a reason for hiding this comment

jonahwilliams commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 7, 2022 •

edited

Loading