Added stack to AA clip mask cache

First step in making AA clip mask cache make use of Skia save/restore information to better cache clip masks

[bsalomon, 2012-05-08 13:15:15]

Mostly LGTM

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h
File src/gpu/GrClipMaskManager.h (right):

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h#newc...
src/gpu/GrClipMaskManager.h:100: void push() {
Maybe a comment about what the state is after the push (entirely open clip I
suppose?)

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h#newc...
src/gpu/GrClipMaskManager.h:204: fLastWidth = -1;
Could use a comment about the difference between the three rectangles (last
width x last height), texture dims, and last bound.

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h#newc...
src/gpu/GrClipMaskManager.h:218: SkTArray<GrClipStackFrame*, false> fStack;
In many places where we have a dyn array we seed it with some small amount of
pre-allocated space so that we don't have frequent new/delete calls. It might
make sense here. Check out SkSTArray subclass of SkTArray.

[reed1, 2012-05-08 13:23:06]

Might consider using SkDeque instead of an array. It has the (potential)
advantage of not insisting on contiguous allocations, so that growing never
involves a copy. It also makes it easy to do in-place initialization, as we do
in SkCanvas's stack, so that we're never calling new per element.

Can we write a unittest for your new API?

[robertphillips, 2012-05-08 18:26:00]

This version adds a unit test, adds comments and fixes a MakeEmpty for setEmpty
bug.

W.r.t. the container, I was actually hoping to use a new derivative of SkTDStack
(i.e., SkTStack). It seems like this would address both your (and my concerns)
since it would have a little bit of extra memory (i.e., the 8 slot record) to
reduce the number of allocations but also eliminate the copies (by chaining
together the records). Thoughts?

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h
File src/gpu/GrClipMaskManager.h (right):

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h#newc...
src/gpu/GrClipMaskManager.h:100: void push() {
On 2012/05/08 13:15:15, bsalomon wrote:
> Maybe a comment about what the state is after the push (entirely open clip I
> suppose?) 

Done.

http://codereview.appspot.com/6201058/diff/1/src/gpu/GrClipMaskManager.h#newc...
src/gpu/GrClipMaskManager.h:204: fLastWidth = -1;
On 2012/05/08 13:15:15, bsalomon wrote:
> Could use a comment about the difference between the three rectangles (last
> width x last height), texture dims, and last bound.
> 

Done.

[bsalomon, 2012-05-08 18:55:32]

LGTM

>W.r.t. the container, I was actually hoping to use a new derivative of
SkTDStack
>(i.e., SkTStack). It seems like this would address both your (and my concerns)
>since it would have a little bit of extra memory (i.e., the 8 slot record) to
>reduce the number of allocations but also eliminate the copies (by chaining
>together the records). Thoughts?

There are probably other places where we are unnecessarily enforcing contiguity
by using an array.

http://codereview.appspot.com/6201058/diff/6001/tests/ClipCacheTest.cpp
File tests/ClipCacheTest.cpp (right):

http://codereview.appspot.com/6201058/diff/6001/tests/ClipCacheTest.cpp#newco...
tests/ClipCacheTest.cpp:32: GrTexture* texture =
context->createUncachedTexture(desc, textureData, 0);
We should have texture->clear(<color>) as a helper method (not as part of this
CL, just a general comment).

[robertphillips, 2012-05-08 19:27:38]

Replaced SkTArray with a SkDeque in GrClipMaskCache

[bsalomon, 2012-05-08 21:39:55]

On 2012/05/08 19:27:38, robertphillips wrote:
> Replaced SkTArray with a SkDeque in GrClipMaskCache

It feels like we're abusing the notion of an Sk"Auto"TUnref. It isn't really
auto if we have to remember to call the struct's destructor so that it can call
SkAutoTUnref's destructor. I could be wrong but maybe making the struct really
be POD and moving the ref/unref burden to the outer class would be cleaner. Feel
free to ignore this suggestion if you disagree :)

[reed1, 2012-05-08 21:42:44]

On 2012/05/08 21:39:55, bsalomon wrote:
> On 2012/05/08 19:27:38, robertphillips wrote:
> > Replaced SkTArray with a SkDeque in GrClipMaskCache
> 
> It feels like we're abusing the notion of an Sk"Auto"TUnref. It isn't really
> auto if we have to remember to call the struct's destructor so that it can
call
> SkAutoTUnref's destructor. I could be wrong but maybe making the struct really
> be POD and moving the ref/unref burden to the outer class would be cleaner.
Feel
> free to ignore this suggestion if you disagree :)

+1

[robertphillips, 2012-05-09 21:16:23]

Received verbal LGTM from Brian & Mike

committed as r3885