commit 10b7541a1edc4c856937346e162086b368107752
author Brian Osman <brianosman@google.com> Mon Jun 29 16:48:46 2020 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Mon Jun 29 23:41:02 2020 +0000
tree f250b0ae37b9bca570bb9c3dc469e644219fcb9a
parent 3a54b68cb4362d3dd44516d32cfb39355254759e

Add proj() intrinsic (XY / Z), fix bug with perspective matrix sampling

Previously, we'd declare a new temporary variable using the matrix
expression. For GrSkSLFP in particular, that would happen *before*
any other code had been emitted, so if the expression referred to
local variables, it would produce SkSL that used a not-yet-defined
variable, and not compile.

Change-Id: I1cdd8920b0c659b905809c126d10c01b996e6a55
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/299778
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

gm/fp_sample_chaining.cpp

diff --git a/gm/fp_sample_chaining.cpp b/gm/fp_sample_chaining.cpp
index a13b395..b7b1aa2 100644
--- a/gm/fp_sample_chaining.cpp
+++ b/gm/fp_sample_chaining.cpp
@@ -294,15 +294,14 @@
 )";
 
 // This form (uniform * constant) is currently detected as variable, thanks to our limited analysis
-// when scanning for sample matrices. If/when that improves, make this expression more complicated.
-// NOTE: An easy fix would be to hoist the expression to a local matrix variable, but that triggers
-// another bug where the resulting expression is used to compute the new local coords *before* the
-// local variable is declared.
+// when scanning for sample matrices. With that pulled into a separate local, it's highly unlikely
+// we'll ever treat this as anything else.
 const char* gVariableMatrixSkSL = R"(
     in shader child;
     uniform float3x3 matrix;
     void main(float2 xy, inout half4 color) {
-        color = sample(child, matrix * 0.5);
+        float3x3 varMatrix = matrix * 0.5;
+        color = sample(child, varMatrix);
     }
 )";
 

src/gpu/glsl/GrGLSLFragmentProcessor.cpp

diff --git a/src/gpu/glsl/GrGLSLFragmentProcessor.cpp b/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
index fb70e45..7a0858c 100644
--- a/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
+++ b/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
@@ -113,12 +113,9 @@
         // Only check perspective for this specific matrix transform, not the aggregate FP property.
         // Any parent perspective will have already been applied when evaluated in the FS.
         if (childProc.sampleMatrix().fHasPerspective) {
-            SkString coords3 = args.fFragBuilder->newTmpVarName("coords3");
-            args.fFragBuilder->codeAppendf("float3 %s = (%s) * %s.xy1;\n",
-                                           coords3.c_str(), skslMatrix.c_str(), args.fSampleCoord);
-            return SkStringPrintf("%s(%s, %s.xy / %s.z)", fFunctionNames[childIndex].c_str(),
-                                  inputColor ? inputColor : "half4(1)", coords3.c_str(),
-                                  coords3.c_str());
+            return SkStringPrintf("%s(%s, proj((%s) * %s.xy1))", fFunctionNames[childIndex].c_str(),
+                                  inputColor ? inputColor : "half4(1)", skslMatrix.c_str(),
+                                  args.fSampleCoord);
         } else {
             return SkStringPrintf("%s(%s, ((%s) * %s.xy1).xy)",
                                   fFunctionNames[childIndex].c_str(),

src/sksl/sksl_blend.inc

diff --git a/src/sksl/sksl_blend.inc b/src/sksl/sksl_blend.inc
index ab6dcb8..4eaa73d 100644
--- a/src/sksl/sksl_blend.inc
+++ b/src/sksl/sksl_blend.inc
@@ -303,4 +303,6 @@
 // The max() guards against division by zero when the incoming color is transparent black
 half4 unpremul(half4 color) { return half4(color.rgb / max(color.a, 0.0001), color.a); }
 float4 unpremul_float(float4 color) { return float4(color.rgb / max(color.a, 0.0001), color.a); }
+
+float2 proj(float3 p) { return p.xy / p.z; }
 )SKSL"