Fallback to moveTo when unable to find the first tangent in cubicTo

Fallback to moveTo when unable to find the first tangent in cubicTo

When calling cubicTo(a, b, c) and if the distance between fPrevPt and a
is too small, b is used instead of a to calculate the first tangent,
even if the distance between fPrevPt and b is too small.

In debug mode, this is causing an assertion to fail in
SkPathStroker::preJoinTo() and, in Release, the use of an
unitialized value.

The first patch set is adding a failing test.
The second one add the fix to SkPathStroker::cubicTo()

BUG=skia:2820
Committed: https://skia.googlesource.com/skia/+/fac4e0e83666ab59373169d6c157d3654cb479a3

[Rémi Piotaix, 2014-08-11 21:06:16]

PTAL

[Rémi Piotaix, 2014-08-18 14:33:16]

On 2014/08/11 21:06:16, Rémi Piotaix wrote:
> PTAL

Ping?

[bsalomon, 2014-08-18 16:22:44]

Using Mike's @google address and adding Cary.

[Justin Novosad, 2014-08-20 14:26:12]

On 2014/08/18 16:22:44, bsalomon wrote:
> Using Mike's @google address and adding Cary.

Review ping.

Looks fine to me but I am not familiar with this area of the code.

[reed1, 2014-08-20 14:47:11]

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:67: static void make_path_crbugskia2820(SkPath* path,
skiatest::Reporter* reporter) {
Seems like a brief comment would help. e.g.

/* This path used to generate valgrid/asan warnings in the stroker code. */

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:68: float rdi =  1./4096.;
All of these literals will generate double->float warnings.

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:76: REPORTER_ASSERT(reporter, SkPath::IsLineDegenerate(orig,
p1));
I don't see why these checks should be in the test. They seem to be leaking
(current) implementation details outside of the SkPath api.

[Rémi Piotaix, 2014-08-20 18:18:08]

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:67: static void make_path_crbugskia2820(SkPath* path,
skiatest::Reporter* reporter) {
On 2014/08/20 14:47:11, reed1 wrote:
> Seems like a brief comment would help. e.g.
> 
> /* This path used to generate valgrid/asan warnings in the stroker code. */

Done.

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:68: float rdi =  1./4096.;
On 2014/08/20 14:47:10, reed1 wrote:
> All of these literals will generate double->float warnings.

Done.

https://codereview.chromium.org/460813002/diff/20001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:76: REPORTER_ASSERT(reporter, SkPath::IsLineDegenerate(orig,
p1));
On 2014/08/20 14:47:10, reed1 wrote:
> I don't see why these checks should be in the test. They seem to be leaking
> (current) implementation details outside of the SkPath api.

These are assertions on the data (orig+p1-3) used to trigger the bug.
Because the behavior of cubicTo depends on these assertions on the input, it is
meaningful to me to keep them but maybe it is not....

[reed1, 2014-08-20 19:07:18]

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:73: orig = SkPoint::Make(SkIntToScalar(1), SkIntToScalar(1));
:)

I find this very hard to read. Looking right below, lines 101.. is very
readable, and does not generate any warnings. I think that patterns is worth
replicating.

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:78: REPORTER_ASSERT(reporter, SkPath::IsLineDegenerate(orig,
p1));
These lines are testing some aspect of IsLineDegenerate, not testing anything
about stroking per-se, as we can always change how we implement stroking. The
only "real" bug here is that we read uninitialized memory, and/or fire an
assert. The purpose of a test is to exercise that, so that we see that neither
of those happen in the future.

[Rémi Piotaix, 2014-08-20 19:21:09]

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:73: orig = SkPoint::Make(SkIntToScalar(1), SkIntToScalar(1));
On 2014/08/20 19:07:18, reed1 wrote:
> :)
> 
> I find this very hard to read. Looking right below, lines 101.. is very
> readable, and does not generate any warnings. I think that patterns is worth
> replicating.

Done.

https://codereview.chromium.org/460813002/diff/60001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:78: REPORTER_ASSERT(reporter, SkPath::IsLineDegenerate(orig,
p1));
On 2014/08/20 19:07:18, reed1 wrote:
> These lines are testing some aspect of IsLineDegenerate, not testing anything
> about stroking per-se, as we can always change how we implement stroking. The
> only "real" bug here is that we read uninitialized memory, and/or fire an
> assert. The purpose of a test is to exercise that, so that we see that neither
> of those happen in the future.

Done.

[Justin Novosad, 2014-08-26 18:10:16]

https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:68: * In debug mode, this path is causing an assertion to
fail in
is -> was

[Rémi Piotaix, 2014-08-26 18:17:42]

https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp
File tests/PathTest.cpp (right):

https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp#newco...
tests/PathTest.cpp:68: * In debug mode, this path is causing an assertion to
fail in
On 2014/08/26 18:10:16, junov wrote:
> is -> was

Done.

[Justin Novosad, 2014-08-26 18:20:14]

On 2014/08/26 18:17:42, Rémi Piotaix wrote:
> https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp
> File tests/PathTest.cpp (right):
> 
>
https://codereview.chromium.org/460813002/diff/80001/tests/PathTest.cpp#newco...
> tests/PathTest.cpp:68: * In debug mode, this path is causing an assertion to
> fail in
> On 2014/08/26 18:10:16, junov wrote:
> > is -> was
> 
> Done.

This look fine to me.

reed?

[reed1, 2014-08-26 18:37:15]

as verbose as it is, I like the test!

lgtm

[Rémi Piotaix, 2014-08-26 18:38:30]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-08-26 18:39:23]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/460813002/100001

[commit-bot: I haz the power, 2014-08-26 18:59:09]

Committed patchset #6 (100001) as fac4e0e83666ab59373169d6c157d3654cb479a3