Big Cleanup: SkBitmapFactory, SkLazyPixelRef, SkImageCache

Big Cleanup: SkBitmapFactory, SkLazyPixelRef, SkImageCache

Removed SkBitmapFactory since no clients were using it.  New cache
selection mechanism can simply pass a SkDiscardableMemory::Factory
into the SkDiscardablePixelRef if non-default SkDiscardableMemory
should be used.  Removed BitmapFactoryTest.

SkDiscardableMemory::Factory interface.  Android will need this
functionality in the future inside their BitmapFactory.

Removed SkLazyPixelRef, since it's functionality is now subsumed into
SkDiscardablePixelRef.  Removed LazyPixelRef test.

Modified SkDiscardablePixelRef to optionally allow it to use a
SkDiscardableMemory::Factory.  This tiny change makes it a replacement
for SkLazyPixelRef.  This functioanlity is also necessary for moving
Android over to SkDiscardablePixelRef from SkImageRef in a later CL.
Added a test for this.

SkDecodingImageGenerator::Install can optionally pass a factory in to
SkDiscardablePixelRef.

Removed SkImageCache, SkLruImageCache, and SkPurgeableImageCache.
This functionality can be handled much more cleanly by
SkDiscardableMemory.

New SkDiscardableMemoryPool class to replace SkLruImageCache.  In a
later CL, we will replace SkImageRef_GlobalPool (used by android) as
well.  This is a concrete implementation of
SkDiscardableMemory::Factory.  Added a test for this.

modified gm/factory.cpp to remove dependnce on SkBitmapFactory +
SkLruImageCache.  Now uses SkDecodingImageGenerator +
SkDiscardablePixelRef + SkDiscardableMemoryPool.

SkImageDecoder::Target replaces SkBitmapFactory::Target.  The
DecodeMemoryToTarget function may disappear in the future.

Moved SkLazyCachingPixelRef::DecodeProc replaces
SkBitmapFactory::DecodeProc.  This is a short term change, since
another CL changes SkLazyCachingPixelRef to use SkImageGenerator
instead of DecodeProc.

Modified DrawBitmapRectTest to use SkDiscardablePixelRef instead of
SkLazyPixelRef.

tools/LazyDecodeBitmap.cpp now uses SkDecodingImageGenerator +
SkDiscardablePixelRef instead of a SkBitmapFactory.

bench_pictures uses the Global SkDiscardableMemoryPool instead of a
global gLruImageCache.

R=reed@google.com, scroggo@google.com

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

[hal.canary, 2013-12-03 22:10:32]

32 files changed
  429 insertions
 1616 deletions

[scroggo, 2013-12-03 23:00:01]

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp
File gm/factory.cpp (right):

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp#newcode41
gm/factory.cpp:41: SkASSERT(NULL == data.get());
How is data NULL here? Did you test with the resource path set?

https://codereview.chromium.org/103033002/diff/1/include/images/SkDecodingIma...
File include/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/103033002/diff/1/include/images/SkDecodingIma...
include/images/SkDecodingImageGenerator.h:42: SkDiscardableMemory::Factory*
factory = NULL);
Can you add a comment about how factory is used?

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
File include/lazy/SkDiscardableMemoryPool.h (right):

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:23: *  This non-global pool can be used
for unit tests to verify that the
If this is just for unit tests, does it need to be in include?

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:47: void free(SkPoolDiscardableMemory*
dm);
I think I follow all your functions, but comments would be nice.

One slightly confusing aspect that would be nice to document here in the header
is that free is called by SkPoolDiscardableMemory's destructor to free the
SkPoolDiscardableMemory's memory.

Also, the fact that these functions are only called by SkPoolDiscardableMemory.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:50: typedef SkNoncopyable INHERITED;
SkDiscardableMemory::Factory should be set to INHERITED.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:54: *  Used for calling getRAMUsed(),
setRAMBudget() or dumpPool(), or to
I would say that this returns (and possibly creates) a threadsafe global
SkDiscardableMemoryPool.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:60: *  Motivation: by combining
SkCreateGlobalPoolDiscardableMemory and
Motivation for what? Is this the motivation for the whole CL? If so, I think
this belongs in the change description, rather than here.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:63: *  SkDiscardablePixelRef as a
SkLazyPixelRef within the Skia tools.
SkLazyPixelRef is removed, correct?

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardablePi...
File include/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardablePi...
include/lazy/SkDiscardablePixelRef.h:38: SkDiscardableMemory::Factory* factory =
NULL);
Can you add a comment about how the factory is used?

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
File src/lazy/SkDiscardableMemoryPool.cpp (right):

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
src/lazy/SkDiscardableMemoryPool.cpp:96: if ((!cur->fLocked) && (cur->fPointer
!= NULL)) {
Will curr->fPointer ever be NULL? I would think not, since we remove them from
fList after setting the pointer to NULL.

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
src/lazy/SkDiscardableMemoryPool.cpp:99: sk_free(dm->fPointer);
Should this function call this->free(dm)?

https://codereview.chromium.org/103033002/diff/1/tools/LazyDecodeBitmap.cpp
File tools/LazyDecodeBitmap.cpp (left):

https://codereview.chromium.org/103033002/diff/1/tools/LazyDecodeBitmap.cpp#o...
tools/LazyDecodeBitmap.cpp:21: DEFINE_bool(useVolatileCache, false, "Use a
volatile cache for deferred image decoding pixels. "
You'll need to modify buildbot/slave/skia_slave_scripts/render_pictures.py to
remove the flag --useVolatileCache.

[hal.canary, 2013-12-04 00:01:36]

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp
File gm/factory.cpp (right):

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp#newcode41
gm/factory.cpp:41: SkASSERT(NULL == data.get());
On 2013/12/03 23:00:01, scroggo wrote:
> How is data NULL here? Did you test with the resource path set?

I meant to write !=, and I haven't gotten around to running gm.  I'll be sure to
test this code path.

https://codereview.chromium.org/103033002/diff/1/include/images/SkDecodingIma...
File include/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/103033002/diff/1/include/images/SkDecodingIma...
include/images/SkDecodingImageGenerator.h:42: SkDiscardableMemory::Factory*
factory = NULL);
On 2013/12/03 23:00:01, scroggo wrote:
> Can you add a comment about how factory is used?

Done.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
File include/lazy/SkDiscardableMemoryPool.h (right):

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:23: *  This non-global pool can be used
for unit tests to verify that the
On 2013/12/03 23:00:01, scroggo wrote:
> If this is just for unit tests, does it need to be in include?

Done.

Mike suggested that too.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:47: void free(SkPoolDiscardableMemory*
dm);
On 2013/12/03 23:00:01, scroggo wrote:
> I think I follow all your functions, but comments would be nice.
> 
> One slightly confusing aspect that would be nice to document here in the
header
> is that free is called by SkPoolDiscardableMemory's destructor to free the
> SkPoolDiscardableMemory's memory.
> 
> Also, the fact that these functions are only called by
SkPoolDiscardableMemory.

Done.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:50: typedef SkNoncopyable INHERITED;
On 2013/12/03 23:00:01, scroggo wrote:
> SkDiscardableMemory::Factory should be set to INHERITED.

Done.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:54: *  Used for calling getRAMUsed(),
setRAMBudget() or dumpPool(), or to
On 2013/12/03 23:00:01, scroggo wrote:
> I would say that this returns (and possibly creates) a threadsafe global
> SkDiscardableMemoryPool.

Done.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:60: *  Motivation: by combining
SkCreateGlobalPoolDiscardableMemory and
On 2013/12/03 23:00:01, scroggo wrote:
> Motivation for what? Is this the motivation for the whole CL? If so, I think
> this belongs in the change description, rather than here.

Done.  Those comments were meant for myself; I meant to move that text into the
CL.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardableMe...
include/lazy/SkDiscardableMemoryPool.h:63: *  SkDiscardablePixelRef as a
SkLazyPixelRef within the Skia tools.
On 2013/12/03 23:00:01, scroggo wrote:
> SkLazyPixelRef is removed, correct?

yes.  I meant to say that I wanted to use  DiscardablePixelRefs wherever
LazyPixelRefs are used inside the skia tools and tests.

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardablePi...
File include/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/103033002/diff/1/include/lazy/SkDiscardablePi...
include/lazy/SkDiscardablePixelRef.h:38: SkDiscardableMemory::Factory* factory =
NULL);
On 2013/12/03 23:00:01, scroggo wrote:
> Can you add a comment about how the factory is used?

Done.

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
File src/lazy/SkDiscardableMemoryPool.cpp (right):

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
src/lazy/SkDiscardableMemoryPool.cpp:96: if ((!cur->fLocked) && (cur->fPointer
!= NULL)) {
On 2013/12/03 23:00:01, scroggo wrote:
> Will curr->fPointer ever be NULL? I would think not, since we remove them from
> fList after setting the pointer to NULL.

Done.

https://codereview.chromium.org/103033002/diff/1/src/lazy/SkDiscardableMemory...
src/lazy/SkDiscardableMemoryPool.cpp:99: sk_free(dm->fPointer);
On 2013/12/03 23:00:01, scroggo wrote:
> Should this function call this->free(dm)?

No.  We have the mutex, so we can't directly call that function.  And I didn't
want to add yet another function.

https://codereview.chromium.org/103033002/diff/1/tools/LazyDecodeBitmap.cpp
File tools/LazyDecodeBitmap.cpp (left):

https://codereview.chromium.org/103033002/diff/1/tools/LazyDecodeBitmap.cpp#o...
tools/LazyDecodeBitmap.cpp:21: DEFINE_bool(useVolatileCache, false, "Use a
volatile cache for deferred image decoding pixels. "
On 2013/12/03 23:00:01, scroggo wrote:
> You'll need to modify buildbot/slave/skia_slave_scripts/render_pictures.py to
> remove the flag --useVolatileCache.

Okay.

We may want to keep useVolatileCache for Android testing when my ashmem-backed
DM lands.

[hal.canary, 2013-12-04 14:30:32]

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp
File gm/factory.cpp (right):

https://codereview.chromium.org/103033002/diff/1/gm/factory.cpp#newcode41
gm/factory.cpp:41: SkASSERT(NULL == data.get());
On 2013/12/04 00:01:36, Hal Canary wrote:
> On 2013/12/03 23:00:01, scroggo wrote:
> > How is data NULL here? Did you test with the resource path set?
> 
> I meant to write !=, and I haven't gotten around to running gm.  I'll be sure
to
> test this code path. 

Tested with `out/Debug/gm --match factory --resourcePath resources` and verified
that this function gets executed.

[hal.canary, 2013-12-04 18:35:00]

please take a look.

[scroggo, 2013-12-04 19:09:24]

lgtm.

I have some suggestions for making the test more readable, but the code looks
good.

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

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:241: *  if x == 0, will fail at getPixels()
This seems like a good candidate for an enum.

Also, the comments aren't quite right. If 0 == x, it will fail getInfo.

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:270: if ((pixels == NULL)
For your == checks (here and above), NULL or 0 should go on the left.

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:314: REPORTER_ASSERT(reporter, success ==
(0 != x));  // since x==2
Does this comment go with the next line? I think the next line would be clearer
if x was an enum, and you did a comparison like

if (kSuccessfulImageGenerator == x)

That doesn't have to be the exact name, but something conveying that information
would make this test easier to read.

The current line would also read better using enums. I'm also not convinced the
speed increase of comparing the boolean to a comparison is worth the sacrifice
in readability.

[hal.canary, 2013-12-04 19:39:40]

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

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:241: *  if x == 0, will fail at getPixels()
On 2013/12/04 19:09:25, scroggo wrote:
> This seems like a good candidate for an enum.
> 
> Also, the comments aren't quite right. If 0 == x, it will fail getInfo.

Done.

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:270: if ((pixels == NULL)
On 2013/12/04 19:09:25, scroggo wrote:
> For your == checks (here and above), NULL or 0 should go on the left.

Done.

https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:314: REPORTER_ASSERT(reporter, success ==
(0 != x));  // since x==2
On 2013/12/04 19:09:25, scroggo wrote:
> Does this comment go with the next line? I think the next line would be
clearer
> if x was an enum, and you did a comparison like
> 
> if (kSuccessfulImageGenerator == x)
> 
> That doesn't have to be the exact name, but something conveying that
information
> would make this test easier to read.
> 
> The current line would also read better using enums. I'm also not convinced
the
> speed increase of comparing the boolean to a comparison is worth the sacrifice
> in readability.

Done.

[scroggo, 2013-12-04 20:01:25]

On 2013/12/04 19:39:40, Hal Canary wrote:
>
https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
> File tests/CachedDecodingPixelRefTest.cpp (right):
> 
>
https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
> tests/CachedDecodingPixelRefTest.cpp:241: *  if x == 0, will fail at
getPixels()
> On 2013/12/04 19:09:25, scroggo wrote:
> > This seems like a good candidate for an enum.
> > 
> > Also, the comments aren't quite right. If 0 == x, it will fail getInfo.
> 
> Done.
> 
>
https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
> tests/CachedDecodingPixelRefTest.cpp:270: if ((pixels == NULL)
> On 2013/12/04 19:09:25, scroggo wrote:
> > For your == checks (here and above), NULL or 0 should go on the left.
> 
> Done.
> 
>
https://codereview.chromium.org/103033002/diff/60001/tests/CachedDecodingPixe...
> tests/CachedDecodingPixelRefTest.cpp:314: REPORTER_ASSERT(reporter, success ==
> (0 != x));  // since x==2
> On 2013/12/04 19:09:25, scroggo wrote:
> > Does this comment go with the next line? I think the next line would be
> clearer
> > if x was an enum, and you did a comparison like
> > 
> > if (kSuccessfulImageGenerator == x)
> > 
> > That doesn't have to be the exact name, but something conveying that
> information
> > would make this test easier to read.
> > 
> > The current line would also read better using enums. I'm also not convinced
> the
> > speed increase of comparing the boolean to a comparison is worth the
sacrifice
> > in readability.
> 
> Done.

lgtm

[reed1, 2013-12-04 20:09:47]

lgtm w/ comment about unneeded empty virtual destructor.

https://codereview.chromium.org/103033002/diff/80001/src/core/SkDiscardableMe...
File src/core/SkDiscardableMemory.h (right):

https://codereview.chromium.org/103033002/diff/80001/src/core/SkDiscardableMe...
src/core/SkDiscardableMemory.h:34: virtual ~Factory() { }
Unneeded, since SkRefCnt has a virtual destructor (and must, by definition).

[hal.canary, 2013-12-05 18:31:54]

Committed patchset #11 manually as r12515 (presubmit successful).