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..8b612057b75e027a6400648a3a94e3423ea91946 100644
--- a/src/images/SkDecodingImageGenerator.cpp
+++ b/src/images/SkDecodingImageGenerator.cpp
@@ -11,7 +11,7 @@
 #include "SkImageGenerator.h"
 #include "SkImagePriv.h"
 #include "SkStream.h"
-
+#include "SkUtils.h"
 
 namespace {
 /**
@@ -20,50 +20,77 @@ 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)

[scroggo, 2013/12/12 22:33:46]

Comments would be nice here, explaining the parameters.

[hal.canary, 2013/12/13 15:48:48]

Done.

         : 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)

[scroggo, 2013/12/12 22:33:46]

When will fTarget be NULL and called?

[hal.canary, 2013/12/13 15:48:48]

probably shouldn't happen.  but a weird decoder could try and use the allocator
multiple times.

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

What should we do in that situation? I think the client will want the final
bitmap to be allocated via the allocator, but in this case, the first
(presumably thrown away) bitmap was allocated via the allocator.

+            || (fConfig != bm->config())
+            || (fWidth != bm->width())

[scroggo, 2013/12/12 22:33:46]

It seems you're mixing uses of this if statement. If the configs don't match,
then we should use the default allocator so we can later use fTarget in copyTo.
If the width or height don't match, isn't that an error? Likewise, I suspect the
ct will never be non NULL unless the configs don't match (since you're
explicitly disallowing Index8).

[hal.canary, 2013/12/13 15:48:48]

I just want to assert that everyone agrees on what the pixels are before we
write to them.

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

You don't assert anything - you allocate new pixels not using fTarget, which may
or may not be what the caller expected.

+            || (fHeight != bm->height())
+            || (ct != NULL)) {
+            return bm->allocPixels(NULL, ct);
         }
-        bm->setConfig(bm->config(), bm->width(), bm->height(),
-                      fRowBytes, bm->alphaType());
-        bm->setPixels(fTarget, ct);
+        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;
         return true;
     }
 
 private:
     void* fTarget;
     size_t fRowBytes;
-    SkImageInfo fInfo;
+    int fWidth;
+    int fHeight;
+    SkBitmap::Config fConfig;
     typedef SkBitmap::Allocator INHERITED;
 };
 }  // namespace
 ////////////////////////////////////////////////////////////////////////////////
 
-SkDecodingImageGenerator::SkDecodingImageGenerator(SkData* data)
+SkDecodingImageGenerator::SkDecodingImageGenerator(SkData* data,
+                                                   const Options* opts)
     : fData(data)
     , fHasInfo(false)
-    , fDoCopyTo(false) {
+    , fDoCopyTo(false)
+    , fOpts() {
     SkASSERT(fData != NULL);
     fStream = SkNEW_ARGS(SkMemoryStream, (fData));
     SkASSERT(fStream != NULL);
     SkASSERT(fStream->unique());
     fData->ref();
+    if (opts != NULL) {
+        fOpts = *opts;
+        SkASSERT(fOpts.fRequestedConfig != SkBitmap::kIndex8_Config);
+    }
 }
 
-SkDecodingImageGenerator::SkDecodingImageGenerator(SkStreamRewindable* stream)
+SkDecodingImageGenerator::SkDecodingImageGenerator(SkStreamRewindable* stream,
+                                                   const Options* opts)
     : fData(NULL)
     , fStream(stream)
     , fHasInfo(false)
-    , fDoCopyTo(false) {
+    , fDoCopyTo(false)
+    , fOpts() {
     SkASSERT(fStream != NULL);
     SkASSERT(fStream->unique());
+    if (opts != NULL) {
+        fOpts = *opts;
+        SkASSERT(fOpts.fRequestedConfig != SkBitmap::kIndex8_Config);
+    }
 }
 
 SkDecodingImageGenerator::~SkDecodingImageGenerator() {
@@ -80,6 +107,7 @@ SkDecodingImageGenerator::~SkDecodingImageGenerator() {
 
 SkData* SkDecodingImageGenerator::refEncodedData() {
     // This functionality is used in `gm --serialize`
+    // Does not encode options.
     if (fData != NULL) {
         return SkSafeRef(fData);
     }
@@ -98,7 +126,43 @@ SkData* SkDecodingImageGenerator::refEncodedData() {
     return SkSafeRef(fData);
 }
 
+// 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;
+    }
+    if (ctOut) {
+        *ctOut = ct;
+    }
+    return true;
+}
+
+
 bool SkDecodingImageGenerator::getInfo(SkImageInfo* info) {
+    if (SkBitmap::kIndex8_Config == fOpts.fRequestedConfig) {
+        // We do not support indexed color with SkImageGenerators,
+        SkDEBUGFAIL("SkBitmap::kIndex8_Config == fOpts.fRequestedConfig");
+        return false;
+    }
+
     // info can be NULL.  If so, will update fInfo, fDoCopyTo, and fHasInfo.
     if (fHasInfo) {
         if (info != NULL) {
@@ -111,7 +175,9 @@ bool SkDecodingImageGenerator::getInfo(SkImageInfo* info) {
     if (NULL == decoder.get()) {
         return false;
     }
+
     SkBitmap bitmap;
+    decoder->setSampleSize(fOpts.fSampleSize);
     if (!decoder->decode(fStream, &bitmap,
                          SkImageDecoder::kDecodeBounds_Mode)) {
         return false;
@@ -119,16 +185,37 @@ bool SkDecodingImageGenerator::getInfo(SkImageInfo* info) {
     if (bitmap.config() == SkBitmap::kNo_Config) {
         return false;
     }
-    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;
+
+    if (fOpts.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.
+            fOpts.fRequestedConfig = SkBitmap::kARGB_8888_Config;
+            fInfo.fWidth = bitmap.width();
+            fInfo.fHeight = bitmap.height();
+            fInfo.fColorType = kPMColor_SkColorType;
+            fInfo.fAlphaType = bitmap.alphaType();
+        } else {
+            fOpts.fRequestedConfig = bitmap.config();  // Save for later!
+            if (!bitmap.asImageInfo(&fInfo)) {
+                SkDEBUGFAIL("!bitmap.asImageInfo() && !kNoConfig");

[scroggo, 2013/12/12 22:33:46]

Can you make this message more informative?

[hal.canary, 2013/12/13 15:48:48]

Done.

+                return false;
+            }
+        }
+    } else {
+        if (!bitmap.canCopyTo(fOpts.fRequestedConfig)) {
+            SkASSERT(SkBitmap::kARGB_4444_Config == fOpts.fRequestedConfig);
+            SkASSERT(bitmap.config() != fOpts.fRequestedConfig);
+            return false;  // Can not translate to needed config.
         }
-        fDoCopyTo = true;
         fInfo.fWidth = bitmap.width();
         fInfo.fHeight = bitmap.height();
-        fInfo.fColorType = kPMColor_SkColorType;
+        if (!config_to_colorType(fOpts.fRequestedConfig,
+                                 &(fInfo.fColorType))) {
+            SkDEBUGFAIL("SkBitmapConfigToColorType fails.");
+            return false;
+        }
         fInfo.fAlphaType = bitmap.alphaType();
     }
     if (info != NULL) {
@@ -138,64 +225,82 @@ bool SkDecodingImageGenerator::getInfo(SkImageInfo* info) {
     return true;
 }
 
+static inline bool check_alpha(SkAlphaType reported, SkAlphaType actual) {
+    return ((reported == actual)
+            || ((reported == kPremul_SkAlphaType)
+                && (actual == kOpaque_SkAlphaType)));
+}
+
 bool SkDecodingImageGenerator::getPixels(const SkImageInfo& info,
                                          void* pixels,
                                          size_t rowBytes) {
     if (NULL == pixels) {
         return false;
     }
-    if (!this->getInfo(NULL)) {
+    if (!this->getInfo(NULL)) {  // Verify that fInfo populated.
         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(fOpts.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(fOpts.fDitherImage);
+    decoder->setPreferQualityOverSpeed(fOpts.fPreferQualityOverSpeed);
+    decoder->setSampleSize(fOpts.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, fOpts.fRequestedConfig);
+    decoder->setAllocator(&allocator);
+    bool success = decoder->decode(fStream, &bitmap, fOpts.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(fOpts.fRequestedConfig));
+        bitmap.copyTo(&bm, fOpts.fRequestedConfig, &allocator);
+        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,
+                                       const Options* opts,
                                        SkDiscardableMemory::Factory* factory) {
     SkASSERT(data != NULL);
     SkASSERT(dst != NULL);
-    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (data)));
+    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (data, opts)));
     return SkInstallDiscardablePixelRef(gen, dst, factory);
 }
 
 bool SkDecodingImageGenerator::Install(SkStreamRewindable* stream,
                                        SkBitmap* dst,
+                                       const Options* opts,
                                        SkDiscardableMemory::Factory* factory) {
     SkASSERT(stream != NULL);
     SkASSERT(dst != NULL);
@@ -203,6 +308,6 @@ bool SkDecodingImageGenerator::Install(SkStreamRewindable* stream,
         SkSafeUnref(stream);
         return false;
     }
-    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (stream)));
+    SkImageGenerator* gen(SkNEW_ARGS(SkDecodingImageGenerator, (stream, opts)));
     return SkInstallDiscardablePixelRef(gen, dst, factory);
 }