Revert "Refactor trimming logic for read/writePixels()"
This reverts commit 977f64cbfad1ecd7fd4b1231c694c7e828fda1f0.
Reason for revert: Triggering nanobench asserts
Original change's description:
> Refactor trimming logic for read/writePixels()
>
> (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.
>
> BUG=skia:6021
>
> Change-Id: I748f50c3b726f7c6de5462e2b1ccb54bc387a510
> Reviewed-on: https://skia-review.googlesource.com/7326
> Reviewed-by: Mike Reed <reed@google.com>
> Commit-Queue: Matt Sarett <msarett@google.com>
>
TBR=msarett@google.com,brianosman@google.com,reed@google.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:6021
Change-Id: If9aacc6ce8b20e3dfe8a0f22ebca653f28356175
Reviewed-on: https://skia-review.googlesource.com/7379
Commit-Queue: Matt Sarett <msarett@google.com>
Reviewed-by: Matt Sarett <msarett@google.com>
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 7d62643..00b2f8e 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -22,7 +22,6 @@
#include "SkTemplates.h"
#include "SkUnPreMultiply.h"
#include "SkWriteBuffer.h"
-#include "SkWritePixelsRec.h"
#include <string.h>
@@ -683,6 +682,7 @@
case kBGRA_8888_SkColorType:
break;
case kGray_8_SkColorType:
+ case kIndex_8_SkColorType:
if (!sameConfigs) {
return false;
}
@@ -714,15 +714,13 @@
return false;
}
- SkWritePixelsRec rec(src.info(), src.addr(), src.rowBytes(), dstX, dstY);
- if (!rec.trim(fInfo.width(), fInfo.height())) {
+ SkPixmap subset;
+ if (!dst.pixmap().extractSubset(&subset,
+ SkIRect::MakeXYWH(dstX, dstY, src.width(), src.height()))) {
return false;
}
- 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());
+ return src.readPixels(subset);
}
bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc) const {
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 76f4a70..0375ab7 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -30,6 +30,7 @@
#include "SkRadialShadowMapShader.h"
#include "SkRasterClip.h"
#include "SkRasterHandleAllocator.h"
+#include "SkReadPixelsRec.h"
#include "SkRRect.h"
#include "SkShadowPaintFilterCanvas.h"
#include "SkShadowShader.h"
@@ -857,6 +858,10 @@
}
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()) {
@@ -903,8 +908,15 @@
if (!device) {
return false;
}
+ const SkISize size = this->getBaseLayerSize();
- return device->readPixels(dstInfo, dstP, rowBytes, x, y);
+ 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);
}
bool SkCanvas::writePixels(const SkBitmap& bitmap, int x, int y) {
@@ -916,30 +928,49 @@
return false;
}
-bool SkCanvas::writePixels(const SkImageInfo& srcInfo, const void* pixels, size_t rowBytes,
+bool SkCanvas::writePixels(const SkImageInfo& origInfo, 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;
}
- // 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;
- }
+ // the intersect may have shrunk info's logical size
+ const SkImageInfo info = origInfo.makeWH(target.width(), target.height());
- // Tell our owning surface to bump its generation ID.
- const bool completeOverwrite =
- srcRect.size() == SkISize::Make(device->width(), device->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
+ pixels = ((const char*)pixels - y * rowBytes - x * info.bytesPerPixel());
+
+ // Tell our owning surface to bump its generation ID
+ const bool completeOverwrite = info.dimensions() == size;
this->predrawNotify(completeOverwrite);
- // 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);
+ // The device can assert that the requested area is always contained in its bounds
+ return device->writePixels(info, pixels, rowBytes, target.x(), target.y());
}
//////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkConfig8888.cpp b/src/core/SkConfig8888.cpp
index d75ea27..98e5b7a 100644
--- a/src/core/SkConfig8888.cpp
+++ b/src/core/SkConfig8888.cpp
@@ -371,7 +371,7 @@
const int height = srcInfo.height();
// Do the easiest one first : both configs are equal
- if (srcInfo == dstInfo) {
+ if (srcInfo == dstInfo && kIndex_8_SkColorType != srcInfo.colorType()) {
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 ab75dbc..7800c37 100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -319,11 +319,31 @@
///////////////////////////////////////////////////////////////////////////////////////////////////
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 1b7c09b..0453c7b 100644
--- a/src/core/SkImageInfo.cpp
+++ b/src/core/SkImageInfo.cpp
@@ -98,6 +98,9 @@
#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;
}
@@ -128,39 +131,3 @@
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 f04acc5..151de82 100644
--- a/src/core/SkImageInfoPriv.h
+++ b/src/core/SkImageInfoPriv.h
@@ -39,7 +39,8 @@
/**
* 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. Do we overwrite the input color table?
+ * We will not convert to kIndex8 unless |src| is kIndex8. This is possible only
+ * in some cases and likley inefficient.
* 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.
@@ -56,7 +57,7 @@
return false;
}
- if (kIndex_8_SkColorType == dst.colorType()) {
+ if (kIndex_8_SkColorType == dst.colorType() && kIndex_8_SkColorType != src.colorType()) {
return false;
}
diff --git a/src/core/SkPixmap.cpp b/src/core/SkPixmap.cpp
index c0889cfd..1e24b93 100644
--- a/src/core/SkPixmap.cpp
+++ b/src/core/SkPixmap.cpp
@@ -16,7 +16,6 @@
#include "SkNx.h"
#include "SkPM4f.h"
#include "SkPixmap.h"
-#include "SkReadPixelsRec.h"
#include "SkSurface.h"
#include "SkUtils.h"
@@ -84,20 +83,37 @@
return true;
}
-bool SkPixmap::readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB, int x, int y)
-const {
- if (!SkImageInfoValidConversion(dstInfo, fInfo)) {
+bool SkPixmap::readPixels(const SkImageInfo& requestedDstInfo, void* dstPixels, size_t dstRB,
+ int x, int y) const {
+ if (!SkImageInfoValidConversion(requestedDstInfo, fInfo)) {
return false;
}
- SkReadPixelsRec rec(dstInfo, dstPixels, dstRB, x, y);
- if (!rec.trim(fInfo.width(), fInfo.height())) {
+ if (nullptr == dstPixels || dstRB < requestedDstInfo.minRowBytes()) {
return false;
}
- 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,
+ 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,
srcInfo, srcPixels, this->rowBytes(), this->ctable());
}
diff --git a/src/core/SkWritePixelsRec.h b/src/core/SkWritePixelsRec.h
deleted file mode 100644
index 652a13a..0000000
--- a/src/core/SkWritePixelsRec.h
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * 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