Hairline batch

Hairline batch

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/6eff8701f027016fbb3147412ec2292dcec2b7f5

Committed: https://skia.googlesource.com/skia/+/658d55cd6121c67488aaf5d0832c9712737f26a5

Committed: https://skia.googlesource.com/skia/+/7bc18b75ee473cd90c5577270e378f99cedd4ab9

[joshualitt, 2015-01-26 16:19:31]

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

[joshualitt, 2015-01-26 16:19:45]

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

[joshualitt, 2015-01-26 16:19:46]

Hairlines working with batch, please take a look

[bsalomon, 2015-01-26 19:07:48]

https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h
File include/core/SkPath.h (left):

https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h#ol...
include/core/SkPath.h:33: SK_DECLARE_INST_COUNT(SkPath);
??

https://codereview.chromium.org/876673002/diff/80001/src/gpu/GrAAHairLinePath...
File src/gpu/GrAAHairLinePathRenderer.cpp (right):

https://codereview.chromium.org/876673002/diff/80001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:259: int generate_lines_and_quads(const
SkPath& path,
let's rename this, generate is becoming overloaded. gather_lines_and_quads?

https://codereview.chromium.org/876673002/diff/80001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:713: static GrBatch* Create(const Geometry&
geometry, const GrIndexBuffer* linesIndexBuffer,
I think we should come up with a way for these to be accessible in genGeo,
rather than baked into grbatch

https://codereview.chromium.org/876673002/diff/80001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:750: bool onCombineIfPossible(GrBatch* t)
SK_OVERRIDE {
private?

https://codereview.chromium.org/876673002/diff/80001/src/gpu/GrAAHairLinePath...
src/gpu/GrAAHairLinePathRenderer.cpp:1163: // NOTE we don't currently do
anything with the strokerec.
didn't we already respect it above (if IsStrokeHairline...)?

[joshualitt, 2015-01-26 19:33:40]

https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h
File include/core/SkPath.h (left):

https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h#ol...
include/core/SkPath.h:33: SK_DECLARE_INST_COUNT(SkPath);
On 2015/01/26 19:07:47, bsalomon wrote:
> ??

I deleted this because SkPath no longer inherits from SkRefCntBase, was this
incorrect?

[bsalomon, 2015-01-26 19:35:08]

On 2015/01/26 19:33:40, joshualitt wrote:
> https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h
> File include/core/SkPath.h (left):
> 
>
https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h#ol...
> include/core/SkPath.h:33: SK_DECLARE_INST_COUNT(SkPath);
> On 2015/01/26 19:07:47, bsalomon wrote:
> > ??
> 
> I deleted this because SkPath no longer inherits from SkRefCntBase, was this
> incorrect?

It never did... I don't think inst counting is restricted to SkRefCnt-derived
classes.

[joshualitt, 2015-01-26 19:39:26]

On 2015/01/26 19:35:08, bsalomon wrote:
> On 2015/01/26 19:33:40, joshualitt wrote:
> > https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h
> > File include/core/SkPath.h (left):
> > 
> >
>
https://codereview.chromium.org/876673002/diff/80001/include/core/SkPath.h#ol...
> > include/core/SkPath.h:33: SK_DECLARE_INST_COUNT(SkPath);
> > On 2015/01/26 19:07:47, bsalomon wrote:
> > > ??
> > 
> > I deleted this because SkPath no longer inherits from SkRefCntBase, was this
> > incorrect?
> 
> It never did... I don't think inst counting is restricted to SkRefCnt-derived
> classes.

feedback inc

[bsalomon, 2015-01-26 19:40:35]

lgtm

[joshualitt, 2015-02-02 14:50:21]

On 2015/01/26 19:40:35, bsalomon wrote:
> lgtm

Brian, could you take another quick look at this patch.  I've made some changes.
 Commit 8 was the last one you reviewed I think.

[bsalomon, 2015-02-02 21:01:13]

https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
File src/gpu/GrAAHairLinePathRenderer.cpp (right):

https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
seems to me like batch target should track this, no?

[joshualitt, 2015-02-02 21:19:25]

On 2015/02/02 21:01:13, bsalomon wrote:
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> File src/gpu/GrAAHairLinePathRenderer.cpp (right):
> 
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
> seems to me like batch target should track this, no?

My first thought too, but its unfortunately not that simple.  The batchTarget
really has no facility for this tracking this because it works at a more
granular level.  I could have added a heap array of # of draws associated with
each batch, but it'd seemed like overkill to push and pop a bunch of heap data
just to track what amounts to an integer value per batch.  I don't really love
this solution either, but its only until we have batch everywhere.

[bsalomon, 2015-02-02 21:31:58]

On 2015/02/02 21:19:25, joshualitt wrote:
> On 2015/02/02 21:01:13, bsalomon wrote:
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > File src/gpu/GrAAHairLinePathRenderer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
> > seems to me like batch target should track this, no?
> 
> My first thought too, but its unfortunately not that simple.  The batchTarget
> really has no facility for this tracking this because it works at a more
> granular level.  I could have added a heap array of # of draws associated with
> each batch, but it'd seemed like overkill to push and pop a bunch of heap data
> just to track what amounts to an integer value per batch.  I don't really love
> this solution either, but its only until we have batch everywhere.

Could the batchtarget just have a draw counter that the iodb (or whoever) resets
after calling the batch?

[joshualitt, 2015-02-02 21:33:07]

On 2015/02/02 21:31:58, bsalomon wrote:
> On 2015/02/02 21:19:25, joshualitt wrote:
> > On 2015/02/02 21:01:13, bsalomon wrote:
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > File src/gpu/GrAAHairLinePathRenderer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
> > > seems to me like batch target should track this, no?
> > 
> > My first thought too, but its unfortunately not that simple.  The
batchTarget
> > really has no facility for this tracking this because it works at a more
> > granular level.  I could have added a heap array of # of draws associated
with
> > each batch, but it'd seemed like overkill to push and pop a bunch of heap
data
> > just to track what amounts to an integer value per batch.  I don't really
love
> > this solution either, but its only until we have batch everywhere.
> 
> Could the batchtarget just have a draw counter that the iodb (or whoever)
resets
> after calling the batch?

Yes, but how would I store that info?  I'm okay with doing that if the
batchTarget sets the batches generated on the batches itself.

[bsalomon, 2015-02-02 21:43:28]

On 2015/02/02 21:33:07, joshualitt wrote:
> On 2015/02/02 21:31:58, bsalomon wrote:
> > On 2015/02/02 21:19:25, joshualitt wrote:
> > > On 2015/02/02 21:01:13, bsalomon wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > > File src/gpu/GrAAHairLinePathRenderer.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > > src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
> > > > seems to me like batch target should track this, no?
> > > 
> > > My first thought too, but its unfortunately not that simple.  The
> batchTarget
> > > really has no facility for this tracking this because it works at a more
> > > granular level.  I could have added a heap array of # of draws associated
> with
> > > each batch, but it'd seemed like overkill to push and pop a bunch of heap
> data
> > > just to track what amounts to an integer value per batch.  I don't really
> love
> > > this solution either, but its only until we have batch everywhere.
> > 
> > Could the batchtarget just have a draw counter that the iodb (or whoever)
> resets
> > after calling the batch?
> 
> Yes, but how would I store that info?  I'm okay with doing that if the
> batchTarget sets the batches generated on the batches itself.

That seems ok. friend GrBatch to GrBatchTarget maybe? I'm not crazy about that
but ok if its temporary.

[joshualitt, 2015-02-02 22:26:55]

On 2015/02/02 21:43:28, bsalomon wrote:
> On 2015/02/02 21:33:07, joshualitt wrote:
> > On 2015/02/02 21:31:58, bsalomon wrote:
> > > On 2015/02/02 21:19:25, joshualitt wrote:
> > > > On 2015/02/02 21:01:13, bsalomon wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > > > File src/gpu/GrAAHairLinePathRenderer.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/876673002/diff/260001/src/gpu/GrAAHairLinePat...
> > > > > src/gpu/GrAAHairLinePathRenderer.cpp:869: fBatchesGenerated++;
> > > > > seems to me like batch target should track this, no?
> > > > 
> > > > My first thought too, but its unfortunately not that simple.  The
> > batchTarget
> > > > really has no facility for this tracking this because it works at a more
> > > > granular level.  I could have added a heap array of # of draws
associated
> > with
> > > > each batch, but it'd seemed like overkill to push and pop a bunch of
heap
> > data
> > > > just to track what amounts to an integer value per batch.  I don't
really
> > love
> > > > this solution either, but its only until we have batch everywhere.
> > > 
> > > Could the batchtarget just have a draw counter that the iodb (or whoever)
> > resets
> > > after calling the batch?
> > 
> > Yes, but how would I store that info?  I'm okay with doing that if the
> > batchTarget sets the batches generated on the batches itself.
> 
> That seems ok. friend GrBatch to GrBatchTarget maybe? I'm not crazy about that
> but ok if its temporary.

Yea, I'm not thrilled with doing this either, but its the least of two evils. 
Feedback incorporated.

[bsalomon, 2015-02-03 00:37:08]

slight quibble with the name. calling the counts "batches" is a bit misleading
since they aren't GrBatches. Would it be more correct to count them as "draws"?

otherwise, lgtm

[joshualitt, 2015-02-03 01:12:08]

Patchset #17 (id:320001) has been deleted

[joshualitt, 2015-02-03 01:12:20]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-03 01:12:56]

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

[commit-bot: I haz the power, 2015-02-03 01:19:44]

Committed patchset #16 (id:300001) as
https://skia.googlesource.com/skia/+/6eff8701f027016fbb3147412ec2292dcec2b7f5

[joshualitt, 2015-02-03 02:11:51]

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

The reason for reverting is: Regression.

[joshualitt, 2015-02-03 14:48:27]

Patchset #17 (id:340001) has been deleted

[joshualitt, 2015-02-03 14:49:31]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-03 14:50:29]

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

[commit-bot: I haz the power, 2015-02-03 14:56:49]

Committed patchset #17 (id:360001) as
https://skia.googlesource.com/skia/+/658d55cd6121c67488aaf5d0832c9712737f26a5

[joshualitt, 2015-02-03 16:39:32]

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

The reason for reverting is: still a performance regression.

[joshualitt, 2015-02-04 00:35:42]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2015-02-04 00:36:16]

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

[commit-bot: I haz the power, 2015-02-04 00:41:46]

Committed patchset #26 (id:540001) as
https://skia.googlesource.com/skia/+/7bc18b75ee473cd90c5577270e378f99cedd4ab9