Reland "Fix genID cloning bugs."

src/core/SkBitmap.cpp

Index: src/core/SkBitmap.cpp
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index cb8ac7231b29a9728dcf0d4804c6660c40291a04..e5a02f1aaa2bfdf9df4fee3b6c735ce7a79d2a9c 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -993,8 +993,10 @@ 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() && this->config() == dstConfig

[reed1, 2014/01/09 13:04:50]

do we need to compare config && info, or just info?

[scroggo, 2014/01/09 20:45:23]

Just info. Fixed.

+                    && dst->pixelRef()->info() == fPixelRef->info())
+                {
                     dst->pixelRef()->cloneGenID(*fPixelRef);
                 }
                 return true;
@@ -1011,6 +1013,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());
@@ -1034,8 +1039,13 @@ bool SkBitmap::copyTo(SkBitmap* dst, Config dstConfig, Allocator* alloc) const {
         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
+
+            // pixelRef must be non NULL or tmpDst.readyToDraw() would have
+            // returned false.
+            SkASSERT(pixelRef != NULL);

[mtklein, 2014/01/09 15:58:07]

I might even move this to right after the if(!tmpDst.readyToDraw()) { return
false; }, like we have for fPixelRef.

[scroggo, 2014/01/09 20:45:23]

Done.

+
+            // If the resulting pixel ref is an exact copy, clone the genID.
+            if (pixelRef->info() == fPixelRef->info()) {

[mtklein, 2014/01/09 15:58:07]

Is it possible for this test to fail?  They've got the same config, width,
height, and size.  Doesn't that imply pixelRef->info() == fPixelRef->info()?

[scroggo, 2014/01/09 20:45:23]

It is possible for this test to fail, but it's a little bit tricky. The
*bitmaps* have the same config, width, height, and size (rowbytes * height), but
the pixel refs do not necessarily match in all those dimensions. If this bitmap
has a width equal to its pixel ref's width (and the default rowbytes), but its
height is smaller, execution will still get here, but the new pixel ref is not a
complete copy (it's missing some rows).

That said, checking the height of the pixel refs should be sufficient. I've made
that change and added some comments and an assert.

Testing this case resulted in changing line 251 of BitmapCopyTest:

    r.set(0, 1, W, 3);

The original, line 290 (sorry for the move and change...), said:

    r.set(1, 1, 2, 2);

[mtklein, 2014/01/09 21:29:18]

Thanks, that's tricky.  The new comments help a lot.

                 pixelRef->cloneGenID(*fPixelRef);
             }
         } else {
@@ -1090,7 +1100,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.

[scroggo, 2014/01/08 23:34:59]

Currently, calling deepCopyTo copies the entire SkPixelRef, so we will always
clone the genID here.

We *could* pass the subset to deepCopy, in which case we would only call
cloneGenID if the whole thing was copied. Would that be desirable?

+                SkASSERT(fPixelRef->info() == pixelRef->info());
                 pixelRef->cloneGenID(*fPixelRef);
                 // Use the same rowBytes as the original.
                 rowBytes = fRowBytes;