add readPixels() to SkBitmap

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 803 matching lines @@
     result->swap(dst);
     return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 #include "SkCanvas.h"
 #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:
         case kRGBA_8888_SkColorType:
         case kBGRA_8888_SkColorType:
             break;
         case kIndex_8_SkColorType:
             if (!sameConfigs) {
                 return false;
             }
             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)) {
         return false;
     }
 
     // if we have a texture, first get those pixels
     SkBitmap tmpSrc;
     const SkBitmap* src = this;
 
@@ skipping 54 matching lines @@
 
     if (!tmpDst.readyToDraw()) {
         // allocator/lock failed
         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->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);
     return true;
 }
 
 bool SkBitmap::deepCopyTo(SkBitmap* dst) const {
     const SkColorType dstCT = this->colorType();
 
     if (!this->canCopyTo(dstCT)) {
@@ skipping 398 matching lines @@
 ///////////////////////////////////////////////////////////////////////////////
 
 #ifdef SK_DEBUG
 void SkImageInfo::validate() const {
     SkASSERT(fWidth >= 0);
     SkASSERT(fHeight >= 0);
     SkASSERT(SkColorTypeIsValid(fColorType));
     SkASSERT(SkAlphaTypeIsValid(fAlphaType));
 }
 #endif