add colortable support to imagegenerator

add colortable support to imagegenerator

BUG=skia:

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

[scroggo, 2014-05-27 16:13:58]

https://codereview.chromium.org/304443003/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/304443003/diff/1/gyp/images.gyp#newcode74
gyp/images.gyp:74: '../src/images/SkImageGenerator.cpp',
This file is missing from your CL.

https://codereview.chromium.org/304443003/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/304443003/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:76: bool getInfo(SkImageInfo* info);
So long as we're updating the API - SkBitmap originally had a function
asImageInfo, which behaved similarly to this. It has been deprecated in favor of
SkBitmap::info(), which always succeeds (but can return kUnknown_SkColorType).
Is there any reason not to follow that model? How might this version fail?

[reed1, 2014-05-27 18:05:46]

ptal

https://codereview.chromium.org/304443003/diff/1/gyp/images.gyp
File gyp/images.gyp (right):

https://codereview.chromium.org/304443003/diff/1/gyp/images.gyp#newcode74
gyp/images.gyp:74: '../src/images/SkImageGenerator.cpp',
On 2014/05/27 16:13:58, scroggo wrote:
> This file is missing from your <span id="abbrex-spnAbbr_632"
class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CL</span>.

Done.

https://codereview.chromium.org/304443003/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/304443003/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:76: bool getInfo(SkImageInfo* info);
On 2014/05/27 16:13:58, scroggo wrote:
> So long as we're updating the <span id="abbrex-spnAbbr_655" class="abbrex-Abbr
abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span> -
SkBitmap originally had a function
> asImageInfo, which behaved similarly to this. It has been deprecated in favor
of
> SkBitmap::info(), which always succeeds (but can return kUnknown_SkColorType).
> Is there any reason not to follow that model? How might this version fail?

A generate can fail because it has bad data, or somehow failed to decode the
head of its data to extract the info.

I agree that SkBitmap's version need no longer return a bool.

[scroggo, 2014-05-27 18:31:22]

Is the intent to check in the changes to make DecodingImageGenerator handle
Index8 as a separate CL? I think it would make sense to make it part of the same
CL in order to make sure this satisfies that need.

The changes needed to make DecodingImageGenerator support Index8 are the
following:
- remove the checks for Index8 in CreateDecodingImageGenerator
- SkCachingPixelRef::onNewLockPixels needs to call the new version of getPixels
- SkCachingPixelRef::onNewLockPixels needs to set the color table on the
LockRec.

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
File gyp/skia_for_chromium_defines.gypi (right):

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
gyp/skia_for_chromium_defines.gypi:18: 'SK_SUPPORT_LEGACY_IMAGEGENERATORAPI',
Remember to add this to the script for generate ninja (unless we've gotten it to
also look at this file).

https://codereview.chromium.org/304443003/diff/20001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/20001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:41: // This implementaion of getInfo()
always returns true.
nits:
implementation*
onGetInfo()*

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
File src/images/SkImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
src/images/SkImageGenerator.cpp:49: if (kIndex_8_SkColorType ==
info.colorType()) {
It seems like we typically do not recover after a failed assert. Is there a
reason this case is different? (Or are we just inconsistent/am I wrong?)

[hal.canary, 2014-05-27 18:39:39]

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

https://codereview.chromium.org/304443003/diff/40001/include/core/SkImageGene...
include/core/SkImageGenerator.h:53: virtual bool getInfo(SkImageInfo* info) {
return this->onGetInfo(info); }
Why not leave this around with the caveat that info.fColorType can not be
indexed?

[hal.canary, 2014-05-27 18:40:52]

LGTM

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

https://codereview.chromium.org/304443003/diff/40001/include/core/SkImageGene...
include/core/SkImageGenerator.h:53: virtual bool getInfo(SkImageInfo* info) {
return this->onGetInfo(info); }
On 2014/05/27 18:39:39, Hal Canary wrote:
> Why not leave this around with the caveat that info.fColorType can not be
> indexed?

On further consideration, I think this is best as you did it.

[reed1, 2014-05-27 18:47:34]

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
File gyp/skia_for_chromium_defines.gypi (right):

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
gyp/skia_for_chromium_defines.gypi:18: 'SK_SUPPORT_LEGACY_IMAGEGENERATORAPI',
On 2014/05/27 18:31:23, scroggo wrote:
> Remember to add this to the script for generate ninja (unless we've gotten it
to
> also look at this file).

already works

https://codereview.chromium.org/304443003/diff/20001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/20001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:41: // This implementaion of getInfo()
always returns true.
On 2014/05/27 18:31:23, scroggo wrote:
> nits:
> implementation*
> onGetInfo()*

Done.

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
File src/images/SkImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
src/images/SkImageGenerator.cpp:49: if (kIndex_8_SkColorType ==
info.colorType()) {
On 2014/05/27 18:31:23, scroggo wrote:
> It seems like we typically do not recover after a failed assert. Is there a
> reason this case is different? (Or are we just inconsistent/am I wrong?)

I am of (at least) two minds here:

1. SkASSERT is debug-only, so release builds would not catch this
2. Perhaps this can silently just return false

As written, this is implementing both ideas, which isn't strictly wrong, but is
odd. Do you have a preference?

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

https://codereview.chromium.org/304443003/diff/40001/include/core/SkImageGene...
include/core/SkImageGenerator.h:53: virtual bool getInfo(SkImageInfo* info) {
return this->onGetInfo(info); }
On 2014/05/27 18:39:39, Hal Canary wrote:
> Why not leave this around with the caveat that info.fColorType can not be
> indexed?

I did. See getPixels() below that only takes 3 parameters

[scroggo, 2014-05-27 19:08:06]

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
File gyp/skia_for_chromium_defines.gypi (right):

https://codereview.chromium.org/304443003/diff/20001/gyp/skia_for_chromium_de...
gyp/skia_for_chromium_defines.gypi:18: 'SK_SUPPORT_LEGACY_IMAGEGENERATORAPI',
On 2014/05/27 18:47:34, reed1 wrote:
> On 2014/05/27 18:31:23, scroggo wrote:
> > Remember to add this to the script for generate ninja (unless we've gotten
it
> to
> > also look at this file).
> 
> already works

Yay! nvm then

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
File src/images/SkImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/20001/src/images/SkImageGenera...
src/images/SkImageGenerator.cpp:49: if (kIndex_8_SkColorType ==
info.colorType()) {
On 2014/05/27 18:47:34, reed1 wrote:
> On 2014/05/27 18:31:23, scroggo wrote:
> > It seems like we typically do not recover after a failed assert. Is there a
> > reason this case is different? (Or are we just inconsistent/am I wrong?)
> 
> I am of (at least) two minds here:
> 
> 1. SkASSERT is debug-only, so release builds would not catch this

I think that's ok. If it ends up being a problem we can figure out why in debug.

> 2. Perhaps this can silently just return false

In that case I'm not sure how someone is supposed to discover they made a
mistake.

> 
> As written, this is implementing both ideas, which isn't strictly wrong, but
is
> odd. Do you have a preference?

I think it's fine as is. I just thought our general idealogy was to skip the
conditional branch in release and just use an assert in debug to catch an error.

[reed1, 2014-05-28 13:55:58]

ptal

[scroggo, 2014-05-28 15:01:37]

https://codereview.chromium.org/304443003/diff/90001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/90001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:225: // We do not support indexed color
with SkImageGenerators,
This block can be removed.

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

https://codereview.chromium.org/304443003/diff/90001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:81: rec->fColorTable = fCTable.get();
This version does not purge the color table on memory pressure, but maybe that's
okay (it's pretty small, after all).

One way to purge would be to request a larger block of discardable memory and
allocate the color table there. The downside is that someone could take a
reference to it (weren't you interested in removing its inheritance from
SkRefCnt?), and then we might delete/overwrite it underneath them.

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

https://codereview.chromium.org/304443003/diff/90001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:21: *  Since SkColorTable is reference-counted,
we do not support indexed
This statement is no longer true.

[reed1, 2014-05-28 15:48:01]

https://codereview.chromium.org/304443003/diff/90001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/304443003/diff/90001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:225: // We do not support indexed color
with SkImageGenerators,
On 2014/05/28 15:01:37, scroggo wrote:
> This block can be removed.

Good catch. Done.

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

https://codereview.chromium.org/304443003/diff/90001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:81: rec->fColorTable = fCTable.get();
On 2014/05/28 15:01:37, scroggo wrote:
> This version does not purge the color table on memory pressure, but maybe
that's
> okay (it's pretty small, after all).
> 
> One way to purge would be to request a larger block of discardable memory and
> allocate the color table there. The downside is that someone could take a
> reference to it (weren't you interested in removing its inheritance from
> SkRefCnt?), and then we might delete/overwrite it underneath them.

Agreed, I don't know how to ram the colortable into the discardable block yet,
so that part is not "discardable". I'll add a comment acknowledging this.

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

https://codereview.chromium.org/304443003/diff/90001/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.h:21: *  Since SkColorTable is reference-counted,
we do not support indexed
On 2014/05/28 15:01:37, scroggo wrote:
> This statement is no longer true.

Done.

[scroggo, 2014-05-28 15:53:38]

lgtm

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

https://codereview.chromium.org/304443003/diff/60008/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:77: // could move it into the block, but then
again perhaps it is small enought that this doesn't
enough*

[reed1, 2014-05-28 15:56:15]

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

https://codereview.chromium.org/304443003/diff/60008/src/lazy/SkDiscardablePi...
src/lazy/SkDiscardablePixelRef.cpp:77: // could move it into the block, but then
again perhaps it is small enought that this doesn't
On 2014/05/28 15:53:38, scroggo wrote:
> enough*

Done.

[reed1, 2014-05-28 15:56:20]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-28 15:56:41]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/304443003/110001

[commit-bot: I haz the power, 2014-05-28 16:02:01]

Change committed as 14916

[reed1, 2014-05-28 16:15:42]

A revert of this CL has been created in
https://codereview.chromium.org/300873007/ by reed@google.com.

The reason for reverting is: failing tests.