add readPixels() to SkBitmap

src/core/SkBitmap.cpp

Index: src/core/SkBitmap.cpp
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 651152e703bd5c2e0bf9c31b6f97f9d33abba532..b64c02913d13e5b4aa818828e906684f829639a9 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -821,11 +821,13 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
 #include "SkPaint.h"
 
 bool SkBitmap::canCopyTo(SkColorType dstColorType) const {
-    if (this->colorType() == kUnknown_SkColorType) {
+    const SkColorType srcCT = this->colorType();
+
+    if (srcCT == kUnknown_SkColorType) {
         return false;
     }
 
-    bool sameConfigs = (this->colorType() == dstColorType);
+    bool sameConfigs = (srcCT == dstColorType);
     switch (dstColorType) {
         case kAlpha_8_SkColorType:
         case kRGB_565_SkColorType:
@@ -838,13 +840,65 @@ bool SkBitmap::canCopyTo(SkColorType dstColorType) const {
             }
             break;
         case kARGB_4444_SkColorType:
-            return sameConfigs || kN32_SkColorType == this->colorType();
+            return sameConfigs || kN32_SkColorType == srcCT || kIndex_8_SkColorType == srcCT;
         default:
             return false;
     }
     return true;
 }
 
+#include "SkConfig8888.h"
+
+bool SkBitmap::readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
+                          int x, int y) const {
+    if (kUnknown_SkColorType == dstInfo.colorType()) {
+        return false;
+    }
+    if (NULL == dstPixels || dstRB < dstInfo.minRowBytes()) {
+        return false;
+    }
+    if (0 == dstInfo.width() || 0 == dstInfo.height()) {

[scroggo, 2014/07/10 15:09:48]

Arguably this could return true.

[reed1, 2014/07/10 15:32:16]

Possibly. Below we return false if the srcR doesn't intersect the bounds, which
would trigger if srcR were empty. I guess I could go either way. If we change
this, I would update canvas::readpixels as well.

+        return false;
+    }
+    
+    SkIRect srcR = SkIRect::MakeXYWH(x, y, dstInfo.width(), dstInfo.height());
+    if (!srcR.intersect(0, 0, this->width(), this->height())) {
+        return false;
+    }
+    
+    SkImageInfo info = dstInfo;
+    // the intersect may have shrunk info's logical size
+    info.fWidth = srcR.width();
+    info.fHeight = srcR.height();

[scroggo, 2014/07/10 15:09:48]

You never use info after this (except to check bytesPerPixel, which is unchanged
from dstInfo). I'm guessing you meant to use it down below when you use dstInfo.

It might be clearer if you change the parameter from dstInfo to requestedDstInfo
and this one to dstInfo.

That brings up the larger question though: if the caller tried to copy a
rectangle that is not a subset of pixels, why did we choose to copy a subset,
rather than report failure? Is it possible we're covering up their mistake?

[reed1, 2014/07/10 15:32:16]

Good catch.
Agreed.
Not necessarily a mistake, but who knows. This allows the caller to have us
write into a subset of their buffer easily. Again, canvas::readpixels has
supported this all along, but we can always rethink this (but possibly in a
different CL).

+    
+    // if x or y are negative, then we have to adjust pixels
+    if (x > 0) {
+        x = 0;
+    }
+    if (y > 0) {
+        y = 0;
+    }
+    // here x,y are either 0 or negative
+    dstPixels = ((char*)dstPixels - y * dstRB - x * info.bytesPerPixel());
+
+    //////////////
+    
+    SkAutoLockPixels alp(*this);
+    
+    // since we don't stop creating un-pixeled devices yet, check for no pixels here
+    if (NULL == this->getPixels()) {
+        return false;
+    }
+    
+    SkImageInfo srcInfo = this->info();
+    srcInfo.fWidth = dstInfo.width();

[scroggo, 2014/07/10 15:09:48]

shouldn't this be:

    srcInfo.fWidth = info.width()

and same for height? They could have been shrunk by the intersection. (or just
look at srcR)

[reed1, 2014/07/10 15:32:15]

Agreed. I will make the suggested change above, and that will trickle down and
fix this.

+    srcInfo.fHeight = dstInfo.height();
+    
+    const void* srcPixels = this->getAddr(srcR.x(), srcR.y());
+    return SkPixelInfo::CopyPixels(dstInfo, dstPixels, dstRB, srcInfo, srcPixels, this->rowBytes(),
+                                   this->getColorTable());
+}
+
 bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType,
                       Allocator* alloc) const {
     if (!this->canCopyTo(dstColorType)) {
@@ -919,59 +973,18 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType,
     // returned false.
     SkASSERT(tmpDst.pixelRef() != NULL);
 
-    /* do memcpy for the same configs cases, else use drawing
-    */
-    if (src->colorType() == dstColorType) {
-        if (tmpDst.getSize() == src->getSize()) {
-            memcpy(tmpDst.getPixels(), src->getPixels(), src->getSafeSize());
+    if (!src->readPixels(tmpDst.info(), tmpDst.getPixels(), tmpDst.rowBytes(), 0, 0)) {
+        return false;
+    }
 
-            SkPixelRef* dstPixelRef = tmpDst.pixelRef();
-            if (dstPixelRef->info() == fPixelRef->info()) {
-                dstPixelRef->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();
-                dstP += tmpDst.rowBytes();
-            }
+    //  Hack (for BitmapHeap) -- clone the pixelref genID even though we have a new pixelref.
+    //  The old copyTo impl did this, so we continue it for now.

[scroggo, 2014/07/10 15:09:48]

I don't agree that this is a hack. The point of the genID is to show that the
pixels are the same, which they are.

You mention that we do it for now; what would be a better long term solution?

[reed1, 2014/07/10 15:32:16]

I don't agree with the hack either, but BitmapHeap and Pipe will need to be
changed if we remove this. Sounds to me like a separate CL, since this is the
current behavior, and not related to this change.

+    //
+    if (src->colorType() == dstColorType && tmpDst.getSize() == src->getSize()) {
+        SkPixelRef* dstPixelRef = tmpDst.pixelRef();
+        if (dstPixelRef->info() == fPixelRef->info()) {
+            dstPixelRef->cloneGenID(*fPixelRef);
         }
-    } else if (kARGB_4444_SkColorType == dstColorType
-               && kN32_SkColorType == src->colorType()) {
-        if (src->alphaType() == kUnpremul_SkAlphaType) {
-            // Our method for converting to 4444 assumes premultiplied.
-            return false;
-        }
-        SkASSERT(src->height() == tmpDst.height());
-        SkASSERT(src->width() == tmpDst.width());
-        for (int y = 0; y < src->height(); ++y) {
-            SkPMColor16* SK_RESTRICT dstRow = (SkPMColor16*) tmpDst.getAddr16(0, y);
-            SkPMColor* SK_RESTRICT srcRow = (SkPMColor*) src->getAddr32(0, y);
-            DITHER_4444_SCAN(y);
-            for (int x = 0; x < src->width(); ++x) {
-                dstRow[x] = SkDitherARGB32To4444(srcRow[x],
-                                                 DITHER_VALUE(x));
-            }
-        }
-    } else {
-        if (tmpDst.alphaType() == kUnpremul_SkAlphaType) {
-            // We do not support drawing to unpremultiplied bitmaps.
-            return false;
-        }
-
-        // Always clear the dest in case one of the blitters accesses it
-        // TODO: switch the allocation of tmpDst to call sk_calloc_throw
-        tmpDst.eraseColor(SK_ColorTRANSPARENT);
-
-        SkCanvas canvas(tmpDst);
-        SkPaint  paint;
-
-        paint.setDither(true);
-        canvas.drawBitmap(*src, 0, 0, &paint);
     }
 
     dst->swap(tmpDst);