Implement a persistent uniqueID-based cache for SkImageFilter.

Implement a persistent uniqueID-based cache for SkImageFilter.

Add a unique ID to SkImageFilter, and use it as part
of a persistent cache of image-filtered results. This is used for
caching frame-to-frame coherent filters.

We also keep track of which filter subtrees do not reference the
src input, and use a GenID of zero for the src input in that case.
That way, subtrees which are not dependent on the filter input can be
cached independently of it.

This gives approximately a 4X speedup on
letmespellitoutforyou.com/samples/svg/filter_terrain.svg on Z620
and Nexus10. The cache key consists of the uniqueID of the filter, the
clip bounds, the CTM and the genID of the input bitmap.

Since this does not yet handle the case where the input primitives
(and part of the resulting filter tree) are unchanged, we have
to keep around the external cache for that painting case.
When the work to cache unchanging input primitives is done, the
old cache can be removed, and the new UniqueIDCache will be renamed
to Cache.

Committed: https://skia.googlesource.com/skia/+/55b6d8be997a447ef9ce0f029697677a940bfc24

[Stephen White, 2014-07-22 19:52:22]

Mike, Brian: PTAL. This is a simplification of the previous SHA1-based cache to
use a generation ID instead.

Note that the sizes of the two caches (CPU and GPU) are currently hardcoded to
128MB for CPU, and 32MB for GPU. I could provide a setImageFilterCacheLimits()
API on GrContext if desired. It looks like the CPU one should go on SkGraphics.
Let me know what you think.

[mtklein, 2014-07-22 20:09:21]

Most of my same questions from the old CL still apply, though clearly we can
ignore the issue of the mutable hashes now.

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
include/core/SkImageFilter.h:340: private:
Double private may be overkill ;)

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
include/core/SkImageFilter.h:346: uint32_t fGenerationID;
Can you tack on // globally unique?   I get confused about which genIDs are
per-instance and which are per-class.

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:27: int32_t NextImageFilterGenerationID() {
static int32_t next_image_filter_generation_id() { ... } ?

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:78: fGenerationID = buffer.readUInt();
Seems like we shouldn't logically be serializing genIDs.  What breaks if we
don't?

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:117: if (NULL == fInputs[i] ||
fInputs[i]->usesSrcInput()) {
Just checking, a NULL input means fUsesSrcInput should be true?

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:149: GenIDCache::Key key(getGenerationID(),
context.ctm(), context.clipBounds(), srcGenID);
Any chance we can safely go a step further and use (uintptr_t)this as part of
the key instead of a genID?

[Stephen White, 2014-07-22 20:33:47]

Thanks for your comments.

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
include/core/SkImageFilter.h:340: private:
On 2014/07/22 20:09:20, mtklein wrote:
> Double private may be overkill ;)

Fixed.

https://codereview.chromium.org/414483003/diff/90001/include/core/SkImageFilt...
include/core/SkImageFilter.h:346: uint32_t fGenerationID;
On 2014/07/22 20:09:20, mtklein wrote:
> Can you tack on // globally unique?   I get confused about which genIDs are
> per-instance and which are per-class.

Done.

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:27: int32_t NextImageFilterGenerationID() {
On 2014/07/22 20:09:20, mtklein wrote:
> static int32_t next_image_filter_generation_id() { ... } ?

Done.

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:78: fGenerationID = buffer.readUInt();
On 2014/07/22 20:09:20, mtklein wrote:
> Seems like we shouldn't logically be serializing genIDs.  What breaks if we
> don't?

This is actually crucially important. Blink maintains a tree 
of SkImageFilters corresponding to DOM nodes. As the DOM nodes are invalidated
(via JS or animation), Blink throws away the 
SkImageFilters from the subgraph rooted at that node. They're then
regenerated from the DOM nodes on the next draw. The caching works
only because the GenIDs of the nodes which haven't changed are the
same between two successive SkPictures that Skia receives.

Note that the GenID is serialized but ignored in the cross-process
case, so it's only within a process that matches are guaranteed.
(If you deserialize cross-process, you get a newly-generated GenID.)
Since genID generation is thread-safe, I think this should be 
correct, even for apps other than Chrome which don't have a strict
producer-consumer relationship.

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:117: if (NULL == fInputs[i] ||
fInputs[i]->usesSrcInput()) {
On 2014/07/22 20:09:20, mtklein wrote:
> Just checking, a NULL input means fUsesSrcInput should be true?

Yes, it looks a little odd, but it's correct: NULL means this input 
isn't connected to anything, so the filter system substitutes the 
rendered input primitive instead. This keeps simple filter graphs
simple.

https://codereview.chromium.org/414483003/diff/90001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:149: GenIDCache::Key key(getGenerationID(),
context.ctm(), context.clipBounds(), srcGenID);
On 2014/07/22 20:09:21, mtklein wrote:
> Any chance we can safely go a step further and use (uintptr_t)this as part of
> the key instead of a genID?

Nope. This is how the old cache works, but it won't work for
frame-to-frame coherence, which is the goal here.

[mtklein, 2014-07-22 20:43:10]

Thanks!  This is starting to make sense to me.  Going to mull it a bit
overnight.

[Stephen White, 2014-07-22 20:47:17]

On 2014/07/22 20:33:47, Stephen White wrote:
> This is actually crucially important. Blink maintains a tree 
> of SkImageFilters corresponding to DOM nodes. As the DOM nodes are invalidated
> (via JS or animation), Blink throws away the 
> SkImageFilters from the subgraph rooted at that node. They're then
> regenerated from the DOM nodes on the next draw. The caching works
> only because the GenIDs of the nodes which haven't changed are the
> same between two successive SkPictures that Skia receives.

Just to follow up on myself: if SkPaint is no longer flattened into
SkPictures (which I think your SkRecord work will enable -- correct
me if I'm wrong), we may not need to use GenIDs anymore, and can use 
the pointers again, since it'll be the same SkImageFilter that was 
created on the Blink side.

[mtklein, 2014-07-22 21:50:50]

> Just to follow up on myself: if SkPaint is no longer flattened into
> SkPictures (which I think your SkRecord work will enable -- correct
> me if I'm wrong), we may not need to use GenIDs anymore, and can use 
> the pointers again, since it'll be the same SkImageFilter that was 
> created on the Blink side.

Yup.  Correct on all points.

[Stephen White, 2014-07-24 20:41:42]

New patch up.

After some offline discussion with Brian, the consensus is not to persist 
texture-backed SkBitmaps between frames on the GPU path. So we now create a 
"transient" cache for the GPU path that's freed after each filter traversal 
(as the existing SkImageFilter cache is), and a persistent one for the CPU 
path. This will mean lower GPU performance for now, but at least we won't 
regress CPU performance over the legacy Blink path.

We'll revisit the GPU path once the GrResourceCache redesign/cleanup is done.

[bsalomon, 2014-07-25 14:11:58]

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
include/core/SkImageFilter.h:64: class GenIDCache : public SkRefCnt {
This guy could probably use a comment about what is actually being cached. It's
called GenID cached but it's really caching the filter results based on the
inputs, filter gen id, matrix, ...

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
include/core/SkImageFilter.h:330: uint32_t fGenerationID; // Globally unique
Is this assigned at creation and then never changes? We've taken to calling such
values "unique id" and using "gen id" for objects where a content change can
invalidate the id (e.g. pixelref).

https://codereview.chromium.org/414483003/diff/230001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/230001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:496: class GenIDCacheImpl : public
SkImageFilter::GenIDCache {
Curious, why the virtual interface?

https://codereview.chromium.org/414483003/diff/230001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/414483003/diff/230001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1429: SkAutoTUnref<SkImageFilter::GenIDCache>
cache(getImageFilterCache());
A comment here about tossing the textures after filtering is completed?

[Stephen White, 2014-07-25 17:34:17]

Thanks for your review.

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
include/core/SkImageFilter.h:64: class GenIDCache : public SkRefCnt {
On 2014/07/25 14:11:58, bsalomon wrote:
> This guy could probably use a comment about what is actually being cached.
It's
> called GenID cached but it's really caching the filter results based on the
> inputs, filter gen id, matrix, ...

Done. BTW, the name is temporary -- my plan is that, once we have
caching of unchanging source primitives implemented, we can remove 
the (external) Cache above, and this one can become the One True
SkImageFilter::Cache.

https://codereview.chromium.org/414483003/diff/230001/include/core/SkImageFil...
include/core/SkImageFilter.h:330: uint32_t fGenerationID; // Globally unique
On 2014/07/25 14:11:58, bsalomon wrote:
> Is this assigned at creation and then never changes? We've taken to calling
such
> values "unique id" and using "gen id" for objects where a content change can
> invalidate the id (e.g. pixelref).

Done.

https://codereview.chromium.org/414483003/diff/230001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/230001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:496: class GenIDCacheImpl : public
SkImageFilter::GenIDCache {
On 2014/07/25 14:11:58, bsalomon wrote:
> Curious, why the virtual interface?

It's just to keep the implementation details out of the .h.
Since the .h is in the public API, I'd rather keep as much
as possible hidden from clients.

https://codereview.chromium.org/414483003/diff/230001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/414483003/diff/230001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1429: SkAutoTUnref<SkImageFilter::GenIDCache>
cache(getImageFilterCache());
On 2014/07/25 14:11:58, bsalomon wrote:
> A comment here about tossing the textures after filtering is completed?

Done. Also done in SkGpuDevice::drawDevice(), which I realized was 
not properly unref'ing it (whoops).

Note that SkCanvas::internalDrawDevice() also does this in the GPU 
case, but not in the CPU case (which gets a ref on the singleton
instead).

[Stephen White, 2014-07-25 17:57:52]

New patch up:

- renamed GenID -> UniqueID where appropriate
- added comments
- fixed SkGpuDevice::drawDevice()

[bsalomon, 2014-07-28 15:07:29]

https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:32: struct SkImageFilter::UniqueIDCache::Key {
Maybe there should be a static assert that this type is tightly packed (since it
is hashed).

[Stephen White, 2014-07-29 15:42:40]

On 2014/07/28 15:07:29, bsalomon wrote:
>
https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter....
> src/core/SkImageFilter.cpp:32: struct SkImageFilter::UniqueIDCache::Key {
> Maybe there should be a static assert that this type is tightly packed (since
it
> is hashed).

Good idea. Is there an Sk-equivalent to GR_STATIC_ASSERT?

[Stephen White, 2014-07-29 15:49:47]

On 2014/07/29 15:42:40, Stephen White wrote:
> On 2014/07/28 15:07:29, bsalomon wrote:
> >
>
https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter.cpp
> > File src/core/SkImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/414483003/diff/250001/src/core/SkImageFilter....
> > src/core/SkImageFilter.cpp:32: struct SkImageFilter::UniqueIDCache::Key {
> > Maybe there should be a static assert that this type is tightly packed
(since
> it
> > is hashed).
> 
> Good idea. Is there an Sk-equivalent to GR_STATIC_ASSERT?

Never mind; found SK_COMPILE_ASSERT.

New patch up.

[bsalomon, 2014-07-29 19:46:11]

lgtm

[mtklein, 2014-07-30 14:15:39]

https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:29: return sk_atomic_inc(&gImageFilterUniqueID);
return 1+sk_atomic_inc(...) ?  The first time you grab an ID, it will be zero. 
Isn't that a problem if you use 0 as below for srcGenID?

[Stephen White, 2014-07-30 14:50:59]

On 2014/07/30 14:15:39, mtklein wrote:
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter....
> src/core/SkImageFilter.cpp:29: return sk_atomic_inc(&gImageFilterUniqueID);
> return 1+sk_atomic_inc(...) ?  The first time you grab an ID, it will be zero.

> Isn't that a problem if you use 0 as below for srcGenID?

srcGenID is for the source bitmap's GenID (which uses zero as an 
out-of-band value), not the filter's UniqueID (which doesn't).

[bsalomon, 2014-07-30 14:55:21]

On 2014/07/30 14:50:59, Stephen White wrote:
> On 2014/07/30 14:15:39, mtklein wrote:
> >
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter.cpp
> > File src/core/SkImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter....
> > src/core/SkImageFilter.cpp:29: return sk_atomic_inc(&gImageFilterUniqueID);
> > return 1+sk_atomic_inc(...) ?  The first time you grab an ID, it will be
zero.
> 
> > Isn't that a problem if you use 0 as below for srcGenID?
> 
> srcGenID is for the source bitmap's GenID (which uses zero as an 
> out-of-band value), not the filter's UniqueID (which doesn't).

We should probably not allow 0. Other unique IDs in Skia don't and we have this:

/** The unique IDs in Skia reserve 0 has an invalid marker.
 */
#define SK_InvalidUniqueID  0

[Stephen White, 2014-07-30 15:11:10]

On 2014/07/30 14:55:21, bsalomon wrote:
> On 2014/07/30 14:50:59, Stephen White wrote:
> > On 2014/07/30 14:15:39, mtklein wrote:
> > >
> >
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter.cpp
> > > File src/core/SkImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/414483003/diff/310001/src/core/SkImageFilter....
> > > src/core/SkImageFilter.cpp:29: return
sk_atomic_inc(&gImageFilterUniqueID);
> > > return 1+sk_atomic_inc(...) ?  The first time you grab an ID, it will be
> zero.
> > 
> > > Isn't that a problem if you use 0 as below for srcGenID?
> > 
> > srcGenID is for the source bitmap's GenID (which uses zero as an 
> > out-of-band value), not the filter's UniqueID (which doesn't).
> 
> We should probably not allow 0. Other unique IDs in Skia don't and we have
this:
> 
> /** The unique IDs in Skia reserve 0 has an invalid marker.
>  */
> #define SK_InvalidUniqueID  0

OK, done. (Code copied from SkImage.cpp; don't blame me for its mild silliness.)

New patch up.

[mtklein, 2014-07-30 15:29:20]

Ah, sorry to get confused and us off track.  LGTM

https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:31: uint32_t id;
int32_t?

https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:33: id = sk_atomic_inc(&gImageFilterUniqueID) + 1;
Ha.   I wonder why we added the extra + 1.

[bsalomon, 2014-07-30 15:38:28]

lgtm

[Stephen White, 2014-07-30 15:46:07]

On 2014/07/30 15:29:20, mtklein wrote:
> Ah, sorry to get confused and us off track.  LGTM
> 
>
https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter....
> src/core/SkImageFilter.cpp:31: uint32_t id;
> int32_t?

Heh.. error also copied from SkImage.cpp. Fixed.

>
https://codereview.chromium.org/414483003/diff/330001/src/core/SkImageFilter....
> src/core/SkImageFilter.cpp:33: id = sk_atomic_inc(&gImageFilterUniqueID) + 1;
> Ha.   I wonder why we added the extra + 1.

So that it doesn't require a second pass through the do-loop on first call,
causing an
enormous performance hit, human sacrifice, dogs and cats living together, mass
hysteria?
Just guessing.

[mtklein, 2014-07-30 15:54:21]

> So that it doesn't require a second pass through the do-loop on first call,
> causing an
> enormous performance hit, human sacrifice, dogs and cats living together, mass
> hysteria?
> Just guessing.

Yeah, I think so.  Seems fine either way.

I always thought dogs and cats living together was a good thing. :P

[Stephen White, 2014-07-30 16:42:51]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-30 16:43:28]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/414483003/35...

[Stephen White, 2014-07-30 16:47:23]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-30 16:47:31]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/414483003/37...

[commit-bot: I haz the power, 2014-07-30 16:59:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[Stephen White, 2014-07-30 17:02:10]

On 2014/07/30 16:59:07, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> The failing builders are:
>   Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia
>
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

Unchecking, since it looks like I'll need to bump the SKP version and add
backwards-compatible reading.

[Stephen White, 2014-07-30 17:02:15]

The CQ bit was unchecked by senorblanco@chromium.org

[Stephen White, 2014-07-30 17:12:29]

OK, I think I've fixed this. Brian, would you mind taking another look 
to see if the latest patch set makes sense? This is my first time 
bumping the picture version.

[bsalomon, 2014-07-30 18:21:19]

On 2014/07/30 17:12:29, Stephen White wrote:
> OK, I think I've fixed this. Brian, would you mind taking another look 
> to see if the latest patch set makes sense? This is my first time 
> bumping the picture version.

Looks ok to me

[Stephen White, 2014-07-30 18:21:30]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-30 18:22:22]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/414483003/41...

[commit-bot: I haz the power, 2014-07-30 18:26:52]

Change committed as 55b6d8be997a447ef9ce0f029697677a940bfc24