Add SkImageGenerator Interface

Add SkImageGenerator Interface

-   Add SkDiscardablePixelRef class that uses SkDiscardableMemory and
    a SkImageGenerator.

-   Add SkDecodingImageGenerator class as an example of a
    SkImageGenerator.

-   Add DecodingImageGenerator unit test.

-   Add SkBasicDiscardableMemory implmentation for unit tests only.

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

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

[reed1, 2013-11-18 21:56:10]

likely SkImageGenerate.h will need to be public.

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h
File src/images/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h...
src/images/SkImageGenerator.h:15: #include "SkBitmap.h"
do you need to include SkData.h?

can we just include SkImageInfo.h?

[Alpha Left Google, 2013-11-19 02:00:48]

https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
File src/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:54: virtual bool getPixels(const
SkImageInfo& info,
There should be a frame index here for animated images.

PixelRef should now contains a reference to SkDecodingImageGenerator and a frame
index.

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h
File src/images/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h...
src/images/SkImageGenerator.h:55: virtual bool getPixels(const SkImageInfo&
info,
There should be a frame index here for animated images.

[hal.canary, 2013-11-19 02:06:28]

https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
File src/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:54: virtual bool getPixels(const
SkImageInfo& info,
On 2013/11/19 02:00:48, Alpha wrote:
> There should be a frame index here for animated images.
> 
> PixelRef should now contains a reference to SkDecodingImageGenerator and a
frame
> index.

Alpha, you should make a subclass of SkImageGenerator in Blink which does what
you want.

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h
File src/images/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h...
src/images/SkImageGenerator.h:15: #include "SkBitmap.h"
On 2013/11/18 21:56:10, reed1 wrote:
> do you need to include SkData.h?
> 
> can we just include SkImageInfo.h?

Done.

[Alpha Left Google, 2013-11-19 02:12:22]

On 2013/11/19 02:06:28, Hal Canary wrote:
>
https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
> File src/images/SkDecodingImageGenerator.h (right):
> 
>
https://codereview.chromium.org/74793011/diff/1/src/images/SkDecodingImageGen...
> src/images/SkDecodingImageGenerator.h:54: virtual bool getPixels(const
> SkImageInfo& info,
> On 2013/11/19 02:00:48, Alpha wrote:
> > There should be a frame index here for animated images.
> > 
> > PixelRef should now contains a reference to SkDecodingImageGenerator and a
> frame
> > index.
> 
> Alpha, you should make a subclass of SkImageGenerator in Blink which does what
> you want.
> 
> https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h
> File src/images/SkImageGenerator.h (right):
> 
>
https://codereview.chromium.org/74793011/diff/1/src/images/SkImageGenerator.h...
> src/images/SkImageGenerator.h:15: #include "SkBitmap.h"
> On 2013/11/18 21:56:10, reed1 wrote:
> > do you need to include SkData.h?
> > 
> > can we just include SkImageInfo.h?
> 
> Done.

I see. SkDecodingImageGenerator.h is for using Skia's internal decoders. While
Blink's implementation should be inheriting from SkImageGenerator.h.

Can you show me how should a SkImageGenerator.h be created and associated with a
SkPixelRef?

[hal.canary, 2013-11-19 02:45:37]

On 2013/11/19 02:12:22, Alpha wrote:
> I see. SkDecodingImageGenerator.h is for using Skia's internal decoders. While
> Blink's implementation should be inheriting from SkImageGenerator.h.
> 
> Can you show me how should a SkImageGenerator.h be created and associated with
a
> SkPixelRef?

I'll try to get you a basic example tomorrow morning.

[reed1, 2013-11-19 15:51:53]

https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:29: ~SkDiscardablePixelRef();
this can be protected (or private I think)

[hal.canary, 2013-11-19 18:23:14]

On 2013/11/19 02:12:22, Alpha wrote:
> Can you show me how should a SkImageGenerator.h be created and associated with
a
> SkPixelRef?

See https://codereview.chromium.org/59823010 for some place to start

[reed1, 2013-11-19 18:36:48]

I think the plan would be that, once you have a blink_subclass of
SkImageGenerator, you would just call our Install factory, and never have to
create an actual SkPixelRef subclass on the blink side!

[hal.canary, 2013-11-19 21:38:19]

Okay, please take a look.

https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:29: ~SkDiscardablePixelRef();
On 2013/11/19 15:51:54, reed1 wrote:
> this can be protected (or private I think)

Done.

[Alpha Left Google, 2013-11-19 21:57:27]

On 2013/11/19 21:38:19, Hal Canary wrote:
> Okay, please take a look.
> 
>
https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
> File src/lazy/SkDiscardablePixelRef.h (right):
> 
>
https://codereview.chromium.org/74793011/diff/110001/src/lazy/SkDiscardablePi...
> src/lazy/SkDiscardablePixelRef.h:29: ~SkDiscardablePixelRef();
> On 2013/11/19 15:51:54, reed1 wrote:
> > this can be protected (or private I think)
> 
> Done.

Okay. Judging from the change of DecodingImageGenerator it will be able to
replace LazyDecodingPixelRef. And a pixel ref will just be a simple pixel ref
without knowing decoding. So I think this change and interfaces look good. We
can work on merging ImageFrameGenerator and DecodingImageGenerator in a later
change.

[reed1, 2013-11-19 22:01:17]

some dox comments, but lgtm enough to land for me.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
File src/core/SkBasicDiscardableMemory.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:16: class SkBasicDiscardableMemory : public
SkDiscardableMemory {
bikeshed: SkMallocDiscardableMemory?

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:24: * obtain a pointer to that memory using
the data() method.
Not sure we should repeat the dox from the base-class. Perhaps better would be
dox about your subclass/impl if you want.

[scroggo, 2013-11-19 22:19:26]

https://codereview.chromium.org/74793011/diff/210001/gyp/ports.gyp
File gyp/ports.gyp (right):

https://codereview.chromium.org/74793011/diff/210001/gyp/ports.gyp#newcode40
gyp/ports.gyp:40: '../src/core/SkBasicDiscardableMemory.cpp',
If this is only used for tests, why is it in core? Can we move it somewhere
else? Perhaps in ports?

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
include/core/SkImageGenerator.h:32: *  Return some information about the image,
allowing the owner fo
of*

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
include/core/SkImageGenerator.h:33: *  this ojbect to allocate pixels.
object*

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
File src/core/SkBasicDiscardableMemory.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.cpp:37: return fPointer;
Should this return NULL if not locked?

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
File src/core/SkBasicDiscardableMemory.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:16: class SkBasicDiscardableMemory : public
SkDiscardableMemory {
It seems dishonest to call this SkBasicDiscardableMemory, when really it's more
something like SkMockDiscardableMemory.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:23: * Locks the memory, prevent it from
being discarded. Once locked. you may
Comments don't apply.

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:35: *  Return some information about the
image, allowing the owner fo
of*

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:36: *  this ojbect to allocate pixels.
object*

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:50: *         This contract also allows
the caller to specify
These two sentences seem to say opposite things. The first says info should be
the same as the answer from getInfo, and the second seems to imply that info can
be different, and the implementation chooses.

Same comment applies to SkImageGenerator.h

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:58: return false;
Who should be responsible for deleting generator if this fails? Right now it
gets leaked. Delete it and update the comments to state who deletes it.

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:44: SkImageInfo fInfo;
Can fInfo, fSize, and fRowBytes be const?

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:45: size_t fSize;  // size of memeory to be
allocated
memory*

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:46: size_t fRowBytes;  // ???
???

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
src/ports/SkDiscardableMemory_none.cpp:11: return
SkBasicDiscardableMemory::Create(bytes);
If we're going to do this, can we at least put a printf that the actual memory
is not discardable?

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

https://codereview.chromium.org/74793011/diff/210001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:256: REPORTER_ASSERT(reporter, success);
If success is false, should we return false?

[hal.canary, 2013-11-20 00:07:09]

https://codereview.chromium.org/74793011/diff/210001/gyp/ports.gyp
File gyp/ports.gyp (right):

https://codereview.chromium.org/74793011/diff/210001/gyp/ports.gyp#newcode40
gyp/ports.gyp:40: '../src/core/SkBasicDiscardableMemory.cpp',
On 2013/11/19 22:19:27, scroggo wrote:
> If this is only used for tests, why is it in core? Can we move it somewhere
> else? Perhaps in ports?

I moved it all into a anonymous namespace inside SkDiscardableMemory_none.cpp

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
include/core/SkImageGenerator.h:32: *  Return some information about the image,
allowing the owner fo
On 2013/11/19 22:19:27, scroggo wrote:
> of*

Done.

https://codereview.chromium.org/74793011/diff/210001/include/core/SkImageGene...
include/core/SkImageGenerator.h:33: *  this ojbect to allocate pixels.
On 2013/11/19 22:19:27, scroggo wrote:
> object*

Done.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
File src/core/SkBasicDiscardableMemory.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.cpp:37: return fPointer;
On 2013/11/19 22:19:27, scroggo wrote:
> Should this return NULL if not locked?

Should we allow the caller to make that mistake?  In Release, I'll put the extra
check in place and return NULL.

Done.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
File src/core/SkBasicDiscardableMemory.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:16: class SkBasicDiscardableMemory : public
SkDiscardableMemory {
On 2013/11/19 22:19:27, scroggo wrote:
> It seems dishonest to call this SkBasicDiscardableMemory, when really it's
more
> something like SkMockDiscardableMemory.

Done.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:23: * Locks the memory, prevent it from
being discarded. Once locked. you may
On 2013/11/19 22:19:27, scroggo wrote:
> Comments don't apply.

Done.

https://codereview.chromium.org/74793011/diff/210001/src/core/SkBasicDiscarda...
src/core/SkBasicDiscardableMemory.h:24: * obtain a pointer to that memory using
the data() method.
On 2013/11/19 22:01:18, reed1 wrote:
> Not sure we should repeat the dox from the base-class. Perhaps better would be
> dox about your subclass/impl if you want.

Done.

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:35: *  Return some information about the
image, allowing the owner fo
On 2013/11/19 22:19:27, scroggo wrote:
> of*

I'll remove the sub-class's redundant comments.

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:36: *  this ojbect to allocate pixels.
On 2013/11/19 22:19:27, scroggo wrote:
> object*

I'll remove the sub-class's redundant comments.

https://codereview.chromium.org/74793011/diff/210001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:50: *         This contract also allows
the caller to specify
On 2013/11/19 22:19:27, scroggo wrote:
> These two sentences seem to say opposite things. The first says info should be
> the same as the answer from getInfo, and the second seems to imply that info
can
> be different, and the implementation chooses.
> 
> Same comment applies to SkImageGenerator.h

It is up to the implmentation.

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:58: return false;
On 2013/11/19 22:19:27, scroggo wrote:
> Who should be responsible for deleting generator if this fails? Right now it
> gets leaked. Delete it and update the comments to state who deletes it.

changed, and commented

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
File src/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:44: SkImageInfo fInfo;
On 2013/11/19 22:19:27, scroggo wrote:
> Can fInfo, fSize, and fRowBytes be const?

Done.

Even the pointer can be const (but not the pointed)

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:45: size_t fSize;  // size of memeory to be
allocated
On 2013/11/19 22:19:27, scroggo wrote:
> memory*

Done.

https://codereview.chromium.org/74793011/diff/210001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:46: size_t fRowBytes;  // ???
On 2013/11/19 22:19:27, scroggo wrote:
> ???

Done.

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
src/ports/SkDiscardableMemory_none.cpp:11: return
SkBasicDiscardableMemory::Create(bytes);
On 2013/11/19 22:19:27, scroggo wrote:
> If we're going to do this, can we at least put a printf that the actual memory
> is not discardable?

I don't like a lot of verbosity in my unit tests (which is where this will be
used).

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

https://codereview.chromium.org/74793011/diff/210001/tests/CachedDecodingPixe...
tests/CachedDecodingPixelRefTest.cpp:256: REPORTER_ASSERT(reporter, success);
On 2013/11/19 22:19:27, scroggo wrote:
> If success is false, should we return false?

We should return early, at least.

[scroggo, 2013-11-20 14:21:53]

Aside from a couple of comments, this lgtm

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
File src/ports/SkDiscardableMemory_none.cpp (right):

https://codereview.chromium.org/74793011/diff/210001/src/ports/SkDiscardableM...
src/ports/SkDiscardableMemory_none.cpp:11: return
SkBasicDiscardableMemory::Create(bytes);
On 2013/11/20 00:07:10, Hal Canary wrote:
> On 2013/11/19 22:19:27, scroggo wrote:
> > If we're going to do this, can we at least put a printf that the actual
memory
> > is not discardable?
> 
> I don't like a lot of verbosity in my unit tests (which is where this will be
> used).

Fair enough. Here's my concern though. Someone can call
SkDiscardableMemory::Create(), expecting to get some memory that may be purged
under memory pressure. Instead, they get something that never purges. That said,
maybe this is fine, since the header is in src, and it makes no promises that it
will ever be purged. I think Chrome doesn't build ports.gyp, so they shouldn't
run into this problem.

https://codereview.chromium.org/74793011/diff/320001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/74793011/diff/320001/include/core/SkImageGene...
include/core/SkImageGenerator.h:16: *  An interface that allows a
SkDiscardablePixelRef to decode and
Since this is an interface that does not necessarily use an
SkDiscardablePixelRef, I think "purgeable PixelRef" is better.

https://codereview.chromium.org/74793011/diff/320001/include/core/SkImageGene...
include/core/SkImageGenerator.h:49: *  @param info Should be identical to the
info returned by
I still think this line should be changed, since the interface does not require
info to be identical to the result of getInfo, and an implementation might allow
an info that is not identical.

[hal.canary, 2013-11-21 15:32:17]

Committed patchset #7 manually as r12341 (presubmit successful).