Reland "Fix genID cloning bugs."
SkBitmap.cpp:
When copyTo calls readPixels, only clone the genID if the resulting
SkPixelRef has the same dimensions as the original. This catches a
bug where copying an SkBitmap representing the subset of an SkPixelRef
(which implements onReadPixels) would result in the copy sharing the
genID. (Thanks to r6710, this case can only happen using setPixelRef,
so the updated GpuBitmapCopyTest checks for that.)
Move some unnecessary NULL checks to asserts.
When copyTo performs a memcpy, only clone the genID if the resulting
SkPixelRef has the same dimensions as the original. This catches a bug
where copying an extracted SkBitmap with the same width as its original
SkPixelRef would incorrectly have the same genID.
Add a comment and assert in deepCopyTo, when cloning the genID, since
that case correctly clones it.
BitmapCopyTest.cpp:
Pull redundant work out of the inner loop (setting up the source bitmaps
and testing extractSubset). Create a new inner loop for extractSubset, to
test copying the result to each different config.
Extract a subset that has the same width as the original, to catch the
bug mentioned above.
Remove the reporter assert which checks for the resulting rowbytes.
Add checks to ensure that copying the extracted subset changes the genID.
GpuBitmapCopyTest:
Create an SkBitmap that shares an existing SkPixelRef, but only represents
a subset. This is to test the first call to cloneGenID in SkBitmap::copyTo.
In this case, the genID should NOT be copied, since only a portion of the
SkPixelRef was copied.
Also test deepCopy on this subset.
TestIndividualCopy now takes a parameter stating whether the genID should
change in the copy. It also does a read back using the appropriate subset.
It no longer differentiates between copyTo and deepCopyTo, since that
distinction was only necessary for copying from/to configs other than 8888
(which are no longer being tested), where copyTo did a read back in 8888 and
then drew the result to the desired config (resulting in an imperfect copy).
BUG=skia:1742
Committed: http://code.google.com/p/skia/source/detail?r=13021
R=mtklein@google.com
Review URL: https://codereview.chromium.org/112113005
git-svn-id: http://skia.googlecode.com/svn/trunk@13090 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index f950e28..9fb29ee 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -993,8 +993,8 @@
// did we get lucky and we can just return tmpSrc?
if (tmpSrc.config() == dstConfig && NULL == alloc) {
dst->swap(tmpSrc);
- if (dst->pixelRef() && this->config() == dstConfig) {
- // TODO(scroggo): fix issue 1742
+ // If the result is an exact copy, clone the gen ID.
+ if (dst->pixelRef() && dst->pixelRef()->info() == fPixelRef->info()) {
dst->pixelRef()->cloneGenID(*fPixelRef);
}
return true;
@@ -1011,6 +1011,9 @@
return false;
}
+ // The only way to be readyToDraw is if fPixelRef is non NULL.
+ SkASSERT(fPixelRef != NULL);
+
SkBitmap tmpDst;
tmpDst.setConfig(dstConfig, src->width(), src->height(), 0,
src->alphaType());
@@ -1028,14 +1031,31 @@
return false;
}
+ // pixelRef must be non NULL or tmpDst.readyToDraw() would have
+ // returned false.
+ SkASSERT(tmpDst.pixelRef() != NULL);
+
/* do memcpy for the same configs cases, else use drawing
*/
if (src->config() == dstConfig) {
if (tmpDst.getSize() == src->getSize()) {
memcpy(tmpDst.getPixels(), src->getPixels(), src->getSafeSize());
SkPixelRef* pixelRef = tmpDst.pixelRef();
- if (NULL != pixelRef && NULL != fPixelRef) {
- // TODO(scroggo): fix issue 1742
+
+ // In order to reach this point, we know that the width, config and
+ // rowbytes of the SkPixelRefs are the same, but it is possible for
+ // the heights to differ, if this SkBitmap's height is a subset of
+ // fPixelRef. Only if the SkPixelRefs' heights match are we
+ // guaranteed that this is an exact copy, meaning we should clone
+ // the genID.
+ if (pixelRef->info().fHeight == fPixelRef->info().fHeight) {
+ // TODO: what to do if the two infos match, BUT
+ // fPixelRef is premul and pixelRef is opaque?
+ // skipping assert for now
+ // https://code.google.com/p/skia/issues/detail?id=2012
+// SkASSERT(pixelRef->info() == fPixelRef->info());
+ SkASSERT(pixelRef->info().fWidth == fPixelRef->info().fWidth);
+ SkASSERT(pixelRef->info().fColorType == fPixelRef->info().fColorType);
pixelRef->cloneGenID(*fPixelRef);
}
} else {
@@ -1090,7 +1110,9 @@
if (pixelRef) {
uint32_t rowBytes;
if (dstConfig == fConfig) {
- // TODO(scroggo): fix issue 1742
+ // Since there is no subset to pass to deepCopy, and deepCopy
+ // succeeded, the new pixel ref must be identical.
+ SkASSERT(fPixelRef->info() == pixelRef->info());
pixelRef->cloneGenID(*fPixelRef);
// Use the same rowBytes as the original.
rowBytes = fRowBytes;
diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp
index 7f78392..9e96c33 100644
--- a/tests/BitmapCopyTest.cpp
+++ b/tests/BitmapCopyTest.cpp
@@ -1,4 +1,3 @@
-
/*
* Copyright 2011 Google Inc.
*
@@ -199,44 +198,103 @@
setPixel(coords[i]->fX, coords[i]->fY, i, bm);
}
-DEF_TEST(BitmapCopy, reporter) {
- static const Pair gPairs[] = {
- { SkBitmap::kNo_Config, "0000000" },
- { SkBitmap::kA8_Config, "0101010" },
- { SkBitmap::kIndex8_Config, "0111010" },
- { SkBitmap::kRGB_565_Config, "0101010" },
- { SkBitmap::kARGB_4444_Config, "0101110" },
- { SkBitmap::kARGB_8888_Config, "0101110" },
- };
+static const Pair gPairs[] = {
+ { SkBitmap::kNo_Config, "0000000" },
+ { SkBitmap::kA8_Config, "0101010" },
+ { SkBitmap::kIndex8_Config, "0111010" },
+ { SkBitmap::kRGB_565_Config, "0101010" },
+ { SkBitmap::kARGB_4444_Config, "0101110" },
+ { SkBitmap::kARGB_8888_Config, "0101110" },
+};
+static const int W = 20;
+static const int H = 33;
+
+static void setup_src_bitmaps(SkBitmap* srcOpaque, SkBitmap* srcPremul,
+ SkBitmap::Config config) {
+ SkColorTable* ctOpaque = NULL;
+ SkColorTable* ctPremul = NULL;
+
+ srcOpaque->setConfig(config, W, H, 0, kOpaque_SkAlphaType);
+ srcPremul->setConfig(config, W, H, 0, kPremul_SkAlphaType);
+ if (SkBitmap::kIndex8_Config == config) {
+ ctOpaque = init_ctable(kOpaque_SkAlphaType);
+ ctPremul = init_ctable(kPremul_SkAlphaType);
+ }
+ srcOpaque->allocPixels(ctOpaque);
+ srcPremul->allocPixels(ctPremul);
+ SkSafeUnref(ctOpaque);
+ SkSafeUnref(ctPremul);
+ init_src(*srcOpaque);
+ init_src(*srcPremul);
+}
+
+DEF_TEST(BitmapCopy_extractSubset, reporter) {
+ for (size_t i = 0; i < SK_ARRAY_COUNT(gPairs); i++) {
+ SkBitmap srcOpaque, srcPremul;
+ setup_src_bitmaps(&srcOpaque, &srcPremul, gPairs[i].fConfig);
+
+ SkBitmap bitmap(srcOpaque);
+ SkBitmap subset;
+ SkIRect r;
+ // Extract a subset which has the same width as the original. This
+ // catches a bug where we cloned the genID incorrectly.
+ r.set(0, 1, W, 3);
+ bitmap.setIsVolatile(true);
+ if (bitmap.extractSubset(&subset, r)) {
+ REPORTER_ASSERT(reporter, subset.width() == W);
+ REPORTER_ASSERT(reporter, subset.height() == 2);
+ REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+ REPORTER_ASSERT(reporter, subset.isVolatile() == true);
+
+ // Test copying an extracted subset.
+ for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
+ SkBitmap copy;
+ bool success = subset.copyTo(©, gPairs[j].fConfig);
+ if (!success) {
+ // Skip checking that success matches fValid, which is redundant
+ // with the code below.
+ REPORTER_ASSERT(reporter, gPairs[i].fConfig != gPairs[j].fConfig);
+ continue;
+ }
+
+ // When performing a copy of an extracted subset, the gen id should
+ // change.
+ REPORTER_ASSERT(reporter, copy.getGenerationID() != subset.getGenerationID());
+
+ REPORTER_ASSERT(reporter, copy.width() == W);
+ REPORTER_ASSERT(reporter, copy.height() == 2);
+
+ if (gPairs[i].fConfig == gPairs[j].fConfig) {
+ SkAutoLockPixels alp0(subset);
+ SkAutoLockPixels alp1(copy);
+ // they should both have, or both not-have, a colortable
+ bool hasCT = subset.getColorTable() != NULL;
+ REPORTER_ASSERT(reporter, (copy.getColorTable() != NULL) == hasCT);
+ }
+ }
+ }
+
+ bitmap = srcPremul;
+ bitmap.setIsVolatile(false);
+ if (bitmap.extractSubset(&subset, r)) {
+ REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+ REPORTER_ASSERT(reporter, subset.isVolatile() == false);
+ }
+ }
+}
+
+DEF_TEST(BitmapCopy, reporter) {
static const bool isExtracted[] = {
false, true
};
- const int W = 20;
- const int H = 33;
-
for (size_t i = 0; i < SK_ARRAY_COUNT(gPairs); i++) {
+ SkBitmap srcOpaque, srcPremul;
+ setup_src_bitmaps(&srcOpaque, &srcPremul, gPairs[i].fConfig);
+
for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
- SkBitmap srcOpaque, srcPremul, dst;
-
- {
- SkColorTable* ctOpaque = NULL;
- SkColorTable* ctPremul = NULL;
-
- srcOpaque.setConfig(gPairs[i].fConfig, W, H, 0, kOpaque_SkAlphaType);
- srcPremul.setConfig(gPairs[i].fConfig, W, H, 0, kPremul_SkAlphaType);
- if (SkBitmap::kIndex8_Config == gPairs[i].fConfig) {
- ctOpaque = init_ctable(kOpaque_SkAlphaType);
- ctPremul = init_ctable(kPremul_SkAlphaType);
- }
- srcOpaque.allocPixels(ctOpaque);
- srcPremul.allocPixels(ctPremul);
- SkSafeUnref(ctOpaque);
- SkSafeUnref(ctPremul);
- }
- init_src(srcOpaque);
- init_src(srcPremul);
+ SkBitmap dst;
bool success = srcPremul.copyTo(&dst, gPairs[j].fConfig);
bool expected = gPairs[i].fValid[j] != '0';
@@ -272,44 +330,6 @@
} else {
REPORTER_ASSERT(reporter, srcPremul.getGenerationID() != dst.getGenerationID());
}
- // test extractSubset
- {
- SkBitmap bitmap(srcOpaque);
- SkBitmap subset;
- SkIRect r;
- r.set(1, 1, 2, 2);
- bitmap.setIsVolatile(true);
- if (bitmap.extractSubset(&subset, r)) {
- REPORTER_ASSERT(reporter, subset.width() == 1);
- REPORTER_ASSERT(reporter, subset.height() == 1);
- REPORTER_ASSERT(reporter,
- subset.alphaType() == bitmap.alphaType());
- REPORTER_ASSERT(reporter,
- subset.isVolatile() == true);
-
- SkBitmap copy;
- REPORTER_ASSERT(reporter,
- subset.copyTo(©, subset.config()));
- REPORTER_ASSERT(reporter, copy.width() == 1);
- REPORTER_ASSERT(reporter, copy.height() == 1);
- REPORTER_ASSERT(reporter, copy.rowBytes() <= 4);
-
- SkAutoLockPixels alp0(subset);
- SkAutoLockPixels alp1(copy);
- // they should both have, or both not-have, a colortable
- bool hasCT = subset.getColorTable() != NULL;
- REPORTER_ASSERT(reporter,
- (copy.getColorTable() != NULL) == hasCT);
- }
- bitmap = srcPremul;
- bitmap.setIsVolatile(false);
- if (bitmap.extractSubset(&subset, r)) {
- REPORTER_ASSERT(reporter,
- subset.alphaType() == bitmap.alphaType());
- REPORTER_ASSERT(reporter,
- subset.isVolatile() == false);
- }
- }
} else {
// dst should be unchanged from its initial state
REPORTER_ASSERT(reporter, dst.config() == SkBitmap::kNo_Config);
diff --git a/tests/GpuBitmapCopyTest.cpp b/tests/GpuBitmapCopyTest.cpp
index 681ea69..fb81e2b 100644
--- a/tests/GpuBitmapCopyTest.cpp
+++ b/tests/GpuBitmapCopyTest.cpp
@@ -40,22 +40,20 @@
* @param success True if the copy succeeded.
* @param src A GPU-backed SkBitmap that had copyTo or deepCopyTo called on it.
* @param dst SkBitmap that was copied to.
- * @param deepCopy True if deepCopyTo was used; false if copyTo was used.
+ * @param expectSameGenID Whether the genIDs should be the same if success is true.
*/
static void TestIndividualCopy(skiatest::Reporter* reporter, const SkBitmap::Config desiredConfig,
const bool success, const SkBitmap& src, const SkBitmap& dst,
- const bool deepCopy = true) {
+ const bool expectSameGenID) {
if (success) {
REPORTER_ASSERT(reporter, src.width() == dst.width());
REPORTER_ASSERT(reporter, src.height() == dst.height());
REPORTER_ASSERT(reporter, dst.config() == desiredConfig);
if (src.config() == dst.config()) {
- // FIXME: When calling copyTo (so deepCopy is false here), sometimes we copy the pixels
- // exactly, in which case the IDs should be the same, but sometimes we do a bitmap draw,
- // in which case the IDs should not be the same. Is there any way to determine which is
- // the case at this point?
- if (deepCopy) {
+ if (expectSameGenID) {
REPORTER_ASSERT(reporter, src.getGenerationID() == dst.getGenerationID());
+ } else {
+ REPORTER_ASSERT(reporter, src.getGenerationID() != dst.getGenerationID());
}
REPORTER_ASSERT(reporter, src.pixelRef() != NULL && dst.pixelRef() != NULL);
@@ -63,11 +61,17 @@
SkBitmap srcReadBack, dstReadBack;
{
SkASSERT(src.getTexture() != NULL);
- bool readBack = src.pixelRef()->readPixels(&srcReadBack);
+ const SkIPoint origin = src.pixelRefOrigin();
+ const SkIRect subset = SkIRect::MakeXYWH(origin.fX, origin.fY,
+ src.width(), src.height());
+ bool readBack = src.pixelRef()->readPixels(&srcReadBack, &subset);
REPORTER_ASSERT(reporter, readBack);
}
if (dst.getTexture() != NULL) {
- bool readBack = dst.pixelRef()->readPixels(&dstReadBack);
+ const SkIPoint origin = dst.pixelRefOrigin();
+ const SkIRect subset = SkIRect::MakeXYWH(origin.fX, origin.fY,
+ dst.width(), dst.height());
+ bool readBack = dst.pixelRef()->readPixels(&dstReadBack, &subset);
REPORTER_ASSERT(reporter, readBack);
} else {
// If dst is not a texture, do a copy instead, to the same config as srcReadBack.
@@ -164,7 +168,7 @@
boolStr(canSucceed));
}
- TestIndividualCopy(reporter, gPairs[j].fConfig, success, src, dst);
+ TestIndividualCopy(reporter, gPairs[j].fConfig, success, src, dst, true);
// Test copying the subset bitmap, using both copyTo and deepCopyTo.
if (extracted) {
@@ -173,7 +177,7 @@
REPORTER_ASSERT(reporter, success == expected);
REPORTER_ASSERT(reporter, success == canSucceed);
TestIndividualCopy(reporter, gPairs[j].fConfig, success, subset, subsetCopy,
- false);
+ true);
// Reset the bitmap so that a failed copyTo will leave it in the expected state.
subsetCopy.reset();
@@ -182,6 +186,32 @@
REPORTER_ASSERT(reporter, success == canSucceed);
TestIndividualCopy(reporter, gPairs[j].fConfig, success, subset, subsetCopy,
true);
+
+ // Now set a bitmap to be a subset that will share the same pixelref.
+ // This allows testing another case of cloning the genID. When calling copyTo
+ // on a bitmap representing a subset of its pixelref, the resulting pixelref
+ // should not share the genID, since we only copied the subset.
+ SkBitmap trueSubset;
+ // FIXME: Once https://codereview.chromium.org/109023008/ lands, call
+ // trueSubset.installPixelRef(src.pixelRef(), subset);
+ trueSubset.setConfig(gPairs[i].fConfig, W/2, H/2);
+ trueSubset.setPixelRef(src.pixelRef(), W/2, H/2);
+
+ subsetCopy.reset();
+ success = trueSubset.copyTo(&subsetCopy, gPairs[j].fConfig);
+ REPORTER_ASSERT(reporter, success == expected);
+ REPORTER_ASSERT(reporter, success == canSucceed);
+ TestIndividualCopy(reporter, gPairs[j].fConfig, success, trueSubset, subsetCopy,
+ false);
+
+ // deepCopyTo copies the entire pixelref, even if the bitmap only represents
+ // a subset. Therefore, the result should share the same genID.
+ subsetCopy.reset();
+ success = trueSubset.deepCopyTo(&subsetCopy, gPairs[j].fConfig);
+ REPORTER_ASSERT(reporter, success == expected);
+ REPORTER_ASSERT(reporter, success == canSucceed);
+ TestIndividualCopy(reporter, gPairs[j].fConfig, success, trueSubset, subsetCopy,
+ true);
}
} // for (size_t j = ...
} // for (size_t i = ...