Make SkImageGenerator::getPixels() return an enum.

include/core/SkImageGenerator.h

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkImageGenerator_DEFINED
 #define SkImageGenerator_DEFINED
 
@@ skipping 27 matching lines @@
 /**
  *  On success, installs a discardable pixelref into destination, based on encoded data.
  *  Regardless of success or failure, the caller must still balance their ownership of encoded.
  */
 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
      *  will call the image generator's destructor.
      */
     virtual ~SkImageGenerator() { }
 
     /**
      *  Return a ref to the encoded (i.e. compressed) representation,
      *  of this data.
      *
      *  If non-NULL is returned, the caller is responsible for calling
      *  unref() on the data when it is finished.
      */
     SkData* refEncodedData() { return this->onRefEncodedData(); }
 
     /**
      *  Return some information about the image, allowing the owner of
      *  this object to allocate pixels.
      *
      *  Repeated calls to this function should give the same results,
      *  allowing the PixelRef to be immutable.
      *
      *  @return false if anything goes wrong.
      */
     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)
      *
      *  Repeated calls to this function should give the same results,
      *  allowing the PixelRef to be immutable.
      *
      *  @param info A description of the format (config, size)
      *         expected by the caller.  This can simply be identical
      *         to the info returned by getInfo().
      *
      *         This contract also allows the caller to specify
      *         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.
      *
      *  If info is not kIndex8_SkColorType, then the last two parameters may be NULL. If ctableCount
      *  is not null, it will be set to 0.
      *
      *  @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
      *  is 0, this imagegenerator should output the sizes and return true if it can efficiently
      *  return YUV planar data. If it cannot, it should return false. Note that either planes and
      *  rowBytes are both fully defined and non NULL/non 0 or they are both NULL or have NULL or 0
      *  entries only. Having only partial planes/rowBytes information is not supported.
      *
      *  If all planes and rowBytes entries are non NULL or non 0, then it should copy the
      *  associated YUV data into those planes of memory supplied by the caller. It should validate
      *  that the sizes match what it expected. If the sizes do not match, it should return false.
      */
     bool getYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3],
                        SkYUVColorSpace* colorSpace);
 
     /**
      *  If the default image decoder system can interpret the specified (encoded) data, then
      *  this returns a new ImageGenerator for it. Otherwise this returns NULL. Either way
      *  the caller is still responsible for managing their ownership of the data.
      */
     static SkImageGenerator* NewFromData(SkData*);
 
 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);
 };
 
 #endif  // SkImageGenerator_DEFINED