Added check for ill-conditioned invert

Added check for ill-conditioned invert

sk_inv_determinant has a guard that the determinant can't get too big so this CL only checks if the determinant gets too small.

BUG=492263
Committed: https://skia.googlesource.com/skia/+/20eee3f047f56b7715b311313b2998daaaf08a96

[robertphillips, 2015-06-17 13:56:35]

robertphillips@google.com changed reviewers:
+ reed@google.com

[robertphillips, 2015-06-17 19:06:45]

PTAL

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

https://codereview.chromium.org/1188433011/diff/20001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/1188433011/diff/20001/include/core/SkMatrix.h...
include/core/SkMatrix.h:726: static void ComputeInv(const SkScalar* src,
SkScalar* dst, SkScalar scale, bool isPersp);
nit:
- skia tries to use [] for parameters that are many...
- document scale as 1/determinant ?
- 1 dst and lots of src params --> (dst, src0, src1, src2)

... ComputeInv(SkScaar dst[], const SkScalar src[], SkScalar invDet, bool
isPersp);

https://codereview.chromium.org/1188433011/diff/20001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/1188433011/diff/20001/src/core/SkMatrix.cpp#n...
src/core/SkMatrix.cpp:894: *(SkMatrix*)this = storage; // need to copy answer
back
why the cast? why not 

if (applyingInPlace) {
    *inv = storage;
}

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

https://codereview.chromium.org/1188433011/diff/20001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.chromium.org/1188433011/diff/20001/include/core/SkMatrix.h...
include/core/SkMatrix.h:726: static void ComputeInv(const SkScalar* src,
SkScalar* dst, SkScalar scale, bool isPersp);
On 2015/06/17 19:27:56, reed1 wrote:
> nit:
> - skia tries to use [] for parameters that are many...
> - document scale as 1/determinant ?
> - 1 dst and lots of src params --> (dst, src0, src1, src2)
> 
> ... ComputeInv(SkScaar dst[], const SkScalar src[], SkScalar invDet, bool
> isPersp);

Done.

https://codereview.chromium.org/1188433011/diff/20001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/1188433011/diff/20001/src/core/SkMatrix.cpp#n...
src/core/SkMatrix.cpp:894: *(SkMatrix*)this = storage; // need to copy answer
back
On 2015/06/17 19:27:56, reed1 wrote:
> why the cast? why not 
> 
> if (applyingInPlace) {
>     *inv = storage;
> }

Done. It was left over from the prior code.

[reed1, 2015-06-17 19:49:29]

I'm not wild about adding SkScalarSmallest. Can we skip adding that, and just
references FLOAT_MIN at the call-site?

https://codereview.chromium.org/1188433011/diff/40001/src/core/SkMatrix.cpp
File src/core/SkMatrix.cpp (right):

https://codereview.chromium.org/1188433011/diff/40001/src/core/SkMatrix.cpp#n...
src/core/SkMatrix.cpp:871: bool checkInv = abs(invDet) <= SK_ScalarSmallest;
seems like we explicitly mean smallest-float, not smallest scalar, right?

[reed1, 2015-06-17 19:50:50]

actually, I think a custom const is even clearer...

// Need a limit to avoid creating non-invertible / non-finite matrices.
// for now use smallest float as the threshold
const float kSmallestReasonableScale = FLOAT_MIN;

[robertphillips, 2015-06-17 20:04:24]

PTAL. Now with a more consistent behavior (always check for a finite answer).

[robertphillips, 2015-06-18 15:25:35]

ping

[reed1, 2015-06-18 20:39:28]

nice.
lgtm

[robertphillips, 2015-06-19 11:49:28]

The CQ bit was checked by robertphillips@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-19 11:49:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188433011/60001

[commit-bot: I haz the power, 2015-06-19 12:01:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 12:01:11]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2015-06-19 12:13:41]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2015-06-19 12:13:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188433011/60001

[commit-bot: I haz the power, 2015-06-19 12:14:29]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/20eee3f047f56b7715b311313b2998daaaf08a96