Fix infinite matrix during an invert.

Fix infinite matrix during an invert.

This patch ensures that when inverting a SkMatrix44, we handle small
floats properly. When inverted these can cause infinite values, but
still evaluate to true in an if condition.

BUG=chromium:498516
Committed: https://skia.googlesource.com/skia/+/a8d4559fd6fb4f8ed213e201bb738dbc7b3c3512

[vmpstr, 2015-06-24 20:31:05]

vmpstr@chromium.org changed reviewers:
+ bsalomon@google.com, reed@google.com

[vmpstr, 2015-06-24 20:31:06]

Please take a look.

[reed1, 2015-06-24 21:30:51]

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

[reed1, 2015-06-24 21:30:52]

We faced a similar bug in SkMatrix, but decided to solve it by checking the
result of the inversion, since even finite scaling values can produce a
non-finite result matrix.

+robert who worked on that.

[vmpstr, 2015-06-25 21:48:14]

On 2015/06/24 21:30:52, reed1 wrote:
> We faced a similar bug in SkMatrix, but decided to solve it by checking the
> result of the inversion, since even finite scaling values can produce a
> non-finite result matrix.
> 
> +robert who worked on that.

robertphillips, can you take a look?

Looking at SkMatrix.cpp, it seems that the scale and translate matrices don't
have a final matrix check, it's just the generic invert that does.

[vmpstr, 2015-06-29 17:28:17]

ping

[reed1, 2015-06-29 21:47:29]

We should expand our test to all of the inverse cases, but either way, the
SkMatrix technique is safer: it checks the resulting matrix rather than just the
computed scale factors, since the scale factors can be finite but still result
in a non-finite result.

Also there should be some unitests to exercise your new check.

[vmpstr, 2015-06-29 21:56:52]

On 2015/06/29 21:47:29, reed1 wrote:
> We should expand our test to all of the inverse cases, but either way, the
> SkMatrix technique is safer: it checks the resulting matrix rather than just
the
> computed scale factors, since the scale factors can be finite but still result
> in a non-finite result.

Do you mean that I should keep this particular check as is, and instead add a
final run through the matrix? It's a bit awkward that without a resulting matrix
we would return "true" here, but if we want the result, we'd return "false",
because of the final check at the end. Or do you think that's fine?

> 
> Also there should be some unitests to exercise your new check.

Ok, I will add these.

[reed1, 2015-06-29 22:02:43]

On 2015/06/29 21:56:52, vmpstr wrote:
> On 2015/06/29 21:47:29, reed1 wrote:
> > We should expand our test to all of the inverse cases, but either way, the
> > SkMatrix technique is safer: it checks the resulting matrix rather than just
> the
> > computed scale factors, since the scale factors can be finite but still
result
> > in a non-finite result.
> 
> Do you mean that I should keep this particular check as is, and instead add a
> final run through the matrix? It's a bit awkward that without a resulting
matrix
> we would return "true" here, but if we want the result, we'd return "false",
> because of the final check at the end. Or do you think that's fine?

Indeed, that is why SkMatrix will compute the result even if the caller didn't
ask for it, otherwise it can't return a consistent answer to the
finite-question.

> 
> > 
> > Also there should be some unitests to exercise your new check.
> 
> Ok, I will add these.

[vmpstr, 2015-06-30 00:00:28]

PTAL

[reed1, 2015-06-30 18:24:10]

thanks, that looks very complete from a functional p.o.v.

I think we are now introducing an extra copy in the case where the caller
provided space for the answer. Perhaps that doesn't matter, but in general these
routines are perf-sensitive.

SkMatrix is a little more complicated, but it separates out the predicate
function isFinite() from the decision to copy or not. Thus it can only copy if
the tmp storage was actually needed.

[vmpstr, 2015-06-30 18:34:10]

On 2015/06/30 18:24:10, reed1 wrote:
> thanks, that looks very complete from a functional p.o.v.
> 
> I think we are now introducing an extra copy in the case where the caller
> provided space for the answer. Perhaps that doesn't matter, but in general
these
> routines are perf-sensitive.
> 
> SkMatrix is a little more complicated, but it separates out the predicate
> function isFinite() from the decision to copy or not. Thus it can only copy if
> the tmp storage was actually needed.

Yeah I thought about that... If the storage was provided though, then we don't
necessarily want to override the values unless we're sure that the matrix is
finite right?

Or is there an understanding that if the matrix is not invertible, then the
resulting matrix is not defined?

[reed1, 2015-06-30 18:43:59]

Very good point!

I think we should consider changing the contract on these sorts of calls, from
will-be-unchanged/igored -> will-be-undefined.

Now, can we do that today for SkMatrix44? I guess we need to survey current
callers (chrome) and find out.

[vmpstr, 2015-06-30 19:55:09]

PTAL

[reed1, 2015-06-30 20:15:05]

very nice. lgtm

btw -- the is_finite can be much faster like this:

SkMScalar accumulator = 0;
for (row ...)
   for (col ...)
      accumulator *= mat[row][col];

return accumulator == 0;

[reed1, 2015-06-30 20:15:44]

On 2015/06/30 20:15:05, reed1 wrote:
> very nice. lgtm
> 
> btw -- the is_finite can be much faster like this:
> 
> SkMScalar accumulator = 0;
> for (row ...)
>    for (col ...)
>       accumulator *= mat[row][col];
> 
> return accumulator == 0;

I concede that SkMatrix should do the same speedup :)

[vmpstr, 2015-06-30 20:28:16]

On 2015/06/30 20:15:05, reed1 wrote:
> very nice. lgtm
> 
> btw -- the is_finite can be much faster like this:
> 
> SkMScalar accumulator = 0;
> for (row ...)
>    for (col ...)
>       accumulator *= mat[row][col];
> 
> return accumulator == 0;

Done. Thanks!

[vmpstr, 2015-06-30 20:28:25]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-30 20:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1209763002/#ps80001
(title: "update")

[commit-bot: I haz the power, 2015-06-30 20:28:47]

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

[commit-bot: I haz the power, 2015-06-30 20:36:10]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/a8d4559fd6fb4f8ed213e201bb738dbc7b3c3512