Fix some extract subset bugs.
In SkBitmap::extractSubset, perform a deepCopy, if the pixelRef supports it.
Fixes a bug in the 'extractbitmap' gm, which attempts to draw a subset of a texture backed bitmap (if the canvas is really an SkGpuCanvas).
Also fix some bugs that happen when there is a pixel offset. These fixes get bypassed by the deepCopy, but a user can still set a pixel offset manually.
When copying GPU backed bitmap with a pixel offset, copy the offset.
If the new config is the same as the old, copy fRowBytes as well.
Add a function to SkBitmap.cpp (getUpperLeftFromOffset) to find the x,y coordinate to use when copying to a new config.
Fix a bug where readPixels copied to the correct desired config and we were setting the generation ID to match even though the desired config was not the same as the original config (caught by my new tests!).
Add some tests to verify the correct behavior.
Review URL: https://codereview.appspot.com/6839043
git-svn-id: http://skia.googlecode.com/svn/trunk@6710 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 9d51f9b..5554f46 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -828,10 +828,18 @@
#define SUB_OFFSET_FAILURE ((size_t)-1)
-static size_t getSubOffset(const SkBitmap& bm, int x, int y) {
- SkASSERT((unsigned)x < (unsigned)bm.width());
- SkASSERT((unsigned)y < (unsigned)bm.height());
+// Declare these non-static so they can be tested by GpuBitmapCopyTest.
+size_t getSubOffset(const SkBitmap& bm, int x, int y);
+bool getUpperLeftFromOffset(const SkBitmap& bm, int* x, int* y);
+/**
+ * Based on the Config and rowBytes() of bm, return the offset into an SkPixelRef of the pixel at
+ * (x, y).
+ * Note that the SkPixelRef does not need to be set yet. deepCopyTo takes advantage of this fact.
+ * Also note that (x, y) may be outside the range of (0 - width(), 0 - height()), so long as it is
+ * within the bounds of the SkPixelRef being used.
+ */
+size_t getSubOffset(const SkBitmap& bm, int x, int y) {
switch (bm.getConfig()) {
case SkBitmap::kA8_Config:
case SkBitmap:: kIndex8_Config:
@@ -855,6 +863,49 @@
return y * bm.rowBytes() + x;
}
+/**
+ * Using the pixelRefOffset(), rowBytes(), and Config of bm, determine the (x, y) coordinate of the
+ * upper left corner of bm relative to its SkPixelRef.
+ * x and y must be non-NULL.
+ */
+bool getUpperLeftFromOffset(const SkBitmap& bm, int* x, int* y) {
+ SkASSERT(x != NULL && y != NULL);
+ const size_t offset = bm.pixelRefOffset();
+ if (0 == offset) {
+ *x = *y = 0;
+ return true;
+ }
+ // Use integer division to find the correct y position.
+ *y = offset / bm.rowBytes();
+ // The remainder will be the x position, after we reverse getSubOffset.
+ *x = offset % bm.rowBytes();
+ switch (bm.getConfig()) {
+ case SkBitmap::kA8_Config:
+ // Fall through.
+ case SkBitmap::kIndex8_Config:
+ // x is unmodified
+ break;
+
+ case SkBitmap::kRGB_565_Config:
+ // Fall through.
+ case SkBitmap::kARGB_4444_Config:
+ *x >>= 1;
+ break;
+
+ case SkBitmap::kARGB_8888_Config:
+ *x >>= 2;
+ break;
+
+ case SkBitmap::kNo_Config:
+ // Fall through.
+ case SkBitmap::kA1_Config:
+ // Fall through.
+ default:
+ return false;
+ }
+ return true;
+}
+
bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
SkDEBUGCODE(this->validate();)
@@ -868,6 +919,21 @@
return false; // r is empty (i.e. no intersection)
}
+ if (fPixelRef->getTexture() != NULL) {
+ // Do a deep copy
+ SkPixelRef* pixelRef = fPixelRef->deepCopy(this->config(), &subset);
+ if (pixelRef != NULL) {
+ SkBitmap dst;
+ dst.setConfig(this->config(), subset.width(), subset.height());
+ dst.setIsVolatile(this->isVolatile());
+ dst.setIsOpaque(this->isOpaque());
+ dst.setPixelRef(pixelRef)->unref();
+ SkDEBUGCODE(dst.validate());
+ result->swap(dst);
+ return true;
+ }
+ }
+
if (kRLE_Index8_Config == fConfig) {
SkAutoLockPixels alp(*this);
// don't call readyToDraw(), since we can operate w/o a colortable
@@ -896,6 +962,11 @@
return true;
}
+ // If the upper left of the rectangle was outside the bounds of this SkBitmap, we should have
+ // exited above.
+ SkASSERT(static_cast<unsigned>(r.fLeft) < static_cast<unsigned>(this->width()));
+ SkASSERT(static_cast<unsigned>(r.fTop) < static_cast<unsigned>(this->height()));
+
size_t offset = getSubOffset(*this, r.fLeft, r.fTop);
if (SUB_OFFSET_FAILURE == offset) {
return false; // config not supported
@@ -961,21 +1032,28 @@
SkBitmap tmpSrc;
const SkBitmap* src = this;
- if (fPixelRef && fPixelRef->readPixels(&tmpSrc)) {
- SkASSERT(tmpSrc.width() == this->width());
- SkASSERT(tmpSrc.height() == this->height());
+ if (fPixelRef) {
+ SkIRect subset;
+ if (getUpperLeftFromOffset(*this, &subset.fLeft, &subset.fTop)) {
+ subset.fRight = subset.fLeft + fWidth;
+ subset.fBottom = subset.fTop + fHeight;
+ if (fPixelRef->readPixels(&tmpSrc, &subset)) {
+ SkASSERT(tmpSrc.width() == this->width());
+ SkASSERT(tmpSrc.height() == this->height());
- // did we get lucky and we can just return tmpSrc?
- if (tmpSrc.config() == dstConfig && NULL == alloc) {
- dst->swap(tmpSrc);
- if (dst->pixelRef()) {
- dst->pixelRef()->fGenerationID = fPixelRef->getGenerationID();
+ // did we get lucky and we can just return tmpSrc?
+ if (tmpSrc.config() == dstConfig && NULL == alloc) {
+ dst->swap(tmpSrc);
+ if (dst->pixelRef() && this->config() == dstConfig) {
+ dst->pixelRef()->fGenerationID = fPixelRef->getGenerationID();
+ }
+ return true;
+ }
+
+ // fall through to the raster case
+ src = &tmpSrc;
}
- return true;
}
-
- // fall through to the raster case
- src = &tmpSrc;
}
// we lock this now, since we may need its colortable
@@ -1049,11 +1127,34 @@
if (fPixelRef) {
SkPixelRef* pixelRef = fPixelRef->deepCopy(dstConfig);
if (pixelRef) {
+ uint32_t rowBytes;
if (dstConfig == fConfig) {
pixelRef->fGenerationID = fPixelRef->getGenerationID();
+ // Use the same rowBytes as the original.
+ rowBytes = fRowBytes;
+ } else {
+ // With the new config, an appropriate fRowBytes will be computed by setConfig.
+ rowBytes = 0;
}
- dst->setConfig(dstConfig, fWidth, fHeight);
- dst->setPixelRef(pixelRef)->unref();
+ dst->setConfig(dstConfig, fWidth, fHeight, rowBytes);
+
+ size_t pixelRefOffset;
+ if (0 == fPixelRefOffset || dstConfig == fConfig) {
+ // Use the same offset as the original.
+ pixelRefOffset = fPixelRefOffset;
+ } else {
+ // Find the correct offset in the new config. This needs to be done after calling
+ // setConfig so dst's fConfig and fRowBytes have been set properly.
+ int x, y;
+ if (!getUpperLeftFromOffset(*this, &x, &y)) {
+ return false;
+ }
+ pixelRefOffset = getSubOffset(*dst, x, y);
+ if (SUB_OFFSET_FAILURE == pixelRefOffset) {
+ return false;
+ }
+ }
+ dst->setPixelRef(pixelRef, pixelRefOffset)->unref();
return true;
}
}
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 9c209b3..d96a16e 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -1437,7 +1437,7 @@
fGpu->resolveRenderTarget(target);
}
-void GrContext::copyTexture(GrTexture* src, GrRenderTarget* dst) {
+void GrContext::copyTexture(GrTexture* src, GrRenderTarget* dst, const SkIPoint* topLeft) {
if (NULL == src || NULL == dst) {
return;
}
@@ -1454,11 +1454,18 @@
drawState->setRenderTarget(dst);
SkMatrix sampleM;
sampleM.setIDiv(src->width(), src->height());
+ SkIRect srcRect = SkIRect::MakeWH(dst->width(), dst->height());
+ if (NULL != topLeft) {
+ srcRect.offset(*topLeft);
+ }
+ SkIRect srcBounds = SkIRect::MakeWH(src->width(), src->height());
+ if (!srcRect.intersect(srcBounds)) {
+ return;
+ }
+ sampleM.preTranslate(SkIntToScalar(srcRect.fLeft), SkIntToScalar(srcRect.fTop));
drawState->createTextureEffect(0, src, sampleM);
- SkRect rect = SkRect::MakeXYWH(0, 0,
- SK_Scalar1 * src->width(),
- SK_Scalar1 * src->height());
- fGpu->drawSimpleRect(rect, NULL);
+ SkRect dstR = SkRect::MakeWH(SkIntToScalar(srcRect.width()), SkIntToScalar(srcRect.height()));
+ fGpu->drawSimpleRect(dstR, NULL);
}
void GrContext::writeRenderTargetPixels(GrRenderTarget* target,
diff --git a/src/gpu/SkGrPixelRef.cpp b/src/gpu/SkGrPixelRef.cpp
index e770ef0..255437e 100644
--- a/src/gpu/SkGrPixelRef.cpp
+++ b/src/gpu/SkGrPixelRef.cpp
@@ -48,8 +48,8 @@
///////////////////////////////////////////////////////////////////////////////
-static SkGrPixelRef* copyToTexturePixelRef(GrTexture* texture,
- SkBitmap::Config dstConfig) {
+static SkGrPixelRef* copyToTexturePixelRef(GrTexture* texture, SkBitmap::Config dstConfig,
+ const SkIRect* subset) {
if (NULL == texture) {
return NULL;
}
@@ -59,8 +59,20 @@
}
GrTextureDesc desc;
- desc.fWidth = texture->width();
- desc.fHeight = texture->height();
+ SkIPoint pointStorage;
+ SkIPoint* topLeft;
+ if (subset != NULL) {
+ SkASSERT(SkIRect::MakeWH(texture->width(), texture->height()).contains(*subset));
+ // Create a new texture that is the size of subset.
+ desc.fWidth = subset->width();
+ desc.fHeight = subset->height();
+ pointStorage.set(subset->x(), subset->y());
+ topLeft = &pointStorage;
+ } else {
+ desc.fWidth = texture->width();
+ desc.fHeight = texture->height();
+ topLeft = NULL;
+ }
desc.fFlags = kRenderTarget_GrTextureFlagBit | kNoStencil_GrTextureFlagBit;
desc.fConfig = SkBitmapConfig2GrPixelConfig(dstConfig);
@@ -69,7 +81,7 @@
return NULL;
}
- context->copyTexture(texture, dst->asRenderTarget());
+ context->copyTexture(texture, dst->asRenderTarget(), topLeft);
// TODO: figure out if this is responsible for Chrome canvas errors
#if 0
@@ -123,7 +135,7 @@
return NULL;
}
-SkPixelRef* SkGrPixelRef::deepCopy(SkBitmap::Config dstConfig) {
+SkPixelRef* SkGrPixelRef::deepCopy(SkBitmap::Config dstConfig, const SkIRect* subset) {
if (NULL == fSurface) {
return NULL;
}
@@ -134,7 +146,7 @@
// a GrTexture owned elsewhere (e.g., SkGpuDevice), and cannot live
// independently of that texture. Texture-backed pixel refs, on the other
// hand, own their GrTextures, and are thus self-contained.
- return copyToTexturePixelRef(fSurface->asTexture(), dstConfig);
+ return copyToTexturePixelRef(fSurface->asTexture(), dstConfig, subset);
}
bool SkGrPixelRef::onReadPixels(SkBitmap* dst, const SkIRect* subset) {