Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 729463002: Cache blur mask for rects which can not break into nine-patch (Closed)

Created:
3 years, 11 months ago by qiankun
Modified:
3 years, 8 months ago
Reviewers:
humper, reed2, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Cache blur mask for rects which can not break into nine-patch With this CL performance improves: blurrectsnonninepatch 42.4us -> 20.5us 0.48x BUG=431021, skia:3118 Committed: https://skia.googlesource.com/skia/+/5a5b4e900ee69540fc35952882a323c63d6d0db9

Patch Set 1 #

Patch Set 2 : move common function to SkMaskCache.cpp #

Patch Set 3 : check rectCount and asABlur before add into cache #

Patch Set 4 : rebase #

Patch Set 5 : add GM and bench #

Patch Set 6 : fix edge artifacts #

Total comments: 4

Patch Set 7 : name Find/Add API clearly and restruct if condition #

Patch Set 8 : update to ToT #

Patch Set 9 : formatting #

Total comments: 4

Patch Set 10 : fix comments in #19 #

Patch Set 11 : fix memory leak #

Total comments: 4

Patch Set 12 : fix clipping bounds #

Patch Set 13 : handle clipped path #

Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -58 lines) Patch
M src/core/SkMaskCache.h View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
M src/core/SkMaskCache.cpp View 1 2 3 4 5 6 3 chunks +50 lines, -0 lines 0 comments Download
M src/core/SkMaskFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +70 lines, -2 lines 0 comments Download
M src/effects/SkBlurMaskFilter.cpp View 1 2 3 4 5 6 7 8 9 6 chunks +6 lines, -56 lines 0 comments Download

Messages

Total messages: 42 (5 generated)
qiankun
reed, I made a prototype to cache result mask for rects which can not break ...
3 years, 11 months ago (2014-11-14 09:42:10 UTC) #2
reed1
3 years, 11 months ago (2014-11-14 14:29:15 UTC) #4
qiankun
PTAL.
3 years, 11 months ago (2014-11-19 16:14:58 UTC) #5
qiankun
Ping. Any comments?
3 years, 10 months ago (2014-11-25 15:30:58 UTC) #6
reed1
Sorry for the long absence. Things were very busy here the last few weeks. Can ...
3 years, 10 months ago (2014-12-05 19:22:17 UTC) #7
qiankun
On 2014/12/05 19:22:17, reed1 wrote: > Sorry for the long absence. Things were very busy ...
3 years, 10 months ago (2014-12-09 08:02:20 UTC) #8
reed1
if possible, landing bench/BlurRectsBench.cpp as its own CL now would be fine (and help confirm ...
3 years, 10 months ago (2014-12-09 16:42:33 UTC) #9
reed1
actually, I see that we also want the new GM to land ahead of time.
3 years, 10 months ago (2014-12-09 18:28:50 UTC) #10
reed1
On 2014/12/09 18:28:50, reed1 wrote: > actually, I see that we also want the new ...
3 years, 10 months ago (2014-12-09 18:46:34 UTC) #11
reed1
For this CL, it shows edge artifacts when I slide the GM around inside SampleApp ...
3 years, 10 months ago (2014-12-09 18:47:08 UTC) #12
qiankun
On 2014/12/09 18:47:08, reed1 wrote: > For this CL, it shows edge artifacts when I ...
3 years, 10 months ago (2014-12-10 10:44:20 UTC) #13
qiankun
hi reed, please take a look at this when you free. Thanks.
3 years, 10 months ago (2014-12-19 02:23:07 UTC) #14
reed1
https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h File src/core/SkMaskCache.h (right): https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h#newcode42 src/core/SkMaskCache.h:42: Why do these take their parameters in a slightly ...
3 years, 10 months ago (2014-12-22 21:38:16 UTC) #15
qiankun
Uploaded a new patch to fix the comments. PTAL. https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h File src/core/SkMaskCache.h (right): https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h#newcode42 src/core/SkMaskCache.h:42: ...
3 years, 10 months ago (2014-12-23 10:54:33 UTC) #16
qiankun
Ping.
3 years, 9 months ago (2015-01-07 14:22:40 UTC) #18
reed1
much clearer, thanks. lgtm w/ nits https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (right): https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp#newcode269 src/core/SkMaskFilter.cpp:269: if (!SkMaskCache::FindAndCopy(matrix.mapRadius(rec.fSigma), rec.fStyle, ...
3 years, 9 months ago (2015-01-07 16:45:47 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/729463002/200001
3 years, 9 months ago (2015-01-08 02:28:25 UTC) #21
commit-bot: I haz the power
Committed patchset #10 (id:200001) as https://skia.googlesource.com/skia/+/5a5b4e900ee69540fc35952882a323c63d6d0db9
3 years, 9 months ago (2015-01-08 02:37:50 UTC) #22
qiankun
Thank you reed! https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (right): https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp#newcode269 src/core/SkMaskFilter.cpp:269: if (!SkMaskCache::FindAndCopy(matrix.mapRadius(rec.fSigma), rec.fStyle, On 2015/01/07 16:45:46, ...
3 years, 9 months ago (2015-01-08 03:01:12 UTC) #23
reed2
Looks like this is leaking memory... ==8017==ERROR: LeakSanitizer: detected memory leaks Direct leak of 25992 ...
3 years, 9 months ago (2015-01-08 03:01:14 UTC) #25
qiankun
On 2015/01/08 03:01:14, reed2 wrote: > Looks like this is leaking memory... > > ==8017==ERROR: ...
3 years, 9 months ago (2015-01-08 03:06:19 UTC) #26
reed2
It is late. I suggest we revert for now, and can review possible fixes tomorrow.
3 years, 9 months ago (2015-01-08 03:11:01 UTC) #27
qiankun
A revert of this CL (patchset #10 id:200001) has been created in https://codereview.chromium.org/844673002/ by qiankun.miao@intel.com. ...
3 years, 9 months ago (2015-01-08 03:13:49 UTC) #28
qiankun
Fixed the memory leak. Please help to review.
3 years, 9 months ago (2015-01-08 04:25:41 UTC) #29
reed1
https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (right): https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp#newcode270 src/core/SkMaskFilter.cpp:270: if (!SkMaskCache::FindAndCopy(scaledSigma, rec.fStyle, rec.fQuality, I think it will be ...
3 years, 9 months ago (2015-01-08 15:51:21 UTC) #30
reed1
If this bounds check is related to clipping, perhaps we need to some how incorporate ...
3 years, 9 months ago (2015-01-08 16:11:34 UTC) #31
qiankun
Thanks for review. https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (right): https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp#newcode270 src/core/SkMaskFilter.cpp:270: if (!SkMaskCache::FindAndCopy(scaledSigma, rec.fStyle, rec.fQuality, On 2015/01/08 ...
3 years, 9 months ago (2015-01-12 04:36:26 UTC) #32
reed1
Does this mean if the blur is clipped (i.e. large or we are tiled) we ...
3 years, 9 months ago (2015-01-12 17:44:55 UTC) #33
qiankun
On 2015/01/12 17:44:55, reed1 wrote: > Does this mean if the blur is clipped (i.e. ...
3 years, 9 months ago (2015-01-13 08:04:33 UTC) #34
qiankun
Ping reed@
3 years, 9 months ago (2015-01-19 10:55:11 UTC) #35
qiankun
Hi reed, please help to review this when you are free, thanks!
3 years, 9 months ago (2015-01-23 15:53:54 UTC) #36
reed1
hmm, I need to think about what our "policy" should be for when to cache ...
3 years, 9 months ago (2015-01-23 21:18:39 UTC) #37
qiankun
On 2015/01/23 21:18:39, reed1 wrote: > hmm, I need to think about what our "policy" ...
3 years, 8 months ago (2015-01-26 08:12:28 UTC) #38
qiankun
Hi reed, do you make decision about the cache policy? BTW, I noted mask which ...
3 years, 8 months ago (2015-02-10 14:10:42 UTC) #39
reed1
On 2015/02/10 14:10:42, qiankun wrote: > Hi reed, do you make decision about the cache ...
3 years, 8 months ago (2015-02-10 14:47:46 UTC) #40
qiankun
On 2015/02/10 14:47:46, reed1 wrote: > On 2015/02/10 14:10:42, qiankun wrote: > > Hi reed, ...
3 years, 8 months ago (2015-02-10 14:54:56 UTC) #41
reed1
3 years, 8 months ago (2015-02-10 14:58:58 UTC) #42
On 2015/02/10 14:54:56, qiankun wrote:
> On 2015/02/10 14:47:46, reed1 wrote:
> > On 2015/02/10 14:10:42, qiankun wrote:
> > > Hi reed, do you make decision about the cache policy? BTW, I noted mask
> which
> > > can break down into nine-patch have same cache behavior, i.e. cache whole
> mask
> > > for each tile.
> > 
> > At the moment I want to table this change, and only cache "reusable" blurs :
> > e.g. ninepatches.
> 
> I see. Thanks for reply. I will keep an eye on the cache size in future. Hope
we
> can find a better method to balance memory and performance.

Yea, this is a hard problem. If we can analyze the Verge's geometry and find
more clever ways to create reusable masks, that would be great.

Powered by Google App Engine
This is Rietveld 408576698