Add SkMaskCache

Add SkMaskCache

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/d9aac34eceeb197c5551f38583670be7729dd00a

[qiankun, 2014-10-22 08:18:39]

qiankun.miao@intel.com changed reviewers:
+ reed@google.com

[qiankun, 2014-10-22 08:18:40]

PTAL.

[reed1, 2014-10-22 13:36:20]

lgtm w/ nit about "int"

https://codereview.chromium.org/670063004/diff/1/src/core/SkMaskCache.h
File src/core/SkMaskCache.h (right):

https://codereview.chromium.org/670063004/diff/1/src/core/SkMaskCache.h#newco...
src/core/SkMaskCache.h:28: int32_t count, SkMask* mask,
nit: skia just uses "int" for counts.

[reed1, 2014-10-22 13:46:34]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-10-22 13:46:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/670063004/1

[commit-bot: I haz the power, 2014-10-22 13:52:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 13:52:07]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[reed1, 2014-10-22 14:13:58]

Hmmm, I think we need to add blurStyle to the key, since the results vary
depending on that enum (as well as sigma and the geometry).

[reed1, 2014-10-22 14:15:51]

On 2014/10/22 14:13:58, reed1 wrote:
> Hmmm, I think we need to add blurStyle to the key, since the results vary
> depending on that enum (as well as sigma and the geometry).

... unless we only ever cache "Normal" style?

[f(malita), 2014-10-22 14:28:06]

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

[f(malita), 2014-10-22 14:28:07]

Is it safe to throw the new key types into the global resource cache? I think it
is iff their size is unique (vs all other possible global keys), which is not
immediately obvious.

This seems like another case begging for key namespacing - I'll take a look at
that soonish.

[qiankun1, 2014-10-22 14:40:08]

On 2014/10/22 14:28:07, Florin Malita wrote:
> Is it safe to throw the new key types into the global resource cache? I think
it
> is iff their size is unique (vs all other possible global keys), which is not
> immediately obvious.
> 
> This seems like another case begging for key namespacing - I'll take a look at
> that soonish.

As discussed before, we should fix cross-domain collisions for key.

[qiankun1, 2014-10-22 14:41:06]

On 2014/10/22 14:15:51, reed1 wrote:
> On 2014/10/22 14:13:58, reed1 wrote:
> > Hmmm, I think we need to add blurStyle to the key, since the results vary
> > depending on that enum (as well as sigma and the geometry).
> 
> ... unless we only ever cache "Normal" style?

It seems also SkBlurQuality should be added to the key.

[reed1, 2014-10-22 16:05:26]

reed@google.com changed reviewers:
+ mtklein@google.com, robertphillips@google.com

[reed1, 2014-10-22 16:05:27]

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

https://codereview.chromium.org/670063004/diff/80001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:29: SkRRect       fRRect;
SkRRect has as its last field an enum "Type". This may introduce uninitialized
bytes at the end, so this may make SkRRect a risk as a key.

I think either we need to either
1. change SkRRect to not have this problem
2. store some "flattened" version of SkRRect

adding Robert to the review for his opinion.

https://codereview.chromium.org/670063004/diff/80001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:30: SkBlurStyle   fStyle;
Since ResourceKey is going to hash through the entire key as an array of int32s,
we have to be sure that our "this" is a multiple of 4 with no uninitialized
padding.

I think to be totally safe, we need to always store types of explicitly known
sizes. SkBlurStyle and SkBlurQuality are enums, so perhaps those should be
stored as explicit int32_t.

[robertphillips, 2014-10-22 16:52:25]

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

https://codereview.chromium.org/670063004/diff/80001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:29: SkRRect       fRRect;
I think we should "fix" SkRRect.
Additionally, some static asserts regarding the packed nature of the data and
its overall size (i.e., a multiple of 4) would be nice.

[qiankun, 2014-10-22 17:19:54]

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

https://codereview.chromium.org/670063004/diff/80001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:29: SkRRect       fRRect;
What about only use rect and radii of SkRRect?

https://codereview.chromium.org/670063004/diff/80001/src/core/SkMaskCache.cpp...
src/core/SkMaskCache.cpp:30: SkBlurStyle   fStyle;
On 2014/10/22 16:05:26, reed1 wrote:
> Since ResourceKey is going to hash through the entire key as an array of
int32s,
> we have to be sure that our "this" is a multiple of 4 with no uninitialized
> padding.
> 
> I think to be totally safe, we need to always store types of explicitly known
> sizes. SkBlurStyle and SkBlurQuality are enums, so perhaps those should be
> stored as explicit int32_t.

Done.

[reed1, 2014-10-22 18:26:16]

Thanks for the enum-ints.

Florin just landed a good fix to add a unique void* to each cache type, so
you'll need to rebase and support that in your Key's init() method.

I see that you worked-around the potential problem with RRect. That looks
correct, but I think its much harder to read. We also just landed a fix to
RRect, so using it now should be safe.

[f(malita), 2014-10-22 18:31:32]

Just landed a Key change which clashes with this CL, sorry. Probably better to
force unique domains upfront though :)

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

https://codereview.chromium.org/670063004/diff/100001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:30: this->init(sizeof(fSigma) + sizeof(fRect) +
sizeof(fRadii) + sizeof(fStyle) + sizeof(fQuality));
After
https://skia.googlesource.com/skia/+/171e5b73a862418f4acd61faf8cecfbc8f58694c,
this needs a namespace param.

https://codereview.chromium.org/670063004/diff/100001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:108: this->init(sizeof(fSigma) + sizeof(fRecCount) +
sizeof(fRects) + sizeof(fStyle));
Same.

[qiankun, 2014-10-23 02:51:10]

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

https://codereview.chromium.org/670063004/diff/100001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:30: this->init(sizeof(fSigma) + sizeof(fRect) +
sizeof(fRadii) + sizeof(fStyle) + sizeof(fQuality));
On 2014/10/22 18:31:31, Florin Malita wrote:
> After
> https://skia.googlesource.com/skia/+/171e5b73a862418f4acd61faf8cecfbc8f58694c,
> this needs a namespace param.

Done.

https://codereview.chromium.org/670063004/diff/100001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:108: this->init(sizeof(fSigma) + sizeof(fRecCount) +
sizeof(fRects) + sizeof(fStyle));
On 2014/10/22 18:31:31, Florin Malita wrote:
> Same.

Done.

[reed1, 2014-10-23 14:06:47]

lgtm

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

https://codereview.chromium.org/670063004/diff/120001/src/core/SkMaskCache.cp...
src/core/SkMaskCache.cpp:104: SkASSERT(1 == count || 2 == count);
nice

[qiankun, 2014-10-23 14:35:51]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2014-10-23 14:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/670063004/120001

[commit-bot: I haz the power, 2014-10-23 14:58:22]

Committed patchset #7 (id:120001) as d9aac34eceeb197c5551f38583670be7729dd00a