Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

Issue 2277053002: Add drawRegion() API to SkCanvas (Closed)

Created:
4 years, 3 months ago by msarett
Modified:
4 years, 3 months ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Convert region to a path in complex case #

Total comments: 5

Patch Set 3 : Improve gm, fallback to paths in appropriate cases #

Total comments: 2

Patch Set 4 : Added shader to test #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+241 lines, -6 lines) Patch
A gm/drawregion.cpp View 1 chunk +54 lines, -0 lines 0 comments Download
A gm/drawregionmodes.cpp View 1 2 3 1 chunk +78 lines, -0 lines 0 comments Download
M include/core/SkCanvas.h View 1 2 3 2 chunks +9 lines, -0 lines 2 comments Download
M include/core/SkDevice.h View 1 chunk +2 lines, -0 lines 0 comments Download
M include/private/SkRecords.h View 2 chunks +4 lines, -0 lines 0 comments Download
M src/core/SkCanvas.cpp View 1 chunk +20 lines, -0 lines 0 comments Download
M src/core/SkDevice.cpp View 1 2 1 chunk +23 lines, -0 lines 0 comments Download
M src/core/SkLiteDL.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/core/SkLiteDL.cpp View 3 chunks +12 lines, -2 lines 0 comments Download
M src/core/SkLiteRecorder.h View 1 chunk +4 lines, -3 lines 0 comments Download
M src/core/SkLiteRecorder.cpp View 1 chunk +3 lines, -0 lines 0 comments Download
M src/core/SkPictureFlat.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/core/SkPicturePlayback.cpp View 1 chunk +8 lines, -0 lines 0 comments Download
M src/core/SkPictureRecord.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/core/SkPictureRecord.cpp View 1 chunk +10 lines, -0 lines 0 comments Download
M src/core/SkRecordDraw.cpp View 2 chunks +5 lines, -0 lines 0 comments Download
M src/core/SkRecorder.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/core/SkRecorder.cpp View 1 chunk +4 lines, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 36 (19 generated)
msarett
4 years, 3 months ago (2016-08-24 21:44:18 UTC) #5
bsalomon
Q: What should happen if the paint is AA and the matrix is say a ...
4 years, 3 months ago (2016-08-24 23:20:24 UTC) #12
bsalomon
On 2016/08/24 23:20:24, bsalomon wrote: > Q: What should happen if the paint is AA ...
4 years, 3 months ago (2016-08-25 00:35:52 UTC) #13
msarett
We now convert the region to a path in the complex case. I also tried ...
4 years, 3 months ago (2016-08-25 13:02:32 UTC) #14
bsalomon
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#newcode78 src/core/SkDevice.cpp:78: if (complexMatrix && complexPaint) { On 2016/08/25 13:02:32, msarett ...
4 years, 3 months ago (2016-08-25 13:58:20 UTC) #15
msarett
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#newcode31 gm/drawregionrotate.cpp:31: void onDraw(SkCanvas* canvas) override { On 2016/08/25 13:02:32, msarett ...
4 years, 3 months ago (2016-08-25 15:07:35 UTC) #17
bsalomon
Should the GM be consolidated into a single GM? Also, perhaps we should have a ...
4 years, 3 months ago (2016-08-25 17:54:34 UTC) #20
msarett
"Should the GM be consolidated into a single GM? Also, perhaps we should have a ...
4 years, 3 months ago (2016-08-25 18:25:55 UTC) #21
bsalomon
lgtm On 2016/08/25 18:25:55, msarett wrote: > "Should the GM be consolidated into a single ...
4 years, 3 months ago (2016-08-25 18:34:29 UTC) #22
reed1
Is there another way to accelerate this except for adding it to all canvases? SkRegion ...
4 years, 3 months ago (2016-08-25 19:12:53 UTC) #25
bsalomon
On 2016/08/25 19:12:53, reed1 wrote: > Is there another way to accelerate this except for ...
4 years, 3 months ago (2016-08-25 19:19:13 UTC) #26
msarett
On 2016/08/25 19:12:53, reed1 wrote: > Is there another way to accelerate this except for ...
4 years, 3 months ago (2016-08-25 20:00:42 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2277053002/70001
4 years, 3 months ago (2016-08-25 20:53:32 UTC) #31
commit-bot: I haz the power
Committed patchset #4 (id:70001) as https://skia.googlesource.com/skia/+/44df651ebefc284acc2f66425dff3ea0b0e14b36
4 years, 3 months ago (2016-08-25 20:54:39 UTC) #33
reed1
On 2016/08/25 20:00:42, msarett wrote: > On 2016/08/25 19:12:53, reed1 wrote: > > Is there ...
4 years, 3 months ago (2016-08-26 12:08:18 UTC) #34
reed1
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#newcode709 include/core/SkCanvas.h:709: void drawRegion(const SkRegion& region, const SkPaint& paint) { tiny ...
4 years, 3 months ago (2016-08-26 12:27:02 UTC) #35
msarett
4 years, 3 months ago (2016-08-26 13:20:14 UTC) #36
Message was sent while issue was closed.
> 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/

Powered by Google App Engine
This is Rietveld 408576698