aboutsummaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/core/SkBitmap.cpp32
-rw-r--r--tests/BitmapCopyTest.cpp158
-rw-r--r--tests/GpuBitmapCopyTest.cpp52
3 files changed, 157 insertions, 85 deletions
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index f950e28cdc..9fb29eefa7 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -993,8 +993,8 @@ bool SkBitmap::copyTo(SkBitmap* dst, Config dstConfig, Allocator* alloc) const {
// 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 @@ bool SkBitmap::copyTo(SkBitmap* dst, Config dstConfig, Allocator* alloc) const {
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 @@ bool SkBitmap::copyTo(SkBitmap* dst, Config dstConfig, Allocator* alloc) const {
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 @@ bool SkBitmap::deepCopyTo(SkBitmap* dst, Config dstConfig) const {
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 7f78392cea..9e96c33a42 100644
--- a/tests/BitmapCopyTest.cpp
+++ b/tests/BitmapCopyTest.cpp
@@ -1,4 +1,3 @@
-
/*
* Copyright 2011 Google Inc.
*
@@ -199,44 +198,103 @@ static void writeCoordPixels(SkBitmap& bm, const Coordinates& coords) {
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 bool isExtracted[] = {
- false, true
- };
+static const int W = 20;
+static const int H = 33;
- const int W = 20;
- 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++) {
- for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
- SkBitmap srcOpaque, srcPremul, dst;
+ 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(&copy, 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;
+ }
- {
- SkColorTable* ctOpaque = NULL;
- SkColorTable* ctPremul = NULL;
+ // When performing a copy of an extracted subset, the gen id should
+ // change.
+ REPORTER_ASSERT(reporter, copy.getGenerationID() != subset.getGenerationID());
- 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);
+ 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);
}
- srcOpaque.allocPixels(ctOpaque);
- srcPremul.allocPixels(ctPremul);
- SkSafeUnref(ctOpaque);
- SkSafeUnref(ctPremul);
}
- init_src(srcOpaque);
- init_src(srcPremul);
+ }
+
+ 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
+ };
+
+ 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 dst;
bool success = srcPremul.copyTo(&dst, gPairs[j].fConfig);
bool expected = gPairs[i].fValid[j] != '0';
@@ -272,44 +330,6 @@ DEF_TEST(BitmapCopy, reporter) {
} 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(&copy, 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 681ea6995b..fb81e2b74c 100644
--- a/tests/GpuBitmapCopyTest.cpp
+++ b/tests/GpuBitmapCopyTest.cpp
@@ -40,22 +40,20 @@ struct Pair {
* @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 @@ static void TestIndividualCopy(skiatest::Reporter* reporter, const SkBitmap::Con
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 @@ DEF_GPUTEST(GpuBitmapCopy, reporter, factory) {
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 @@ DEF_GPUTEST(GpuBitmapCopy, reporter, factory) {
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 @@ DEF_GPUTEST(GpuBitmapCopy, reporter, factory) {
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 = ...
generated by cgit v1.2.3 (git 2.39.1) at 2024-06-29 13:59:18 +0000