Proposed fix for small bug in gpu conical gradients

Proposed fix for small bug in gpu conical gradients

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/0125847c165d59c0efcbf9ab81e6b51715829afd

[joshua.litt, 2014-09-20 19:30:00]

joshualitt@chromium.org changed reviewers:
+ senorblanco@google.com

[joshua.litt, 2014-09-20 19:30:01]

I highly doubt this is the right way to fix this bug, but I figured I'd give it
a shot.  Basically:

In SkTwoPointConicalGradient, in the function set_matrix_circle_conical, there
is a test for whether or not to draw with a degenerate case:

    SkScalar A = centerEndTrans.fX * centerEndTrans.fX + centerEndTrans.fY *
centerEndTrans.fY - radiusEnd * radiusEnd + 2 * radiusEnd - 1;

This A value is tested against kEdgeErrorTol.

There is also an assert in the degenerate case:

        SkASSERT(SkScalarAbs(SkScalarAbs(fDiffRadius) - SkScalarAbs(fCenterX1))
<
                 kEdgeErrorTol * (fRadius0 < kErrorTol ? shader.getEndRadius() :
fRadius0));

The bug is that sometimes A < kEdgeErrorTol, when abs(abs(fDiffRadius) -
fCenterX1 > kEdgeErrorTol * fRadius0.

I'm not exactly sure what the A above is, or if it is being computed correctly

[Stephen White, 2014-09-21 21:19:23]

senorblanco@chromium.org changed reviewers:
+ bsalomon@google.com, egdaniel@google.com
- senorblanco@google.com

[Stephen White, 2014-09-21 21:20:25]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-09-21 21:20:25]

+bsalomon, +egdaniel

I'm not really the right person to review this -- I wrote the two-point radial
gradient, but not the conical ones.

[egdaniel, 2014-09-22 13:59:35]

https://codereview.chromium.org/593443002/diff/1/src/effects/gradients/SkTwoP...
File src/effects/gradients/SkTwoPointConicalGradient_gpu.cpp (right):

https://codereview.chromium.org/593443002/diff/1/src/effects/gradients/SkTwoP...
src/effects/gradients/SkTwoPointConicalGradient_gpu.cpp:788: if
(SkScalarAbs(SkScalarAbs(shader.getDiffRadius()) - shader.getCenterX1()) <
can we do "SkScalarAbs(||centerEndTrans|| - (radiusEnd -1)) < kErrorTol"?

It involves the exact same math as your version and we only have to worry about
the radius scaling of error in the shader instead of both places.

[joshua.litt, 2014-09-22 14:24:00]

feedback incorporated

[egdaniel, 2014-09-22 14:57:02]

https://codereview.chromium.org/593443002/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointConicalGradient_gpu.cpp (right):

https://codereview.chromium.org/593443002/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointConicalGradient_gpu.cpp:920: radius2 = radius1 +
diffLen + increase;
radius2 = radius1 + diffLen + increase + radius1 * kEdgeErrorTol;

https://codereview.chromium.org/593443002/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointConicalGradient_gpu.cpp:1155: radius2 = radius1
+ random->nextRangeF(0.f, diffLen);
can we change this to:

radius2 = radius1 + random->nextRangeF(0.f, diffLen - radius1 * kEdgeErrorTol);

This way we can never create an edge case during our outside test create.

[joshua.litt, 2014-09-22 15:19:56]

feedback incorporated

[egdaniel, 2014-09-22 15:20:31]

lgtm

[joshua.litt, 2014-09-22 16:32:54]

feedback inc

[joshua.litt, 2014-09-22 17:22:19]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-09-22 17:22:28]

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

[commit-bot: I haz the power, 2014-09-22 17:29:36]

Committed patchset #5 (id:80001) as 0125847c165d59c0efcbf9ab81e6b51715829afd