From 7624a48dc6818fa97bae41a405b2b948d4d2727d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 28 Nov 2023 20:33:51 -0800 Subject: [PATCH 1/2] [Impeller] fix order of operations in SkSL generated texture lookup, --- impeller/compiler/spirv_sksl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/compiler/spirv_sksl.cc b/impeller/compiler/spirv_sksl.cc index 99b14d863ba5a..7536416eeb3c5 100644 --- a/impeller/compiler/spirv_sksl.cc +++ b/impeller/compiler/spirv_sksl.cc @@ -466,7 +466,7 @@ std::string CompilerSkSL::to_function_args(const TextureFunctionArguments& args, return "()"; } - return name + "_size * " + no_shader; + return name + "_size * (" + no_shader + ")"; } } // namespace compiler From d8afb4abe0014ebc7d4361cc1509c85203d789d6 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 29 Nov 2023 08:15:25 -0800 Subject: [PATCH 2/2] Add unit test. --- impeller/compiler/compiler_test.cc | 8 ++++ impeller/compiler/compiler_test.h | 4 ++ impeller/compiler/compiler_unittests.cc | 58 +++++++++++++++++++++---- impeller/fixtures/BUILD.gn | 1 + impeller/fixtures/texture_lookup.frag | 11 +++++ 5 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 impeller/fixtures/texture_lookup.frag diff --git a/impeller/compiler/compiler_test.cc b/impeller/compiler/compiler_test.cc index 024ea63ed130b..b19a84176cf36 100644 --- a/impeller/compiler/compiler_test.cc +++ b/impeller/compiler/compiler_test.cc @@ -65,6 +65,14 @@ std::unique_ptr CompilerTest::GetReflectionJson( return fml::FileMapping::CreateReadOnly(fd); } +std::unique_ptr CompilerTest::GetShaderFile( + const char* fixture_name, + TargetPlatform platform) const { + auto filename = SLFileName(fixture_name, platform); + auto fd = fml::OpenFileReadOnly(intermediates_directory_, filename.c_str()); + return fml::FileMapping::CreateReadOnly(fd); +} + bool CompilerTest::CanCompileAndReflect(const char* fixture_name, SourceType source_type, SourceLanguage source_language, diff --git a/impeller/compiler/compiler_test.h b/impeller/compiler/compiler_test.h index 64a5795709e2e..0f3c5f08c7921 100644 --- a/impeller/compiler/compiler_test.h +++ b/impeller/compiler/compiler_test.h @@ -24,6 +24,10 @@ class CompilerTest : public ::testing::TestWithParam { std::unique_ptr GetReflectionJson( const char* fixture_name) const; + std::unique_ptr GetShaderFile( + const char* fixture_name, + TargetPlatform platform) const; + bool CanCompileAndReflect( const char* fixture_name, SourceType source_type = SourceType::kUnknown, diff --git a/impeller/compiler/compiler_unittests.cc b/impeller/compiler/compiler_unittests.cc index cf70a62cee51f..539db7cd32129 100644 --- a/impeller/compiler/compiler_unittests.cc +++ b/impeller/compiler/compiler_unittests.cc @@ -2,7 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include "flutter/testing/testing.h" +#include "gtest/gtest.h" #include "impeller/base/validation.h" #include "impeller/compiler/compiler.h" #include "impeller/compiler/compiler_test.h" @@ -26,6 +28,9 @@ TEST(CompilerTest, ShaderKindMatchingIsSuccessful) { } TEST_P(CompilerTest, CanCompile) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect("sample.vert")); ASSERT_TRUE(CanCompileAndReflect("sample.vert", SourceType::kVertexShader)); ASSERT_TRUE(CanCompileAndReflect("sample.vert", SourceType::kVertexShader, @@ -33,11 +38,17 @@ TEST_P(CompilerTest, CanCompile) { } TEST_P(CompilerTest, CanCompileHLSL) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect( "simple.vert.hlsl", SourceType::kVertexShader, SourceLanguage::kHLSL)); } TEST_P(CompilerTest, CanCompileHLSLWithMultipleStages) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect("multiple_stages.hlsl", SourceType::kVertexShader, SourceLanguage::kHLSL, "VertexShader")); @@ -47,12 +58,18 @@ TEST_P(CompilerTest, CanCompileHLSLWithMultipleStages) { } TEST_P(CompilerTest, CanCompileTessellationControlShader) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect("sample.tesc")); ASSERT_TRUE(CanCompileAndReflect("sample.tesc", SourceType::kTessellationControlShader)); } TEST_P(CompilerTest, CanCompileTessellationEvaluationShader) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect("sample.tese")); ASSERT_TRUE(CanCompileAndReflect("sample.tese", SourceType::kTessellationEvaluationShader)); @@ -67,12 +84,18 @@ TEST_P(CompilerTest, CanCompileComputeShader) { } TEST_P(CompilerTest, MustFailDueToExceedingResourcesLimit) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ScopedValidationDisable disable_validation; ASSERT_FALSE( CanCompileAndReflect("resources_limit.vert", SourceType::kVertexShader)); } TEST_P(CompilerTest, MustFailDueToMultipleLocationPerStructMember) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ScopedValidationDisable disable_validation; ASSERT_FALSE(CanCompileAndReflect("struct_def_bug.vert")); } @@ -98,6 +121,9 @@ TEST_P(CompilerTest, BindingBaseForFragShader) { } TEST_P(CompilerTest, UniformsHaveBindingAndSet) { + if (GetParam() == TargetPlatform::kSkSL) { + GTEST_SKIP() << "Not supported with SkSL"; + } ASSERT_TRUE(CanCompileAndReflect("sample_with_binding.vert", SourceType::kVertexShader)); ASSERT_TRUE(CanCompileAndReflect("sample.frag", SourceType::kFragmentShader)); @@ -123,14 +149,30 @@ TEST_P(CompilerTest, UniformsHaveBindingAndSet) { ASSERT_EQ(vert_uniform_binding.binding, 17u); } -#define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ - INSTANTIATE_TEST_SUITE_P( \ - suite_name, CompilerTest, \ - ::testing::Values( \ - TargetPlatform::kOpenGLES, TargetPlatform::kOpenGLDesktop, \ - TargetPlatform::kMetalDesktop, TargetPlatform::kMetalIOS), \ - [](const ::testing::TestParamInfo& info) { \ - return TargetPlatformToString(info.param); \ +TEST_P(CompilerTest, SkSLTextureLookUpOrderOfOperations) { + if (GetParam() != TargetPlatform::kSkSL) { + GTEST_SKIP() << "Only supported on SkSL"; + } + ASSERT_TRUE( + CanCompileAndReflect("texture_lookup.frag", SourceType::kFragmentShader)); + + auto shader = GetShaderFile("texture_lookup.frag", GetParam()); + auto string_mapping = reinterpret_cast(shader->GetMapping()); + + EXPECT_TRUE(strcmp(string_mapping, + "textureA.eval(textureA_size * ( vec2(1.0) + " + "flutter_FragCoord.xy));") != -1); +} + +#define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name) \ + INSTANTIATE_TEST_SUITE_P( \ + suite_name, CompilerTest, \ + ::testing::Values(TargetPlatform::kOpenGLES, \ + TargetPlatform::kOpenGLDesktop, \ + TargetPlatform::kMetalDesktop, \ + TargetPlatform::kMetalIOS, TargetPlatform::kSkSL), \ + [](const ::testing::TestParamInfo& info) { \ + return TargetPlatformToString(info.param); \ }); INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite); diff --git a/impeller/fixtures/BUILD.gn b/impeller/fixtures/BUILD.gn index c225cad315172..566718ef270f5 100644 --- a/impeller/fixtures/BUILD.gn +++ b/impeller/fixtures/BUILD.gn @@ -101,6 +101,7 @@ test_fixtures("file_fixtures") { "two_triangles.glb", "types.h", "wtf.otf", + "texture_lookup.frag", ] if (host_os == "mac") { fixtures += [ "/System/Library/Fonts/Apple Color Emoji.ttc" ] diff --git a/impeller/fixtures/texture_lookup.frag b/impeller/fixtures/texture_lookup.frag new file mode 100644 index 0000000000000..cbf7d5db7136d --- /dev/null +++ b/impeller/fixtures/texture_lookup.frag @@ -0,0 +1,11 @@ +// 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. + +uniform sampler2D textureA; + +out vec4 frag_color; + +void main() { + frag_color = texture(textureA, vec2(1.0) + gl_FragCoord.xy); +}