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

Issue 699623003: Crop the fast path dashed lines to the cull rect (Closed)

Created:
6 years, 1 month ago by robertphillips
Modified:
6 years, 1 month ago
Reviewers:
danakj, egdaniel, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Project:
skia
Visibility:
Public.

Description

Crop the fast path dashed lines to the cull rect Without: maxrss loops min median mean max stddev samples config bench 56M 1 13.3ms 13.6ms 13.6ms 14.2ms 2% Ooooo..... 8888 GM_dashing5_bw 56M 13 390us 417us 416us 459us 5% ooooO..o.o gpu GM_dashing5_bw 56M 1 13.4ms 13.9ms 14.1ms 15ms 3% Oooo..ooOo 8888 GM_dashing5_aa 56M 13 402us 421us 416us 425us 2% Ooo.ooOOOO gpu GM_dashing5_aa With: 40M 1 1.53ms 1.54ms 1.54ms 1.55ms 0% oo.O...o.. 8888 GM_dashing5_bw 40M 12 407us 412us 415us 445us 3% ...Oo..... gpu GM_dashing5_bw 40M 1 1.7ms 1.7ms 1.7ms 1.72ms 0% o.O....... 8888 GM_dashing5_aa 43M 13 405us 409us 409us 415us 1% ooo.Ooo..o gpu GM_dashing5_aa The GM images (including the new one) are the same with and without this CL. BUG=428296 Committed: https://skia.googlesource.com/skia/+/9f2251c73ed6f417dd1057d487bf523e04488440

Patch Set 1 #

Total comments: 2

Patch Set 2 : Address code review comments #

Total comments: 6

Patch Set 3 : Next step #

Patch Set 4 : Address coordinate xform issue #

Unified diffs Side-by-side diffs Delta from patch set Stats (+215 lines, -18 lines) Patch
M gm/dashing.cpp View 1 2 3 3 chunks +85 lines, -10 lines 0 comments Download
M include/core/SkMatrix.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M src/core/SkDraw.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/effects/SkDashPathEffect.cpp View 1 2 3 3 chunks +120 lines, -3 lines 0 comments Download
M src/utils/SkDashPath.cpp View 1 1 chunk +5 lines, -4 lines 0 comments Download

Messages

Total messages: 21 (2 generated)
robertphillips
https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect.cpp File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect.cpp#newcode43 src/effects/SkDashPathEffect.cpp:43: This code was adapted from src\utils\SkDashPath.cpp:cull_path.
6 years, 1 month ago (2014-11-03 21:20:07 UTC) #2
reed1
Can we add this after the initial isLine() check? If so, then we could just ...
6 years, 1 month ago (2014-11-03 21:50:36 UTC) #3
egdaniel
https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect.cpp File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/699623003/diff/1/src/effects/SkDashPathEffect.cpp#newcode98 src/effects/SkDashPathEffect.cpp:98: minX = bounds.fLeft - SkScalarMod(bounds.fLeft - minX, intervalLength); This ...
6 years, 1 month ago (2014-11-03 22:03:31 UTC) #4
robertphillips
PTAL This version: enhances the GM/bench with phases, flipping the order of the line endpoints ...
6 years, 1 month ago (2014-11-04 18:10:23 UTC) #5
danakj
On 2014/11/04 18:10:23, robertphillips wrote: > PTAL > > This version: > > enhances the ...
6 years, 1 month ago (2014-11-04 18:19:19 UTC) #6
danakj
On 2014/11/04 18:19:19, danakj wrote: > On 2014/11/04 18:10:23, robertphillips wrote: > > PTAL > ...
6 years, 1 month ago (2014-11-04 18:24:11 UTC) #7
reed1
https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEffect.cpp File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEffect.cpp#newcode77 src/effects/SkDashPathEffect.cpp:77: if (dx && dy) { can/should we move this ...
6 years, 1 month ago (2014-11-04 18:24:58 UTC) #8
danakj
https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEffect.cpp File src/effects/SkDashPathEffect.cpp (right): https://codereview.chromium.org/699623003/diff/20001/src/effects/SkDashPathEffect.cpp#newcode91 src/effects/SkDashPathEffect.cpp:91: if (maxX < bounds.fLeft || minX > bounds.fRight) { ...
6 years, 1 month ago (2014-11-04 18:30:05 UTC) #9
robertphillips
Dana - could you try this version? Note: it still doesn't deal correctly with the ...
6 years, 1 month ago (2014-11-04 18:51:16 UTC) #10
danakj
On 2014/11/04 18:51:16, robertphillips wrote: > Dana - could you try this version? Yep it ...
6 years, 1 month ago (2014-11-04 18:59:46 UTC) #11
reed1
have we been able to catch these tricky cases in a unittest or some other ...
6 years, 1 month ago (2014-11-04 19:01:24 UTC) #12
danakj
On 2014/11/04 18:59:46, danakj wrote: > On 2014/11/04 18:51:16, robertphillips wrote: > > Dana - ...
6 years, 1 month ago (2014-11-04 19:14:54 UTC) #13
robertphillips
On 2014/11/04 19:14:54, danakj wrote: > On 2014/11/04 18:59:46, danakj wrote: > > On 2014/11/04 ...
6 years, 1 month ago (2014-11-04 19:25:08 UTC) #14
robertphillips
Okay - new version with coordinate transform support.
6 years, 1 month ago (2014-11-04 20:12:14 UTC) #15
danakj
On 2014/11/04 20:12:14, robertphillips wrote: > Okay - new version with coordinate transform support. This ...
6 years, 1 month ago (2014-11-04 20:25:45 UTC) #16
robertphillips
On 2014/11/04 19:01:24, reed1 wrote: > have we been able to catch these tricky cases ...
6 years, 1 month ago (2014-11-04 20:55:16 UTC) #17
reed1
ok. I was thinking specifically of whatever the conditions were that had been caught just ...
6 years, 1 month ago (2014-11-04 21:11:37 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/699623003/60001
6 years, 1 month ago (2014-11-04 21:21:24 UTC) #20
commit-bot: I haz the power
6 years, 1 month ago (2014-11-04 21:33:54 UTC) #21
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 9f2251c73ed6f417dd1057d487bf523e04488440

Powered by Google App Engine
This is Rietveld 408576698