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
R=mtklein@google.com, bsalomon@google.com, reed@google.com

Author: scroggo@google.com

Review URL: https://codereview.chromium.org/112113005

git-svn-id: http://skia.googlecode.com/svn/trunk@13021 2bbb7eff-a529-9590-31e7-b0007b416f81

3 files changed, 151 insertions, 85 deletions

diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index f950e28cdc..a7615bd55c 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,25 @@ 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) {
+                SkASSERT(pixelRef->info() == fPixelRef->info());
                 pixelRef->cloneGenID(*fPixelRef);
             }
         } else {
@@ -1090,7 +1104,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 d831e5e120..93367bfe2d 100644
--- a/tests/BitmapCopyTest.cpp
+++ b/tests/BitmapCopyTest.cpp
@@ -1,4 +1,3 @@
-
 /*
  * Copyright 2011 Google Inc.
  *
@@ -200,44 +199,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';
@@ -273,44 +331,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 a1434dcf11..bfc3020e3f 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 @@ static void TestGpuBitmapCopy(skiatest::Reporter* reporter, GrContextFactory* fa
                            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 @@ static void TestGpuBitmapCopy(skiatest::Reporter* reporter, GrContextFactory* fa
                     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 @@ static void TestGpuBitmapCopy(skiatest::Reporter* reporter, GrContextFactory* fa
                     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 = ...