Reland "Refactor trimming logic for read/writePixels()"

Original CL: https://skia-review.googlesource.com/c/7326/

(1) Move trimming logic into Bitmap/Pixmap level for
    raster.  Everything goes through here, so we'll
    only do the work once.
(2) This means it also goes to GPU level.
(3) Always use SkReadPixelsRec rather than inlining
    the logic.
(4) Create an SkWritePixelsRec to encapsulate write
    trimming.
(5) Disabled kIndex8 as a dst - always.

CQ_INCLUDE_TRYBOTS=skia.primary:Perf-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug

BUG=skia:6021

Change-Id: I25a964e3c610c4e36d195a255e2150657baec649
Reviewed-on: https://skia-review.googlesource.com/7404
Reviewed-by: Matt Sarett <msarett@google.com>
Commit-Queue: Matt Sarett <msarett@google.com>

11 files changed, 146 insertions, 118 deletions

diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 00b2f8e503..f54488d0a5 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -12,6 +12,7 @@
 #include "SkData.h"
 #include "SkFilterQuality.h"
 #include "SkHalf.h"
+#include "SkImageInfoPriv.h"
 #include "SkMallocPixelRef.h"
 #include "SkMask.h"
 #include "SkMath.h"
@@ -22,6 +23,7 @@
 #include "SkTemplates.h"
 #include "SkUnPreMultiply.h"
 #include "SkWriteBuffer.h"
+#include "SkWritePixelsRec.h"
 
 #include <string.h>
 
@@ -682,7 +684,6 @@ bool SkBitmap::canCopyTo(SkColorType dstCT) const {
         case kBGRA_8888_SkColorType:
             break;
         case kGray_8_SkColorType:
-        case kIndex_8_SkColorType:
             if (!sameConfigs) {
                 return false;
             }
@@ -714,13 +715,19 @@ bool SkBitmap::writePixels(const SkPixmap& src, int dstX, int dstY) {
         return false;
     }
 
-    SkPixmap subset;
-    if (!dst.pixmap().extractSubset(&subset,
-                                    SkIRect::MakeXYWH(dstX, dstY, src.width(), src.height()))) {
+    if (!SkImageInfoValidConversion(fInfo, src.info())) {
+        return false;
+    }
+
+    SkWritePixelsRec rec(src.info(), src.addr(), src.rowBytes(), dstX, dstY);
+    if (!rec.trim(fInfo.width(), fInfo.height())) {
         return false;
     }
 
-    return src.readPixels(subset);
+    void* dstPixels = this->getAddr(rec.fX, rec.fY);
+    const SkImageInfo dstInfo = fInfo.makeWH(rec.fInfo.width(), rec.fInfo.height());
+    return SkPixelInfo::CopyPixels(dstInfo, dstPixels, this->rowBytes(),
+                                   rec.fInfo, rec.fPixels, rec.fRowBytes, src.ctable());
 }
 
 bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc) const {
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index d57b62fa99..7596c3a702 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -30,7 +30,6 @@
 #include "SkRadialShadowMapShader.h"
 #include "SkRasterClip.h"
 #include "SkRasterHandleAllocator.h"
-#include "SkReadPixelsRec.h"
 #include "SkRRect.h"
 #include "SkShadowPaintFilterCanvas.h"
 #include "SkShadowShader.h"
@@ -858,10 +857,6 @@ SkBaseDevice* SkCanvas::getTopDevice() const {
 }
 
 bool SkCanvas::readPixels(SkBitmap* bitmap, int x, int y) {
-    if (kUnknown_SkColorType == bitmap->colorType()) {
-        return false;
-    }
-
     bool weAllocated = false;
     if (nullptr == bitmap->pixelRef()) {
         if (!bitmap->tryAllocPixels()) {
@@ -908,15 +903,8 @@ bool SkCanvas::readPixels(const SkImageInfo& dstInfo, void* dstP, size_t rowByte
     if (!device) {
         return false;
     }
-    const SkISize size = this->getBaseLayerSize();
 
-    SkReadPixelsRec rec(dstInfo, dstP, rowBytes, x, y);
-    if (!rec.trim(size.width(), size.height())) {
-        return false;
-    }
-
-    // The device can assert that the requested area is always contained in its bounds
-    return device->readPixels(rec.fInfo, rec.fPixels, rec.fRowBytes, rec.fX, rec.fY);
+    return device->readPixels(dstInfo, dstP, rowBytes, x, y);
 }
 
 bool SkCanvas::writePixels(const SkBitmap& bitmap, int x, int y) {
@@ -928,49 +916,30 @@ bool SkCanvas::writePixels(const SkBitmap& bitmap, int x, int y) {
     return false;
 }
 
-bool SkCanvas::writePixels(const SkImageInfo& origInfo, const void* pixels, size_t rowBytes,
+bool SkCanvas::writePixels(const SkImageInfo& srcInfo, const void* pixels, size_t rowBytes,
                            int x, int y) {
-    switch (origInfo.colorType()) {
-        case kUnknown_SkColorType:
-        case kIndex_8_SkColorType:
-            return false;
-        default:
-            break;
-    }
-    if (nullptr == pixels || rowBytes < origInfo.minRowBytes()) {
-        return false;
-    }
-
-    const SkISize size = this->getBaseLayerSize();
-    SkIRect target = SkIRect::MakeXYWH(x, y, origInfo.width(), origInfo.height());
-    if (!target.intersect(0, 0, size.width(), size.height())) {
-        return false;
-    }
-
     SkBaseDevice* device = this->getDevice();
     if (!device) {
         return false;
     }
 
-    // the intersect may have shrunk info's logical size
-    const SkImageInfo info = origInfo.makeWH(target.width(), target.height());
-
-    // if x or y are negative, then we have to adjust pixels
-    if (x > 0) {
-        x = 0;
-    }
-    if (y > 0) {
-        y = 0;
+    // This check gives us an early out and prevents generation ID churn on the surface.
+    // This is purely optional: it is a subset of the checks performed by SkWritePixelsRec.
+    SkIRect srcRect = SkIRect::MakeXYWH(x, y, srcInfo.width(), srcInfo.height());
+    if (!srcRect.intersect(0, 0, device->width(), device->height())) {
+        return false;
     }
-    // here x,y are either 0 or negative
-    pixels = ((const char*)pixels - y * rowBytes - x * info.bytesPerPixel());
 
-    // Tell our owning surface to bump its generation ID
-    const bool completeOverwrite = info.dimensions() == size;
+    // Tell our owning surface to bump its generation ID.
+    const bool completeOverwrite =
+            srcRect.size() == SkISize::Make(device->width(), device->height());
     this->predrawNotify(completeOverwrite);
 
-    // The device can assert that the requested area is always contained in its bounds
-    return device->writePixels(info, pixels, rowBytes, target.x(), target.y());
+    // This can still fail, most notably in the case of a invalid color type or alpha type
+    // conversion.  We could pull those checks into this function and avoid the unnecessary
+    // generation ID bump.  But then we would be performing those checks twice, since they
+    // are also necessary at the bitmap/pixmap entry points.
+    return device->writePixels(srcInfo, pixels, rowBytes, x, y);
 }
 
 //////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkConfig8888.cpp b/src/core/SkConfig8888.cpp
index 98e5b7aced..d75ea27dc6 100644
--- a/src/core/SkConfig8888.cpp
+++ b/src/core/SkConfig8888.cpp
@@ -371,7 +371,7 @@ bool SkPixelInfo::CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t
     const int height = srcInfo.height();
 
     // Do the easiest one first : both configs are equal
-    if (srcInfo == dstInfo && kIndex_8_SkColorType != srcInfo.colorType()) {
+    if (srcInfo == dstInfo) {
         size_t bytes = width * srcInfo.bytesPerPixel();
         for (int y = 0; y < height; ++y) {
             memcpy(dstPixels, srcPixels, bytes);
diff --git a/src/core/SkDevice.cpp b/src/core/SkDevice.cpp
index 7800c370c1..ab75dbc0bc 100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -319,31 +319,11 @@ sk_sp<SkSpecialImage> SkBaseDevice::snapSpecial() { return nullptr; }
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 bool SkBaseDevice::readPixels(const SkImageInfo& info, void* dstP, size_t rowBytes, int x, int y) {
-#ifdef SK_DEBUG
-    SkASSERT(info.width() > 0 && info.height() > 0);
-    SkASSERT(dstP);
-    SkASSERT(rowBytes >= info.minRowBytes());
-    SkASSERT(x >= 0 && y >= 0);
-
-    const SkImageInfo& srcInfo = this->imageInfo();
-    SkASSERT(x + info.width() <= srcInfo.width());
-    SkASSERT(y + info.height() <= srcInfo.height());
-#endif
     return this->onReadPixels(info, dstP, rowBytes, x, y);
 }
 
 bool SkBaseDevice::writePixels(const SkImageInfo& info, const void* pixels, size_t rowBytes,
                                int x, int y) {
-#ifdef SK_DEBUG
-    SkASSERT(info.width() > 0 && info.height() > 0);
-    SkASSERT(pixels);
-    SkASSERT(rowBytes >= info.minRowBytes());
-    SkASSERT(x >= 0 && y >= 0);
-
-    const SkImageInfo& dstInfo = this->imageInfo();
-    SkASSERT(x + info.width() <= dstInfo.width());
-    SkASSERT(y + info.height() <= dstInfo.height());
-#endif
     return this->onWritePixels(info, pixels, rowBytes, x, y);
 }
 
diff --git a/src/core/SkImageInfo.cpp b/src/core/SkImageInfo.cpp
index 0453c7b38f..1b7c09b2f9 100644
--- a/src/core/SkImageInfo.cpp
+++ b/src/core/SkImageInfo.cpp
@@ -98,9 +98,6 @@ bool SkColorTypeValidateAlphaType(SkColorType colorType, SkAlphaType alphaType,
 #include "SkReadPixelsRec.h"
 
 bool SkReadPixelsRec::trim(int srcWidth, int srcHeight) {
-    if (kIndex_8_SkColorType == fInfo.colorType()) {
-        return false;
-    }
     if (nullptr == fPixels || fRowBytes < fInfo.minRowBytes()) {
         return false;
     }
@@ -131,3 +128,39 @@ bool SkReadPixelsRec::trim(int srcWidth, int srcHeight) {
 
     return true;
 }
+
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+#include "SkWritePixelsRec.h"
+
+bool SkWritePixelsRec::trim(int dstWidth, int dstHeight) {
+    if (nullptr == fPixels || fRowBytes < fInfo.minRowBytes()) {
+        return false;
+    }
+    if (0 >= fInfo.width() || 0 >= fInfo.height()) {
+        return false;
+    }
+
+    int x = fX;
+    int y = fY;
+    SkIRect dstR = SkIRect::MakeXYWH(x, y, fInfo.width(), fInfo.height());
+    if (!dstR.intersect(0, 0, dstWidth, dstHeight)) {
+        return false;
+    }
+
+    // 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
+    fPixels = ((const char*)fPixels - y * fRowBytes - x * fInfo.bytesPerPixel());
+    // the intersect may have shrunk info's logical size
+    fInfo = fInfo.makeWH(dstR.width(), dstR.height());
+    fX = dstR.x();
+    fY = dstR.y();
+
+    return true;
+}
diff --git a/src/core/SkImageInfoPriv.h b/src/core/SkImageInfoPriv.h
index 151de82632..f04acc5484 100644
--- a/src/core/SkImageInfoPriv.h
+++ b/src/core/SkImageInfoPriv.h
@@ -39,8 +39,7 @@ static inline bool SkImageInfoIsValid(const SkImageInfo& info) {
 /**
  *  Returns true if Skia has defined a pixel conversion from the |src| to the |dst|.
  *  Returns false otherwise.  Some discussion of false cases:
- *      We will not convert to kIndex8 unless |src| is kIndex8.  This is possible only
- *      in some cases and likley inefficient.
+ *      We will not convert to kIndex8.  Do we overwrite the input color table?
  *      We do not convert to kGray8 when the |src| is not kGray8.  We may add this
  *      feature - it just requires some work to convert to luminance while handling color
  *      spaces correctly.  Currently no one is asking for this.
@@ -57,7 +56,7 @@ static inline bool SkImageInfoValidConversion(const SkImageInfo& dst, const SkIm
         return false;
     }
 
-    if (kIndex_8_SkColorType == dst.colorType() && kIndex_8_SkColorType != src.colorType()) {
+    if (kIndex_8_SkColorType == dst.colorType()) {
         return false;
     }
 
diff --git a/src/core/SkPixmap.cpp b/src/core/SkPixmap.cpp
index 1e24b93a60..c0889cfd84 100644
--- a/src/core/SkPixmap.cpp
+++ b/src/core/SkPixmap.cpp
@@ -16,6 +16,7 @@
 #include "SkNx.h"
 #include "SkPM4f.h"
 #include "SkPixmap.h"
+#include "SkReadPixelsRec.h"
 #include "SkSurface.h"
 #include "SkUtils.h"
 
@@ -83,37 +84,20 @@ bool SkPixmap::extractSubset(SkPixmap* result, const SkIRect& subset) const {
     return true;
 }
 
-bool SkPixmap::readPixels(const SkImageInfo& requestedDstInfo, void* dstPixels, size_t dstRB,
-                          int x, int y) const {
-    if (!SkImageInfoValidConversion(requestedDstInfo, fInfo)) {
+bool SkPixmap::readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB, int x, int y)
+const {
+    if (!SkImageInfoValidConversion(dstInfo, fInfo)) {
         return false;
     }
 
-    if (nullptr == dstPixels || dstRB < requestedDstInfo.minRowBytes()) {
+    SkReadPixelsRec rec(dstInfo, dstPixels, dstRB, x, y);
+    if (!rec.trim(fInfo.width(), fInfo.height())) {
         return false;
     }
 
-    SkIRect srcR = SkIRect::MakeXYWH(x, y, requestedDstInfo.width(), requestedDstInfo.height());
-    if (!srcR.intersect(0, 0, this->width(), this->height())) {
-        return false;
-    }
-
-    // the intersect may have shrunk info's logical size
-    const SkImageInfo dstInfo = requestedDstInfo.makeWH(srcR.width(), srcR.height());
-
-    // 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 * dstInfo.bytesPerPixel());
-
-    const SkImageInfo srcInfo = this->info().makeWH(dstInfo.width(), dstInfo.height());
-    const void* srcPixels = this->addr(srcR.x(), srcR.y());
-    return SkPixelInfo::CopyPixels(dstInfo, dstPixels, dstRB,
+    const void* srcPixels = this->addr(rec.fX, rec.fY);
+    const SkImageInfo srcInfo = fInfo.makeWH(rec.fInfo.width(), rec.fInfo.height());
+    return SkPixelInfo::CopyPixels(rec.fInfo, rec.fPixels, rec.fRowBytes,
                                    srcInfo, srcPixels, this->rowBytes(), this->ctable());
 }
 
diff --git a/src/core/SkWritePixelsRec.h b/src/core/SkWritePixelsRec.h
new file mode 100644
index 0000000000..652a13a822
--- /dev/null
+++ b/src/core/SkWritePixelsRec.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkWritePixelsRec_DEFINED
+#define SkWritePixelsRec_DEFINED
+
+#include "SkImageInfo.h"
+
+/**
+ *  Helper class to package and trim the parameters passed to writePixels()
+ */
+struct SkWritePixelsRec {
+    SkWritePixelsRec(const SkImageInfo& info, const void* pixels, size_t rowBytes, int x, int y)
+        : fPixels(pixels)
+        , fRowBytes(rowBytes)
+        , fInfo(info)
+        , fX(x)
+        , fY(y)
+    {}
+
+    const void* fPixels;
+    size_t      fRowBytes;
+    SkImageInfo fInfo;
+    int         fX;
+    int         fY;
+
+    /*
+     *  On true, may have modified its fields (except fRowBytes) to make it a legal subset
+     *  of the specified dst width/height.
+     *
+     *  On false, leaves self unchanged, but indicates that it does not overlap dst, or
+     *  is not valid (e.g. bad fInfo) for writePixels().
+     */
+    bool trim(int dstWidth, int dstHeight);
+};
+
+#endif
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index 85eb671206..c5ade9e7b1 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -35,6 +35,7 @@
 #include "SkPictureData.h"
 #include "SkRRect.h"
 #include "SkRasterClip.h"
+#include "SkReadPixelsRec.h"
 #include "SkRecord.h"
 #include "SkSpecialImage.h"
 #include "SkStroke.h"
@@ -43,6 +44,7 @@
 #include "SkTLazy.h"
 #include "SkUtils.h"
 #include "SkVertState.h"
+#include "SkWritePixelsRec.h"
 #include "effects/GrBicubicEffect.h"
 #include "effects/GrSimpleTextureEffect.h"
 #include "effects/GrTextureDomain.h"
@@ -199,7 +201,12 @@ bool SkGpuDevice::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size
         return false;
     }
 
-    return fRenderTargetContext->readPixels(dstInfo, dstPixels, dstRowBytes, x, y);
+    SkReadPixelsRec rec(dstInfo, dstPixels, dstRowBytes, x, y);
+    if (!rec.trim(this->width(), this->height())) {
+        return false;
+    }
+
+    return fRenderTargetContext->readPixels(rec.fInfo, rec.fPixels, rec.fRowBytes, rec.fX, rec.fY);
 }
 
 bool SkGpuDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPixels,
@@ -210,7 +217,12 @@ bool SkGpuDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPixel
         return false;
     }
 
-    return fRenderTargetContext->writePixels(srcInfo, srcPixels, srcRowBytes, x, y);
+    SkWritePixelsRec rec(srcInfo, srcPixels, srcRowBytes, x, y);
+    if (!rec.trim(this->width(), this->height())) {
+        return false;
+    }
+
+    return fRenderTargetContext->writePixels(rec.fInfo, rec.fPixels, rec.fRowBytes, rec.fX, rec.fY);
 }
 
 bool SkGpuDevice::onAccessPixels(SkPixmap* pmap) {
diff --git a/src/image/SkImage.cpp b/src/image/SkImage.cpp
index 137f9166ba..8efe91a706 100644
--- a/src/image/SkImage.cpp
+++ b/src/image/SkImage.cpp
@@ -51,11 +51,7 @@ bool SkImage::peekPixels(SkPixmap* pm) const {
 
 bool SkImage::readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
                            int srcX, int srcY, CachingHint chint) const {
-    SkReadPixelsRec rec(dstInfo, dstPixels, dstRowBytes, srcX, srcY);
-    if (!rec.trim(this->width(), this->height())) {
-        return false;
-    }
-    return as_IB(this)->onReadPixels(rec.fInfo, rec.fPixels, rec.fRowBytes, rec.fX, rec.fY, chint);
+    return as_IB(this)->onReadPixels(dstInfo, dstPixels, dstRowBytes, srcX, srcY, chint);
 }
 
 bool SkImage::scalePixels(const SkPixmap& dst, SkFilterQuality quality, CachingHint chint) const {
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index 7247ae36fa..fb93350039 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -27,6 +27,7 @@
 #include "SkImageInfoPriv.h"
 #include "SkMipMap.h"
 #include "SkPixelRef.h"
+#include "SkReadPixelsRec.h"
 
 SkImage_Gpu::SkImage_Gpu(int w, int h, uint32_t uniqueID, SkAlphaType at, sk_sp<GrTexture> tex,
                          sk_sp<SkColorSpace> colorSpace, SkBudgeted budgeted)
@@ -128,20 +129,26 @@ static void apply_premul(const SkImageInfo& info, void* pixels, size_t rowBytes)
     }
 }
 
-bool SkImage_Gpu::onReadPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
+bool SkImage_Gpu::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
                                int srcX, int srcY, CachingHint) const {
-    if (!SkImageInfoValidConversion(info, this->onImageInfo())) {
+    if (!SkImageInfoValidConversion(dstInfo, this->onImageInfo())) {
         return false;
     }
 
-    GrPixelConfig config = SkImageInfo2GrPixelConfig(info, *fTexture->getContext()->caps());
+    SkReadPixelsRec rec(dstInfo, dstPixels, dstRB, srcX, srcY);
+    if (!rec.trim(this->width(), this->height())) {
+        return false;
+    }
+
+    GrPixelConfig config = SkImageInfo2GrPixelConfig(rec.fInfo, *fTexture->getContext()->caps());
     uint32_t flags = 0;
-    if (kUnpremul_SkAlphaType == info.alphaType() && kPremul_SkAlphaType == fAlphaType) {
+    if (kUnpremul_SkAlphaType == rec.fInfo.alphaType() && kPremul_SkAlphaType == fAlphaType) {
         // let the GPU perform this transformation for us
         flags = GrContext::kUnpremul_PixelOpsFlag;
     }
-    if (!fTexture->readPixels(fColorSpace.get(), srcX, srcY, info.width(), info.height(), config,
-                              info.colorSpace(), pixels, rowBytes, flags)) {
+    if (!fTexture->readPixels(fColorSpace.get(), rec.fX, rec.fY, rec.fInfo.width(),
+                              rec.fInfo.height(), config, rec.fInfo.colorSpace(), rec.fPixels,
+                              rec.fRowBytes, flags)) {
         return false;
     }
     // do we have to manually fix-up the alpha channel?
@@ -152,8 +159,8 @@ bool SkImage_Gpu::onReadPixels(const SkImageInfo& info, void* pixels, size_t row
     //
     // Should this be handled by Ganesh? todo:?
     //
-    if (kPremul_SkAlphaType == info.alphaType() && kUnpremul_SkAlphaType == fAlphaType) {
-        apply_premul(info, pixels, rowBytes);
+    if (kPremul_SkAlphaType == rec.fInfo.alphaType() && kUnpremul_SkAlphaType == fAlphaType) {
+        apply_premul(rec.fInfo, rec.fPixels, rec.fRowBytes);
     }
     return true;
 }