Extend SkImageGenerator to support natively generated GrTextures

Extend SkImageGenerator to support natively generated GrTextures. As part of this, added uniqueID() to the generator, and made it be in the same namespace is bitmaps, pixelrefs, images.

To do this, create SkImageCacherator, which wraps a generator and provides an
interface to get a cached answer for either the raster or texture output of
the generator.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8f34372f7e97482e5e61ab298b7edaa008ba2f4c

[reed1, 2015-08-13 14:34:00]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 14:34:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/20001

[commit-bot: I haz the power, 2015-08-13 14:35:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 14:35:30]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2015-08-13 14:41:16]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 14:41:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/40001

[commit-bot: I haz the power, 2015-08-13 14:42:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 14:42:59]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2015-08-13 18:28:37]

reed@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com, scroggo@google.com

[reed1, 2015-08-13 18:28:38]

ptal -- the cacherator is private, intended to be used by
SkImage-from-generators.

[reed1, 2015-08-13 18:28:42]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 18:28:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/60001

[commit-bot: I haz the power, 2015-08-13 18:35:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 18:35:21]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2015-08-13 19:09:13]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 19:09:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/80001

[commit-bot: I haz the power, 2015-08-13 19:11:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 19:11:15]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[scroggo, 2015-08-13 19:59:13]

> Extend SkImageGenerator to support natively generated GrTextures. As part of
this, added uniqueID() to the generator, and made it be in the same namespace is
bitmaps, pixelrefs, images.

Can you make the first sentence the short description, and move the rest into
the long description?

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
include/core/SkImageGenerator.h:61: uint32_t uniqueID() const { return
fUniqueID; }
If an SkImage or SkBitmap uses this generator, should it share this ID?

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
include/core/SkImageGenerator.h:148: *  native size) it may, so the caller must
inspect the texture's width/height
I lost the thread of this sentence. Maybe it would be clearer to say:

If the context (...) must return a different sized texture (...) to support the
specified usage, it may.

I do not understand how the remainder of the sentence is related. It's also not
clear to me how it relates to the SkImageUsageType (except that kUntiled has
some special meaning relative to the others).

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageInf...
include/core/SkImageInfo.h:23: enum SkImageUsageType {
Maybe this name should incorporate the word "tiling" somehow, since it only
applies to tiling?

It also seems like it only relates to drawing via the GPU? Does it belong here?
Maybe in some GPU specific file? (I guess you want to still be able to reference
it even when SK_SUPPORT_GPU is false?)

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
File src/core/SkImageCacherator.cpp (right):

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:28: delete fGenerator;
SkDELETE?

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:31: static bool check_output_bitmap(const
SkBitmap& bitmap, uint32_t expectedID) {
This always returns true, so maybe it should return void?

Should this be "validate"?

Ah, is this just a helper so you can combine the following into one line?

check_output_bitmap(...);
return true;

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:67: const SkImageInfo& info = this->info();
Maybe move this down closer to where it's used?

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:73: // Try to get a texture and read it back to
raster (and then cache that with our ID)
A read back is potentially expensive, right? Should the header explain that this
function might do this expensive operation?

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:19: SkImageCacherator(SkImageGenerator* gen);
Maybe this should be a factory, so we don't end up getting any calls to

new SkImageCacherator(nullptr)

?

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:29: *  The cached bitmap is valid until it goes out
of scope.
Are you saying the bitmap passed to this function is valid until it goes out of
scope, or until the SkImageCacherator goes out of scope? What happens if the
client passes one on the heap?

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:35: *  when it is done. Will return NULL on
failure.
nullptr?

[bsalomon, 2015-08-13 20:08:29]

https://codereview.chromium.org/1291803002/diff/80001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1291803002/diff/80001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:16: class SkImageCacherator {
comment about what this is?

https://codereview.chromium.org/1291803002/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1291803002/diff/80001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:688: SkImageUsageType usage) {
Can we invert this and make the old function call this variant? Seems like
GrTextureParams should be the convenience method. Or if that causes a lot of
rippling changes, leave a TODO?

[reed1, 2015-08-13 20:08:33]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-13 20:08:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/100001

[reed1, 2015-08-13 20:14:37]

https://codereview.chromium.org/1291803002/diff/80001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1291803002/diff/80001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:16: class SkImageCacherator {
On 2015/08/13 20:08:29, bsalomon wrote:
> comment about what this is?

Done.

https://codereview.chromium.org/1291803002/diff/80001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/1291803002/diff/80001/src/gpu/SkGr.cpp#newcod...
src/gpu/SkGr.cpp:688: SkImageUsageType usage) {
On 2015/08/13 20:08:29, bsalomon wrote:
> Can we invert this and make the old function call this variant? Seems like
> GrTextureParams should be the convenience method. Or if that causes a lot of
> rippling changes, leave a TODO?

TODO

[reed1, 2015-08-13 20:24:43]

ptal

[reed1, 2015-08-13 20:24:49]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-08-13 20:25:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291803002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291803002/120001

[commit-bot: I haz the power, 2015-08-13 20:25:06]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-08-14 02:25 UTC

[bsalomon, 2015-08-13 20:26:39]

lgtm

[commit-bot: I haz the power, 2015-08-13 20:32:44]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/8f34372f7e97482e5e61ab298b7edaa008ba2f4c

[reed1, 2015-08-13 20:57:08]

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
include/core/SkImageGenerator.h:61: uint32_t uniqueID() const { return
fUniqueID; }
On 2015/08/13 19:59:12, scroggo wrote:
> If an SkImage or SkBitmap uses this generator, should it share this ID?

It will -- that is handled in my following CL

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageGen...
include/core/SkImageGenerator.h:148: *  native size) it may, so the caller must
inspect the texture's width/height
On 2015/08/13 19:59:12, scroggo wrote:
> I lost the thread of this sentence. Maybe it would be clearer to say:
> 
> If the context (...) must return a different sized texture (...) to support
the
> specified usage, it may.
> 
> I do not understand how the remainder of the sentence is related. It's also
not
> clear to me how it relates to the SkImageUsageType (except that kUntiled has
> some special meaning relative to the others).

Done.

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageInfo.h
File include/core/SkImageInfo.h (right):

https://codereview.chromium.org/1291803002/diff/60001/include/core/SkImageInf...
include/core/SkImageInfo.h:23: enum SkImageUsageType {
On 2015/08/13 19:59:13, scroggo wrote:
> Maybe this name should incorporate the word "tiling" somehow, since it only
> applies to tiling?
> 
> It also seems like it only relates to drawing via the GPU? Does it belong
here?
> Maybe in some GPU specific file? (I guess you want to still be able to
reference
> it even when SK_SUPPORT_GPU is false?)

It also appears in SkImageGenrerator.h which is public, so I think it has to be
public and not necessarily gpu-specific. I'm fine to change the name later.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
File src/core/SkImageCacherator.cpp (right):

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:28: delete fGenerator;
On 2015/08/13 19:59:13, scroggo wrote:
> SkDELETE?

Done.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:31: static bool check_output_bitmap(const
SkBitmap& bitmap, uint32_t expectedID) {
On 2015/08/13 19:59:13, scroggo wrote:
> This always returns true, so maybe it should return void?
> 
> Should this be "validate"?
> 
> Ah, is this just a helper so you can combine the following into one line?
> 
> check_output_bitmap(...);
> return true;

Acknowledged.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:67: const SkImageInfo& info = this->info();
On 2015/08/13 19:59:13, scroggo wrote:
> Maybe move this down closer to where it's used?

Done.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.cpp:73: // Try to get a texture and read it back to
raster (and then cache that with our ID)
On 2015/08/13 19:59:13, scroggo wrote:
> A read back is potentially expensive, right? Should the header explain that
this
> function might do this expensive operation?

Calling the generator itself can be very expensive. I presume it is implied that
the cacherator will be potentially slow the first time it is accessed, but
faster after that.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
File src/core/SkImageCacherator.h (right):

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:19: SkImageCacherator(SkImageGenerator* gen);
On 2015/08/13 19:59:13, scroggo wrote:
> Maybe this should be a factory, so we don't end up getting any calls to
> 
> new SkImageCacherator(nullptr)
> 
> ?

Done.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:29: *  The cached bitmap is valid until it goes out
of scope.
On 2015/08/13 19:59:13, scroggo wrote:
> Are you saying the bitmap passed to this function is valid until it goes out
of
> scope, or until the SkImageCacherator goes out of scope? What happens if the
> client passes one on the heap?

Fixed.

https://codereview.chromium.org/1291803002/diff/60001/src/core/SkImageCachera...
src/core/SkImageCacherator.h:35: *  when it is done. Will return NULL on
failure.
On 2015/08/13 19:59:13, scroggo wrote:
> nullptr?

Hmmm, I'm not sure that distinction is important in a comment, but I hadn't
really considered it.