Add the lazy decoder from PictureFlags to SkImageDecoder

Add the lazy decoder from PictureFlags to SkImageDecoder

Committed: http://code.google.com/p/skia/source/detail?r=10111

[sglez, 2013-07-12 18:12:27]

I am leaving PictureFlags alone until I get Leon's feedback on these changes.

[caryclark, 2013-07-12 18:29:39]

LGTM but let Leon weigh in on it as well before you commit

https://codereview.chromium.org/19109002/diff/1/include/core/SkImageDecoder.h
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/19109002/diff/1/include/core/SkImageDecoder.h...
include/core/SkImageDecoder.h:19: #include "SkPurgeableImageCache.h"
this may be correct but why is it required? I don't see a reference in your
changes below

[sglez, 2013-07-13 04:11:27]

Small push.
Left a comment for Leon below.

https://codereview.chromium.org/19109002/diff/1/include/core/SkImageDecoder.h
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/19109002/diff/1/include/core/SkImageDecoder.h...
include/core/SkImageDecoder.h:19: #include "SkPurgeableImageCache.h"
On 2013/07/12 18:29:39, caryclark wrote:
> this may be correct but why is it required? I don't see a reference in your
> changes below

Good catch. I was using it when trying to implement Leon's MyCacheSelector. I
decided to wait until I got to talk to him, but forgot to remove this include

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder.cpp
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder....
src/images/SkImageDecoder.cpp:287: SkLruImageCache
SkImageDecoder::fLruImageCache(1024*1024);
Leon, why 1024^2? I copied this from what you have in PictureRenderFlags but I
don't really understand the choice of size.

Also, this is not a verbatim copy of lazy_decode_bitmap. I didn't implement your
elastic cache class. I'll talk to you on Monday?

[scroggo, 2013-07-15 15:09:28]

I think SkImageDecoder is the wrong place to put LazyDecodeBitmap. More details
in line. Let me know if you want to discuss.

https://codereview.chromium.org/19109002/diff/5001/include/core/SkImageDecoder.h
File include/core/SkImageDecoder.h (right):

https://codereview.chromium.org/19109002/diff/5001/include/core/SkImageDecode...
include/core/SkImageDecoder.h:104: static SkLruImageCache* getLruImageCache() {
This does not generally relate to SkImageDecoder, so I don't think that it
belongs here.

https://codereview.chromium.org/19109002/diff/5001/include/core/SkImageDecode...
include/core/SkImageDecoder.h:105: return &fLruImageCache;
SkImageDecoder.h is built by chromium, but SkImageDecoder.cpp is not (instead,
it builds SkImageDecoder_empty.cpp, because it does not use SkImageDecoder, but
some of our other files depend on it). This means that this static variable
(which chromium tries to avoid anyway) won't get initialized.

https://codereview.chromium.org/19109002/diff/5001/include/core/SkImageDecode...
include/core/SkImageDecoder.h:321: static bool LazyDecodeBitmap(const void*
buffer, size_t size, SkBitmap* bitmap);
I support moving LazyDecodeBitmap to a header file for documentation/ease of
access, but I don't think SkImageDecoder is the right place for it.

Although it uses SkImageDecoder, SkImageDecoder does not use it, so it could
easily live in its own file(s).

I lean towards leaving the files in tools/ until/unless we decide to make it
public. LazyDecodeBitmap is only used by our tools, and may undergo some changes
before we get our clients to use it.

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder.cpp
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder....
src/images/SkImageDecoder.cpp:19: __SK_FORCE_IMAGE_DECODER_LINKING;
If we move __SK_FORCE_IMAGE_DECODER_LINKING here, we might as well move all of
the contents of SkForceLinking here.

The code for forcing linking has moved around a bit - it was here in the past.
I'll have to look back at it's history to reaffirm that its current location
makes sense. (You're free to investigate as well. It moved to SkForceLinking in
https://codereview.chromium.org/15806010)

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder....
src/images/SkImageDecoder.cpp:287: SkLruImageCache
SkImageDecoder::fLruImageCache(1024*1024);
On 2013/07/13 04:11:27, sglez wrote:
> Leon, why 1024^2? I copied this from what you have in PictureRenderFlags but I
> don't really understand the choice of size.

This was arbitrary. I may have based it off of a number used elsewhere. If we
are to actually use it in production code, we should test to find the ideal
size.

> 
> Also, this is not a verbatim copy of lazy_decode_bitmap. I didn't implement
your
> elastic cache class. I'll talk to you on Monday?

https://codereview.chromium.org/19109002/diff/5001/src/images/SkImageDecoder....
src/images/SkImageDecoder.cpp:296: if (!gOnce) {
This is not threadsafe. In its current use in our tools, we never call this
function from anything but the main thread, but if we are to make it public, we
should acquire a mutex.

Searching through our code, it appears we do not generally lock a mutex for a
lazy initialization like this...

[sglez, 2013-07-15 17:09:17]

Thanks for the review. This new patch leaves SkImageDecoder untouched.

New files are tools/LazyDecodeBitmap.(cpp|h) and they are basically copies of
what used to be the lazy_decode_bitmap section in PictuRenderingFlags.

I am not sure about defining the useVolatileCache flag in this file and
including PictureRenderingFlags.h (which is not necessary for compilation)

I kept your MyCache class but renamed it to ElasticCache because I liked the
name.

[scroggo, 2013-07-15 18:02:41]

https://codereview.chromium.org/19109002/diff/18010/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/19109002/diff/18010/gyp/tools.gyp#newcode134
gyp/tools.gyp:134: '../tools/LazyDecodeBitmap.cpp',
I think it would be better to include LazyDecodeBitmap.cpp in picture_renderer.
As is, picture_renderer is incomplete (requires compiling an additional file in
order to depend on it). If you include it in picture_renderer, you can add
picture_renderer to dependencies (if it's not already there) instead of
compiling this extra file.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cpp
File tools/LazyDecodeBitmap.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cp...
tools/LazyDecodeBitmap.cpp:1: #include "LazyDecodeBitmap.h"
Copyright header.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cp...
tools/LazyDecodeBitmap.cpp:22: class ElasticCacheSelector : public
SkBitmapFactory::CacheSelector {
Naming nit: I don't see what makes this CacheSelector "elastic".

On the other hand, I don't think MyCacheSelector is necessarily a better name.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h
File tools/LazyDecodeBitmap.h (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:2: * Copyright 2006 The Android Open Source Project
2013

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:12: #include "SkBitmap.h"
SkBitmap can be forward-declared here, and included in the implementation file.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:13: 
Maybe add a comment here, explaining its purpose. The current implementation is
intertwined with PictureRenderingFlags and SkCommandLineFlags, which is not
currently obvious by looking at the header.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:14: bool LazyDecodeBitmap(const void* buffer, size_t
size, SkBitmap* bitmap);
This should go inside a class or namespace. It could go in the namespace for
sk_tools, or something more specific to bitmap decoding.

https://codereview.chromium.org/19109002/diff/18010/tools/bench_pictures_main...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/bench_pictures_main...
tools/bench_pictures_main.cpp:148: extern bool lazy_decode_bitmap(const void*
buffer, size_t size, SkBitmap* bitmap);
This is no longer needed.

https://codereview.chromium.org/19109002/diff/18010/tools/lua/lua_pictures.cpp
File tools/lua/lua_pictures.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/lua/lua_pictures.cp...
tools/lua/lua_pictures.cpp:32: extern bool lazy_decode_bitmap(const void*
buffer, size_t size, SkBitmap*);
No longer needed.

https://codereview.chromium.org/19109002/diff/18010/tools/render_pictures_mai...
File tools/render_pictures_main.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/render_pictures_mai...
tools/render_pictures_main.cpp:50: extern bool lazy_decode_bitmap(const void*
buffer, size_t size, SkBitmap* bitmap);
No longer needed.

[sglez, 2013-07-16 15:59:32]

https://codereview.chromium.org/19109002/diff/18010/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/19109002/diff/18010/gyp/tools.gyp#newcode134
gyp/tools.gyp:134: '../tools/LazyDecodeBitmap.cpp',
On 2013/07/15 18:02:41, scroggo wrote:
> I think it would be better to include LazyDecodeBitmap.cpp in
picture_renderer.
> As is, picture_renderer is incomplete (requires compiling an additional file
in
> order to depend on it). If you include it in picture_renderer, you can add
> picture_renderer to dependencies (if it's not already there) instead of
> compiling this extra file.

Good idea

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cpp
File tools/LazyDecodeBitmap.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cp...
tools/LazyDecodeBitmap.cpp:22: class ElasticCacheSelector : public
SkBitmapFactory::CacheSelector {
On 2013/07/15 18:02:41, scroggo wrote:
> Naming nit: I don't see what makes this CacheSelector "elastic".
> 
> On the other hand, I don't think MyCacheSelector is necessarily a better name.

How about just CacheSelector?

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h
File tools/LazyDecodeBitmap.h (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:12: #include "SkBitmap.h"
On 2013/07/15 18:02:41, scroggo wrote:
> SkBitmap can be forward-declared here, and included in the implementation
file.

It compiles with just the forward declaration. Some other header must be
including SkBitmap.

https://codereview.chromium.org/19109002/diff/18010/tools/bench_pictures_main...
File tools/bench_pictures_main.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/bench_pictures_main...
tools/bench_pictures_main.cpp:148: extern bool lazy_decode_bitmap(const void*
buffer, size_t size, SkBitmap* bitmap);
On 2013/07/15 18:02:41, scroggo wrote:
> This is no longer needed.

I sould have caught these.

[scroggo, 2013-07-16 16:06:18]

lgtm, once you add the include in LazyDecodeBitmap.cpp

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cpp
File tools/LazyDecodeBitmap.cpp (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.cp...
tools/LazyDecodeBitmap.cpp:22: class ElasticCacheSelector : public
SkBitmapFactory::CacheSelector {
On 2013/07/16 15:59:33, sglez wrote:
> On 2013/07/15 18:02:41, scroggo wrote:
> > Naming nit: I don't see what makes this CacheSelector "elastic".
> > 
> > On the other hand, I don't think MyCacheSelector is necessarily a better
name.
> 
> How about just CacheSelector? 

Fine with me.

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h
File tools/LazyDecodeBitmap.h (right):

https://codereview.chromium.org/19109002/diff/18010/tools/LazyDecodeBitmap.h#...
tools/LazyDecodeBitmap.h:12: #include "SkBitmap.h"
On 2013/07/16 15:59:33, sglez wrote:
> On 2013/07/15 18:02:41, scroggo wrote:
> > SkBitmap can be forward-declared here, and included in the implementation
> file.
> 
> It compiles with just the forward declaration. Some other header must be
> including SkBitmap.

Although the forward declaration is enough to compile, it is better practice to
include the things you use. If the include chain ceases to include SkBitmap.h,
you'll get a break here without changing LazyDecodeBitmap directly. Please add
#include "SkBitmap.h" in LazyDecodeBitmap.cpp.

[commit-bot: I haz the power, 2013-07-16 18:13:22]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sglez@google.com/19109002/46002

[commit-bot: I haz the power, 2013-07-16 18:21:48]

Change committed as 10111