PDF: support perspective in simple shaders. (this version does not work well with tilling)

src/pdf/SkPDFShader.cpp

Index: src/pdf/SkPDFShader.cpp
diff --git a/src/pdf/SkPDFShader.cpp b/src/pdf/SkPDFShader.cpp
index 70fb616a24f672cbf0b57603cf400defb69d0973..ba45d70dccde657700435176fc1ba6a0e6bd5536 100644
--- a/src/pdf/SkPDFShader.cpp
+++ b/src/pdf/SkPDFShader.cpp
@@ -210,16 +210,73 @@ static void tileModeCode(SkShader::TileMode mode, SkString* result) {
     }
 }
 

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

Add a comment and a better name.  apply_perspective_to_coordinates ?

[edisonn, 2013/10/18 16:10:13]

Done.

-static SkString linearCode(const SkShader::GradientInfo& info) {
-    SkString function("{pop\n");  // Just ditch the y value.
+static SkString transformCoordonates(const SkMatrix& xy2xy) {

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

nit: transform_coordinates  (hacker style and spelling)

[edisonn, 2013/10/18 16:10:13]

Done.

[edisonn, 2013/10/18 16:10:13]

Done.

+    SkString code;
+    if (!xy2xy.hasPerspective()) {
+        return code;
+    }
+
+    // Perspective matrix should be:
+    // 1   0  0
+    // 0   1  0
+    // p0 p1 p2
+
+    SkScalar p0 = xy2xy[SkMatrix::kMPersp0];
+    SkScalar p1 = xy2xy[SkMatrix::kMPersp1];
+    SkScalar p2 = xy2xy[SkMatrix::kMPersp2];
+
+    // y = y / (p2 + p0 x + p1 y)

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

SkMatrix.cpp:1201 suggests that this should be *, not / - maybe why you ended up
with the inverse perspective matrix?

[edisonn, 2013/10/18 16:10:13]

It is the inverse perspective matrix. I updated the name.

[edisonn, 2013/10/18 16:10:13]

it is the inverse perspective matrix

[vandebo (ex-Chrome), 2013/10/21 19:23:18]

Regardless of it being the inverse matrix or not, SkMatrix.cpp:1201 says that
the way to apply a matrix containing perspective is with multiplication, not
division.  Please help me understand why division is the right thing here.

[edisonn, 2013/10/23 19:45:17]

It is the inverse perspective, it moves the point from a a space with
perspective, in a space without the perspective

+    // x = x / (p2 + p0 x + p1 y)
+
+    code.append(" ");
+
+    // Input on stack: x y
+    code.append("dup ");                // x y y
+    code.appendScalar(p1);              // x y y p1
+    code.append(" mul ");               // x y y*p1

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

Consider the following formatting to be consistent with the rest of the file:
code.append("dup ");                // x y y
code.appendScalar(p1);              // x y y p1
code.append(" mul "                 // x y y*p1
            " 2 index ");           // x y y*p1 x
code.appendScalar(p0);              // x y y p1 x p0
code.append(" mul ");               // x y y*p1 x*p0
code.appendScalar(p2);              // x y y p1 x*p0 p2
code.append("add "                  // x y y*p1 x*p0+p2
            "add "                  // x y y*p1+x*p0+p2
            "1 index "              // x y y*p1+x*p0+p2 y
            "1 index "              // x y y*p1+x*p0+p2 y y*p1+x*p0+p2
            "div "                  // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2)
            "4 1 roll "             // y/(y*p1+x*p0+p2) x y y*p1+x*p0+p2
            "exch "                 // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 y
            "pop "                  // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2
            "div "                  // y/(y*p1+x*p0+p2) x/(y*p1+x*p0+p2)
            "exch\n");              // x/(y*p1+x*p0+p2) y/(y*p1+x*p0+p2)

[edisonn, 2013/10/18 16:10:13]

Done.

+    code.appendS32(2);                  // x y y*p1 2
+    code.append(" index ");             // x y y*p1 x
+    code.appendScalar(p0);              // x y y p1 x p0
+    code.append(" mul ");               // x y y*p1 x*p0
+    code.appendScalar(p2);              // x y y p1 x*p0 p2
+    code.append(" add ");               // x y y*p1 x*p0+p2

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

nit: extra space between adds (several more below)

[edisonn, 2013/10/18 16:10:13]

Done.

+    code.append(" add ");               // x y y*p1+x*p0+p2
+    code.appendS32(1);                  // x y y*p1+x*p0+p2 1

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

If you infact want mul and not div, consider this sequence, it's a couple
instructions shorter:

code.append("add "                  // x y y*p1 x*p0+p2
            "add "                  // x y z
            "3 1 roll "             // z x y
            "2 index "              // z x y z
            "mul "                  // z x y*z
            "3 1 roll "             // y*z z x
            "mul "                  // y*z z*x
            "exch\n");              // x*z y*z

[edisonn, 2013/10/18 16:10:13]

I need div

+    code.append(" index ");             // x y y*p1+x*p0+p2 y
+    code.appendS32(1);                  // x y y*p1+x*p0+p2 y 1
+    code.append(" index ");             // x y y*p1+x*p0+p2 y y*p1+x*p0+p2
+    code.append(" div ");               // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2)
+    code.appendS32(4);                  // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2) 4
+    code.append(" ");
+    code.appendS32(1);                  // x y y*p1+x*p0+p2 y/(y*p1+x*p0+p2) 4 1
+    code.append(" roll ");              // y/(y*p1+x*p0+p2) x y y*p1+x*p0+p2
+    code.append(" exch ");              // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2 y
+    code.append(" pop ");               // y/(y*p1+x*p0+p2) x y*p1+x*p0+p2
+    code.append(" div ");               // y/(y*p1+x*p0+p2) x/(y*p1+x*p0+p2)
+    code.append(" exch ");              // x/(y*p1+x*p0+p2) y/(y*p1+x*p0+p2)
+
+    return code;
+}
+
+static SkString linearCode(const SkShader::GradientInfo& info,
+                           const SkMatrix& perspectiveRemover) {
+    SkString function("{");
+
+    function.append(transformCoordonates(perspectiveRemover));
+
+    function.append("pop\n");  // Just ditch the y value.
     tileModeCode(info.fTileMode, &function);
     gradientFunctionCode(info, &function);
     function.append("}");
     return function;
 }
 
-static SkString radialCode(const SkShader::GradientInfo& info) {
+static SkString radialCode(const SkShader::GradientInfo& info,
+                           const SkMatrix& perspectiveRemover) {
     SkString function("{");
+
+    function.append(transformCoordonates(perspectiveRemover));
+
     // Find the distance from the origin.
     function.append("dup "      // x y y
                     "mul "      // x y^2
@@ -239,7 +296,8 @@ static SkString radialCode(const SkShader::GradientInfo& info) {
    with one simplification, the coordinate space has been scaled so that
    Dr = 1.  This means we don't need to scale the entire equation by 1/Dr^2.
  */
-static SkString twoPointRadialCode(const SkShader::GradientInfo& info) {
+static SkString twoPointRadialCode(const SkShader::GradientInfo& info,
+                                   const SkMatrix& perspectiveRemover) {
     SkScalar dx = info.fPoint[0].fX - info.fPoint[1].fX;
     SkScalar dy = info.fPoint[0].fY - info.fPoint[1].fY;
     SkScalar sr = info.fRadius[0];
@@ -249,6 +307,9 @@ static SkString twoPointRadialCode(const SkShader::GradientInfo& info) {
     // We start with a stack of (x y), copy it and then consume one copy in
     // order to calculate b and the other to calculate c.
     SkString function("{");
+
+    function.append(transformCoordonates(perspectiveRemover));
+
     function.append("2 copy ");
 
     // Calculate -b and b^2.
@@ -286,7 +347,8 @@ static SkString twoPointRadialCode(const SkShader::GradientInfo& info) {
 /* Conical gradient shader, based on the Canvas spec for radial gradients
    See: http://www.w3.org/TR/2dcontext/#dom-context-2d-createradialgradient
  */
-static SkString twoPointConicalCode(const SkShader::GradientInfo& info) {
+static SkString twoPointConicalCode(const SkShader::GradientInfo& info,
+                                    const SkMatrix& perspectiveRemover) {
     SkScalar dx = info.fPoint[1].fX - info.fPoint[0].fX;
     SkScalar dy = info.fPoint[1].fY - info.fPoint[0].fY;
     SkScalar r0 = info.fRadius[0];
@@ -300,6 +362,9 @@ static SkString twoPointConicalCode(const SkShader::GradientInfo& info) {
     // We start with a stack of (x y), copy it and then consume one copy in
     // order to calculate b and the other to calculate c.
     SkString function("{");
+
+    function.append(transformCoordonates(perspectiveRemover));
+
     function.append("2 copy ");
 
     // Calculate b and b^2; b = -2 * (y * dy + x * dx + r0 * dr).
@@ -395,7 +460,8 @@ static SkString twoPointConicalCode(const SkShader::GradientInfo& info) {
     return function;
 }
 
-static SkString sweepCode(const SkShader::GradientInfo& info) {
+static SkString sweepCode(const SkShader::GradientInfo& info,
+                          const SkMatrix& perspectiveRemover) {
     SkString function("{exch atan 360 div\n");
     tileModeCode(info.fTileMode, &function);
     gradientFunctionCode(info, &function);
@@ -725,10 +791,49 @@ SkPDFAlphaFunctionShader::SkPDFAlphaFunctionShader(SkPDFShader::State* state)
                                  SkMatrix::I());
 }
 
+// Finds simple and persp such that in = simple * persp.
+// but it returns the inverse of perspective matrix.
+static bool splitPerspective(const SkMatrix in, SkMatrix* simple,

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

nit: split_perspective

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

nit: simple -> affine (also update comment)

[edisonn, 2013/10/18 16:10:13]

Done.

[edisonn, 2013/10/18 16:10:13]

Done.

+                             SkMatrix* perspectiveInverse) {
+    SkScalar p2 = in[SkMatrix::kMPersp2];
+
+    if (SkScalarNearlyZero(p2)) {
+        return false;
+    }
+
+    SkScalar zero = SkIntToScalar(0);

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

nit: make these all const ?

[edisonn, 2013/10/18 16:10:13]

Done.

+    SkScalar one = SkIntToScalar(1);
+
+    SkScalar sx = in[SkMatrix::kMScaleX];
+    SkScalar kx = in[SkMatrix::kMSkewX];
+    SkScalar tx = in[SkMatrix::kMTransX];
+    SkScalar ky = in[SkMatrix::kMSkewY];
+    SkScalar sy = in[SkMatrix::kMScaleY];
+    SkScalar ty = in[SkMatrix::kMTransY];
+    SkScalar p0 = in[SkMatrix::kMPersp0];
+    SkScalar p1 = in[SkMatrix::kMPersp1];
+
+    // Perspective matrix would be:
+    // 1  0  0
+    // 0  1  0
+    // p0 p1 p2
+    // But we need the inverse of persp.
+    perspectiveInverse->setAll(one,          zero,       zero,
+                               zero,         one,        zero,
+                               -p0 / p2,     -p1/p2,     1/p2);
+
+    simple->setAll(sx - p0 * tx / p2,       kx - p1 * tx / p2,      tx / p2,

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

Isn't this simple->SetConcat(in, *perspectiveInverse) ?

[edisonn, 2013/10/18 16:10:13]

I would prefer not to add up errors in the matrix. I would rather use the exact
formulas.

+                   ky - p0 * ty / p2,       sy - p1 * ty / p2,      ty / p2,
+                   zero,                    zero,                   one);
+
+    return true;
+}
+
 SkPDFFunctionShader::SkPDFFunctionShader(SkPDFShader::State* state)
         : SkPDFDict("Pattern"),
           fState(state) {
-    SkString (*codeFunction)(const SkShader::GradientInfo& info) = NULL;
+    SkString (*codeFunction)(const SkShader::GradientInfo& info,
+                             const SkMatrix& perspectiveRemover) = NULL;
     SkPoint transformPoints[2];
 
     // Depending on the type of the gradient, we want to transform the
@@ -780,8 +885,22 @@ SkPDFFunctionShader::SkPDFFunctionShader(SkPDFShader::State* state)
     // the gradient can be drawn on on the unit segment.
     SkMatrix mapperMatrix;
     unitToPointsMatrix(transformPoints, &mapperMatrix);
+
+    SkMatrix perspectiveInverseOnly = SkMatrix::I();
+
     SkMatrix finalMatrix = fState.get()->fCanvasTransform;
     finalMatrix.preConcat(fState.get()->fShaderTransform);
+
+    // Preserves as much as posible in the final matrix, and only removes
+    // the perspective, stored in a second matrix which has only
+    // 3 numbers (p0, p1, p2)so the shader will handle it eficiently.

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

This comment isn't consistent with the code (perspective vs perspective inverse)

[edisonn, 2013/10/18 16:10:13]

Done.

+    if (finalMatrix.hasPerspective()) {
+        if (!splitPerspective(finalMatrix,
+                              &finalMatrix, &perspectiveInverseOnly)) {
+            return;
+        }
+    }
+
     finalMatrix.preConcat(mapperMatrix);
 
     SkRect bbox;
@@ -812,9 +931,9 @@ SkPDFFunctionShader::SkPDFFunctionShader(SkPDFShader::State* state)
             inverseMapperMatrix.mapRadius(info->fRadius[0]);
         twoPointRadialInfo.fRadius[1] =
             inverseMapperMatrix.mapRadius(info->fRadius[1]);
-        functionCode = codeFunction(twoPointRadialInfo);
+        functionCode = codeFunction(twoPointRadialInfo, perspectiveInverseOnly);

[vandebo (ex-Chrome), 2013/10/17 18:19:36]

I often get it wrong, so please correct me.  Don't we want to pass the
perspective matrix down (not the inverse)?  The shader matrix doesn't contain
any perspective, so it still needs to be applied...

[edisonn, 2013/10/18 16:10:13]

First, the two point radial gm looks fine to me.
Second, I think not. I want the inverse of the perspective, because when I apply
it the point is in perspective space, so with the inverse perspective I move the
point in the space without perspective where our shader code works fine.

[vandebo (ex-Chrome), 2013/10/21 19:23:18]

Using the forward perspective matrix here and switching to multiply in
apply_perspective_to_coordinates() seems to give the same results as what you
have.

In the non-perspective case, the shader coordinates (those going through the ps
function) are in a unit gradient space.  The shader
 matrix transforms them to the canvas space.  The shader matrix first goes from
the unit gradient space to the specified gradient space, then through the shader
transform, and finally uses the canvas matrix to get to the canvas coordinate
space.

It's similar in the perspective space.  The ps function operates in the unit
shader coordinate space, you then manually apply the perspective transform and
finally finish the transform to the canvas space.

[edisonn, 2013/10/23 19:45:17]

updated the code, the perspective is removed from the final matrix after
applying the mapper matrix

[vandebo (ex-Chrome), 2013/10/23 22:42:21]

I don't understand why my argument is wrong, but the code seems to work, so I
guess I am.

     } else {
-        functionCode = codeFunction(*info);
+        functionCode = codeFunction(*info, perspectiveInverseOnly);
     }
 
     SkAutoTUnref<SkPDFDict> pdfShader(new SkPDFDict);