GPU implementation of drawRegion()

GPU implementation of drawRegion()

Nexus 6P drawregion Bench Performance
Before 48.0ms
After  3.57ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006

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

[msarett, 2016-08-25 16:18:18]

Description was changed from

==========
GPU implementation of drawRegion()

I think I'm taking a lot of shortcuts here, needs some work.

BUG=skia:
==========

to

==========
GPU implementation of drawRegion()

I think I'm taking a lot of shortcuts here, needs some work.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

[msarett, 2016-08-25 16:25:11]

Patchset #1 (id:1) has been deleted

[msarett, 2016-08-25 16:36:17]

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

[msarett, 2016-08-25 16:36:18]

This is a WIP, I've commented on some things that I need to follow up on...

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:957: SkAutoTUnref<GrDrawBatch>
batch(GrRegionBatch::Create(paint.getColor(), viewMatrix, region));
Probably not ok to assume that we can always just fill with the paint color...

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (left):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:79: const SkRect* localRect, const SkMatrix*
localMatrix)
Do I need to worry about localRect and localMatrix?

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:63: class RegionBatch : public GrVertexBatch {
Could/should this extend NonAAFillRectBatch?

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:71: while (!iter.done()) {
Any way to use push_back_n() here?

[bsalomon, 2016-08-25 17:49:03]

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:957: SkAutoTUnref<GrDrawBatch>
batch(GrRegionBatch::Create(paint.getColor(), viewMatrix, region));
On 2016/08/25 16:36:17, msarett wrote:
> Probably not ok to assume that we can always just fill with the paint color...

It's ok.. if there is a shader or what not, that will be applied by the pipeline
stuff below.

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

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:534: bool isNonTranslate = draw.fMatrix->getType() &
~(SkMatrix::kTranslate_Mask);
I think we should handle all the matrix, aa, and style stuff in GrDrawContext.
I'd just handle the mask filter bail out here.

GrDrawContext::drawRegion(clip, grpaint, matrix, region, GrStyle(paint))

in GrDC GrStyle can be queried using GrStyle::isSimpleFill() which will return
true if it is fill w/ no path effect.

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (left):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:79: const SkRect* localRect, const SkMatrix*
localMatrix)
On 2016/08/25 16:36:17, msarett wrote:
> Do I need to worry about localRect and localMatrix?

no

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:63: class RegionBatch : public GrVertexBatch {
On 2016/08/25 16:36:17, msarett wrote:
> Could/should this extend NonAAFillRectBatch?

I don't think it should inherit from it. It's possible we could store a region
as an alternative to a SkRect in NonAAFillRectBatch::Geom but I think that is
more complexity than we need. I'd just make it its own batch that stores exactly
one region, matrix, and color (for now).

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:71: while (!iter.done()) {
On 2016/08/25 16:36:17, msarett wrote:
> Any way to use push_back_n() here?

I think we should keep it in region form until generateGeometry()

[msarett, 2016-08-25 18:21:36]

Patchset #3 (id:60001) has been deleted

[msarett, 2016-08-25 18:21:40]

Patchset #3 (id:80001) has been deleted

[msarett, 2016-08-25 18:47:19]

Description was changed from

==========
GPU implementation of drawRegion()

I think I'm taking a lot of shortcuts here, needs some work.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

to

==========
GPU implementation of drawRegion()

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

[msarett, 2016-08-25 20:51:13]

Description was changed from

==========
GPU implementation of drawRegion()

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

to

==========
GPU implementation of drawRegion()

Nexus 6P drawregion Bench Performance
Before 48.0ms
After  3.57ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

[msarett, 2016-08-25 20:51:42]

Please take a look.  Performance looks great :).

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:957: SkAutoTUnref<GrDrawBatch>
batch(GrRegionBatch::Create(paint.getColor(), viewMatrix, region));
On 2016/08/25 17:49:02, bsalomon wrote:
> On 2016/08/25 16:36:17, msarett wrote:
> > Probably not ok to assume that we can always just fill with the paint
color...
> 
> It's ok.. if there is a shader or what not, that will be applied by the
pipeline
> stuff below.

Acknowledged.

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

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:534: bool isNonTranslate = draw.fMatrix->getType() &
~(SkMatrix::kTranslate_Mask);
On 2016/08/25 17:49:02, bsalomon wrote:
> I think we should handle all the matrix, aa, and style stuff in GrDrawContext.
> I'd just handle the mask filter bail out here.
> 
> GrDrawContext::drawRegion(clip, grpaint, matrix, region, GrStyle(paint))
> 
> in GrDC GrStyle can be queried using GrStyle::isSimpleFill() which will return
> true if it is fill w/ no path effect.

SGTM, done.

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (left):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:79: const SkRect* localRect, const SkMatrix*
localMatrix)
On 2016/08/25 17:49:02, bsalomon wrote:
> On 2016/08/25 16:36:17, msarett wrote:
> > Do I need to worry about localRect and localMatrix?
> 
> no

Acknowledged.

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
File src/gpu/batches/GrRegionBatch.cpp (right):

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:63: class RegionBatch : public GrVertexBatch {
On 2016/08/25 17:49:02, bsalomon wrote:
> On 2016/08/25 16:36:17, msarett wrote:
> > Could/should this extend NonAAFillRectBatch?
> 
> I don't think it should inherit from it. It's possible we could store a region
> as an alternative to a SkRect in NonAAFillRectBatch::Geom but I think that is
> more complexity than we need. I'd just make it its own batch that stores
exactly
> one region, matrix, and color (for now).

Alright cool, thanks.

https://codereview.chromium.org/2267273006/diff/40001/src/gpu/batches/GrRegio...
src/gpu/batches/GrRegionBatch.cpp:71: while (!iter.done()) {
On 2016/08/25 17:49:02, bsalomon wrote:
> On 2016/08/25 16:36:17, msarett wrote:
> > Any way to use push_back_n() here?
> 
> I think we should keep it in region form until generateGeometry()

Done.

[bsalomon, 2016-08-25 21:19:50]

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:963: bool complexPaint = !style.isSimpleFill();
maybe complexStyle?

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
File src/gpu/batches/GrRegionBatch.cpp (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:42: static const int kColorOffset =
sizeof(SkPoint);
 blah blah local coords

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:94: // When this is called on a batch, there
is only one geometry bundle
Can you change "geometry bundle" to "region"?

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:147: if (fOverrides.canTweakAlphaForCoverage()
&& !that->fOverrides.canTweakAlphaForCoverage()) {
This can be removed.

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
File src/gpu/batches/GrRegionBatch.h (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.h:2: * Copyright 2015 Google Inc.
2016

[msarett, 2016-08-25 21:23:13]

Patchset #4 (id:120001) has been deleted

[msarett, 2016-08-25 21:26:35]

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:963: bool complexPaint = !style.isSimpleFill();
On 2016/08/25 21:19:50, bsalomon wrote:
> maybe complexStyle?

Done.

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
File src/gpu/batches/GrRegionBatch.cpp (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:42: static const int kColorOffset =
sizeof(SkPoint);
On 2016/08/25 21:19:50, bsalomon wrote:
>  blah blah local coords

Done, thanks.

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:94: // When this is called on a batch, there
is only one geometry bundle
On 2016/08/25 21:19:50, bsalomon wrote:
> Can you change "geometry bundle" to "region"?

Done.

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.cpp:147: if (fOverrides.canTweakAlphaForCoverage()
&& !that->fOverrides.canTweakAlphaForCoverage()) {
On 2016/08/25 21:19:50, bsalomon wrote:
> This can be removed.

Done.

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
File src/gpu/batches/GrRegionBatch.h (right):

https://codereview.chromium.org/2267273006/diff/100001/src/gpu/batches/GrRegi...
src/gpu/batches/GrRegionBatch.h:2: * Copyright 2015 Google Inc.
On 2016/08/25 21:19:50, bsalomon wrote:
> 2016

Done.

[msarett, 2016-08-25 21:31:02]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 21:31:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 21:44:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 21:44:20]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2016-08-25 21:48:13]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 21:48:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 21:59:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 21:59:48]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[msarett, 2016-08-25 22:29:36]

Patchset #5 (id:160001) has been deleted

[msarett, 2016-08-25 22:29:41]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:29:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 22:55:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 22:55:08]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-08-25 23:55:08]

lgtm

https://codereview.chromium.org/2267273006/diff/180001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/180001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:962: bool isNonTranslate = (0 != (viewMatrix.getType()
& ~(SkMatrix::kTranslate_Mask)));
SkToBool?

[msarett, 2016-08-26 00:35:12]

https://codereview.chromium.org/2267273006/diff/180001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/2267273006/diff/180001/src/gpu/GrDrawContext....
src/gpu/GrDrawContext.cpp:962: bool isNonTranslate = (0 != (viewMatrix.getType()
& ~(SkMatrix::kTranslate_Mask)));
On 2016/08/25 23:55:08, bsalomon wrote:
> SkToBool?

Of course, thanks.  Done.

[msarett, 2016-08-26 00:35:18]

The CQ bit was checked by msarett@google.com

[msarett, 2016-08-26 00:35:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2267273006/#ps200001
(title: "SkToBool")

[commit-bot: I haz the power, 2016-08-26 00:35:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 01:07:21]

Description was changed from

==========
GPU implementation of drawRegion()

Nexus 6P drawregion Bench Performance
Before 48.0ms
After  3.57ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006
==========

to

==========
GPU implementation of drawRegion()

Nexus 6P drawregion Bench Performance
Before 48.0ms
After  3.57ms

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2267273006

Committed:
https://skia.googlesource.com/skia/+/cc319b95a58ae15e88c3c028b8726e7cab9121ff
==========

[commit-bot: I haz the power, 2016-08-26 01:07:22]

Committed patchset #6 (id:200001) as
https://skia.googlesource.com/skia/+/cc319b95a58ae15e88c3c028b8726e7cab9121ff