Added check for ill-conditioned invert

src/core/SkMatrix.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkMatrix.h"
 #include "SkFloatBits.h"
 #include "SkString.h"
@@ skipping 734 matching lines @@
 
     // Since the determinant is on the order of the cube of the matrix members,
     // compare to the cube of the default nearly-zero constant (although an
     // estimate of the condition number would be better if it wasn't so expensive).
     if (SkScalarNearlyZero((float)det, SK_ScalarNearlyZero * SK_ScalarNearlyZero * SK_ScalarNearlyZero)) {
         return 0;
     }
     return 1.0 / det;
 }
 
+bool SkMatrix::isFinite() const {
+    for (int i = 0; i < 9; ++i) {
+        if (!SkScalarIsFinite(fMat[i])) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 void SkMatrix::SetAffineIdentity(SkScalar affine[6]) {
     affine[kAScaleX] = 1;
     affine[kASkewY] = 0;
     affine[kASkewX] = 0;
     affine[kAScaleY] = 1;
     affine[kATransX] = 0;
     affine[kATransY] = 0;
 }
 
 bool SkMatrix::asAffine(SkScalar affine[6]) const {
     if (this->hasPerspective()) {
         return false;
     }
     if (affine) {
         affine[kAScaleX] = this->fMat[kMScaleX];
         affine[kASkewY] = this->fMat[kMSkewY];
         affine[kASkewX] = this->fMat[kMSkewX];
         affine[kAScaleY] = this->fMat[kMScaleY];
         affine[kATransX] = this->fMat[kMTransX];
         affine[kATransY] = this->fMat[kMTransY];
     }
     return true;
 }
 
+void SkMatrix::ComputeInv(const SkScalar* src, SkScalar* dst, SkScalar scale, bool isPersp) {
+    SkASSERT(src != dst);
+    SkASSERT(src && dst);
+
+    if (isPersp) {
+        dst[kMScaleX] = scross_dscale(src[kMScaleY], src[kMPersp2], src[kMTransY], src[kMPersp1], scale);
+        dst[kMSkewX]  = scross_dscale(src[kMTransX], src[kMPersp1], src[kMSkewX],  src[kMPersp2], scale);
+        dst[kMTransX] = scross_dscale(src[kMSkewX],  src[kMTransY], src[kMTransX], src[kMScaleY], scale);
+
+        dst[kMSkewY]  = scross_dscale(src[kMTransY], src[kMPersp0], src[kMSkewY],  src[kMPersp2], scale);
+        dst[kMScaleY] = scross_dscale(src[kMScaleX], src[kMPersp2], src[kMTransX], src[kMPersp0], scale);
+        dst[kMTransY] = scross_dscale(src[kMTransX], src[kMSkewY],  src[kMScaleX], src[kMTransY], scale);
+
+        dst[kMPersp0] = scross_dscale(src[kMSkewY],  src[kMPersp1], src[kMScaleY], src[kMPersp0], scale);
+        dst[kMPersp1] = scross_dscale(src[kMSkewX],  src[kMPersp0], src[kMScaleX], src[kMPersp1], scale);
+        dst[kMPersp2] = scross_dscale(src[kMScaleX], src[kMScaleY], src[kMSkewX],  src[kMSkewY],  scale);
+    } else {   // not perspective
+        dst[kMScaleX] = SkDoubleToScalar(src[kMScaleY] * scale);
+        dst[kMSkewX]  = SkDoubleToScalar(-src[kMSkewX] * scale);
+        dst[kMTransX] = dcross_dscale(src[kMSkewX], src[kMTransY], src[kMScaleY], src[kMTransX], scale);
+
+        dst[kMSkewY]  = SkDoubleToScalar(-src[kMSkewY] * scale);
+        dst[kMScaleY] = SkDoubleToScalar(src[kMScaleX] * scale);
+        dst[kMTransY] = dcross_dscale(src[kMSkewY], src[kMTransX], src[kMScaleX], src[kMTransY], scale);
+
+        dst[kMPersp0] = 0;
+        dst[kMPersp1] = 0;
+        dst[kMPersp2] = 1;
+    }
+}
+
 bool SkMatrix::invertNonIdentity(SkMatrix* inv) const {
     SkASSERT(!this->isIdentity());
 
     TypeMask mask = this->getType();
 
     if (0 == (mask & ~(kScale_Mask | kTranslate_Mask))) {
         bool invertible = true;
         if (inv) {
             if (mask & kScale_Mask) {
                 SkScalar invX = fMat[kMScaleX];
@@ skipping 29 matching lines @@
         return invertible;
     }
 
     int    isPersp = mask & kPerspective_Mask;
     double scale = sk_inv_determinant(fMat, isPersp);
 
     if (scale == 0) { // underflow
         return false;
     }
 
-    if (inv) {
-        SkMatrix tmp;
-        if (inv == this) {
-            inv = &tmp;
-        }
+    // If the scale is close to the float min check the validity of the 
+    // inverse result regardless of if the actual result is requested
+    bool checkInv = abs(scale) <= SK_ScalarSmallest;
 
-        if (isPersp) {
-            inv->fMat[kMScaleX] = scross_dscale(fMat[kMScaleY], fMat[kMPersp2], fMat[kMTransY], fMat[kMPersp1], scale);
-            inv->fMat[kMSkewX]  = scross_dscale(fMat[kMTransX], fMat[kMPersp1], fMat[kMSkewX],  fMat[kMPersp2], scale);
-            inv->fMat[kMTransX] = scross_dscale(fMat[kMSkewX],  fMat[kMTransY], fMat[kMTransX], fMat[kMScaleY], scale);
+    if (!inv && !checkInv) {
+        return true;        // no reason to actually compute the inverse
+    }
 
-            inv->fMat[kMSkewY]  = scross_dscale(fMat[kMTransY], fMat[kMPersp0], fMat[kMSkewY],  fMat[kMPersp2], scale);
-            inv->fMat[kMScaleY] = scross_dscale(fMat[kMScaleX], fMat[kMPersp2], fMat[kMTransX], fMat[kMPersp0], scale);
-            inv->fMat[kMTransY] = scross_dscale(fMat[kMTransX], fMat[kMSkewY],  fMat[kMScaleX], fMat[kMTransY], scale);
+    bool applyingInPlace = (inv == this);
 
-            inv->fMat[kMPersp0] = scross_dscale(fMat[kMSkewY],  fMat[kMPersp1], fMat[kMScaleY], fMat[kMPersp0], scale);
-            inv->fMat[kMPersp1] = scross_dscale(fMat[kMSkewX],  fMat[kMPersp0], fMat[kMScaleX], fMat[kMPersp1], scale);
-            inv->fMat[kMPersp2] = scross_dscale(fMat[kMScaleX], fMat[kMScaleY], fMat[kMSkewX],  fMat[kMSkewY],  scale);
-        } else {   // not perspective
-            inv->fMat[kMScaleX] = SkDoubleToScalar(fMat[kMScaleY] * scale);
-            inv->fMat[kMSkewX]  = SkDoubleToScalar(-fMat[kMSkewX] * scale);
-            inv->fMat[kMTransX] = dcross_dscale(fMat[kMSkewX], fMat[kMTransY], fMat[kMScaleY], fMat[kMTransX], scale);
+    SkMatrix* tmp = inv;
 
-            inv->fMat[kMSkewY]  = SkDoubleToScalar(-fMat[kMSkewY] * scale);
-            inv->fMat[kMScaleY] = SkDoubleToScalar(fMat[kMScaleX] * scale);
-            inv->fMat[kMTransY] = dcross_dscale(fMat[kMSkewY], fMat[kMTransX], fMat[kMScaleX], fMat[kMTransY], scale);
+    SkMatrix storage;
+    if (applyingInPlace || NULL == tmp) {
+        tmp = &storage;     // we either need to avoid trampling memory or have no memory
+    }
 
-            inv->fMat[kMPersp0] = 0;
-            inv->fMat[kMPersp1] = 0;
-            inv->fMat[kMPersp2] = 1;
-        }
+    ComputeInv(fMat, tmp->fMat, scale, isPersp);
+    if (!tmp->isFinite()) {
+        return false;
+    }
 
-        inv->setTypeMask(fTypeMask);
+    tmp->setTypeMask(fTypeMask);
 
-        if (inv == &tmp) {
-            *(SkMatrix*)this = tmp;
-        }
+    if (applyingInPlace) {
+        *(SkMatrix*)this = storage; // need to copy answer back

[reed1, 2015/06/17 19:27:56]

why the cast? why not 

if (applyingInPlace) {
    *inv = storage;
}

[robertphillips, 2015/06/17 19:44:46]

Done. It was left over from the prior code.

     }
+
     return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkMatrix::Identity_pts(const SkMatrix& m, SkPoint dst[], const SkPoint src[], int count) {
     SkASSERT(m.getType() == 0);
 
     if (dst != src && count > 0) {
         memcpy(dst, src, count * sizeof(SkPoint));
@@ skipping 917 matching lines @@
         rotation1->fX = cos1;
         rotation1->fY = sin1;
     }
     if (rotation2) {
         rotation2->fX = cos2;
         rotation2->fY = sin2;
     }
 
     return true;
 }