Implement instanced rendering for simple shapes

Implement instanced rendering for simple shapes

<TODO>

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1897203002

[Chris Dalton, 2016-04-19 09:47:27]

Description was changed from

==========
Implement instanced rendering for simple shapes

<TODO>

BUG=skia:
==========

to

==========
Implement instanced rendering for simple shapes

<TODO>

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-04-19 09:50:32]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, robertphillips@google.com

[Chris Dalton, 2016-04-19 09:50:33]

https://codereview.chromium.org/1897203002/diff/1/src/gpu/GrDrawingManager.cpp
File src/gpu/GrDrawingManager.cpp (right):

https://codereview.chromium.org/1897203002/diff/1/src/gpu/GrDrawingManager.cp...
src/gpu/GrDrawingManager.cpp:48: }
This is a bit of a cop out. Should we add abandon/release methods to
GrDrawTarget?

[Chris Dalton, 2016-04-19 09:51:46]

I remember hearing you can run trybots and pre-approve images. Is that right?
Can someone show me how that works?

[Kimmo Kinnunen, 2016-04-19 10:21:05]

On 2016/04/19 09:51:46, Chris Dalton wrote:
> I remember hearing you can run trybots and pre-approve images. Is that right?
> Can someone show me how that works?

I think you can click on the gold link above to get the images. I don't know
about the approval, maybe you can take it from there.
The link does not work until the results are available, as far as I understand.

[Kimmo Kinnunen, 2016-04-19 13:34:51]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 13:34:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1897203002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1897203002/1

[commit-bot: I haz the power, 2016-04-19 13:34:58]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-04-19 19:34 UTC

[commit-bot: I haz the power, 2016-04-19 13:38:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 13:38:07]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bsalomon, 2016-04-25 13:22:39]

This is really nicely integrated. I'd like to get Rob's opinion on interaction
with having draw targets per RT rather than a unified DT, though.

Mostly my comments are naming quibbles. One more: do you anticipate that any of
this would be reused if we used instancing for other unrelated primitive types
that didn't batch with those covered here? It seems like the naming is centered
around Instanced but I wonder whether it should be centered around RRects or
InstancedRRects. I guess the question is: isn't all of this pretty specific to
RRect subtypes (and DRRect)?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:184: #if SK_SCALAR_IS_FLOAT
I'd just static assert that SkScalar is float here. Ganesh already does so in
many places and there will be no testing of the #else code.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:292: #if SK_SCALAR_IS_FLOAT
ditto

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:321: // We can't generally rely on fTracker
here because it might change when batches get combined.
Isn't this always called before batches are combined, when installing the
pipeline object?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
Is it really necessary to you your own custom allocator for these batches? It
seems like an unfortunate complication. GrBatches already live in block
allocated memory.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:19: * This class serves as a centralized
clearinghouse for instanced rendering. It accumulates data for
If we had a completely different instanced primitive type that didn't batch with
(d)rrect family of shapes would we use this same object? I'm wondering whether
this is really a InstancedRRectRenderer. I know we did it with path rendering
but having class names that are verbs strikes me as a little odd.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:34: 
I think these could use a comment

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:49: * must call commitToGpu() before attempting
to draw batches returned by this class. After
Maybe before attempting to flush batches returned by this class?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:52: GrDrawBatch* SK_WARN_UNUSED_RESULT
recordRect(const SkRect&, const SkMatrix&, GrColor,
It seems to me like the "recording" aspect of these calls is relevant to the
implementation, but from the callers' perspectives these methods are factory
functions. Perhaps createFooBatch?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:77: void commitToGpu(GrResourceProvider*);
Wonder if something like prepareToFlush or willFlush would more clearly tie this
into flushing the GrDTs

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:83: void restart();
Minor but I think we more commonly use reset()

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:94: void clearGpuResources(ClearType);
freeGpuResources? again just trying to mentally map into when it is used by
naming it something like the higher level calls that use it.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRenderingTypes.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:18: class GrInstancedRenderingTypes {
Why this pattern of inheriting from this class which contains no interface? Why
aren't these just defined in GrInstancedRendering?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:64: enum ShapeType {
I think use of the word "shape" may conflict with some work that is in progress
which will encapsulate rrect as well as non-rrect geometries using the word
shape. Could we call these something else? GeometryType? DrawType? RRectType?

Or can this just use the similar enum already defined in SkRRect?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:119: struct BatchTracker {
We've been moving away from using "BatchTracker" in other batch subclasses and
just store the common properties in GrBatch member vars. However, this in this
instance it may make sense. I've never been a fan of the name, though. Could we
call this something like BatchProperties or CommonProperties? or ...

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:151: bool      fUsesColor                   
: 1;
Uses color and uses coverage are rarely useful, do you need to track them? We've
been thinking about removing these.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
File src/gpu/effects/GrInstanceProcessor.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
src/gpu/effects/GrInstanceProcessor.h:23: static uint32_t
GetSupportedAAModes(const GrGLSLCaps&, const GrCaps&);
Having GLSL caps here is sort of violation of the layering, should be agnostic.
Can this be made to work with ShaderCaps?

[Chris Dalton, 2016-04-25 17:01:19]

Replying to a few comments. Thanks for the comprehensive review!

To answer your main question, yes. I hope to see this class also drawing beziers
and other types of shapes in the near future. In my mind, the distinctive
feature is that it uses instanced rendering, not that the set of supported
shapes in v1.0 can all be generalized as round rects. (See the comment in
GrInstancedRendering.h)

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:321: // We can't generally rely on fTracker
here because it might change when batches get combined.
On 2016/04/25 13:22:38, bsalomon wrote:
> Isn't this always called before batches are combined, when installing the
> pipeline object?

Yes, but for example a non-discarding non-AA draw can't make any conclusions
about coverage based on "fTracker.isSimpleRects()". Because it might get
combined later with an oval that actually needs to set coverage to zero on
certain pixels.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/04/25 13:22:38, bsalomon wrote:
> Is it really necessary to you your own custom allocator for these batches? It
> seems like an unfortunate complication. GrBatches already live in block
> allocated memory.

Oh really? How long have they lived in block allocated memory?

But it really is necessary for this class to keep track of all the batches it
has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It loops
through all the batches in fBatchAllocator in order to generate the draw
indirect buffer for the flush.

Also, for now we just dump the instance data linearly into a buffer. But if we
want to make this work without EXT_base_instance, we will need to similarly
defer the instance buffer generation until commitToGpu (after all combining has
occurred). 

And as we start putting more data in the texel buffer (coord transforms for
starters, basic uniforms, clip planes) we will also need to defer the texel
buffer generation until commitToGpu.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:19: * This class serves as a centralized
clearinghouse for instanced rendering. It accumulates data for
On 2016/04/25 13:22:38, bsalomon wrote:
> If we had a completely different instanced primitive type that didn't batch
with
> (d)rrect family of shapes would we use this same object? I'm wondering whether
> this is really a InstancedRRectRenderer. I know we did it with path rendering
> but having class names that are verbs strikes me as a little odd.

My vision for this class is yes! The basic geometry is so small and simple,
there's no reason we can't fill the vertex and index buffers with explicit
polygons, beziers, etc.

Right now the shaders only rasterize arcs and rectangles, but gl_SampleMask is
PERFECT for Loop-Blinn. We could (and should) easily add support for various
types of non-overlapping shapes.

Then a lot of simple-ish path rendering can be achieved by selecting the right
index range from a pre-generated library of simple shapes, and pushing a few
parameters to a texel buffer.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:52: GrDrawBatch* SK_WARN_UNUSED_RESULT
recordRect(const SkRect&, const SkMatrix&, GrColor,
On 2016/04/25 13:22:38, bsalomon wrote:
> It seems to me like the "recording" aspect of these calls is relevant to the
> implementation, but from the callers' perspectives these methods are factory
> functions. Perhaps createFooBatch?

When naming these methods I did think this through, and decided I liked "record"
better than "create" :)

My rationale was that "record" signifies better that this class is internally
making a record for the desired shape, and the batch that gets returned is
literally just an index to that record. (Thus reinforcing the understanding that
this class is what actually does the rendering, not the batch, and the batch
depends on this class).

A traditional factory-style "create" name implies to me that the batch is more
autonomous, which is not the case here.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:83: void restart();
On 2016/04/25 13:22:38, bsalomon wrote:
> Minor but I think we more commonly use reset()

I also thought this one through. I decided to go with "restart" specifically
because "reset" in Skia so commonly means "destroy all objects and reset
everything back to zero".

That's not the case here. This method simply releases a few scratch buffers that
were tied to the current flush and puts the class back into recording mode.

I'm not attached to the name "record" though, I'm just saying "reset" might
mislead somebody to want to call this method instead of clearGpuResources (name
in question), which is what you should actually call to destroy all gpu memory.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:94: void clearGpuResources(ClearType);
On 2016/04/25 13:22:38, bsalomon wrote:
> freeGpuResources? again just trying to mentally map into when it is used by
> naming it something like the higher level calls that use it.


clearGpuResources(ClearType::kDestroy) would be the equivalent of
freeGpuResources().

clearGpuResources(ClearType::kAbandon) would be the equivalent of
abandonGpuResources().

I guess I got "clear" from the Blink-style naming conventions on smart pointers.
In this context though "clear" is a loaded name. How about:

resetGpuResources(ResetType)

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRenderingTypes.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:18: class GrInstancedRenderingTypes {
On 2016/04/25 13:22:39, bsalomon wrote:
> Why this pattern of inheriting from this class which contains no interface?
Why
> aren't these just defined in GrInstancedRendering?

Mainly because then GrInstanceProcessor.cpp gets littered with
"GrInstancedRendering::" all over, or if we put them in GrInstanceProcessor,
GrInstanceRendering gets littered with "GrInstanceProcessor::" everywhere :)

But also, I don't see these types as specifically belonging to one class or the
other. They're really just part of the instanced rendering subsystem. Maybe a
namespace would be more appropriate but I don't see much precedent for that in
Skia.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:64: enum ShapeType {
On 2016/04/25 13:22:39, bsalomon wrote:
> I think use of the word "shape" may conflict with some work that is in
progress
> which will encapsulate rrect as well as non-rrect geometries using the word
> shape. Could we call these something else? GeometryType? DrawType? RRectType?
> 
> Or can this just use the similar enum already defined in SkRRect?
> 

SkRRect also has kEmpty, which I would prefer to not include in this enum.

Also, my vision for these classes is that they're a framework for drawing shapes
using instanced rendering (see the comment in GrInstancedRendering.h). It does
turn out that the shapes supported in v1.0 can all be generalized as round
rects, but if I get my way it won't stay like that forever.

Is it really a problem for a class to use the word "shape" internally?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:119: struct BatchTracker {
On 2016/04/25 13:22:38, bsalomon wrote:
> We've been moving away from using "BatchTracker" in other batch subclasses and
> just store the common properties in GrBatch member vars. However, this in this
> instance it may make sense. I've never been a fan of the name, though. Could
we
> call this something like BatchProperties or CommonProperties? or ...

Agreed, I don't like the name either. We can come up with a better one.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:151: bool      fUsesColor                   
: 1;
On 2016/04/25 13:22:39, bsalomon wrote:
> Uses color and uses coverage are rarely useful, do you need to track them?
We've
> been thinking about removing these.

Ok, I'll gladly take them out.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
File src/gpu/effects/GrInstanceProcessor.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
src/gpu/effects/GrInstanceProcessor.h:23: static uint32_t
GetSupportedAAModes(const GrGLSLCaps&, const GrCaps&);
On 2016/04/25 13:22:39, bsalomon wrote:
> Having GLSL caps here is sort of violation of the layering, should be
agnostic.
> Can this be made to work with ShaderCaps?

Is it really a layering violation when the class has a method called
"createGLSLInstance" that also accepts GLSL caps? I guess it didn't seem weird
to me for the method of deciding which AA modes are supported to be tied to a
specific shading language.

If another shading language came along, I figured we would just add a couple
more methods like this:

 static uint32_t GetSupportedAAModes(const GrHLSLCaps&, const GrCaps&);
 ...
 GrHLSLPrimitiveProcessor* createHLSLInstance(const GrHLSLCaps&) const override;


In theory it could probably be made agnostic, but for now a lot of the caps that
GetSupportedAAModes(GrGLSLCaps) needs to check live on GrGLSLCaps instead of
GrShaderCaps.

[Chris Dalton, 2016-04-26 19:18:08]

Ok, all the comments are either addressed now or open for more discussion.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:184: #if SK_SCALAR_IS_FLOAT
On 2016/04/25 13:22:38, bsalomon wrote:
> I'd just static assert that SkScalar is float here. Ganesh already does so in
> many places and there will be no testing of the #else code.

Done. (SkScalar == double isn't on the table?)

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:292: #if SK_SCALAR_IS_FLOAT
On 2016/04/25 13:22:38, bsalomon wrote:
> ditto

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:321: // We can't generally rely on fTracker
here because it might change when batches get combined.
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > Isn't this always called before batches are combined, when installing the
> > pipeline object?
> 
> Yes, but for example a non-discarding non-AA draw can't make any conclusions
> about coverage based on "fTracker.isSimpleRects()". Because it might get
> combined later with an oval that actually needs to set coverage to zero on
> certain pixels.

Done. (Made the comment better)

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > Is it really necessary to you your own custom allocator for these batches?
It
> > seems like an unfortunate complication. GrBatches already live in block
> > allocated memory.
> 
> Oh really? How long have they lived in block allocated memory?
> 
> But it really is necessary for this class to keep track of all the batches it
> has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It loops
> through all the batches in fBatchAllocator in order to generate the draw
> indirect buffer for the flush.
> 
> Also, for now we just dump the instance data linearly into a buffer. But if we
> want to make this work without EXT_base_instance, we will need to similarly
> defer the instance buffer generation until commitToGpu (after all combining
has
> occurred). 
> 
> And as we start putting more data in the texel buffer (coord transforms for
> starters, basic uniforms, clip planes) we will also need to defer the texel
> buffer generation until commitToGpu.

Added a comment to the BatchAllocator class in GrInstancedRendering.h

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:34: 
On 2016/04/25 13:22:38, bsalomon wrote:
> I think these could use a comment

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:49: * must call commitToGpu() before attempting
to draw batches returned by this class. After
On 2016/04/25 13:22:38, bsalomon wrote:
> Maybe before attempting to flush batches returned by this class?

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:52: GrDrawBatch* SK_WARN_UNUSED_RESULT
recordRect(const SkRect&, const SkMatrix&, GrColor,
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > It seems to me like the "recording" aspect of these calls is relevant to the
> > implementation, but from the callers' perspectives these methods are factory
> > functions. Perhaps createFooBatch?
> 
> When naming these methods I did think this through, and decided I liked
"record"
> better than "create" :)
> 
> My rationale was that "record" signifies better that this class is internally
> making a record for the desired shape, and the batch that gets returned is
> literally just an index to that record. (Thus reinforcing the understanding
that
> this class is what actually does the rendering, not the batch, and the batch
> depends on this class).
> 
> A traditional factory-style "create" name implies to me that the batch is more
> autonomous, which is not the case here.

Updated the comment.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:77: void commitToGpu(GrResourceProvider*);
On 2016/04/25 13:22:38, bsalomon wrote:
> Wonder if something like prepareToFlush or willFlush would more clearly tie
this
> into flushing the GrDTs

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:83: void restart();
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > Minor but I think we more commonly use reset()
> 
> I also thought this one through. I decided to go with "restart" specifically
> because "reset" in Skia so commonly means "destroy all objects and reset
> everything back to zero".
> 
> That's not the case here. This method simply releases a few scratch buffers
that
> were tied to the current flush and puts the class back into recording mode.
> 
> I'm not attached to the name "record" though, I'm just saying "reset" might
> mislead somebody to want to call this method instead of clearGpuResources
(name
> in question), which is what you should actually call to destroy all gpu
memory.

How's endFlush()?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:94: void clearGpuResources(ClearType);
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > freeGpuResources? again just trying to mentally map into when it is used by
> > naming it something like the higher level calls that use it.
> 
> 
> clearGpuResources(ClearType::kDestroy) would be the equivalent of
> freeGpuResources().
> 
> clearGpuResources(ClearType::kAbandon) would be the equivalent of
> abandonGpuResources().
> 
> I guess I got "clear" from the Blink-style naming conventions on smart
pointers.
> In this context though "clear" is a loaded name. How about:
> 
> resetGpuResources(ResetType)

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRenderingTypes.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:119: struct BatchTracker {
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > We've been moving away from using "BatchTracker" in other batch subclasses
and
> > just store the common properties in GrBatch member vars. However, this in
this
> > instance it may make sense. I've never been a fan of the name, though. Could
> we
> > call this something like BatchProperties or CommonProperties? or ...
> 
> Agreed, I don't like the name either. We can come up with a better one.

Done.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:151: bool      fUsesColor                   
: 1;
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:39, bsalomon wrote:
> > Uses color and uses coverage are rarely useful, do you need to track them?
> We've
> > been thinking about removing these.
> 
> Ok, I'll gladly take them out.

Done.

[bsalomon, 2016-04-27 14:10:32]

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:184: #if SK_SCALAR_IS_FLOAT
On 2016/04/26 19:18:07, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > I'd just static assert that SkScalar is float here. Ganesh already does so
in
> > many places and there will be no testing of the #else code.
> 
> Done. (SkScalar == double isn't on the table?)


It's on the table but there is no plan to tackle it in the next year. And if we
did it I think there would be lots of hard thinking about how to make Ganesh
work with doubles. Also, probably many of things that are currently SkScalar
would become explicit float. E.g., you want doubles for positions but maybe not
for fields of a color matrix.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:321: // We can't generally rely on fTracker
here because it might change when batches get combined.
On 2016/04/26 19:18:07, Chris Dalton wrote:
> On 2016/04/25 17:01:19, Chris Dalton wrote:
> > On 2016/04/25 13:22:38, bsalomon wrote:
> > > Isn't this always called before batches are combined, when installing the
> > > pipeline object?
> > 
> > Yes, but for example a non-discarding non-AA draw can't make any conclusions
> > about coverage based on "fTracker.isSimpleRects()". Because it might get
> > combined later with an oval that actually needs to set coverage to zero on
> > certain pixels.
> 
> Done. (Made the comment better)

Ok, that makes it clearer.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > Is it really necessary to you your own custom allocator for these batches?
It
> > seems like an unfortunate complication. GrBatches already live in block
> > allocated memory.
> 
> Oh really? How long have they lived in block allocated memory?

Until they're unreffed (at the end of GrDT flush). blocks are freed after all
the batches have been freed.

> 
> But it really is necessary for this class to keep track of all the batches it
> has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It loops
> through all the batches in fBatchAllocator in order to generate the draw
> indirect buffer for the flush.

But isn't there a difference between "keeping track of" and allocating? Would it
hurt to keep an array of ptrs rather than own the storage?

> 
> Also, for now we just dump the instance data linearly into a buffer. But if we
> want to make this work without EXT_base_instance, we will need to similarly
> defer the instance buffer generation until commitToGpu (after all combining
has
> occurred). 
> 
> And as we start putting more data in the texel buffer (coord transforms for
> starters, basic uniforms, clip planes) we will also need to defer the texel
> buffer generation until commitToGpu.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:19: * This class serves as a centralized
clearinghouse for instanced rendering. It accumulates data for
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > If we had a completely different instanced primitive type that didn't batch
> with
> > (d)rrect family of shapes would we use this same object? I'm wondering
whether
> > this is really a InstancedRRectRenderer. I know we did it with path
rendering
> > but having class names that are verbs strikes me as a little odd.
> 
> My vision for this class is yes! The basic geometry is so small and simple,
> there's no reason we can't fill the vertex and index buffers with explicit
> polygons, beziers, etc.
> 
> Right now the shaders only rasterize arcs and rectangles, but gl_SampleMask is
> PERFECT for Loop-Blinn. We could (and should) easily add support for various
> types of non-overlapping shapes.
> 
> Then a lot of simple-ish path rendering can be achieved by selecting the right
> index range from a pre-generated library of simple shapes, and pushing a few
> parameters to a texel buffer.

Ok, let's keep the name then

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:52: GrDrawBatch* SK_WARN_UNUSED_RESULT
recordRect(const SkRect&, const SkMatrix&, GrColor,
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:38, bsalomon wrote:
> > It seems to me like the "recording" aspect of these calls is relevant to the
> > implementation, but from the callers' perspectives these methods are factory
> > functions. Perhaps createFooBatch?
> 
> When naming these methods I did think this through, and decided I liked
"record"
> better than "create" :)
> 
> My rationale was that "record" signifies better that this class is internally
> making a record for the desired shape, and the batch that gets returned is
> literally just an index to that record. (Thus reinforcing the understanding
that
> this class is what actually does the rendering, not the batch, and the batch
> depends on this class).
> 
> A traditional factory-style "create" name implies to me that the batch is more
> autonomous, which is not the case here.

Ok, that makes sense to me.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.h:83: void restart();
On 2016/04/26 19:18:07, Chris Dalton wrote:
> On 2016/04/25 17:01:19, Chris Dalton wrote:
> > On 2016/04/25 13:22:38, bsalomon wrote:
> > > Minor but I think we more commonly use reset()
> > 
> > I also thought this one through. I decided to go with "restart" specifically
> > because "reset" in Skia so commonly means "destroy all objects and reset
> > everything back to zero".
> > 
> > That's not the case here. This method simply releases a few scratch buffers
> that
> > were tied to the current flush and puts the class back into recording mode.
> > 
> > I'm not attached to the name "record" though, I'm just saying "reset" might
> > mislead somebody to want to call this method instead of clearGpuResources
> (name
> > in question), which is what you should actually call to destroy all gpu
> memory.
> 
> How's endFlush()?

sgtm

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRenderingTypes.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:18: class GrInstancedRenderingTypes {
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:39, bsalomon wrote:
> > Why this pattern of inheriting from this class which contains no interface?
> Why
> > aren't these just defined in GrInstancedRendering?
> 
> Mainly because then GrInstanceProcessor.cpp gets littered with
> "GrInstancedRendering::" all over, or if we put them in GrInstanceProcessor,
> GrInstanceRendering gets littered with "GrInstanceProcessor::" everywhere :)
> 
> But also, I don't see these types as specifically belonging to one class or
the
> other. They're really just part of the instanced rendering subsystem. Maybe a
> namespace would be more appropriate but I don't see much precedent for that in
> Skia.

I'd prefer a namespace and using decls.

What do you think about organizing all the instanced classes into their own
subdir, say src/gpu/instanced or something like that?

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:64: enum ShapeType {
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:39, bsalomon wrote:
> > I think use of the word "shape" may conflict with some work that is in
> progress
> > which will encapsulate rrect as well as non-rrect geometries using the word
> > shape. Could we call these something else? GeometryType? DrawType?
RRectType?
> > 
> > Or can this just use the similar enum already defined in SkRRect?
> > 
> 
> SkRRect also has kEmpty, which I would prefer to not include in this enum.
> 
> Also, my vision for these classes is that they're a framework for drawing
shapes
> using instanced rendering (see the comment in GrInstancedRendering.h). It does
> turn out that the shapes supported in v1.0 can all be generalized as round
> rects, but if I get my way it won't stay like that forever.
> 
> Is it really a problem for a class to use the word "shape" internally?

I guess it is ok.

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
File src/gpu/effects/GrInstanceProcessor.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/effects/GrInst...
src/gpu/effects/GrInstanceProcessor.h:23: static uint32_t
GetSupportedAAModes(const GrGLSLCaps&, const GrCaps&);
On 2016/04/25 17:01:19, Chris Dalton wrote:
> On 2016/04/25 13:22:39, bsalomon wrote:
> > Having GLSL caps here is sort of violation of the layering, should be
> agnostic.
> > Can this be made to work with ShaderCaps?
> 
> Is it really a layering violation when the class has a method called
> "createGLSLInstance" that also accepts GLSL caps? I guess it didn't seem weird
> to me for the method of deciding which AA modes are supported to be tied to a
> specific shading language.
> 
> If another shading language came along, I figured we would just add a couple
> more methods like this:
> 
>  static uint32_t GetSupportedAAModes(const GrHLSLCaps&, const GrCaps&);
>  ...
>  GrHLSLPrimitiveProcessor* createHLSLInstance(const GrHLSLCaps&) const
override;
> 
> 
> In theory it could probably be made agnostic, but for now a lot of the caps
that
> GetSupportedAAModes(GrGLSLCaps) needs to check live on GrGLSLCaps instead of
> GrShaderCaps.

It is definitely the case that if we added another shading language we'd have to
add new GrNewSLFooProcessor factory methods (as well as getNewSLKey() methods).
However, we've tried to limit the number of SL-specific methods. Currently,
they're all called in the GLSL-specific code, not in API-neutral layer, which I
suppose is also the case here. Looking at the caps used they'd all make sense to
me as GrShaderCaps with the exception of canUseAnyFunctionInShader which is
definitely a GLSL-specific driver bug workaround. So, I suppose it is ok to keep
as is.

[csmartdalton, 2016-05-20 00:46:57]

csmartdalton@google.com changed reviewers:
+ csmartdalton@google.com

[csmartdalton, 2016-05-20 01:09:46]

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/04/27 14:10:32, bsalomon wrote:
> On 2016/04/25 17:01:19, Chris Dalton wrote:
> > On 2016/04/25 13:22:38, bsalomon wrote:
> > > Is it really necessary to you your own custom allocator for these batches?
> It
> > > seems like an unfortunate complication. GrBatches already live in block
> > > allocated memory.
> > 
> > Oh really? How long have they lived in block allocated memory?
> 
> Until they're unreffed (at the end of GrDT flush). blocks are freed after all
> the batches have been freed.
> 
> > 
> > But it really is necessary for this class to keep track of all the batches
it
> > has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It loops
> > through all the batches in fBatchAllocator in order to generate the draw
> > indirect buffer for the flush.
> 
> But isn't there a difference between "keeping track of" and allocating? Would
it
> hurt to keep an array of ptrs rather than own the storage?

The main advantage to owning is that you know when a batch is deleted and
therefore no longer in use. We could do it with pointers too, but in order to
know if a batch is still in use then we might need to peek at the ref count and
see if it's >1 (assuming one of the refs comes from us.)

I can see it working, but I'm not sure I understand the advantage in complexity
of an array of ref'd pointers vs an array of structs. Is your main beef the
special handling when the ref count reaches zero?

> 
> > 
> > Also, for now we just dump the instance data linearly into a buffer. But if
we
> > want to make this work without EXT_base_instance, we will need to similarly
> > defer the instance buffer generation until commitToGpu (after all combining
> has
> > occurred). 
> > 
> > And as we start putting more data in the texel buffer (coord transforms for
> > starters, basic uniforms, clip planes) we will also need to defer the texel
> > buffer generation until commitToGpu.
>

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRenderingTypes.h (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRenderingTypes.h:18: class GrInstancedRenderingTypes {
On 2016/04/27 14:10:32, bsalomon wrote:
> On 2016/04/25 17:01:19, Chris Dalton wrote:
> > On 2016/04/25 13:22:39, bsalomon wrote:
> > > Why this pattern of inheriting from this class which contains no
interface?
> > Why
> > > aren't these just defined in GrInstancedRendering?
> > 
> > Mainly because then GrInstanceProcessor.cpp gets littered with
> > "GrInstancedRendering::" all over, or if we put them in GrInstanceProcessor,
> > GrInstanceRendering gets littered with "GrInstanceProcessor::" everywhere :)
> > 
> > But also, I don't see these types as specifically belonging to one class or
> the
> > other. They're really just part of the instanced rendering subsystem. Maybe
a
> > namespace would be more appropriate but I don't see much precedent for that
in
> > Skia.
> 
> I'd prefer a namespace and using decls.
> 
> What do you think about organizing all the instanced classes into their own
> subdir, say src/gpu/instanced or something like that?

sgtm

[bsalomon, 2016-05-20 14:04:49]

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/05/20 01:09:45, csmartdalton wrote:
> On 2016/04/27 14:10:32, bsalomon wrote:
> > On 2016/04/25 17:01:19, Chris Dalton wrote:
> > > On 2016/04/25 13:22:38, bsalomon wrote:
> > > > Is it really necessary to you your own custom allocator for these
batches?
> > It
> > > > seems like an unfortunate complication. GrBatches already live in block
> > > > allocated memory.
> > > 
> > > Oh really? How long have they lived in block allocated memory?
> > 
> > Until they're unreffed (at the end of GrDT flush). blocks are freed after
all
> > the batches have been freed.
> > 
> > > 
> > > But it really is necessary for this class to keep track of all the batches
> it
> > > has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It
loops
> > > through all the batches in fBatchAllocator in order to generate the draw
> > > indirect buffer for the flush.
> > 
> > But isn't there a difference between "keeping track of" and allocating?
Would
> it
> > hurt to keep an array of ptrs rather than own the storage?
> 
> The main advantage to owning is that you know when a batch is deleted and
> therefore no longer in use. We could do it with pointers too, but in order to
> know if a batch is still in use then we might need to peek at the ref count
and
> see if it's >1 (assuming one of the refs comes from us.)

There is no reason we couldn't have the instanced batch subclass call a function
on the GrInstancedRendering when it is destroyed, perhaps conveying its index
into the batches that the GrIR is keeping track of. 

> 
> I can see it working, but I'm not sure I understand the advantage in
complexity
> of an array of ref'd pointers vs an array of structs. Is your main beef the
> special handling when the ref count reaches zero?

It just seems like we're deploying a heavier hammer than is necessary here in
ways that aren't limited in scope to GrIR and GrIR::Batch. We're block
allocating an already block allocated type, adding a onDelete to the base class,
and adding a GrTDeleteNonAtomicRef overload [batches will likely not derive from
GrNonAtomicRef in the future]. AIUI GrIR just needs to be able to iterate over
its batches and know when they're deleted. It seems doable by having GrIR have
non-ref'ed ptrs and having a notification call back in GrIR:Batch::~Batch to let
GrIR know the instance is deleted.

What exactly is the notification requirement? Is it to handle batch combining in
GrDT? Otherwise it seems like any batch added to a GrDT must survive until the
flush.

> 
> > 
> > > 
> > > Also, for now we just dump the instance data linearly into a buffer. But
if
> we
> > > want to make this work without EXT_base_instance, we will need to
similarly
> > > defer the instance buffer generation until commitToGpu (after all
combining
> > has
> > > occurred). 
> > > 
> > > And as we start putting more data in the texel buffer (coord transforms
for
> > > starters, basic uniforms, clip planes) we will also need to defer the
texel
> > > buffer generation until commitToGpu.
> > 
>

[csmartdalton, 2016-05-21 06:54:25]

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/05/20 14:04:48, bsalomon wrote:
> On 2016/05/20 01:09:45, csmartdalton wrote:
> > On 2016/04/27 14:10:32, bsalomon wrote:
> > > On 2016/04/25 17:01:19, Chris Dalton wrote:
> > > > On 2016/04/25 13:22:38, bsalomon wrote:
> > > > > Is it really necessary to you your own custom allocator for these
> batches?
> > > It
> > > > > seems like an unfortunate complication. GrBatches already live in
block
> > > > > allocated memory.
> > > > 
> > > > Oh really? How long have they lived in block allocated memory?
> > > 
> > > Until they're unreffed (at the end of GrDT flush). blocks are freed after
> all
> > > the batches have been freed.
> > > 
> > > > 
> > > > But it really is necessary for this class to keep track of all the
batches
> > it
> > > > has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It
> loops
> > > > through all the batches in fBatchAllocator in order to generate the draw
> > > > indirect buffer for the flush.
> > > 
> > > But isn't there a difference between "keeping track of" and allocating?
> Would
> > it
> > > hurt to keep an array of ptrs rather than own the storage?
> > 
> > The main advantage to owning is that you know when a batch is deleted and
> > therefore no longer in use. We could do it with pointers too, but in order
to
> > know if a batch is still in use then we might need to peek at the ref count
> and
> > see if it's >1 (assuming one of the refs comes from us.)
> 
> There is no reason we couldn't have the instanced batch subclass call a
function
> on the GrInstancedRendering when it is destroyed, perhaps conveying its index
> into the batches that the GrIR is keeping track of. 
> 

OK, I like that idea.


> > 
> > I can see it working, but I'm not sure I understand the advantage in
> complexity
> > of an array of ref'd pointers vs an array of structs. Is your main beef the
> > special handling when the ref count reaches zero?
> 
> It just seems like we're deploying a heavier hammer than is necessary here in
> ways that aren't limited in scope to GrIR and GrIR::Batch. We're block
> allocating an already block allocated type, adding a onDelete to the base
class,
> and adding a GrTDeleteNonAtomicRef overload [batches will likely not derive
from
> GrNonAtomicRef in the future]. AIUI GrIR just needs to be able to iterate over
> its batches and know when they're deleted. It seems doable by having GrIR have
> non-ref'ed ptrs and having a notification call back in GrIR:Batch::~Batch to
let
> GrIR know the instance is deleted.
> 
> What exactly is the notification requirement? Is it to handle batch combining
in
> GrDT? Otherwise it seems like any batch added to a GrDT must survive until the
> flush.
> 

Correct, it's just for when a batch gets combined into a previous one.

> > 
> > > 
> > > > 
> > > > Also, for now we just dump the instance data linearly into a buffer. But
> if
> > we
> > > > want to make this work without EXT_base_instance, we will need to
> similarly
> > > > defer the instance buffer generation until commitToGpu (after all
> combining
> > > has
> > > > occurred). 
> > > > 
> > > > And as we start putting more data in the texel buffer (coord transforms
> for
> > > > starters, basic uniforms, clip planes) we will also need to defer the
> texel
> > > > buffer generation until commitToGpu.
> > > 
> > 
>

[bsalomon, 2016-05-23 14:05:43]

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
File src/gpu/GrInstancedRendering.cpp (right):

https://codereview.chromium.org/1897203002/diff/100001/src/gpu/GrInstancedRen...
src/gpu/GrInstancedRendering.cpp:398: void
GrInstancedRendering::Batch::onDelete() const {
On 2016/05/21 06:54:24, csmartdalton wrote:
> On 2016/05/20 14:04:48, bsalomon wrote:
> > On 2016/05/20 01:09:45, csmartdalton wrote:
> > > On 2016/04/27 14:10:32, bsalomon wrote:
> > > > On 2016/04/25 17:01:19, Chris Dalton wrote:
> > > > > On 2016/04/25 13:22:38, bsalomon wrote:
> > > > > > Is it really necessary to you your own custom allocator for these
> > batches?
> > > > It
> > > > > > seems like an unfortunate complication. GrBatches already live in
> block
> > > > > > allocated memory.
> > > > > 
> > > > > Oh really? How long have they lived in block allocated memory?
> > > > 
> > > > Until they're unreffed (at the end of GrDT flush). blocks are freed
after
> > all
> > > > the batches have been freed.
> > > > 
> > > > > 
> > > > > But it really is necessary for this class to keep track of all the
> batches
> > > it
> > > > > has created. Take a look at GrGLInstancedRendering::onCommitToGpu. It
> > loops
> > > > > through all the batches in fBatchAllocator in order to generate the
draw
> > > > > indirect buffer for the flush.
> > > > 
> > > > But isn't there a difference between "keeping track of" and allocating?
> > Would
> > > it
> > > > hurt to keep an array of ptrs rather than own the storage?
> > > 
> > > The main advantage to owning is that you know when a batch is deleted and
> > > therefore no longer in use. We could do it with pointers too, but in order
> to
> > > know if a batch is still in use then we might need to peek at the ref
count
> > and
> > > see if it's >1 (assuming one of the refs comes from us.)
> > 
> > There is no reason we couldn't have the instanced batch subclass call a
> function
> > on the GrInstancedRendering when it is destroyed, perhaps conveying its
index
> > into the batches that the GrIR is keeping track of. 
> > 
> 
> OK, I like that idea.
> 
> 
> > > 
> > > I can see it working, but I'm not sure I understand the advantage in
> > complexity
> > > of an array of ref'd pointers vs an array of structs. Is your main beef
the
> > > special handling when the ref count reaches zero?
> > 
> > It just seems like we're deploying a heavier hammer than is necessary here
in
> > ways that aren't limited in scope to GrIR and GrIR::Batch. We're block
> > allocating an already block allocated type, adding a onDelete to the base
> class,
> > and adding a GrTDeleteNonAtomicRef overload [batches will likely not derive
> from
> > GrNonAtomicRef in the future]. AIUI GrIR just needs to be able to iterate
over
> > its batches and know when they're deleted. It seems doable by having GrIR
have
> > non-ref'ed ptrs and having a notification call back in GrIR:Batch::~Batch to
> let
> > GrIR know the instance is deleted.
> > 
> > What exactly is the notification requirement? Is it to handle batch
combining
> in
> > GrDT? Otherwise it seems like any batch added to a GrDT must survive until
the
> > flush.
> > 
> 
> Correct, it's just for when a batch gets combined into a previous one.
> 
> > > 
> > > > 
> > > > > 
> > > > > Also, for now we just dump the instance data linearly into a buffer.
But
> > if
> > > we
> > > > > want to make this work without EXT_base_instance, we will need to
> > similarly
> > > > > defer the instance buffer generation until commitToGpu (after all
> > combining
> > > > has
> > > > > occurred). 
> > > > > 
> > > > > And as we start putting more data in the texel buffer (coord
transforms
> > for
> > > > > starters, basic uniforms, clip planes) we will also need to defer the
> > texel
> > > > > buffer generation until commitToGpu.
> > > > 
> > > 
> > 
> 

Cool, I suppose the notification could happen from the merge? I don't know which
makes more sense.

[csmartdalton, 2016-06-15 19:41:25]

Moved to https://codereview.chromium.org/2066993003/

[csmartdalton, 2016-06-15 19:41:36]

Description was changed from

==========
Implement instanced rendering for simple shapes

<TODO>

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement instanced rendering for simple shapes

<TODO>

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========