added stroking support to GrAALinearizingConvexPathRenderer

added stroking support to GrAALinearizingConvexPathRenderer

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/2436f191e6602953b32a51cf50f2d7a4e2af90fd

[ethannicholas, 2015-06-15 18:42:00]

ethannicholas@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[robertphillips, 2015-06-16 13:13:02]

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (left):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:782: for (int cur = 0; cur < this->numPts();
++cur) {
So, can computeRealDepth go too ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (right):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:23: 
Expand this comment a bit - are they in device space? Are they curvature
tolerances?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:156: bool
GrAAConvexTessellator::createInsetRings(Ring& previousRing, SkScalar
initialDepth,
tab this line over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:169: bool done =
this->createInsetRing(*currentRing, nextRing, initialDepth, initialCoverage,
tab this line over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:204: Ring outerStrokeRing;
What happens when fStrokeWidth is 1.00001 ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:205: this->createOuterRing(fInitialRing,
fStrokeWidth / 2 - kAntialiasingRadius, coverage,
tab this line over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:217: if (fStrokeWidth >= 0.0f &&
fInitialRing.numPts() > 2) {
Do we actually need to get the Ring* back from createInsetRings ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:219: SkScalar strokeDepth = fStrokeWidth / 2 -
kAntialiasingRadius;
this->createInsetRings ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:330: if (this->numPts() >= 3) {
Don't we still want to remove the last point if it is on the line from the
second to last to the first point?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:383: 
SkPoint::Make ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:435: 
Make outsetSq const ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:506: if (lengthSq > miterLimitSq) {
Shouldn't we just cap the miter in this case - not revert to bevel ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:569: 
compute_coverage since static ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:570: static SkScalar computeCoverage(SkScalar
depth, SkScalar initialDepth, SkScalar initialCoverage,
tab this over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:582: bool
GrAAConvexTessellator::createInsetRing(const Ring& lastRing, Ring* nextRing,
tab these lines over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:691: // fused (and is thus movable)
Is there any downside to splitting this into 2 statements ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:692: newIdx =
this->addPt(fCandidateVerts.point(i), depth,
this->computeCoverage

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:693: computeCoverage(depth, initialDepth,
initialCoverage,
tab this line over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:698:
this->updatePt(fCandidateVerts.originatingIdx(i), fCandidateVerts.point(i),
depth,
tab this over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.h (right):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:22: 
// Add a comment ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:214: bool createInsetRings(Ring& previousRing,
SkScalar initialDepth, SkScalar initialCoverage,
tab this line over ?

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAALinearizing...
File src/gpu/GrAALinearizingConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAALinearizing...
src/gpu/GrAALinearizingConvexPathRenderer.cpp:31: 
More informative name? Which buffer ?

[ethannicholas, 2015-06-16 14:53:29]

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (left):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:782: for (int cur = 0; cur < this->numPts();
++cur) {
On 2015/06/16 13:13:01, robertphillips wrote:
> So, can computeRealDepth go too ?

Looks like it.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (right):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:23: 
On 2015/06/16 13:13:01, robertphillips wrote:
> Expand this comment a bit - are they in device space? Are they curvature
> tolerances? 

Done.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:156: bool
GrAAConvexTessellator::createInsetRings(Ring& previousRing, SkScalar
initialDepth,
On 2015/06/16 13:13:01, robertphillips wrote:
> tab this line over ?

Sorry, been using "two tabs for continuations" for like twenty years now, it's a
hard habit to break :-)

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:204: Ring outerStrokeRing;
On 2015/06/16 13:13:01, robertphillips wrote:
> What happens when fStrokeWidth is 1.00001 ?

I tested it out, and it doesn't appear to cause any problems.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:217: if (fStrokeWidth >= 0.0f &&
fInitialRing.numPts() > 2) {
On 2015/06/16 13:13:01, robertphillips wrote:
> Do we actually need to get the Ring* back from createInsetRings ?

When stroking, we call createInsetRings twice. The first call creates the inner
line of the stroke, and then the next call insets that further with zero
coverage to create the antialiased edge. Getting the ring back from the first
call allows me to use it as the basis for the second call. I'm open to other
suggestions if there is a better way to do this.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:330: if (this->numPts() >= 3) {
On 2015/06/16 13:13:00, robertphillips wrote:
> Don't we still want to remove the last point if it is on the line from the
> second to last to the first point?

Oops, hadn't meant to leave that commented out.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:506: if (lengthSq > miterLimitSq) {
On 2015/06/16 13:13:01, robertphillips wrote:
> Shouldn't we just cap the miter in this case - not revert to bevel ?

As far as I can tell, what I am doing is visually identical to what the standard
renderer is doing.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:569: 
On 2015/06/16 13:13:01, robertphillips wrote:
> compute_coverage since static ?

Done.

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.h (right):

https://codereview.chromium.org/1180903006/diff/40001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.h:22: 
On 2015/06/16 13:13:02, robertphillips wrote:
> // Add a comment ?

Done.

[robertphillips, 2015-06-16 15:39:52]

lgtm + a nit
Brian - any comments?

https://codereview.chromium.org/1180903006/diff/60001/src/gpu/GrAAConvexTesse...
File src/gpu/GrAAConvexTessellator.cpp (right):

https://codereview.chromium.org/1180903006/diff/60001/src/gpu/GrAAConvexTesse...
src/gpu/GrAAConvexTessellator.cpp:694: SkScalar coverage =
compute_coverage(depth, initialDepth, initialCoverage,
tab this over ?

[bsalomon, 2015-06-16 15:41:48]

no, lgtm

[ethannicholas, 2015-06-25 20:52:17]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2015-06-25 20:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1180903006/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-25 20:52:36]

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

[commit-bot: I haz the power, 2015-06-25 20:55:50]

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

[commit-bot: I haz the power, 2015-06-25 20:55:51]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[ethannicholas, 2015-06-25 20:57:00]

The CQ bit was unchecked by ethannicholas@google.com

[ethannicholas, 2015-06-25 21:32:05]

The CQ bit was checked by ethannicholas@google.com

[ethannicholas, 2015-06-25 21:32:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1180903006/#ps100001
(title: "windows build issue")

[commit-bot: I haz the power, 2015-06-25 21:32:18]

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

[commit-bot: I haz the power, 2015-06-25 21:42:38]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/2436f191e6602953b32a51cf50f2d7a4e2af90fd

[reed2, 2015-06-26 04:32:32]

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

[reed2, 2015-06-26 04:32:33]

could this CL be causing the DEPS roll (linux_blink_rel) to fail?

[reed2, 2015-06-26 12:06:23]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1212833002/ by reed@chromium.org.

The reason for reverting is: speculative revert to try to fix DEPS roll
(layouttests).