Context batches

Context batches

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/0823bfae151dd27f364db3a7f07017a7cef6bfdd

[joshualitt, 2015-02-24 22:17:26]

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

[robertphillips, 2015-02-25 15:50:26]

First pass

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:487: GrColor fColor;
Not the view matrix again :(

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:602: 
specified triangle -> specified rect ?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:603: /*  create a triangle strip that strokes the
specified triangle. There are 8
repreat ?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:607: */
Do we need an assert that the stroke width/2 is less than both the height and
width? I'm pretty sure stroked rects that turn into rects are filtered out
earlier but an assert shouldn't hurt.

Also where is the handling for the other join types?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:608: void setStrokeRectStrip(SkPoint verts[10], SkRect
rect, SkScalar width) {
Can we call 'sort' on the SkRect when we put it in the batch and then skip it
here (and pass rect as a const ref) ?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:808: GrColor fColor;
:(

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:809: SkMatrix fViewMatrix;
SkTDArrays?
raw memory?
SkAutoFree/SkAutoMalloc?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:813: SkTArray<SkPoint>* fLocalCoords;
Aren't these last two duplicated in the SkTArrays?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:855: 
Not that I'm suggesting it right now but, could we do all this interleaving
before this and store the interleaved data?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:907: }
Can we ever delete a DrawVerticesBatch without first calling generateGeometry?
Also, aren't these all inited to NULL so can't we just blanket delete them all
at the end?

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:960: installedGeo.fPositions =
SkNEW_ARGS(SkTArray<SkPoint>, (positions, vertexCount));
Don't think you need these SkToBools here

https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:962: installedGeo.fIndices =
SkNEW_ARGS(SkTArray<uint16_t>, (indices, geometry.fIndexCount));
Can we use the NULLness of fIndices in place of fHasIndices?

[joshualitt, 2015-02-27 16:36:45]

On 2015/02/25 15:50:26, robertphillips wrote:
> First pass
> 
> https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:487: GrColor fColor;
> Not the view matrix again :(
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:602: 
> specified triangle -> specified rect ?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:603: /*  create a triangle strip that strokes the
> specified triangle. There are 8
> repreat ?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:607: */
> Do we need an assert that the stroke width/2 is less than both the height and
> width? I'm pretty sure stroked rects that turn into rects are filtered out
> earlier but an assert shouldn't hurt.
> 
> Also where is the handling for the other join types?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:608: void setStrokeRectStrip(SkPoint verts[10], SkRect
> rect, SkScalar width) {
> Can we call 'sort' on the SkRect when we put it in the batch and then skip it
> here (and pass rect as a const ref) ?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:808: GrColor fColor;
> :(
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:809: SkMatrix fViewMatrix;
> SkTDArrays?
> raw memory?
> SkAutoFree/SkAutoMalloc?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:813: SkTArray<SkPoint>* fLocalCoords;
> Aren't these last two duplicated in the SkTArrays?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:855: 
> Not that I'm suggesting it right now but, could we do all this interleaving
> before this and store the interleaved data?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:907: }
> Can we ever delete a DrawVerticesBatch without first calling generateGeometry?
> Also, aren't these all inited to NULL so can't we just blanket delete them all
> at the end?
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:960: installedGeo.fPositions =
> SkNEW_ARGS(SkTArray<SkPoint>, (positions, vertexCount));
> Don't think you need these SkToBools here
> 
>
https://codereview.chromium.org/913253003/diff/20001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:962: installedGeo.fIndices =
> SkNEW_ARGS(SkTArray<uint16_t>, (indices, geometry.fIndexCount));
> Can we use the NULLness of fIndices in place of fHasIndices?

feedback incorporated

[robertphillips, 2015-02-27 17:37:37]

lgtm + nits

https://codereview.chromium.org/913253003/diff/60001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/913253003/diff/60001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:620: // this is a bug
Shouldn't this be rect.width()/2 and rect.height()/2 ?

https://codereview.chromium.org/913253003/diff/60001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:642: bool onCombineIfPossible(GrBatch* t) SK_OVERRIDE {
missing space here ?

https://codereview.chromium.org/913253003/diff/60001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:917: 
interleaved ?

[joshualitt, 2015-02-27 17:59:58]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-02-27 17:59:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/913253003/#ps100001
(title: "cleanup")

[commit-bot: I haz the power, 2015-02-27 18:00:21]

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

[commit-bot: I haz the power, 2015-02-27 18:06:20]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/0823bfae151dd27f364db3a7f07017a7cef6bfdd