Create GrDraw and start fast pathing src over rects

Create GrDraw and start fast pathing src over rects

BUG=skia:

[joshualitt, 2015-09-22 19:43:16]

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

[joshualitt, 2015-09-22 19:43:16]

okay, the only thing left really to do is to move the storage for the drawsnap
to the batch itself.

[bsalomon, 2015-09-23 13:49:38]

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

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:115: template <class T> T* getSnapStorage() {
Can we do anything here with var args to forward constructor args a la
SkTLazy::init?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:163: GrDrawSnap*                        fSnap;
alignment

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrNonAA...
File src/gpu/batches/GrNonAAStrokeRectBatch.h (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrNonAA...
src/gpu/batches/GrNonAAStrokeRectBatch.h:29: void Append(GrBatch*,
Probably can take GrDrawBatch here (and other Appends). Also should probably doc
that this must be a batch returned by the above Create.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h
File src/gpu/draws/GrDraw.h (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:22: * for batching.
"fast path" needs some explanation. something about skipping intermediate
transformations and pushing a new bit of geometry directly into an existing
batch.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:61: class GrDrawSnap : public GrNonAtomicRef {
/** ... */

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
File src/gpu/draws/GrDrawRect.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:40: , fMaskFilter(SkSafeRef(paint.getMaskFilter()))
Maybe just punt if the originating paint has a IM, MF, or CF? Going to be very
rare.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:68: static bool CanSnap(const SkPaint& paint) {
kSrcOver_Mode is always a nullptr for paint.getXfermode(). Maybe save a function
call here and just return nullptr == paint.getXfermode()?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:125: case kAAStroke_RectBatchType:
Would it make sense to store an Append()/Create() proc ptr pair rather than have
an enum?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:168: // TODO we also do this on the paint, do we
need to?
I'd just punt on CF. I'm also changing how CF works in skptogrp

[robertphillips, 2015-09-25 12:48:49]

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

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:509: 
Where did this use to get handled ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
File src/gpu/batches/GrAAFillRectBatch.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
src/gpu/batches/GrAAFillRectBatch.cpp:316: const SkRect& rect,
We still need 'devRect' here ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
src/gpu/batches/GrAAFillRectBatch.cpp:355: const SkRect& rect,
We still need 'devRect' here ?

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

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:163: GrDrawSnap*                        fSnap;
How was 256 arrived at ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h
File src/gpu/draws/GrDraw.h (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:32: virtual void execute(GrDrawTarget*) const=0;
Some comments about how these class IDs work and what is expected of derived
classes ?

// Each derived class must have a unique class id. This is achieved by providing
each
// derived class with a templatized version of ClassID() which uses the
GrDraw-global
// gCurrDrawClassID counter.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:55: static int32_t gCurrDrawClassID;
Do we need/even use 'fClassID'. Don't we just call <derived-class>::ClassID() ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:60: 
I second the motion for a comment

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:64: uint32_t classID() const {
use != ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
File src/gpu/draws/GrDrawRect.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:25: 
AAStroke
AAFill
BWStroke
BWFill

?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:112: 
drawSnap -> getLastSnap ?

calling drawSnap here sounds like we're doing something else

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:125: case kAAStroke_RectBatchType:
We would still have to deal with all the different parameters.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:154: 
Didn't we pull all the info for fStrokeInfo out of the SkPaint ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:175: RectBatchType batchType =
compute_rect_batch_type(useAA, width);
can_fastpath -> attempt_fastpath ?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:211: drawTarget->drawBatch(pipelineBuilder, batch);
So if we can't snap we just don't draw ?

[joshualitt, 2015-09-25 18:17:13]

feedback incorporated, I also got rid of the virtual call to execute.

There are three TODOs before I can land this:
1) clipping is not correctly handled right now, I can in advance compute the
draw bounds and pass them down.  I can also make only batch in cases where we
have a wide open clip or the clip does not apply.  This is probably a good
starting point
2) I'm not correctly not batching if I get an override color
3) I don't handle the case of src over degenerating into src mode.

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

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:509: 
On 2015/09/25 12:48:48, robertphillips wrote:
> Where did this use to get handled

at the SkGpuDevice::drawRect level

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
File src/gpu/batches/GrAAFillRectBatch.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
src/gpu/batches/GrAAFillRectBatch.cpp:316: const SkRect& rect,
On 2015/09/25 12:48:48, robertphillips wrote:
> We still need 'devRect' here ?

I can fix in a follow on

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrAAFil...
src/gpu/batches/GrAAFillRectBatch.cpp:355: const SkRect& rect,
On 2015/09/25 12:48:48, robertphillips wrote:
> We still need 'devRect' here ?


I can fix in a follow on

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

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:115: template <class T> T* getSnapStorage() {
On 2015/09/23 13:49:38, bsalomon wrote:
> Can we do anything here with var args to forward constructor args a la
> SkTLazy::init?

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:163: GrDrawSnap*                        fSnap;
On 2015/09/23 13:49:38, bsalomon wrote:
> alignment

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:163: GrDrawSnap*                        fSnap;
On 2015/09/25 12:48:48, robertphillips wrote:
> How was 256 arrived at ?

magic!  I'll add a comment

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrNonAA...
File src/gpu/batches/GrNonAAStrokeRectBatch.h (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/batches/GrNonAA...
src/gpu/batches/GrNonAAStrokeRectBatch.h:29: void Append(GrBatch*,
On 2015/09/23 13:49:38, bsalomon wrote:
> Probably can take GrDrawBatch here (and other Appends). Also should probably
doc
> that this must be a batch returned by the above Create.

I can't unfortunately because lastBatch on GrDrawTarget returns a GrBatch.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h
File src/gpu/draws/GrDraw.h (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:22: * for batching.
On 2015/09/23 13:49:38, bsalomon wrote:
> "fast path" needs some explanation. something about skipping intermediate
> transformations and pushing a new bit of geometry directly into an existing
> batch.

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:32: virtual void execute(GrDrawTarget*) const=0;
On 2015/09/25 12:48:49, robertphillips wrote:
> Some comments about how these class IDs work and what is expected of derived
> classes ?
> 
> // Each derived class must have a unique class id. This is achieved by
providing
> each
> // derived class with a templatized version of ClassID() which uses the
> GrDraw-global
> // gCurrDrawClassID counter.

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:55: static int32_t gCurrDrawClassID;
On 2015/09/25 12:48:49, robertphillips wrote:
> Do we need/even use 'fClassID'. Don't we just call <derived-class>::ClassID()
?

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:60: 
On 2015/09/25 12:48:48, robertphillips wrote:
> I second the motion for a comment

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDraw.h#...
src/gpu/draws/GrDraw.h:61: class GrDrawSnap : public GrNonAtomicRef {
On 2015/09/23 13:49:38, bsalomon wrote:
> /** ... */

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
File src/gpu/draws/GrDrawRect.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:25: 
On 2015/09/25 12:48:49, robertphillips wrote:
> AAStroke
> AAFill
> BWStroke
> BWFill
> 
> ?

Brian doesn't like BW

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:40: , fMaskFilter(SkSafeRef(paint.getMaskFilter()))
On 2015/09/23 13:49:38, bsalomon wrote:
> Maybe just punt if the originating paint has a IM, MF, or CF? Going to be very
> rare.

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:68: static bool CanSnap(const SkPaint& paint) {
On 2015/09/23 13:49:38, bsalomon wrote:
> kSrcOver_Mode is always a nullptr for paint.getXfermode(). Maybe save a
function
> call here and just return nullptr == paint.getXfermode()?

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:112: 
On 2015/09/25 12:48:49, robertphillips wrote:
> drawSnap -> getLastSnap ?
> 
> calling drawSnap here sounds like we're doing something else 

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:125: case kAAStroke_RectBatchType:
On 2015/09/25 12:48:49, robertphillips wrote:
> We would still have to deal with all the different parameters.

yea, I think in the short term this is the best way to handle things.  I think
we can come up with something better, but I'm not sure what that is at this
point in time.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:154: 
On 2015/09/25 12:48:49, robertphillips wrote:
> Didn't we pull all the info for fStrokeInfo out of the SkPaint ?

yes, but why pull it out again if we don't have to?

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:168: // TODO we also do this on the paint, do we
need to?
On 2015/09/23 13:49:38, bsalomon wrote:
> I'd just punt on CF. I'm also changing how CF works in skptogrp

I need to handle the color here either way unfortunately.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:175: RectBatchType batchType =
compute_rect_batch_type(useAA, width);
On 2015/09/25 12:48:49, robertphillips wrote:
> can_fastpath -> attempt_fastpath ?

Acknowledged.

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:211: drawTarget->drawBatch(pipelineBuilder, batch);
On 2015/09/25 12:48:49, robertphillips wrote:
> So if we can't snap we just don't draw ?

If we can't snap, we *do* draw(drawTarget->drawBatch above), we just leave the
snap on the batch null

[robertphillips, 2015-09-25 20:00:35]

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
File src/gpu/draws/GrDrawRect.cpp (right):

https://codereview.chromium.org/1355353002/diff/80001/src/gpu/draws/GrDrawRec...
src/gpu/draws/GrDrawRect.cpp:25: 
On 2015/09/25 18:17:13, joshualitt wrote:
> On 2015/09/25 12:48:49, robertphillips wrote:
> > AAStroke
> > AAFill
> > BWStroke
> > BWFill
> > 
> > ?
> 
> Brian doesn't like BW

I proposing a reordering not a renaming.