Ovals batch

Ovals batch

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/76e7fb6cbaa0994778cc4cf48bfe5e70bae59821

[joshualitt, 2015-02-05 22:24:44]

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

[joshualitt, 2015-02-09 17:46:55]

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

[joshualitt, 2015-02-09 17:46:56]

whoops, might have forgot to publish this

[bsalomon, 2015-02-10 14:31:21]

Can we break up the cpp? It's getting a bit out of hand.

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

https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
src/gpu/GrOvalRenderer.cpp:719: void
getInvariantOutputCoverage(GrInitInvariantOutput* out) const SK_OVERRIDE {
\n

https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
src/gpu/GrOvalRenderer.cpp:851: // TODO vertex color
unclear... use vertex colors to avoid breaking batches for color changes?

https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
src/gpu/GrOvalRenderer.cpp:1000: if (!this->viewMatrix().invert(&invert)) {
wonder if fin future we will need to do this inversion up front... so that we
know the total # of verts before genGeo called. I guess we could always insert
dummy offscreen verts if the matrix failed to invert.

https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
src/gpu/GrOvalRenderer.cpp:1645: class RRectCircleRendererBatch : public GrBatch
{
wondering if circular rrects can batch with circles (and same for e.r.r. and
ellipses). Might want to check with Jim.

[joshualitt, 2015-02-10 15:08:08]

feedback incorporated.  As mentioned offline, I'm planning a huge cleanup pass
after we've implemented GrBatch to tidy up all of these files and move logic out
of GrContext.

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

https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
src/gpu/GrOvalRenderer.cpp:1000: if (!this->viewMatrix().invert(&invert)) {
On 2015/02/10 14:31:21, bsalomon wrote:
> wonder if fin future we will need to do this inversion up front... so that we
> know the total # of verts before genGeo called. I guess we could always insert
> dummy offscreen verts if the matrix failed to invert.

The model I'm kind of imagining for this is basically 'worst case' vertex
counts.  Paths, for example, are going to be expensive to precompute unless we
can use worst case numbers.  Actually, do SkPaths count the number of
quads/conics/lines?  We probably can get a tight fitting number in many cases. 
I'm not sure how much effort we should put into tightly packing vertex buffers
though, but maybe it is worth it.

I think we'd actually have to fallback to paths or something if we can't invert?

[joshualitt, 2015-02-11 16:04:08]

On 2015/02/10 15:08:08, joshualitt wrote:
> feedback incorporated.  As mentioned offline, I'm planning a huge cleanup pass
> after we've implemented GrBatch to tidy up all of these files and move logic
out
> of GrContext.
> 
>
https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.cpp
> File src/gpu/GrOvalRenderer.cpp (right):
> 
>
https://codereview.chromium.org/904753002/diff/20001/src/gpu/GrOvalRenderer.c...
> src/gpu/GrOvalRenderer.cpp:1000: if (!this->viewMatrix().invert(&invert)) {
> On 2015/02/10 14:31:21, bsalomon wrote:
> > wonder if fin future we will need to do this inversion up front... so that
we
> > know the total # of verts before genGeo called. I guess we could always
insert
> > dummy offscreen verts if the matrix failed to invert.
> 
> The model I'm kind of imagining for this is basically 'worst case' vertex
> counts.  Paths, for example, are going to be expensive to precompute unless we
> can use worst case numbers.  Actually, do SkPaths count the number of
> quads/conics/lines?  We probably can get a tight fitting number in many cases.

> I'm not sure how much effort we should put into tightly packing vertex buffers
> though, but maybe it is worth it.
> 
> I think we'd actually have to fallback to paths or something if we can't
invert?

Any other feedback Brian?

[bsalomon, 2015-02-11 16:43:47]

lgtm

[joshualitt, 2015-02-11 16:45:24]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-11 16:46:05]

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

[commit-bot: I haz the power, 2015-02-11 16:52:31]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/76e7fb6cbaa0994778cc4cf48bfe5e70bae59821