Refactor SkBitmapRegionDecoderInterface for Android

tools/SkBitmapRegionCodec.cpp

Index: tools/SkBitmapRegionCodec.cpp
diff --git a/tools/SkBitmapRegionCodec.cpp b/tools/SkBitmapRegionCodec.cpp
index 3f2cd24b78fdb44133e634ae68bc096386c00024..3d01fe517334825ace5e14b59525653bebe4d8e4 100644
--- a/tools/SkBitmapRegionCodec.cpp
+++ b/tools/SkBitmapRegionCodec.cpp
@@ -15,129 +15,101 @@ SkBitmapRegionCodec::SkBitmapRegionCodec(SkAndroidCodec* codec)
     , fCodec(codec)
 {}
 
-/*
- * Three differences from the Android version:
- *     Returns a skia bitmap instead of an Android bitmap.
- *     Android version attempts to reuse a recycled bitmap.
- *     Removed the options object and used parameters for color type and sample size.
- */
-// FIXME: Should this function should take in SkIRect?
-SkBitmap* SkBitmapRegionCodec::decodeRegion(int inputX, int inputY, int inputWidth, int inputHeight,
-        int sampleSize, SkColorType dstColorType) {
-
+bool SkBitmapRegionCodec::prepareRegion(const SkIRect& desiredSubset,
+                                         int sampleSize,
+                                         SkColorType dstColorType,
+                                         bool requireUnpremul,
+                                         SkImageInfo* outInfo) {
     // Fix the input sampleSize if necessary.
     if (sampleSize < 1) {
         sampleSize = 1;
     }
+    fSampleSize = sampleSize;
 
-    // The size of the output bitmap is determined by the size of the
-    // requested subset, not by the size of the intersection of the subset
-    // and the image dimensions.
-    // If inputX is negative, we will need to place decoded pixels into the
-    // output bitmap starting at a left offset.  Call this outX.
-    // If outX is non-zero, subsetX must be zero.
-    // If inputY is negative, we will need to place decoded pixels into the
-    // output bitmap starting at a top offset.  Call this outY.
-    // If outY is non-zero, subsetY must be zero.
     int outX;
     int outY;
-    SkIRect subset = SkIRect::MakeXYWH(inputX, inputY, inputWidth, inputHeight);
-    SubsetType type = adjust_subset_rect(fCodec->getInfo().dimensions(), &subset, &outX, &outY);
-    if (SubsetType::kOutside_SubsetType == type) {
-        return nullptr;
+    fSubset = desiredSubset;
+    fSubsetType = adjust_subset_rect(fCodec->getInfo().dimensions(), &fSubset, &outX, &outY);

[scroggo, 2015/10/27 15:00:50]

If you wanted to, you could put an SkIRect on the stack instead of modifying
fSubset directly. That way, if you returned false, fSubset would remain
unmodified. You could take that further and set fSubset to something invalid at
the beginning of this function, so decodeRegion will never be called with some
half-correct fSubset (we'll know to fail as if prepareRegion was never called to
begin with).

+    if (SubsetType::kOutside_SubsetType == fSubsetType) {
+        return false;
     }
 
     // Ask the codec for a scaled subset
-    if (!fCodec->getSupportedSubset(&subset)) {
+    if (!fCodec->getSupportedSubset(&fSubset)) {
         SkCodecPrintf("Error: Could not get subset.\n");
         return nullptr;
     }
-    SkISize scaledSize = fCodec->getSampledSubsetDimensions(sampleSize, subset);
+    SkISize scaledSize = fCodec->getSampledSubsetDimensions(fSampleSize, fSubset);
 
-    // Create the image info for the decode
+    // Create the image info for the decode.
     SkAlphaType dstAlphaType = fCodec->getInfo().alphaType();
-    if (kUnpremul_SkAlphaType == dstAlphaType) {
-        dstAlphaType = kPremul_SkAlphaType;
+    if (kOpaque_SkAlphaType != dstAlphaType) {
+        dstAlphaType = requireUnpremul ? kUnpremul_SkAlphaType : kPremul_SkAlphaType;
     }
-    SkImageInfo decodeInfo = SkImageInfo::Make(scaledSize.width(), scaledSize.height(),
+    fDecodeInfo = SkImageInfo::Make(scaledSize.width(), scaledSize.height(),
             dstColorType, dstAlphaType);
 
-    // Construct a color table for the decode if necessary
-    SkAutoTUnref<SkColorTable> colorTable(nullptr);
-    SkPMColor* colorPtr = nullptr;
-    int* colorCountPtr = nullptr;
-    int maxColors = 256;
-    SkPMColor colors[256];
-    if (kIndex_8_SkColorType == dstColorType) {
-        // TODO (msarett): This performs a copy that is unnecessary since
-        //                 we have not yet initialized the color table.
-        //                 And then we need to use a const cast to get
-        //                 a pointer to the color table that we can
-        //                 modify during the decode.  We could alternatively
-        //                 perform the decode before creating the bitmap and
-        //                 the color table.  We still would need to copy the
-        //                 colors into the color table after the decode.
-        colorTable.reset(new SkColorTable(colors, maxColors));
-        colorPtr = const_cast<SkPMColor*>(colorTable->readColors());
-        colorCountPtr = &maxColors;
-    }
-
-    // Initialize the destination bitmap
-    SkAutoTDelete<SkBitmap> bitmap(new SkBitmap());
-    int scaledOutX = 0;
-    int scaledOutY = 0;
+    // Calculate the output bitmap dimensions.
+    fScaledOutX = 0;
+    fScaledOutY = 0;
     int scaledOutWidth = scaledSize.width();
     int scaledOutHeight = scaledSize.height();
-    if (SubsetType::kPartiallyInside_SubsetType == type) {
-        scaledOutX = outX / sampleSize;
-        scaledOutY = outY / sampleSize;
+    if (SubsetType::kPartiallyInside_SubsetType == fSubsetType) {
+        fScaledOutX = outX / sampleSize;
+        fScaledOutY = outY / sampleSize;
         // We need to be safe here because getSupportedSubset() may have modified the subset.
-        const int extraX = SkTMax(0, inputWidth - outX - subset.width());
-        const int extraY = SkTMax(0, inputHeight - outY - subset.height());
-        const int scaledExtraX = extraX / sampleSize;
-        const int scaledExtraY = extraY / sampleSize;
-        scaledOutWidth += scaledOutX + scaledExtraX;
-        scaledOutHeight += scaledOutY + scaledExtraY;
+        const int extraX = SkTMax(0, desiredSubset.width() - outX - fSubset.width());
+        const int extraY = SkTMax(0, desiredSubset.height() - outY - fSubset.height());
+        const int scaledExtraX = extraX / fSampleSize;
+        const int scaledExtraY = extraY / fSampleSize;
+        scaledOutWidth += fScaledOutX + scaledExtraX;
+        scaledOutHeight += fScaledOutY + scaledExtraY;
     }
-    SkImageInfo outInfo = decodeInfo.makeWH(scaledOutWidth, scaledOutHeight);
-    if (!bitmap->tryAllocPixels(outInfo, nullptr, colorTable.get())) {
-        SkCodecPrintf("Error: Could not allocate pixels.\n");
-        return nullptr;
+    fOutInfo = fDecodeInfo.makeWH(scaledOutWidth, scaledOutHeight);
+    *outInfo = fOutInfo;
+
+    return conversion_possible(fOutInfo, fCodec->getInfo());
+}
+
+bool SkBitmapRegionCodec::decodeRegion(SkBitmap& bitmap) {
+    if (fOutInfo.isEmpty()) {
+        SkCodecPrintf("Error: Call prepareRegion() first.");
+        return false;
+    }
+
+    if (bitmap.width() < fOutInfo.width() || bitmap.height() < fOutInfo.height() ||

[scroggo, 2015/10/27 15:00:50]

Should this just check whether bitmap.info() == fOutInfo? Down below it looks
like you're assuming that they used fOutInfo

+            bitmap.colorType() != fOutInfo.colorType()) {
+        SkCodecPrintf("Error: Invalid size or color type for input bitmap.");
+        return false;
     }
+    bitmap.setAlphaType(fOutInfo.alphaType());
 
     // Zero the bitmap if the region is not completely within the image.
     // TODO (msarett): Can we make this faster by implementing it to only
     //                 zero parts of the image that we won't overwrite with
     //                 pixels?
     // TODO (msarett): This could be skipped if memory is zero initialized.
-    //                 This would matter if this code is moved to Android and
-    //                 uses Android bitmaps.
-    if (SubsetType::kPartiallyInside_SubsetType == type) {
-        void* pixels = bitmap->getPixels();
-        size_t bytes = outInfo.getSafeSize(bitmap->rowBytes());
+    if (SubsetType::kPartiallyInside_SubsetType == fSubsetType) {
+        void* pixels = bitmap.getPixels();
+        size_t bytes = fOutInfo.getSafeSize(bitmap.rowBytes());
         memset(pixels, 0, bytes);
     }
 
     // Decode into the destination bitmap
     SkAndroidCodec::AndroidOptions options;
-    options.fSampleSize = sampleSize;
-    options.fSubset = ⊂
-    options.fColorPtr = colorPtr;
-    options.fColorCount = colorCountPtr;
-    void* dst = bitmap->getAddr(scaledOutX, scaledOutY);
-    size_t rowBytes = bitmap->rowBytes();
-    SkCodec::Result result = fCodec->getAndroidPixels(decodeInfo, dst, rowBytes, &options);
+    options.fSampleSize = fSampleSize;
+    options.fSubset = &fSubset;
+    SkColorTable* colorTable = bitmap.getColorTable();
+    options.fColorPtr = colorTable ? const_cast<SkPMColor*>(colorTable->readColors()) : nullptr;
+    int numColors = 0;
+    options.fColorCount = &numColors;
+    void* dst = bitmap.getAddr(fScaledOutX, fScaledOutY);
+    size_t rowBytes = bitmap.rowBytes();
+    SkCodec::Result result = fCodec->getAndroidPixels(fDecodeInfo, dst, rowBytes, &options);
     if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) {
         SkCodecPrintf("Error: Could not get pixels.\n");
-        return nullptr;
+        return false;
     }
 
-    return bitmap.detach();
-}
-
-bool SkBitmapRegionCodec::conversionSupported(SkColorType colorType) {
-    // FIXME: Call virtual function when it lands.
-    SkImageInfo info = SkImageInfo::Make(0, 0, colorType, fCodec->getInfo().alphaType(),
-            fCodec->getInfo().profileType());
-    return conversion_possible(info, fCodec->getInfo());
+    return true;
 }