fix for conic fuzz

fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003

Committed: https://skia.googlesource.com/skia/+/ac78863acdef4b428aaf66985b80c76d1be0fdea

[caryclark, 2016-09-20 14:35:12]

Description was changed from

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

Still thinking about lighter-weight solutions.

R=reed@google.com
BUG=647922
==========

to

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

Still thinking about lighter-weight solutions.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003
==========

[caryclark, 2016-09-20 14:35:55]

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

[commit-bot: I haz the power, 2016-09-20 14:36:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caryclark, 2016-09-20 14:37:56]

Have these conics already been split so they're monotonically increasing?

[commit-bot: I haz the power, 2016-09-20 14:43:51]

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

[commit-bot: I haz the power, 2016-09-20 14:43:52]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-09-20 17:33:26]

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp
File src/core/SkGeometry.cpp (right):

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1157: static bool between(SkScalar a, SkScalar b,
SkScalar c) {
This is super cool (assuming it works). Seems to work if the intermediate
multiply is inf, since inf is signed (should we test this?)

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1170: if (between(src.fPts[0].fY, src.fPts[1].fY,
src.fPts[2].fY)) {
// Need to ensure that after a chop of a monotonic in Y src, that the resulting
2 dsts are each also monotonic in Y.
// (why does we care about this?)

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1174: if (!between(dst[0].fPts[1].fY, dst[1].fPts[0].fY,
dst[1].fPts[1].fY)) {
Each of these 3 seem to be the same...

ensure_monotonic(SkScalar& a, SkScalar& b, SkScalar& c) {
    if (!between(a, b, c)) {
        b = a;
    }
}

How do we know which direction to push the middle value? This code always pushes
it towards a (the first value). Can it ever be that we want to nudge it towards
c? I presume we're accounting for tiny errors in the calculation, and that b
(when its not mono) is nearly equal to a or c ?

[caryclark, 2016-09-21 14:37:48]

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp
File src/core/SkGeometry.cpp (right):

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1157: static bool between(SkScalar a, SkScalar b,
SkScalar c) {
On 2016/09/20 17:33:25, reed1 wrote:
> This is super cool (assuming it works). Seems to work if the intermediate
> multiply is inf, since inf is signed (should we test this?)

It works. I've been testing it for years in pathops. I added a comment to
document this.

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1170: if (between(src.fPts[0].fY, src.fPts[1].fY,
src.fPts[2].fY)) {
On 2016/09/20 17:33:25, reed1 wrote:
> // Need to ensure that after a chop of a monotonic in Y src, that the
resulting
> 2 dsts are each also monotonic in Y.
> // (why does we care about this?)

Added comments.

https://codereview.chromium.org/2350263003/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:1174: if (!between(dst[0].fPts[1].fY, dst[1].fPts[0].fY,
dst[1].fPts[1].fY)) {
On 2016/09/20 17:33:25, reed1 wrote:
> Each of these 3 seem to be the same...
> 
> ensure_monotonic(SkScalar& a, SkScalar& b, SkScalar& c) {
>     if (!between(a, b, c)) {
>         b = a;
>     }
> }
> 
> How do we know which direction to push the middle value? This code always
pushes
> it towards a (the first value). Can it ever be that we want to nudge it
towards
> c? I presume we're accounting for tiny errors in the calculation, and that b
> (when its not mono) is nearly equal to a or c ?

The intention of moving it to one side was for consistency and with the thought
that these quads have all become lines at this point anyway, so moving the
control to equal the first point is no different from moving the control to
equal the last point.

On closer examination, it is more robust to treat aligning the control points
differently from aligning endpoints. Now, the end point is moved to the middle,
and each control point is, for consistency, moved to the end. I added comments
and asserts to verify that all five points are ordered.

[caryclark, 2016-09-21 14:38:03]

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

[commit-bot: I haz the power, 2016-09-21 14:38:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 15:18:11]

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

[commit-bot: I haz the power, 2016-09-21 15:18:12]

Dry run: This issue passed the CQ dry run.

[caryclark, 2016-09-21 15:19:19]

Description was changed from

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

Still thinking about lighter-weight solutions.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003
==========

to

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003
==========

[caryclark, 2016-09-21 19:18:16]

This changes more GMs than I expect, though the changes are small.

Maybe it would have less impact if the middle split point was moved to the
closer of the two endpoints
rather than midway between them. I'll try that next.

[caryclark, 2016-09-21 20:35:48]

With Patch Set 3, there are no GM changes.
Sound good?

[reed1, 2016-09-21 20:37:23]

lgtm

[caryclark, 2016-09-21 20:38:13]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2016-09-21 20:38:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 12:15:16]

Description was changed from

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003
==========

to

==========
fix for conic fuzz

A fuzzer generates a conic that hangs when drawn.
The quads that approximate the conics move up and down
in y, confusing the renderer.

This fix ensures that the split conic maintains the
same y direction as the original conic.

R=reed@google.com
BUG=647922
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2350263003

Committed:
https://skia.googlesource.com/skia/+/ac78863acdef4b428aaf66985b80c76d1be0fdea
==========

[commit-bot: I haz the power, 2016-09-22 12:15:17]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/ac78863acdef4b428aaf66985b80c76d1be0fdea

[caryclark, 2016-09-22 14:42:19]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2361473004/ by caryclark@google.com.

The reason for reverting is: See if this fixes the layout tests..