Add drawRegion() API to SkCanvas

Add drawRegion() API to SkCanvas

This will allow us to optimize for the RectGrid macrobench.
Currently, SkiaGL is much slower than OpenGL.
SkiaGL  12 items/s
OpenGL 160 items/s

This contains everything except for the fast implementation on GPU.

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

Committed: https://skia.googlesource.com/skia/+/44df651ebefc284acc2f66425dff3ea0b0e14b36

[msarett, 2016-08-24 21:39:48]

Description was changed from

==========
Add drawRegion() API to SkCanvas

This will allow us to optimize for the RectGrid macrobench.
Currently, SkiaGL is much slower than OpenGL.
SkiaGL  12 items/s
OpenGL 160 items/s

This contains everything except for the fast implementation on GPU.

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

to

==========
Add drawRegion() API to SkCanvas

This will allow us to optimize for the RectGrid macrobench.
Currently, SkiaGL is much slower than OpenGL.
SkiaGL  12 items/s
OpenGL 160 items/s

This contains everything except for the fast implementation on GPU.

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

[msarett, 2016-08-24 21:43:49]

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

[commit-bot: I haz the power, 2016-08-24 21:43:53]

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

[msarett, 2016-08-24 21:44:17]

msarett@google.com changed reviewers:
+ bsalomon@google.com, djsollen@google.com, mtklein@google.com, reed@google.com

[commit-bot: I haz the power, 2016-08-24 21:47:22]

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

[commit-bot: I haz the power, 2016-08-24 21:47:23]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

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

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

[commit-bot: I haz the power, 2016-08-24 21:48:33]

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-24 22:15:57]

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

[commit-bot: I haz the power, 2016-08-24 22:15:57]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-08-24 23:20:24]

Q: What should happen if the paint is AA and the matrix is say a rotation?

I believe the implementation as is will show interior cracks (in src-over
anyway).

Should the default impl sniff the matrix and if the rects aren't going to be
integer/axis-aligned make a path?

[bsalomon, 2016-08-25 00:35:52]

On 2016/08/24 23:20:24, bsalomon wrote:
> Q: What should happen if the paint is AA and the matrix is say a rotation?
> 
> I believe the implementation as is will show interior cracks (in src-over
> anyway).
> 
> Should the default impl sniff the matrix and if the rects aren't going to be
> integer/axis-aligned make a path?

Similar for mask/image filters.

[msarett, 2016-08-25 13:02:32]

We now convert the region to a path in the complex case.  I also tried to add a
gm with rotation.

https://codereview.chromium.org/2277053002/diff/10001/gm/drawregionrotate.cpp
File gm/drawregionrotate.cpp (right):

https://codereview.chromium.org/2277053002/diff/10001/gm/drawregionrotate.cpp...
gm/drawregionrotate.cpp:31: void onDraw(SkCanvas* canvas) override {
I wanted to write a gm that shows interior cracks when we don't use the path
implementation.  This one isn't working for me.  Brian, any thoughts?

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:78: if (complexMatrix && complexPaint) {
Hwui also checks that the paint must be kFill_Style before choosing "simple
rects".  Is this something that we also need to worry about?

[bsalomon, 2016-08-25 13:58:20]

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:78: if (complexMatrix && complexPaint) {
On 2016/08/25 13:02:32, msarett wrote:
> Hwui also checks that the paint must be kFill_Style before choosing "simple
> rects".  Is this something that we also need to worry about?

To me a clear definition of drawRegion(region, paint) is that it draws the same
as drawPath(region.getBoundaryPath(), paint).

If we accept that definition then a path effect or non-fill style would also
throw us out of the rect-at-a-time implementation.

[msarett, 2016-08-25 15:05:44]

Patchset #3 (id:30001) has been deleted

[msarett, 2016-08-25 15:07:35]

https://codereview.chromium.org/2277053002/diff/10001/gm/drawregionrotate.cpp
File gm/drawregionrotate.cpp (right):

https://codereview.chromium.org/2277053002/diff/10001/gm/drawregionrotate.cpp...
gm/drawregionrotate.cpp:31: void onDraw(SkCanvas* canvas) override {
On 2016/08/25 13:02:32, msarett wrote:
> I wanted to write a gm that shows interior cracks when we don't use the path
> implementation.  This one isn't working for me.  Brian, any thoughts?

Problem was that SkRegion was smartly combining all my rects into a single rect,
thanks Brian for catching this.

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2277053002/diff/10001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:78: if (complexMatrix && complexPaint) {
On 2016/08/25 13:58:20, bsalomon wrote:
> On 2016/08/25 13:02:32, msarett wrote:
> > Hwui also checks that the paint must be kFill_Style before choosing "simple
> > rects".  Is this something that we also need to worry about?
> 
> To me a clear definition of drawRegion(region, paint) is that it draws the
same
> as drawPath(region.getBoundaryPath(), paint).
> 
> If we accept that definition then a path effect or non-fill style would also
> throw us out of the rect-at-a-time implementation.

Agreed.

New checks should catch everything I think:
(1) Default to path if non-translate.
(2) Default to path if non-fill, mask filter, or path effect.
(3) Default to path if anti-alias AND we'll actually have non-integer pixels.

We'll miss the fast path on the case where we have a scale matrix with integer
scales - I think that's ok.

Image filters should work fine on the fast path.

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

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

[commit-bot: I haz the power, 2016-08-25 17:17:27]

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

[bsalomon, 2016-08-25 17:54:34]

Should the GM be consolidated into a single GM? Also, perhaps we should have a
region rendered with a shader.

https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h...
include/core/SkCanvas.h:705: /** Draw the specified region using the specified
paint.
Should we say "outline of the specified region"?

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

"Should the GM be consolidated into a single GM? Also, perhaps we should have a
region rendered with a shader."

Added a shader case to the test.

I like having the gms separate because I've marked the first
one (the Android one) to also run as a bench.  It's nice to
be able to measure the performance of that test.

https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h...
include/core/SkCanvas.h:705: /** Draw the specified region using the specified
paint.
On 2016/08/25 17:54:34, bsalomon wrote:
> Should we say "outline of the specified region"?

SGTM

[bsalomon, 2016-08-25 18:34:29]

lgtm

On 2016/08/25 18:25:55, msarett wrote:
> "Should the GM be consolidated into a single GM? Also, perhaps we should have
a
> region rendered with a shader."
> 
> Added a shader case to the test.
> 
> I like having the gms separate because I've marked the first
> one (the Android one) to also run as a bench.  It's nice to
> be able to measure the performance of that test.
> 

That makes sense

> https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h
> File include/core/SkCanvas.h (right):
> 
>
https://codereview.chromium.org/2277053002/diff/50001/include/core/SkCanvas.h...
> include/core/SkCanvas.h:705: /** Draw the specified region using the specified
> paint.
> On 2016/08/25 17:54:34, bsalomon wrote:
> > Should we say "outline of the specified region"?
> 
> SGTM

[msarett, 2016-08-25 18:45:15]

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

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

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

[reed1, 2016-08-25 19:12:53]

Is there another way to accelerate this except for adding it to all canvases?
SkRegion itself could/should be private except for this call (used only by
android afaik).

Can it be a custom drawable (in the derek sense) that we know to call a
(private) Ganesh API on? Seems sad to do so much work solely for a benchmark.

[bsalomon, 2016-08-25 19:19:13]

On 2016/08/25 19:12:53, reed1 wrote:
> Is there another way to accelerate this except for adding it to all canvases?
> SkRegion itself could/should be private except for this call (used only by
> android afaik).
> 
> Can it be a custom drawable (in the derek sense) that we know to call a
> (private) Ganesh API on? Seems sad to do so much work solely for a benchmark.


The framework actually does have a code path for drawing paths as regions that
isn't just used by the benchmark. The framework's path class tracks both a
SkRegion and SkPath representation and favors drawing the SkRegion if it thinks
it can get away with it.

[commit-bot: I haz the power, 2016-08-25 19:23:20]

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

[commit-bot: I haz the power, 2016-08-25 19:23:21]

Dry run: This issue passed the CQ dry run.

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

On 2016/08/25 19:12:53, reed1 wrote:
> Is there another way to accelerate this except for adding it to all canvases?
> SkRegion itself could/should be private except for this call (used only by
> android afaik).
> 
> Can it be a custom drawable (in the derek sense) that we know to call a
> (private) Ganesh API on? Seems sad to do so much work solely for a benchmark.

I think that's a good point - I'm opened to thinking about different ways to
expose this functionality to Android.  Though I wouldn't say that the API is
completely useless, given that we can get here from the Java API.  For now, I'm
going to go ahead and land - this will have a huge impact on a particular
Android test.

Happy to discuss and refactor later on.

[msarett, 2016-08-25 20:53:28]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-08-25 20:53:32]

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 20:54:39]

Description was changed from

==========
Add drawRegion() API to SkCanvas

This will allow us to optimize for the RectGrid macrobench.
Currently, SkiaGL is much slower than OpenGL.
SkiaGL  12 items/s
OpenGL 160 items/s

This contains everything except for the fast implementation on GPU.

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

to

==========
Add drawRegion() API to SkCanvas

This will allow us to optimize for the RectGrid macrobench.
Currently, SkiaGL is much slower than OpenGL.
SkiaGL  12 items/s
OpenGL 160 items/s

This contains everything except for the fast implementation on GPU.

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

Committed:
https://skia.googlesource.com/skia/+/44df651ebefc284acc2f66425dff3ea0b0e14b36
==========

[commit-bot: I haz the power, 2016-08-25 20:54:39]

Committed patchset #4 (id:70001) as
https://skia.googlesource.com/skia/+/44df651ebefc284acc2f66425dff3ea0b0e14b36

[reed1, 2016-08-26 12:08:18]

On 2016/08/25 20:00:42, msarett wrote:
> On 2016/08/25 19:12:53, reed1 wrote:
> > Is there another way to accelerate this except for adding it to all
canvases?
> > SkRegion itself could/should be private except for this call (used only by
> > android afaik).
> > 
> > Can it be a custom drawable (in the derek sense) that we know to call a
> > (private) Ganesh API on? Seems sad to do so much work solely for a
benchmark.
> 
> I think that's a good point - I'm opened to thinking about different ways to
> expose this functionality to Android.  Though I wouldn't say that the API is
> completely useless, given that we can get here from the Java API.  For now,
I'm
> going to go ahead and land - this will have a huge impact on a particular
> Android test.
> 
> Happy to discuss and refactor later on.

Hmmm, ok. We worked hard in the past to try to eliminate SkRegion from the API.
There are several other parts of the Java API that we "simulate" because we
chose deliberately to not have them in our canonical API (e.g. save-flags, etc.)
Florin as worked on some of these. I'm not claiming we *can't* get here via Java
API, just that I'm not convinced its known to be important enough.

Let's do talk about this on Monday.

[reed1, 2016-08-26 12:27:02]

https://codereview.chromium.org/2277053002/diff/70001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2277053002/diff/70001/include/core/SkCanvas.h...
include/core/SkCanvas.h:709: void drawRegion(const SkRegion& region, const
SkPaint& paint) {
tiny nit: most of these non-virtuals we also put in the cpp. We should think
about detecting empty and just-a-rect at this level, before we dispatch to the
virtuals.

[msarett, 2016-08-26 13:20:14]

> Hmmm, ok. We worked hard in the past to try to eliminate SkRegion from the
API.
> There are several other parts of the Java API that we "simulate" because we
> chose deliberately to not have them in our canonical API (e.g. save-flags,
etc.)
> Florin as worked on some of these. I'm not claiming we *can't* get here via
Java
> API, just that I'm not convinced its known to be important enough.
>
> Let's do talk about this on Monday.

Sounds good.

It's more important to me that we provide this for Android,
and less important how we decide to expose it.

Maybe something like this?  In utils?
SkSomething::DrawRegion(SkCanvas* canvas, const SkRegion& region, const SkPaint&
paint);

Then we need some way to call into the gpu code from
there...

https://codereview.chromium.org/2277053002/diff/70001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2277053002/diff/70001/include/core/SkCanvas.h...
include/core/SkCanvas.h:709: void drawRegion(const SkRegion& region, const
SkPaint& paint) {
On 2016/08/26 12:27:02, reed1 wrote:
> tiny nit: most of these non-virtuals we also put in the cpp. We should think
> about detecting empty and just-a-rect at this level, before we dispatch to the
> virtuals.

SGTM, see https://codereview.chromium.org/2286693002/