Add GrDrawTarget::drawIndexedInstances

This allows small text draws to be concatenated. It also may help some types of path rendering, though that hasn't been hooked up in this change.

[bsalomon, 2012-03-20 15:21:07]

GrDrawTarget.h is the place to start with this CL.

[TomH, 2012-03-20 15:58:32]

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

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:266: * specified it can be used for multiple draw.
However, the time at which
nit: draws? draw calls?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:510: * (k+1)V-1. As a concrete example, the folliwng
index buffer for drawing
nit: following

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:513: *      (0,1,2,0,2,3, 4,5,6,4,6,7, 8,9,10,8,10,11,
...)
OK, this is hard to read, but the example saves it.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:521: * except instanceCount are identical among the
sucessive calls.
This paragraph is unclear. We'd want to work on that before writing user doc,
but it's probably sufficient for now.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:524: * setIndexSourceToBuffer(indexBuffer) had been
called.
Hmm, seems like we're ignoring part of the API? Why not require
sISTB(indexBuffer), and reduce the size of the signature?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:227: }
Do we want to do this earlier to avoid side effects of aborted calls?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:241: GrAssert(geomSrc.fIndexBuffer ==
indexBuffer);
Is there any way this wouldn't be true after line 223?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:243: // how many instances can be in a single
draw
Can we GrAssert(indexBuffer->sizeInBytes() > 0)? otherwise some nasty infinite
loop possibility later...

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:245: (sizeof(uint16_t) * indicesPerInstance);
So EGT went through some pain when they had to convert indices from 16b to 32b.
Do we want to use a typedef here?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:264: vertexBuffer->ref();
Ugh - manually reffing these because you're storing them in a dumb pointer on
the POD Draw struct, but they're ref-counted underneath?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:289: int instancesToDraw =
GrMin(instancesLeftInCurrDraw, instancesPerDraw);
Meta: it's disorienting that we're using Draw as both verb and noun.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:299: draw->fVertexBuffer = vertexBuffer;
So if you pass through this loop multiple times are you taking multiple refs on
each buffer?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:338: this->resetDrawTracking();
Umm - shouldn't we be flushing any pending draws?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.h
File src/gpu/GrInOrderDrawBuffer.h (right):

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.h...
src/gpu/GrInOrderDrawBuffer.h:180: // call to invalidate tracking data the
attempts to concatenate draws
nit: grammar

http://codereview.appspot.com/5848064/diff/1008/src/gpu/GrDefaultTextContext.cpp
File src/gpu/GrDefaultTextContext.cpp (right):

http://codereview.appspot.com/5848064/diff/1008/src/gpu/GrDefaultTextContext....
src/gpu/GrDefaultTextContext.cpp:58: fCurrVertex >> 2,
Assume this is some sort of vert index -> quad index conversion? >>2 is opaque

[bsalomon, 2012-03-20 19:02:29]

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

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:510: * (k+1)V-1. As a concrete example, the folliwng
index buffer for drawing
On 2012/03/20 15:58:32, TomH wrote:
> nit: following

Done.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:524: * setIndexSourceToBuffer(indexBuffer) had been
called.
On 2012/03/20 15:58:32, TomH wrote:
> Hmm, seems like we're ignoring part of the API? Why not require
> sISTB(indexBuffer), and reduce the size of the signature?

This is a really good point. It was nagging me as well. I made the call work
with any index source (set beforehand using setIndexSourceToArray,
setIndexSourceToBuffer or reserveVertexAndIndexSpace).

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:227: }
On 2012/03/20 15:58:32, TomH wrote:
> Do we want to do this earlier to avoid side effects of aborted calls?

Done.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:241: GrAssert(geomSrc.fIndexBuffer ==
indexBuffer);
On 2012/03/20 15:58:32, TomH wrote:
> Is there any way this wouldn't be true after line 223?
N/A in updated patch.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:243: // how many instances can be in a single
draw
On 2012/03/20 15:58:32, TomH wrote:
> Can we GrAssert(indexBuffer->sizeInBytes() > 0)? otherwise some nasty infinite
> loop possibility later...
good point.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:245: (sizeof(uint16_t) * indicesPerInstance);
On 2012/03/20 15:58:32, TomH wrote:
> So EGT went through some pain when they had to convert indices from 16b to
32b.
> Do we want to use a typedef here?

We already assume this in so many places that I don't think one more will kill
us. (Referenced code is moved to GrDrawTarget.h in latest patch).

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:264: vertexBuffer->ref();
On 2012/03/20 15:58:32, TomH wrote:
> Ugh - manually reffing these because you're storing them in a dumb pointer on
> the POD Draw struct, but they're ref-counted underneath?

Correct. :(

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:289: int instancesToDraw =
GrMin(instancesLeftInCurrDraw, instancesPerDraw);
On 2012/03/20 15:58:32, TomH wrote:
> Meta: it's disorienting that we're using Draw as both verb and noun.

Agreed. I've tried rewriting the various counts several different ways and have
never been satisfied with the names, the number of them, or the number of
divides. Maybe the update version is slightly better?

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:299: draw->fVertexBuffer = vertexBuffer;
On 2012/03/20 15:58:32, TomH wrote:
> So if you pass through this loop multiple times are you taking multiple refs
on
> each buffer?

The playback unrefs after every draw. It doesn't know how the draws were
inserted into the queue.

http://codereview.appspot.com/5848064/diff/6011/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:338: this->resetDrawTracking();
On 2012/03/20 15:58:32, TomH wrote:
> Umm - shouldn't we be flushing any pending draws?

Definitely not. This class doesn't actually draw anything. All it does is
accumulate draws and play them back into another GrDrawTarget. The
resetDrawTracking() here is to prevent incorrect draw concatenations (combining
two draws into one).

http://codereview.appspot.com/5848064/diff/1008/src/gpu/GrDefaultTextContext.cpp
File src/gpu/GrDefaultTextContext.cpp (right):

http://codereview.appspot.com/5848064/diff/1008/src/gpu/GrDefaultTextContext....
src/gpu/GrDefaultTextContext.cpp:58: fCurrVertex >> 2,
On 2012/03/20 15:58:32, TomH wrote:
> Assume this is some sort of vert index -> quad index conversion? >>2 is opaque

Done.

[TomH, 2012-03-20 20:39:53]

LGTM

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

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:1025: indicesPerInstance;
Nice how this resolved so cleanly.

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

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:521: * the calls.
If we can do it concisely, this paragraph really needs more concrete examples
like the one above. I'm not confident that I understand sentences 2-3 yet.

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:218: kBuffer_GeometrySrcType ==
geomSrc.fIndexSrc) {
Change from before, right? Was this a bug? Or just because of new semantics w/
using index source & vertex source?

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.h
File src/gpu/GrInOrderDrawBuffer.h (right):

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.h...
src/gpu/GrInOrderDrawBuffer.h:216: fIndicesPerInstance= 0;
nit: space before =

[bsalomon, 2012-03-20 21:19:08]

Closed with r3444.

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

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:521: * the calls.
On 2012/03/20 20:39:53, TomH wrote:
> If we can do it concisely, this paragraph really needs more concrete examples
> like the one above. I'm not confident that I understand sentences 2-3 yet.

I took one more stab at it in the checked in version.

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (right):

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.c...
src/gpu/GrInOrderDrawBuffer.cpp:218: kBuffer_GeometrySrcType ==
geomSrc.fIndexSrc) {
On 2012/03/20 20:39:53, TomH wrote:
> Change from before, right? Was this a bug? Or just because of new semantics w/
> using index source & vertex source?

Just because of the new semantics. The code below only works correctly when the
index source is buffer. It probably could be made to work generally but I'd
rather cross that bridge when we get there.

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.h
File src/gpu/GrInOrderDrawBuffer.h (right):

http://codereview.appspot.com/5848064/diff/8003/src/gpu/GrInOrderDrawBuffer.h...
src/gpu/GrInOrderDrawBuffer.h:216: fIndicesPerInstance= 0;
On 2012/03/20 20:39:53, TomH wrote:
> nit: space before =

Done.