GrBatchPrototype

GrBatchPrototype

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/d15e4e45374275c045572b304c229237c4a82be4

Committed: https://skia.googlesource.com/skia/+/d5a7db4a867c7e6ccf8451a053d987b470099198

Committed: https://skia.googlesource.com/skia/+/4d8da81562852e0ff7e18b66ee1cebd50ad81ee8

[joshualitt, 2015-01-15 20:56:29]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com

[joshualitt, 2015-01-15 20:56:29]

So, I think this is mostly ready(gms match).  I didn't yet take the step to
break program descriptor / prim proc off of optstate.  I like the simplicity of
just passing one thing into gpu / GLPrograms, so I'll leave that call up to you.

The only major part I'm unhappy with is the generateGeometry block on
AARectFillBatch. Its tough to think how to refactor it without having a function
with a gazillion arguments.

Anyway, just give this a high level look, and let me know what I should start
addressing.  I still have to debug why we are doing that one extra draw call.

[joshualitt, 2015-01-20 15:59:04]

On 2015/01/15 20:56:29, joshualitt wrote:
> So, I think this is mostly ready(gms match).  I didn't yet take the step to
> break program descriptor / prim proc off of optstate.  I like the simplicity
of
> just passing one thing into gpu / GLPrograms, so I'll leave that call up to
you.
> 
> The only major part I'm unhappy with is the generateGeometry block on
> AARectFillBatch. Its tough to think how to refactor it without having a
function
> with a gazillion arguments.
> 
> Anyway, just give this a high level look, and let me know what I should start
> addressing.  I still have to debug why we are doing that one extra draw call.

Updated, please take a look

[bsalomon, 2015-01-20 16:14:03]

these are comments are from an older patchset, I guess I forgot to hit publish.
I tried to delete the ones that no longer apply, but might have missed some.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrAARectRenderer...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrAARectRenderer...
src/gpu/GrAARectRenderer.cpp:22: // TODO this doesn't build.  The next step is
to figure out how to further thread GrBatch through
still true?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrBatch.h
File src/gpu/GrBatch.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrBatch.h#newcode27
src/gpu/GrBatch.h:27: class GrBatch : public SkNoncopyable {
needs a block comment explaining what this is/does.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrBatch.h#newcode92
src/gpu/GrBatch.h:92: SkFAIL("This should never wrap as it should only be called
once for each GrProcessor "
each GrBatch subclass

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch != batch) {
all this storing of mutable ptrs seems very fragile. Can we move to uniqueIDs?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:439: * GrBatch provides the initial seed for these loops
based off of the batch it starts with
based off of the ... it starts with.

data?
primitives?

saying a batch starts with a batch seems confusing.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:481: mutable const GrBatch* fColorBatch;
color and coverage batches?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.cpp...
src/gpu/GrDrawTarget.cpp:594: // TODO: We should continue with incorrect
blending.
let's just remove the todo... we will probably never do such a thing.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
what's primtype doing here?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.cpp:286: if (draw->fBatch->canMakeEqual(*batch)) {
probably want to rename these... canMerge? canCombine? Also, should the query
and merger happen in one function? i.e. bool combineIfPossible(*batch).

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.h:208: // TODO it wouldn't be too hard to let
batches allocate in the cmd buffer
as opposed to in the mem pool? should grbatch be ref cnted?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.h:210: 
extra \n

[joshualitt, 2015-01-20 17:03:08]

Some comment responses / questions, most feedback incorporated

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch != batch) {
On 2015/01/20 16:14:02, bsalomon wrote:
> all this storing of mutable ptrs seems very fragile. Can we move to uniqueIDs?

How could we assign a uniqueID to each batch without using the pointer?  I agree
its fragile, I loathe this code, and right now its only for one
call(willBlendWithDst) which is just used by NVPR.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
On 2015/01/20 16:14:03, bsalomon wrote:
> what's primtype doing here?

I'd prefer to roll out batch everywhere, and then move primitive type to the
PrimitiveProcessor.  Its not a huge deal, but I think it might be annoying to
deal with at this point.  Is it alright if this is just a TODO?

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.h:208: // TODO it wouldn't be too hard to let
batches allocate in the cmd buffer
On 2015/01/20 16:14:03, bsalomon wrote:
> as opposed to in the mem pool? should grbatch be ref cnted?

Right, as opposed to the mem pool.  I don't think it really needs to be
refcnted, but its your call.

[bsalomon, 2015-01-20 20:49:58]

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch != batch) {
On 2015/01/20 17:03:08, joshualitt wrote:
> On 2015/01/20 16:14:02, bsalomon wrote:
> > all this storing of mutable ptrs seems very fragile. Can we move to
uniqueIDs?
> 
> How could we assign a uniqueID to each batch without using the pointer?  I
agree
> its fragile, I loathe this code, and right now its only for one
> call(willBlendWithDst) which is just used by NVPR.

I mean its fragile because two batches that exist at different times can share
an address. Unique IDs can only be shared after 4 billion batches have been
created between.

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
On 2015/01/20 17:03:08, joshualitt wrote:
> On 2015/01/20 16:14:03, bsalomon wrote:
> > what's primtype doing here?
> 
> I'd prefer to roll out batch everywhere, and then move primitive type to the
> PrimitiveProcessor.  Its not a huge deal, but I think it might be annoying to
> deal with at this point.  Is it alright if this is just a TODO?

I guess I just don't understand why it's here or what that implies. Does a batch
have to know ahead of time whether its drawing tris, lines, etc? What does that
make easier?

https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:493: GrBatch* batch =
AAFillRectBatch::Create(geometry);
I do kind of thing=k the batches should be ref counted. This implicit ownership
transfer seems scary.

[joshualitt, 2015-01-21 19:32:44]

On 2015/01/20 20:49:58, bsalomon wrote:
> https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
> File src/gpu/GrDrawState.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
> src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch != batch)
{
> On 2015/01/20 17:03:08, joshualitt wrote:
> > On 2015/01/20 16:14:02, bsalomon wrote:
> > > all this storing of mutable ptrs seems very fragile. Can we move to
> uniqueIDs?
> > 
> > How could we assign a uniqueID to each batch without using the pointer?  I
> agree
> > its fragile, I loathe this code, and right now its only for one
> > call(willBlendWithDst) which is just used by NVPR.
> 
> I mean its fragile because two batches that exist at different times can share
> an address. Unique IDs can only be shared after 4 billion batches have been
> created between.
> 
> https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
> src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
> On 2015/01/20 17:03:08, joshualitt wrote:
> > On 2015/01/20 16:14:03, bsalomon wrote:
> > > what's primtype doing here?
> > 
> > I'd prefer to roll out batch everywhere, and then move primitive type to the
> > PrimitiveProcessor.  Its not a huge deal, but I think it might be annoying
to
> > deal with at this point.  Is it alright if this is just a TODO?
> 
> I guess I just don't understand why it's here or what that implies. Does a
batch
> have to know ahead of time whether its drawing tris, lines, etc? What does
that
> make easier?
> 
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> File src/gpu/GrAARectRenderer.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> src/gpu/GrAARectRenderer.cpp:493: GrBatch* batch =
> AAFillRectBatch::Create(geometry);
> I do kind of thing=k the batches should be ref counted. This implicit
ownership
> transfer seems scary.

updated.

[joshualitt, 2015-01-21 19:32:46]

On 2015/01/20 20:49:58, bsalomon wrote:
> https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
> File src/gpu/GrDrawState.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
> src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch != batch)
{
> On 2015/01/20 17:03:08, joshualitt wrote:
> > On 2015/01/20 16:14:02, bsalomon wrote:
> > > all this storing of mutable ptrs seems very fragile. Can we move to
> uniqueIDs?
> > 
> > How could we assign a uniqueID to each batch without using the pointer?  I
> agree
> > its fragile, I loathe this code, and right now its only for one
> > call(willBlendWithDst) which is just used by NVPR.
> 
> I mean its fragile because two batches that exist at different times can share
> an address. Unique IDs can only be shared after 4 billion batches have been
> created between.
> 
> https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
> File src/gpu/GrDrawTarget.h (right):
> 
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
> src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
> On 2015/01/20 17:03:08, joshualitt wrote:
> > On 2015/01/20 16:14:03, bsalomon wrote:
> > > what's primtype doing here?
> > 
> > I'd prefer to roll out batch everywhere, and then move primitive type to the
> > PrimitiveProcessor.  Its not a huge deal, but I think it might be annoying
to
> > deal with at this point.  Is it alright if this is just a TODO?
> 
> I guess I just don't understand why it's here or what that implies. Does a
batch
> have to know ahead of time whether its drawing tris, lines, etc? What does
that
> make easier?
> 
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> File src/gpu/GrAARectRenderer.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> src/gpu/GrAARectRenderer.cpp:493: GrBatch* batch =
> AAFillRectBatch::Create(geometry);
> I do kind of thing=k the batches should be ref counted. This implicit
ownership
> transfer seems scary.

updated.

[joshualitt, 2015-01-21 19:55:12]

On 2015/01/21 19:32:46, joshualitt wrote:
> On 2015/01/20 20:49:58, bsalomon wrote:
> > https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp
> > File src/gpu/GrDrawState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawState.cpp#...
> > src/gpu/GrDrawState.cpp:239: if (!fColorProcInfoValid || fColorBatch !=
batch)
> {
> > On 2015/01/20 17:03:08, joshualitt wrote:
> > > On 2015/01/20 16:14:02, bsalomon wrote:
> > > > all this storing of mutable ptrs seems very fragile. Can we move to
> > uniqueIDs?
> > > 
> > > How could we assign a uniqueID to each batch without using the pointer?  I
> > agree
> > > its fragile, I loathe this code, and right now its only for one
> > > call(willBlendWithDst) which is just used by NVPR.
> > 
> > I mean its fragile because two batches that exist at different times can
share
> > an address. Unique IDs can only be shared after 4 billion batches have been
> > created between.
> > 
> > https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h
> > File src/gpu/GrDrawTarget.h (right):
> > 
> >
>
https://codereview.chromium.org/845103005/diff/80001/src/gpu/GrDrawTarget.h#n...
> > src/gpu/GrDrawTarget.h:265: GrPrimitiveType,
> > On 2015/01/20 17:03:08, joshualitt wrote:
> > > On 2015/01/20 16:14:03, bsalomon wrote:
> > > > what's primtype doing here?
> > > 
> > > I'd prefer to roll out batch everywhere, and then move primitive type to
the
> > > PrimitiveProcessor.  Its not a huge deal, but I think it might be annoying
> to
> > > deal with at this point.  Is it alright if this is just a TODO?
> > 
> > I guess I just don't understand why it's here or what that implies. Does a
> batch
> > have to know ahead of time whether its drawing tris, lines, etc? What does
> that
> > make easier?
> > 
> >
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> > File src/gpu/GrAARectRenderer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/845103005/diff/180001/src/gpu/GrAARectRendere...
> > src/gpu/GrAARectRenderer.cpp:493: GrBatch* batch =
> > AAFillRectBatch::Create(geometry);
> > I do kind of thing=k the batches should be ref counted. This implicit
> ownership
> > transfer seems scary.
> 
> updated.

Okay, please take a look.  I did a kind of funny optimization with the
viewmatrix in AARectRenderer, so take a look at that, and also GrBatchBuffer. 
Thanks.

[bsalomon, 2015-01-21 21:39:29]

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatch.h
File src/gpu/GrBatch.h (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatch.h#newco...
src/gpu/GrBatch.h:31: * Batches are created generally in renderers, but they
cannot be reused.  Instead, they are owned
How about

"Batches are created when GrContext processes a draw call. Batches of the same
subclass may be merged using canMakeEqual/MakeEqual. When two batches merge, one
takes on the union of the data and the other is left empty. The merged batch
becomes responsible for drawing the data from both the original batches."

also looks like these were renamed from equal to combine.

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatch.h#newco...
src/gpu/GrBatch.h:36: * GrBatches are always drawn with a GrOptDrawState.  Of
the GrOptDrawState determines any
The second sentence is confusing. Also, this might make more sense after the
third para.

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatch.h#newco...
src/gpu/GrBatch.h:40: * Finally, generateGeometry is called with a
GrOptDrawState, and a GrBatchBuffer.  GrBatchBuffer
Can you just explain it directly in terms of GrBatchBuffer rather than in terms
of something that a reader might now be familiar with?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatchBuffer.cpp
File src/gpu/GrBatchBuffer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatchBuffer.c...
src/gpu/GrBatchBuffer.cpp:14: fVertexPool->unmap();
index pool unmap?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatchBuffer.h
File src/gpu/GrBatchBuffer.h (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrBatchBuffer.h...
src/gpu/GrBatchBuffer.h:16: * GrBatchBuffer is a simple class which lets
GrDrawTarget subclasses defer flushing of batched
I'm not crazy about this name

GrBatchTarget?

Probably more useful to explain in terms of how it interacts with GrBatch rather
than GrDT.

"GrBatch instances use this object to allocate space for their geometry and to
issue the draws that render their batch."?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrDrawTarget.h#...
src/gpu/GrDrawTarget.h:263: void batchDraw(GrDrawState*,
drawBatch? Just seems more consistent

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:477: currentState = ss;
misaligned

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:534: 
extra \n

[bsalomon, 2015-01-22 14:05:02]

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:69: const char* name() const SK_OVERRIDE { return
"AAFillRectBatch"; }
What are batch names used for? not opposed to it, just curious.

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:80: void
getInvariantOutputCoverage(GrInitInvariantOutput* out,
need a \n here

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:94: void initBatchTracker(const
GrGeometryProcessor::InitBT& init) SK_OVERRIDE {
Why do we need a BT? Just to fit into the GP scheme?

Are there GPs that are in any way extending BT? Just wondering if it can be
downgraded to a struct that just communicates optimizations/overrides from
GrODS.

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:109: bool onCombineIfPossible(GrBatch* t)
SK_OVERRIDE {
private?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:137: GrColor color() const { return fBatch.fColor;
}
do all these need to be public or have getters at all? Are they just used in the
merge function?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:230: void generateGeometry(void* vertices,
this has the same name as a virtual override... could we rename it?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:287: // Make verts point to vertex color and then
set all the color and coverage vertex attrs values.
wrap

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:316: *reinterpret_cast<float*>(verts + i *
vertexStride + sizeof(GrColor)) = innerCoverage;
wrap

[joshualitt, 2015-01-22 15:47:46]

some comments inline, most feedback incorporated. ptal

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:69: const char* name() const SK_OVERRIDE { return
"AAFillRectBatch"; }
On 2015/01/22 14:05:02, bsalomon wrote:
> What are batch names used for? not opposed to it, just curious.

Debugging, I can remove if desired

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:80: void
getInvariantOutputCoverage(GrInitInvariantOutput* out,
On 2015/01/22 14:05:02, bsalomon wrote:
> need a \n here

I'm confused, isn't there a newline here?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:94: void initBatchTracker(const
GrGeometryProcessor::InitBT& init) SK_OVERRIDE {
There really isn't a 'batch tracker object' in batch world.  AAFillRect just has
a local struct for documenting batch properties.

Even if right now none of the Batches are really going to do much with batch
properties, I'd like to leave the functionality in for the short term.  I
definitely think there might be possible optimizations we can do in the future,
ie if the viewmatrix doesn't have perspective, AAFillRect can still batch, but
it'd have to track this decision somewhere.

However, I can rename the function if you want.  We have to do two init steps in
the short term unfortunately because non-batch uses the BatchTracker on
SetState, but in the long term I think we can move to a single init on GrBatch
called in OptState.  I can leave a todo

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:137: GrColor color() const { return fBatch.fColor;
}
On 2015/01/22 14:05:02, bsalomon wrote:
> do all these need to be public or have getters at all? Are they just used in
the
> merge function?

Well, its basically for the merge and generate geometry steps.  I think its a
bit nicer than writing out the full fBatch., fBatchOpt., fGeoData[] etc.  Its
your call.

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:477: currentState = ss;
On 2015/01/21 21:39:29, bsalomon wrote:
> misaligned

I'm sorry, maybe I'm just blind but was is misaligned here?

[bsalomon, 2015-01-22 16:00:59]

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:80: void
getInvariantOutputCoverage(GrInitInvariantOutput* out,
On 2015/01/22 15:47:46, joshualitt wrote:
> On 2015/01/22 14:05:02, bsalomon wrote:
> > need a \n here
> 
> I'm confused, isn't there a newline here?

before this line, after the preceeding }

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:94: void initBatchTracker(const
GrGeometryProcessor::InitBT& init) SK_OVERRIDE {
On 2015/01/22 15:47:46, joshualitt wrote:
> There really isn't a 'batch tracker object' in batch world.  AAFillRect just
has
> a local struct for documenting batch properties.
> 
> Even if right now none of the Batches are really going to do much with batch
> properties, I'd like to leave the functionality in for the short term.  I
> definitely think there might be possible optimizations we can do in the
future,
> ie if the viewmatrix doesn't have perspective, AAFillRect can still batch, but
> it'd have to track this decision somewhere.
> 
> However, I can rename the function if you want.  We have to do two init steps
in
> the short term unfortunately because non-batch uses the BatchTracker on
> SetState, but in the long term I think we can move to a single init on GrBatch
> called in OptState.  I can leave a todo

By single init do you mean combining initBatchTracker and initBatchOpt?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:137: GrColor color() const { return fBatch.fColor;
}
On 2015/01/22 15:47:46, joshualitt wrote:
> On 2015/01/22 14:05:02, bsalomon wrote:
> > do all these need to be public or have getters at all? Are they just used in
> the
> > merge function?
> 
> Well, its basically for the merge and generate geometry steps.  I think its a
> bit nicer than writing out the full fBatch., fBatchOpt., fGeoData[] etc.  Its
> your call.

Ok, but can we make them private?

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:477: currentState = ss;
On 2015/01/22 15:47:46, joshualitt wrote:
> On 2015/01/21 21:39:29, bsalomon wrote:
> > misaligned
> 
> I'm sorry, maybe I'm just blind but was is misaligned here?

currentState = ss is tabbed in when it seems like it should be aligned with the
preceding }

https://codereview.chromium.org/845103005/diff/300001/src/gpu/GrBatch.h
File src/gpu/GrBatch.h (right):

https://codereview.chromium.org/845103005/diff/300001/src/gpu/GrBatch.h#newco...
src/gpu/GrBatch.h:56: virtual void initBatchOpt(const GrBatchOpt&) = 0;
The virtuals that the client overrides could use some documentation.

[joshualitt, 2015-01-22 16:18:57]

On 2015/01/22 16:00:59, bsalomon wrote:
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
> File src/gpu/GrAARectRenderer.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
> src/gpu/GrAARectRenderer.cpp:80: void
> getInvariantOutputCoverage(GrInitInvariantOutput* out,
> On 2015/01/22 15:47:46, joshualitt wrote:
> > On 2015/01/22 14:05:02, bsalomon wrote:
> > > need a \n here
> > 
> > I'm confused, isn't there a newline here?
> 
> before this line, after the preceeding }
> 
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
> src/gpu/GrAARectRenderer.cpp:94: void initBatchTracker(const
> GrGeometryProcessor::InitBT& init) SK_OVERRIDE {
> On 2015/01/22 15:47:46, joshualitt wrote:
> > There really isn't a 'batch tracker object' in batch world.  AAFillRect just
> has
> > a local struct for documenting batch properties.
> > 
> > Even if right now none of the Batches are really going to do much with batch
> > properties, I'd like to leave the functionality in for the short term.  I
> > definitely think there might be possible optimizations we can do in the
> future,
> > ie if the viewmatrix doesn't have perspective, AAFillRect can still batch,
but
> > it'd have to track this decision somewhere.
> > 
> > However, I can rename the function if you want.  We have to do two init
steps
> in
> > the short term unfortunately because non-batch uses the BatchTracker on
> > SetState, but in the long term I think we can move to a single init on
GrBatch
> > called in OptState.  I can leave a todo
> 
> By single init do you mean combining initBatchTracker and initBatchOpt?
> 
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrAARectRendere...
> src/gpu/GrAARectRenderer.cpp:137: GrColor color() const { return
fBatch.fColor;
> }
> On 2015/01/22 15:47:46, joshualitt wrote:
> > On 2015/01/22 14:05:02, bsalomon wrote:
> > > do all these need to be public or have getters at all? Are they just used
in
> > the
> > > merge function?
> > 
> > Well, its basically for the merge and generate geometry steps.  I think its
a
> > bit nicer than writing out the full fBatch., fBatchOpt., fGeoData[] etc. 
Its
> > your call.
> 
> Ok, but can we make them private?
> 
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
> File src/gpu/GrInOrderDrawBuffer.cpp (right):
> 
>
https://codereview.chromium.org/845103005/diff/260001/src/gpu/GrInOrderDrawBu...
> src/gpu/GrInOrderDrawBuffer.cpp:477: currentState = ss;
> On 2015/01/22 15:47:46, joshualitt wrote:
> > On 2015/01/21 21:39:29, bsalomon wrote:
> > > misaligned
> > 
> > I'm sorry, maybe I'm just blind but was is misaligned here?
> 
> currentState = ss is tabbed in when it seems like it should be aligned with
the
> preceding }
> 
> https://codereview.chromium.org/845103005/diff/300001/src/gpu/GrBatch.h
> File src/gpu/GrBatch.h (right):
> 
>
https://codereview.chromium.org/845103005/diff/300001/src/gpu/GrBatch.h#newco...
> src/gpu/GrBatch.h:56: virtual void initBatchOpt(const GrBatchOpt&) = 0;
> The virtuals that the client overrides could use some documentation.

I think I got the feedback.  There were some tabs on the currentState line which
is why it was showing up funny.  Not sure how they got there.

[bsalomon, 2015-01-22 21:17:17]

https://codereview.chromium.org/845103005/diff/340001/src/gpu/GrBatch.h
File src/gpu/GrBatch.h (right):

https://codereview.chromium.org/845103005/diff/340001/src/gpu/GrBatch.h#newco...
src/gpu/GrBatch.h:59: * is a hook for the some additional overrides from the
GrXferProcessor.  This is a bit
It still feels like InitBT should be renamed and/or moved out of GrPrimProc...
it doesn't really have anything to do with batching at this point.

[joshualitt, 2015-01-23 15:25:56]

On 2015/01/22 21:17:17, bsalomon wrote:
> https://codereview.chromium.org/845103005/diff/340001/src/gpu/GrBatch.h
> File src/gpu/GrBatch.h (right):
> 
>
https://codereview.chromium.org/845103005/diff/340001/src/gpu/GrBatch.h#newco...
> src/gpu/GrBatch.h:59: * is a hook for the some additional overrides from the
> GrXferProcessor.  This is a bit
> It still feels like InitBT should be renamed and/or moved out of GrPrimProc...
> it doesn't really have anything to do with batching at this point.

Okay, renamed to GrPipelineInfo and moved out of PrimProc class. PTAL

[bsalomon, 2015-01-23 15:58:33]

lgtm

[joshualitt, 2015-01-26 15:22:20]

Brian, please take another quick look.  I added a new gm to catch the bug, and
the actual bug fix is in GrInOrderDrawBuffer.cpp::onFlush.  I also had a quick
question in AARectRenderer.cpp

https://codereview.chromium.org/845103005/diff/400001/src/gpu/GrAARectRendere...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/845103005/diff/400001/src/gpu/GrAARectRendere...
src/gpu/GrAARectRenderer.cpp:480: target->drawBatch(pipelineBuilder, batch,
kTriangles_GrPrimitiveType);
Brian, should I be passing in devrect?

[bsalomon, 2015-01-26 18:46:18]

We should be passing in the devrect to make smaller dst copies.

https://codereview.chromium.org/845103005/diff/450001/gm/dstreadshuffle.cpp
File gm/dstreadshuffle.cpp (right):

https://codereview.chromium.org/845103005/diff/450001/gm/dstreadshuffle.cpp#n...
gm/dstreadshuffle.cpp:27: kShapeTypeCircle,
style nit, kCircle_ShapeType, etc

https://codereview.chromium.org/845103005/diff/450001/gm/dstreadshuffle.cpp#n...
gm/dstreadshuffle.cpp:101: color = SK_ColorTRANSPARENT;
breaks?

https://codereview.chromium.org/845103005/diff/450001/gm/dstreadshuffle.cpp#n...
gm/dstreadshuffle.cpp:121: p->setStyle(SkPaint::kStroke_Style);
breaks in here?

[joshualitt, 2015-01-26 21:23:09]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-01-26 21:24:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/845103005/570001

[commit-bot: I haz the power, 2015-01-26 21:30:14]

Committed patchset #30 (id:570001) as
https://skia.googlesource.com/skia/+/d15e4e45374275c045572b304c229237c4a82be4

[joshualitt, 2015-01-27 14:40:28]

A revert of this CL (patchset #30 id:570001) has been created in
https://codereview.chromium.org/862823004/ by joshualitt@google.com.

The reason for reverting is: creates large performance regression.

[joshualitt, 2015-01-27 21:46:53]

Patchset #32 (id:610001) has been deleted

[joshualitt, 2015-01-27 23:32:05]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-01-27 23:33:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/845103005/630001

[commit-bot: I haz the power, 2015-01-27 23:39:13]

Committed patchset #32 (id:630001) as
https://skia.googlesource.com/skia/+/d5a7db4a867c7e6ccf8451a053d987b470099198

[Kimmo Kinnunen, 2015-01-28 13:08:49]

kkinnunen@nvidia.com changed reviewers:
+ kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-01-28 13:08:51]

Just for reference to whom it may concern:
I'm seeing small non-determinism with the pixels in dstreadshuffle, when run
with MSAA-related modes:

gm/dstreadshuffle.png
0.1033% of pixels differ (0.0148% weighted)
(372 pixels)

Max alpha channel mismatch 114
Total alpha channel mismatch 326

Average color mismatch 0
Max color mismatch 163

[joshualitt, 2015-01-28 14:53:12]

A revert of this CL (patchset #32 id:630001) has been created in
https://codereview.chromium.org/877393002/ by joshualitt@google.com.

The reason for reverting is: One last try to fix mac perf regression.

[joshualitt, 2015-01-28 18:35:20]

On 2015/01/28 14:53:12, joshualitt wrote:
> A revert of this CL (patchset #32 id:630001) has been created in
> https://codereview.chromium.org/877393002/ by mailto:joshualitt@google.com.
> 
> The reason for reverting is: One last try to fix mac perf regression.

Brian, please take a look.  This is the version of the fix I want to land.

Kimmo, those msaa flakes unfortunately happen even without this CL.  I've
uploaded the gm on its own, https://codereview.chromium.org/883053002/

[bsalomon, 2015-01-28 18:45:25]

lgtm

[joshualitt, 2015-01-28 20:31:47]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-01-28 20:32:00]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2015-01-28 20:43:41]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-01-28 20:44:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/845103005/690001

[commit-bot: I haz the power, 2015-01-28 20:53:58]

Committed patchset #35 (id:690001) as
https://skia.googlesource.com/skia/+/4d8da81562852e0ff7e18b66ee1cebd50ad81ee8