Tesselate path once for tiled offscreen AA

[reed1, 2011-06-30 19:31:30]

Is it reasonable to have a block comment at the top that illustrates the
life-cycle of a pathrenderer?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrMemory.h
File gpu/include/GrMemory.h (left):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrMemory.h#oldcod...
gpu/include/GrMemory.h:171: template <int COUNT, typename T>
Is there an Sk equivalent? Does it have the same semantics? Can they be
combined/shared?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h
File gpu/include/GrPathRenderer.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:97: 
Ick ick. Do we really need this blind-long-term-pointer/reference pattern? 

1. perhaps SkPath needs a ref'd shallow copy (like region and string)
2. create refcnt wrapper for path+fill?
3. will I like it better if you pass const SkPath* instead of const SkPath& ?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrScalar.h
File gpu/include/GrScalar.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrScalar.h#newcode42
gpu/include/GrScalar.h:42: #define GrMul(a,b)          SkScalarMul(a,b) //
deprecated, prefer GrScalarMul
prefer SkScalarMul ?

[Stephen White, 2011-06-30 19:43:34]

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
File gpu/include/GrTesselatedPathRenderer.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
gpu/include/GrTesselatedPathRenderer.h:30: virtual void removePath();
Maybe "clearPath" would be a better name?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
gpu/include/GrTesselatedPathRenderer.h:44: GrDrawTarget*   fTarget;
Seems a shame that these members are duplicated on both the Default and
Tesselated flavours.  Could they be moved to the base class?

Alternatively, perhaps there could be a "canSetPath()" virtual that the Default
renderer would return true for, and the Tess would not?  (since presumably the
latter never gets called for drawing more than once anyway).  Then the Tess
could continue to have a drawPath() interface, and not support the setPath(). 
Just a thought -- I don't have strong feelings about this.

[TomH, 2011-06-30 20:41:17]

Mostly nits from me; I don't have a problem with anything. Looking forward to
having this roll in.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h
File gpu/include/GrPathRenderer.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:79: * and drawPathToStencil will be occur between
setPath and resetPath. The
typo 'will be occur'.
Maybe 'must be made'?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:80: * path will not be modified externally between
setPath and resetPath. The
will -> must?

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:88: * @param path                  the path t0
draw.
t0 -> to

http://codereview.appspot.com/4661062/diff/1001/gpu/src/GrPathRenderer.cpp
File gpu/src/GrPathRenderer.cpp (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/src/GrPathRenderer.cpp#ne...
gpu/src/GrPathRenderer.cpp:220: void GrDefaultPathRenderer::subdivideContours(
I thought the name in the draft CL made more sense. From the caller's point of
view, isn't this creating vertex buffers / tesselating curves / ...?

[bsalomon, 2011-07-01 14:30:52]

New version, PTAL.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrMemory.h
File gpu/include/GrMemory.h (left):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrMemory.h#oldcod...
gpu/include/GrMemory.h:171: template <int COUNT, typename T>
On 2011/06/30 19:31:30, reed1 wrote:
> Is there an Sk equivalent? Does it have the same semantics? Can they be
> combined/shared?
Done in another CL

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h
File gpu/include/GrPathRenderer.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:79: * and drawPathToStencil will be occur between
setPath and resetPath. The
On 2011/06/30 20:41:18, TomH wrote:
> typo 'will be occur'.
> Maybe 'must be made'?

Done.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:80: * path will not be modified externally between
setPath and resetPath. The
On 2011/06/30 20:41:18, TomH wrote:
> will -> must?

Done.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:88: * @param path                  the path t0
draw.
On 2011/06/30 20:41:18, TomH wrote:
> t0 -> to

Done.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrPathRenderer.h#...
gpu/include/GrPathRenderer.h:97: 
On 2011/06/30 19:31:30, reed1 wrote:
> Ick ick. Do we really need this blind-long-term-pointer/reference pattern? 
> 
> 1. perhaps SkPath needs a ref'd shallow copy (like region and string)
> 2. create refcnt wrapper for path+fill?
> 3. will I like it better if you pass const SkPath* instead of const SkPath& ?

I changes it to const SkPath*, like it better? :) 

I'm not sure I really get how 1 would work. What happens if the original path
object is modified? It seems like you'd have to have copy-on-write to enforce
that the ref'd copy was really immutable.

There is a GEN ID on path that seems to only be implemented in ANDROID. We could
consider using it to assert that the path is unchanged between setPath,
clearPath.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrScalar.h
File gpu/include/GrScalar.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrScalar.h#newcode42
gpu/include/GrScalar.h:42: #define GrMul(a,b)          SkScalarMul(a,b) //
deprecated, prefer GrScalarMul
On 2011/06/30 19:31:30, reed1 wrote:
> prefer SkScalarMul ?

I'd rather not just start mixing SkScalar* ops in with existing code that uses
GrScalar*. If (when) we switch to SkScalar I'd rather have it all go in one
global CL.

I made the call site this CL introduces just use GrMul.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
File gpu/include/GrTesselatedPathRenderer.h (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
gpu/include/GrTesselatedPathRenderer.h:30: virtual void removePath();
On 2011/06/30 19:43:34, Stephen White wrote:
> Maybe "clearPath" would be a better name?

Done.

http://codereview.appspot.com/4661062/diff/1001/gpu/include/GrTesselatedPathR...
gpu/include/GrTesselatedPathRenderer.h:44: GrDrawTarget*   fTarget;
On 2011/06/30 19:43:34, Stephen White wrote:
> Seems a shame that these members are duplicated on both the Default and
> Tesselated flavours.  Could they be moved to the base class?

Done, the parent class now implements setPath and clearPath. The subclasses can
chose to be notified if they want. The default PR gets notified on clearPath and
the Tess PR doesn't need to be notified so it isn't.

> 
> Alternatively, perhaps there could be a "canSetPath()" virtual that the
Default
> renderer would return true for, and the Tess would not?  (since presumably the
> latter never gets called for drawing more than once anyway).  Then the Tess
> could continue to have a drawPath() interface, and not support the setPath(). 
> Just a thought -- I don't have strong feelings about this.

I think the above pattern still makes it easy enough to implement a subclass
without forcing the call site to do additional checks.

http://codereview.appspot.com/4661062/diff/1001/gpu/src/GrPathRenderer.cpp
File gpu/src/GrPathRenderer.cpp (right):

http://codereview.appspot.com/4661062/diff/1001/gpu/src/GrPathRenderer.cpp#ne...
gpu/src/GrPathRenderer.cpp:220: void GrDefaultPathRenderer::subdivideContours(
On 2011/06/30 20:41:18, TomH wrote:
> I thought the name in the draft CL made more sense. From the caller's point of
> view, isn't this creating vertex buffers / tesselating curves / ...?

I had changed it to createGeom(). (I just wanted to remove the word Verts from
the name since we will likely also be computing indices in a upcoming CL).

[TomH, 2011-07-01 14:37:25]

LGTM

[bsalomon, 2011-07-01 15:00:07]

Closing with r1777. (Mike gave a verbal LGTM.)

Stephen, I went ahead and submitted since you're out today and I won't be in
until Wed. I'm pretty sure I addressed all your comments, but I'm happy to
accommodate any concerns.