From 7bfdfda809e7273d7c962cc62ef9390b5007fb5a Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Tue, 11 Apr 2017 15:37:50 +0000 Subject: [PATCH] Revert "remove unused SkBitmap::copyPixelsTo" This reverts commit 0f3fdfacf32261f943dcac5cdfd14475011f40db. Reason for revert: Blink-headless in Google3 needs an update too. Original change's description: > remove unused SkBitmap::copyPixelsTo > > Needs https://codereview.chromium.org/2812853002/ to land first > > Bug: skia:6465 > Change-Id: I531e33b2848cd995f20844786ed1a8d34d63fb64 > Reviewed-on: https://skia-review.googlesource.com/13171 > Reviewed-by: Mike Reed > Commit-Queue: Mike Reed > TBR=reed@google.com,reviews@skia.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I5e7c4b0d05772e4948cb1dffdcc40e095fbdba41 Reviewed-on: https://skia-review.googlesource.com/13185 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- include/core/SkBitmap.h | 21 ++ src/core/SkBitmap.cpp | 55 ++++++ tests/BitmapCopyTest.cpp | 416 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 492 insertions(+) diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h index 0fcfa2b24c..34da542edd 100644 --- a/include/core/SkBitmap.h +++ b/include/core/SkBitmap.h @@ -345,6 +345,27 @@ 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 case when the dstRowBytes matches the bitmap's rowBytes, the copy + may be made faster by copying over the dst's per-row padding (for all + rows but the last). By setting preserveDstPad to true the caller can + disable this optimization and ensure that pixels in the padding are not + overwritten. + + 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 0, uses + bitmap's internal stride. + @param preserveDstPad Must we preserve padding in the dst + */ + bool copyPixelsTo(void* const dst, size_t dstSize, size_t dstRowBytes = 0, + bool preserveDstPad = false) const; + /** Use the standard HeapAllocator to create the pixelref that manages the pixel memory. It will be sized based on the current ImageInfo. If this is called multiple times, a new pixelref object will be created diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 30c06debcc..9b8ea7f96c 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -468,6 +468,61 @@ bool SkBitmap::HeapAllocator::allocPixelRef(SkBitmap* dst, /////////////////////////////////////////////////////////////////////////////// +static bool copy_pixels_to(const SkPixmap& src, void* const dst, size_t dstSize, + size_t dstRowBytes, bool preserveDstPad) { + const SkImageInfo& info = src.info(); + + if (0 == dstRowBytes) { + dstRowBytes = src.rowBytes(); + } + if (dstRowBytes < info.minRowBytes()) { + return false; + } + + if (!preserveDstPad && static_cast(dstRowBytes) == src.rowBytes()) { + size_t safeSize = src.getSafeSize(); + if (safeSize > dstSize || safeSize == 0) + return false; + else { + // 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, src.addr(), safeSize); + return true; + } + } else { + // If destination has different stride than us, then copy line by line. + if (info.getSafeSize(dstRowBytes) > dstSize) { + return false; + } else { + // Just copy what we need on each line. + size_t rowBytes = info.minRowBytes(); + const uint8_t* srcP = reinterpret_cast(src.addr()); + uint8_t* dstP = reinterpret_cast(dst); + for (int row = 0; row < info.height(); ++row) { + memcpy(dstP, srcP, rowBytes); + srcP += src.rowBytes(); + dstP += dstRowBytes; + } + + return true; + } + } +} + +bool SkBitmap::copyPixelsTo(void* dst, size_t dstSize, size_t dstRB, bool preserveDstPad) const { + if (nullptr == dst) { + return false; + } + SkAutoPixmapUnlock result; + if (!this->requestLock(&result)) { + return false; + } + return copy_pixels_to(result.pixmap(), dst, dstSize, dstRB, preserveDstPad); +} + +/////////////////////////////////////////////////////////////////////////////// + bool SkBitmap::isImmutable() const { return fPixelRef ? fPixelRef->isImmutable() : false; } diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp index d2fda9e0b7..5eb9a2a918 100644 --- a/tests/BitmapCopyTest.cpp +++ b/tests/BitmapCopyTest.cpp @@ -10,6 +10,66 @@ #include "SkTemplates.h" #include "Test.h" +static const char* boolStr(bool value) { + return value ? "true" : "false"; +} + +static const char* color_type_name(SkColorType colorType) { + switch (colorType) { + case kUnknown_SkColorType: + return "None"; + case kAlpha_8_SkColorType: + return "A8"; + case kRGB_565_SkColorType: + return "565"; + case kARGB_4444_SkColorType: + return "4444"; + case kRGBA_8888_SkColorType: + return "RGBA"; + case kBGRA_8888_SkColorType: + return "BGRA"; + case kIndex_8_SkColorType: + return "Index8"; + case kGray_8_SkColorType: + return "Gray8"; + case kRGBA_F16_SkColorType: + return "F16"; + } + return ""; +} + +static void report_opaqueness(skiatest::Reporter* reporter, const SkBitmap& src, + const SkBitmap& dst) { + ERRORF(reporter, "src %s opaque:%d, dst %s opaque:%d", + color_type_name(src.colorType()), src.isOpaque(), + color_type_name(dst.colorType()), dst.isOpaque()); +} + +static bool canHaveAlpha(SkColorType ct) { + return kRGB_565_SkColorType != ct; +} + +// copyTo() should preserve isOpaque when it makes sense +static void test_isOpaque(skiatest::Reporter* reporter, + const SkBitmap& srcOpaque, const SkBitmap& srcPremul, + SkColorType dstColorType) { + SkBitmap dst; + + if (canHaveAlpha(srcPremul.colorType()) && canHaveAlpha(dstColorType)) { + REPORTER_ASSERT(reporter, srcPremul.copyTo(&dst, dstColorType)); + REPORTER_ASSERT(reporter, dst.colorType() == dstColorType); + if (srcPremul.isOpaque() != dst.isOpaque()) { + report_opaqueness(reporter, srcPremul, dst); + } + } + + REPORTER_ASSERT(reporter, srcOpaque.copyTo(&dst, dstColorType)); + REPORTER_ASSERT(reporter, dst.colorType() == dstColorType); + if (srcOpaque.isOpaque() != dst.isOpaque()) { + report_opaqueness(reporter, srcOpaque, dst); + } +} + static void init_src(const SkBitmap& bitmap) { SkAutoLockPixels lock(bitmap); if (bitmap.getPixels()) { @@ -41,6 +101,60 @@ struct Pair { // reportCopyVerification() // writeCoordPixels() +// Utility function to read the value of a given pixel in bm. All +// values converted to uint32_t for simplification of comparisons. +static uint32_t getPixel(int x, int y, const SkBitmap& bm) { + uint32_t val = 0; + uint16_t val16; + uint8_t val8; + SkAutoLockPixels lock(bm); + const void* rawAddr = bm.getAddr(x,y); + + switch (bm.bytesPerPixel()) { + case 4: + memcpy(&val, rawAddr, sizeof(uint32_t)); + break; + case 2: + memcpy(&val16, rawAddr, sizeof(uint16_t)); + val = val16; + break; + case 1: + memcpy(&val8, rawAddr, sizeof(uint8_t)); + val = val8; + 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. +static void setPixel(int x, int y, uint32_t val, SkBitmap& bm) { + uint16_t val16; + uint8_t val8; + SkAutoLockPixels lock(bm); + void* rawAddr = bm.getAddr(x,y); + + switch (bm.bytesPerPixel()) { + case 4: + memcpy(rawAddr, &val, sizeof(uint32_t)); + break; + case 2: + val16 = val & 0xFFFF; + memcpy(rawAddr, &val16, sizeof(uint16_t)); + break; + case 1: + val8 = val & 0xFF; + memcpy(rawAddr, &val8, sizeof(uint8_t)); + break; + default: + // Ignore. + break; + } +} + // Helper struct to contain pixel locations, while avoiding need for STL. struct Coordinates { @@ -60,6 +174,31 @@ struct Coordinates { } }; +// A function to verify that two bitmaps contain the same pixel values +// at all coordinates indicated by coords. Simplifies verification of +// copied bitmaps. +static void reportCopyVerification(const SkBitmap& bm1, const SkBitmap& bm2, + Coordinates& coords, + const char* msg, + skiatest::Reporter* reporter){ + // Confirm all pixels in the list match. + for (int i = 0; i < coords.length; ++i) { + uint32_t p1 = getPixel(coords[i]->fX, coords[i]->fY, bm1); + uint32_t p2 = getPixel(coords[i]->fX, coords[i]->fY, bm2); +// SkDebugf("[%d] (%d %d) p1=%x p2=%x\n", i, coords[i]->fX, coords[i]->fY, p1, p2); + if (p1 != p2) { + ERRORF(reporter, "%s [colortype = %s]", msg, color_type_name(bm1.colorType())); + break; + } + } +} + +// Writes unique pixel values at locations specified by coords. +static 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 const Pair gPairs[] = { { kUnknown_SkColorType, "0000000" }, { kAlpha_8_SkColorType, "0100000" }, @@ -148,6 +287,283 @@ DEF_TEST(BitmapCopy_extractSubset, reporter) { } } +DEF_TEST(BitmapCopy, reporter) { + static const bool isExtracted[] = { + false, true + }; + + for (size_t i = 0; i < SK_ARRAY_COUNT(gPairs); i++) { + SkBitmap srcOpaque, srcPremul; + setup_src_bitmaps(&srcOpaque, &srcPremul, gPairs[i].fColorType); + + for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) { + SkBitmap dst; + + bool success = srcPremul.copyTo(&dst, gPairs[j].fColorType); + bool expected = gPairs[i].fValid[j] != '0'; + if (success != expected) { + ERRORF(reporter, "SkBitmap::copyTo from %s to %s. expected %s " + "returned %s", color_type_name(gPairs[i].fColorType), + color_type_name(gPairs[j].fColorType), + boolStr(expected), boolStr(success)); + } + + bool canSucceed = srcPremul.canCopyTo(gPairs[j].fColorType); + if (success != canSucceed) { + ERRORF(reporter, "SkBitmap::copyTo from %s to %s. returned %s " + "canCopyTo %s", color_type_name(gPairs[i].fColorType), + color_type_name(gPairs[j].fColorType), + boolStr(success), boolStr(canSucceed)); + } + + if (success) { + REPORTER_ASSERT(reporter, srcPremul.width() == dst.width()); + REPORTER_ASSERT(reporter, srcPremul.height() == dst.height()); + REPORTER_ASSERT(reporter, dst.colorType() == gPairs[j].fColorType); + test_isOpaque(reporter, srcOpaque, srcPremul, dst.colorType()); + if (srcPremul.colorType() == dst.colorType()) { + SkAutoLockPixels srcLock(srcPremul); + SkAutoLockPixels dstLock(dst); + REPORTER_ASSERT(reporter, srcPremul.readyToDraw()); + REPORTER_ASSERT(reporter, dst.readyToDraw()); + const char* srcP = (const char*)srcPremul.getAddr(0, 0); + const char* dstP = (const char*)dst.getAddr(0, 0); + REPORTER_ASSERT(reporter, srcP != dstP); + REPORTER_ASSERT(reporter, !memcmp(srcP, dstP, + srcPremul.getSize())); + REPORTER_ASSERT(reporter, srcPremul.getGenerationID() == dst.getGenerationID()); + } else { + REPORTER_ASSERT(reporter, srcPremul.getGenerationID() != dst.getGenerationID()); + } + } else { + // dst should be unchanged from its initial state + REPORTER_ASSERT(reporter, dst.colorType() == kUnknown_SkColorType); + 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. + + // Tests for getSafeSize64(). + // Test with a very large configuration without pixel buffer + // attached. + SkBitmap tstSafeSize; + tstSafeSize.setInfo(SkImageInfo::Make(100000000U, 100000000U, + gPairs[i].fColorType, kPremul_SkAlphaType)); + int64_t safeSize = tstSafeSize.computeSafeSize64(); + if (safeSize < 0) { + ERRORF(reporter, "getSafeSize64() negative: %s", + color_type_name(tstSafeSize.colorType())); + } + bool sizeFail = false; + // Compare against hand-computed values. + switch (gPairs[i].fColorType) { + case kUnknown_SkColorType: + break; + + case kAlpha_8_SkColorType: + case kIndex_8_SkColorType: + if (safeSize != 0x2386F26FC10000LL) { + sizeFail = true; + } + break; + + case kRGB_565_SkColorType: + case kARGB_4444_SkColorType: + if (safeSize != 0x470DE4DF820000LL) { + sizeFail = true; + } + break; + + case kN32_SkColorType: + if (safeSize != 0x8E1BC9BF040000LL) { + sizeFail = true; + } + break; + + default: + break; + } + if (sizeFail) { + ERRORF(reporter, "computeSafeSize64() wrong size: %s", + color_type_name(tstSafeSize.colorType())); + } + + int subW = 2; + int subH = 2; + + // Create bitmap to act as source for copies and subsets. + SkBitmap src, subset; + sk_sp ct; + if (kIndex_8_SkColorType == src.colorType()) { + ct = init_ctable(); + } + + int localSubW; + if (isExtracted[copyCase]) { // A larger image to extract from. + localSubW = 2 * subW + 1; + } else { // Tests expect a 2x2 bitmap, so make smaller. + localSubW = subW; + } + // could fail if we pass kIndex_8 for the colortype + if (src.tryAllocPixels(SkImageInfo::Make(localSubW, subH, gPairs[i].fColorType, + kPremul_SkAlphaType))) { + // failure is fine, as we will notice later on + } + + // Either copy src or extract into 'subset', which is used + // for subsequent calls to copyPixelsTo/From. + bool srcReady = false; + // Test relies on older behavior that extractSubset will fail on + // kUnknown_SkColorType + if (kUnknown_SkColorType != src.colorType() && + isExtracted[copyCase]) { + // The extractedSubset() test case allows us to test copy- + // ing when src and dst mave possibly different strides. + SkIRect r; + r.set(1, 0, 1 + subW, subH); // 2x2 extracted bitmap + + srcReady = src.extractSubset(&subset, r); + } else { + srcReady = src.copyTo(&subset); + } + + // 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 size_t bufSize = subH * + SkColorTypeMinRowBytes(src.colorType(), subW) * 2; + SkAutoTMalloc autoBuf (bufSize); + uint8_t* buf = autoBuf.get(); + + SkBitmap bufBm; // Attach buf to this bitmap. + bool successExpected; + + // Set up values for each pixel being copied. + Coordinates coords(subW * subH); + for (int x = 0; x < subW; ++x) + for (int y = 0; y < subH; ++y) + { + int index = y * subW + x; + SkASSERT(index < coords.length); + coords[index]->fX = x; + coords[index]->fY = y; + } + + writeCoordPixels(subset, coords); + + // Test #1 //////////////////////////////////////////// + + const SkImageInfo info = SkImageInfo::Make(subW, subH, + gPairs[i].fColorType, + kPremul_SkAlphaType); + // 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.installPixels(info, buf, info.minRowBytes() * 2); + 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.installPixels(info, buf, subset.rowBytes()); + 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.installPixels(info, buf, subset.rowBytes()+1); + 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.installPixels(info, buf, info.minRowBytes()); + 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); + +#if 0 // copyPixelsFrom is gone + // Test #5 //////////////////////////////////////////// + // Tests the case where the source stride is too small + // for the source configuration. + memset(buf, 0xFF, bufSize); + bufBm.installPixels(info, buf, info.minRowBytes()); + 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.installPixels(info, buf, info.minRowBytes()); + 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); + +#endif + } + } // for (size_t copyCase ... + } +} + #include "SkColorPriv.h" #include "SkUtils.h"