Refactor GrDrawTarget vertex/index api

Separate API calls to reserve verts/indices.
Make drawing nestable by push/pop geom mechanism on draw target.
Many other related smaller improvements.

[TomH, 2011-06-20 17:58:09]

I did some more reading of the Pool since lunch, but still not very confident I
grok all this.

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

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:773: * Resets vertex sources. Indexed Drawing from
reset indices is illegal. Set
typo - index

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:1176: kReserved_GeometrySrcType, //<! src was set
using reserveAndLockGeometry
obsolete comment?

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:1226: virtual void releaseReservedIndexSpace() = 0;
I liked the consistency of naming onFoo() things that a subclass must implement,
but it's broken by release() and geometry() here

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

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrGpu.h#newcode538
gpu/include/GrGpu.h:538: int vertexCount);
changing these from protected: to private:?

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

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrGpu.cpp#newcode590
gpu/src/GrGpu.cpp:590: newState.fPoolStartIndex = ~0;
Is it worth naming a symbolic constant for all of these ~0?

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

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrInOrderDrawBuffer.cpp#...
gpu/src/GrInOrderDrawBuffer.cpp:230:
VertexSize(this->getGeomSrc().fVertexLayout);
Can we not mix vertex layouts within a pool/draw?

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrInOrderDrawBuffer.cpp#...
gpu/src/GrInOrderDrawBuffer.cpp:295: case kArray_GeometrySrcType: {
OK, elsewhere I thought I'd understood the difference between Buffer and Array,
but reading this I think I don't understand at all. Are there better names?

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

http://codereview.appspot.com/4631056/diff/4001/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:622: }
Pop releases a couple of buffers that aren't acquired during push(); are we
guaranteed that they were acquired somewhere?

[bsalomon, 2011-06-20 18:43:00]

I think you have the second best understanding of how this code works by a long
shot. :)

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

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:773: * Resets vertex sources. Indexed Drawing from
reset indices is illegal. Set
On 2011/06/20 17:58:09, TomH wrote:
> typo - index

Done.

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrDrawTarget.h#newco...
gpu/include/GrDrawTarget.h:1176: kReserved_GeometrySrcType, //<! src was set
using reserveAndLockGeometry
On 2011/06/20 17:58:09, TomH wrote:
> obsolete comment?

Done.

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

http://codereview.appspot.com/4631056/diff/1/gpu/include/GrGpu.h#newcode538
gpu/include/GrGpu.h:538: int vertexCount);
On 2011/06/20 17:58:09, TomH wrote:
> changing these from protected: to private:?
Yeah, subclasses of GrGpu have to implement onGpuDraw*. So call stack would be:

GrGpuGL::onGpuDrawIndexed(...)
GrGpu::onDrawIndexed(...)
GrDrawTarget::drawIndexed(...)

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

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrGpu.cpp#newcode590
gpu/src/GrGpu.cpp:590: newState.fPoolStartIndex = ~0;
On 2011/06/20 17:58:09, TomH wrote:
> Is it worth naming a symbolic constant for all of these ~0?

Done.

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

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrInOrderDrawBuffer.cpp#...
gpu/src/GrInOrderDrawBuffer.cpp:230:
VertexSize(this->getGeomSrc().fVertexLayout);
On 2011/06/20 17:58:09, TomH wrote:
> Can we not mix vertex layouts within a pool/draw?

Currently a vertex source is specified the layout is specified along with it. So
all draws have to use the same layout until the source changes (either
respecified at same stack level or after pushGeometrySource). I thought about
relaxing that but then decided the change was big enough.

The underlying vertex pool used by GrInOrderDrawBuffer and GrGpu can support
mixed layouts in a buffer (either because of nested pushes or accumulation of
draws when deferring).

http://codereview.appspot.com/4631056/diff/1/gpu/src/GrInOrderDrawBuffer.cpp#...
gpu/src/GrInOrderDrawBuffer.cpp:295: case kArray_GeometrySrcType: {
On 2011/06/20 17:58:09, TomH wrote:
> OK, elsewhere I thought I'd understood the difference between Buffer and
Array,
> but reading this I think I don't understand at all. Are there better names?

Buffer is an object allocated by the GPU driver. Array is an array of CPU
accessible memory (malloc). We could rename them to GPUBuffer and CPUArray or
something like that.

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

http://codereview.appspot.com/4631056/diff/4001/gpu/src/GrDrawTarget.cpp#newc...
gpu/src/GrDrawTarget.cpp:622: }
On 2011/06/20 17:58:09, TomH wrote:
> Pop releases a couple of buffers that aren't acquired during push(); are we
> guaranteed that they were acquired somewhere?

After a push we start off with kNone as the source type. If caller then sets src
to a buffer we take refs at that point (set*SourceToBuffer). If they haven't
unset the buffer (by setting a different source or reset*Source()) then at pop
time we have to unref it.

[TomH, 2011-06-21 15:26:52]

LGTM

[bsalomon, 2011-06-21 18:55:25]

Closed with r1662.