From 86bff1f008308267c4ff75456b202a9cf60c6193 Mon Sep 17 00:00:00 2001 From: "wjmaclean@chromium.org" Date: Tue, 16 Nov 2010 20:22:41 +0000 Subject: [PATCH] Add safe size/copy functions to Skia. This patch adds four methods to SkBitmap. There are two functions to return "safe size", defined as the number of pixels from the value returned by getPixels() to the end of the allocated buffer. There is one version of fillPixels() to copy the bitmap instance into an external buffer (with specified size, and using specified stride), and another fillPixels() to copy from an external buffer to the instance bitmap. In the latter case the specified height, width and pixel format must match that used by the bitmap instance, although the specified stride may be any value at least as large as the minimum stride for the specified geometry. It is assumed that the external buffer is of size at least (height - 1)*stride + width * bytesPerPixel. Both fillPixels() functions return false if the copy is not possible with the specified parameters. Review URL: http://codereview.appspot.com/2837041/ git-svn-id: http://skia.googlecode.com/svn/trunk@625 2bbb7eff-a529-9590-31e7-b0007b416f81 --- include/core/SkBitmap.h | 64 ++++++- src/core/SkBitmap.cpp | 118 ++++++++++++ tests/BitmapCopyTest.cpp | 401 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 580 insertions(+), 3 deletions(-) diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h index 1f6f5a6b03..4c12b673de 100644 --- a/include/core/SkBitmap.h +++ b/include/core/SkBitmap.h @@ -132,6 +132,12 @@ public: */ size_t getSize() const { return fHeight * fRowBytes; } + /** Return the number of bytes from the pointer returned by getPixels() + to the end of the allocated space in the buffer. Required in + cases where extractBitmap has been called. + */ + size_t getSafeSize() const ; + /** Return the byte size of the pixels, based on the height and rowBytes. This routine is slightly slower than getSize(), but does not truncate the answer to 32bits. @@ -142,6 +148,10 @@ public: return size; } + /** Same as getSafeSize(), but does not truncate the answer to 32bits. + */ + Sk64 getSafeSize64() const ; + /** Returns true if the bitmap is opaque (has no translucent/transparent pixels). */ bool isOpaque() const; @@ -192,6 +202,44 @@ public: */ void setPixels(void* p, SkColorTable* ctable = NULL); + /** Copies the bitmap's pixels to the location pointed at by dst and returns + true if possible, returns false otherwise. + + In the event that the bitmap's stride is equal to dstRowBytes, and if + it is greater than strictly required by the bitmap's current config + (this may happen if the bitmap is an extracted subset of another), then + this function will copy bytes past the eand of each row, excluding the + last row. No copies are made outside of the declared size of dst, + however. + + Always returns false for RLE formats. + + @param dst Location of destination buffer. + @param dstSize Size of destination buffer. Must be large enough to hold + pixels using indicated stride. + @param dstRowBytes Width of each line in the buffer. If -1, uses + bitmap's internal stride. + */ + bool copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes = -1) + const; + + /** Copies the pixels at location src to the bitmap's pixel buffer. + Returns true if copy if possible (bitmap buffer is large enough), + false otherwise. + + Like copyPixelsTo, this function may write values beyond the end of + each row, although never outside the defined buffer. + + Always returns false for RLE formats. + + @param src Location of the source buffer. + @param srcSize Height of source buffer in pixels. + @param srcRowBytes Width of each line in the buffer. If -1, uses i + bitmap's internal stride. + */ + bool copyPixelsFrom(const void* const src, size_t srcSize, + int srcRowBytes = -1); + /** Use the standard HeapAllocator to create the pixelref that manages the pixel memory. It will be sized based on the current width/height/config. If this is called multiple times, a new pixelref object will be created @@ -230,7 +278,7 @@ public: */ bool allocPixels(Allocator* allocator, SkColorTable* ctable); - /** Return the current pixelref object, of any + /** Return the current pixelref object, if any */ SkPixelRef* pixelRef() const { return fPixelRef; } /** Return the offset into the pixelref, if any. Will return 0 if there is @@ -261,7 +309,8 @@ public: */ bool readyToDraw() const { return this->getPixels() != NULL && - ((this->config() != kIndex8_Config && this->config() != kRLE_Index8_Config) || + ((this->config() != kIndex8_Config && + this->config() != kRLE_Index8_Config) || fColorTable != NULL); } @@ -479,6 +528,17 @@ private: uint8_t fFlags; uint8_t fBytesPerPixel; // based on config + /* Internal computations for safe size. + */ + static Sk64 ComputeSafeSize64(Config config, + uint32_t width, + uint32_t height, + uint32_t rowBytes); + static size_t ComputeSafeSize(Config config, + uint32_t width, + uint32_t height, + uint32_t rowBytes); + /* Unreference any pixelrefs or colortables */ void freePixels(); diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 624e0cf214..e706326099 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -225,6 +225,30 @@ size_t SkBitmap::ComputeSize(Config c, int width, int height) { return isPos32Bits(size) ? size.get32() : 0; } +Sk64 SkBitmap::ComputeSafeSize64(Config config, + uint32_t width, + uint32_t height, + uint32_t rowBytes) { + Sk64 safeSize; + safeSize.setZero(); + if (height > 0) { + safeSize.set(ComputeRowBytes(config, width)); + Sk64 sizeAllButLastRow; + sizeAllButLastRow.setMul(height - 1, rowBytes); + safeSize.add(sizeAllButLastRow); + } + SkASSERT(!safeSize.isNeg()); + return safeSize; +} + +size_t SkBitmap::ComputeSafeSize(Config config, + uint32_t width, + uint32_t height, + uint32_t rowBytes) { + Sk64 safeSize = ComputeSafeSize64(config, width, height, rowBytes); + return (safeSize.is32() ? safeSize.get32() : 0); +} + void SkBitmap::setConfig(Config c, int width, int height, int rowBytes) { this->freePixels(); @@ -400,6 +424,100 @@ bool SkBitmap::HeapAllocator::allocPixelRef(SkBitmap* dst, /////////////////////////////////////////////////////////////////////////////// +size_t SkBitmap::getSafeSize() const { + // This is intended to be a size_t version of ComputeSafeSize64(), just + // faster. The computation is meant to be identical. + return (fHeight ? ((fHeight - 1) * fRowBytes) + + ComputeRowBytes(getConfig(), fWidth): 0); +} + +Sk64 SkBitmap::getSafeSize64() const { + return ComputeSafeSize64(getConfig(), fWidth, fHeight, fRowBytes); +} + +bool SkBitmap::copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes) + const { + + if (dstRowBytes == -1) + dstRowBytes = fRowBytes; + SkASSERT(dstRowBytes >= 0); + + if (getConfig() == kRLE_Index8_Config || + dstRowBytes < ComputeRowBytes(getConfig(), fWidth) || + dst == NULL || (getPixels() == NULL && pixelRef() == NULL)) + return false; + + if (static_cast(dstRowBytes) == fRowBytes) { + size_t safeSize = getSafeSize(); + if (safeSize > dstSize || safeSize == 0) + return false; + else { + SkAutoLockPixels lock(*this); + // This implementation will write bytes beyond the end of each row, + // excluding the last row, if the bitmap's stride is greater than + // strictly required by the current config. + memcpy(dst, getPixels(), safeSize); + + return true; + } + } else { + // If destination has different stride than us, then copy line by line. + if (ComputeSafeSize(getConfig(), fWidth, fHeight, dstRowBytes) > + dstSize) + return false; + else { + // Just copy what we need on each line. + uint32_t rowBytes = ComputeRowBytes(getConfig(), fWidth); + SkAutoLockPixels lock(*this); + const uint8_t* srcP = reinterpret_cast(getPixels()); + uint8_t* dstP = reinterpret_cast(dst); + for (uint32_t row = 0; row < fHeight; + row++, srcP += fRowBytes, dstP += dstRowBytes) { + memcpy(dstP, srcP, rowBytes); + } + + return true; + } + } +} + +bool SkBitmap::copyPixelsFrom(const void* const src, size_t srcSize, + int srcRowBytes) { + + if (srcRowBytes == -1) + srcRowBytes = fRowBytes; + SkASSERT(srcRowBytes >= 0); + + size_t safeSize = getSafeSize(); + uint32_t rowBytes = ComputeRowBytes(getConfig(), fWidth); + if (getConfig() == kRLE_Index8_Config || src == NULL || + static_cast(srcRowBytes) < rowBytes || + safeSize == 0 || + srcSize < ComputeSafeSize(getConfig(), fWidth, fHeight, srcRowBytes)) { + return false; + } + + SkAutoLockPixels lock(*this); + if (static_cast(srcRowBytes) == fRowBytes) { + // This implementation will write bytes beyond the end of each row, + // excluding the last row, if the bitmap's stride is greater than + // strictly required by the current config. + memcpy(getPixels(), src, safeSize); + } else { + // Just copy the bytes we need on each line. + const uint8_t* srcP = reinterpret_cast(src); + uint8_t* dstP = reinterpret_cast(getPixels()); + for (uint32_t row = 0; row < fHeight; + row++, srcP += srcRowBytes, dstP += fRowBytes) { + memcpy(dstP, srcP, rowBytes); + } + } + + return true; +} + +/////////////////////////////////////////////////////////////////////////////// + bool SkBitmap::isOpaque() const { switch (fConfig) { case kNo_Config: diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp index be3850e9be..7af8ac95a2 100644 --- a/tests/BitmapCopyTest.cpp +++ b/tests/BitmapCopyTest.cpp @@ -81,6 +81,151 @@ struct Pair { const char* fValid; }; +// Utility functions for copyPixelsTo()/copyPixelsFrom() tests. +// getPixel() +// setPixel() +// getSkConfigName() +// struct Coordinates +// reportCopyVerification() +// writeCoordPixels() + +// Utility function to read the value of a given pixel in bm. All +// values converted to uint32_t for simplification of comparisons. +uint32_t getPixel(int x, int y, const SkBitmap& bm) { + uint32_t val = 0; + uint16_t val16; + uint8_t val8, shift; + SkAutoLockPixels lock(bm); + const void* rawAddr = bm.getAddr(x,y); + + switch (bm.getConfig()) { + case SkBitmap::kARGB_8888_Config: + memcpy(&val, rawAddr, sizeof(uint32_t)); + break; + case SkBitmap::kARGB_4444_Config: + case SkBitmap::kRGB_565_Config: + memcpy(&val16, rawAddr, sizeof(uint16_t)); + val = val16; + break; + case SkBitmap::kA8_Config: + case SkBitmap::kIndex8_Config: + memcpy(&val8, rawAddr, sizeof(uint8_t)); + val = val8; + break; + case SkBitmap::kA1_Config: + memcpy(&val8, rawAddr, sizeof(uint8_t)); + shift = x % 8; + val = (val8 >> shift) & 0x1 ; + break; + default: + break; + } + return val; +} + +// Utility function to set value of any pixel in bm. +// bm.getConfig() specifies what format 'val' must be +// converted to, but at present uint32_t can handle all formats. +void setPixel(int x, int y, uint32_t val, SkBitmap& bm) { + uint16_t val16; + uint8_t val8, shift; + SkAutoLockPixels lock(bm); + void* rawAddr = bm.getAddr(x,y); + + switch (bm.getConfig()) { + case SkBitmap::kARGB_8888_Config: + memcpy(rawAddr, &val, sizeof(uint32_t)); + break; + case SkBitmap::kARGB_4444_Config: + case SkBitmap::kRGB_565_Config: + val16 = val & 0xFFFF; + memcpy(rawAddr, &val16, sizeof(uint16_t)); + break; + case SkBitmap::kA8_Config: + case SkBitmap::kIndex8_Config: + val8 = val & 0xFF; + memcpy(rawAddr, &val8, sizeof(uint8_t)); + break; + case SkBitmap::kA1_Config: + shift = x % 8; // We assume we're in the right byte. + memcpy(&val8, rawAddr, sizeof(uint8_t)); + if (val & 0x1) // Turn bit on. + val8 |= (0x1 << shift); + else // Turn bit off. + val8 &= ~(0x1 << shift); + memcpy(rawAddr, &val8, sizeof(uint8_t)); + break; + default: + // Ignore. + break; + } +} + +// Utility to return string containing name of each format, to +// simplify diagnostic output. +const char* getSkConfigName(const SkBitmap& bm) { + switch (bm.getConfig()) { + case SkBitmap::kNo_Config: return "SkBitmap::kNo_Config"; + case SkBitmap::kA1_Config: return "SkBitmap::kA1_Config"; + case SkBitmap::kA8_Config: return "SkBitmap::kA8_Config"; + case SkBitmap::kIndex8_Config: return "SkBitmap::kIndex8_Config"; + case SkBitmap::kRGB_565_Config: return "SkBitmap::kRGB_565_Config"; + case SkBitmap::kARGB_4444_Config: return "SkBitmap::kARGB_4444_Config"; + case SkBitmap::kARGB_8888_Config: return "SkBitmap::kARGB_8888_Config"; + case SkBitmap::kRLE_Index8_Config: + return "SkBitmap::kRLE_Index8_Config,"; + default: return "Unknown SkBitmap configuration."; + } +} + +// Helper struct to contain pixel locations, while avoiding need for STL. +struct Coordinates { + + const int length; + SkIPoint* const data; + + explicit Coordinates(int _length): length(_length) + , data(new SkIPoint[length]) { } + + ~Coordinates(){ + delete [] data; + } + + SkIPoint* operator[](int i) const { + // Use with care, no bounds checking. + return data + i; + } +}; + +// A function to verify that two bitmaps contain the same pixel values +// at all coordinates indicated by coords. Simplifies verification of +// copied bitmaps. +void reportCopyVerification(const SkBitmap& bm1, const SkBitmap& bm2, + Coordinates& coords, + const char* msg, + skiatest::Reporter* reporter){ + bool success = true; + + // Confirm all pixels in the list match. + for (int i = 0; i < coords.length; ++i) + success = success && + (getPixel(coords[i]->fX, coords[i]->fY, bm1) == + getPixel(coords[i]->fX, coords[i]->fY, bm2)); + + if (!success) { + SkString str; + str.printf("%s [config = %s]", + msg, getSkConfigName(bm1)); + reporter->reportFailed(str); + } +} + +// Writes unique pixel values at locations specified by coords. +void writeCoordPixels(SkBitmap& bm, const Coordinates& coords) { + for (int i = 0; i < coords.length; ++i) + setPixel(coords[i]->fX, coords[i]->fY, i, bm); +} + static void TestBitmapCopy(skiatest::Reporter* reporter) { static const Pair gPairs[] = { { SkBitmap::kNo_Config, "00000000" }, @@ -94,6 +239,10 @@ static void TestBitmapCopy(skiatest::Reporter* reporter) { // { SkBitmap::kRLE_Index8_Config, "00101111" } }; + static const bool isExtracted[] = { + false, true + }; + const int W = 20; const int H = 33; @@ -176,7 +325,257 @@ static void TestBitmapCopy(skiatest::Reporter* reporter) { REPORTER_ASSERT(reporter, dst.width() == 0); REPORTER_ASSERT(reporter, dst.height() == 0); } - } + } // for (size_t j = ... + + // Tests for getSafeSize(), getSafeSize64(), copyPixelsTo(), + // copyPixelsFrom(). + // + for (size_t copyCase = 0; copyCase < SK_ARRAY_COUNT(isExtracted); + ++copyCase) { + // Test copying to/from external buffer. + // Note: the tests below have hard-coded values --- + // Please take care if modifying. + if (gPairs[i].fConfig != SkBitmap::kRLE_Index8_Config) { + + // Tests for getSafeSize64(). + // Test with a very large configuration without pixel buffer + // attached. + SkBitmap tstSafeSize; + tstSafeSize.setConfig(gPairs[i].fConfig, 100000000U, + 100000000U); + Sk64 safeSize = tstSafeSize.getSafeSize64(); + if (safeSize.isNeg()) { + SkString str; + str.printf("getSafeSize64() negative: %s", + getSkConfigName(tstSafeSize)); + reporter->reportFailed(str); + } + bool sizeFail = false; + // Compare against hand-computed values. + switch (gPairs[i].fConfig) { + case SkBitmap::kNo_Config: + break; + + case SkBitmap::kA1_Config: + if (safeSize.fHi != 0x470DE || + safeSize.fLo != 0x4DF82000) + sizeFail = true; + break; + + case SkBitmap::kA8_Config: + case SkBitmap::kIndex8_Config: + if (safeSize.fHi != 0x2386F2 || + safeSize.fLo != 0x6FC10000) + sizeFail = true; + break; + + case SkBitmap::kRGB_565_Config: + case SkBitmap::kARGB_4444_Config: + if (safeSize.fHi != 0x470DE4 || + safeSize.fLo != 0xDF820000) + sizeFail = true; + break; + + case SkBitmap::kARGB_8888_Config: + if (safeSize.fHi != 0x8E1BC9 || + safeSize.fLo != 0xBF040000) + sizeFail = true; + break; + + case SkBitmap::kRLE_Index8_Config: + break; + + default: + break; + } + if (sizeFail) { + SkString str; + str.printf("getSafeSize64() wrong size: %s", + getSkConfigName(tstSafeSize)); + reporter->reportFailed(str); + } + + size_t subW, subH; + // Set sizes to be height = 2 to force the last row of the + // source to be used, thus verifying correct operation if + // the bitmap is an extracted subset. + if (gPairs[i].fConfig == SkBitmap::kA1_Config) { + // If one-bit per pixel, use 9 pixels to force more than + // one byte per row. + subW = 9; + subH = 2; + } else { + // All other configurations are at least one byte per pixel, + // and different configs will test copying different numbers + // of bytes. + subW = subH = 2; + } + + // Create bitmap to act as source for copies and subsets. + SkBitmap src, subset; + SkColorTable* ct = NULL; + if (isExtracted[copyCase]) { // A larger image to extract from. + src.setConfig(gPairs[i].fConfig, 2 * subW + 1, subH); + } else // Tests expect a 2x2 bitmap, so make smaller. + src.setConfig(gPairs[i].fConfig, subW, subH); + if (SkBitmap::kIndex8_Config == src.config() || + SkBitmap::kRLE_Index8_Config == src.config()) { + ct = init_ctable(); + } + + src.allocPixels(ct); + SkSafeUnref(ct); + + // Either copy src or extract into 'subset', which is used + // for subsequent calls to copyPixelsTo/From. + bool srcReady = false; + if (isExtracted[copyCase]) { + // The extractedSubset() test case allows us to test copy- + // ing when src and dst mave possibly different strides. + SkIRect r; + if (gPairs[i].fConfig == SkBitmap::kA1_Config) + // This config seems to need byte-alignment of + // extracted subset bits. + r.set(0, 0, subW, subH); + else + r.set(1, 0, 1 + subW, subH); // 2x2 extracted bitmap + + srcReady = src.extractSubset(&subset, r); + } else { + srcReady = src.copyTo(&subset, src.getConfig()); + } + + // Not all configurations will generate a valid 'subset'. + if (srcReady) { + + // Allocate our target buffer 'buf' for all copies. + // To simplify verifying correctness of copies attach + // buf to a SkBitmap, but copies are done using the + // raw buffer pointer. + const uint32_t bufSize = subH * + SkBitmap::ComputeRowBytes(src.getConfig(), subW) * 2; + uint8_t* buf = new uint8_t[bufSize]; + + SkBitmap bufBm; // Attach buf to this bitmap. + bool successExpected; + + // Set up values for each pixel being copied. + Coordinates coords(subW * subH); + for (size_t x = 0; x < subW; ++x) + for (size_t y = 0; y < subH; ++y) + { + size_t index = y * subW + x; + SkASSERT(index < coords.length); + coords[index]->fX = x; + coords[index]->fY = y; + } + + writeCoordPixels(subset, coords); + + // Test #1 //////////////////////////////////////////// + + // Before/after comparisons easier if we attach buf + // to an appropriately configured SkBitmap. + memset(buf, 0xFF, bufSize); + // Config with stride greater than src but that fits in buf. + bufBm.setConfig(gPairs[i].fConfig, subW, subH, + SkBitmap::ComputeRowBytes(subset.getConfig(), subW) + * 2); + bufBm.setPixels(buf); + successExpected = false; + // Then attempt to copy with a stride that is too large + // to fit in the buffer. + REPORTER_ASSERT(reporter, + subset.copyPixelsTo(buf, bufSize, bufBm.rowBytes() * 3) + == successExpected); + + if (successExpected) + reportCopyVerification(subset, bufBm, coords, + "copyPixelsTo(buf, bufSize, 1.5*maxRowBytes)", + reporter); + + // Test #2 //////////////////////////////////////////// + // This test should always succeed, but in the case + // of extracted bitmaps only because we handle the + // issue of getSafeSize(). Without getSafeSize() + // buffer overrun/read would occur. + memset(buf, 0xFF, bufSize); + bufBm.setConfig(gPairs[i].fConfig, subW, subH, + subset.rowBytes()); + bufBm.setPixels(buf); + successExpected = subset.getSafeSize() <= bufSize; + REPORTER_ASSERT(reporter, + subset.copyPixelsTo(buf, bufSize) == + successExpected); + if (successExpected) + reportCopyVerification(subset, bufBm, coords, + "copyPixelsTo(buf, bufSize)", reporter); + + // Test #3 //////////////////////////////////////////// + // Copy with different stride between src and dst. + memset(buf, 0xFF, bufSize); + bufBm.setConfig(gPairs[i].fConfig, subW, subH, + subset.rowBytes()+1); + bufBm.setPixels(buf); + successExpected = true; // Should always work. + REPORTER_ASSERT(reporter, + subset.copyPixelsTo(buf, bufSize, + subset.rowBytes()+1) == successExpected); + if (successExpected) + reportCopyVerification(subset, bufBm, coords, + "copyPixelsTo(buf, bufSize, rowBytes+1)", reporter); + + // Test #4 //////////////////////////////////////////// + // Test copy with stride too small. + memset(buf, 0xFF, bufSize); + bufBm.setConfig(gPairs[i].fConfig, subW, subH); + bufBm.setPixels(buf); + successExpected = false; + // Request copy with stride too small. + REPORTER_ASSERT(reporter, + subset.copyPixelsTo(buf, bufSize, bufBm.rowBytes()-1) + == successExpected); + if (successExpected) + reportCopyVerification(subset, bufBm, coords, + "copyPixelsTo(buf, bufSize, rowBytes()-1)", reporter); + + // Test #5 //////////////////////////////////////////// + // Tests the case where the source stride is too small + // for the source configuration. + memset(buf, 0xFF, bufSize); + bufBm.setConfig(gPairs[i].fConfig, subW, subH); + bufBm.setPixels(buf); + writeCoordPixels(bufBm, coords); + REPORTER_ASSERT(reporter, + subset.copyPixelsFrom(buf, bufSize, 1) == false); + + // Test #6 //////////////////////////////////////////// + // Tests basic copy from an external buffer to the bitmap. + // If the bitmap is "extracted", this also tests the case + // where the source stride is different from the dest. + // stride. + // We've made the buffer large enough to always succeed. + bufBm.setConfig(gPairs[i].fConfig, subW, subH); + bufBm.setPixels(buf); + writeCoordPixels(bufBm, coords); + REPORTER_ASSERT(reporter, + subset.copyPixelsFrom(buf, bufSize, bufBm.rowBytes()) == + true); + reportCopyVerification(bufBm, subset, coords, + "copyPixelsFrom(buf, bufSize)", + reporter); + + // Test #7 //////////////////////////////////////////// + // Tests the case where the source buffer is too small + // for the transfer. + REPORTER_ASSERT(reporter, + subset.copyPixelsFrom(buf, 1, subset.rowBytes()) == + false); + + delete [] buf; + } + } + } // for (size_t copyCase ... } }