add scaledimagecache

add scaledimagecache

BUG=

Committed: https://code.google.com/p/skia/source/detail?r=10286

[reed1, 2013-07-22 22:09:20]

work in progress, most up for API makes-sense reviewing at this stage

1. Need to expose budget up to SkGraphics.h
2. No cracks about the linear search, it will improve
3. Does not handle mipmaps yet, that may be a separate CL (not sure)
4. Will add unittests before landing

[humper, 2013-07-23 01:25:44]

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.cpp
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:91: SkScaledImageCache::ID*
SkScaledImageCache::findAndLock(const SkBitmap& orig,
Possibly worth a comment here saying that any necessary mutexes will be acquired
(by the static method of nearly the same name) before this function is called.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:114: SkScaledImageCache::ID*
SkScaledImageCache::addAndLock(const SkBitmap& orig,
same as above re: comment

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:134: void
SkScaledImageCache::unlock(SkScaledImageCache::ID* id) {
ditto above re: comment

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:221: SkDebugf("");
Obviously the debug statements are intended to be temporary, just wanted to
remind you to remove them at some point :)

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:301: static SkScaledImageCache* gCache;
I know the standard says that this will be initialized to 0, but it seems like
good practice to explicitly = NULL here anyhow.  Is there a Skia coding
convention that says not to?

[ernstm, 2013-07-23 06:36:50]

I tested the cache in Chrome on Linux with a higher value for the memory limit
(64 MB on a workstation). It works as expected: raster performance on a cache
hit is as fast as without filtering.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.cpp
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:5: static const size_t DEF_BYTE_LIMIT = 2 * 1024
* 1024;
Two megabyte is quite low. Should we set this to a value that makes the cache
big enough to hold at least one full screen image? Maybe use a platform
dependent value?

[reed1, 2013-07-23 14:50:29]

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.cpp
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:5: static const size_t DEF_BYTE_LIMIT = 2 * 1024
* 1024;
On 2013/07/23 06:36:50, Manfred Ernst wrote:
> Two megabyte is quite low. Should we set this to a value that makes the cache
> big enough to hold at least one full screen image? Maybe use a platform
> dependent value?

There is now a define that chrome can set in skia.gyp if its likes, or it can
call the public api in SkGraphics. This is similar to how the current fontcache
is managed.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:91: SkScaledImageCache::ID*
SkScaledImageCache::findAndLock(const SkBitmap& orig,
On 2013/07/23 01:25:44, humper wrote:
> Possibly worth a comment here saying that any necessary mutexes will be
acquired
> (by the static method of nearly the same name) before this function is called.

Done.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:114: SkScaledImageCache::ID*
SkScaledImageCache::addAndLock(const SkBitmap& orig,
On 2013/07/23 01:25:44, humper wrote:
> same as above re: comment

Done.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:134: void
SkScaledImageCache::unlock(SkScaledImageCache::ID* id) {
On 2013/07/23 01:25:44, humper wrote:
> ditto above re: comment

Done.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:221: SkDebugf("");
On 2013/07/23 01:25:44, humper wrote:
> Obviously the debug statements are intended to be temporary, just wanted to
> remind you to remove them at some point :)

Done.

https://codereview.chromium.org/20005003/diff/1/src/core/SkScaledImageCache.c...
src/core/SkScaledImageCache.cpp:301: static SkScaledImageCache* gCache;
On 2013/07/23 01:25:44, humper wrote:
> I know the standard says that this will be initialized to 0, but it seems like
> good practice to explicitly = NULL here anyhow.  Is there a Skia coding
> convention that says not to?

I don't know if anything is written in the doc, but the skia and chrome use the
implicit null in many places.

[reed1, 2013-07-23 19:14:01]

Committed patchset #6 manually as r10286 (presubmit successful).