Make SkImageGenerator::getPixels() return an enum.

include/core/SkImageGenerator.h

Index: include/core/SkImageGenerator.h
diff --git a/include/core/SkImageGenerator.h b/include/core/SkImageGenerator.h
index 2967974c7845cb95cfb6c5b7f2e3b0815fde9f88..f4eed0e8fef381181469595a0b9a312a226fc49f 100644
--- a/include/core/SkImageGenerator.h
+++ b/include/core/SkImageGenerator.h
@@ -45,7 +45,7 @@ SK_API bool SkInstallDiscardablePixelRef(SkData* encoded, SkBitmap* destination)
  *  An interface that allows a purgeable PixelRef (such as a
  *  SkDiscardablePixelRef) to decode and re-decode an image as needed.
  */
-class SK_API SkImageGenerator {
+class SK_API SkImageGenerator : public SkNoncopyable {

[scroggo, 2015/02/11 16:01:25]

Hal, was this previously deliberately copyable?

 public:
     /**
      *  The PixelRef which takes ownership of this SkImageGenerator
@@ -74,6 +74,48 @@ public:
     bool getInfo(SkImageInfo* info);
 
     /**
+     *  Used to describe the result of a call to getPixels().
+     *
+     *  Result is the union of possible results from subclasses.
+     */
+    enum Result {
+        /**
+         *  The generator cannot convert to match the request.

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

/**  The generator cannot convert ColorType to match the request. */

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

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.

+         */
+        kInvalidConversion,
+        /**
+         *  The generator cannot scale to requested size.
+         */
+        kInvalidScale,
+        /**
+         *  Parameters (besides info) are invalid. e.g. NULL pixels, rowBytes
+         *  too small, etc.
+         */
+        kInvalidParameters,

[scroggo, 2015/02/11 16:01:25]

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.

+        /**
+         *  The input did not contain a valid image.
+         */
+        kInvalidInput,
+        /**
+         *  Fulfilling this request requires rewinding the input, which is not
+         *  supported for this input.
+         */
+        kCouldNotRewind,
+        /**
+         *  This method is not implemented by this generator.
+         */
+        kUnimplemented,
+        /**
+         *  The input is incomplete. A partial image was generated.
+         */
+        kIncompleteInput,

[scroggo, 2015/02/11 16:01:25]

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.

+        /**
+         *  General return value for success.
+         */
+        kSuccess,
+    };
+
+    /**
      *  Decode into the given pixels, a block of memory of size at
      *  least (info.fHeight - 1) * rowBytes + (info.fWidth *
      *  bytesPerPixel)
@@ -89,6 +131,9 @@ public:
      *         different output-configs, which the implementation can
      *         decide to support or not.
      *
+     *         A size that does not match getInfo() implies a request
+     *         to scale.

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

add a little more info here that the generator can return InvalidScale if the
underlying implementation does not support that scale factor.

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

Done.

+     *
      *  If info is kIndex8_SkColorType, then the caller must provide storage for up to 256
      *  SkPMColor values in ctable. On success the generator must copy N colors into that storage,
      *  (where N is the logical number of table entries) and set ctableCount to N.
@@ -99,13 +144,13 @@ public:
      *  @return false if anything goes wrong or if the image info is
      *          unsupported.
      */
-    bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
-                   SkPMColor ctable[], int* ctableCount);
+    Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,

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

perhaps for a diff CL -- but I think it is more consistent with other parts of
skia to rename this readPixels(...)

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

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.

+                     SkPMColor ctable[], int* ctableCount);
 
     /**
      *  Simplified version of getPixels() that asserts that info is NOT kIndex8_SkColorType.
      */
-    bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes);
+    Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes);
 
     /**
      *  If planes or rowBytes is NULL or if any entry in planes is NULL or if any entry in rowBytes
@@ -131,9 +176,9 @@ public:
 protected:
     virtual SkData* onRefEncodedData();
     virtual bool onGetInfo(SkImageInfo* info);
-    virtual bool onGetPixels(const SkImageInfo& info,
-                             void* pixels, size_t rowBytes,
-                             SkPMColor ctable[], int* ctableCount);
+    virtual Result onGetPixels(const SkImageInfo& info,
+                               void* pixels, size_t rowBytes,
+                               SkPMColor ctable[], int* ctableCount);
     virtual bool onGetYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3]);
     virtual bool onGetYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3],
                                  SkYUVColorSpace* colorSpace);