Change SkDecodingImageGenerator API

Add Options to SkDecodingImageGenerator, simplify API.

Motivation: We want to remove redundant classes from Skia.  To
that end we want to remove SkImageRef and its subclasses and
replace their uses with SkDiscardablePixelRef +
SkDecodingImageGenerator.  Since Android uses SkImageRef, we need
to make sure that SkDecodingImageGenerator allows all of the
settings that Android exposes in BitmapFactory.Options.

To that end, we have created an Options struct for the
SkDecodingImageGenerator which lets the client of the generator set
sample size, dithering, and bitmap config.

We have made the SkDecodingImageGenerator constructor private
and replaced the SkDecodingImageGenerator::Install functions
with a SkDecodingImageGenerator::Create functions (one for
SkData and one for SkStream) which now take a
SkDecodingImageGenerator::Options struct.

Also added a ImageDecoderOptions test which loops through a list
of sets of options and tries them on a set of 5 small encoded
images.

Also updated several users of SkDecodingImageGenerator::Install to
follow new call signature - gm/factory.cpp, LazyDecodeBitmap.cpp,
and PictureTest.cpp, CachedDecodingPixelRefTest.cpp.

We also added a new ImprovedBitmapFactory Test which simulates the
exact function that Android will need to modify to use this,
installPixelRef() in BitmapFactory.

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

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

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

[hal.canary, 2013-12-12 18:50:09]

One more step on the way to destroying SkImageRef

[scroggo, 2013-12-12 19:02:00]

On 2013/12/12 18:50:09, Hal Canary wrote:
> One more step on the way to destroying SkImageRef

Yes, but at what cost? I don't think anyone likes the Options struct, and I
don't feel great about it leaking into our API.

[scroggo, 2013-12-12 19:21:00]

On 2013/12/12 19:02:00, scroggo wrote:
> On 2013/12/12 18:50:09, Hal Canary wrote:
> > One more step on the way to destroying SkImageRef
> 
> Yes, but at what cost? I don't think anyone likes the Options struct, and I
> don't feel great about it leaking into our API.

In order to introduce no regressions into Android, it appears that the only
fields they actually respect for SkImageRef are sampleSize and config. Perhaps
we should only try to add that behavior?

I also think we should push forward to SkImageInfo rather than relying on
config.

[hal.canary, 2013-12-12 20:41:53]

On 2013/12/12 19:21:00, scroggo wrote:
> In order to introduce no regressions into Android, it appears that the only
> fields they actually respect for SkImageRef are sampleSize and config. Perhaps
> we should only try to add that behavior?

I'll remove fPreferQualityOverSpeed since that is missing from Android.  Dither
is there in SkImageRef::setDitherImage.

I want all of the options to be passed into the constructor, so that we can
treat them as constants.

> I also think we should push forward to SkImageInfo rather than relying on
> config.

I could replace SkBitmap::Config with SkColorType in the Options struct.  Should
I?

[scroggo, 2013-12-12 22:33:45]

On 2013/12/12 20:41:53, Hal Canary wrote:
> On 2013/12/12 19:21:00, scroggo wrote:
> > In order to introduce no regressions into Android, it appears that the only
> > fields they actually respect for SkImageRef are sampleSize and config.
Perhaps
> > we should only try to add that behavior?
> 
> I'll remove fPreferQualityOverSpeed since that is missing from Android. 
Dither
> is there in SkImageRef::setDitherImage.
> 
> I want all of the options to be passed into the constructor, so that we can
> treat them as constants.
> 
> > I also think we should push forward to SkImageInfo rather than relying on
> > config.
> 
> I could replace SkBitmap::Config with SkColorType in the Options struct. 
Should
> I?

It's fine to stick with Config for now.

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

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:27: SkBitmap::Config config)
Comments would be nice here, explaining the parameters.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:37: if ((NULL == fTarget)
When will fTarget be NULL and called?

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:39: || (fWidth != bm->width())
It seems you're mixing uses of this if statement. If the configs don't match,
then we should use the default allocator so we can later use fTarget in copyTo.
If the width or height don't match, isn't that an error? Likewise, I suspect the
ct will never be non NULL unless the configs don't match (since you're
explicitly disallowing Index8).

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:202: SkDEBUGFAIL("!bitmap.asImageInfo()
&& !kNoConfig");
Can you make this message more informative?

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

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:24: struct Options;
Perhaps a note here to look down below?

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:31: SkDecodingImageGenerator(SkData* data,
const Options* opts);
I like your thinking here, to allow opts to be NULL, but I think Mike generally
frowns on pointer to const as a parameter. If you made it a const reference, the
only difference would be requiring the caller to create an object and then
calling the copy constructor, yes?

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:57:
SkDecodingImageGenerator(SkStreamRewindable* stream,
Do these get called directly? Or do callers call Install? Wondering if these
should be private.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:136: *  @param fPreferQualityOverSpeed
Should the decoder should try to
Whether*

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:142: *         this one. If this config
won't work, getInfo() will
This comment seems vague. getInfo() on which class?

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:168: Options             fOpts;
Can be const?

https://codereview.chromium.org/93703004/diff/1/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/1/tests/ImageDecodingTest.cpp#n...
tests/ImageDecodingTest.cpp:379: SkAutoDataUnref
encoded(SkData::NewFromFileName(path.c_str()));
Why only test one or the other?

[hal.canary, 2013-12-13 15:48:48]

extensive API change

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

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:27: SkBitmap::Config config)
On 2013/12/12 22:33:46, scroggo wrote:
> Comments would be nice here, explaining the parameters.

Done.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:37: if ((NULL == fTarget)
On 2013/12/12 22:33:46, scroggo wrote:
> When will fTarget be NULL and called?

probably shouldn't happen.  but a weird decoder could try and use the allocator
multiple times.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:39: || (fWidth != bm->width())
On 2013/12/12 22:33:46, scroggo wrote:
> It seems you're mixing uses of this if statement. If the configs don't match,
> then we should use the default allocator so we can later use fTarget in
copyTo.
> If the width or height don't match, isn't that an error? Likewise, I suspect
the
> ct will never be non NULL unless the configs don't match (since you're
> explicitly disallowing Index8).

I just want to assert that everyone agrees on what the pixels are before we
write to them.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:202: SkDEBUGFAIL("!bitmap.asImageInfo()
&& !kNoConfig");
On 2013/12/12 22:33:46, scroggo wrote:
> Can you make this message more informative?

Done.

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

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:24: struct Options;
On 2013/12/12 22:33:46, scroggo wrote:
> Perhaps a note here to look down below?

Done.  Moved Options to the top.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:31: SkDecodingImageGenerator(SkData* data,
const Options* opts);
On 2013/12/12 22:33:46, scroggo wrote:
> I like your thinking here, to allow opts to be NULL, but I think Mike
generally
> frowns on pointer to const as a parameter. If you made it a const reference,
the
> only difference would be requiring the caller to create an object and then
> calling the copy constructor, yes?

Done.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:57:
SkDecodingImageGenerator(SkStreamRewindable* stream,
On 2013/12/12 22:33:46, scroggo wrote:
> Do these get called directly? Or do callers call Install? Wondering if these
> should be private.

I'm in the process of eliminating that use case.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:136: *  @param fPreferQualityOverSpeed
Should the decoder should try to
On 2013/12/12 22:33:46, scroggo wrote:
> Whether*

removed this param, as per your suggestion.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:142: *         this one. If this config
won't work, getInfo() will
On 2013/12/12 22:33:46, scroggo wrote:
> This comment seems vague. getInfo() on which class?

Done.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.h:168: Options             fOpts;
On 2013/12/12 22:33:46, scroggo wrote:
> Can be const?

Done.

https://codereview.chromium.org/93703004/diff/1/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/1/tests/ImageDecodingTest.cpp#n...
tests/ImageDecodingTest.cpp:379: SkAutoDataUnref
encoded(SkData::NewFromFileName(path.c_str()));
On 2013/12/12 22:33:46, scroggo wrote:
> Why only test one or the other?

I didn't want another for loop.

[scroggo, 2013-12-13 17:23:02]

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

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:37: if ((NULL == fTarget)
On 2013/12/13 15:48:48, Hal Canary wrote:
> On 2013/12/12 22:33:46, scroggo wrote:
> > When will fTarget be NULL and called?
> 
> probably shouldn't happen.  but a weird decoder could try and use the
allocator
> multiple times.

What should we do in that situation? I think the client will want the final
bitmap to be allocated via the allocator, but in this case, the first
(presumably thrown away) bitmap was allocated via the allocator.

https://codereview.chromium.org/93703004/diff/1/src/images/SkDecodingImageGen...
src/images/SkDecodingImageGenerator.cpp:39: || (fWidth != bm->width())
On 2013/12/13 15:48:48, Hal Canary wrote:
> On 2013/12/12 22:33:46, scroggo wrote:
> > It seems you're mixing uses of this if statement. If the configs don't
match,
> > then we should use the default allocator so we can later use fTarget in
> copyTo.
> > If the width or height don't match, isn't that an error? Likewise, I suspect
> the
> > ct will never be non NULL unless the configs don't match (since you're
> > explicitly disallowing Index8).
> 
> I just want to assert that everyone agrees on what the pixels are before we
> write to them.
> 

You don't assert anything - you allocate new pixels not using fTarget, which may
or may not be what the caller expected.

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

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:43: // Call default allcoator.
allocator*

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:52: fTarget = NULL;  // never alloc smae
pixels twice!
same*

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:57: void* fTarget;  // Pointer to block
of memory.
While these comments are true, I don't think they're particularly helpful for
someone trying to read and understand this class. How about something like:

fTarget; // Block of memory to be supplied as pixel memory in allocPixelRef.
Must be large enough to hold a bitmap described by fWidth, fHeight, and
fRowBytes.

fRowBytes; // rowbytes for the destination bitmap
fWidth; // Along with fHeight and fConfig, the information about the bitmap
whose pixels this allocator is expected to allocate. If they do not match the
bitmap passed to allocPixelRef, it is assumed that the bitmap will be copied to
a bitmap with the correct info using this allocator, so the default allocator
will be used instead of fTarget.
fHeight;
fConfig;

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:221:
SkASSERT(SkBitmap::kARGB_4444_Config == opts.fRequestedConfig);
I don't think this assert belongs here. What if canCopyTo changes in the future?

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:284: bitmap.copyTo(&bm,
fRequestedConfig, &allocator);
Check the return value of copyTo?

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

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:59: *  The SkData version of is preferred.
 If the stream has an
of is?

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:62: *  This object will unref the stream
when done.  Since streams
Can you be more specific that it will also unref the stream on failure?

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:66: *  that there are no other consumers
of the stream who will
Isn't that redundant with passing ownership?

https://codereview.chromium.org/93703004/diff/100001/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/100001/tests/ImageDecodingTest....
tests/ImageDecodingTest.cpp:349: Options(1, true, SkBitmap::kNo_Config),
Would it be better to generate this programmatically? Then if we decide to
modify Options in the future the change should be much simpler.

[hal.canary, 2013-12-16 15:10:26]

I'm getting closer on this.  I'm now changing gears to work on Chrome rather
than Android.

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

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:43: // Call default allcoator.
On 2013/12/13 17:23:03, scroggo wrote:
> allocator*

Done.

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:52: fTarget = NULL;  // never alloc smae
pixels twice!
On 2013/12/13 17:23:03, scroggo wrote:
> same*

Done.

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:57: void* fTarget;  // Pointer to block
of memory.
On 2013/12/13 17:23:03, scroggo wrote:
> While these comments are true, I don't think they're particularly helpful for
> someone trying to read and understand this class. How about something like:
> 
> fTarget; // Block of memory to be supplied as pixel memory in allocPixelRef.
> Must be large enough to hold a bitmap described by fWidth, fHeight, and
> fRowBytes.
> 
> fRowBytes; // rowbytes for the destination bitmap
> fWidth; // Along with fHeight and fConfig, the information about the bitmap
> whose pixels this allocator is expected to allocate. If they do not match the
> bitmap passed to allocPixelRef, it is assumed that the bitmap will be copied
to
> a bitmap with the correct info using this allocator, so the default allocator
> will be used instead of fTarget.
> fHeight;
> fConfig;

Done.

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:221:
SkASSERT(SkBitmap::kARGB_4444_Config == opts.fRequestedConfig);
On 2013/12/13 17:23:03, scroggo wrote:
> I don't think this assert belongs here. What if canCopyTo changes in the
future?

Done.

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:284: bitmap.copyTo(&bm,
fRequestedConfig, &allocator);
On 2013/12/13 17:23:03, scroggo wrote:
> Check the return value of copyTo?

Done.

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

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:59: *  The SkData version of is preferred.
 If the stream has an
On 2013/12/13 17:23:03, scroggo wrote:
> of is?

Done.

https://codereview.chromium.org/93703004/diff/100001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:62: *  This object will unref the stream
when done.  Since streams
On 2013/12/13 17:23:03, scroggo wrote:
> Can you be more specific that it will also unref the stream on failure?

Done.

[scroggo, 2013-12-16 15:38:36]

lgtm

https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest....
tests/ImageDecodingTest.cpp:326: * Given either a SkStream or a SkData, try to
decode the endoded
encoded*

[hal.canary, 2013-12-17 17:08:11]

Mike, I make two small changes in include/, so PTAL.

https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest....
tests/ImageDecodingTest.cpp:326: * Given either a SkStream or a SkData, try to
decode the endoded
On 2013/12/16 15:38:36, scroggo wrote:
> encoded*

Done.

[scroggo, 2013-12-17 17:10:08]

On 2013/12/17 17:08:11, Hal Canary wrote:
> Mike, I make two small changes in include/, so PTAL.
> 
>
https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest.cpp
> File tests/ImageDecodingTest.cpp (right):
> 
>
https://codereview.chromium.org/93703004/diff/170001/tests/ImageDecodingTest....
> tests/ImageDecodingTest.cpp:326: * Given either a SkStream or a SkData, try to
> decode the endoded
> On 2013/12/16 15:38:36, scroggo wrote:
> > encoded*
> 
> Done.

lgtm, at patch set 8

[reed1, 2013-12-17 17:38:52]

https://codereview.chromium.org/93703004/diff/190001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/93703004/diff/190001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:225: Sk64 SkBitmap::ComputeSize64(const SkImageInfo& info)
{
Why are these in bitmap, rather than SkImageInfo?

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

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:38: struct SK_API SkDecoderOptions {
Do these need to appear anywhere w/o the generator? If not, can we make this a
nested struct inside the generator? That is slightly clearer for name spacing.

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:49: const SkBitmap::Config
fRequestedConfig;
1. Can we replace config with SkColorType
2. Can we use SkImageInfo instead of samplesize?

[hal.canary, 2013-12-17 21:00:21]

https://codereview.chromium.org/93703004/diff/190001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/93703004/diff/190001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:225: Sk64 SkBitmap::ComputeSize64(const SkImageInfo& info)
{
On 2013/12/17 17:38:52, reed1 wrote:
> Why are these in bitmap, rather than SkImageInfo?

Good question.  Now that I look in SkImageInfo, I see I can replace

	SkBitmap::ComputeSize(info)

with

	info.getSafeSize(info.minRowBytes())

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

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:38: struct SK_API SkDecoderOptions {
On 2013/12/17 17:38:52, reed1 wrote:
> Do these need to appear anywhere w/o the generator? If not, can we make this a
> nested struct inside the generator? That is slightly clearer for name spacing.

okay.  I'll have to move the generator definition back to the header.

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:49: const SkBitmap::Config
fRequestedConfig;
On 2013/12/17 17:38:52, reed1 wrote:
> 1. Can we replace config with SkColorType
> 2. Can we use SkImageInfo instead of samplesize?

1. Yes.  Done.

2. No: the samplesize is interpreted differently by each decoder - sometimes
they round up, and sometimes they round down after a divide.  There is no way
for the caller to know how that will happen.

[reed1, 2013-12-17 22:28:57]

If scroggo approves, who am I to stand in the way. lgtm

[scroggo, 2013-12-17 22:53:33]

On 2013/12/17 22:28:57, reed1 wrote:
> If scroggo approves, who am I to stand in the way. lgtm

If the changes since patch set 8 are basically Hal's responses to
https://codereview.chromium.org/93703004/#msg12 (code has moved and it's hard to
follow along to tell whether it changed during the move as well) then I approve.

I do have a few questions though:

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

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:37: virtual bool allocPixelRef(SkBitmap*
bm, SkColorTable* ct) SK_OVERRIDE {
Why did you remove this?

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

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:46: *  @param fRequestedColorType If not
given, then use whichever
What is this set to if not given?

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:57: , fRequestedColorType() { }
Will valgrind be okay with this? What does it get set to?

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:63: Options(int sampleSize, bool dither,
SkColorType colorType)
Do we use all three constructors?

[hal.canary, 2013-12-18 15:38:40]

On 2013/12/17 22:53:33, scroggo wrote:
> On 2013/12/17 22:28:57, reed1 wrote:
> > If scroggo approves, who am I to stand in the way. lgtm
> 
> If the changes since patch set 8 are basically Hal's responses to
> https://codereview.chromium.org/93703004/#msg12 (code has moved and it's hard
to
> follow along to tell whether it changed during the move as well) then I
approve.

Yes, I just moved some code around.  I also added functions I needed to
SkImagePriv

> I do have a few questions though:
> 
>
https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
> File src/images/SkDecodingImageGenerator.cpp (right):
> 
>
https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
> src/images/SkDecodingImageGenerator.cpp:37: virtual bool
allocPixelRef(SkBitmap*
> bm, SkColorTable* ct) SK_OVERRIDE {
> Why did you remove this?

remove what?

>
https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
> src/images/SkDecodingImageGenerator.h:57: , fRequestedColorType() { }
> Will valgrind be okay with this? What does it get set to?


I ran my unit tests through valgrind on my workstation and it did not complain.

[hal.canary, 2013-12-18 15:39:05]

https://codereview.chromium.org/93703004/diff/100001/tests/ImageDecodingTest.cpp
File tests/ImageDecodingTest.cpp (right):

https://codereview.chromium.org/93703004/diff/100001/tests/ImageDecodingTest....
tests/ImageDecodingTest.cpp:349: Options(1, true, SkBitmap::kNo_Config),
On 2013/12/13 17:23:03, scroggo wrote:
> Would it be better to generate this programmatically? Then if we decide to
> modify Options in the future the change should be much simpler.

Done.

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

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:37: virtual bool allocPixelRef(SkBitmap*
bm, SkColorTable* ct) SK_OVERRIDE {
On 2013/12/17 22:53:34, scroggo wrote:
> Why did you remove this?

Remove what?

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

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:46: *  @param fRequestedColorType If not
given, then use whichever
On 2013/12/17 22:53:34, scroggo wrote:
> What is this set to if not given?

The value of opts.fRequestedColorType is only referenced four times.  Each of
these is in a code path where opts.fRequestedColorType is true.  I hope valgrind
sees that and respects that.  I am running the valgrind bot to verify.

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:57: , fRequestedColorType() { }
On 2013/12/17 22:53:34, scroggo wrote:
> Will valgrind be okay with this? What does it get set to?

See above.

https://codereview.chromium.org/93703004/diff/220001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.h:63: Options(int sampleSize, bool dither,
SkColorType colorType)
On 2013/12/17 22:53:34, scroggo wrote:
> Do we use all three constructors?

We use the default constructor in many of our tests.

Android will use the third constructor.

I include the second constructor for completeness (in case we want to set
sampleSize or dither without specifying a color type (these ought to be
orthogonal).  I exercise that in the unit tests only.

[scroggo, 2013-12-18 17:02:19]

lgtm

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

https://codereview.chromium.org/93703004/diff/190001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:37: virtual bool allocPixelRef(SkBitmap*
bm, SkColorTable* ct) SK_OVERRIDE {
On 2013/12/18 15:39:07, Hal Canary wrote:
> On 2013/12/17 22:53:34, scroggo wrote:
> > Why did you remove this?
> 
> Remove what?

Reitveld was showing that SK_OVERRIDE was no longer there. Now it's back though.

[hal.canary, 2013-12-18 17:40:34]

Committed patchset #13 manually as r12744 (presubmit successful).

[robertphillips, 2013-12-18 18:28:14]

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

The reason for reverting is: Test failures.

[hal.canary, 2014-01-02 13:15:23]

Committed patchset #19 manually as r12855 (presubmit successful).