[Impeller] fix order of operations in SkSL generated texture lookup. (#48488)

Fixes flutter/flutter#139017

When multiplying by the texture size we need to add parens incase the texture argument is actually an expression and not a single value.

Author: Jonah Williams

impeller/compiler/compiler_test.cc

@@ -65,6 +65,14 @@ std::unique_ptr<fml::FileMapping> CompilerTest::GetReflectionJson(
   return fml::FileMapping::CreateReadOnly(fd);
 }
 
+std::unique_ptr<fml::FileMapping> 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,

impeller/compiler/compiler_test.h

@@ -24,6 +24,10 @@ class CompilerTest : public ::testing::TestWithParam<TargetPlatform> {
   std::unique_ptr<fml::FileMapping> GetReflectionJson(
       const char* fixture_name) const;
 
+  std::unique_ptr<fml::FileMapping> GetShaderFile(
+      const char* fixture_name,
+      TargetPlatform platform) const;
+
   bool CanCompileAndReflect(
       const char* fixture_name,
       SourceType source_type = SourceType::kUnknown,

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 <cstring>
 #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,18 +28,27 @@ 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,
                                    SourceLanguage::kGLSL));
 }
 
 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<CompilerTest::ParamType>& 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<const char*>(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<CompilerTest::ParamType>& info) {  \
+        return TargetPlatformToString(info.param);                         \
       });
 
 INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(CompilerSuite);

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

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" ]

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);
+}