Cr46591 Proposed Patch

BUG=46591

This CL modifies skia/src/SkMatrix.cpp to update a constant used to determine if it is safe to invert a matrix or not. The existing value (SkScalarNearlyZero^2) was failing with Coats_of_arms_of_Argentina.svg due to an unusually large range of coordinate values and scale factors. The previous comment in the code justifies ^2 since the determinant is "on the order of the square of the matrix members", but this should really be ^3 for a 3x3 determinant. If a comparably  inexpensive estimate of the matrix condition number can be found that would be even more appropriate, but no such thing exists to my knowledge.

[wjmaclean, 2010-07-19 16:27:49]

A proposed solution to Cr 46591.

[brettw, 2010-07-19 17:27:26]

Don't forget to write a CL description and use BUG= in the description (it will
auto-linkify it). I also don't see any reasoning in the comments or in the CL
description about why you think this is the right solution. Since I don't
actually have any background about what the problem is, it's impossible for me
to judge this CL.

[wjmaclean, 2010-07-19 17:33:27]

Brett -

This is my first Skia submission. Is there some file I'm supposed to modify
for the CL description, otherwise where should it go? I assumed you had seen
my comments regarding Chromium issue 46591 on the Chromium issue tracker,
hence the paucity of comments.

J

On Mon, Jul 19, 2010 at 1:27 PM, <brettw@chromium.org> wrote:

> Don't forget to write a CL description and use BUG= in the description
> (it will auto-linkify it). I also don't see any reasoning in the
> comments or in the CL description about why you think this is the right
> solution. Since I don't actually have any background about what the
> problem is, it's impossible for me to judge this CL.
>
>
> http://codereview.appspot.com/1731053/show
>

[brettw, 2010-07-19 17:39:58]

I see, then it looks like you should update the comment in the code you think is
wrong to explain why ^3 is correct.

[wjmaclean, 2010-07-19 17:53:30]

Revised issue to improve CL description and revise comments in SkMatrix.cpp.

[wjmaclean, 2010-07-19 17:54:23]

Sorry about that ... fixed :-)

J

On Mon, Jul 19, 2010 at 1:39 PM, <brettw@chromium.org> wrote:

> I see, then it looks like you should update the comment in the code you
> think is wrong to explain why ^3 is correct.
>
>
> http://codereview.appspot.com/1731053/show
>

[brettw, 2010-07-19 23:51:43]

LGTM, be sure to get Stephen's opinion before checking in.

[Stephen White, 2010-07-20 14:47:02]

LGTM (I'm trusting you on the math..)

http://codereview.appspot.com/1731053/diff/8001/9001
File src/core/SkMatrix.cpp (right):

http://codereview.appspot.com/1731053/diff/8001/9001#newcode657
src/core/SkMatrix.cpp:657: // compare to the square of the default nearly-zero
constant (although an
square -> cube here too?

[wjmaclean, 2010-07-20 15:00:24]

Thanks for noticing that other 'square' - I've uploaded a new patch that fixes
that.

Stephen, do you have committer status, as I don't?

Cheers,

James

[Stephen White, 2010-07-20 15:20:36]

On Tue, Jul 20, 2010 at 11:00 AM, <wjmaclean@chromium.org> wrote:

> Thanks for noticing that other 'square' - I've uploaded a new patch that
> fixes that.
>
> Stephen, do you have committer status, as I don't?
>

Landed as r589.

Stephen


> Cheers,
>
> James
>
>
> http://codereview.appspot.com/1731053/show
>

[reed, 2010-07-22 16:55:48]

^2 is sort of correct for a non-perspective matix (which is all we should see in
SVG), but it was all a hack/heuristic anyway to know how small we can accept
before we divide.

On 2010/07/20 15:20:36, Stephen White wrote:
> On Tue, Jul 20, 2010 at 11:00 AM, <mailto:wjmaclean@chromium.org> wrote:
> 
> > Thanks for noticing that other 'square' - I've uploaded a new patch that
> > fixes that.
> >
> > Stephen, do you have committer status, as I don't?
> >
> 
> Landed as r589.
> 
> Stephen
> 
> 
> > Cheers,
> >
> > James
> >
> >
> > http://codereview.appspot.com/1731053/show
> >
>

[wjmaclean, 2010-07-22 17:03:31]

Agreed ... the best is really to do a condition number, but that is (in
general) a bit too expensive (unless I'm mistaken), so I guess this is still
hackery but with a smaller threshold value.

Cheers,

James

On Thu, Jul 22, 2010 at 12:55 PM, <reed@android.com> wrote:

> ^2 is sort of correct for a non-perspective matix (which is all we
> should see in SVG), but it was all a hack/heuristic anyway to know how
> small we can accept before we divide.
>
> On 2010/07/20 15:20:36, Stephen White wrote:
>
>> On Tue, Jul 20, 2010 at 11:00 AM, <mailto:wjmaclean@chromium.org>
>>
> wrote:
>
>  > Thanks for noticing that other 'square' - I've uploaded a new patch
>>
> that
>
>> > fixes that.
>> >
>> > Stephen, do you have committer status, as I don't?
>> >
>>
>
>  Landed as r589.
>>
>
>  Stephen
>>
>
>
>  > Cheers,
>> >
>> > James
>> >
>> >
>> > http://codereview.appspot.com/1731053/show
>> >
>>
>
>
>
>
> http://codereview.appspot.com/1731053/show
>