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

Jonah Williams · jonahwilliams · commit 22416c28feb9 · 2022-12-12T13:24:15.000-08:00
* [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
diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc
@@ -35,6 +35,58 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir,
   sl_options.msl_version =
       spirv_cross::CompilerMSL::Options::make_msl_version(1, 2);
   sl_compiler->set_msl_options(sl_options);
+
+  // Set metal resource mappings to be consistent with location based mapping
+  // used on other backends when creating fragment shaders. This doesn't seem
+  // to work with the generated bindings for compute shaders, nor for certain
+  // shaders in the flutter/engine tree.
+  if (source_options.remap_samplers) {
+    std::vector<uint32_t> sampler_offsets;
+    std::vector<uint32_t> float_offsets;
+    ir.for_each_typed_id<spirv_cross::SPIRVariable>(
+        [&](uint32_t, const spirv_cross::SPIRVariable& var) {
+          if (var.storage != spv::StorageClassUniformConstant) {
+            return;
+          }
+          const auto spir_type = sl_compiler->get_type(var.basetype);
+          auto location = sl_compiler->get_decoration(
+              var.self, spv::Decoration::DecorationLocation);
+          if (spir_type.basetype ==
+              spirv_cross::SPIRType::BaseType::SampledImage) {
+            sampler_offsets.push_back(location);
+          } else if (spir_type.basetype ==
+                     spirv_cross::SPIRType::BaseType::Float) {
+            float_offsets.push_back(location);
+          }
+        });
+    if (sampler_offsets.size() > 0) {
+      auto start_offset =
+          *std::min_element(sampler_offsets.begin(), sampler_offsets.end());
+      for (auto offset : sampler_offsets) {
+        sl_compiler->add_msl_resource_binding({
+            .stage = spv::ExecutionModel::ExecutionModelFragment,
+            .basetype = spirv_cross::SPIRType::BaseType::SampledImage,
+            .binding = offset,
+            .count = 1u,
+            .msl_texture = offset - start_offset,
+        });
+      }
+    }
+    if (float_offsets.size() > 0) {
+      auto start_offset =
+          *std::min_element(float_offsets.begin(), float_offsets.end());
+      for (auto offset : float_offsets) {
+        sl_compiler->add_msl_resource_binding({
+            .stage = spv::ExecutionModel::ExecutionModelFragment,
+            .basetype = spirv_cross::SPIRType::BaseType::Float,
+            .binding = offset,
+            .count = 1u,
+            .msl_buffer = offset - start_offset,
+        });
+      }
+    }
+  }
+
   return CompilerBackend(sl_compiler);
 }
 
diff --git a/impeller/compiler/impellerc_main.cc b/impeller/compiler/impellerc_main.cc
@@ -73,6 +73,7 @@ bool Main(const fml::CommandLine& command_line) {
       switches.source_file_name, options.type, options.source_language,
       switches.entry_point);
   options.json_format = switches.json_format;
+  options.remap_samplers = switches.remap_samplers;
   options.gles_language_version = switches.gles_language_version;
 
   Reflector::Options reflector_options;
diff --git a/impeller/compiler/source_options.h b/impeller/compiler/source_options.h
@@ -27,6 +27,7 @@ struct SourceOptions {
   uint32_t gles_language_version = 100;
   std::vector<std::string> defines;
   bool json_format = false;
+  bool remap_samplers = false;
 
   SourceOptions();
 
diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc
@@ -71,6 +71,9 @@ void Switches::PrintHelp(std::ostream& stream) {
   stream << "[optional] --depfile=<depfile_path>" << std::endl;
   stream << "[optional] --gles-language-verision=<number>" << std::endl;
   stream << "[optional] --json" << std::endl;
+  stream << "[optional] --remap-samplers (force metal sampler index to match "
+            "declared order)"
+         << std::endl;
 }
 
 Switches::Switches() = default;
@@ -125,6 +128,7 @@ Switches::Switches(const fml::CommandLine& command_line)
           command_line.GetOptionValueWithDefault("reflection-cc", "")),
       depfile_path(command_line.GetOptionValueWithDefault("depfile", "")),
       json_format(command_line.HasOption("json")),
+      remap_samplers(command_line.HasOption("remap-samplers")),
       gles_language_version(
           stoi(command_line.GetOptionValueWithDefault("gles-language-version",
                                                       "0"))),
diff --git a/impeller/compiler/switches.h b/impeller/compiler/switches.h
@@ -32,6 +32,7 @@ struct Switches {
   std::string depfile_path;
   std::vector<std::string> defines;
   bool json_format;
+  bool remap_samplers;
   SourceLanguage source_language = SourceLanguage::kUnknown;
   uint32_t gles_language_version;
   std::string entry_point;
diff --git a/impeller/fixtures/ordering/shader_with_matrix.frag b/impeller/fixtures/ordering/shader_with_matrix.frag
@@ -0,0 +1,15 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Declare matrix in different order than usage.
+uniform mat4 matrix;
+uniform float floatB;
+
+out vec4 frag_color;
+
+void main() {
+  vec4 sample_1 = vec4(floatB);
+  vec4 sample_2 = sample_1 * matrix;
+  frag_color = sample_1 + sample_2;
+}
diff --git a/impeller/fixtures/ordering/shader_with_ordered_floats.frag b/impeller/fixtures/ordering/shader_with_ordered_floats.frag
@@ -0,0 +1,15 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Declare floats in different order than usage.
+uniform float floatA;
+uniform float floatB;
+
+out vec4 frag_color;
+
+void main() {
+  vec4 sample_1 = vec4(floatB);
+  vec4 sample_2 = vec4(floatA);
+  frag_color = sample_1 + sample_2;
+}
diff --git a/impeller/fixtures/ordering/shader_with_samplers.frag b/impeller/fixtures/ordering/shader_with_samplers.frag
@@ -0,0 +1,15 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Declare samplers in different order than usage.
+uniform sampler2D textureA;
+uniform sampler2D textureB;
+
+out vec4 frag_color;
+
+void main() {
+  vec4 sample_1 = texture(textureB, vec2(1.0));
+  vec4 sample_2 = texture(textureA, vec2(1.0));
+  frag_color = sample_1 + sample_2;
+}
diff --git a/impeller/tools/impeller.gni b/impeller/tools/impeller.gni
@@ -233,9 +233,13 @@ template("impellerc") {
     iplr = invoker.iplr
   }
   json = false
+  remap_samplers = false
   if (defined(invoker.json) && invoker.json) {
     json = invoker.json
   }
+  if (defined(invoker.remap_samplers) && invoker.remap_samplers) {
+    remap_samplers = invoker.remap_samplers
+  }
 
   # Not needed on every path.
   not_needed([
@@ -248,6 +252,8 @@ template("impellerc") {
   # Optional: invoker.intermediates_subdir specifies the subdirectory in which
   #           to put intermediates.
   # Optional: invoker.json Causes output format to be JSON instead of flatbuffer.
+  # Optional: invoker.remap_samplers Output metal samplers according to
+  #           declaration order instead of usage order.
 
   _impellerc(target_name) {
     sources = invoker.shaders
@@ -284,6 +290,9 @@ template("impellerc") {
     if (json) {
       args += [ "--json" ]
     }
+    if (remap_samplers) {
+      args += [ "--remap-samplers" ]
+    }
 
     if (sksl) {
       sl_intermediate =
diff --git a/lib/ui/fixtures/shaders/BUILD.gn b/lib/ui/fixtures/shaders/BUILD.gn
@@ -33,10 +33,24 @@ if (enable_unittests) {
     json = true
   }
 
+  impellerc("sampler_order_fixture") {
+    shaders = [
+      "//flutter/impeller/fixtures/ordering/shader_with_samplers.frag",
+      "//flutter/impeller/fixtures/ordering/shader_with_ordered_floats.frag",
+      "//flutter/impeller/fixtures/ordering/shader_with_matrix.frag",
+    ]
+    shader_target_flag = "--runtime-stage-metal"
+    intermediates_subdir = "iplr-remap"
+    sl_file_extension = "iplr"
+    iplr = true
+    remap_samplers = true
+  }
+
   test_fixtures("fixtures") {
     deps = [
       ":ink_sparkle",
       ":ink_sparkle_web",
+      ":sampler_order_fixture",
     ]
     fixtures = get_target_outputs(":ink_sparkle")
     dest = "$root_gen_dir/flutter/lib/ui"
diff --git a/testing/dart/fragment_shader_test.dart b/testing/dart/fragment_shader_test.dart
@@ -332,6 +332,47 @@ void main() async {
     shader.dispose();
   });
 
+  // This test can't rely on actual pixels rendered since it needs to run on a
+  // metal shader on iOS. instead parse the source code.
+  test('impellerc orders samplers in metal shader according to declaration and not usage', () async {
+    if (!Platform.isMacOS) {
+      return;
+    }
+    final Directory directory = shaderDirectory('iplr-remap');
+    final String data = readAsStringLossy(File(path.join(directory.path, 'shader_with_samplers.frag.iplr')));
+
+    const String expected = 'texture2d<float> textureA [[texture(0)]],'
+      ' texture2d<float> textureB [[texture(1)]]';
+
+    expect(data, contains(expected));
+  });
+
+  test('impellerc orders floats in metal shader according to declaration and not usage', () async {
+    if (!Platform.isMacOS) {
+      return;
+    }
+    final Directory directory = shaderDirectory('iplr-remap');
+    final String data = readAsStringLossy(File(path.join(directory.path, 'shader_with_ordered_floats.frag.iplr')));
+
+    const String expected = 'constant float& floatA [[buffer(0)]], '
+      'constant float& floatB [[buffer(1)]]';
+
+    expect(data, contains(expected));
+  });
+
+  test('impellerc orders floats/matrix in metal shader according to declaration and not usage', () async {
+    if (!Platform.isMacOS) {
+      return;
+    }
+    final Directory directory = shaderDirectory('iplr-remap');
+    final String data = readAsStringLossy(File(path.join(directory.path, 'shader_with_matrix.frag.iplr')));
+
+    const String expected = 'constant float4x4& matrix [[buffer(0)]], '
+      'constant float& floatB [[buffer(1)]]';
+
+    expect(data, contains(expected));
+  });
+
   // Test all supported GLSL ops. See lib/spirv/lib/src/constants.dart
   final Map<String, FragmentProgram> iplrSupportedGLSLOpShaders = await _loadShaderAssets(
     path.join('supported_glsl_op_shaders', 'iplr'),
@@ -484,3 +525,8 @@ Image _createBlueGreenImageSync() {
     picture.dispose();
   }
 }
+
+// Ignore invalid utf8 since file is not actually text.
+String readAsStringLossy(File file) {
+  return convert.utf8.decode(file.readAsBytesSync(), allowMalformed: true);
+}
