Issue 4926045: Add AA hairline path renderer

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1082 lines, -47 lines)			Patch
M	gm/base-win/complexclip_gpu.png	View	7	Binary file		0 comments	Download
A	gpu/src/GrAAHairLinePathRenderer.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+62 lines, -0 lines	0 comments	Download
A	gpu/src/GrAAHairLinePathRenderer.cpp	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+633 lines, -0 lines	0 comments	Download
A	gpu/src/GrAddPathRenderers_aahairline.cpp	View	7	1 chunk	+20 lines, -0 lines	0 comments	Download
M	gpu/src/GrContext.cpp	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -2 lines	0 comments	Download
M	gpu/src/GrDefaultPathRenderer.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+2 lines, -1 line	0 comments	Download
M	gpu/src/GrDrawTarget.h	View	1 2 3 4 5 6 7 8 9	11 chunks	+63 lines, -11 lines	0 comments	Download
M	gpu/src/GrDrawTarget.cpp	View	1 2 3 4 5 6 7 8 9	8 chunks	+70 lines, -17 lines	0 comments	Download
M	gpu/src/GrGLProgram.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+4 lines, -0 lines	0 comments	Download
M	gpu/src/GrGLProgram.cpp	View	1 2 3 4 5 6 7 8 9	3 chunks	+23 lines, -1 line	0 comments	Download
M	gpu/src/GrGpuGLFixed.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+9 lines, -2 lines	0 comments	Download
M	gpu/src/GrGpuGLShaders.cpp	View	1 2 3 4 5 6 7 8 9	6 chunks	+53 lines, -7 lines	0 comments	Download
M	gpu/src/GrPathRenderer.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+16 lines, -1 line	0 comments	Download
M	gpu/src/GrPathRendererChain.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -1 line	0 comments	Download
M	gpu/src/GrTesselatedPathRenderer.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+2 lines, -1 line	0 comments	Download
M	gpu/src/GrTesselatedPathRenderer.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+2 lines, -1 line	0 comments	Download
M	gyp/SampleApp.gyp	View	1 2 3 4 5 6 7 8 9	3 chunks	+2 lines, -1 line	0 comments	Download
M	gyp/gpu.gyp	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -1 line	0 comments	Download
A	samplecode/SampleHairCurves.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+112 lines, -0 lines	0 comments	Download

Messages

Total messages: 14

Expand All Messages | Collapse All Messages

TomH

Beginning of review... More tomorrow. http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h File gpu/src/GrPathRenderer.h (right): http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h#newcode57 gpu/src/GrPathRenderer.h:57: * modifications are limited ...

12 years, 10 months ago (2011-08-25 21:48:31 UTC) #2

bsalomon

http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h File gpu/src/GrPathRenderer.h (right): http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h#newcode57 gpu/src/GrPathRenderer.h:57: * modifications are limited to scaling and translating. On ...

12 years, 10 months ago (2011-08-26 12:55:48 UTC) #3

http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h
File gpu/src/GrPathRenderer.h (right):

http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h#newc...
gpu/src/GrPathRenderer.h:57: * modifications are limited to scaling and
translating.
On 2011/08/25 21:48:31, TomH wrote:
> "are limited" isn't clear to me. Does this mean that Skia may make those
changes
> unbeknownst to the user? Or that the user may make those changes, but must not
> make any other kind of change?

The user is skia (well Ganesh). I'm regretting the whole stateful nature of
GrPathRenderer (setPath, clearPath) that was put in to avoid retessellating for
each tile of OSSAA. This contract between GrContext and GrPathRenderer as to
what state can change between calls is far too complicated.

I'd like to make OSSAA an object that can be thrown on the stack, whacks a
GrDrawTarget's state, and cleans up in its destructor. Then a path renderer can
decide to use it internally to do AA and we can get rid of setPath/clearPath.

In the meantime I tried to make the comment a little clearer.

http://codereview.appspot.com/4926045/diff/8002/gpu/src/GrPathRenderer.h#newc...
gpu/src/GrPathRenderer.h:66: GrPathFill fill) const = 0;
On 2011/08/25 21:48:31, TomH wrote:
> I don't see any of the current implementations using this new parameter?

It's been added to their overrides but they aren't using it. The new one checks
whether the matrix on target has perspective (until I get to making it work in
perspective).

http://codereview.appspot.com/4926045/diff/8002/gyp/SampleApp.gyp
File gyp/SampleApp.gyp (right):

http://codereview.appspot.com/4926045/diff/8002/gyp/SampleApp.gyp#newcode72
gyp/SampleApp.gyp:72: '../samplecode/SampleHairCubics.cpp',
On 2011/08/25 21:48:31, TomH wrote:
> These files aren't in this commit - were they just previously omitted? Or do
> they need a svn add?

SampleHairCubics is part of this Issue. In fact you commented on it :)
SampleAARects was just moved to its rightful alphabetical position in this list.

http://codereview.appspot.com/4926045/diff/8002/gyp/gpu.gyp
File gyp/gpu.gyp (left):

http://codereview.appspot.com/4926045/diff/8002/gyp/gpu.gyp#oldcode129
gyp/gpu.gyp:129: '../gpu/src/GrAddPathRenderers_none.cpp',
On 2011/08/25 21:48:31, TomH wrote:
> Shouldn't this have been obviated previously by the path renderer chain
scheme?

The GrAddPathRenderers_*.cpp file is the guy who decides which path renderers
are in the chain and in what order. Its because of ordering that we don't have
an auto-register-yourself-by-static-initialization scheme. There's room for
improvement.

http://codereview.appspot.com/4926045/diff/8002/samplecode/SampleHairCubics.cpp
File samplecode/SampleHairCubics.cpp (right):

http://codereview.appspot.com/4926045/diff/8002/samplecode/SampleHairCubics.c...
samplecode/SampleHairCubics.cpp:64: curves.quadTo(pts[2], pts[3],
On 2011/08/25 21:48:31, TomH wrote:
> Can we delete the #elif 0 & #else clauses? Or is there some reason to keep
them
> around?

Its useful when debugging or analyzing perf of changes to the hairline renderer
to quickly modify the sample to draw quads or lines.

http://codereview.appspot.com/4926045/diff/8002/samplecode/SampleHairCubics.c...
samplecode/SampleHairCubics.cpp:84: //canvas->drawPath(ctrlPts, paint);
On 2011/08/25 21:48:31, TomH wrote:
> Clean up stale code?

It's useful for debugging changes to the hairline renderer. If I notice that a
curve looks significantly different than raster I can modify the code to only
draw that curve and draw its hull and control points.

Stephen White

LGTM. My comments are mostly cosmetic. http://codereview.appspot.com/4926045/diff/15001/gpu/src/GrAAHairLinePathRenderer.cpp File gpu/src/GrAAHairLinePathRenderer.cpp (right): http://codereview.appspot.com/4926045/diff/15001/gpu/src/GrAAHairLinePathRenderer.cpp#newcode28 gpu/src/GrAAHairLinePathRenderer.cpp:28: data[8 + 9*i] ...

12 years, 10 months ago (2011-08-29 19:18:04 UTC) #5

bsalomon

Thanks for the review! I incorporated all Stephen's comments into patchset 8. On 2011/08/29 19:18:04, ...

12 years, 10 months ago (2011-08-29 20:42:43 UTC) #6

Stephen White

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRenderer.cpp File gpu/src/GrAAHairLinePathRenderer.cpp (right): http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRenderer.cpp#newcode43 gpu/src/GrAAHairLinePathRenderer.cpp:43: data[0 + kIdxsPerQuad*i] = kVertsPerQuad*i + 0; // a0 ...

12 years, 10 months ago (2011-08-29 20:49:43 UTC) #7

bsalomon

Patchset 9 incorporates Stephen's second set of comments, renames SampleHairCubics to SampleHairCubics, and fixes a ...

12 years, 10 months ago (2011-08-29 21:10:01 UTC) #8

Stephen White

On 2011/08/29 21:10:01, bsalomon wrote: > Patchset 9 incorporates Stephen's second set of comments, renames ...

12 years, 10 months ago (2011-08-29 21:14:48 UTC) #9

TomH

Add a little more and we can have a 1kLOC commit! http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRenderer.cpp File gpu/src/GrAAHairLinePathRenderer.cpp (right): ...

12 years, 10 months ago (2011-08-30 14:33:12 UTC) #11

bsalomon

Patchset 11 addresses Tom's comments. http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRenderer.cpp File gpu/src/GrAAHairLinePathRenderer.cpp (right): http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRenderer.cpp#newcode11 gpu/src/GrAAHairLinePathRenderer.cpp:11: // AA stroke around ...

12 years, 10 months ago (2011-08-30 17:49:50 UTC) #12

Patchset 11 addresses Tom's comments.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
File gpu/src/GrAAHairLinePathRenderer.cpp (right):

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:11: // AA stroke around the center-curve.
On 2011/08/30 14:33:12, TomH wrote:
> I do not understand this comment. Am I expected to?
> Edit: ok, first function in the file has it. maybe add "(q.v.
> push_quad_index_data)"?

Done.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:50: data[7 + kIdxsPerQuad*i] =
kVertsPerQuad*i + 4; // a1
On 2011/08/30 14:33:12, TomH wrote:
> Should this comment be // c1?

Done.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:72: SkAutoTUnref<GrIndexBuffer>
qIdxBuffer(qIdxBuf);
On 2011/08/30 14:33:12, TomH wrote:
> Why do we need this?
GrAAHairLinePathRenderer will take a ref to the object. This function will want
to release its ref to the object (or not if it failed to allocate the IB). I
added a comment.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:131: static const SkScalar
gDegernateToLineTol = SK_Scalar1;
On 2011/08/30 14:33:12, TomH wrote:
> Misspelled?

Done.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:138: int sublevel = 0) {
On 2011/08/30 14:33:12, TomH wrote:
> Why do we do this - so we only have to have an implementation that deals with
> quads, and not also one that deals with cubics?

Correct, added a comment

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:190: return (((*(int*)&x) & 0x7f800000) >>
23) - 127;
On 2011/08/30 14:33:12, TomH wrote:
> Unit test? This looks like the sort of thing prone to fail cross-platform.
(Then
> again, it isn't exposed, so I guess we can't unit test. I never really
> understood unit tests, since anything like this that's worth testing and
> testable isn't accessible without hackery.)

I added a unit test on first run in debug.

http://codereview.appspot.com/4926045/diff/4002/gpu/src/GrAAHairLinePathRende...
gpu/src/GrAAHairLinePathRenderer.cpp:210: static const SkScalar gSubdivTol = 175
* SK_Scalar1;
On 2011/08/30 14:33:12, TomH wrote:
> Is the right place for this here, or next to where we declare
> gDegenerateToLineTol?

I moved gDegenerateToLineTol to be local. I originally was using it in another
function as well, but not anymore.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.cpp
File gpu/src/GrDrawTarget.cpp (right):

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:91: }
On 2011/08/30 14:33:12, TomH wrote:
> Could we now just do size += num_text_coords(vertexLayout) * vecSize?

Done.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:146: int offset = vecSize *
(num_tex_coords(vertexLayout) + 1); //+1 for pos
On 2011/08/30 14:33:12, TomH wrote:
> It's a little thing, but is it worthwhile to call VertexColorOffset here and
> avoid duplicating the code?
It'll return -1 if there is no per-vertex color. We could do something like make
a helper that returns where the vertex color offset would be if it was present.
Then VertexColorOffset and this function could use it. I'll leave that to a
future change.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.h
File gpu/src/GrDrawTarget.h (right):

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.h#newcode64
gpu/src/GrDrawTarget.h:64: * edges are in use. The edges are always 4 component,
even when they aren't
On 2011/08/30 14:33:12, TomH wrote:
> ...even when not all 4 components are needed?
> 
> Also, neither of our current types require 4 components. Why have them - pad?
> some well-known near-term requirement? binary compatibility with possible
future
> requirement?

I wanted to keep it the same for all edge types to avoid further complicating
the vertex layout calculations. It seemed weird to align at 3 scalars (as
required by lines). Also, leaves room for expressing an edge as a line segment
using pt + vector. I originally did this for the hair lines, bloated the ends of
the line segment, and computed pt-pt distance when the pixel center was outside
of the two endpts. We might consider doing it that way in the future, though its
more math per fragment.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrDrawTarget.h#newcod...
gpu/src/GrDrawTarget.h:567: * is set.
On 2011/08/30 14:33:12, TomH wrote:
> Can we assert, or check? "Only used" isn't clear - is that an expected
> constraint on callers?

It's just informational. If the caller isn't specifying per-vertex edge data
then the setting has no effect. I clarified the comment.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:807: // use canonical value when not set to avoid
cache misses
On 2011/08/30 14:33:12, TomH wrote:
> comment not clear to the uninitiated, i.e. me
I added a meta-comment at the top of the function.

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrPathRenderer.h
File gpu/src/GrPathRenderer.h (right):

http://codereview.appspot.com/4926045/diff/7008/gpu/src/GrPathRenderer.h#newc...
gpu/src/GrPathRenderer.h:213: // and just before the path is cleared.
On 2011/08/30 14:33:12, TomH wrote:
> ... and needs to call these after the path is set / before the path is
cleared?
> This seems a bit tricky, since in just reading the GrAAHairlinePathRenderer
> constructor it isn't obvious that the path will clear after the constructor
> exits.

The base class calls these notifiers when it sees a setPath / clearPath. There
is no requirement for the subclass to call them. I just called it from the
GrAAHair..er cons to avoid retyping common code. I added a separate function
that is now called from the cons and pathWillClear().

Checked in at r2196.

Expand All Messages | Collapse All Messages

Issue 4926045: Add AA hairline path renderer (Closed)

Patch Set 1 #

Patch Set 2 : update #

Patch Set 3 : #if->#ifdef and fix endlines #

Patch Set 4 : remove unused label #

Patch Set 5 : only test quadtric code in program unit test when deriv funcs available #

Patch Set 6 : cleanup #

Patch Set 7 : response to comments and fix var name in debug assert #

Patch Set 8 : incorporate comments #

Patch Set 9 : incorporate comments, rename SampleHairCubics, fix lines vertex order bug #

Patch Set 10 : incorporate comments #

Patch Set 11 : incorporate comments #

Messages