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

Issue 1491293002: detect when we can filter bitmaps/images directly, w/o a tmp layer (Closed)

Created:
5 years ago by reed1
Modified:
5 years ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia:1073 Committed: https://skia.googlesource.com/skia/+/262a71b7f95ce98ff3dd8dba845afbd724470903

Patch Set 1 #

Patch Set 2 : #

Total comments: 10

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : remove dead (testing) code #

Total comments: 3

Patch Set 6 : update clip-predicate w/ bug number, and add bug to this CL #

Total comments: 6

Patch Set 7 : change to clearer names (drawBitmapAsSprite) #

Patch Set 8 : use SkTreadAsSprite #

Total comments: 2

Patch Set 9 : move include to the top #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -35 lines) Patch
M include/core/SkCanvas.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M include/core/SkDevice.h View 1 2 3 4 5 6 1 chunk +5 lines, -1 line 0 comments Download
M src/core/SkCanvas.cpp View 1 2 3 4 5 6 7 8 6 chunks +69 lines, -29 lines 0 comments Download
M src/core/SkDevice.cpp View 1 2 3 4 5 6 1 chunk +23 lines, -0 lines 0 comments Download
M src/image/SkImage_Base.h View 1 chunk +6 lines, -0 lines 0 comments Download
M src/image/SkImage_Gpu.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/image/SkImage_Gpu.cpp View 3 chunks +11 lines, -5 lines 0 comments Download

Messages

Total messages: 62 (26 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/1
5 years ago (2015-12-02 18:15:10 UTC) #2
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-12-02 18:28:25 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/20001
5 years ago (2015-12-02 18:36:58 UTC) #6
reed1
Show some small diffs in GMs (e.g. Erode and MatrixConvolution). Investigating them now.
5 years ago (2015-12-02 18:38:25 UTC) #8
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-12-02 19:01:12 UTC) #10
reed1
gm to exercise some of the small GM diffs I noticed w/ this CL. https://codereview.chromium.org/1498523002/
5 years ago (2015-12-02 20:27:23 UTC) #11
reed1
precursor for chrome: https://codereview.chromium.org/1489163004/
5 years ago (2015-12-02 21:28:00 UTC) #12
robertphillips
https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#newcode599 src/core/SkCanvas.cpp:599: while (looper.next(SkDrawFilter::kBitmap_Type)) { \ indent this ? https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#newcode1419 src/core/SkCanvas.cpp:1419: ...
5 years ago (2015-12-02 22:37:18 UTC) #13
Stephen White
Is this supposed to help you move forward with your Chrome patch? I'm not sure ...
5 years ago (2015-12-02 23:45:15 UTC) #15
reed1
Good catch. I was just blindly replicating the pattern from drawSprite, and forgot that the ...
5 years ago (2015-12-03 14:37:28 UTC) #16
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/40001
5 years ago (2015-12-03 15:10:05 UTC) #18
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-12-03 17:15:23 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/40001
5 years ago (2015-12-03 20:16:36 UTC) #23
commit-bot: I haz the power
Note for Reviewers: The CQ is waiting for an approval. If you believe that the ...
5 years ago (2015-12-03 20:16:36 UTC) #24
reed1
ptal https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h File include/core/SkDevice.h (right): https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h#newcode386 include/core/SkDevice.h:386: * - the device itself does not want ...
5 years ago (2015-12-03 21:26:51 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/80001
5 years ago (2015-12-03 21:28:11 UTC) #29
robertphillips
lgtm
5 years ago (2015-12-04 15:07:54 UTC) #31
robertphillips
https://codereview.chromium.org/1491293002/diff/80001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/80001/src/core/SkCanvas.cpp#newcode2200 src/core/SkCanvas.cpp:2200: } Add a bug # ?
5 years ago (2015-12-04 15:15:48 UTC) #32
robertphillips
I would like Stephen to weigh in too though
5 years ago (2015-12-04 15:22:06 UTC) #34
reed1
ptal
5 years ago (2015-12-04 15:22:30 UTC) #35
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/100001
5 years ago (2015-12-04 15:22:41 UTC) #37
reed1
Using the bench to verify was a great idea. Are there other concerns/questions?
5 years ago (2015-12-04 18:05:07 UTC) #39
Stephen White
What are the Chrome issues that prevent landing this without an #ifdef? https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h ...
5 years ago (2015-12-04 19:22:52 UTC) #40
Stephen White
https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h#newcode1437 include/core/SkCanvas.h:1437: bool canCallFilterSprite(const SkRect& bounds, const SkPaint&); On 2015/12/04 19:22:51, ...
5 years ago (2015-12-04 19:23:35 UTC) #41
reed1
Not sure if we'll see any chrome layout diffs, but we see a few in ...
5 years ago (2015-12-04 19:33:29 UTC) #42
reed1
https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#newcode2198 src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { On 2015/12/04 19:33:29, reed1 ...
5 years ago (2015-12-04 19:44:03 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/120001
5 years ago (2015-12-04 19:52:05 UTC) #45
Stephen White
https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#newcode2198 src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { On 2015/12/04 19:44:03, reed1 ...
5 years ago (2015-12-04 20:18:16 UTC) #47
reed1
On 2015/12/04 20:18:16, Stephen White wrote: > https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp > File src/core/SkCanvas.cpp (right): > > https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#newcode2198 ...
5 years ago (2015-12-04 20:28:30 UTC) #48
reed1
now w/ SkTreatAsSprite
5 years ago (2015-12-04 21:29:03 UTC) #49
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/140001
5 years ago (2015-12-04 21:29:16 UTC) #51
Stephen White
LGTM https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp#newcode2188 src/core/SkCanvas.cpp:2188: #include "SkMatrixUtils.h" Nit: move this #include to the ...
5 years ago (2015-12-04 21:41:08 UTC) #52
reed1
https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp#newcode2188 src/core/SkCanvas.cpp:2188: #include "SkMatrixUtils.h" On 2015/12/04 21:41:08, Stephen White wrote: > ...
5 years ago (2015-12-04 21:52:11 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/160001
5 years ago (2015-12-04 21:52:24 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/160001
5 years ago (2015-12-05 21:06:50 UTC) #60
commit-bot: I haz the power
5 years ago (2015-12-05 21:07:31 UTC) #62
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/262a71b7f95ce98ff3dd8dba845afbd724470903

Powered by Google App Engine
This is Rietveld 408576698