Revert of Add SkBitmap::readPixels() and reimplement copyTo and SkCanvas::readPixels (https://codereview.chromium.org/388803007/)

Reason for revert:
still failing (randomly?) bench sometimes. need stack dump to diagnose.

Original issue's description:
> Add SkBitmap::readPixels() and reimplement copyTo and SkCanvas::readPixels
> usning it.
>
> Revert "Revert of add readPixels() to SkBitmap (https://codereview.chromium.org/377303002/)"
>
> This reverts commit d08cb905a7.
>
> TBR=scroggo@google.com
>
> Committed: https://skia.googlesource.com/skia/+/debba5c3d091159149f8a88ab5dcd44dd72e0dc7

R=reed@google.com
TBR=reed@google.com
NOTREECHECKS=true
NOTRY=true

Author: reed@chromium.org

Review URL: https://codereview.chromium.org/382543005
This commit is contained in:
reed 2014-07-12 13:16:10 -07:00 committed by Commit bot
parent debba5c3d0
commit 651eaeadeb
7 changed files with 141 additions and 346 deletions

View File

@ -590,28 +590,6 @@ public:
return this->copyTo(dst, this->colorType(), allocator);
}
/**
* Copy the bitmap's pixels into the specified buffer (pixels + rowBytes),
* converting them into the requested format (SkImageInfo). The src pixels are read
* starting at the specified (srcX,srcY) offset, relative to the top-left corner.
*
* The specified ImageInfo and (srcX,srcY) offset specifies a source rectangle
*
* srcR.setXYWH(srcX, srcY, dstInfo.width(), dstInfo.height());
*
* srcR is intersected with the bounds of the bitmap. If this intersection is not empty,
* then we have two sets of pixels (of equal size). Replace the dst pixels with the
* corresponding src pixels, performing any colortype/alphatype transformations needed
* (in the case where the src and dst have different colortypes or alphatypes).
*
* This call can fail, returning false, for several reasons:
* - If srcR does not intersect the bitmap bounds.
* - If the requested colortype/alphatype cannot be converted from the src's types.
* - If the src pixels are not available.
*/
bool readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
int srcX, int srcY) const;
/**
* Returns true if this bitmap's pixels can be converted into the requested
* colorType, such that copyTo() could succeed.

View File

@ -234,31 +234,30 @@ public:
/**
* Copy the pixels from the base-layer into the specified buffer (pixels + rowBytes),
* converting them into the requested format (SkImageInfo). The base-layer pixels are read
* starting at the specified (srcX,srcY) location in the coordinate system of the base-layer.
* starting at the specified (x,y) location in the coordinate system of the base-layer.
*
* The specified ImageInfo and (srcX,srcY) offset specifies a source rectangle
* The specified ImageInfo and (x,y) offset specifies a source rectangle
*
* srcR.setXYWH(srcX, srcY, dstInfo.width(), dstInfo.height());
* srcR(x, y, info.width(), info.height());
*
* srcR is intersected with the bounds of the base-layer. If this intersection is not empty,
* then we have two sets of pixels (of equal size). Replace the dst pixels with the
* corresponding src pixels, performing any colortype/alphatype transformations needed
* (in the case where the src and dst have different colortypes or alphatypes).
* SrcR is intersected with the bounds of the base-layer. If this intersection is not empty,
* then we have two sets of pixels (of equal size), the "src" specified by base-layer at (x,y)
* and the "dst" by info+pixels+rowBytes. Replace the dst pixels with the corresponding src
* pixels, performing any colortype/alphatype transformations needed (in the case where the
* src and dst have different colortypes or alphatypes).
*
* This call can fail, returning false, for several reasons:
* - If srcR does not intersect the base-layer bounds.
* - If the requested colortype/alphatype cannot be converted from the base-layer's types.
* - If this canvas is not backed by pixels (e.g. picture or PDF)
*/
bool readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
int srcX, int srcY);
bool readPixels(const SkImageInfo&, void* pixels, size_t rowBytes, int x, int y);
/**
* Helper for calling readPixels(info, ...). This call will check if bitmap has been allocated.
* If not, it will attempt to call allocPixels(). If this fails, it will return false. If not,
* it calls through to readPixels(info, ...) and returns its result.
*/
bool readPixels(SkBitmap* bitmap, int srcX, int srcY);
bool readPixels(SkBitmap* bitmap, int x, int y);
/**
* Helper for allocating pixels and then calling readPixels(info, ...). The bitmap is resized

View File

@ -821,13 +821,11 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
#include "SkPaint.h"
bool SkBitmap::canCopyTo(SkColorType dstColorType) const {
const SkColorType srcCT = this->colorType();
if (srcCT == kUnknown_SkColorType) {
if (this->colorType() == kUnknown_SkColorType) {
return false;
}
bool sameConfigs = (srcCT == dstColorType);
bool sameConfigs = (this->colorType() == dstColorType);
switch (dstColorType) {
case kAlpha_8_SkColorType:
case kRGB_565_SkColorType:
@ -840,66 +838,15 @@ bool SkBitmap::canCopyTo(SkColorType dstColorType) const {
}
break;
case kARGB_4444_SkColorType:
return sameConfigs || kN32_SkColorType == srcCT || kIndex_8_SkColorType == srcCT;
return sameConfigs || kN32_SkColorType == this->colorType();
default:
return false;
}
return true;
}
#include "SkConfig8888.h"
bool SkBitmap::readPixels(const SkImageInfo& requestedDstInfo, void* dstPixels, size_t dstRB,
int x, int y) const {
if (kUnknown_SkColorType == requestedDstInfo.colorType()) {
return false;
}
if (NULL == dstPixels || dstRB < requestedDstInfo.minRowBytes()) {
return false;
}
if (0 == requestedDstInfo.width() || 0 == requestedDstInfo.height()) {
return false;
}
SkIRect srcR = SkIRect::MakeXYWH(x, y, requestedDstInfo.width(), requestedDstInfo.height());
if (!srcR.intersect(0, 0, this->width(), this->height())) {
return false;
}
SkImageInfo dstInfo = requestedDstInfo;
// the intersect may have shrunk info's logical size
dstInfo.fWidth = srcR.width();
dstInfo.fHeight = 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());
//////////////
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();
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 {
bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType,
Allocator* alloc) const {
if (!this->canCopyTo(dstColorType)) {
return false;
}
@ -972,21 +919,59 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc)
// returned false.
SkASSERT(tmpDst.pixelRef() != NULL);
if (!src->readPixels(tmpDst.info(), tmpDst.getPixels(), tmpDst.rowBytes(), 0, 0)) {
return false;
}
/* 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());
// (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.
//
// TODO: should we ignore rowbytes (i.e. getSize)? Then it could just be
// if (src_pixelref->info == dst_pixelref->info)
//
if (src->colorType() == dstColorType && tmpDst.getSize() == src->getSize()) {
SkPixelRef* dstPixelRef = tmpDst.pixelRef();
if (dstPixelRef->info() == fPixelRef->info()) {
dstPixelRef->cloneGenID(*fPixelRef);
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();
}
}
} 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);

View File

@ -141,8 +141,59 @@ void* SkBitmapDevice::onAccessPixels(SkImageInfo* info, size_t* rowBytes) {
return NULL;
}
static void rect_memcpy(void* dst, size_t dstRB, const void* src, size_t srcRB, size_t bytesPerRow,
int rowCount) {
SkASSERT(bytesPerRow <= srcRB);
SkASSERT(bytesPerRow <= dstRB);
for (int i = 0; i < rowCount; ++i) {
memcpy(dst, src, bytesPerRow);
dst = (char*)dst + dstRB;
src = (const char*)src + srcRB;
}
}
#include "SkConfig8888.h"
static bool copy_pixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRowBytes) {
if (srcInfo.dimensions() != dstInfo.dimensions()) {
return false;
}
if (4 == srcInfo.bytesPerPixel() && 4 == dstInfo.bytesPerPixel()) {
SkDstPixelInfo dstPI;
dstPI.fColorType = dstInfo.colorType();
dstPI.fAlphaType = dstInfo.alphaType();
dstPI.fPixels = dstPixels;
dstPI.fRowBytes = dstRowBytes;
SkSrcPixelInfo srcPI;
srcPI.fColorType = srcInfo.colorType();
srcPI.fAlphaType = srcInfo.alphaType();
srcPI.fPixels = srcPixels;
srcPI.fRowBytes = srcRowBytes;
return srcPI.convertPixelsTo(&dstPI, srcInfo.width(), srcInfo.height());
}
if (srcInfo.colorType() == dstInfo.colorType()) {
switch (srcInfo.colorType()) {
case kRGB_565_SkColorType:
case kAlpha_8_SkColorType:
break;
case kARGB_4444_SkColorType:
if (srcInfo.alphaType() != dstInfo.alphaType()) {
return false;
}
break;
default:
return false;
}
rect_memcpy(dstPixels, dstRowBytes, srcPixels, srcRowBytes,
srcInfo.width() * srcInfo.bytesPerPixel(), srcInfo.height());
}
// TODO: add support for more conversions as needed
return false;
}
bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPixels,
size_t srcRowBytes, int x, int y) {
// since we don't stop creating un-pixeled devices yet, check for no pixels here
@ -157,7 +208,7 @@ bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPi
void* dstPixels = fBitmap.getAddr(x, y);
size_t dstRowBytes = fBitmap.rowBytes();
if (SkPixelInfo::CopyPixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes)) {
if (copy_pixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes)) {
fBitmap.notifyPixelsChanged();
return true;
}
@ -166,7 +217,28 @@ bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPi
bool SkBitmapDevice::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
int x, int y) {
return fBitmap.readPixels(dstInfo, dstPixels, dstRowBytes, x, y);
// since we don't stop creating un-pixeled devices yet, check for no pixels here
if (NULL == fBitmap.getPixels()) {
return false;
}
SkImageInfo srcInfo = fBitmap.info();
// perhaps can relax these in the future
if (4 != dstInfo.bytesPerPixel()) {
return false;
}
if (4 != srcInfo.bytesPerPixel()) {
return false;
}
srcInfo.fWidth = dstInfo.width();
srcInfo.fHeight = dstInfo.height();
const void* srcPixels = fBitmap.getAddr(x, y);
const size_t srcRowBytes = fBitmap.rowBytes();
return copy_pixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes);
}
///////////////////////////////////////////////////////////////////////////////

View File

@ -1,15 +1,5 @@
/*
* Copyright 2014 Google Inc.
*
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
#include "SkBitmap.h"
#include "SkCanvas.h"
#include "SkConfig8888.h"
#include "SkColorPriv.h"
#include "SkDither.h"
#include "SkMathPriv.h"
#include "SkUnPreMultiply.h"
@ -125,135 +115,3 @@ bool SkSrcPixelInfo::convertPixelsTo(SkDstPixelInfo* dst, int width, int height)
}
return true;
}
static void rect_memcpy(void* dst, size_t dstRB, const void* src, size_t srcRB, size_t bytesPerRow,
int rowCount) {
SkASSERT(bytesPerRow <= srcRB);
SkASSERT(bytesPerRow <= dstRB);
for (int i = 0; i < rowCount; ++i) {
memcpy(dst, src, bytesPerRow);
dst = (char*)dst + dstRB;
src = (const char*)src + srcRB;
}
}
bool SkPixelInfo::CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRB,
SkColorTable* ctable) {
if (srcInfo.dimensions() != dstInfo.dimensions()) {
return false;
}
const int width = srcInfo.width();
const int height = srcInfo.height();
// Handle fancy alpha swizzling if both are ARGB32
if (4 == srcInfo.bytesPerPixel() && 4 == dstInfo.bytesPerPixel()) {
SkDstPixelInfo dstPI;
dstPI.fColorType = dstInfo.colorType();
dstPI.fAlphaType = dstInfo.alphaType();
dstPI.fPixels = dstPixels;
dstPI.fRowBytes = dstRB;
SkSrcPixelInfo srcPI;
srcPI.fColorType = srcInfo.colorType();
srcPI.fAlphaType = srcInfo.alphaType();
srcPI.fPixels = srcPixels;
srcPI.fRowBytes = srcRB;
return srcPI.convertPixelsTo(&dstPI, width, height);
}
// If they agree on colorType and the alphaTypes are compatible, then we just memcpy.
// Note: we've already taken care of 32bit colortypes above.
if (srcInfo.colorType() == dstInfo.colorType()) {
switch (srcInfo.colorType()) {
case kRGB_565_SkColorType:
case kAlpha_8_SkColorType:
break;
case kIndex_8_SkColorType:
case kARGB_4444_SkColorType:
if (srcInfo.alphaType() != dstInfo.alphaType()) {
return false;
}
break;
default:
return false;
}
rect_memcpy(dstPixels, dstRB, srcPixels, srcRB, width * srcInfo.bytesPerPixel(), height);
return true;
}
/*
* Begin section where we try to change colorTypes along the way. Not all combinations
* are supported.
*/
// Can no longer draw directly into 4444, but we can manually whack it for a few combinations
if (kARGB_4444_SkColorType == dstInfo.colorType() &&
(kN32_SkColorType == srcInfo.colorType() || kIndex_8_SkColorType == srcInfo.colorType())) {
if (srcInfo.alphaType() == kUnpremul_SkAlphaType) {
// Our method for converting to 4444 assumes premultiplied.
return false;
}
const SkPMColor* table = NULL;
if (kIndex_8_SkColorType == srcInfo.colorType()) {
if (NULL == ctable) {
return false;
}
table = ctable->lockColors();
}
for (int y = 0; y < height; ++y) {
DITHER_4444_SCAN(y);
SkPMColor16* SK_RESTRICT dstRow = (SkPMColor16*)dstPixels;
if (table) {
const uint8_t* SK_RESTRICT srcRow = (const uint8_t*)srcPixels;
for (int x = 0; x < width; ++x) {
dstRow[x] = SkDitherARGB32To4444(table[srcRow[x]], DITHER_VALUE(x));
}
} else {
const SkPMColor* SK_RESTRICT srcRow = (const SkPMColor*)srcPixels;
for (int x = 0; x < width; ++x) {
dstRow[x] = SkDitherARGB32To4444(srcRow[x], DITHER_VALUE(x));
}
}
dstPixels = (char*)dstPixels + dstRB;
srcPixels = (const char*)srcPixels + srcRB;
}
if (table) {
ctable->unlockColors();
}
return true;
}
if (dstInfo.alphaType() == kUnpremul_SkAlphaType) {
// We do not support drawing to unpremultiplied bitmaps.
return false;
}
// Final fall-back, draw with a canvas
//
// Always clear the dest in case one of the blitters accesses it
// TODO: switch the allocation of tmpDst to call sk_calloc_throw
{
SkBitmap bm;
if (!bm.installPixels(srcInfo, const_cast<void*>(srcPixels), srcRB, ctable, NULL, NULL)) {
return false;
}
SkAutoTUnref<SkCanvas> canvas(SkCanvas::NewRasterDirect(dstInfo, dstPixels, dstRB));
if (NULL == canvas.get()) {
return false;
}
SkPaint paint;
paint.setDither(true);
canvas->clear(0);
canvas->drawBitmap(bm, 0, 0, &paint);
return true;
}
}

View File

@ -14,10 +14,6 @@ struct SkPixelInfo {
SkColorType fColorType;
SkAlphaType fAlphaType;
size_t fRowBytes;
static bool CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes,
const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRowBytes,
SkColorTable* srcCTable = NULL);
};
struct SkDstPixelInfo : SkPixelInfo {

View File

@ -185,7 +185,7 @@ static void writeCoordPixels(SkBitmap& bm, const Coordinates& coords) {
static const Pair gPairs[] = {
{ kUnknown_SkColorType, "000000" },
{ kAlpha_8_SkColorType, "010101" },
{ kIndex_8_SkColorType, "011111" },
{ kIndex_8_SkColorType, "011101" },
{ kRGB_565_SkColorType, "010101" },
{ kARGB_4444_SkColorType, "010111" },
{ kN32_SkColorType, "010111" },
@ -542,96 +542,3 @@ DEF_TEST(BitmapCopy, reporter) {
} // for (size_t copyCase ...
}
}
#include "SkColorPriv.h"
#include "SkUtils.h"
/**
* Construct 4x4 pixels where we can look at a color and determine where it should be in the grid.
* alpha = 0xFF, blue = 0x80, red = x, green = y
*/
static void fill_4x4_pixels(SkPMColor colors[16]) {
for (int y = 0; y < 4; ++y) {
for (int x = 0; x < 4; ++x) {
colors[y*4+x] = SkPackARGB32(0xFF, x, y, 0x80);
}
}
}
static bool check_4x4_pixel(SkPMColor color, unsigned x, unsigned y) {
SkASSERT(x < 4 && y < 4);
return 0xFF == SkGetPackedA32(color) &&
x == SkGetPackedR32(color) &&
y == SkGetPackedG32(color) &&
0x80 == SkGetPackedB32(color);
}
/**
* Fill with all zeros, which will never match any value from fill_4x4_pixels
*/
static void clear_4x4_pixels(SkPMColor colors[16]) {
sk_memset32(colors, 0, 16);
}
// Much of readPixels is exercised by copyTo testing, since readPixels is the backend for that
// method. Here we explicitly test subset copies.
//
DEF_TEST(BitmapReadPixels, reporter) {
const int W = 4;
const int H = 4;
const size_t rowBytes = W * sizeof(SkPMColor);
const SkImageInfo srcInfo = SkImageInfo::MakeN32Premul(W, H);
SkPMColor srcPixels[16];
fill_4x4_pixels(srcPixels);
SkBitmap srcBM;
srcBM.installPixels(srcInfo, srcPixels, rowBytes);
SkImageInfo dstInfo = SkImageInfo::MakeN32Premul(W, H);
SkPMColor dstPixels[16];
const struct {
bool fExpectedSuccess;
SkIPoint fRequestedSrcLoc;
SkISize fRequestedDstSize;
// If fExpectedSuccess, check these, otherwise ignore
SkIPoint fExpectedDstLoc;
SkIRect fExpectedSrcR;
} gRec[] = {
{ true, { 0, 0 }, { 4, 4 }, { 0, 0 }, { 0, 0, 4, 4 } },
{ true, { 1, 1 }, { 2, 2 }, { 0, 0 }, { 1, 1, 3, 3 } },
{ true, { 2, 2 }, { 4, 4 }, { 0, 0 }, { 2, 2, 4, 4 } },
{ true, {-1,-1 }, { 2, 2 }, { 1, 1 }, { 0, 0, 1, 1 } },
{ false, {-1,-1 }, { 1, 1 }, { 0, 0 }, { 0, 0, 0, 0 } },
};
for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) {
clear_4x4_pixels(dstPixels);
dstInfo.fWidth = gRec[i].fRequestedDstSize.width();
dstInfo.fHeight = gRec[i].fRequestedDstSize.height();
bool success = srcBM.readPixels(dstInfo, dstPixels, rowBytes,
gRec[i].fRequestedSrcLoc.x(), gRec[i].fRequestedSrcLoc.y());
REPORTER_ASSERT(reporter, gRec[i].fExpectedSuccess == success);
if (success) {
const SkIRect srcR = gRec[i].fExpectedSrcR;
const int dstX = gRec[i].fExpectedDstLoc.x();
const int dstY = gRec[i].fExpectedDstLoc.y();
// Walk the dst pixels, and check if we got what we expected
for (int y = 0; y < H; ++y) {
for (int x = 0; x < W; ++x) {
SkPMColor dstC = dstPixels[y*4+x];
// get into src coordinates
int sx = x - dstX + srcR.x();
int sy = y - dstY + srcR.y();
if (srcR.contains(sx, sy)) {
REPORTER_ASSERT(reporter, check_4x4_pixel(dstC, sx, sy));
} else {
REPORTER_ASSERT(reporter, 0 == dstC);
}
}
}
}
}
}