Optimize CSS box-shadow performance

Optimize CSS box-shadow performance

Cache the SkMask of blur effect with SkScaledImageCache to improve CSS box-shadow
performance. This CL is based on Jun Jiang's CL(https://codereview.chromium.org/286273002).

BUG=173807, 425427

[qiankun, 2014-08-13 09:56:12]

Hi reviewers, I did a update to fix comments in
https://codereview.chromium.org/286273002. Could you take a look?

[reed1, 2014-08-15 15:38:42]

I like the direction of trying to extend the existing image cache. Adding florin
who is also consider this for caching picture (shaders).

I wonder if we can extend the Key w/o needing virtuals. Have we timed this
change, to see if that is affecting anything?

Perhaps Key just needs to identify its 32-bit aligned content and a size, so we
can inline the notion of performing a compare...

[tomhudson, 2014-08-15 16:04:15]

On 2014/08/15 15:38:42, reed1 wrote:
> I wonder if we can extend the Key w/o needing virtuals.

Templates?

[reed1, 2014-08-15 16:06:23]

On 2014/08/15 16:04:15, tomhudson wrote:
> On 2014/08/15 15:38:42, reed1 wrote:
> > I wonder if we can extend the Key w/o needing virtuals.
> 
> Templates?

Could, but then we end up having to dup the search code itself code at each
call-site (assuming we're trying to support extensible private keys). I'm
suggesting just ptr/len since that seems to be the == and < operators before and
after this CL.

[reed1, 2014-08-15 16:07:02]

Perhaps I can try posting a faux CL that suggests what I have in mind.

[reed1, 2014-08-15 17:16:57]

This is a draft of my idea. If we land something like it, then this current CL
could be refactored to take advantage of this new api on the cache.

[reed1, 2014-08-15 17:19:37]

On 2014/08/15 17:16:57, reed1 wrote:
> This is a draft of my idea. If we land something like it, then this current CL
> could be refactored to take advantage of this new api on the cache.

https://codereview.chromium.org/472343002/

[qiankun, 2014-08-18 10:16:54]

On 2014/08/15 15:38:42, reed1 wrote:
> I like the direction of trying to extend the existing image cache. Adding
florin
> who is also consider this for caching picture (shaders).
> 
> I wonder if we can extend the Key w/o needing virtuals. Have we timed this
> change, to see if that is affecting anything?
> 
For the attached bug=173807, boxBlurInterp(...) isn't the hotspot any more. 
For Skia nanobench, performance of blurroundrect_WH[100x100]_cr[30]'s 8888
config improved from 44 ms to 27 ms on my desktop.

> Perhaps Key just needs to identify its 32-bit aligned content and a size, so
we
> can inline the notion of performing a compare...

[qiankun, 2014-08-21 09:03:37]

I rebased this CL to the general Key. PTAL.

[reed1, 2014-08-21 12:26:13]

I'm glad the generalized key idea seems to be working.

I think we can take advantage of it even more, and not need to modify the
imagecache itself to know about each new client (e.g. sigmas).

Also, we should try to do this without exposing anything in the public header
(e.g. SkMask.h) if possible.

I will show a CL that should allow you to use the cache w/o having to modify it.

[reed1, 2014-08-21 15:13:56]

refactor of the base cache https://codereview.chromium.org/483493003/

[qiankun, 2014-08-22 06:28:47]

Updated Cl based on SkBitmapCache, PTAL.

[reed1, 2014-08-22 15:43:59]

I think the maskfilter code should create its own private header/wrapper for the
cache. Sort of a maskcache instead of bitmapcache.

[reed1, 2014-08-22 15:45:03]

lets try to land the cleanups in scaledbitmapcache in its own CL.

https://codereview.chromium.org/471473002/diff/40001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.h (left):

https://codereview.chromium.org/471473002/diff/40001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.h:132: *  Use (generationID, width, and height) or
(original, scaleX,
This is a good delete, but we might try to land it in its own CL

[qiankun, 2014-08-25 06:22:03]

Add SkMaskCache.

https://codereview.chromium.org/471473002/diff/40001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.h (left):

https://codereview.chromium.org/471473002/diff/40001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.h:132: *  Use (generationID, width, and height) or
(original, scaleX,
On 2014/08/22 15:45:03, reed1 wrote:
> This is a good delete, but we might try to land it in its own CL

Done.

[reed1, 2014-08-25 11:58:43]

https://codereview.chromium.org/471473002/diff/60001/src/core/SkMaskCache.cpp
File src/core/SkMaskCache.cpp (right):

https://codereview.chromium.org/471473002/diff/60001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:61: bitmap.installMaskPixels(mask);
installMaskPixels does not copy the memory for the pixels, but the caller of
this will free those pixels when the mask goes away.

https://codereview.chromium.org/471473002/diff/60001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (left):

https://codereview.chromium.org/471473002/diff/60001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:12: #include "SkRect.h"
nit: we can land this change separately.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:127: bool
SkBlurMaskFilterImpl::getCachedBlurMask(SkScalar sigma,
Do we really need these helper methods? The caller has to track the cacheID and
remember to unlock it anway, so I think it may be clearer to just have them call
FindAndLock.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:129: SkCachedMask &mask) {
skia's convention is to only use reference-parameters when they are const (with
the one exception of ReadBuffer). Please pass mask as a ptr.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:131: mask.fCacheId = cacheId;
Skia's convention is to not set the result fields if we're going to return
false. e.g. mask.fMask should also not be modified if this fails.

[reed1, 2014-08-25 14:44:22]

BTW -- I have an experimental change locally that should resolve the "who owns
the memory for the mask pixels" question. Perhaps this CL can pause until I post
this other change.

[qiankun, 2014-08-26 08:36:33]

Fixed some comments and looking forward to reed's solution for "memory owning"
problem.

https://codereview.chromium.org/471473002/diff/60001/src/core/SkMaskCache.cpp
File src/core/SkMaskCache.cpp (right):

https://codereview.chromium.org/471473002/diff/60001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:61: bitmap.installMaskPixels(mask);
On 2014/08/25 11:58:42, reed1 wrote:
> installMaskPixels does not copy the memory for the pixels, but the caller of
> this will free those pixels when the mask goes away.

The pixels owned by mask won't be freed in this blurMask case when mask goes
away. Or I can do a deepCopyTo for bitmap. I am looking forward to your
solution.

https://codereview.chromium.org/471473002/diff/60001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (left):

https://codereview.chromium.org/471473002/diff/60001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:12: #include "SkRect.h"
On 2014/08/25 11:58:42, reed1 wrote:
> nit: we can land this change separately.

Done.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:127: bool
SkBlurMaskFilterImpl::getCachedBlurMask(SkScalar sigma,
On 2014/08/25 11:58:42, reed1 wrote:
> Do we really need these helper methods? The caller has to track the cacheID
and
> remember to unlock it anway, so I think it may be clearer to just have them
call
> FindAndLock.

Done.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:129: SkCachedMask &mask) {
On 2014/08/25 11:58:42, reed1 wrote:
> skia's convention is to only use reference-parameters when they are const
(with
> the one exception of ReadBuffer). Please pass mask as a ptr.

Done.

https://codereview.chromium.org/471473002/diff/60001/src/effects/SkBlurMaskFi...
src/effects/SkBlurMaskFilter.cpp:131: mask.fCacheId = cacheId;
On 2014/08/25 11:58:42, reed1 wrote:
> Skia's convention is to not set the result fields if we're going to return
> false. e.g. mask.fMask should also not be modified if this fails.

Done.

[qiankun, 2014-08-28 07:28:17]

New CL based on generic cache was uploaded. PTAL.

[reed1, 2014-08-28 20:13:35]

That looks very clean, thanks.

I think now there are two issues remaining.

1. Who owns the mask pixels after Find and after Add. I think I can figure it
out from looking at the impl, but that will need to be very clearly documented
in SkMaskCache.h, whatever the answer. This is unfortunately harder than for
bitmaps, which internalize the reference-counter of their pixels, so its not
really an issue.

2. How to take advantage of ashmem when its available. Bitmap do this today be
calling the custom allocator on SkResourceCache::GetAllocator, but that only
works for bitmaps today.

Perhaps (and this is just a thought) we can address both of these issues by
using SkBitmap for the actual "value" type in this cache. You still have a
custom key based on sigma etc., but what comes in and out of the cache is a
bitmap. You can always trivially have a local mask point to the pixels in a
bitmap (since the returned bitmap will be locked, you can call getAddr8(0, 0)).
What do you think?

[qiankun, 2014-08-29 09:28:54]

On 2014/08/28 20:13:35, reed1 wrote:
> That looks very clean, thanks.
> 
> I think now there are two issues remaining.
> 
> 1. Who owns the mask pixels after Find and after Add. I think I can figure it
> out from looking at the impl, but that will need to be very clearly documented
> in SkMaskCache.h, whatever the answer. This is unfortunately harder than for
> bitmaps, which internalize the reference-counter of their pixels, so its not
> really an issue.
> 
> 2. How to take advantage of ashmem when its available. Bitmap do this today be
> calling the custom allocator on SkResourceCache::GetAllocator, but that only
> works for bitmaps today.
> 
> Perhaps (and this is just a thought) we can address both of these issues by
> using SkBitmap for the actual "value" type in this cache. You still have a
> custom key based on sigma etc., but what comes in and out of the cache is a
> bitmap. You can always trivially have a local mask point to the pixels in a
> bitmap (since the returned bitmap will be locked, you can call getAddr8(0,
0)).
> What do you think?

It's not very easy to fix these two issues. I followed your thought to implement
a WIP prototype. Is it the correct direction? Could you take a look?

[qiankun, 2014-09-04 08:45:16]

Uploaded a new patch. Use bitmap as the real value in blur mask. PTAL.

[reed1, 2014-09-04 14:15:39]

Looks like we could really use a shared find_and_return function. Perhaps all
that is needed is that all of our variants that store fBitmap as their "value"
should have a common baseclass that has that (but no key).

I still need to review how/where we allocate the memory for the mask pixels...

https://codereview.chromium.org/471473002/diff/160001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/471473002/diff/160001/src/core/SkBitmapCache....
src/core/SkBitmapCache.cpp:86: virtual size_t bytesUsed() const SK_OVERRIDE {
return sizeof(fKey) + sizeof(fBitmap); }
sizeof(fBitmap) does not account for the ram of the pixels. should be
fBitmap.getSize()

[qiankun, 2014-09-09 03:03:03]

Upload a new patch to share find_and_return.

https://codereview.chromium.org/471473002/diff/160001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/471473002/diff/160001/src/core/SkBitmapCache....
src/core/SkBitmapCache.cpp:86: virtual size_t bytesUsed() const SK_OVERRIDE {
return sizeof(fKey) + sizeof(fBitmap); }
On 2014/09/04 14:15:39, reed1 wrote:
> sizeof(fBitmap) does not account for the ram of the pixels. should be
> fBitmap.getSize()

Done.

[reed1, 2014-09-09 20:06:51]

Definitely don't want to add a bitmap to mask -- that sort of defeats the
purpose of mask today, which is a lighter-weight version of mask.

[qiankun, 2014-09-10 01:56:40]

On 2014/09/09 20:06:51, reed1 wrote:
> Definitely don't want to add a bitmap to mask -- that sort of defeats the
> purpose of mask today, which is a lighter-weight version of mask.

So what about patch set #7? It's looks very clean. And It makes sense to SkRRect
and SkRect blur, takes little impact on other part. Maybe we can land it first,
then refactor SkMask in future. What do you think?

[reed1, 2014-09-11 15:53:05]

On 2014/09/10 01:56:40, qiankun wrote:
> On 2014/09/09 20:06:51, reed1 wrote:
> > Definitely don't want to add a bitmap to mask -- that sort of defeats the
> > purpose of mask today, which is a lighter-weight version of mask.
> 
> So what about patch set #7? It's looks very clean. And It makes sense to
SkRRect
> and SkRect blur, takes little impact on other part. Maybe we can land it
first,
> then refactor SkMask in future. What do you think?

I don't see what the ownership rules for the pixels are in #7.

One solution might be to have the cache take/return an SkData* that contains the
mask pixels. At least that is sharable, so the client doesn't have to make a
deep-copy of the pixels when they are found in the cache...

[qiankun, 2014-09-15 09:17:04]

On 2014/09/11 15:53:05, reed1 wrote:
> On 2014/09/10 01:56:40, qiankun wrote:
> > On 2014/09/09 20:06:51, reed1 wrote:
> > > Definitely don't want to add a bitmap to mask -- that sort of defeats the
> > > purpose of mask today, which is a lighter-weight version of mask.
> > 
> > So what about patch set #7? It's looks very clean. And It makes sense to
> SkRRect
> > and SkRect blur, takes little impact on other part. Maybe we can land it
> first,
> > then refactor SkMask in future. What do you think?
> 
> I don't see what the ownership rules for the pixels are in #7.
> 
> One solution might be to have the cache take/return an SkData* that contains
the
> mask pixels. At least that is sharable, so the client doesn't have to make a
> deep-copy of the pixels when they are found in the cache...

Uploaded a new patch using SkData for memory management. PTAL.

[qiankun, 2014-09-18 17:50:41]

Ping.

[reed1, 2014-09-19 20:46:21]

I like the simplicity, but mask doesn't have any lifetime management (on
purpose) so adding a data field seems fragile.

I do think storing/retrieving SkData from the cache is perfect -- lightweight
and already thread-safe-reffable.

Now I'm just thinking about the MaskCache api. Can we make it clearer to its
clients what the ownership rules are?

e.g.

/**
 * On success, return a ref to the SkData that holds the pixels, and have mask
 * already point to that memory.
 *
 * On failure, return NULL.
 */
SkData* MaskCache::FindAndRef(..., &mask);

void MaskCache::Add(..., data);

[reed2, 2014-09-20 12:51:32]

reed@chromium.org changed reviewers:
+ reed@chromium.org

[reed2, 2014-09-20 12:51:33]

Actually, its harder than I thought. We need to allow for the mask pixels to be
allocated by the cache itself, for the case when we are trying to use ashmem
(discardable memory).

Today we only have a wrapper for that for bitmaps. This is a bit of overkill, so
it might not be worth it until we create a different wrapper more suited to
light-weight clients like mask pixels.

I think we tried bitmaps once, but that change also stored the bitmap inside
SkMask. I do not want to add any lifetime burden to that struct.

Since this CL is having to use/modify resourcecache, which is itself very new
and still undergoing semi-frequent internal changes, perhaps you might table
this CL, or assign the bug to me. Not required, but there are lots of Issues in
skia you might work on that could be more straight-forward than this one (at
this time).

[Stephen White, 2014-09-20 15:09:39]

On 2014/09/20 12:51:33, reed2 wrote:
> Actually, its harder than I thought. We need to allow for the mask pixels to
be
> allocated by the cache itself, for the case when we are trying to use ashmem
> (discardable memory).
> 
> Today we only have a wrapper for that for bitmaps. This is a bit of overkill,
so
> it might not be worth it until we create a different wrapper more suited to
> light-weight clients like mask pixels.
> 
> I think we tried bitmaps once, but that change also stored the bitmap inside
> SkMask. I do not want to add any lifetime burden to that struct.
> 
> Since this CL is having to use/modify resourcecache, which is itself very new
> and still undergoing semi-frequent internal changes, perhaps you might table
> this CL, or assign the bug to me. Not required, but there are lots of Issues
in
> skia you might work on that could be more straight-forward than this one (at
> this time).

Drive-by question: is there a reason we can't replace SkMask with SkBitmap in 
the future? Is there functionality that SkMask has that an A8 SkBitmap doesn't
(or
couldn't/shouldn't?)

[qiankun, 2014-09-23 09:13:10]

On 2014/09/20 12:51:33, reed2 wrote:
> Actually, its harder than I thought. We need to allow for the mask pixels to
be
> allocated by the cache itself, for the case when we are trying to use ashmem
> (discardable memory).
> 
> Today we only have a wrapper for that for bitmaps. This is a bit of overkill,
so
> it might not be worth it until we create a different wrapper more suited to
> light-weight clients like mask pixels.
> 
> I think we tried bitmaps once, but that change also stored the bitmap inside
> SkMask. I do not want to add any lifetime burden to that struct.
> 
> Since this CL is having to use/modify resourcecache, which is itself very new
> and still undergoing semi-frequent internal changes, perhaps you might table
> this CL, or assign the bug to me. Not required, but there are lots of Issues
in
> skia you might work on that could be more straight-forward than this one (at
> this time).

A new patch is uploaded to fix some issues:
1) remove SkData field in SkMask.
2) Clear the SkMaskCache API. SkData is used for stroing/retrieving mask cache.

At present, this implementation doesn't support ashmem. I prefer landing this CL
then we improve it continuously with ResourceCache and ashmem in future. I am
also glad to work on other issues in Skia.

[reed1, 2014-09-23 13:46:06]

On 2014/09/23 09:13:10, qiankun wrote:
> On 2014/09/20 12:51:33, reed2 wrote:
> > Actually, its harder than I thought. We need to allow for the mask pixels to
> be
> > allocated by the cache itself, for the case when we are trying to use ashmem
> > (discardable memory).
> > 
> > Today we only have a wrapper for that for bitmaps. This is a bit of
overkill,
> so
> > it might not be worth it until we create a different wrapper more suited to
> > light-weight clients like mask pixels.
> > 
> > I think we tried bitmaps once, but that change also stored the bitmap inside
> > SkMask. I do not want to add any lifetime burden to that struct.
> > 
> > Since this CL is having to use/modify resourcecache, which is itself very
new
> > and still undergoing semi-frequent internal changes, perhaps you might table
> > this CL, or assign the bug to me. Not required, but there are lots of Issues
> in
> > skia you might work on that could be more straight-forward than this one (at
> > this time).
> 
> A new patch is uploaded to fix some issues:
> 1) remove SkData field in SkMask.
> 2) Clear the SkMaskCache API. SkData is used for stroing/retrieving mask
cache.
> 
> At present, this implementation doesn't support ashmem. I prefer landing this
CL
> then we improve it continuously with ResourceCache and ashmem in future. I am
> also glad to work on other issues in Skia.

Here is a supporting CL that just tries to enable the ResourceCache to support
"data" w/ and w/o ashmem. If it lands, perhaps this CL can be modified to take
advantage of it?

https://codereview.chromium.org/592843003

[reveman, 2014-09-27 03:07:50]

On 2014/09/23 09:13:10, qiankun wrote:
> On 2014/09/20 12:51:33, reed2 wrote:
> > Actually, its harder than I thought. We need to allow for the mask pixels to
> be
> > allocated by the cache itself, for the case when we are trying to use ashmem
> > (discardable memory).
> > 
> > Today we only have a wrapper for that for bitmaps. This is a bit of
overkill,
> so
> > it might not be worth it until we create a different wrapper more suited to
> > light-weight clients like mask pixels.
> > 
> > I think we tried bitmaps once, but that change also stored the bitmap inside
> > SkMask. I do not want to add any lifetime burden to that struct.
> > 
> > Since this CL is having to use/modify resourcecache, which is itself very
new
> > and still undergoing semi-frequent internal changes, perhaps you might table
> > this CL, or assign the bug to me. Not required, but there are lots of Issues
> in
> > skia you might work on that could be more straight-forward than this one (at
> > this time).
> 
> A new patch is uploaded to fix some issues:
> 1) remove SkData field in SkMask.
> 2) Clear the SkMaskCache API. SkData is used for stroing/retrieving mask
cache.
> 
> At present, this implementation doesn't support ashmem. I prefer landing this
CL
> then we improve it continuously with ResourceCache and ashmem in future. I am
> also glad to work on other issues in Skia.

I don't think we want to use this in chromium unless it can take advantage of
SkDiscardableMemory. If it uses SkDiscardable then this is great and definitely
something we want! If it doesn't, then please allow it to be disabled in
chromium until it does.

[qiankun, 2014-10-15 08:03:07]

Hi reviewers, a new patch is uploaded, which used SkCachedData as cache backend
and SkDicardableMemory as mask data backend. PTAL.

[reed1, 2014-10-15 17:25:03]

I think it is confusing to add a cachedata field to SkMask, when that struct
does not manage it in any way.

I think it is clearer (though maybe more typing) to make that be external to
SkMask, either passing it as a 2nd parameter, or creating a new class that
extends/wraps both a mask and its cached data. (e.g. SkMipMap took this latter
route to the extreme, where there is only the cached version. I think that would
be a bad idea for SkMask, which has lots of uses today where it is not in a
cache.)

[qiankun, 2014-10-16 08:48:56]

On 2014/10/15 17:25:03, reed1 wrote:
> I think it is confusing to add a cachedata field to SkMask, when that struct
> does not manage it in any way.
> 
> I think it is clearer (though maybe more typing) to make that be external to
> SkMask, either passing it as a 2nd parameter, or creating a new class that
> extends/wraps both a mask and its cached data. (e.g. SkMipMap took this latter
> route to the extreme, where there is only the cached version. I think that
would
> be a bad idea for SkMask, which has lots of uses today where it is not in a
> cache.)

Pass cached data as a 2nd parameter when needed. PTAL.

[enne (OOO), 2014-10-21 19:17:29]

I added issue 425427 to this bug's description as this patch makes a giant
performance improvement there.

[reed1, 2014-10-21 21:29:49]

https://codereview.chromium.org/471473002/diff/300001/include/core/SkMaskFilt...
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/471473002/diff/300001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:63: SkIPoint* margin, SkCachedData** data) const;
What is the contract with the caller? If they pass a non-null for data, must it
always be used/returned-as-a-cached-data?

Perhaps, since we're changing the virtual API anyway, would it help/simplify
things to have this (or variants of it) be non-virtual, and introduce a new
onFilterMask virtual? That might allow the non-virtual code to check that data
has been properly allocated if present. I'm still trying to understand the
contract, so perhaps this suggestion is misguided.

https://codereview.chromium.org/471473002/diff/300001/src/core/SkMaskCache.cpp
File src/core/SkMaskCache.cpp (right):

https://codereview.chromium.org/471473002/diff/300001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:68: if (mask->fImage) {
What is this test doing? Have we not already checked this in the Visitor?

https://codereview.chromium.org/471473002/diff/300001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:87: for (int i = 0; i < count; i++) {
will we crash if count > 2? Should we assert (or check in our caller) that count
is exactly 1 or 2?

https://codereview.chromium.org/471473002/diff/300001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:140: if (mask->fImage) {
ditto about this test?

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:321: SkMask::kJustComputeBounds_CreateMode,
&patch->fData);
If we are just computing bounds, shouldn't we pass NULL for the last parameter?

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:375: patch->fCenter.fX =
SkScalarCeilToInt(leftUnstretched) + 1;
Can we set these fields of the patch once, before even calling the cache? That
way we don't have to keep this block, and the one on line 399, in sync.

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:443: SkMask::kJustComputeBounds_CreateMode,
&patch->fData);
again, can/should we pass NULL for the last parameter?

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:486: patch->fOuterRect = dstM.fBounds;
again, can we share setup code for patch between here and line 532?

[reed1, 2014-10-21 21:32:55]

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMask....
src/effects/SkBlurMask.cpp:548: if (NULL == data) {
we have to repeat this pattern pretty often. This kinda suggest a way to
encapsulate the mask and its allocator (alluded to that in an earlier review).
Did you find any pattern that might have worked for that?

https://codereview.chromium.org/471473002/diff/300001/src/effects/SkBlurMask....
src/effects/SkBlurMask.cpp:623: SkMask::FreeImage(dp);
We don't have the if/else pattern here. How is this balanced when data != NULL?
This is hard to see (for me).

[reed1, 2014-10-21 21:47:06]

There are so many sites affected with this approach, and at many of them we are
either passing null, or ignoring the new parameter (is that always safe?), I
wonder if it would be clearer if we just had SkBlurMask.cpp itself perform all
of the cache logic, and just copy the answer into the "dst" (as it does today,
but it also has to compute it). I wonder if that couldn't get this CL (an
alternate CL) down to maybe only 3 files.

https://codereview.chromium.org/471473002/diff/300001/include/core/SkMaskFilt...
File include/core/SkMaskFilter.h (right):

https://codereview.chromium.org/471473002/diff/300001/include/core/SkMaskFilt...
include/core/SkMaskFilter.h:14: #include "SkCachedData.h"
SkCachedData.h is a private header, and so cannot be included here. You should
be able to forward-declare it instead...

[reed1, 2014-10-21 22:22:21]

This chain of CLs has been extremely valuable. Inspired and in support of these,
we have landed numerous related changes to SkResourceCache, and then
SkBitmapCache and SkMipMapCache. All of these, in some way, have been leading up
to Skia's ability to have some sort of caching for masks.

There is now a high-priority bug associated with this area, so landing a fix for
masks is very important.

I know I just gave you a slew of comments. None of them are really about the
core technology, your SkMaskCache, but how we try to use it inside Skia. The
approach this CL is taking is definitely worth trying, but looking at it now,
there are a surprising (but correct) number of call sites that had to be
modified. Perhaps this is the best way, or perhaps there is another. Either way,
I'd like to land (with unittests) SkMaskCache now. Can you pull that into a
separate CL with just those two files + the test?

With that landed, I think we should compare this current integration approach
(this CL) with at least one alternative, the idea of just performing the caching
logic inside SkBlurMask.cpp. I don't know if that alternate approach will be
better or not, but together I think we should compare the two ideas.

My proposal is the following:
1. you land your SkMaskCache.[cpp,h] + a unittest asap (new CL)
2.1 you consider the comments I have made on this CL
2.2 in parallel I will investigate an alternate way to use your SkMaskCache
3. we can together compare the two approaches.

Does that sound ok?

[qiankun, 2014-10-22 08:25:54]

Thanks for your review, reed. I separated SkMaskCache from this CL. You can find
it at: https://codereview.chromium.org/670063004/. About the next work, I agreed
to your proposal. Hope we can find the best implementation:)

[qiankun, 2014-10-29 01:56:58]

https://codereview.chromium.org/669993003/ (by reed) and
https://codereview.chromium.org/670063004/ (by Qiankun) have been landed to fix
the poor box shadow performance. So, close this issue.