Create SkLazyPixelRef which performs lazy decoding and caching.

Create SkLazyPixelRef which performs lazy decoding.

The new pixel ref behaves similarly to SkImageRef, with some key differences:
It does not depend on the images project.
It requires an SkImageCache, which handles allocation and caching of the pixel
memory.
It takes a function signature for decoding which decodes into already allocated
pixel memory rather than into an SkBitmap.

Add two implementations of SkImageCache: SkLruImageCache and SkAshmemImageCache.

Replace SkSerializationHelpers::DecodeBitmap with SkPicture::InstallPixelRefProc,
and update sites that referenced it.

SkBitmapFactory now sets the pixel ref to a new object of the new
class SkLazyPixelRef, provided it has an SkImageCache for caching.

Provide an option to do lazy decodes in render_pictures and bench_pictures.

SkPicture:
Eliminate the default parameters in the constructor.
If a proc for decoding bitmaps is installed, use it to decode any encoded
data in subpictures.
When parsing deserializing subpictures, check for success.
When serializing subpictures, pass the picture's bitmap encoder to the
subpicture's call to serialize.

Update BitmapFactoryTest to test its new behavior.

BUG=https://code.google.com/p/skia/issues/detail?id=1008
BUG=https://code.google.com/p/skia/issues/detail?id=1009

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

[reed1, 2013-01-07 21:33:45]

just a comment on SkLazyPixelRef.h (so far)

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h
File include/core/SkLazyPixelRef.h (right):

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h#n...
include/core/SkLazyPixelRef.h:29: *  onLockPixels to set the pixels properly.
Unfortunately, we may need to pass in rowBytes and config for now, as inputs to
the codec, since SkBitmap will have already decided what these are. We can
(hopefully) relax this later if we have the pixelref *return* its
rowbytes/config/width/height.

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h#n...
include/core/SkLazyPixelRef.h:37: *  @param DecodeBitmap function pointer to
decode the pixels when needed. Must be non-NULL.
Can we take SkData instead of ptr/length here?

The decode-proc guy doesn't ever care, since it is stateless, but this guy, by
definition, will hang onto the data for a while. This signature forces us to
make a deep copy.

[Leon, 2013-01-07 22:15:15]

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h
File include/core/SkLazyPixelRef.h (right):

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h#n...
include/core/SkLazyPixelRef.h:29: *  onLockPixels to set the pixels properly.
On 2013/01/07 21:33:45, reed1 wrote:
> Unfortunately, we may need to pass in rowBytes and config for now, as inputs
to
> the codec, since SkBitmap will have already decided what these are. We can
> (hopefully) relax this later if we have the pixelref *return* its
> rowbytes/config/width/height.

Unfortunately we don't have that information. (But we could if we land
https://codereview.appspot.com/6904072/). Still, I'm not sure what the codec
would do with that information, besides assert.

https://codereview.appspot.com/7060052/diff/1/include/core/SkLazyPixelRef.h#n...
include/core/SkLazyPixelRef.h:37: *  @param DecodeBitmap function pointer to
decode the pixels when needed. Must be non-NULL.
On 2013/01/07 21:33:45, reed1 wrote:
> Can we take SkData instead of ptr/length here?
> 
> The decode-proc guy doesn't ever care, since it is stateless, but this guy, by
> definition, will hang onto the data for a while. This signature forces us to
> make a deep copy.

Done.

[reed1, 2013-01-08 22:34:23]

Is it clearer if we move the function-ptr defs into SkBitmap.h?

e.g.

    typedef SkData* (*EncodeProc)(const SkBitmap& src);
    
    enum DecodeStage {
        kJustBounds_DecodeStage,
        kBoundsAndPixels_DecodeStage,
    };
    typedef bool (*DecodeProc)(const void* src, size_t length, DecodeStage,
                               SkBitmap* dst);

(with lots of comments)

https://codereview.appspot.com/7060052/diff/7001/include/core/SkLazyPixelRef.h
File include/core/SkLazyPixelRef.h (right):

https://codereview.appspot.com/7060052/diff/7001/include/core/SkLazyPixelRef....
include/core/SkLazyPixelRef.h:55: SkData*      fData;
is fBitmap really fCachedLockedBitmapAsReturnedFromDecodeProc?

[Leon, 2013-01-09 20:50:49]

> Is it clearer if we move the function-ptr defs into SkBitmap.h?
>
> e.g.
>
>     typedef SkData* (*EncodeProc)(const SkBitmap& src);
>
>     enum DecodeStage {
>         kJustBounds_DecodeStage,
>         kBoundsAndPixels_DecodeStage,
>     };
>     typedef bool (*DecodeProc)(const void* src, size_t length, DecodeStage,
>                                SkBitmap* dst);
>
> (with lots of comments)

That seems reasonable to me. I'm not sure I need the DecodeStage enum though.

https://codereview.appspot.com/7060052/diff/7001/include/core/SkLazyPixelRef.h
File include/core/SkLazyPixelRef.h (right):

https://codereview.appspot.com/7060052/diff/7001/include/core/SkLazyPixelRef....
include/core/SkLazyPixelRef.h:55: SkData*      fData;
On 2013/01/08 22:34:23, reed1 wrote:
> is fBitmap really fCachedLockedBitmapAsReturnedFromDecodeProc?

Yes, although I'm not a fan of such a long name. I'll come up with something
more succinct and add a comment.

[Leon, 2013-01-10 18:31:19]

New patch. I've updated the description, and removed you as a reviewer from my
other CLs, so this is the one you should look at.

[reed1, 2013-01-10 19:03:34]

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:590: *  Function signature defining a function that sets
up an SkBitmap from encoded data. On
s/pixels/pixelRef

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:591: *  success, the SkBitmap should have its Config,
width, height, rowBytes and pixels set.
* If the installed pixelref has decoded the data into pixels, then the src
buffer need not be copied. If the pixelref defers the actual decode until its
lockPixels() is called, then it must make a copy of the src buffer.

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:596: */
InstallPixelRefProc?

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:597: typedef bool (*DecodeProc)(const void* src, size_t
length, SkBitmap* dst);
I wonder if we want this or a diff verison to take SkData for the src, so the
caller doesn't have to make a deep copy...?

In the case of picture-deserialization, how is the encoded-data stored when you
call this?

https://codereview.appspot.com/7060052/diff/7003/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/core/SkPicture.h#new...
include/core/SkPicture.h:46: explicit SkPicture(SkStream*, bool* success = NULL,
SkBitmap::DecodeProc decoder = NULL);
how about a new constructor with no default arguments.

https://codereview.appspot.com/7060052/diff/7003/include/images/SkBitmapFacto...
File include/images/SkBitmapFactory.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/images/SkBitmapFacto...
include/images/SkBitmapFactory.h:26: static bool DecodeBitmap(const void* data,
size_t length, SkBitmap*);
- DirectDecodeProc
- LazyDecodeProc
- AshmemDecodeProc?

This makes me wonder if the caller may want an object instead of just a
function-ptr, so it can have state.

[Leon, 2013-01-11 17:52:04]

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:590: *  Function signature defining a function that sets
up an SkBitmap from encoded data. On
On 2013/01/10 19:03:34, reed1 wrote:
> s/pixels/pixelRef

Done.

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:591: *  success, the SkBitmap should have its Config,
width, height, rowBytes and pixels set.
On 2013/01/10 19:03:34, reed1 wrote:
> * If the installed pixelref has decoded the data into pixels, then the src
> buffer need not be copied. If the pixelref defers the actual decode until its
> lockPixels() is called, then it must make a copy of the src buffer.

Done.

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:596: */
On 2013/01/10 19:03:34, reed1 wrote:
> InstallPixelRefProc?

Done.

https://codereview.appspot.com/7060052/diff/7003/include/core/SkBitmap.h#newc...
include/core/SkBitmap.h:597: typedef bool (*DecodeProc)(const void* src, size_t
length, SkBitmap* dst);
On 2013/01/10 19:03:34, reed1 wrote:
> I wonder if we want this or a diff verison to take SkData for the src, so the
> caller doesn't have to make a deep copy...?

Agreed that we want to avoid making a deep copy if possible. Maybe the question
is what form the data will be in coming from Chrome. If it is not already in the
form of an SkData, and the caller doesn't know whether the implementation will
need to keep it around, the caller may have to make a deep copy. On the other
hand, as written, the implementation needs to make a deep copy in order to
safely hang onto it.

> In the case of picture-deserialization, how is the encoded-data stored when
you
> call this?

During picture-deserialization, the encoded data comes from an
SkOrderedReadBuffer that does not stick around, so in that case it needs to be
copied if the caller wants to lazily decode.

SkBitmapFactory::installPixelRef now takes an SkData parameter, but this one
continues to take a pointer/length. This makes sense for picture
deserialization, since a copy will need to be made anyway in the case of a lazy
decode, and it allows using the same signature as SkImageDecoder::DecodeMemory,
so deserialization can easily do a simple decode instead.

https://codereview.appspot.com/7060052/diff/7003/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/core/SkPicture.h#new...
include/core/SkPicture.h:46: explicit SkPicture(SkStream*, bool* success = NULL,
SkBitmap::DecodeProc decoder = NULL);
On 2013/01/10 19:03:34, reed1 wrote:
> how about a new constructor with no default arguments.

Done.

https://codereview.appspot.com/7060052/diff/7003/include/images/SkBitmapFacto...
File include/images/SkBitmapFactory.h (right):

https://codereview.appspot.com/7060052/diff/7003/include/images/SkBitmapFacto...
include/images/SkBitmapFactory.h:26: static bool DecodeBitmap(const void* data,
size_t length, SkBitmap*);
On 2013/01/10 19:03:34, reed1 wrote:
> - DirectDecodeProc
> - LazyDecodeProc
> - AshmemDecodeProc?
> 
> This makes me wonder if the caller may want an object instead of just a
> function-ptr, so it can have state.


I have a separate CL that will provide the image caching, but I see the argument
for having an object. In my latest patch, I have created an object that can be
used, but I'm still using a function pointer for deserialization. I think that
makes sense because it allows the caller to continue using
SkImageDecoder::DecodeMemory. How does that sound?

[Leon, 2013-02-14 22:57:39]

Based on your verbal feedback, I have made numerous changes to this CL. I have
separated out allocation from decoding, and now the DecodeProc no longer takes
an SkBitmap. I have also included the new image caching mechanism. This required
writing new image decoding wrappers.

You made some suggestions that didn't quite work out: 

1. DecodeProc should match deserialization. I did not make them match, though,
since DecodeProc, by design, decodes into allocated memory, while
deserialization should leave the option to install a pixel ref so that we can
install a lazy pixel ref.

2. Separate the allocator from the cache. Since the cache releases the memory,
it needs to know how it was allocated, so that it can properly reverse the
process. Further,  In light of that, I think it makes more sense to keep them
tied together.

Please take a look when you get a chance.

[reed1, 2013-02-15 15:56:22]

Wow, this has become a big CL. It is possible to show just a CL that does one
piece of this? Perhaps just a CL that adds the function-ptr to pictures, or just
a CL that implements a deferred pixelref?

We can talk in person next week too.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:18: 
I'm not sure where else, but it does seem a little odd that this header
introduces a struct it never defines, and function-ptrs it never references...

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:601: */
I understand we are not *always* decoding when this function executes, but I
still find its name ambiguous. InstallPixelRefFromEncodedDataProc is probably
too long.

The equivalent in SkImage world is SkImage::NewEncodedData (though that API
doesn't yet have a raw ptr+len varient).

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:59: 
Maybe its time to replace this constructor with a Factory, which can just return
NULL on failure.

[Leon, 2013-02-19 16:31:46]

> Wow, this has become a big CL. It is possible to show just a CL that does one
> piece of this? Perhaps just a CL that adds the function-ptr to pictures, or
just
> a CL that implements a deferred pixelref?

I agree, it has gotten a little big. Is there anything about the general
approach that doesn't match the constraints you want?

> We can talk in person next week too.

Yes, let's. My schedule is wide open. In particular, let's discuss what should
be a good initial check in.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:18: 
On 2013/02/15 15:56:22, reed1 wrote:
> I'm not sure where else, but it does seem a little odd that this header
> introduces a struct it never defines, and function-ptrs it never references...

You recommended adding the function-ptrs here, but I'll think about what might
be a better place for them. I think it makes sense, since they both operate on
SkBitmaps. Right now, InstallPixelRefProc is only used by SkPicture (and to
support that, SkPicturePlayback and SkOrderedReadBuffer). That hints that
SkPicture may be a good place for them, although if we intend to have other
callers that will no longer be a good place. (EncodeProc isn't yet used
anywhere, so should be left out of this CL.)

The struct is referenced here so it can call ComputeSafeSize, which is a private
function on SkBitmap, but I can make it a public function outside of SkBitmap
(since it doesn't actually take , in which case we don't need to reference
SkImageInfo here.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:601: */
On 2013/02/15 15:56:22, reed1 wrote:
> I understand we are not *always* decoding when this function executes, but I
> still find its name ambiguous. InstallPixelRefFromEncodedDataProc is probably
> too long.
> 
> The equivalent in SkImage world is SkImage::NewEncodedData (though that API
> doesn't yet have a raw ptr+len varient).

Noted. I'll think about a better name while I work on a new home for it.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:59: 
On 2013/02/15 15:56:22, reed1 wrote:
> Maybe its time to replace this constructor with a Factory, which can just
return
> NULL on failure.

Sounds good to me.

[reed1, 2013-02-20 15:57:11]

https://codereview.appspot.com/7060052/diff/43001/include/core/SkImageCache.h
File include/core/SkImageCache.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkImageCache.h...
include/core/SkImageCache.h:29: */
intptr_t instead of int32_t

[Leon, 2013-02-21 22:23:56]

New CL up for review.

The latest addresses our discussion the other day: the decode proc now has an
output struct (shared with SkImage - I did not use the new SkImageInfo because I
take advantage of existing functions that use SkImage::Info) and an input struct
for address/rowbytes.

Speaking of rowbytes, what is the correct type? SkBitmap stores it as a
uint32_t, but SkBitmap::rowBytes() returns an int, and SkImageMinRowBytes
returns a size_t.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:18: 
On 2013/02/15 15:56:22, reed1 wrote:
> I'm not sure where else, but it does seem a little odd that this header
> introduces a struct it never defines, and function-ptrs it never references...

Removed the function pointers (InstallPixelRefProc was moved into SkPicture, and
EncodeProc was removed).

https://codereview.appspot.com/7060052/diff/43001/include/core/SkImageCache.h
File include/core/SkImageCache.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkImageCache.h...
include/core/SkImageCache.h:29: */
On 2013/02/20 15:57:11, reed1 wrote:
> intptr_t instead of int32_t

Done.

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/7060052/diff/43001/include/core/SkPicture.h#ne...
include/core/SkPicture.h:59: 
On 2013/02/15 15:56:22, reed1 wrote:
> Maybe its time to replace this constructor with a Factory, which can just
return
> NULL on failure.

I'd like to leave this to a separate CL

[reed1, 2013-02-22 17:10:02]

really close, nice work.

not clear that lazy is the perfect name for the directory, but that'll do for
now.

https://codereview.appspot.com/7060052/diff/78001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/core/SkImage.h#newc...
include/core/SkImage.h:69: *  for this size.
1. who calls this?
2. does it need to be public?
3. from the dox, is sounds like ComputeMinRowBytesAndSize

https://codereview.appspot.com/7060052/diff/78001/include/images/SkImageDecod...
File include/images/SkImageDecoder.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/images/SkImageDecod...
include/images/SkImageDecoder.h:235: *  @return bool Whether the function
succeeded.
Perhaps show the two obvious ways to call this in the dox...?

// Just determine the image's info : width/height/config
DecodememoryToTarget(src, size, &info, NULL);

... allocate space for the result
target.fAddr = ...
target.fRowBytes = ...

// Now decode the actual pixels into target. &info is optional, and could be
NULL
DecodememoryToTarget(src, size, &info, &target);

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkImageCache.h
File include/lazy/SkImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkImageCache.h...
include/lazy/SkImageCache.h:59: *  ID which does not correspond to any valid
cache.
Can we use 0 instead (seems more natural, at least when the ID might be a ptr)?

BAD or UNINITIALIZED?

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkLruImageCache.h
File include/lazy/SkLruImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkLruImageCach...
include/lazy/SkLruImageCache.h:33: virtual void* allocAndPinCache(size_t bytes,
intptr_t* ID) SK_OVERRIDE;
nit: no need for space between these declarations

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
File include/ports/SkAshmemImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
include/ports/SkAshmemImageCache.h:19: *  Get a pointer to the single global
instance of SkAshmemImageCache.
// Can this one instance be used from multiple threads?

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
include/ports/SkAshmemImageCache.h:23: virtual void* allocAndPinCache(size_t
bytes, intptr_t* ID) SK_OVERRIDE;
spacing nit

https://codereview.appspot.com/7060052/diff/78001/src/core/SkOrderedReadBuffer.h
File src/core/SkOrderedReadBuffer.h (right):

https://codereview.appspot.com/7060052/diff/78001/src/core/SkOrderedReadBuffe...
src/core/SkOrderedReadBuffer.h:19: 
Why is this forward decl needed?

https://codereview.appspot.com/7060052/diff/78001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/7060052/diff/78001/src/core/SkPicture.cpp#newc...
src/core/SkPicture.cpp:267: SkPicture::SkPicture(SkStream* stream)
nit: don't need to mention base constructor for SkRefCnt()

[Leon, 2013-02-22 18:43:58]

https://codereview.appspot.com/7060052/diff/78001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/core/SkImage.h#newc...
include/core/SkImage.h:69: *  for this size.
On 2013/02/22 17:10:02, reed1 wrote:
> 1. who calls this?

LazyPixelRef, and a function in render_pictures_main.

> 2. does it need to be public?

For LazyPixelRef, no. render_pictures_main uses it to decide whether to use the
Ashmem cache or the normal one. Perhaps there is no reason to give that much
flexibility to the client. Is it possible that the caller will decide based on
different criteria, or based on different sizes (currently it just checks to see
if it is above some arbitrary value that Android uses)?

> 3. from the dox, is sounds like ComputeMinRowBytesAndSize
Agreed. Will fix.

https://codereview.appspot.com/7060052/diff/78001/include/images/SkImageDecod...
File include/images/SkImageDecoder.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/images/SkImageDecod...
include/images/SkImageDecoder.h:235: *  @return bool Whether the function
succeeded.
On 2013/02/22 17:10:02, reed1 wrote:
> Perhaps show the two obvious ways to call this in the dox...?
> 
> // Just determine the image's info : width/height/config
> DecodememoryToTarget(src, size, &info, NULL);
> 
> ... allocate space for the result
> target.fAddr = ...
> target.fRowBytes = ...
> 
> // Now decode the actual pixels into target. &info is optional, and could be
> NULL
> DecodememoryToTarget(src, size, &info, &target);

Done.

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkImageCache.h
File include/lazy/SkImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkImageCache.h...
include/lazy/SkImageCache.h:59: *  ID which does not correspond to any valid
cache.
On 2013/02/22 17:10:02, reed1 wrote:
> Can we use 0 instead (seems more natural, at least when the ID might be a
ptr)?
> 
> BAD or UNINITIALIZED?

Done.

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkLruImageCache.h
File include/lazy/SkLruImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/lazy/SkLruImageCach...
include/lazy/SkLruImageCache.h:33: virtual void* allocAndPinCache(size_t bytes,
intptr_t* ID) SK_OVERRIDE;
On 2013/02/22 17:10:02, reed1 wrote:
> nit: no need for space between these declarations

Done.

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
File include/ports/SkAshmemImageCache.h (right):

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
include/ports/SkAshmemImageCache.h:19: *  Get a pointer to the single global
instance of SkAshmemImageCache.
On 2013/02/22 17:10:02, reed1 wrote:
> // Can this one instance be used from multiple threads?

That was the intent, and I guard calls with a mutex so that it is possible.
However, I was reading somewhere that ashmem is per process, so I'll need to
handle that somehow.

https://codereview.appspot.com/7060052/diff/78001/include/ports/SkAshmemImage...
include/ports/SkAshmemImageCache.h:23: virtual void* allocAndPinCache(size_t
bytes, intptr_t* ID) SK_OVERRIDE;
On 2013/02/22 17:10:02, reed1 wrote:
> spacing nit

Done.

https://codereview.appspot.com/7060052/diff/78001/src/core/SkOrderedReadBuffer.h
File src/core/SkOrderedReadBuffer.h (right):

https://codereview.appspot.com/7060052/diff/78001/src/core/SkOrderedReadBuffe...
src/core/SkOrderedReadBuffer.h:19: 
On 2013/02/22 17:10:02, reed1 wrote:
> Why is this forward decl needed?

Because I removed #include "SkBitmap.h". readBitmap references SkBitmap

https://codereview.appspot.com/7060052/diff/78001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.appspot.com/7060052/diff/78001/src/core/SkPicture.cpp#newc...
src/core/SkPicture.cpp:267: SkPicture::SkPicture(SkStream* stream)
On 2013/02/22 17:10:02, reed1 wrote:
> nit: don't need to mention base constructor for SkRefCnt()

For some reason some of the existing constructors in this file do that. Fixed
for all cases in this file.

[reed1, 2013-02-22 20:17:54]

lgtm w/ two requests to move public things to more private/secluded ones

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h#newc...
include/core/SkImage.h:70: */
Can we move this guy to be private-impl for lazypixelref.cpp?

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h#newc...
include/core/SkImage.h:75: */
Can we move this into bitmapfactory.h?

[Leon, 2013-02-22 21:37:48]

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h#newc...
include/core/SkImage.h:70: */
On 2013/02/22 20:17:54, reed1 wrote:
> Can we move this guy to be private-impl for lazypixelref.cpp?

Done.

https://codereview.appspot.com/7060052/diff/88001/include/core/SkImage.h#newc...
include/core/SkImage.h:75: */
On 2013/02/22 20:17:54, reed1 wrote:
> Can we move this into bitmapfactory.h?

Done.