Change SkDecodingImageGenerator API

src/images/SkDecodingImageGenerator.cpp

Index: src/images/SkDecodingImageGenerator.cpp
diff --git a/src/images/SkDecodingImageGenerator.cpp b/src/images/SkDecodingImageGenerator.cpp
index a833c636ff9c8d30df1c1ade660743ea8b2a47d2..494f9f6882ed2c19f32afe82d08cbe8af1b2ee98 100644
--- a/src/images/SkDecodingImageGenerator.cpp
+++ b/src/images/SkDecodingImageGenerator.cpp
@@ -5,13 +5,14 @@
  * found in the LICENSE file.
  */
 
-#include "SkDecodingImageGenerator.h"
 #include "SkData.h"
+#include "SkDecodingImageGenerator.h"
 #include "SkImageDecoder.h"
+#include "SkImageInfo.h"
 #include "SkImageGenerator.h"
 #include "SkImagePriv.h"
 #include "SkStream.h"
-
+#include "SkUtils.h"
 
 namespace {
 /**
@@ -20,66 +21,108 @@ namespace {
  */
 class TargetAllocator : public SkBitmap::Allocator {
 public:
-    TargetAllocator(void* target, size_t rowBytes, const SkImageInfo& info)
+    TargetAllocator(void* target,
+                    size_t rowBytes,
+                    int width,
+                    int height,
+                    SkBitmap::Config config)
         : fTarget(target)
         , fRowBytes(rowBytes)
-        , fInfo(info) { }
+        , fWidth(width)
+        , fHeight(height)
+        , fConfig(config) { }
+
+    bool isReady() { return (fTarget != NULL); }
 
     virtual bool allocPixelRef(SkBitmap* bm, SkColorTable* ct) SK_OVERRIDE {
-        if ((SkImageInfoToBitmapConfig(fInfo) != bm->config())
-            || (bm->width() != fInfo.fWidth)
-            || (bm->height() != fInfo.fHeight)) {
-            return false;
+        if ((NULL == fTarget)
+            || (fConfig != bm->config())
+            || (fWidth != bm->width())
+            || (fHeight != bm->height())
+            || (ct != NULL)) {
+            // Call default allcoator.

[scroggo, 2013/12/13 17:23:03]

allocator*

[hal.canary, 2013/12/16 15:10:27]

Done.

+            return bm->allocPixels(NULL, ct);
         }
-        bm->setConfig(bm->config(), bm->width(), bm->height(),
-                      fRowBytes, bm->alphaType());
-        bm->setPixels(fTarget, ct);
+        // make sure fRowBytes is correct.
+        bm->setConfig(fConfig, fWidth, fHeight, fRowBytes, bm->alphaType());
+        // TODO(halcanary): verify that all callers of this function
+        // will respect new RowBytes.  Will be moot once rowbytes belongs
+        // to PixelRef.
+        bm->setPixels(fTarget, NULL);
+        fTarget = NULL;  // never alloc smae pixels twice!

[scroggo, 2013/12/13 17:23:03]

same*

[hal.canary, 2013/12/16 15:10:27]

Done.

         return true;
     }
 
 private:
-    void* fTarget;
-    size_t fRowBytes;
-    SkImageInfo fInfo;
+    void* fTarget;  // Pointer to block of memory.

[scroggo, 2013/12/13 17:23:03]

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;

[hal.canary, 2013/12/16 15:10:27]

Done.

+    size_t fRowBytes;  // Will make sure target bitmap knows this.
+    int fWidth;  // Verify that the target bitmap
+    int fHeight;  //  has correct dimensions.
+    SkBitmap::Config fConfig;  // And correct config.
     typedef SkBitmap::Allocator INHERITED;
 };
-}  // namespace
-////////////////////////////////////////////////////////////////////////////////
 
-SkDecodingImageGenerator::SkDecodingImageGenerator(SkData* data)
-    : fData(data)
-    , fHasInfo(false)
-    , fDoCopyTo(false) {
-    SkASSERT(fData != NULL);
-    fStream = SkNEW_ARGS(SkMemoryStream, (fData));
-    SkASSERT(fStream != NULL);
-    SkASSERT(fStream->unique());
-    fData->ref();
-}
+/**
+ *  An implementation of SkImageGenerator that calls into
+ *  SkImageDecoder.  Since the consumers of this class only rely on
+ *  virtual functions, we hide all implementation details inside this
+ *  source file.  This implementaion of getInfo() always returns true.
+ */
+class DecodingImageGenerator : public SkImageGenerator {
+public:
+    DecodingImageGenerator(SkData* data,
+                           SkStreamRewindable* stream,
+                           const SkImageInfo& info,
+                           int sampleSize,
+                           bool ditherImage,
+                           SkBitmap::Config requestedConfig)
+        : fData(data)
+        , fStream(stream)
+        , fInfo(info)
+        , fSampleSize(sampleSize)
+        , fDitherImage(ditherImage)
+        , fRequestedConfig(requestedConfig) {
+        SkASSERT(stream != NULL);
+        SkSafeRef(fData);  // may be NULL.
+    }
+    virtual ~DecodingImageGenerator() {
+        SkSafeUnref(fData);
+        fStream->unref();
+    }
 
-SkDecodingImageGenerator::SkDecodingImageGenerator(SkStreamRewindable* stream)
-    : fData(NULL)
-    , fStream(stream)
-    , fHasInfo(false)
-    , fDoCopyTo(false) {
-    SkASSERT(fStream != NULL);
-    SkASSERT(fStream->unique());
-}
+    virtual SkData* refEncodedData() SK_OVERRIDE;
 
-SkDecodingImageGenerator::~SkDecodingImageGenerator() {
-    SkSafeUnref(fData);
-    fStream->unref();
-}
+    virtual bool getInfo(SkImageInfo* info) SK_OVERRIDE {
+        if (info != NULL) {
+            *info = fInfo;
+        }
+        return true;
+    }
+
+    virtual bool getPixels(const SkImageInfo& info,
+                           void* pixels,
+                           size_t rowBytes) SK_OVERRIDE;
+
+private:
+    SkData*                fData;
+    SkStreamRewindable*    fStream;
+    const SkImageInfo      fInfo;
+    const int              fSampleSize;
+    const bool             fDitherImage;
+    const SkBitmap::Config fRequestedConfig;
+    typedef SkImageGenerator INHERITED;
+};
 
-// TODO(halcanary): Give this macro a better name and move it into SkTypes.h
 #ifdef SK_DEBUG
     #define SkCheckResult(expr, value)  SkASSERT((value) == (expr))
 #else
     #define SkCheckResult(expr, value)  (void)(expr)
 #endif
 
-SkData* SkDecodingImageGenerator::refEncodedData() {
+
+SkData* DecodingImageGenerator::refEncodedData() {
     // This functionality is used in `gm --serialize`
+    // Does not encode options.
     if (fData != NULL) {
         return SkSafeRef(fData);
     }
@@ -98,111 +141,180 @@ SkData* SkDecodingImageGenerator::refEncodedData() {
     return SkSafeRef(fData);
 }
 
-bool SkDecodingImageGenerator::getInfo(SkImageInfo* info) {
-    // info can be NULL.  If so, will update fInfo, fDoCopyTo, and fHasInfo.
-    if (fHasInfo) {
-        if (info != NULL) {
-            *info = fInfo;
-        }
-        return true;
+// TODO(halcanary) move this function into a standard header.
+static bool config_to_colorType(SkBitmap::Config config, SkColorType* ctOut) {
+    SkColorType ct;
+    switch (config) {
+        case SkBitmap::kA8_Config:
+            ct = kAlpha_8_SkColorType;
+            break;
+        case SkBitmap::kIndex8_Config:
+            ct = kIndex_8_SkColorType;
+            break;
+        case SkBitmap::kRGB_565_Config:
+            ct = kRGB_565_SkColorType;
+            break;
+        case SkBitmap::kARGB_4444_Config:
+            ct = kARGB_4444_SkColorType;
+            break;
+        case SkBitmap::kARGB_8888_Config:
+            ct = kPMColor_SkColorType;
+            break;
+        case SkBitmap::kNo_Config:
+        default:
+            return false;
     }
-    SkAssertResult(fStream->rewind());
-    SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(fStream));
+    if (ctOut) {
+        *ctOut = ct;
+    }
+    return true;
+}
+
+// A contructor-type function that returns NULL on failure.  This
+// prevents the returned SkImageGenerator from ever being in a bad
+// state.
+static SkImageGenerator* create_decoding_generator(SkData* data,
+                                                   SkStreamRewindable* stream,
+                                                   const SkDecoderOptions& opts) {
+    SkASSERT(stream);
+    SkAutoTUnref<SkStreamRewindable> autoStream(stream);  // always unref this.
+    if (SkBitmap::kIndex8_Config == opts.fRequestedConfig) {
+        // We do not support indexed color with SkImageGenerators,
+        return NULL;
+    }
+    SkAssertResult(autoStream->rewind());
+    SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(autoStream));
     if (NULL == decoder.get()) {
-        return false;
+        return NULL;
     }
     SkBitmap bitmap;
-    if (!decoder->decode(fStream, &bitmap,
+    decoder->setSampleSize(opts.fSampleSize);
+    if (!decoder->decode(stream, &bitmap,
                          SkImageDecoder::kDecodeBounds_Mode)) {
-        return false;
+        return NULL;
     }
     if (bitmap.config() == SkBitmap::kNo_Config) {
-        return false;
+        return NULL;
     }
-    if (!bitmap.asImageInfo(&fInfo)) {
-        // We can't use bitmap.config() as is.
-        if (!bitmap.canCopyTo(SkBitmap::kARGB_8888_Config)) {
-            SkDEBUGFAIL("!bitmap->canCopyTo(SkBitmap::kARGB_8888_Config)");
-            return false;
+
+    SkImageInfo info;
+    SkBitmap::Config config = opts.fRequestedConfig;
+    if (opts.fRequestedConfig == SkBitmap::kNo_Config) {
+        // Use default config.
+        if (SkBitmap::kIndex8_Config == bitmap.config()) {
+            // We don't support kIndex8 because we don't support
+            // colortables in this workflow.
+            config = SkBitmap::kARGB_8888_Config;
+            info.fWidth = bitmap.width();
+            info.fHeight = bitmap.height();
+            info.fColorType = kPMColor_SkColorType;
+            info.fAlphaType = bitmap.alphaType();
+        } else {
+            config = bitmap.config();  // Save for later!
+            if (!bitmap.asImageInfo(&info)) {
+                SkDEBUGFAIL("Getting SkImageInfo from bitmap failed.");
+                return NULL;
+            }
         }
-        fDoCopyTo = true;
-        fInfo.fWidth = bitmap.width();
-        fInfo.fHeight = bitmap.height();
-        fInfo.fColorType = kPMColor_SkColorType;
-        fInfo.fAlphaType = bitmap.alphaType();
-    }
-    if (info != NULL) {
-        *info = fInfo;
+    } else {
+        if (!bitmap.canCopyTo(opts.fRequestedConfig)) {
+            SkASSERT(SkBitmap::kARGB_4444_Config == opts.fRequestedConfig);

[scroggo, 2013/12/13 17:23:03]

I don't think this assert belongs here. What if canCopyTo changes in the future?

[hal.canary, 2013/12/16 15:10:27]

Done.

+            SkASSERT(bitmap.config() != opts.fRequestedConfig);
+            return NULL;  // Can not translate to needed config.
+        }
+        info.fWidth = bitmap.width();
+        info.fHeight = bitmap.height();
+        if (!config_to_colorType(opts.fRequestedConfig,
+                                 &(info.fColorType))) {
+            SkDEBUGFAIL("SkBitmapConfigToColorType fails.");
+            return NULL;
+        }
+        info.fAlphaType = bitmap.alphaType();
     }
-    fHasInfo = true;
-    return true;
+    return SkNEW_ARGS(DecodingImageGenerator,
+                      (data, autoStream.detach(), info,
+                       opts.fSampleSize, opts.fDitherImage, config));
 }
 
-bool SkDecodingImageGenerator::getPixels(const SkImageInfo& info,
-                                         void* pixels,
-                                         size_t rowBytes) {
+inline bool check_alpha(SkAlphaType reported, SkAlphaType actual) {
+    return ((reported == actual)
+            || ((reported == kPremul_SkAlphaType)
+                && (actual == kOpaque_SkAlphaType)));
+}
+
+bool DecodingImageGenerator::getPixels(const SkImageInfo& info,
+                                       void* pixels,
+                                       size_t rowBytes) {
     if (NULL == pixels) {
         return false;
     }
-    if (!this->getInfo(NULL)) {
-        return false;
-    }
-    if (SkImageInfoToBitmapConfig(info) == SkBitmap::kNo_Config) {
-        return false;  // Unsupported SkColorType.
-    }
-    SkAssertResult(fStream->rewind());
-    SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(fStream));
-    if (NULL == decoder.get()) {
-        return false;
-    }
     if (fInfo != info) {
-        // The caller has specified a different info.  For now, this
-        // is an error.  In the future, we will check to see if we can
-        // convert.
+        // 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;
     }
-    int bpp = SkBitmap::ComputeBytesPerPixel(SkImageInfoToBitmapConfig(info));
+    int bpp = SkBitmap::ComputeBytesPerPixel(fRequestedConfig);
     if (static_cast<size_t>(bpp * info.fWidth) > rowBytes) {
+        // The caller has specified a bad rowBytes.
         return false;
     }
-    SkBitmap bitmap;
-    if (!bitmap.setConfig(info, rowBytes)) {
+
+    SkAssertResult(fStream->rewind());
+    SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(fStream));
+    if (NULL == decoder.get()) {
         return false;
     }
+    decoder->setDitherImage(fDitherImage);
+    decoder->setSampleSize(fSampleSize);
 
-    TargetAllocator allocator(pixels, rowBytes, info);
-    if (!fDoCopyTo) {
-        decoder->setAllocator(&allocator);
-    }
-    bool success = decoder->decode(fStream, &bitmap,
+    SkBitmap bitmap;
+    TargetAllocator allocator(pixels, rowBytes, info.fWidth,
+                              info.fHeight, fRequestedConfig);
+    decoder->setAllocator(&allocator);
+    bool success = decoder->decode(fStream, &bitmap, fRequestedConfig,
                                    SkImageDecoder::kDecodePixels_Mode);
     decoder->setAllocator(NULL);
     if (!success) {
         return false;
     }
-    if (fDoCopyTo) {
-        SkBitmap bm8888;
-        bitmap.copyTo(&bm8888, SkBitmap::kARGB_8888_Config, &allocator);
+    if (allocator.isReady()) {  // Did not use pixels!
+        SkBitmap bm;
+        SkASSERT(bitmap.canCopyTo(fRequestedConfig));
+        bitmap.copyTo(&bm, fRequestedConfig, &allocator);

[scroggo, 2013/12/13 17:23:03]

Check the return value of copyTo?

[hal.canary, 2013/12/16 15:10:27]

Done.

+        if (allocator.isReady()) {
+            SkDEBUGFAIL("bitmap.copyTo(requestedConfig) failed.");
+            return false;
+        }
+        SkASSERT(check_alpha(fInfo.fAlphaType, bm.alphaType()));
+    } else {
+        SkASSERT(check_alpha(fInfo.fAlphaType, bitmap.alphaType()));
     }
     return true;
 }
-bool SkDecodingImageGenerator::Install(SkData* data, SkBitmap* dst,
-                                       SkDiscardableMemory::Factory* factory) {
+}  // namespace
+////////////////////////////////////////////////////////////////////////////////
+
+SkImageGenerator* SkNewDecodingImageGenerator(SkData* data,
+                                              const SkDecoderOptions& opts) {
     SkASSERT(data != NULL);
-    SkASSERT(dst != NULL);
-    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (data)));
-    return SkInstallDiscardablePixelRef(gen, dst, factory);
+    if (NULL == data) {
+        return NULL;
+    }
+    SkStreamRewindable* stream = SkNEW_ARGS(SkMemoryStream, (data));
+    SkASSERT(stream != NULL);
+    SkASSERT(stream->unique());
+    return create_decoding_generator(data, stream, opts);
 }
 
-bool SkDecodingImageGenerator::Install(SkStreamRewindable* stream,
-                                       SkBitmap* dst,
-                                       SkDiscardableMemory::Factory* factory) {
+SkImageGenerator* SkNewDecodingImageGenerator(SkStreamRewindable* stream,
+                                              const SkDecoderOptions& opts) {
     SkASSERT(stream != NULL);
-    SkASSERT(dst != NULL);
+    SkASSERT(stream->unique());
     if ((stream == NULL) || !stream->unique()) {
         SkSafeUnref(stream);
         return false;
     }
-    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (stream)));
-    return SkInstallDiscardablePixelRef(gen, dst, factory);
+    return create_decoding_generator(NULL, stream, opts);
 }
+