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

Review URL: https://codereview.chromium.org/919693002

diff --git a/gyp/skia_for_chromium_defines.gypi b/gyp/skia_for_chromium_defines.gypi
index bcc6096..bcb33a2 100644 (file)
--- a/gyp/skia_for_chromium_defines.gypi
+++ b/gyp/skia_for_chromium_defines.gypi
@@ -14,6 +14,7 @@
     #
     'skia_for_chromium_defines': [
       'SK_LEGACY_DRAWPICTURECALLBACK',
+      'SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN',
     ],
   },
 }

diff --git a/include/core/SkImageGenerator.h b/include/core/SkImageGenerator.h
index 2967974..de58b68 100644 (file)
--- a/include/core/SkImageGenerator.h
+++ b/include/core/SkImageGenerator.h
@@ -15,6 +15,8 @@ class SkBitmap;
 class SkData;
 class SkImageGenerator;
 
+//#define SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN
+
 /**
  *  Takes ownership of SkImageGenerator.  If this method fails for
  *  whatever reason, it will return false and immediatetely delete
@@ -45,7 +47,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 {
 public:
     /**
      *  The PixelRef which takes ownership of this SkImageGenerator
@@ -74,6 +76,49 @@ 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 {
+        /**
+         *  General return value for success.
+         */
+        kSuccess,
+        /**
+         *  The input is incomplete. A partial image was generated.
+         */
+        kIncompleteInput,
+        /**
+         *  The generator cannot convert to match the request, ignoring
+         *  dimensions.
+         */
+        kInvalidConversion,
+        /**
+         *  The generator cannot scale to requested size.
+         */
+        kInvalidScale,
+        /**
+         *  Parameters (besides info) are invalid. e.g. NULL pixels, rowBytes
+         *  too small, etc.
+         */
+        kInvalidParameters,
+        /**
+         *  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,
+    };
+
+    /**
      *  Decode into the given pixels, a block of memory of size at
      *  least (info.fHeight - 1) * rowBytes + (info.fWidth *
      *  bytesPerPixel)
@@ -89,6 +134,10 @@ 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. If the generator cannot perform this scale,
+     *         it will return kInvalidScale.
+     *
      *  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.
@@ -96,16 +145,15 @@ public:
      *  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.
+     *  @return Result kSuccess, or another value explaining the type of failure.
      */
-    bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
-                   SkPMColor ctable[], int* ctableCount);
+    Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
+                     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 +179,15 @@ public:
 protected:
     virtual SkData* onRefEncodedData();
     virtual bool onGetInfo(SkImageInfo* info);
+#ifdef SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN
     virtual bool onGetPixels(const SkImageInfo& info,
                              void* pixels, size_t rowBytes,
                              SkPMColor ctable[], int* ctableCount);
+#endif
+    // TODO (scroggo): rename to onGetPixels.
+    virtual Result onGetPixelsEnum(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);

diff --git a/src/core/SkImageGenerator.cpp b/src/core/SkImageGenerator.cpp
index 0f63db5..c6167ff 100644 (file)
--- a/src/core/SkImageGenerator.cpp
+++ b/src/core/SkImageGenerator.cpp
@@ -15,21 +15,22 @@ bool SkImageGenerator::getInfo(SkImageInfo* info) {
     return this->onGetInfo(info);
 }
 
-bool SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
-                                 SkPMColor ctable[], int* ctableCount) {
+SkImageGenerator::Result SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels,
+                                                     size_t rowBytes, SkPMColor ctable[],
+                                                     int* ctableCount) {
     if (kUnknown_SkColorType == info.colorType()) {
-        return false;
+        return kInvalidConversion;
     }
     if (NULL == pixels) {
-        return false;
+        return kInvalidParameters;
     }
     if (rowBytes < info.minRowBytes()) {
-        return false;
+        return kInvalidParameters;
     }
 
     if (kIndex_8_SkColorType == info.colorType()) {
         if (NULL == ctable || NULL == ctableCount) {
-            return false;
+            return kInvalidParameters;
         }
     } else {
         if (ctableCount) {
@@ -39,18 +40,19 @@ bool SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels, size_t r
         ctable = NULL;
     }
 
-    bool success = this->onGetPixels(info, pixels, rowBytes, ctable, ctableCount);
+    const Result result = this->onGetPixelsEnum(info, pixels, rowBytes, ctable, ctableCount);
 
-    if (success && ctableCount) {
+    if ((kIncompleteInput == result || kSuccess == result) && ctableCount) {
         SkASSERT(*ctableCount >= 0 && *ctableCount <= 256);
     }
-    return success;
+    return result;
 }
 
-bool SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes) {
+SkImageGenerator::Result SkImageGenerator::getPixels(const SkImageInfo& info, void* pixels,
+                                                     size_t rowBytes) {
     SkASSERT(kIndex_8_SkColorType != info.colorType());
     if (kIndex_8_SkColorType == info.colorType()) {
-        return false;
+        return kInvalidConversion;
     }
     return this->getPixels(info, pixels, rowBytes, NULL, NULL);
 }
@@ -117,6 +119,19 @@ bool SkImageGenerator::onGetInfo(SkImageInfo*) {
     return false;
 }
 
-bool SkImageGenerator::onGetPixels(const SkImageInfo&, void*, size_t, SkPMColor*, int*) {
-    return false;
+#ifdef SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN
+bool SkImageGenerator::onGetPixels(const SkImageInfo&, void*, size_t,
+                                                       SkPMColor*, int*) {
+    return kUnimplemented;
+}
+#endif
+SkImageGenerator::Result SkImageGenerator::onGetPixelsEnum(const SkImageInfo& info, void* pixels,
+                                                           size_t rowBytes, SkPMColor* colors,
+                                                           int* colorCount) {
+#ifdef SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN
+    if (this->onGetPixels(info, pixels, rowBytes, colors, colorCount)) {
+        return kSuccess;
+    }
+#endif
+    return kUnimplemented;
 }

diff --git a/src/images/SkDecodingImageGenerator.cpp b/src/images/SkDecodingImageGenerator.cpp
index e7a775e..cce01d2 100644 (file)
--- a/src/images/SkDecodingImageGenerator.cpp
+++ b/src/images/SkDecodingImageGenerator.cpp
@@ -42,9 +42,9 @@ protected:
         *info = fInfo;
         return true;
     }
-    virtual bool onGetPixels(const SkImageInfo& info,
-                             void* pixels, size_t rowBytes,
-                             SkPMColor ctable[], int* ctableCount) SK_OVERRIDE;
+    virtual Result onGetPixelsEnum(const SkImageInfo& info,
+                              void* pixels, size_t rowBytes,
+                              SkPMColor ctable[], int* ctableCount) SK_OVERRIDE;
     virtual bool onGetYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3],
                                  SkYUVColorSpace* colorSpace) SK_OVERRIDE;
 
@@ -147,20 +147,22 @@ SkData* DecodingImageGenerator::onRefEncodedData() {
     return SkSafeRef(fData);
 }
 
-bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
-                                         void* pixels, size_t rowBytes,
-                                         SkPMColor ctableEntries[], int* ctableCount) {
+SkImageGenerator::Result DecodingImageGenerator::onGetPixelsEnum(const SkImageInfo& info,
+        void* pixels, size_t rowBytes, SkPMColor ctableEntries[], int* ctableCount) {
     if (fInfo != info) {
         // The caller has specified a different info.  This is an
         // error for this kind of SkImageGenerator.  Use the Options
         // to change the settings.
-        return false;
+        if (info.dimensions() != fInfo.dimensions()) {
+            return kInvalidScale;
+        }
+        return kInvalidConversion;
     }
 
     SkAssertResult(fStream->rewind());
     SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(fStream));
     if (NULL == decoder.get()) {
-        return false;
+        return kInvalidInput;
     }
     decoder->setDitherImage(fDitherImage);
     decoder->setSampleSize(fSampleSize);
@@ -169,11 +171,11 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
     SkBitmap bitmap;
     TargetAllocator allocator(fInfo, pixels, rowBytes);
     decoder->setAllocator(&allocator);
-    bool success = decoder->decode(fStream, &bitmap, info.colorType(),
-                                   SkImageDecoder::kDecodePixels_Mode) != SkImageDecoder::kFailure;
+    const SkImageDecoder::Result decodeResult = decoder->decode(fStream, &bitmap, info.colorType(),
+                                                                SkImageDecoder::kDecodePixels_Mode);
     decoder->setAllocator(NULL);
-    if (!success) {
-        return false;
+    if (SkImageDecoder::kFailure == decodeResult) {
+        return kInvalidInput;
     }
     if (allocator.isReady()) {  // Did not use pixels!
         SkBitmap bm;
@@ -182,7 +184,7 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
         if (!copySuccess || allocator.isReady()) {
             SkDEBUGFAIL("bitmap.copyTo(requestedConfig) failed.");
             // Earlier we checked canCopyto(); we expect consistency.
-            return false;
+            return kInvalidConversion;
         }
         SkASSERT(check_alpha(info.alphaType(), bm.alphaType()));
     } else {
@@ -191,17 +193,21 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info,
 
     if (kIndex_8_SkColorType == info.colorType()) {
         if (kIndex_8_SkColorType != bitmap.colorType()) {
-            return false;   // they asked for Index8, but we didn't receive that from decoder
+            // they asked for Index8, but we didn't receive that from decoder
+            return kInvalidConversion;
         }
         SkColorTable* ctable = bitmap.getColorTable();
         if (NULL == ctable) {
-            return false;
+            return kInvalidConversion;
         }
         const int count = ctable->count();
         memcpy(ctableEntries, ctable->readColors(), count * sizeof(SkPMColor));
         *ctableCount = count;
     }
-    return true;
+    if (SkImageDecoder::kPartialSuccess == decodeResult) {
+        return kIncompleteInput;
+    }
+    return kSuccess;
 }
 
 bool DecodingImageGenerator::onGetYUV8Planes(SkISize sizes[3], void* planes[3],

diff --git a/src/lazy/SkCachingPixelRef.cpp b/src/lazy/SkCachingPixelRef.cpp
index 5ab9656..570fc6f 100644 (file)
--- a/src/lazy/SkCachingPixelRef.cpp
+++ b/src/lazy/SkCachingPixelRef.cpp
@@ -52,9 +52,15 @@ bool SkCachingPixelRef::onNewLockPixels(LockRec* rec) {
             fErrorInDecoding = true;
             return false;
         }
-        if (!fImageGenerator->getPixels(info, fLockedBitmap.getPixels(), fRowBytes)) {
-            fErrorInDecoding = true;
-            return false;
+        const SkImageGenerator::Result result = fImageGenerator->getPixels(info,
+                fLockedBitmap.getPixels(), fRowBytes);
+        switch (result) {
+            case SkImageGenerator::kIncompleteInput:
+            case SkImageGenerator::kSuccess:
+                break;
+            default:
+                fErrorInDecoding = true;
+                return false;
         }
         fLockedBitmap.setImmutable();
         SkBitmapCache::Add(

diff --git a/src/lazy/SkDiscardablePixelRef.cpp b/src/lazy/SkDiscardablePixelRef.cpp
index b51daa6..5098858 100644 (file)
--- a/src/lazy/SkDiscardablePixelRef.cpp
+++ b/src/lazy/SkDiscardablePixelRef.cpp
@@ -64,11 +64,17 @@ bool SkDiscardablePixelRef::onNewLockPixels(LockRec* rec) {
     SkPMColor colors[256];
     int colorCount = 0;
 
-    if (!fGenerator->getPixels(info, pixels, fRowBytes, colors, &colorCount)) {
-        fDiscardableMemory->unlock();
-        SkDELETE(fDiscardableMemory);
-        fDiscardableMemory = NULL;
-        return false;
+    const SkImageGenerator::Result result = fGenerator->getPixels(info, pixels, fRowBytes,
+                                                                  colors, &colorCount);
+    switch (result) {
+        case SkImageGenerator::kSuccess:
+        case SkImageGenerator::kIncompleteInput:
+            break;
+        default:
+            fDiscardableMemory->unlock();
+            SkDELETE(fDiscardableMemory);
+            fDiscardableMemory = NULL;
+            return false;
     }
 
     // Note: our ctable is not purgeable, as it is not stored in the discardablememory block.

diff --git a/src/ports/SkImageGenerator_skia.cpp b/src/ports/SkImageGenerator_skia.cpp
index 878a44d..079da56 100644 (file)
--- a/src/ports/SkImageGenerator_skia.cpp
+++ b/src/ports/SkImageGenerator_skia.cpp
@@ -52,8 +52,8 @@ protected:
         return true;
     }
 
-    virtual bool onGetPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
-                             SkPMColor ctableEntries[], int* ctableCount) SK_OVERRIDE {
+    virtual Result onGetPixelsEnum(const SkImageInfo& info, void* pixels, size_t rowBytes,
+                               SkPMColor ctableEntries[], int* ctableCount) SK_OVERRIDE {
         SkMemoryStream stream(fData->data(), fData->size(), false);
         SkAutoTUnref<BareMemoryAllocator> allocator(SkNEW_ARGS(BareMemoryAllocator,
                                                                (info, pixels, rowBytes)));
@@ -61,13 +61,10 @@ protected:
         fDecoder->setRequireUnpremultipliedColors(kUnpremul_SkAlphaType == info.alphaType());
 
         SkBitmap bm;
-        SkImageDecoder::Result result = fDecoder->decode(&stream, &bm, info.colorType(),
-                                                         SkImageDecoder::kDecodePixels_Mode);
-        switch (result) {
-            case SkImageDecoder::kSuccess:
-                break;
-            default:
-                return false;
+        const SkImageDecoder::Result result = fDecoder->decode(&stream, &bm, info.colorType(),
+                                                               SkImageDecoder::kDecodePixels_Mode);
+        if (SkImageDecoder::kFailure == result) {
+            return kInvalidInput;
         }
 
         SkASSERT(info.colorType() == bm.info().colorType());
@@ -77,13 +74,16 @@ protected:
 
             SkColorTable* ctable = bm.getColorTable();
             if (NULL == ctable) {
-                return false;
+                return kInvalidConversion;
             }
             const int count = ctable->count();
             memcpy(ctableEntries, ctable->readColors(), count * sizeof(SkPMColor));
             *ctableCount = count;
         }
-        return true;
+        if (SkImageDecoder::kPartialSuccess == result) {
+            return kIncompleteInput;
+        }
+        return kSuccess;
     }
 
     bool onGetYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3],

diff --git a/tests/CachedDecodingPixelRefTest.cpp b/tests/CachedDecodingPixelRefTest.cpp
index e8fea2f..51cb7ba 100644 (file)
--- a/tests/CachedDecodingPixelRefTest.cpp
+++ b/tests/CachedDecodingPixelRefTest.cpp
@@ -189,15 +189,15 @@ protected:
         return true;
     }
 
-    virtual bool onGetPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
-                             SkPMColor ctable[], int* ctableCount) SK_OVERRIDE {
+    virtual Result onGetPixelsEnum(const SkImageInfo& info, void* pixels, size_t rowBytes,
+                               SkPMColor ctable[], int* ctableCount) SK_OVERRIDE {
         REPORTER_ASSERT(fReporter, pixels != NULL);
-        size_t minRowBytes = static_cast<size_t>(info.width() * info.bytesPerPixel());
-        REPORTER_ASSERT(fReporter, rowBytes >= minRowBytes);
-        if ((NULL == pixels)
-            || (fType != kSucceedGetPixels_TestType)
-            || (info.colorType() != kN32_SkColorType)) {
-            return false;
+        REPORTER_ASSERT(fReporter, rowBytes >= info.minRowBytes());
+        if (fType != kSucceedGetPixels_TestType) {
+            return kUnimplemented;
+        }
+        if (info.colorType() != kN32_SkColorType) {
+            return kInvalidConversion;
         }
         char* bytePtr = static_cast<char*>(pixels);
         for (int y = 0; y < info.height(); ++y) {
@@ -205,7 +205,7 @@ protected:
                         TestImageGenerator::Color(), info.width());
             bytePtr += rowBytes;
         }
-        return true;
+        return kSuccess;
     }
 
 private: