Break up SkLazyPixelRef functionally into class hierarchy.

Break up SkLazyPixelRef functionally into class hierarchy.

The reason for this CL is to allow greater decoder flexibility.
Chrome currently uses its own decoding functions.  These allow for
greater flexibility in dealing with images with multiple frames or
partial data.  The DecodeProc function was not flexible enough to
handle these.  Instead of asking the decoder to squeeze everything
into the DecodeProc, we now ask the downstream library to inherit from
SkCachingPixelRef.  If WebKit's LazyDecodingPixelRef is re-tooled to
inherit from SkCachingPixelRef, then it can make use of Skia's caching
ability while still allowing it to deal with multiple frames, scaling,
subsetting, and partial data.

-   The abstract SkCachingPixelRef class handles caching the decoded
    data in a SkScaledImageCache.  This class relies on the virtual
    functions onDecodeInfo() and onDecode() to do the actual decoding
    of data.

-   The SkLazyCachingPixelRef class is derived from SkCachingPixelRef.
    It provides an implementation of onDecodeInfo() and onDecode() in
    terms of calls to a SkBitmapFactory::DecodeProc function.  It also
    provides an Install() static method which installs a new
    SkLazyCachingPixelRef into a SkBitmap.

SkLazyCachingPixelRef exists for two reasons: to test
SkCachingPixelRef within Skia and as an example for downstream
developers to make their own classes that inherit from
SkCachingPixelRef.

-   The CachedDecodingPixelRefTest was updated to test the
    SkLazyCachingPixelRef class and indirectly the SkCachingPixelRef
    class.

BUG=

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

[scroggo, 2013-10-31 21:53:06]

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

https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h...
src/core/SkScaledImageCache.h:34: static SkScaledImageCache* Instance();
I think typically our functions to get the global instance are named something
like "Get*". Maybe this should be GetGlobalInstance?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cpp
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:15: if (fCache == NULL) {
NULL == fCache.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:19: SkCachingPixelRef::~SkCachingPixelRef() {
Maybe assert that fScaledCacheId is NULL?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:23: if (fErrorInDecoding ||
!fBitmapSize.isZero()) {
This block is painful for me to look at. I think it'd be clearer if you kept the
fErrorInDecoding and size checks separate.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:79: return pixels;
Similar comment why pixels is still valid?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:84: if (!fErrorInDecoding && (NULL !=
fScaledCacheId)) {
If fErrorInDecoding is false, is it ever correct to have a NULL fScaledCacheId
here?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:2: * Copyright 2012 Google Inc.
2013

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:23: *  or be destroyed before the next lock.  If
so, the image will be
or may not*

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:38: explicit SkCachingPixelRef(SkScaledImageCache*
cache = NULL);
It may be obvious, but should we point out that SkCachingPixelRef does not take
ownership of cache?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:42: // SkOrderedWriteBuffer will check the encoded
data and write that
This statement is only true if you have implemented onRefEncodedData.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:78: SkISize                     fBitmapSize;
Why do you have these three instead of an SkImage::Info?

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:82: // lazily initialized our cached info. Returns
NULL on failure.
Returns false on failure.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.cpp:2: * Copyright 2012 Google Inc.
2013

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRef.h
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.h:2: * Copyright 2012 Google Inc.
2013

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.h:42: *  A simplified version of SkBitmapFactory.
 Installes a new
Installs.

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
tests/CachedDecodingPixelRefTest.cpp:158: *  This checks to see that a
SkLazyPixelRef works as advertized.
advertised.

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
tests/CachedDecodingPixelRefTest.cpp:160: DEF_TEST(LazyCachedPixelRefTest,
reporter) {
Could these two tests share code?

[hal.canary, 2013-11-01 03:29:24]

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

https://codereview.chromium.org/54203006/diff/1/src/core/SkScaledImageCache.h...
src/core/SkScaledImageCache.h:34: static SkScaledImageCache* Instance();
On 2013/10/31 21:53:06, scroggo wrote:
> I think typically our functions to get the global instance are named something
> like "Get*". Maybe this should be GetGlobalInstance?

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cpp
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:15: if (fCache == NULL) {
On 2013/10/31 21:53:06, scroggo wrote:
> NULL == fCache.

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:19: SkCachingPixelRef::~SkCachingPixelRef() {
On 2013/10/31 21:53:06, scroggo wrote:
> Maybe assert that fScaledCacheId is NULL?

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:23: if (fErrorInDecoding ||
!fBitmapSize.isZero()) {
On 2013/10/31 21:53:06, scroggo wrote:
> This block is painful for me to look at. I think it'd be clearer if you kept
the
> fErrorInDecoding and size checks separate.

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:79: return pixels;
On 2013/10/31 21:53:06, scroggo wrote:
> Similar comment why pixels is still valid?

I cleaned up the code to make this clear.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.cp...
src/lazy/SkCachingPixelRef.cpp:84: if (!fErrorInDecoding && (NULL !=
fScaledCacheId)) {
On 2013/10/31 21:53:06, scroggo wrote:
> If fErrorInDecoding is false, is it ever correct to have a NULL fScaledCacheId
> here?

I don't think so.  Let me make this clear.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:2: * Copyright 2012 Google Inc.
On 2013/10/31 21:53:06, scroggo wrote:
> 2013

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:23: *  or be destroyed before the next lock.  If
so, the image will be
On 2013/10/31 21:53:06, scroggo wrote:
> or may not*

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:38: explicit SkCachingPixelRef(SkScaledImageCache*
cache = NULL);
On 2013/10/31 21:53:06, scroggo wrote:
> It may be obvious, but should we point out that SkCachingPixelRef does not
take
> ownership of cache?

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:42: // SkOrderedWriteBuffer will check the encoded
data and write that
On 2013/10/31 21:53:06, scroggo wrote:
> This statement is only true if you have implemented onRefEncodedData.

I'll remove this section for now.  Since this is an abstract class, I'm not sure
it matters.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:78: SkISize                     fBitmapSize;
On 2013/10/31 21:53:06, scroggo wrote:
> Why do you have these three instead of an SkImage::Info?

I wanted to keep the onDecodeInfo() interface simple.  These are exactly the
parameters I need to call bitmap.setConfig(...)

And I want this general-purpose interface to be able to produce any valid
bitmap.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:82: // lazily initialized our cached info. Returns
NULL on failure.
On 2013/10/31 21:53:06, scroggo wrote:
> Returns false on failure.

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.cpp:2: * Copyright 2012 Google Inc.
On 2013/10/31 21:53:06, scroggo wrote:
> 2013

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRef.h
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.h:2: * Copyright 2012 Google Inc.
On 2013/10/31 21:53:06, scroggo wrote:
> 2013

Done.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkLazyCachingPixelRe...
src/lazy/SkLazyCachingPixelRef.h:42: *  A simplified version of SkBitmapFactory.
 Installes a new
On 2013/10/31 21:53:06, scroggo wrote:
> Installs.

Done.

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
tests/CachedDecodingPixelRefTest.cpp:158: *  This checks to see that a
SkLazyPixelRef works as advertized.
On 2013/10/31 21:53:06, scroggo wrote:
> advertised.

Done.

https://codereview.chromium.org/54203006/diff/1/tests/CachedDecodingPixelRefT...
tests/CachedDecodingPixelRefTest.cpp:160: DEF_TEST(LazyCachedPixelRefTest,
reporter) {
On 2013/10/31 21:53:06, scroggo wrote:
> Could these two tests share code?

Done.

[Tom Hudson, 2013-11-01 10:12:44]

The CL comment gives us a great description of what you're doing, but doesn't
say why.

[hal.canary, 2013-11-01 13:05:41]

On 2013/11/01 10:12:44, Tom Hudson wrote:
> The CL comment gives us a great description of what you're doing, but doesn't
> say why.

Fixed.

[scroggo, 2013-11-01 15:47:59]

From the issue description:

> SkLazyCachingPixelRef exists for two reasons: to test
> SkCachingPixelRef within Skia and as an example for downstream
> developers to make theor own classes that inherit from
> SkCachingPixelRef.

their*

I'm not convinced that onDecodeInfo should take three parameters instead of one,
and there are a few small things that need fixing, but overall it lgtm.

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/1/src/lazy/SkCachingPixelRef.h#...
src/lazy/SkCachingPixelRef.h:78: SkISize                     fBitmapSize;
On 2013/11/01 03:29:24, halcanary wrote:
> On 2013/10/31 21:53:06, scroggo wrote:
> > Why do you have these three instead of an SkImage::Info?
> 
> I wanted to keep the onDecodeInfo() interface simple.  These are exactly the
> parameters I need to call bitmap.setConfig(...)

Wouldn't

bool onDecodeInfo(SkImage::Info*)

be simpler than

bool onDecodeInfo(SkISize*, SkBitmap::Config*, SkAlphaType*)

?


> 
> And I want this general-purpose interface to be able to produce any valid
> bitmap.

Does using an SkImage::Info mean that it's less general-purpose or it cannot
produce some valid bitmap?

https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:23: *  attempt to re-decoded.
re-decode.

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:118: void
testThreeEncodings(skiatest::Reporter* reporter,
This function should be static.

Also, static functions should have underscore_names.

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:146: void
compareWithSkLazyPixelRef(skiatest::Reporter* reporter,
ditto

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:179: void
compareWithLazyCachedPixelRef(skiatest::Reporter* reporter,
ditto

[hal.canary, 2013-11-01 16:30:59]

On 2013/11/01 15:47:59, scroggo wrote:
> their*

fixed

> I'm not convinced that onDecodeInfo should take three parameters instead of
one,

I claim that since a SkBitmap+SkPixelRef is not a SkImage, we should stick to
SkBitmap::Config instead of SkColorType.  After all, the primary way to ask a
pixelref what its bytes mean is to ask the hosting bitmap.  Mike, what do you
think?

[hal.canary, 2013-11-01 16:31:09]

https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/230001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:23: *  attempt to re-decoded.
On 2013/11/01 15:48:00, scroggo wrote:
> re-decode.

Done.

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:118: void
testThreeEncodings(skiatest::Reporter* reporter,
On 2013/11/01 15:48:00, scroggo wrote:
> This function should be static.
> 
> Also, static functions should have underscore_names.

Done.

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:146: void
compareWithSkLazyPixelRef(skiatest::Reporter* reporter,
On 2013/11/01 15:48:00, scroggo wrote:
> ditto

Done.

https://codereview.chromium.org/54203006/diff/230001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:179: void
compareWithLazyCachedPixelRef(skiatest::Reporter* reporter,
On 2013/11/01 15:48:00, scroggo wrote:
> ditto

Done.

[hal.canary, 2013-11-01 18:08:40]

Mike, let me know if you find the changes to Skbitmap.[h|cpp] acceptible.  You
guys are right that SkImageInfo is a nice clean interface.

[reed1, 2013-11-01 18:22:27]

This is at least two different CLs, one being some changes to how mutexes are
applied to the global cache.

Please separate this out, so we can focus just on the hierarchy changes.

https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:403: size_t SkScaledImageCache::getBytesUsed()
const {
Why is this better than the inlines?

[scroggo, 2013-11-01 18:27:55]

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:68: *  (info->fHeight * fRowBytes) bytes.  Should
follow the format
fRowBytes should be rowBytes, since this class knows nothing of fRowBytes.

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:46: SkImageInfo info = this->getInfo();  //
local copy of info.
Shouldn't this be const SkImageInfo& info, as it is currently written?

I kind of like the model where getInfo() returns a pointer though, so instead of
checking fWidth, down below, we can just compare against NULL.

[hal.canary, 2013-11-01 18:52:59]

https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/54203006/diff/370001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:403: size_t SkScaledImageCache::getBytesUsed()
const {
On 2013/11/01 18:22:27, reed1 wrote:
> Why is this better than the inlines?

I did it because it exactly matches the behavior of the static methods.

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:68: *  (info->fHeight * fRowBytes) bytes.  Should
follow the format
On 2013/11/01 18:27:55, scroggo wrote:
> fRowBytes should be rowBytes, since this class knows nothing of fRowBytes.

Done.

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/370001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:46: SkImageInfo info = this->getInfo();  //
local copy of info.
On 2013/11/01 18:27:55, scroggo wrote:
> Shouldn't this be const SkImageInfo& info, as it is currently written?
 
DecodeProc takes a non-const info, so I have to copy it.  This is a quirk of
that function.

> I kind of like the model where getInfo() returns a pointer though, so instead
of
> checking fWidth, down below, we can just compare against NULL.

That's not a bad idea.

[reed1, 2013-11-01 21:41:10]

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:39: explicit SkCachingPixelRef(SkScaledImageCache*
cache = NULL);
Do we have a use-case in mind for taking the cache explicitly? If not, I suggest
for simplicity we skip this parameter, and only add it in later as needed (esp.
easy since this is a private subclass).

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:35: SkScaledImageCache* cache = NULL);
See other comment/question about the need to specify a cache at the moment.

[scroggo, 2013-11-01 22:30:10]

On 2013/11/01 21:41:10, reed1 wrote:
>
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
> File src/lazy/SkCachingPixelRef.h (right):
> 
>
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
> src/lazy/SkCachingPixelRef.h:39: explicit
SkCachingPixelRef(SkScaledImageCache*
> cache = NULL);
> Do we have a use-case in mind for taking the cache explicitly? If not, I
suggest
> for simplicity we skip this parameter, and only add it in later as needed
(esp.
> easy since this is a private subclass).
> 
>
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkLazyCachingPi...
> File src/lazy/SkLazyCachingPixelRef.h (right):
> 
>
https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkLazyCachingPi...
> src/lazy/SkLazyCachingPixelRef.h:35: SkScaledImageCache* cache = NULL);
> See other comment/question about the need to specify a cache at the moment.

lgtm at patch set 6.

[hal.canary, 2013-11-04 13:38:49]

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/230002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:39: explicit SkCachingPixelRef(SkScaledImageCache*
cache = NULL);
On 2013/11/01 21:41:10, reed1 wrote:
> Do we have a use-case in mind for taking the cache explicitly? If not, I
suggest
> for simplicity we skip this parameter, and only add it in later as needed
(esp.
> easy since this is a private subclass).

I restored the use-case in the unit test.

[hal.canary, 2013-11-04 14:41:20]

Okay, Mike, please take a look.

[reed1, 2013-11-04 14:49:06]

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:29: explicit SkCachingPixelRef();
don't need this word if there is no parameter

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:61: virtual bool onDecode(void* pixels, size_t
rowBytes) = 0;
nit: I think onDecodePixels would be clearer, since we have been using modifiers
after onDecode... for other virtuals.

in a different conversation w/ leon, we discussed the idea of passing in Info as
input to this call, to help debugger so the impl can confirm that the caller
knows what its asking for (config, size).

e.g.

onDecodePixels(const SkImageInfo&, void* pixels, size_t rowBytes)

Such a contract also allows us to have the caller's info specify different
output-configs, which the impl can decide to support or not.

[hal.canary, 2013-11-04 15:25:21]

done

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:29: explicit SkCachingPixelRef();
On 2013/11/04 14:49:06, reed1 wrote:
> don't need this word if there is no parameter

Done.

https://codereview.chromium.org/54203006/diff/690001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:61: virtual bool onDecode(void* pixels, size_t
rowBytes) = 0;
On 2013/11/04 14:49:06, reed1 wrote:
> nit: I think onDecodePixels would be clearer, since we
> have been using modifiers after onDecode... for other
> virtuals.
>
> in a different conversation w/ leon, we discussed the
> idea of passing in Info as input to this call, to help
> debugger so the impl can confirm that the caller knows
> what its asking for (config, size).
>
> e.g.
>
> onDecodePixels(const SkImageInfo&, void* pixels, size_t rowBytes)
>
> Such a contract also allows us to have the caller's
> info specify different output-configs, which the impl
> can decide to support or not.

Done

[reed1, 2013-11-04 15:44:33]

https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:48: const SkImageInfo* getInfo();
hmmm, seems safer as an API to return a copy:

e.g.

SkImageInfo getInfo() const;

or

void getInfo(SkImageInfo*) const;

https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:57: *  (info.fHeight * rowBytes) bytes.
nit: the actual requirement is:

(height - 1) * rowBytes + width * bytesPerPixel

[scroggo, 2013-11-04 16:09:56]

https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/750001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:48: const SkImageInfo* getInfo();
On 2013/11/04 15:44:33, reed1 wrote:
> hmmm, seems safer as an API to return a copy:
> 
> e.g.
> 
> SkImageInfo getInfo() const;
> 
> or
> 
> void getInfo(SkImageInfo*) const;

I encouraged him to return a pointer so he could compare against NULL. We modify
the second one to fit our common pattern:

bool getInfo(SkImageInfo*) const;

[scroggo, 2013-11-04 17:18:02]

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:14: // Please note: a non-NULL cache is not
thread-safe.
Obsolete comment.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:28: *info = fInfo;
Can you assert that info is not NULL?

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:58: info.fWidth,
It looks like these are tabs? If so, please convert to spaces.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:62: *         size).  This a contract also allows
the
This contract*

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:42: return 0 == memcmp(&lhs, &rhs,
sizeof(SkImageInfo));
Is this meaningfully different from what the compiler would have produced?

If so, should it go with SkImageInfo, so it can be used by other callers?

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:45: return !(lhs == rhs);
Is this meaningfully different from what the compiler would have produced?

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:78: *  (info.fHeight * rowBytes) bytes.
comment still references the wrong size.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:82: *         that the caller knows what its
asking for (config,
it's*

[hal.canary, 2013-11-04 18:24:19]

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:14: // Please note: a non-NULL cache is not
thread-safe.
On 2013/11/04 17:18:03, scroggo wrote:
> Obsolete comment.

Done.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:28: *info = fInfo;
On 2013/11/04 17:18:03, scroggo wrote:
> Can you assert that info is not NULL?

Done.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:58: info.fWidth,
On 2013/11/04 17:18:03, scroggo wrote:
> It looks like these are tabs? If so, please convert to spaces.

Done.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.h:62: *         size).  This a contract also allows
the
On 2013/11/04 17:18:03, scroggo wrote:
> This contract*

Done.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:42: return 0 == memcmp(&lhs, &rhs,
sizeof(SkImageInfo));
On 2013/11/04 17:18:03, scroggo wrote:
> Is this meaningfully different from what the compiler would have produced?

My compiler refuses to write its own comparison operator at all.

> If so, should it go with SkImageInfo, so it can be used by other callers?

You're right.  If I don't put it into the header it belongs in, someone else
will have to write the same code somewhere down the line.

Done

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.cpp:45: return !(lhs == rhs);
On 2013/11/04 17:18:03, scroggo wrote:
> Is this meaningfully different from what the compiler would have produced?

Smae thing: my compiler refuses to write its own comparison operator.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
File src/lazy/SkLazyCachingPixelRef.h (right):

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:78: *  (info.fHeight * rowBytes) bytes.
On 2013/11/04 17:18:03, scroggo wrote:
> comment still references the wrong size.

Done.

https://codereview.chromium.org/54203006/diff/820001/src/lazy/SkLazyCachingPi...
src/lazy/SkLazyCachingPixelRef.h:82: *         that the caller knows what its
asking for (config,
On 2013/11/04 17:18:03, scroggo wrote:
> it's*

Done.

[reed1, 2013-11-04 18:34:46]

testing could consider writing a couple of subclasses of CachingPixelRef, that
return errors either on onDecodeInfo or onDecodePixels, just to test our
wrappers and asserts.

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:106: fErrorInDecoding = true;
This could just be

if (!(this->configure(&tmp) && tmp.allocPixels())) {
    return false;
}

No need to touch fErrorInDecoding, since we are just calling other apis, and it
is their job to worry about internal error codes.

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:110: fErrorInDecoding = true;
should you set fInfo.fWidth = -1 here? If not, perhaps we shouldn't assert on
that up in getInfo().

[hal.canary, 2013-11-04 19:41:07]

I added one more test.

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
File src/lazy/SkCachingPixelRef.cpp (right):

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:106: fErrorInDecoding = true;
On 2013/11/04 18:34:47, reed1 wrote:
> This could just be
> 
> if (!(this->configure(&tmp) && tmp.allocPixels())) {
>     return false;
> }
> 
> No need to touch fErrorInDecoding, since we are just calling other apis, and
it
> is their job to worry about internal error codes.

Done.

https://codereview.chromium.org/54203006/diff/680002/src/lazy/SkCachingPixelR...
src/lazy/SkCachingPixelRef.cpp:110: fErrorInDecoding = true;
On 2013/11/04 18:34:47, reed1 wrote:
> should you set fInfo.fWidth = -1 here? If not, perhaps we shouldn't assert on
> that up in getInfo().

Good catch.

[reed1, 2013-11-04 20:08:06]

lgtm

[scroggo, 2013-11-04 23:09:51]

On 2013/11/04 20:08:06, reed1 wrote:
> lgtm

lgtm at patch set 14

[commit-bot: I haz the power, 2013-11-05 17:00:25]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/54203006/730002

[commit-bot: I haz the power, 2013-11-06 10:08:34]

Change committed as 12149