From 7d7aa3bc8612fd20705caac25c1973cdc82e62b1 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 6 Dec 2022 19:10:35 -0800 Subject: [PATCH 1/9] [Impeller] order metal samplers according to declared order and not use order --- impeller/compiler/compiler.cc | 38 +++++++++++++++++++++++++++++ impeller/compiler/impellerc_main.cc | 1 + impeller/compiler/source_options.h | 1 + impeller/compiler/switches.cc | 2 ++ impeller/compiler/switches.h | 1 + 5 files changed, 43 insertions(+) diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index c8fe2f3890404..06f52f3c1ded1 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -35,6 +35,44 @@ 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 relies on the + // fact that the order of uniforms in the IR mirrors the declared order in the + // shader source. + if (source_options.remap_samplers) { + std::vector sampler_offsets; + ir.for_each_typed_id( + [&](uint32_t, const spirv_cross::SPIRVariable& var) { + if (var.storage != spv::StorageClassUniformConstant) { + return; + } + const auto spir_type = sl_compiler->get_type(var.basetype); + if (spir_type.basetype != + spirv_cross::SPIRType::BaseType::SampledImage) { + return; + } + auto location = sl_compiler->get_decoration( + var.self, spv::Decoration::DecorationLocation); + sampler_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_buffer = offset - start_offset, + .msl_texture = offset - start_offset, + .msl_sampler = offset - start_offset, + }); + } + } + } + return CompilerBackend(sl_compiler); } diff --git a/impeller/compiler/impellerc_main.cc b/impeller/compiler/impellerc_main.cc index f25ecd3f86509..f412c21d86495 100644 --- 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 index ccd264562a3d0..100462ea0d095 100644 --- 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 defines; bool json_format = false; + bool remap_samplers = false; SourceOptions(); diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc index 7618b0ed0ad73..e2d4139eb4aeb 100644 --- a/impeller/compiler/switches.cc +++ b/impeller/compiler/switches.cc @@ -71,6 +71,7 @@ void Switches::PrintHelp(std::ostream& stream) { stream << "[optional] --depfile=" << std::endl; stream << "[optional] --gles-language-verision=" << 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 +126,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 index 01774285e7f27..198c67590443e 100644 --- a/impeller/compiler/switches.h +++ b/impeller/compiler/switches.h @@ -32,6 +32,7 @@ struct Switches { std::string depfile_path; std::vector defines; bool json_format; + bool remap_samplers; SourceLanguage source_language = SourceLanguage::kUnknown; uint32_t gles_language_version; std::string entry_point; From 3d8d5f84ad37997f435c8dd9b1ef645f6b52e271 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 6 Dec 2022 19:11:08 -0800 Subject: [PATCH 2/9] ++ --- impeller/compiler/switches.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc index e2d4139eb4aeb..f37bf41ac5054 100644 --- a/impeller/compiler/switches.cc +++ b/impeller/compiler/switches.cc @@ -71,7 +71,9 @@ void Switches::PrintHelp(std::ostream& stream) { stream << "[optional] --depfile=" << std::endl; stream << "[optional] --gles-language-verision=" << std::endl; stream << "[optional] --json" << std::endl; - stream << "[optional] --remap-samplers (force metal sampler index to match declared order)" << std::endl; + stream << "[optional] --remap-samplers (force metal sampler index to match " + "declared order)" + << std::endl; } Switches::Switches() = default; From 2fffb05aeea9cfcbd0df051540054ca0d6c337c0 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 6 Dec 2022 22:25:33 -0800 Subject: [PATCH 3/9] always enabl remapping --- impeller/compiler/compiler.cc | 82 +++++++++++++++++------------ impeller/compiler/impellerc_main.cc | 1 - impeller/compiler/source_options.h | 1 - impeller/compiler/switches.cc | 4 -- impeller/compiler/switches.h | 1 - 5 files changed, 49 insertions(+), 40 deletions(-) diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index 06f52f3c1ded1..de316bb27d052 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -20,6 +20,24 @@ namespace impeller { namespace compiler { +static std::optional SourceTypeToExecutionModel( + const SourceType& type) { + switch (type) { + case SourceType::kUnknown: + return std::nullopt; + case SourceType::kVertexShader: + return spv::ExecutionModel::ExecutionModelVertex; + case SourceType::kFragmentShader: + return spv::ExecutionModel::ExecutionModelFragment; + case SourceType::kTessellationControlShader: + return spv::ExecutionModel::ExecutionModelTessellationControl; + case SourceType::kTessellationEvaluationShader: + return spv::ExecutionModel::ExecutionModelTessellationEvaluation; + case SourceType::kComputeShader: + return spv::ExecutionModel::ExecutionModelGLCompute; + } +} + const uint32_t kFragBindingBase = 128; const size_t kNumUniformKinds = static_cast(shaderc_uniform_kind::shaderc_uniform_kind_buffer) + 1; @@ -34,42 +52,40 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, // Metal to AIR must be updated as well. sl_options.msl_version = spirv_cross::CompilerMSL::Options::make_msl_version(1, 2); + sl_options.enable_decoration_binding = true; 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 relies on the - // fact that the order of uniforms in the IR mirrors the declared order in the - // shader source. - if (source_options.remap_samplers) { - std::vector sampler_offsets; - ir.for_each_typed_id( - [&](uint32_t, const spirv_cross::SPIRVariable& var) { - if (var.storage != spv::StorageClassUniformConstant) { - return; - } - const auto spir_type = sl_compiler->get_type(var.basetype); - if (spir_type.basetype != - spirv_cross::SPIRType::BaseType::SampledImage) { - return; - } - auto location = sl_compiler->get_decoration( - var.self, spv::Decoration::DecorationLocation); - sampler_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_buffer = offset - start_offset, - .msl_texture = offset - start_offset, - .msl_sampler = offset - start_offset, - }); - } + // used on other backends when creating shaders. + std::vector sampler_offsets; + ir.for_each_typed_id( + [&](uint32_t, const spirv_cross::SPIRVariable& var) { + if (var.storage != spv::StorageClassUniformConstant) { + return; + } + const auto spir_type = sl_compiler->get_type(var.basetype); + if (spir_type.basetype != + spirv_cross::SPIRType::BaseType::SampledImage) { + return; + } + auto location = sl_compiler->get_decoration( + var.self, spv::Decoration::DecorationLocation); + sampler_offsets.push_back(location); + }); + auto stage = SourceTypeToExecutionModel(source_options.type); + if (stage.has_value() && 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 = stage.value(), + .basetype = spirv_cross::SPIRType::BaseType::SampledImage, + .binding = offset, + .count = 1u, + .msl_buffer = offset - start_offset, + .msl_texture = offset - start_offset, + .msl_sampler = offset - start_offset, + }); } } diff --git a/impeller/compiler/impellerc_main.cc b/impeller/compiler/impellerc_main.cc index f412c21d86495..f25ecd3f86509 100644 --- a/impeller/compiler/impellerc_main.cc +++ b/impeller/compiler/impellerc_main.cc @@ -73,7 +73,6 @@ 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 index 100462ea0d095..ccd264562a3d0 100644 --- a/impeller/compiler/source_options.h +++ b/impeller/compiler/source_options.h @@ -27,7 +27,6 @@ struct SourceOptions { uint32_t gles_language_version = 100; std::vector defines; bool json_format = false; - bool remap_samplers = false; SourceOptions(); diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc index d4f6ecee9e0fe..17bc358ac3f09 100644 --- a/impeller/compiler/switches.cc +++ b/impeller/compiler/switches.cc @@ -71,9 +71,6 @@ void Switches::PrintHelp(std::ostream& stream) { stream << "[optional] --depfile=" << std::endl; stream << "[optional] --gles-language-verision=" << std::endl; stream << "[optional] --json" << std::endl; - stream << "[optional] --remap-samplers (force metal sampler index to match " - "declared order)" - << std::endl; } Switches::Switches() = default; @@ -128,7 +125,6 @@ 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 index 198c67590443e..01774285e7f27 100644 --- a/impeller/compiler/switches.h +++ b/impeller/compiler/switches.h @@ -32,7 +32,6 @@ struct Switches { std::string depfile_path; std::vector defines; bool json_format; - bool remap_samplers; SourceLanguage source_language = SourceLanguage::kUnknown; uint32_t gles_language_version; std::string entry_point; From 6ce0ca350ed47d3445833f6f35c4669058cf7f36 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 10:43:36 -0800 Subject: [PATCH 4/9] Revert "always enabl remapping" This reverts commit 2fffb05aeea9cfcbd0df051540054ca0d6c337c0. --- impeller/compiler/compiler.cc | 82 ++++++++++++----------------- impeller/compiler/impellerc_main.cc | 1 + impeller/compiler/source_options.h | 1 + impeller/compiler/switches.cc | 4 ++ impeller/compiler/switches.h | 1 + 5 files changed, 40 insertions(+), 49 deletions(-) diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index de316bb27d052..06f52f3c1ded1 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -20,24 +20,6 @@ namespace impeller { namespace compiler { -static std::optional SourceTypeToExecutionModel( - const SourceType& type) { - switch (type) { - case SourceType::kUnknown: - return std::nullopt; - case SourceType::kVertexShader: - return spv::ExecutionModel::ExecutionModelVertex; - case SourceType::kFragmentShader: - return spv::ExecutionModel::ExecutionModelFragment; - case SourceType::kTessellationControlShader: - return spv::ExecutionModel::ExecutionModelTessellationControl; - case SourceType::kTessellationEvaluationShader: - return spv::ExecutionModel::ExecutionModelTessellationEvaluation; - case SourceType::kComputeShader: - return spv::ExecutionModel::ExecutionModelGLCompute; - } -} - const uint32_t kFragBindingBase = 128; const size_t kNumUniformKinds = static_cast(shaderc_uniform_kind::shaderc_uniform_kind_buffer) + 1; @@ -52,40 +34,42 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, // Metal to AIR must be updated as well. sl_options.msl_version = spirv_cross::CompilerMSL::Options::make_msl_version(1, 2); - sl_options.enable_decoration_binding = true; sl_compiler->set_msl_options(sl_options); // Set metal resource mappings to be consistent with location based mapping - // used on other backends when creating shaders. - std::vector sampler_offsets; - ir.for_each_typed_id( - [&](uint32_t, const spirv_cross::SPIRVariable& var) { - if (var.storage != spv::StorageClassUniformConstant) { - return; - } - const auto spir_type = sl_compiler->get_type(var.basetype); - if (spir_type.basetype != - spirv_cross::SPIRType::BaseType::SampledImage) { - return; - } - auto location = sl_compiler->get_decoration( - var.self, spv::Decoration::DecorationLocation); - sampler_offsets.push_back(location); - }); - auto stage = SourceTypeToExecutionModel(source_options.type); - if (stage.has_value() && 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 = stage.value(), - .basetype = spirv_cross::SPIRType::BaseType::SampledImage, - .binding = offset, - .count = 1u, - .msl_buffer = offset - start_offset, - .msl_texture = offset - start_offset, - .msl_sampler = offset - start_offset, - }); + // used on other backends when creating fragment shaders. This relies on the + // fact that the order of uniforms in the IR mirrors the declared order in the + // shader source. + if (source_options.remap_samplers) { + std::vector sampler_offsets; + ir.for_each_typed_id( + [&](uint32_t, const spirv_cross::SPIRVariable& var) { + if (var.storage != spv::StorageClassUniformConstant) { + return; + } + const auto spir_type = sl_compiler->get_type(var.basetype); + if (spir_type.basetype != + spirv_cross::SPIRType::BaseType::SampledImage) { + return; + } + auto location = sl_compiler->get_decoration( + var.self, spv::Decoration::DecorationLocation); + sampler_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_buffer = offset - start_offset, + .msl_texture = offset - start_offset, + .msl_sampler = offset - start_offset, + }); + } } } diff --git a/impeller/compiler/impellerc_main.cc b/impeller/compiler/impellerc_main.cc index f25ecd3f86509..f412c21d86495 100644 --- 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 index ccd264562a3d0..100462ea0d095 100644 --- 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 defines; bool json_format = false; + bool remap_samplers = false; SourceOptions(); diff --git a/impeller/compiler/switches.cc b/impeller/compiler/switches.cc index 17bc358ac3f09..d4f6ecee9e0fe 100644 --- a/impeller/compiler/switches.cc +++ b/impeller/compiler/switches.cc @@ -71,6 +71,9 @@ void Switches::PrintHelp(std::ostream& stream) { stream << "[optional] --depfile=" << std::endl; stream << "[optional] --gles-language-verision=" << 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 index 01774285e7f27..198c67590443e 100644 --- a/impeller/compiler/switches.h +++ b/impeller/compiler/switches.h @@ -32,6 +32,7 @@ struct Switches { std::string depfile_path; std::vector defines; bool json_format; + bool remap_samplers; SourceLanguage source_language = SourceLanguage::kUnknown; uint32_t gles_language_version; std::string entry_point; From d089ef1944d514fc22502e6e8f3f4799c781df25 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 10:45:56 -0800 Subject: [PATCH 5/9] ++ --- impeller/compiler/compiler.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index 06f52f3c1ded1..dc19809142c52 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -37,10 +37,11 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, 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 relies on the - // fact that the order of uniforms in the IR mirrors the declared order in the - // shader source. + // 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) { + sl_options.enable_decoration_binding = true; std::vector sampler_offsets; ir.for_each_typed_id( [&](uint32_t, const spirv_cross::SPIRVariable& var) { From eb267f4fbfec7e224e734596b3aad9a8a47ea5ac Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 11:48:51 -0800 Subject: [PATCH 6/9] add test --- impeller/fixtures/shader_with_samplers.frag | 15 +++++++++++++++ impeller/tools/impeller.gni | 9 +++++++++ lib/ui/fixtures/shaders/BUILD.gn | 10 ++++++++++ testing/dart/fragment_shader_test.dart | 12 ++++++++++++ 4 files changed, 46 insertions(+) create mode 100644 impeller/fixtures/shader_with_samplers.frag diff --git a/impeller/fixtures/shader_with_samplers.frag b/impeller/fixtures/shader_with_samplers.frag new file mode 100644 index 0000000000000..2900797d22d00 --- /dev/null +++ b/impeller/fixtures/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 index 818f19dc964a7..d1fb6eb3ee7b9 100644 --- a/impeller/tools/impeller.gni +++ b/impeller/tools/impeller.gni @@ -241,9 +241,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([ @@ -256,6 +260,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 @@ -292,6 +298,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 index ff4b9b458e7cb..60cc85f02ec2e 100644 --- a/lib/ui/fixtures/shaders/BUILD.gn +++ b/lib/ui/fixtures/shaders/BUILD.gn @@ -33,10 +33,20 @@ if (enable_unittests) { json = true } + impellerc("sampler_order_fixture") { + shaders = [ "//flutter/impeller/fixtures/shader_with_samplers.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 index 9fd025ab04085..c791a84109bc8 100644 --- a/testing/dart/fragment_shader_test.dart +++ b/testing/dart/fragment_shader_test.dart @@ -332,6 +332,18 @@ 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 { + final Directory directory = shaderDirectory('iplr-remap'); + final String data = File(path.join(directory.path, 'shader_with_samplers.frag.iplr')).readAsStringSync(); + + const String expected = 'texture2d textureA [[texture(0)]],' + ' texture2d textureB [[texture(1)]]'; + + expect(data, contains(expected)); + }); + // Test all supported GLSL ops. See lib/spirv/lib/src/constants.dart final Map iplrSupportedGLSLOpShaders = await _loadShaderAssets( path.join('supported_glsl_op_shaders', 'iplr'), From 5f576e6da6f594fd4fd3e826e5b691fc736d2c9d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 12:19:19 -0800 Subject: [PATCH 7/9] ++ --- impeller/compiler/compiler.cc | 29 ++++++++++++++----- .../fixtures/ordering/shader_with_matrix.frag | 15 ++++++++++ .../ordering/shader_with_ordered_floats.frag | 15 ++++++++++ .../{ => ordering}/shader_with_samplers.frag | 0 lib/ui/fixtures/shaders/BUILD.gn | 6 +++- testing/dart/fragment_shader_test.dart | 20 +++++++++++++ 6 files changed, 76 insertions(+), 9 deletions(-) create mode 100644 impeller/fixtures/ordering/shader_with_matrix.frag create mode 100644 impeller/fixtures/ordering/shader_with_ordered_floats.frag rename impeller/fixtures/{ => ordering}/shader_with_samplers.frag (100%) diff --git a/impeller/compiler/compiler.cc b/impeller/compiler/compiler.cc index dc19809142c52..8f3eed1edd0fa 100644 --- a/impeller/compiler/compiler.cc +++ b/impeller/compiler/compiler.cc @@ -41,21 +41,23 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, // to work with the generated bindings for compute shaders, nor for certain // shaders in the flutter/engine tree. if (source_options.remap_samplers) { - sl_options.enable_decoration_binding = true; std::vector sampler_offsets; + std::vector float_offsets; ir.for_each_typed_id( [&](uint32_t, const spirv_cross::SPIRVariable& var) { if (var.storage != spv::StorageClassUniformConstant) { return; } const auto spir_type = sl_compiler->get_type(var.basetype); - if (spir_type.basetype != - spirv_cross::SPIRType::BaseType::SampledImage) { - return; - } auto location = sl_compiler->get_decoration( var.self, spv::Decoration::DecorationLocation); - sampler_offsets.push_back(location); + 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 = @@ -66,9 +68,20 @@ static CompilerBackend CreateMSLCompiler(const spirv_cross::ParsedIR& ir, .basetype = spirv_cross::SPIRType::BaseType::SampledImage, .binding = offset, .count = 1u, - .msl_buffer = offset - start_offset, .msl_texture = offset - start_offset, - .msl_sampler = 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, }); } } diff --git a/impeller/fixtures/ordering/shader_with_matrix.frag b/impeller/fixtures/ordering/shader_with_matrix.frag new file mode 100644 index 0000000000000..86dbd1b07f8c3 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..a0064cbaf683f --- /dev/null +++ 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/shader_with_samplers.frag b/impeller/fixtures/ordering/shader_with_samplers.frag similarity index 100% rename from impeller/fixtures/shader_with_samplers.frag rename to impeller/fixtures/ordering/shader_with_samplers.frag diff --git a/lib/ui/fixtures/shaders/BUILD.gn b/lib/ui/fixtures/shaders/BUILD.gn index 60cc85f02ec2e..a148bad71c699 100644 --- a/lib/ui/fixtures/shaders/BUILD.gn +++ b/lib/ui/fixtures/shaders/BUILD.gn @@ -34,7 +34,11 @@ if (enable_unittests) { } impellerc("sampler_order_fixture") { - shaders = [ "//flutter/impeller/fixtures/shader_with_samplers.frag" ] + 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" diff --git a/testing/dart/fragment_shader_test.dart b/testing/dart/fragment_shader_test.dart index c791a84109bc8..ae4d23de2b48f 100644 --- a/testing/dart/fragment_shader_test.dart +++ b/testing/dart/fragment_shader_test.dart @@ -344,6 +344,26 @@ void main() async { expect(data, contains(expected)); }); + test('impellerc orders floats 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_ordered_floats.frag.iplr')).readAsStringSync(); + + 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 { + final Directory directory = shaderDirectory('iplr-remap'); + final String data = File(path.join(directory.path, 'shader_with_matrix.frag.iplr')).readAsStringSync(); + + 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 iplrSupportedGLSLOpShaders = await _loadShaderAssets( path.join('supported_glsl_op_shaders', 'iplr'), From b487d0d2118fa51749c124903e68bdd5839ad717 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 13:15:29 -0800 Subject: [PATCH 8/9] ++ --- testing/dart/fragment_shader_test.dart | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/testing/dart/fragment_shader_test.dart b/testing/dart/fragment_shader_test.dart index ae4d23de2b48f..8c531976e1077 100644 --- a/testing/dart/fragment_shader_test.dart +++ b/testing/dart/fragment_shader_test.dart @@ -336,7 +336,7 @@ void main() async { // 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(); + final String data = readAsStringLossy(File(path.join(directory.path, 'shader_with_samplers.frag.iplr'))); const String expected = 'texture2d textureA [[texture(0)]],' ' texture2d textureB [[texture(1)]]'; @@ -346,7 +346,7 @@ void main() async { test('impellerc orders floats 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_ordered_floats.frag.iplr')).readAsStringSync(); + 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)]]'; @@ -356,7 +356,7 @@ void main() async { test('impellerc orders floats/matrix 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_matrix.frag.iplr')).readAsStringSync(); + 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)]]'; @@ -516,3 +516,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); +} From 939f10e8a02145d12167b6807cc0da1e08e0077a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 7 Dec 2022 14:50:37 -0800 Subject: [PATCH 9/9] only run on mac --- testing/dart/fragment_shader_test.dart | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/testing/dart/fragment_shader_test.dart b/testing/dart/fragment_shader_test.dart index 8c531976e1077..0bbc093582bf5 100644 --- a/testing/dart/fragment_shader_test.dart +++ b/testing/dart/fragment_shader_test.dart @@ -335,6 +335,9 @@ void main() async { // 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'))); @@ -345,6 +348,9 @@ void main() async { }); 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'))); @@ -355,6 +361,9 @@ void main() async { }); 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')));