Reland "Fix genID cloning bugs."

src/core/SkBitmap.cpp

 
 /*
  * Copyright 2008 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkBitmap.h"
@@ skipping 975 matching lines @@
     if (fPixelRef) {
         SkIRect subset;
         subset.setXYWH(fPixelRefOrigin.fX, fPixelRefOrigin.fY, fWidth, fHeight);
         if (fPixelRef->readPixels(&tmpSrc, &subset)) {
             SkASSERT(tmpSrc.width() == this->width());
             SkASSERT(tmpSrc.height() == this->height());
 
             // 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;
             }
 
             // fall through to the raster case
             src = &tmpSrc;
         }
     }
 
     // we lock this now, since we may need its colortable
     SkAutoLockPixels srclock(*src);
     if (!src->readyToDraw()) {
         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());
 
     // allocate colortable if srcConfig == kIndex8_Config
     SkColorTable* ctable = (dstConfig == kIndex8_Config) ?
     new SkColorTable(*src->getColorTable()) : NULL;
     SkAutoUnref au(ctable);
     if (!tmpDst.allocPixels(alloc, ctable)) {
         return false;
     }
 
     if (!tmpDst.readyToDraw()) {
         // allocator/lock failed
         return false;
     }
 
     /* 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
+
+            // 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 {
             const char* srcP = reinterpret_cast<const char*>(src->getPixels());
             char* dstP = reinterpret_cast<char*>(tmpDst.getPixels());
             // to be sure we don't read too much, only copy our logical pixels
             size_t bytesToCopy = tmpDst.width() * tmpDst.bytesPerPixel();
             for (int y = 0; y < tmpDst.height(); y++) {
                 memcpy(dstP, srcP, bytesToCopy);
                 srcP += src->rowBytes();
@@ skipping 34 matching lines @@
         return false;
     }
 
     // If we have a PixelRef, and it supports deep copy, use it.
     // Currently supported only by texture-backed bitmaps.
     if (fPixelRef) {
         SkPixelRef* pixelRef = fPixelRef->deepCopy(dstConfig);
         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;
             } else {
                 // With the new config, an appropriate fRowBytes will be computed by setConfig.
                 rowBytes = 0;
             }
             dst->setConfig(dstConfig, fWidth, fHeight, rowBytes);
             dst->setPixelRef(pixelRef, fPixelRefOrigin)->unref();
             return true;
@@ skipping 532 matching lines @@
         if (NULL != uri) {
             str->appendf(" uri:\"%s\"", uri);
         } else {
             str->appendf(" pixelref:%p", pr);
         }
     }
 
     str->append(")");
 }
 #endif