add support for capped hairlines

add support for  capped hairlines

Extend the ends of hairline and haircurve segments when the paint is set to square or round, and the line or curve is at the start or end of a contour.

R=reed@google.com
BUG=skia:4599
Committed: https://skia.googlesource.com/skia/+/2028d7ff744c36855ed36d602e3e050e9f18ec9f

[caryclark, 2015-12-02 23:47:03]

Description was changed from

==========
wip square hairlines

BUG=skia:
==========

to

==========
add support for  capped hairlines

Extend the ends of hairline and haircurve segments when the paint is set to
square or round, and the line or curve is at the start or end of a contour.

R=reed@google.com
BUG=skia:4599
==========

[caryclark, 2015-12-02 23:47:20]

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

[caryclark, 2015-12-02 23:47:28]

please review

[commit-bot: I haz the power, 2015-12-02 23:47:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491843006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491843006/40001

[commit-bot: I haz the power, 2015-12-02 23:48:33]

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

[commit-bot: I haz the power, 2015-12-02 23:48:33]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[caryclark, 2015-12-02 23:53:09]

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

[commit-bot: I haz the power, 2015-12-02 23:53:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491843006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491843006/60001

[commit-bot: I haz the power, 2015-12-03 00:06:36]

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

[commit-bot: I haz the power, 2015-12-03 00:06:38]

Dry run: This issue passed the CQ dry run.

[caryclark, 2015-12-04 15:19:03]

comments?

[caryclark, 2015-12-04 15:26:13]

caryclark@google.com changed reviewers:
+ reed@google.com

[caryclark, 2015-12-04 15:26:34]

oops, forgot to set reviewer

[reed1, 2015-12-04 18:01:10]

https://codereview.chromium.org/1491843006/diff/60001/gm/hairlines.cpp
File gm/hairlines.cpp (right):

https://codereview.chromium.org/1491843006/diff/60001/gm/hairlines.cpp#newcod...
gm/hairlines.cpp:246: draw_squarehair_tests(canvas, 0, SkPaint::kSquare_Cap,
false);
Wonder if adding 0.999 would be helpful.

[caryclark, 2015-12-04 20:55:15]

https://codereview.chromium.org/1491843006/diff/60001/gm/hairlines.cpp
File gm/hairlines.cpp (right):

https://codereview.chromium.org/1491843006/diff/60001/gm/hairlines.cpp#newcod...
gm/hairlines.cpp:246: draw_squarehair_tests(canvas, 0, SkPaint::kSquare_Cap,
false);
On 2015/12/04 18:01:10, reed1 wrote:
> Wonder if adding 0.999 would be helpful.

Done.

[caryclark, 2015-12-07 13:22:09]

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

[commit-bot: I haz the power, 2015-12-07 13:22:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491843006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491843006/120001

[commit-bot: I haz the power, 2015-12-07 13:23:18]

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

[commit-bot: I haz the power, 2015-12-07 13:23:19]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[caryclark, 2015-12-07 13:23:48]

Perhaps it's worth treating square and round differently, allowing the end
coverage to reflect the geometry of the end cap. Consider whether the most
recent CL is worth the additional complexity.

[caryclark, 2015-12-07 13:26:56]

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

[commit-bot: I haz the power, 2015-12-07 13:27:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491843006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491843006/140001

[commit-bot: I haz the power, 2015-12-07 13:40:19]

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

[commit-bot: I haz the power, 2015-12-07 13:40:20]

Dry run: This issue passed the CQ dry run.

[caryclark, 2015-12-08 12:30:40]

What would you like to see next on this patch?

[reed1, 2015-12-08 16:59:02]

Seems more readable to me than the prev, thanks.
Any meansurable perf impact from nanobench? Do we have decent coverage in
nanobench for hairs (I bet we do, but not sure)?

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkPathRef.cpp
File src/core/SkPathRef.cpp (right):

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkPathRef.cpp...
src/core/SkPathRef.cpp:577: const uint8_t* next = fVerbs - 1;
I wonder if this should just SkASSERT if we read past the end. It seems like a
client would never need to call pack *after* receiving a kDone_Verb. 

Might also inline it if its gets that simple. (or not, just a thought).

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkScan_Hairli...
File src/core/SkScan_Hairline.cpp (right):

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkScan_Hairli...
src/core/SkScan_Hairline.cpp:357: const SkScalar capOutset =
SkPaint::kSquare_Cap == capStyle ? 0.5f : SK_ScalarPI / 8;
1. assert that the capStyle is either Square or Round?
2. document why PI/8 for round?

[caryclark, 2015-12-08 19:59:13]

On 2015/12/08 16:59:02, reed1 wrote:
> Seems more readable to me than the prev, thanks.
> Any meansurable perf impact from nanobench? Do we have decent coverage in
> nanobench for hairs (I bet we do, but not sure)?
> 
Perf hit for butt or round? I'll check for both.

[caryclark, 2015-12-08 19:59:39]

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkPathRef.cpp
File src/core/SkPathRef.cpp (right):

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkPathRef.cpp...
src/core/SkPathRef.cpp:577: const uint8_t* next = fVerbs - 1;
On 2015/12/08 16:59:02, reed1 wrote:
> I wonder if this should just SkASSERT if we read past the end. It seems like a
> client would never need to call pack *after* receiving a kDone_Verb. 
> 
> Might also inline it if its gets that simple. (or not, just a thought).

I'll try the assert. I thought that if fVerbs is pointing to the last verb, next
is off the end and it would be reasonable to check.

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkScan_Hairli...
File src/core/SkScan_Hairline.cpp (right):

https://codereview.chromium.org/1491843006/diff/140001/src/core/SkScan_Hairli...
src/core/SkScan_Hairline.cpp:357: const SkScalar capOutset =
SkPaint::kSquare_Cap == capStyle ? 0.5f : SK_ScalarPI / 8;
On 2015/12/08 16:59:02, reed1 wrote:
> 1. assert that the capStyle is either Square or Round?
> 2. document why PI/8 for round?

Done.

[reed1, 2015-12-08 20:03:25]

brain-fart on expecting an actual Done verb in the data stream.

lgtm pending perf happiness

(do we need a chrome guard?)

[caryclark, 2015-12-09 21:51:26]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2015-12-09 21:51:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1491843006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1491843006/160001

[commit-bot: I haz the power, 2015-12-09 22:04:50]

Description was changed from

==========
add support for  capped hairlines

Extend the ends of hairline and haircurve segments when the paint is set to
square or round, and the line or curve is at the start or end of a contour.

R=reed@google.com
BUG=skia:4599
==========

to

==========
add support for  capped hairlines

Extend the ends of hairline and haircurve segments when the paint is set to
square or round, and the line or curve is at the start or end of a contour.

R=reed@google.com
BUG=skia:4599

Committed:
https://skia.googlesource.com/skia/+/2028d7ff744c36855ed36d602e3e050e9f18ec9f
==========

[commit-bot: I haz the power, 2015-12-09 22:04:50]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/2028d7ff744c36855ed36d602e3e050e9f18ec9f