From: scroggo@google.com Date: Fri, 3 May 2013 20:14:28 +0000 (+0000) Subject: Test region decoding in skimage, plus fixes. X-Git-Tag: accepted/tizen/5.0/unified/20181102.025319~12508 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=7e6fceeffd250d99eff9f1dbb459a916ae4a754e;p=platform%2Fupstream%2FlibSkiaSharp.git 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 --- diff --git a/include/images/SkImageDecoder.h b/include/images/SkImageDecoder.h index 54176bd..f6f64e1 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 de0bc14..9ffae5f 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 c55f4c4..3ed3806 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 0e02ccf..d967a69 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 a33d0f9..e623f37 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 7906386..688d3a2 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 gMissingCodecs; static SkTArray gDecodeFailures; static SkTArray gEncodeFailures; static SkTArray gSuccessfulDecodes; +static SkTArray gSuccessfulSubsetDecodes; +static SkTArray 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; }