Improve edge AA performance

Batch up draws into triangle fans as large as possible when drawing convex edge AA polys, so we minimize state changes and GL calls.  This requires querying GL for the maximum
number of fragment uniforms.  It also makes the shader generator produce custom shaders for the number of relevant edges.  This gives a ~5X speedup on the "Shapes" SampleApp.

[bsalomon, 2011-05-18 21:00:54]

Overall looks like a good change. The performance gains sound great!

In one of the comments I mention "ProgramUnitTest" It's something I should have
mentioned before. We have a unit test that can be run from GrGpuGLShaders
constructor by changing this #if 0 to a 1:

#if 0
    ProgramUnitTest();
#endif

It generates random params for the ProgramDesc and builds the shader. We should
check this when we change the shader code (in a debug build to catch asserts).
We'll be able to build against osmesa on the skia bot soon and at that point
I'll make a gr unit test program that we can run per-checkin that runs this.

Also I mentioned in the inline comments that the edge data will need to be ref
counted in the DrState for deferred drawing. I'd hate to make your loop allocate
a new array per iteration. I think we can assume that if the ref-count is 1
after you call drawIndexed() that the previous array has been consumed and can
be reused. Right now that'll be true all the time but in the future the target
param could be a different subclass of GrDrawTarget. We'll need to update
GrInOrderDrawBuffer to ref and unref the edge data when it saves / deletes a
DrState.

http://codereview.appspot.com/4536070/diff/1/gpu/include/GrDrawTarget.h
File gpu/include/GrDrawTarget.h (right):

http://codereview.appspot.com/4536070/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:177: const Edge             *fEdgeAAEdges;
We make copies of this struct when rendering is deferred. If we're not storing
the data here then it needs to be in a ref-counted object.

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGLProgram.cpp#newcode364
gpu/src/GrGLProgram.cpp:364: if (fProgramDesc.fEdgeAANumEdges > 0) {
I had to move this block to line 463 to pass the the ProgramUnitTest locally.

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

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:446: Edge* edges = new Edge[count];
Maybe we should have a statically determined max so we can avoid the new and
delete?  It could be some number larger than what we expect to actually be
available in practice.

The getMaxEdges() call would have to clamp to it.

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrTesselatedPathRenderer...
File gpu/src/GrTesselatedPathRenderer.cpp (right):

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrTesselatedPathRenderer...
gpu/src/GrTesselatedPathRenderer.cpp:278: target->setIndexSourceToArray(indices,
j);
Might be able to squeeze out some more performance here by using one index array
set outside the loop and using the startVertex param to offset into the vertex
array

http://codereview.appspot.com/4536070/diff/1/src/gpu/GrPrintf_skia.cpp
File src/gpu/GrPrintf_skia.cpp (right):

http://codereview.appspot.com/4536070/diff/1/src/gpu/GrPrintf_skia.cpp#newcode26
src/gpu/GrPrintf_skia.cpp:26: const size_t MAX_BUFFER_SIZE = 2048;
+1 :)

[Stephen White, 2011-05-18 22:11:06]

Thanks for the review.  New version incoming.

On 2011/05/18 21:00:54, bsalomon wrote:
> Overall looks like a good change. The performance gains sound great!
> 
> In one of the comments I mention "ProgramUnitTest" It's something I should
have
> mentioned before. We have a unit test that can be run from GrGpuGLShaders
> constructor by changing this #if 0 to a 1:
> 
> #if 0
>     ProgramUnitTest();
> #endif
> 
> It generates random params for the ProgramDesc and builds the shader. We
should
> check this when we change the shader code (in a debug build to catch asserts).

Will do, thanks.

> We'll be able to build against osmesa on the skia bot soon and at that point
> I'll make a gr unit test program that we can run per-checkin that runs this.
> 
> Also I mentioned in the inline comments that the edge data will need to be ref
> counted in the DrState for deferred drawing. I'd hate to make your loop
allocate
> a new array per iteration. I think we can assume that if the ref-count is 1
> after you call drawIndexed() that the previous array has been consumed and can
> be reused. Right now that'll be true all the time but in the future the target
> param could be a different subclass of GrDrawTarget. We'll need to update
> GrInOrderDrawBuffer to ref and unref the edge data when it saves / deletes a
> DrState.

Since I've changed it to have an absolute max size (see below), I've made it
simply memcpy() the supplied data as it was before.

http://codereview.appspot.com/4536070/diff/1/gpu/include/GrDrawTarget.h
> File gpu/include/GrDrawTarget.h (right):
> 
>
http://codereview.appspot.com/4536070/diff/1/gpu/include/GrDrawTarget.h#newco...
> gpu/include/GrDrawTarget.h:177: const Edge             *fEdgeAAEdges;
> We make copies of this struct when rendering is deferred. If we're not storing
> the data here then it needs to be in a ref-counted object.

See above.

> http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGLProgram.cpp
> File gpu/src/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGLProgram.cpp#newcode364
> gpu/src/GrGLProgram.cpp:364: if (fProgramDesc.fEdgeAANumEdges > 0) {
> I had to move this block to line 463 to pass the the ProgramUnitTest locally.

Fixed, thanks.

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGpuGLShaders.cpp
> File gpu/src/GrGpuGLShaders.cpp (right):
> 
>
http://codereview.appspot.com/4536070/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
> gpu/src/GrGpuGLShaders.cpp:446: Edge* edges = new Edge[count];
> Maybe we should have a statically determined max so we can avoid the new and
> delete?  It could be some number larger than what we expect to actually be
> available in practice.

Done.  I actually made it somewhat smaller, since desktop machines have upwards
of 512 vectors, which is probably overkill.

> The getMaxEdges() call would have to clamp to it.

Done.

http://codereview.appspot.com/4536070/diff/1/gpu/src/GrTesselatedPathRenderer...
> File gpu/src/GrTesselatedPathRenderer.cpp (right):
> 
>
http://codereview.appspot.com/4536070/diff/1/gpu/src/GrTesselatedPathRenderer...
> gpu/src/GrTesselatedPathRenderer.cpp:278:
target->setIndexSourceToArray(indices,
> j);
> Might be able to squeeze out some more performance here by using one index
array
> set outside the loop and using the startVertex param to offset into the vertex
> array

Actually, I don't think I can, since I need to re-specify index 0 each time to
anchor each fan at the same vert.  But you made me realize I don't really need
to have indexed data at all, since I can just clobber the base vert with vert 0
(in the same way that I was the base edge with edge 0).

http://codereview.appspot.com/4536070/diff/1/src/gpu/GrPrintf_skia.cpp
> File src/gpu/GrPrintf_skia.cpp (right):
> 
>
http://codereview.appspot.com/4536070/diff/1/src/gpu/GrPrintf_skia.cpp#newcode26
> src/gpu/GrPrintf_skia.cpp:26: const size_t MAX_BUFFER_SIZE = 2048;
> +1 :)

[Stephen White, 2011-05-18 22:11:42]

Updated as per review comments

[bsalomon, 2011-05-19 13:54:22]

On 2011/05/18 22:11:42, Stephen White wrote:
> Updated as per review comments

I gotcha about the first vertex. Now I think your first version might be better
simply because the index data will be more compact than the vertex data if we
have to reupload something. I don't mean to hold up the change over this, the
difference might not matter at all to the performance.

There's a couple place where you use GR_CT_MIN.  That guy was added for places
where the templated function GrMin() wouldn't work because we needed a
compile-time (CT) value (e.g. template parameter). I think regular GrMin should
work for you. It sometimes requires an explicit template parameter if the two
params don't agree in signedness or one is a enum value.

Otherwise, LGTM.

[Stephen White, 2011-05-19 14:22:45]

On 2011/05/19 13:54:22, bsalomon wrote:
> On 2011/05/18 22:11:42, Stephen White wrote:
> > Updated as per review comments
> 
> I gotcha about the first vertex. Now I think your first version might be
better
> simply because the index data will be more compact than the vertex data if we
> have to reupload something. I don't mean to hold up the change over this, the
> difference might not matter at all to the performance.

My thinking is that in the first case, you have to upload all of the verts
anyway, plus the indices.  In the second case, you only upload verts.  So while
there is some overlap in the verts (2 per fan), there's also overlap in the
indices in the first version.  Hmm, let me work it out:

First version:  count * sizeof(vert) + (count + numFans * 2) * sizeof(index)
= count * 8 + (count + numFans * 2) * 2
= count * 8 + count * 2 + numFans * 4

Second version:  (count + numFans * 2)) * sizeof(vert)
= (count + numFans * 2) * 8
= count * 8 + numFans * 16

So the second one is better if 
count * 8 + numFans * 16 < count * 8 + count * 2 + numFans * 4
numFans * 16 < count * 2 + numFans * 4
numFans * 12 < count * 2
numFans * 6 < count

numFans is count / maxEdges (roughly), so
(count / maxEdges) * 6 < count
6 / maxEdges < 1
maxEdges > 6.

> There's a couple place where you use GR_CT_MIN.  That guy was added for places
> where the templated function GrMin() wouldn't work because we needed a
> compile-time (CT) value (e.g. template parameter). I think regular GrMin
should
> work for you. It sometimes requires an explicit template parameter if the two
> params don't agree in signedness or one is a enum value.

Yes, one of them was an enum value, and GrMin failed me.  I think maybe the
other one I can fix.
> 
> Otherwise, LGTM.

[Stephen White, 2011-05-19 16:13:40]

On 2011/05/19 14:22:45, Stephen White wrote:
> Yes, one of them was an enum value, and GrMin failed me.  I think maybe the
> other one I can fix.

Turns out the other one is a size_t vs an int, which GrMin doesn't like either. 
So I'm going to land this as-is, if that's ok.

> > 
> > Otherwise, LGTM.

[bsalomon, 2011-05-19 17:01:26]

On Thu, May 19, 2011 at 12:13 PM, <senorblanco@chromium.org> wrote:

> On 2011/05/19 14:22:45, Stephen White wrote:
>
>> Yes, one of them was an enum value, and GrMin failed me.  I think
>>
> maybe the
>
>> other one I can fix.
>>
>
> Turns out the other one is a size_t vs an int, which GrMin doesn't like
> either.  So I'm going to land this as-is, if that's ok.
>
> Another option is GrMin<size_t>(a,b) but either way is fine.

[Stephen White, 2011-05-19 18:36:22]

Landed as r1380, closing.