Test region decoding in skimage, plus fixes.

Add tests in skimage to perform region decoding. Write out a
PNG of the region as well as a bitmap obtained with extractSubset
for comparison.

Rename decodeRegion to decodeSubset, so it will not be confused
with SkRegion. (Leave a function called decodeRegion which calls
decodeSubset.)

Clean up some comments.

Use png_set_interlaced_pass instead of modifying pass directly.

Make some changes to region decoding to fix problems I discovered
during testing:

Only call getAddr within a valid range.
Check for a NULL fInputStream.
Return a boolean for whether cropBitmap succeeded.
In cropBitmap, do not attempt to draw to a bitmap to an Index8
bitmap, which crashes. Use extractSubset instead.
Remove an assert.

R=djsollen@google.com

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

git-svn-id: http://skia.googlecode.com/svn/trunk@8996 2bbb7eff-a529-9590-31e7-b0007b416f81

6 files changed, 158 insertions, 31 deletions

diff --git a/include/images/SkImageDecoder.h b/include/images/SkImageDecoder.h
index 54176bd9b9..f6f64e15b3 100644
--- a/include/images/SkImageDecoder.h
+++ b/include/images/SkImageDecoder.h
@@ -216,13 +216,21 @@ public:
     bool buildTileIndex(SkStream*, int *width, int *height);
 
     /**
-     * Decode a rectangle region in the image specified by rect.
+     * Decode a rectangle subset in the image.
      * The method can only be called after buildTileIndex().
      *
      * Return true for success.
      * Return false if the index is never built or failing in decoding.
      */
-    bool decodeRegion(SkBitmap* bitmap, const SkIRect& rect, SkBitmap::Config pref);
+    bool decodeSubset(SkBitmap* bm, const SkIRect& subset, SkBitmap::Config pref);
+
+    /**
+     *  @Deprecated
+     *  Use decodeSubset instead.
+     */
+    bool decodeRegion(SkBitmap* bitmap, const SkIRect& rect, SkBitmap::Config pref) {
+        return this->decodeSubset(bitmap, rect, pref);
+    }
 
     /** Given a stream, this will try to find an appropriate decoder object.
         If none is found, the method returns NULL.
@@ -344,7 +352,7 @@ protected:
 
     // If the decoder wants to support tiled based decoding,
     // this method must be overridden. This guy is called by decodeRegion(...)
-    virtual bool onDecodeRegion(SkBitmap* bitmap, const SkIRect& rect) {
+    virtual bool onDecodeSubset(SkBitmap* bitmap, const SkIRect& rect) {
         return false;
     }
 
@@ -359,10 +367,11 @@ protected:
      * @param (dstX, dstY) the upper-left point of the dest bitmap in terms of
      *                     the coordinate in the original bitmap.
      * @param (width, height) the width and height of the unsampled dst.
-     * @param (srcX, srcY) the upper-left point of the src bitimap in terms of
+     * @param (srcX, srcY) the upper-left point of the src bitmap in terms of
      *                     the coordinate in the original bitmap.
+     * @return bool Whether or not it succeeded.
      */
-    void cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
+    bool cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
                     int dstX, int dstY, int width, int height,
                     int srcX, int srcY);
 
diff --git a/src/images/SkImageDecoder.cpp b/src/images/SkImageDecoder.cpp
index de0bc14c90..9ffae5f0e7 100644
--- a/src/images/SkImageDecoder.cpp
+++ b/src/images/SkImageDecoder.cpp
@@ -33,9 +33,14 @@ void SkImageDecoder::SetDeviceConfig(SkBitmap::Config config)
 ///////////////////////////////////////////////////////////////////////////////
 
 SkImageDecoder::SkImageDecoder()
-    : fPeeker(NULL), fChooser(NULL), fAllocator(NULL), fSampleSize(1),
-      fDefaultPref(SkBitmap::kNo_Config), fDitherImage(true),
-      fUsePrefTable(false),fPreferQualityOverSpeed(false) {
+    : fPeeker(NULL)
+    , fChooser(NULL)
+    , fAllocator(NULL)
+    , fSampleSize(1)
+    , fDefaultPref(SkBitmap::kNo_Config)
+    , fDitherImage(true)
+    , fUsePrefTable(false)
+    , fPreferQualityOverSpeed(false) {
 }
 
 SkImageDecoder::~SkImageDecoder() {
@@ -177,14 +182,14 @@ bool SkImageDecoder::decode(SkStream* stream, SkBitmap* bm,
     return true;
 }
 
-bool SkImageDecoder::decodeRegion(SkBitmap* bm, const SkIRect& rect,
+bool SkImageDecoder::decodeSubset(SkBitmap* bm, const SkIRect& rect,
                                   SkBitmap::Config pref) {
-    // we reset this to false before calling onDecodeRegion
+    // we reset this to false before calling onDecodeSubset
     fShouldCancelDecode = false;
     // assign this, for use by getPrefConfig(), in case fUsePrefTable is false
     fDefaultPref = pref;
 
-    return this->onDecodeRegion(bm, rect);
+    return this->onDecodeSubset(bm, rect);
 }
 
 bool SkImageDecoder::buildTileIndex(SkStream* stream,
@@ -195,11 +200,25 @@ bool SkImageDecoder::buildTileIndex(SkStream* stream,
     return this->onBuildTileIndex(stream, width, height);
 }
 
-void SkImageDecoder::cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
-                int dstX, int dstY, int width, int height,
-                int srcX, int srcY) {
+bool SkImageDecoder::cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
+                                int dstX, int dstY, int width, int height,
+                                int srcX, int srcY) {
     int w = width / sampleSize;
     int h = height / sampleSize;
+    if (src->getConfig() == SkBitmap::kIndex8_Config) {
+        // kIndex8 does not allow drawing via an SkCanvas, as is done below.
+        // Instead, use extractSubset. Note that this shares the SkPixelRef and
+        // SkColorTable.
+        // FIXME: Since src is discarded in practice, this holds on to more
+        // pixels than is strictly necessary. Switch to a copy if memory
+        // savings are more important than speed here. This also means
+        // that the pixels in dst can not be reused (though there is no
+        // allocation, which was already done on src).
+        int x = (dstX - srcX) / sampleSize;
+        int y = (dstY - srcY) / sampleSize;
+        SkIRect subset = SkIRect::MakeXYWH(x, y, w, h);
+        return src->extractSubset(dst, subset);
+    }
     // if the destination has no pixels then we must allocate them.
     if (dst->isNull()) {
         dst->setConfig(src->getConfig(), w, h);
@@ -207,13 +226,15 @@ void SkImageDecoder::cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
 
         if (!this->allocPixelRef(dst, NULL)) {
             SkDEBUGF(("failed to allocate pixels needed to crop the bitmap"));
-            return;
+            return false;
         }
     }
     // check to see if the destination is large enough to decode the desired
     // region. If this assert fails we will just draw as much of the source
     // into the destination that we can.
-    SkASSERT(dst->width() >= w && dst->height() >= h);
+    if (dst->width() < w || dst->height() < h) {
+        SkDEBUGF(("SkImageDecoder::cropBitmap does not have a large enough bitmap.\n"));
+    }
 
     // Set the Src_Mode for the paint to prevent transparency issue in the
     // dest in the event that the dest was being re-used.
@@ -224,6 +245,7 @@ void SkImageDecoder::cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
     canvas.drawSprite(*src, (srcX - dstX) / sampleSize,
                             (srcY - dstY) / sampleSize,
                             &paint);
+    return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/src/images/SkImageDecoder_libjpeg.cpp b/src/images/SkImageDecoder_libjpeg.cpp
index c55f4c4123..3ed3806543 100644
--- a/src/images/SkImageDecoder_libjpeg.cpp
+++ b/src/images/SkImageDecoder_libjpeg.cpp
@@ -118,7 +118,7 @@ public:
 protected:
 #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK
     virtual bool onBuildTileIndex(SkStream *stream, int *width, int *height) SK_OVERRIDE;
-    virtual bool onDecodeRegion(SkBitmap* bitmap, const SkIRect& rect) SK_OVERRIDE;
+    virtual bool onDecodeSubset(SkBitmap* bitmap, const SkIRect& rect) SK_OVERRIDE;
 #endif
     virtual bool onDecode(SkStream* stream, SkBitmap* bm, Mode) SK_OVERRIDE;
 
@@ -556,7 +556,7 @@ bool SkJPEGImageDecoder::onBuildTileIndex(SkStream* stream, int *width, int *hei
     return true;
 }
 
-bool SkJPEGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
+bool SkJPEGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     if (NULL == fImageIndex) {
         return false;
     }
diff --git a/src/images/SkImageDecoder_libpng.cpp b/src/images/SkImageDecoder_libpng.cpp
index 0e02ccf08d..d967a69e84 100644
--- a/src/images/SkImageDecoder_libpng.cpp
+++ b/src/images/SkImageDecoder_libpng.cpp
@@ -72,7 +72,7 @@ public:
 protected:
 #ifdef SK_BUILD_FOR_ANDROID
     virtual bool onBuildTileIndex(SkStream *stream, int *width, int *height) SK_OVERRIDE;
-    virtual bool onDecodeRegion(SkBitmap* bitmap, const SkIRect& region) SK_OVERRIDE;
+    virtual bool onDecodeSubset(SkBitmap* bitmap, const SkIRect& region) SK_OVERRIDE;
 #endif
     virtual bool onDecode(SkStream* stream, SkBitmap* bm, Mode) SK_OVERRIDE;
 
@@ -642,7 +642,11 @@ bool SkPNGImageDecoder::onBuildTileIndex(SkStream* sk_stream, int *width, int *h
     return true;
 }
 
-bool SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
+bool SkPNGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
+    if (NULL == fImageIndex) {
+        return false;
+    }
+
     png_structp png_ptr = fImageIndex->png_ptr;
     png_infop info_ptr = fImageIndex->info_ptr;
     if (setjmp(png_jmpbuf(png_ptr))) {
@@ -657,7 +661,7 @@ bool SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
     SkIRect rect = SkIRect::MakeWH(origWidth, origHeight);
 
     if (!rect.intersect(region)) {
-        // If the requested region is entirely outsides the image, just
+        // If the requested region is entirely outside the image, just
         // returns false
         return false;
     }
@@ -731,8 +735,8 @@ bool SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
 #if defined(PNG_1_0_X) || defined (PNG_1_2_X)
     png_ptr->pass = 0;
 #else
-    // FIXME: Figure out what (if anything) to do when Android moves to
-    // libpng > 1.2.
+    // FIXME: This sets pass as desired, but also sets iwidth. Is that ok?
+    png_set_interlaced_pass(png_ptr, 0);
 #endif
     png_read_update_info(png_ptr, info_ptr);
 
@@ -745,7 +749,8 @@ bool SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
                 uint8_t* bmRow = decodedBitmap.getAddr8(0, 0);
                 png_read_rows(png_ptr, &bmRow, png_bytepp_NULL, 1);
             }
-            for (png_uint_32 y = 0; y < origHeight; y++) {
+            png_uint_32 bitmapHeight = (png_uint_32) decodedBitmap.height();
+            for (png_uint_32 y = 0; y < bitmapHeight; y++) {
                 uint8_t* bmRow = decodedBitmap.getAddr8(0, y);
                 png_read_rows(png_ptr, &bmRow, png_bytepp_NULL, 1);
             }
@@ -826,12 +831,10 @@ bool SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
 
     if (swapOnly) {
         bm->swap(decodedBitmap);
-    } else {
-        cropBitmap(bm, &decodedBitmap, sampleSize, region.x(), region.y(),
-                   region.width(), region.height(), 0, rect.y());
+        return true;
     }
-
-    return true;
+    return this->cropBitmap(bm, &decodedBitmap, sampleSize, region.x(), region.y(),
+                            region.width(), region.height(), 0, rect.y());
 }
 #endif
 
diff --git a/src/images/SkImageDecoder_libwebp.cpp b/src/images/SkImageDecoder_libwebp.cpp
index a33d0f9b2f..e623f376d7 100644
--- a/src/images/SkImageDecoder_libwebp.cpp
+++ b/src/images/SkImageDecoder_libwebp.cpp
@@ -111,7 +111,7 @@ public:
 
 protected:
     virtual bool onBuildTileIndex(SkStream *stream, int *width, int *height) SK_OVERRIDE;
-    virtual bool onDecodeRegion(SkBitmap* bitmap, const SkIRect& rect) SK_OVERRIDE;
+    virtual bool onDecodeSubset(SkBitmap* bitmap, const SkIRect& rect) SK_OVERRIDE;
     virtual bool onDecode(SkStream* stream, SkBitmap* bm, Mode) SK_OVERRIDE;
 
 private:
@@ -321,7 +321,7 @@ static bool is_config_compatible(const SkBitmap& bitmap) {
            config == SkBitmap::kARGB_8888_Config;
 }
 
-bool SkWEBPImageDecoder::onDecodeRegion(SkBitmap* decodedBitmap,
+bool SkWEBPImageDecoder::onDecodeSubset(SkBitmap* decodedBitmap,
                                         const SkIRect& region) {
     SkIRect rect = SkIRect::MakeWH(fOrigWidth, fOrigHeight);
 
diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index 7906386cdc..688d3a2275 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -13,6 +13,7 @@
 #include "SkImageDecoder.h"
 #include "SkImageEncoder.h"
 #include "SkOSFile.h"
+#include "SkRandom.h"
 #include "SkStream.h"
 #include "SkTArray.h"
 #include "SkTemplates.h"
@@ -20,6 +21,7 @@
 DEFINE_string2(readPath, r, "", "Folder(s) and files to decode images. Required.");
 DEFINE_string2(writePath, w, "",  "Write rendered images into this directory.");
 DEFINE_bool(reencode, true, "Reencode the images to test encoding.");
+DEFINE_bool(testSubsetDecoding, true, "Test decoding subsets of images.");
 
 struct Format {
     SkImageEncoder::Type    fType;
@@ -92,6 +94,8 @@ static SkTArray<SkString, false> gMissingCodecs;
 static SkTArray<SkString, false> gDecodeFailures;
 static SkTArray<SkString, false> gEncodeFailures;
 static SkTArray<SkString, false> gSuccessfulDecodes;
+static SkTArray<SkString, false> gSuccessfulSubsetDecodes;
+static SkTArray<SkString, false> gFailedSubsetDecodes;
 
 static bool write_bitmap(const char outName[], SkBitmap* bm) {
     SkBitmap bitmap8888;
@@ -112,6 +116,47 @@ static bool write_bitmap(const char outName[], SkBitmap* bm) {
     return SkImageEncoder::EncodeFile(outName, *bm, SkImageEncoder::kPNG_Type, 100);
 }
 
+/**
+ *  Return a random SkIRect inside the range specified.
+ *  @param rand Random number generator.
+ *  @param maxX Exclusive maximum x-coordinate. SkIRect's fLeft and fRight will be
+ *      in the range [0, maxX)
+ *  @param maxY Exclusive maximum y-coordinate. SkIRect's fTop and fBottom will be
+ *      in the range [0, maxY)
+ *  @return SkIRect Non-empty, non-degenerate rectangle.
+ */
+static SkIRect generate_random_rect(SkRandom* rand, int32_t maxX, int32_t maxY) {
+    SkASSERT(maxX > 1 && maxY > 1);
+    int32_t left = rand->nextULessThan(maxX);
+    int32_t right = rand->nextULessThan(maxX);
+    int32_t top = rand->nextULessThan(maxY);
+    int32_t bottom = rand->nextULessThan(maxY);
+    SkIRect rect = SkIRect::MakeLTRB(left, top, right, bottom);
+    rect.sort();
+    // Make sure rect is not empty.
+    if (rect.fLeft == rect.fRight) {
+        if (rect.fLeft > 0) {
+            rect.fLeft--;
+        } else {
+            rect.fRight++;
+            // This branch is only taken if 0 == rect.fRight, and
+            // maxX must be at least 2, so it must still be in
+            // range.
+            SkASSERT(rect.fRight < maxX);
+        }
+    }
+    if (rect.fTop == rect.fBottom) {
+        if (rect.fTop > 0) {
+            rect.fTop--;
+        } else {
+            rect.fBottom++;
+            // Again, this must be in range.
+            SkASSERT(rect.fBottom < maxY);
+        }
+    }
+    return rect;
+}
+
 static void decodeFileAndWrite(const char srcPath[], const SkString* writePath) {
     SkBitmap bitmap;
     SkFILEStream stream(srcPath);
@@ -137,6 +182,49 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
 
     gSuccessfulDecodes.push_back().printf("%s [%d %d]", srcPath, bitmap.width(), bitmap.height());
 
+    if (FLAGS_testSubsetDecoding) {
+        bool couldRewind = stream.rewind();
+        SkASSERT(couldRewind);
+        int width, height;
+        // Build the tile index for decoding subsets. If the image is 1x1, skip subset
+        // decoding since there are no smaller subsets.
+        if (codec->buildTileIndex(&stream, &width, &height) && width > 1 && height > 1) {
+            SkASSERT(bitmap.width() == width && bitmap.height() == height);
+            // Call decodeSubset multiple times:
+            SkRandom rand(0);
+            for (int i = 0; i < 5; i++) {
+                SkBitmap bitmapFromDecodeSubset;
+                // FIXME: Come up with a more representative set of rectangles.
+                SkIRect rect = generate_random_rect(&rand, width, height);
+                SkString subsetDim = SkStringPrintf("[%d,%d,%d,%d]", rect.fLeft, rect.fTop,
+                                                    rect.fRight, rect.fBottom);
+                if (codec->decodeSubset(&bitmapFromDecodeSubset, rect, SkBitmap::kNo_Config)) {
+                    gSuccessfulSubsetDecodes.push_back().printf("Decoded subset %s from %s",
+                                                          subsetDim.c_str(), srcPath);
+                    if (writePath != NULL) {
+                        // Write the region to a file whose name includes the dimensions.
+                        SkString suffix = SkStringPrintf("_%s.png", subsetDim.c_str());
+                        SkString outPath;
+                        make_outname(&outPath, writePath->c_str(), srcPath, suffix.c_str());
+                        bool success = write_bitmap(outPath.c_str(), &bitmapFromDecodeSubset);
+                        SkASSERT(success);
+                        gSuccessfulSubsetDecodes.push_back().printf("\twrote %s", outPath.c_str());
+                        // Also use extractSubset from the original for visual comparison.
+                        SkBitmap extractedSubset;
+                        if (bitmap.extractSubset(&extractedSubset, rect)) {
+                            suffix.printf("_%s_extracted.png", subsetDim.c_str());
+                            make_outname(&outPath, writePath->c_str(), srcPath, suffix.c_str());
+                            success = write_bitmap(outPath.c_str(), &extractedSubset);
+                            SkASSERT(success);
+                        }
+                    }
+                } else {
+                    gFailedSubsetDecodes.push_back().printf("Failed to decode region %s from %s\n",
+                                                            subsetDim.c_str(), srcPath);
+                }
+            }
+        }
+    }
     if (FLAGS_reencode) {
         // Encode to the format the file was originally in, or PNG if the encoder for the same
         // format is unavailable.
@@ -289,6 +377,11 @@ int tool_main(int argc, char** argv) {
     failed |= print_strings("Failed to encode", gEncodeFailures);
     print_strings("Decoded", gSuccessfulDecodes);
 
+    if (FLAGS_testSubsetDecoding) {
+        failed |= print_strings("Failed subset decodes", gFailedSubsetDecodes);
+        print_strings("Decoded subsets", gSuccessfulSubsetDecodes);
+    }
+
     return failed ? -1 : 0;
 }