Fix SkPathStroker::lineTo() for line with length SK_ScalarNearlyZero

Fix SkPathStroker::lineTo() for line with length SK_ScalarNearlyZero
Committed: https://code.google.com/p/skia/source/detail?r=3650

[epoger, 2012-04-09 17:41:52]

The 1-character fix is taken from Erik's
https://critique.corp.google.com/#review/29047930

The test was adapted from an email from Justin

I have confirmed that I get a segfault in the new test without the fix, and the
fix makes the segfault go away.

[epoger, 2012-04-09 17:51:19]

Oops, had to add patchset 3 to add some SkIntToScalar() wrappers to the test...

[reed1, 2012-04-09 18:02:42]

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp
File src/core/SkPoint.cpp (right):

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp#newcode114
src/core/SkPoint.cpp:114: if (mag2 >= SK_ScalarNearlyZero * SK_ScalarNearlyZero)
{
Can we change it to > instead of >= ? It seems it might be reasonable (possible)
for nearlyzero to be set to zero, in which case we would divide-by-zero here.

[reed1, 2012-04-09 18:10:30]

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp
File src/core/SkPoint.cpp (right):

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp#newcode114
src/core/SkPoint.cpp:114: if (mag2 >= SK_ScalarNearlyZero * SK_ScalarNearlyZero)
{
My bad -- I thought the change also replaced one nearlyzero with nearlyzero^2,
but I see that it was already squared.

This seems an awfully fragile fix. Can we tweak the new test to make it fail, by
just slightly changing the input values?

On 2012/04/09 18:02:42, reed1 wrote:
> Can we change it to > instead of >= ? It seems it might be reasonable
(possible)
> for nearlyzero to be set to zero, in which case we would divide-by-zero here.

[epoger, 2012-04-09 18:28:28]

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp
File src/core/SkPoint.cpp (right):

https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp#newcode114
src/core/SkPoint.cpp:114: if (mag2 >= SK_ScalarNearlyZero * SK_ScalarNearlyZero)
{
On 2012/04/09 18:10:31, reed1 wrote:
> My bad -- I thought the change also replaced one nearlyzero with nearlyzero^2,
> but I see that it was already squared.
> 
> This seems an awfully fragile fix. Can we tweak the new test to make it fail,
by
> just slightly changing the input values?
> 
> On 2012/04/09 18:02:42, reed1 wrote:
> > Can we change it to > instead of >= ? It seems it might be reasonable
> (possible)
> > for nearlyzero to be set to zero, in which case we would divide-by-zero
here.
> 

My understanding from Erik's change in
https://critique.corp.google.com/#review/29047930 is that there is currently
some bad interaction between this check and IsLineDegenerate() in
http://code.google.com/p/skia/source/browse/trunk/include/core/SkPath.h .  Like,
if they have different border conditions (> vs. >=) then we get bad results in
certain cases...

Erik, is that right?

[hendriks, 2012-04-09 18:52:15]

On Mon, Apr 9, 2012 at 11:28 AM, <epoger@google.com> wrote:

>
>
https://codereview.appspot.**com/5992077/diff/5001/src/**core/SkPoint.cpp<htt...
> File src/core/SkPoint.cpp (right):
>
> https://codereview.appspot.**com/5992077/diff/5001/src/**
>
core/SkPoint.cpp#newcode114<https://codereview.appspot.com/5992077/diff/5001/src/core/SkPoint.cpp#newcode114>
> src/core/SkPoint.cpp:114: if (mag2 >= SK_ScalarNearlyZero *
> SK_ScalarNearlyZero) {
> On 2012/04/09 18:10:31, reed1 wrote:
>
>> My bad -- I thought the change also replaced one nearlyzero with
>>
> nearlyzero^2,
>
>> but I see that it was already squared.
>>
>
>  This seems an awfully fragile fix. Can we tweak the new test to make
>>
> it fail, by
>
>> just slightly changing the input values?
>>
>
>  On 2012/04/09 18:02:42, reed1 wrote:
>> > Can we change it to > instead of >= ? It seems it might be
>>
> reasonable
>
>> (possible)
>> > for nearlyzero to be set to zero, in which case we would
>>
> divide-by-zero here.
>
>
> My understanding from Erik's change in
>
https://critique.corp.google.**com/#review/29047930<https://critique.corp.goo...
that there is
> currently some bad interaction between this check and IsLineDegenerate()
> in
> http://code.google.com/p/skia/**source/browse/trunk/include/**
>
core/SkPath.h<http://code.google.com/p/skia/source/browse/trunk/include/core/SkPath.h>
> .  Like, if they have different border conditions (> vs. >=) then we get
> bad results in certain cases...
>
> Erik, is that right?
>

Yeah...  IsDegenerateLine checks that the length is >= the threshold.
 setLength checks that we are > the threshold.

Specifically we're seeing the assertion at
http://code.google.com/p/skia/source/browse/trunk/src/core/SkStroke.cpp#123fails
because we get past the IsDegenerateLine check at
http://code.google.com/p/skia/source/browse/trunk/src/core/SkStroke.cpp#216 but
not the check in setLength() reached via set_normal_unitnormal() ->
setNormalize().

I agree that the math looks fragile - esp if you're not looking at
horizontal or vertical lines.  I think the IsDegenerateLine check should
become more stringent than the setLength in that case which should work out
ok.  Of course, it'd be nice if there was only a single check or if the
check in preJoinTo could fail cleanly but I was looking for a non-invasive
solution.

- Erik

[reed1, 2012-04-09 19:16:57]

What if we change IsDegenerateLine to perform exactly the test done in setLength

i.e.  mag2 < nearlyzero^2

This actually seems faster than two sets of fabs + compares. Do we think that
would fix this bug, and not make things worse?

[epoger, 2012-04-10 15:25:26]

Mike, please see patchset 5... it seems to me like this ought to work, but now I
hit an assertion failure elsewhere... please come over when you have a few
minutes to look at it with me.

https://codereview.appspot.com/5992077/diff/9001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/5992077/diff/9001/include/core/SkPath.h#newcod...
include/core/SkPath.h:197: return p1.equalsWithinTolerance(p2,
SK_ScalarNearlyZero) &&
We should change these to use the new "standard" equalsWithinTolerance() also;
waiting until I can fix the remaining problems.

[epoger, 2012-04-10 15:55:36]

OK, it works now!  Mike, please let me know what you think...

[reed1, 2012-04-10 17:22:10]

I like the header changes, but I'm not sure about the rewrite of CanNormalize.
Can we disassemble it, and setLength, and confirm that each of these are still
leaf-functions (i.e. make no function calls out)?

[epoger, 2012-04-10 19:34:06]

On 2012/04/10 17:22:10, reed1 wrote:
> I like the header changes, but I'm not sure about the rewrite of CanNormalize.
> Can we disassemble it, and setLength, and confirm that each of these are still
> leaf-functions (i.e. make no function calls out)?

Mike and I looked at the disassembly and are satisfied that performance will not
be hurt (all appropriate code is inlined, etc.)

Mike, can I get an LG?

[reed1, 2012-04-10 20:27:06]

why yes you may

lgtm

[epoger, 2012-04-10 20:32:46]

On 2012/04/10 20:27:06, reed1 wrote:
> why yes you may
> 
> lgtm

Uh oh, I found a new problem... this CL breaks PointTest for SK_SCALAR_IS_FIXED,
because that test requires SkPoint::Length() and SkPoint::Normalize() to return
exactly equal values... and with this change they are computed differently and
thus may be slightly off.

[epoger, 2012-04-11 15:39:29]

Mike/Brian, please see SkPoint.cpp as of patchset 10 :
https://codereview.appspot.com/5992077/diff/9005/src/core/SkPoint.cpp

(no other files have changed since Mike's previous LGTM)

With this change, all tests pass on both float and fixed.  In fixed-point mode,
SkPoint::Normalize() now uses the same high-precision (Sk64) math that
CanNormalize() and Length() do.

[reed1, 2012-04-11 15:43:12]

lgtm

[reed1, 2012-04-11 15:52:55]

https://codereview.appspot.com/5992077/diff/9005/src/core/SkPoint.cpp
File src/core/SkPoint.cpp (right):

https://codereview.appspot.com/5992077/diff/9005/src/core/SkPoint.cpp#newcode98
src/core/SkPoint.cpp:98: // in *result.  Returns true if the distance is judged
to be "not nearly zero".
// This logic is encapsulated in a helper method to make it explicit that we
always perform this check in the same manner, to avoid inconsistencies (e.g. in
SkStroker).

[bsalomon, 2012-04-11 15:54:46]

https://codereview.appspot.com/5992077/diff/9005/src/core/SkPoint.cpp
File src/core/SkPoint.cpp (right):

https://codereview.appspot.com/5992077/diff/9005/src/core/SkPoint.cpp#newcode99
src/core/SkPoint.cpp:99: static inline bool isDistanceSquaredNotNearlyZero(float
dx, float dy,
How about static inline bool isLengthNearlyZero(float dx, float dy, float*
lengthSqd)

[reed1, 2012-04-11 15:57:56]

Hmmm, 

Now the comment has to read:

// Return true if the length is nearly zero, but return the length squared.

Does the name need to not agree with its return-arg?

[bsalomon, 2012-04-11 16:30:40]

On 2012/04/11 15:57:56, reed1 wrote:
> Hmmm, 
> 
> Now the comment has to read:
> 
> // Return true if the length is nearly zero, but return the length squared.
> 
> Does the name need to not agree with its return-arg?

Ok, fine, isLengthSqdNearlyZero

[bsalomon, 2012-04-11 16:31:12]

On 2012/04/11 16:30:40, bsalomon wrote:
> On 2012/04/11 15:57:56, reed1 wrote:
> > Hmmm, 
> > 
> > Now the comment has to read:
> > 
> > // Return true if the length is nearly zero, but return the length squared.
> > 
> > Does the name need to not agree with its return-arg?
> 
> Ok, fine, isLengthSqdNearlyZero

Or isLengthSqdNearlyZeroSqd :)

[epoger, 2012-04-11 16:36:54]

Brian/Mike, please see latest change to function names:
https://codereview.appspot.com/5992077/diff2/9005:8009/src/core/SkPoint.cpp

Looking for that crucial third LGTM...

[bsalomon, 2012-04-11 17:39:01]

On 2012/04/11 16:36:54, epoger wrote:
> Brian/Mike, please see latest change to function names:
> https://codereview.appspot.com/5992077/diff2/9005:8009/src/core/SkPoint.cpp
> 
> Looking for that crucial third LGTM...

LGTM