Expand testing of SkMatrix::decomposeScale

Bug: skia:7211
Change-Id: If03ad1d364b33e174d91010ca250cd6c2d2f67c2
Reviewed-on: https://skia-review.googlesource.com/140185
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>

4 files changed, 65 insertions, 16 deletions

diff --git a/src/core/SkMatrix.cpp b/src/core/SkMatrix.cpp
index 3d50659e08..44e9a566b4 100644
--- a/src/core/SkMatrix.cpp
+++ b/src/core/SkMatrix.cpp
@@ -1638,6 +1638,7 @@ bool SkMatrix::decomposeScale(SkSize* scale, SkMatrix* remaining) const {
     }
     if (remaining) {
         *remaining = *this;
+        // As per skbug.com/7211, this should actually be preScale
         remaining->postScale(SkScalarInvert(sx), SkScalarInvert(sy));
     }
     return true;
diff --git a/src/effects/imagefilters/SkXfermodeImageFilter.cpp b/src/effects/imagefilters/SkXfermodeImageFilter.cpp
index fef28f1d57..cc5cacb71c 100644
--- a/src/effects/imagefilters/SkXfermodeImageFilter.cpp
+++ b/src/effects/imagefilters/SkXfermodeImageFilter.cpp
@@ -112,8 +112,8 @@ void SkXfermodeImageFilter_Base::flatten(SkWriteBuffer& buffer) const {
 }
 
 sk_sp<SkSpecialImage> SkXfermodeImageFilter_Base::onFilterImage(SkSpecialImage* source,
-                                                           const Context& ctx,
-                                                           SkIPoint* offset) const {
+                                                                const Context& ctx,
+                                                                SkIPoint* offset) const {
     SkIPoint backgroundOffset = SkIPoint::Make(0, 0);
     sk_sp<SkSpecialImage> background(this->filterInput(0, source, ctx, &backgroundOffset));
 
@@ -129,7 +129,7 @@ sk_sp<SkSpecialImage> SkXfermodeImageFilter_Base::onFilterImage(SkSpecialImage*
     SkIRect srcBounds = SkIRect::EmptyIRect();
     if (background) {
         srcBounds = SkIRect::MakeXYWH(backgroundOffset.x(), backgroundOffset.y(),
-                                       background->width(), background->height());
+                                      background->width(), background->height());
     }
 
     srcBounds.join(foregroundBounds);
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index ffe237f803..f031e5edf1 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -1058,23 +1058,26 @@ void SkGpuDevice::drawSprite(const SkBitmap& bitmap,
 }
 
 
-void SkGpuDevice::drawSpecial(SkSpecialImage* special1, int left, int top, const SkPaint& paint,
+void SkGpuDevice::drawSpecial(SkSpecialImage* special, int left, int top, const SkPaint& paint,
                               SkImage* clipImage, const SkMatrix& clipMatrix) {
     ASSERT_SINGLE_OWNER
     GR_CREATE_TRACE_MARKER_CONTEXT("SkGpuDevice", "drawSpecial", fContext.get());
 
     // TODO: clipImage support.
 
-    SkIPoint offset = { 0, 0 };
-
     sk_sp<SkSpecialImage> result;
     if (paint.getImageFilter()) {
-        result = this->filterTexture(special1, left, top, &offset, paint.getImageFilter());
+        SkIPoint offset = { 0, 0 };
+
+        result = this->filterTexture(special, left, top, &offset, paint.getImageFilter());
         if (!result) {
             return;
         }
+
+        left += offset.fX;
+        top += offset.fY;
     } else {
-        result = sk_ref_sp(special1);
+        result = sk_ref_sp(special);
     }
 
     SkASSERT(result->isTextureBacked());
@@ -1088,7 +1091,7 @@ void SkGpuDevice::drawSpecial(SkSpecialImage* special1, int left, int top, const
     SkPaint tmpUnfiltered(paint);
     if (tmpUnfiltered.getMaskFilter()) {
         SkMatrix ctm = this->ctm();
-        ctm.postTranslate(-SkIntToScalar(left + offset.fX), -SkIntToScalar(top + offset.fY));
+        ctm.postTranslate(-SkIntToScalar(left), -SkIntToScalar(top));
         tmpUnfiltered.setMaskFilter(tmpUnfiltered.getMaskFilter()->makeWithMatrix(ctm));
     }
 
@@ -1116,8 +1119,7 @@ void SkGpuDevice::drawSpecial(SkSpecialImage* special1, int left, int top, const
             std::move(grPaint),
             GrAA(tmpUnfiltered.isAntiAlias()),
             SkMatrix::I(),
-            SkRect::Make(SkIRect::MakeXYWH(left + offset.fX, top + offset.fY, subset.width(),
-                                           subset.height())),
+            SkRect::Make(SkIRect::MakeXYWH(left, top, subset.width(), subset.height())),
             SkRect::Make(subset));
 }
 
diff --git a/tests/MatrixTest.cpp b/tests/MatrixTest.cpp
index 03661dc437..fccbf6d8f5 100644
--- a/tests/MatrixTest.cpp
+++ b/tests/MatrixTest.cpp
@@ -20,7 +20,7 @@ static bool nearly_equal_scalar(SkScalar a, SkScalar b) {
 static bool nearly_equal(const SkMatrix& a, const SkMatrix& b) {
     for (int i = 0; i < 9; i++) {
         if (!nearly_equal_scalar(a[i], b[i])) {
-            SkDebugf("not equal %g %g\n", (float)a[i], (float)b[i]);
+            SkDebugf("matrices not equal [%d] %g %g\n", i, (float)a[i], (float)b[i]);
             return false;
         }
     }
@@ -804,18 +804,56 @@ static void test_matrix_homogeneous(skiatest::Reporter* reporter) {
 
 }
 
-static bool check_decompScale(const SkMatrix& matrix) {
+static bool check_decompScale(const SkMatrix& original) {
     SkSize scale;
     SkMatrix remaining;
 
-    if (!matrix.decomposeScale(&scale, &remaining)) {
+    if (!original.decomposeScale(&scale, &remaining)) {
         return false;
     }
     if (scale.width() <= 0 || scale.height() <= 0) {
         return false;
     }
-    remaining.preScale(scale.width(), scale.height());
-    return nearly_equal(matrix, remaining);
+
+    // First ensure that the decomposition reconstitutes back to the original
+    {
+        SkMatrix reconstituted = remaining;
+
+        // This should be 'preScale' but, due to skbug.com/7211, it is reversed!
+        reconstituted.postScale(scale.width(), scale.height());
+        if (!nearly_equal(original, reconstituted)) {
+            return false;
+        }
+    }
+
+    // Then push some points through both paths and make sure they are the same.
+    static const int kNumPoints = 5;
+    const SkPoint testPts[kNumPoints] = {
+        {  0.0f,  0.0f },
+        {  1.0f,  1.0f },
+        {  1.0f,  0.5f },
+        { -1.0f, -0.5f },
+        { -1.0f,  2.0f }
+    };
+
+    SkPoint v1[kNumPoints];
+    original.mapPoints(v1, testPts, kNumPoints);
+
+    SkPoint v2[kNumPoints];
+    SkMatrix scaleMat = SkMatrix::MakeScale(scale.width(), scale.height());
+
+    // Note, we intend the decomposition to be applied in the order scale and then remainder but,
+    // due to skbug.com/7211, the order is reversed!
+    remaining.mapPoints(v2, testPts, kNumPoints);
+    scaleMat.mapPoints(v2, kNumPoints);
+
+    for (int i = 0; i < kNumPoints; ++i) {
+        if (!SkPointPriv::EqualsWithinTolerance(v1[i], v2[i], 0.00001f)) {
+            return false;
+        }
+    }
+
+    return true;
 }
 
 static void test_decompScale(skiatest::Reporter* reporter) {
@@ -830,6 +868,14 @@ static void test_decompScale(skiatest::Reporter* reporter) {
 
     m.setScale(1, 0);
     REPORTER_ASSERT(reporter, !check_decompScale(m));
+
+    m.setRotate(35, 0, 0);
+    m.preScale(2, 3);
+    REPORTER_ASSERT(reporter, check_decompScale(m));
+
+    m.setRotate(35, 0, 0);
+    m.postScale(2, 3);
+    REPORTER_ASSERT(reporter, check_decompScale(m));
 }
 
 DEF_TEST(Matrix, reporter) {