Make SkImageGenerator::getPixels() return an enum.

Make SkImageGenerator::getPixels() return an enum.

The new enum describes the nature of the failure. This is in
preparation for writing a replacement for SkImageDecoder, which will
use this interface.

Update the comments for getPixels() to specify what it means to pass
an SkImageInfo with a different size.

Make SkImageGenerator Noncopyable.

Leave onGetYUV8Planes alone, since we have separate discussions
regarding modifying that API.

Make callers of SkImageDecoder consistently handle kPartialSuccess.
Previously, some callers considered it a failure, and others considered
it a success.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/0864908ca50049d3d907fc5c3749bc8a436b4738

[scroggo, 2015-02-11 16:01:24]

scroggo@google.com changed reviewers:
+ djsollen@google.com, halcanary@google.com, reed@google.com

[scroggo, 2015-02-11 16:01:26]

As discussed elsewhere, this merges some of the ideas from SkCodec into
SkImageGenerator.

I've left out scanline/subset decoding, so we can push forward with standard
decoding for now, which is more straightforward (and I'd still like to discuss
scanline/subsets with webp team more).

A follow-up change will reintroduce SkCodec, as a subclass of SkImageGenerator.

This will require a change to Chromium (which in turn means we may need to stage
this, but I want to agree on the outcome before adding staging macros).

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

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:48: class SK_API SkImageGenerator : public
SkNoncopyable {
Hal, was this previously deliberately copyable?

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:94: kInvalidParameters,
We could have more specific enums (e.g. kNullPixels, kRowBytesTooSmall, etc).
There's a tradeoff between specificity and bloating this enum. I leaned towards
one enum for bad parameters, but welcome input.

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:111: kIncompleteInput,
For SkImageDecoder::Result, I made incomplete and success positive numbers, and
failure zero, so that callers could treat the return value as a boolean,
considering incomplete and success as true.

We cannot do that here (since we have more than one false), which means we need
to make sure we catch all callers of getPixels (and don't introduce new ones
that treat is a boolean!). I somewhat expected a call like

  if (!generator->getPixels(...)) {

to generate a warning, but it does not generate one for me locally.

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

https://codereview.chromium.org/919693002/diff/1/src/images/SkDecodingImageGe...
src/images/SkDecodingImageGenerator.cpp:201: return kInvalidConversion;
This mimics the old behavior, but it seems like this should be an assert? This
would mean the decoder did something wrong.

https://codereview.chromium.org/919693002/diff/1/src/ports/SkImageGenerator_s...
File src/ports/SkImageGenerator_skia.cpp (left):

https://codereview.chromium.org/919693002/diff/1/src/ports/SkImageGenerator_s...
src/ports/SkImageGenerator_skia.cpp:80: return false;
This code sure looks like it was copied from DecodingImageGenerator. It also
seems like it should be an assert.

https://codereview.chromium.org/919693002/diff/1/src/ports/SkImageGenerator_s...
File src/ports/SkImageGenerator_skia.cpp (right):

https://codereview.chromium.org/919693002/diff/1/src/ports/SkImageGenerator_s...
src/ports/SkImageGenerator_skia.cpp:66: if (SkImageDecoder::kFailure == result)
{
This is a change in behavior, to match DecodingImageGenerator. (Treat partial
success as success.)

[scroggo, 2015-02-12 13:37:16]

Ping! Derek and Mike, will you please review? I'd like to get an interface
checked in so Matt and I can write implementations in parallel.

[djsollen, 2015-02-12 14:22:15]

looks good overall, just a few nits.

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

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:135: *         to scale.
add a little more info here that the generator can return InvalidScale if the
underlying implementation does not support that scale factor.

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

https://codereview.chromium.org/919693002/diff/1/src/images/SkDecodingImageGe...
src/images/SkDecodingImageGenerator.cpp:201: return kInvalidConversion;
On 2015/02/11 16:01:25, scroggo wrote:
> This mimics the old behavior, but it seems like this should be an assert? This
> would mean the decoder did something wrong.

I think we may want to have another error type that reports back if the input
params are invalid. We could then throw that error here as well as if they did
something like pass nullptr for the pixels

https://codereview.chromium.org/919693002/diff/1/tests/CachedDecodingPixelRef...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/919693002/diff/1/tests/CachedDecodingPixelRef...
tests/CachedDecodingPixelRefTest.cpp:195: const size_t minRowBytes =
info.minRowBytes();
remove this and have >= info.minRowBytes on next line.

[reed1, 2015-02-12 14:46:08]

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

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:147: Result getPixels(const SkImageInfo& info,
void* pixels, size_t rowBytes,
perhaps for a diff CL -- but I think it is more consistent with other parts of
skia to rename this readPixels(...)

[reed1, 2015-02-12 14:46:20]

lgtm

[hal.canary, 2015-02-12 14:54:59]

1gtm

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

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:83: *  The generator cannot convert to match the
request.
/**  The generator cannot convert ColorType to match the request. */

[scroggo, 2015-02-12 15:26:11]

New patchsets have been uploaded after l-g-t-m from reed@google.com

[scroggo, 2015-02-12 15:28:36]

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

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:83: *  The generator cannot convert to match the
request.
On 2015/02/12 14:54:59, Hal Canary wrote:
> /**  The generator cannot convert ColorType to match the request. */

I don't think that's quite accurate. The AlphaType may be infeasible (e.g.
requested Opaque on a non-Opaque image). Or the ColorProfileType request may not
match the encoded image. (Is it possible we'll add more enums to ImageInfo in
the future?)

It should be clear that it does not apply to the dimensions though. Comment
updated.

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:135: *         to scale.
On 2015/02/12 14:22:15, djsollen wrote:
> add a little more info here that the generator can return InvalidScale if the
> underlying implementation does not support that scale factor.

Done.

https://codereview.chromium.org/919693002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:147: Result getPixels(const SkImageInfo& info,
void* pixels, size_t rowBytes,
On 2015/02/12 14:46:08, reed1 wrote:
> perhaps for a diff CL -- but I think it is more consistent with other parts of
> skia to rename this readPixels(...)

Interesting... I just looked at the the definitions of readPixels(...). They
appear to be consistent with each other. The first thing I noticed is that their
signature is not exactly the same - they start off the same:

const SkImageInfo& info, void* pixels, size_t rowBytes

but they each also include an x,y offset.

More interestingly, they treat the info differently from the intent here. Here,
an info with different dimensions implies a scale (although I just looked at the
existing implementations in Chrome:

- DecodingImageGenerator (not to be confused with our DecodingImageGenerator,
although it behaves the same in this respect): if the width and height do not
match, it just returns false

- VideoImageGenerator: it ignores the info entirely...)

In <various>::readPixels, different dimensions imply a subset. To me that
implies that we should have a different name (at least).

On another note, we're actually "generating" pixels rather than "reading" them,
although that is perhaps true for some of the existing readPixels methods.

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

https://codereview.chromium.org/919693002/diff/1/src/images/SkDecodingImageGe...
src/images/SkDecodingImageGenerator.cpp:201: return kInvalidConversion;
On 2015/02/12 14:22:15, djsollen wrote:
> On 2015/02/11 16:01:25, scroggo wrote:
> > This mimics the old behavior, but it seems like this should be an assert?
This
> > would mean the decoder did something wrong.
> 
> I think we may want to have another error type that reports back if the input
> params are invalid. We could then throw that error here as well as if they did
> something like pass nullptr for the pixels

We already have that: kInvalidParameters, which gets returned in
SkImageGenerator::getPixels (the non-virtual that always gets called).

But that's not what happened if we reached here. The input from the caller is
ctableEntries. If that was null, we would have returned kInvalidParameters in
getPixels.

Looking more closely at this, I realize that I was confused. The colorType we're
checking is the requested colorType. I thought it was the bitmap's colorType -
i.e. the bitmap was Index8 but did *not* have a colorTable, which would be a bug
in the decoder.

I still think we would never reach here though (predating my changes). If the
caller requested Index8 (this block), we have two possibilities:

A) The decoder gave us Index8 - if bitmap does *not* have a colorTable, there
was a bug in the decoder (my original concern).

B) The decoder gave us something else. In this case, the above block (if
allocator.isReady) would have attempted to copy to Index8 - which always fails -
you can only copy to Index8 from another Index8. So we would return
kInvalidConversion before reaching here. (The assert that canCopyTo returned
true just tells me that we aren't actually testing this path.)

https://codereview.chromium.org/919693002/diff/1/tests/CachedDecodingPixelRef...
File tests/CachedDecodingPixelRefTest.cpp (right):

https://codereview.chromium.org/919693002/diff/1/tests/CachedDecodingPixelRef...
tests/CachedDecodingPixelRefTest.cpp:195: const size_t minRowBytes =
info.minRowBytes();
On 2015/02/12 14:22:15, djsollen wrote:
> remove this and have >= info.minRowBytes on next line.

I originally wrote that, but then I thought I saw minRowBytes somewhere
else.....

Done.

[scroggo, 2015-02-13 16:52:14]

I just added changes to help with staging in Chrome.

Chrome overrides onGetPixels, so we need to let them continue overriding the
original signature.

You can't have an two methods with the same parameters but different return
types, so I had to (temporarily) rename the new version.

Fortunately, the only caller of getPixels is inside Skia, so we don't need to
guard for the public API.

[scroggo, 2015-02-13 18:37:06]

On 2015/02/13 16:52:14, scroggo wrote:
> I just added changes to help with staging in Chrome.
> 
> Chrome overrides onGetPixels, so we need to let them continue overriding the
> original signature.
> 
> You can't have an two methods with the same parameters but different return
> types, so I had to (temporarily) rename the new version.
> 
> Fortunately, the only caller of getPixels is inside Skia, so we don't need to
> guard for the public API.

Mike, does this still look good to you? I have the chrome side changes ready to
review, but I want to make sure this looks good first.

[reed1, 2015-02-13 19:02:19]

api lgtm

[scroggo, 2015-02-13 19:05:16]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2015-02-13 19:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919693002/60001

[commit-bot: I haz the power, 2015-02-13 19:13:38]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/0864908ca50049d3d907fc5c3749bc8a436b4738