Changes the SkCanvas::readPixels API. Allows caller to read into prealloced bitmap pixels. Changes how clipping to device bounds is handled.

Review URL: http://codereview.appspot.com/5307077/



git-svn-id: http://skia.googlecode.com/svn/trunk@2584 2bbb7eff-a529-9590-31e7-b0007b416f81

20 files changed, 521 insertions, 109 deletions

diff --git a/gm/gmmain.cpp b/gm/gmmain.cpp
index 5269b3405e..f2ab7033fc 100644
--- a/gm/gmmain.cpp
+++ b/gm/gmmain.cpp
@@ -251,7 +251,9 @@ static bool generate_image(GM* gm, const ConfigData& gRec,
         // the device is as large as the current rendertarget, so we explicitly
         // only readback the amount we expect (in size)
         // overwrite our previous allocation
-        gc.readPixels(SkIRect::MakeSize(size), bitmap);
+        bitmap->setConfig(SkBitmap::kARGB_8888_Config, size.fWidth,
+                                                       size.fHeight);
+        gc.readPixels(bitmap, 0, 0);
     }
     return true;
 }
diff --git a/gyp/tests.gyp b/gyp/tests.gyp
index d1904876f0..92aba93a03 100644
--- a/gyp/tests.gyp
+++ b/gyp/tests.gyp
@@ -47,6 +47,7 @@
         '../tests/PDFPrimitivesTest.cpp',
         '../tests/PointTest.cpp',
         '../tests/Reader32Test.cpp',
+        '../tests/ReadPixelsTest.cpp',
         '../tests/RefDictTest.cpp',
         '../tests/RegionTest.cpp',
         '../tests/Sk64Test.cpp',
diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h
index e98a294bde..ae56739c5f 100644
--- a/include/core/SkBitmap.h
+++ b/include/core/SkBitmap.h
@@ -225,12 +225,11 @@ public:
     /** Copies the bitmap's pixels to the location pointed at by dst and returns
         true if possible, returns false otherwise.
 
-        In the event that the bitmap's stride is equal to dstRowBytes, and if
-        it is greater than strictly required by the bitmap's current config
-        (this may happen if the bitmap is an extracted subset of another), then
-        this function will copy bytes past the eand of each row, excluding the
-        last row. No copies are made outside of the declared size of dst,
-        however.
+        In the case when the dstRowBytes matches the bitmap's rowBytes, the copy
+        may be made faster by copying over the dst's per-row padding (for all
+        rows but the last). By setting preserveDstPad to true the caller can
+        disable this optimization and ensure that pixels in the padding are not
+        overwritten.
 
         Always returns false for RLE formats.
 
@@ -239,8 +238,10 @@ public:
                         pixels using indicated stride.
         @param dstRowBytes  Width of each line in the buffer. If -1, uses
                             bitmap's internal stride.
+        @param preserveDstPad Must we preserve padding in the dst
     */
-    bool copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes = -1)
+    bool copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes = -1,
+                      bool preserveDstPad = false)
          const;
 
     /** Use the standard HeapAllocator to create the pixelref that manages the
diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h
index b2bdafe738..90bc1f433e 100644
--- a/include/core/SkCanvas.h
+++ b/include/core/SkCanvas.h
@@ -105,14 +105,45 @@ public:
 
     ///////////////////////////////////////////////////////////////////////////
 
+    /**

+     *  On success (returns true), copy the canvas pixels into the bitmap.

+     *  On failure, the bitmap parameter is left unchanged and false is

+     *  returned.

+     *

+     *  If the canvas is backed by a non-raster device (e.g. PDF) then

+     *  readPixels will fail.

+     *

+     *  If the bitmap has pixels already allocated, the canvas pixels will be

+     *  written there. If not, bitmap->allocPixels() will be called 

+     *  automatically. If the bitmap is backed by a texture readPixels will

+     *  fail.

+     *

+     *  The canvas' pixels are converted to the bitmap's config. The only

+     *  supported config is kARGB_8888_Config, though this may be relaxed in

+     *  future.

+     *

+     *  The actual pixels written is the intersection of the canvas' bounds, and

+     *  the rectangle formed by the bitmap's width,height and the specified x,y.

+     *  If bitmap pixels extend outside of that intersection, they will not be

+     *  modified.

+     *

+     *  Example that reads the entire canvas into a bitmap:

+     *  SkISize size = canvas->getDeviceSize();

+     *  bitmap->setConfig(SkBitmap::kARGB_8888_Config, size.fWidth,

+     *                                                 size.fHeight);

+     *  if (canvas->readPixels(bitmap, 0, 0)) {

+     *     // use the pixels

+     *  }

+     */
+    bool readPixels(SkBitmap* bitmap, int x, int y);
+
     /**
-     *  Copy the pixels from the device into bitmap. Returns true on success.
-     *  If false is returned, then the bitmap parameter is left unchanged.
-     *  The bitmap parameter is treated as output-only, and will be completely
-     *  overwritten (if the method returns true).
+     * DEPRECATED: This will be removed as soon as webkit is no longer relying
+     * on it. The bitmap is resized to the intersection of srcRect and the
+     * canvas bounds. New pixels are always allocated on success. Bitmap is
+     * unmodified on failure.
      */
     bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap);
-    bool readPixels(SkBitmap* bitmap);
 
     /**
      *  Similar to draw sprite, this method will copy the pixels in bitmap onto
diff --git a/include/core/SkDevice.h b/include/core/SkDevice.h
index 95b63892ae..5d184e478f 100644
--- a/include/core/SkDevice.h
+++ b/include/core/SkDevice.h
@@ -108,11 +108,23 @@ public:
 
     /**
      *  Copy the pixels from the device into bitmap. Returns true on success.
-     *  If false is returned, then the bitmap parameter is left unchanged.
-     *  The bitmap parameter is treated as output-only, and will be completely
-     *  overwritten (if the method returns true).
+     *  If false is returned, then the bitmap parameter is left unchanged. The
+     *  rectangle read is defined by x, y and the bitmap's width and height.
+     *
+     *  If the bitmap has pixels allocated the canvas will write directly to
+     *  into that memory (if the call succeeds).
+     *
+     *  The read is clipped to the device bounds. If bitmap pixels were
+     *  preallocated then pixels outside the clip are left unmodified. If the
+     *  call allocates bitmap pixels then pixels outside the clip will be
+     *  uninitialized.
+     *
+     *  Currently bitmap must have kARGB_8888_Config or readPixels will fail.
+     *  This will likely be relaxed in the future.
+     *
+     *  The bitmap parameter is not modified if the call fails.
      */
-    virtual bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap);
+    bool readPixels(SkBitmap* bitmap, int x, int y);
 
     /**
      *  Similar to draw sprite, this method will copy the pixels in bitmap onto
@@ -256,6 +268,17 @@ protected:
         fBitmap.setPixelRef(pr, offset);
         return pr;
     }
+    
+    /**
+     * Implements readPixels API. The caller will ensure that:
+     *  1. bitmap has pixel config kARGB_8888_Config.
+     *  2. bitmap has pixels.
+     *  3. The rectangle (x, y, x + bitmap->width(), y + bitmap->height()) is
+     *     contained in the device bounds.
+     *  4. the bitmap struct is safe to partially overwrite in case of failure
+     */
+    virtual bool onReadPixels(const SkBitmap* bitmap, int x, int y);
+
 
     /** Called when this device is installed into a Canvas. Balanaced by a call
         to unlockPixels() when the device is removed from a Canvas.
diff --git a/include/device/xps/SkXPSDevice.h b/include/device/xps/SkXPSDevice.h
index 1fb9220d15..ed61ced8d0 100644
--- a/include/device/xps/SkXPSDevice.h
+++ b/include/device/xps/SkXPSDevice.h
@@ -71,11 +71,6 @@ public:
         return kVector_Capability;
     }
 
-    virtual bool readPixels(const SkIRect& srcRect,
-                            SkBitmap* bitmap) SK_OVERRIDE {
-        return false;
-    }
-
 protected:
     virtual void clear(SkColor color) SK_OVERRIDE;
 
@@ -146,6 +141,12 @@ protected:
         int x, int y,
         const SkPaint& paint) SK_OVERRIDE;
 
+    virtual bool onReadPixels(const SkBitmap* bitmap,
+                              int x,
+                              int y) SK_OVERRIDE {
+        return false;
+    }
+
 private:
     class TypefaceUse : ::SkNoncopyable {
     public:
diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h
index f980702dd6..d61b8bb5a0 100644
--- a/include/gpu/GrContext.h
+++ b/include/gpu/GrContext.h
@@ -432,6 +432,8 @@ public:
      * @param height        height of rectangle to read in pixels.
      * @param config        the pixel config of the destination buffer
      * @param buffer        memory to read the rectangle into.
+     * @param rowBytes      number of bytes bewtween consecueive rows. Zero
+     *                      means rows are tightly packed.
      *
      * @return true if the read succeeded, false if not. The read can fail
      *              because of a unsupported pixel config or because no render
@@ -439,7 +441,8 @@ public:
      */
     bool readRenderTargetPixels(GrRenderTarget* target,
                                 int left, int top, int width, int height,
-                                GrPixelConfig config, void* buffer);
+                                GrPixelConfig config, void* buffer, 
+                                size_t rowBytes = 0);
 
     /**
      * Reads a rectangle of pixels from a texture.
diff --git a/include/gpu/GrGLDefines.h b/include/gpu/GrGLDefines.h
index 7a3d6767fa..5d11b9f27e 100644
--- a/include/gpu/GrGLDefines.h
+++ b/include/gpu/GrGLDefines.h
@@ -348,7 +348,9 @@
 #define GR_GL_EXTENSIONS                     0x1F03
 
 /* Pixel Mode / Transfer */
-#define GR_GL_UNPACK_ROW_LENGTH            0x0CF2
+#define GR_GL_UNPACK_ROW_LENGTH              0x0CF2
+#define GR_GL_PACK_ROW_LENGTH                0x0D02
+
 
 /* TextureMagFilter */
 #define GR_GL_NEAREST                        0x2600
diff --git a/include/gpu/SkGpuDevice.h b/include/gpu/SkGpuDevice.h
index f5613a7fd8..047fd0740c 100644
--- a/include/gpu/SkGpuDevice.h
+++ b/include/gpu/SkGpuDevice.h
@@ -68,7 +68,6 @@ public:
     // overrides from SkDevice
 
     virtual void clear(SkColor color);
-    virtual bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap);
     virtual void writePixels(const SkBitmap& bitmap, int x, int y);
 
     virtual void setMatrixClip(const SkMatrix& matrix, const SkRegion& clip,
@@ -140,6 +139,11 @@ protected:
         TexCache        fTex;
     };
     friend class SkAutoTexCache;
+    
+    // overrides from SkDevice
+    virtual bool onReadPixels(const SkBitmap* bitmap,
+                              int x, int y) SK_OVERRIDE;
+
 
 private:
     GrContext*      fContext;
diff --git a/include/pdf/SkPDFDevice.h b/include/pdf/SkPDFDevice.h
index 9e985e7702..b25e39a7d0 100644
--- a/include/pdf/SkPDFDevice.h
+++ b/include/pdf/SkPDFDevice.h
@@ -66,10 +66,6 @@ public:
 
     virtual void clear(SkColor color);
 
-    virtual bool readPixels(const SkIRect& srcRect, SkBitmap* bitmap) {
-        return false;
-    }
-
     /** These are called inside the per-device-layer loop for each draw call.
      When these are called, we have already applied any saveLayer operations,
      and are handling any looping from the paint, and any effects from the
@@ -160,6 +156,13 @@ public:
     const SkPDFGlyphSetMap& getFontGlyphUsage() const {
         return *(fFontGlyphUsage.get());
     }
+    
+protected:
+    virtual bool onReadPixels(const SkBitmap* bitmap,
+                              int x, int y) SK_OVERRIDE {
+        return false;
+    }
+
 
 private:
     // TODO(vandebo): push most of SkPDFDevice's state into a core object in
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 9683654db6..760bab7d8a 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -456,8 +456,8 @@ Sk64 SkBitmap::getSafeSize64() const {
     return ComputeSafeSize64(getConfig(), fWidth, fHeight, fRowBytes);
 }
 
-bool SkBitmap::copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes)
-     const {
+bool SkBitmap::copyPixelsTo(void* const dst, size_t dstSize, 
+                            int dstRowBytes, bool preserveDstPad) const {
 
     if (dstRowBytes == -1)
         dstRowBytes = fRowBytes;
@@ -468,7 +468,7 @@ bool SkBitmap::copyPixelsTo(void* const dst, size_t dstSize, int dstRowBytes)
         dst == NULL || (getPixels() == NULL && pixelRef() == NULL))
         return false;
 
-    if (static_cast<uint32_t>(dstRowBytes) == fRowBytes) {
+    if (!preserveDstPad && static_cast<uint32_t>(dstRowBytes) == fRowBytes) {
         size_t safeSize = getSafeSize();
         if (safeSize > dstSize || safeSize == 0)
             return false;
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 3ea9a9c36b..da7aeb971a 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -550,24 +550,32 @@ SkDevice* SkCanvas::setBitmapDevice(const SkBitmap& bitmap) {
     return device;
 }
 
-bool SkCanvas::readPixels(const SkIRect& srcRect, SkBitmap* bitmap) {
+bool SkCanvas::readPixels(SkBitmap* bitmap, int x, int y) {
     SkDevice* device = this->getDevice();
     if (!device) {
         return false;
     }
-    return device->readPixels(srcRect, bitmap);
+    return device->readPixels(bitmap, x, y);
 }
 
-//////////////////////////////////////////////////////////////////////////////
-
-bool SkCanvas::readPixels(SkBitmap* bitmap) {
+bool SkCanvas::readPixels(const SkIRect& srcRect, SkBitmap* bitmap) {
     SkDevice* device = this->getDevice();
-    if (!device) {
+    

+    SkIRect bounds;

+    bounds.set(0, 0, device->width(), device->height());
+    if (!bounds.intersect(srcRect)) {

+        return false;

+    }
+
+    SkBitmap tmp;
+    tmp.setConfig(SkBitmap::kARGB_8888_Config, bounds.width(),
+                                               bounds.height());
+    if (this->readPixels(&tmp, bounds.fLeft, bounds.fTop)) {
+        bitmap->swap(tmp);
+        return true;
+    } else {
         return false;
     }
-    SkIRect bounds;
-    bounds.set(0, 0, device->width(), device->height());
-    return this->readPixels(bounds, bitmap);
 }
 
 void SkCanvas::writePixels(const SkBitmap& bitmap, int x, int y) {
diff --git a/src/core/SkDevice.cpp b/src/core/SkDevice.cpp
index 18087ae04d..f5523bd9db 100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -102,27 +102,70 @@ void SkDevice::setMatrixClip(const SkMatrix& matrix, const SkRegion& region,
 
 ///////////////////////////////////////////////////////////////////////////////
 
-bool SkDevice::readPixels(const SkIRect& srcRect, SkBitmap* bitmap) {
-    const SkBitmap& src = this->accessBitmap(false);
-
-    SkIRect bounds;
-    bounds.set(0, 0, src.width(), src.height());
-    if (!bounds.intersect(srcRect)) {
+bool SkDevice::readPixels(SkBitmap* bitmap, int x, int y) {
+    if (SkBitmap::kARGB_8888_Config != bitmap->config() ||
+        NULL != bitmap->getTexture()) {
         return false;
     }
 
-    SkBitmap subset;
-    if (!src.extractSubset(&subset, bounds)) {
+    const SkBitmap& src = this->accessBitmap(false);
+
+    SkIRect srcRect = SkIRect::MakeXYWH(x, y, bitmap->width(),
+                                              bitmap->height());
+    SkIRect devbounds = SkIRect::MakeWH(src.width(), src.height());
+    if (!srcRect.intersect(devbounds)) {
         return false;
     }
 
     SkBitmap tmp;
-    if (!subset.copyTo(&tmp, SkBitmap::kARGB_8888_Config)) {
-        return false;
+    SkBitmap* bmp;
+    if (bitmap->isNull()) {
+        tmp.setConfig(SkBitmap::kARGB_8888_Config, bitmap->width(),
+                                                   bitmap->height());
+        if (!tmp.allocPixels()) {
+            return false;
+        }
+        bmp = &tmp;
+    } else {
+        bmp = bitmap;
+    }
+
+    SkIRect subrect = srcRect;
+    subrect.offset(-x, -y);
+    SkBitmap bmpSubset;
+    bmp->extractSubset(&bmpSubset, subrect);
+
+    bool result = this->onReadPixels(&bmpSubset, srcRect.fLeft, srcRect.fTop);
+    if (result && bmp == &tmp) {
+        tmp.swap(*bitmap);
     }
+    return result;
+}
+
+bool SkDevice::onReadPixels(const SkBitmap* bitmap, int x, int y) {
+    SkASSERT(SkBitmap::kARGB_8888_Config == bitmap->config());
+    SkASSERT(!bitmap->isNull());
+    SkASSERT(SkIRect::MakeWH(this->width(), this->height()).contains(SkIRect::MakeXYWH(x, y, bitmap->width(), bitmap->height())));
+
+    SkIRect srcRect = SkIRect::MakeXYWH(x, y, bitmap->width(),
+                                              bitmap->height());
+    const SkBitmap& src = this->accessBitmap(false);
 
-    tmp.swap(*bitmap);
-    return true;
+    SkBitmap subset;
+    if (!src.extractSubset(&subset, srcRect)) {
+        return false;
+    }
+    if (SkBitmap::kARGB_8888_Config != subset.config()) {
+        // It'd be preferable to do this directly to bitmap.
+        // We'd need a SkBitmap::copyPixelsTo that takes a config
+        // or make copyTo lazily allocate.
+        subset.copyTo(&subset, SkBitmap::kARGB_8888_Config); 
+    }
+    SkAutoLockPixels alp(*bitmap);
+    return subset.copyPixelsTo(bitmap->getPixels(),
+                               bitmap->getSize(),
+                               bitmap->rowBytes(),
+                               true);
 }
 
 void SkDevice::writePixels(const SkBitmap& bitmap, int x, int y) {
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 7028c91147..3fc5d7d773 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -1637,15 +1637,16 @@ bool GrContext::readTexturePixels(GrTexture* texture,
     if (NULL != target) {
         return fGpu->readPixels(target,
                                 left, top, width, height, 
-                                config, buffer);
+                                config, buffer, 0);
     } else {
         return false;
     }
 }
 
 bool GrContext::readRenderTargetPixels(GrRenderTarget* target,
-                                      int left, int top, int width, int height,
-                                      GrPixelConfig config, void* buffer) {
+                                       int left, int top, int width, int height,
+                                       GrPixelConfig config, void* buffer,
+                                       size_t rowBytes) {
     SK_TRACE_EVENT0("GrContext::readRenderTargetPixels");
     uint32_t flushFlags = 0;
     if (NULL == target) { 
@@ -1655,7 +1656,7 @@ bool GrContext::readRenderTargetPixels(GrRenderTarget* target,
     this->flush(flushFlags);
     return fGpu->readPixels(target,
                             left, top, width, height, 
-                            config, buffer);
+                            config, buffer, rowBytes);
 }
 
 void GrContext::writePixels(int left, int top, int width, int height,
diff --git a/src/gpu/GrGpu.cpp b/src/gpu/GrGpu.cpp
index f0808d394f..1fc8d47339 100644
--- a/src/gpu/GrGpu.cpp
+++ b/src/gpu/GrGpu.cpp
@@ -221,10 +221,12 @@ void GrGpu::forceRenderTargetFlush() {
 
 bool GrGpu::readPixels(GrRenderTarget* target,
                        int left, int top, int width, int height,
-                       GrPixelConfig config, void* buffer) {
+                       GrPixelConfig config, void* buffer,
+                       size_t rowBytes) {
 
     this->handleDirtyContext();
-    return this->onReadPixels(target, left, top, width, height, config, buffer);
+    return this->onReadPixels(target, left, top, width, height,
+                              config, buffer, rowBytes);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h
index 9107554cd6..cf29ed7555 100644
--- a/src/gpu/GrGpu.h
+++ b/src/gpu/GrGpu.h
@@ -180,6 +180,8 @@ public:
      * @param height        height of rectangle to read in pixels.
      * @param config        the pixel config of the destination buffer
      * @param buffer        memory to read the rectangle into.
+     * @param rowBytes      the number of bytes between consecutive rows. Zero
+     *                      means rows are tightly packed.
      *
      * @return true if the read succeeded, false if not. The read can fail
      *              because of a unsupported pixel config or because no render
@@ -187,7 +189,7 @@ public:
      */
     bool readPixels(GrRenderTarget* renderTarget,
                     int left, int top, int width, int height,
-                    GrPixelConfig config, void* buffer);
+                    GrPixelConfig config, void* buffer, size_t rowBytes);
 
     const GrGpuStats& getStats() const;
     void resetStats();
@@ -321,7 +323,7 @@ protected:
     // overridden by API-specific derived class to perform the read pixels.
     virtual bool onReadPixels(GrRenderTarget* target,
                               int left, int top, int width, int height,
-                              GrPixelConfig, void* buffer) = 0;
+                              GrPixelConfig, void* buffer, size_t rowBytes) = 0;
 
     // called to program the vertex data, indexCount will be 0 if drawing non-
     // indexed geometry. The subclass may adjust the startVertex and/or
diff --git a/src/gpu/GrGpuGL.cpp b/src/gpu/GrGpuGL.cpp
index 78655929f3..dc4d78ac43 100644
--- a/src/gpu/GrGpuGL.cpp
+++ b/src/gpu/GrGpuGL.cpp
@@ -1384,14 +1384,18 @@ void GrGpuGL::onForceRenderTargetFlush() {
 }
 
 bool GrGpuGL::onReadPixels(GrRenderTarget* target,
-                           int left, int top, int width, int height,
-                           GrPixelConfig config, void* buffer) {
+                           int left, int top,
+                           int width, int height,
+                           GrPixelConfig config, 
+                           void* buffer, size_t rowBytes) {
     GrGLenum internalFormat;  // we don't use this for glReadPixels
     GrGLenum format;
     GrGLenum type;
     if (!this->canBeTexture(config, &internalFormat, &format, &type)) {
         return false;
-    }    
+    }
+    
+    // resolve the render target if necessary
     GrGLRenderTarget* tgt = static_cast<GrGLRenderTarget*>(target);
     GrAutoTPtrValueRestore<GrRenderTarget*> autoTargetRestore;
     switch (tgt->getResolveType()) {
@@ -1417,26 +1421,62 @@ bool GrGpuGL::onReadPixels(GrRenderTarget* target,
     // the read rect is viewport-relative
     GrGLIRect readRect;
     readRect.setRelativeTo(glvp, left, top, width, height);
+    
+    size_t tightRowBytes = GrBytesPerPixel(config) * width;
+    if (0 == rowBytes) {
+        rowBytes = tightRowBytes;
+    }
+    size_t readDstRowBytes = tightRowBytes;
+    void* readDst = buffer;
+    
+    // determine if GL can read using the passed rowBytes or if we need
+    // a scratch buffer.
+    SkAutoSMalloc<32 * sizeof(GrColor)> scratch;
+    if (rowBytes != tightRowBytes) {
+        if (kDesktop_GrGLBinding == this->glBinding()) {
+            GrAssert(!(rowBytes % sizeof(GrColor)));
+            GL_CALL(PixelStorei(GR_GL_PACK_ROW_LENGTH, rowBytes / sizeof(GrColor)));
+            readDstRowBytes = rowBytes;
+        } else {
+            scratch.reset(tightRowBytes * height);
+            readDst = scratch.get();
+        }
+    }
     GL_CALL(ReadPixels(readRect.fLeft, readRect.fBottom,
                        readRect.fWidth, readRect.fHeight,
-                       format, type, buffer));
+                       format, type, readDst));
+    if (readDstRowBytes != tightRowBytes) {
+        GL_CALL(PixelStorei(GR_GL_PACK_ROW_LENGTH, 0));
+    }
 
     // now reverse the order of the rows, since GL's are bottom-to-top, but our
-    // API presents top-to-bottom
-    {
-        size_t stride = width * GrBytesPerPixel(config);
-        SkAutoMalloc rowStorage(stride);
-        void* tmp = rowStorage.get();
-
+    // API presents top-to-bottom. We must preserve the padding contents. Note
+    // that the above readPixels did not overwrite the padding.
+    if (readDst == buffer) {
+        GrAssert(rowBytes == readDstRowBytes);
+        scratch.reset(tightRowBytes);
+        void* tmpRow = scratch.get();
+        // flip y in-place by rows
         const int halfY = height >> 1;
         char* top = reinterpret_cast<char*>(buffer);
-        char* bottom = top + (height - 1) * stride;
+        char* bottom = top + (height - 1) * rowBytes;
         for (int y = 0; y < halfY; y++) {
-            memcpy(tmp, top, stride);
-            memcpy(top, bottom, stride);
-            memcpy(bottom, tmp, stride);
-            top += stride;
-            bottom -= stride;
+            memcpy(tmpRow, top, tightRowBytes);
+            memcpy(top, bottom, tightRowBytes);
+            memcpy(bottom, tmpRow, tightRowBytes);
+            top += rowBytes;
+            bottom -= rowBytes;
+        }
+    } else {
+        GrAssert(readDst != buffer);
+        // copy from readDst to buffer while flipping y
+        const int halfY = height >> 1;
+        const char* src = reinterpret_cast<const char*>(readDst);
+        char* dst = reinterpret_cast<char*>(buffer) + (height-1) * rowBytes;
+        for (int y = 0; y < height; y++) {
+            memcpy(dst, src, tightRowBytes);
+            src += readDstRowBytes;
+            dst -= rowBytes;
         }
     }
     return true;
diff --git a/src/gpu/GrGpuGL.h b/src/gpu/GrGpuGL.h
index 2d246e8479..51eb4aed02 100644
--- a/src/gpu/GrGpuGL.h
+++ b/src/gpu/GrGpuGL.h
@@ -90,8 +90,10 @@ protected:
     virtual void onForceRenderTargetFlush();
 
     virtual bool onReadPixels(GrRenderTarget* target,
-                              int left, int top, int width, int height,
-                              GrPixelConfig, void* buffer);
+                              int left, int top, 
+                              int width, int height,
+                              GrPixelConfig, 
+                              void* buffer, size_t rowBytes) SK_OVERRIDE;
 
     virtual void onGpuDrawIndexed(GrPrimitiveType type,
                                   uint32_t startVertex,
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index d75838350e..790cf6d09e 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -256,36 +256,19 @@ void SkGpuDevice::makeRenderTargetCurrent() {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-bool SkGpuDevice::readPixels(const SkIRect& srcRect, SkBitmap* bitmap) {
-    SkIRect bounds;
-    bounds.set(0, 0, this->width(), this->height());
-    if (!bounds.intersect(srcRect)) {
-        return false;
-    }
-
-    const int w = bounds.width();
-    const int h = bounds.height();
-    SkBitmap tmp;
-    // note we explicitly specify our rowBytes to be snug (no gap between rows)
-    tmp.setConfig(SkBitmap::kARGB_8888_Config, w, h, w * 4);
-    if (!tmp.allocPixels()) {
-        return false;
-    }
-
-    tmp.lockPixels();
-
-    bool read = fContext->readRenderTargetPixels(fRenderTarget,
-                                                 bounds.fLeft, bounds.fTop,
-                                                 bounds.width(), bounds.height(),
-                                                 kRGBA_8888_GrPixelConfig,
-                                                 tmp.getPixels());
-    tmp.unlockPixels();
-    if (!read) {
-        return false;
-    }
-
-    tmp.swap(*bitmap);
-    return true;
+bool SkGpuDevice::onReadPixels(const SkBitmap* bitmap, int x, int y) {
+    SkASSERT(SkBitmap::kARGB_8888_Config == bitmap->config());
+    SkASSERT(!bitmap->isNull());
+    SkASSERT(SkIRect::MakeWH(this->width(), this->height()).contains(SkIRect::MakeXYWH(x, y, bitmap->width(), bitmap->height())));
+
+    SkAutoLockPixels alp(*bitmap);
+    return fContext->readRenderTargetPixels(fRenderTarget,
+                                            x, y,
+                                            bitmap->width(),
+                                            bitmap->height(),
+                                            kRGBA_8888_GrPixelConfig,
+                                            bitmap->getPixels(),
+                                            bitmap->rowBytes());
 }
 
 void SkGpuDevice::writePixels(const SkBitmap& bitmap, int x, int y) {
diff --git a/tests/ReadPixelsTest.cpp b/tests/ReadPixelsTest.cpp
new file mode 100644
index 0000000000..a7083bb5a6
--- /dev/null
+++ b/tests/ReadPixelsTest.cpp
@@ -0,0 +1,260 @@
+
+/*
+ * Copyright 2011 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "Test.h"
+#include "SkCanvas.h"
+#include "SkRegion.h"
+#include "SkGpuDevice.h"
+
+
+static const int DEV_W = 100, DEV_H = 100;
+static const SkIRect DEV_RECT = SkIRect::MakeWH(DEV_W, DEV_H);
+static const SkRect DEV_RECT_S = SkRect::MakeWH(DEV_W * SK_Scalar1, 
+                                                DEV_H * SK_Scalar1);
+
+namespace {
+SkPMColor getCanvasColor(int x, int y) {
+    SkASSERT(x >= 0 && x < DEV_W);
+    SkASSERT(y >= 0 && y < DEV_H);
+    return SkPackARGB32(0xff, x, y, 0x0);
+}
+    
+SkPMColor getBitmapColor(int x, int y, int w, int h) {
+    int n = y * w + x;
+    
+    U8CPU b = n & 0xff;
+    U8CPU g = (n >> 8) & 0xff;
+    U8CPU r = (n >> 16) & 0xff;
+    return SkPackARGB32(0xff, r, g , b);
+}
+
+void fillCanvas(SkCanvas* canvas) {
+    static SkBitmap bmp;
+    if (bmp.isNull()) {
+        bmp.setConfig(SkBitmap::kARGB_8888_Config, DEV_W, DEV_H);
+        bool alloc = bmp.allocPixels();
+        SkASSERT(alloc);
+        SkAutoLockPixels alp(bmp);
+        intptr_t pixels = reinterpret_cast<intptr_t>(bmp.getPixels());
+        for (int y = 0; y < DEV_H; ++y) {
+            for (int x = 0; x < DEV_W; ++x) {
+                SkPMColor* pixel = reinterpret_cast<SkPMColor*>(pixels + y * bmp.rowBytes() + x * bmp.bytesPerPixel());
+                *pixel = getCanvasColor(x, y);
+            }
+        }
+    }
+    canvas->save();
+    canvas->setMatrix(SkMatrix::I());
+    canvas->clipRect(DEV_RECT_S, SkRegion::kReplace_Op);
+    SkPaint paint;
+    paint.setXfermodeMode(SkXfermode::kSrc_Mode);
+    canvas->drawBitmap(bmp, 0, 0, &paint);
+    canvas->restore();
+}
+    
+void fillBitmap(SkBitmap* bitmap) {
+    SkASSERT(bitmap->lockPixelsAreWritable());
+    SkAutoLockPixels alp(*bitmap);
+    int w = bitmap->width();
+    int h = bitmap->height();
+    intptr_t pixels = reinterpret_cast<intptr_t>(bitmap->getPixels());
+    for (int y = 0; y < h; ++y) {
+        for (int x = 0; x < w; ++x) {
+            SkPMColor* pixel = reinterpret_cast<SkPMColor*>(pixels + y * bitmap->rowBytes() + x * bitmap->bytesPerPixel());
+            *pixel = getBitmapColor(x, y, w, h);
+        }
+    }
+}
+
+// checks the bitmap contains correct pixels after the readPixels
+// if the bitmap was prefilled with pixels it checks that these weren't
+// overwritten in the area outside the readPixels.
+bool checkRead(skiatest::Reporter* reporter,
+               const SkBitmap& bitmap,
+               int x, int y,
+               bool preFilledBmp) {
+    SkASSERT(SkBitmap::kARGB_8888_Config == bitmap.config());
+    SkASSERT(!bitmap.isNull());
+    
+    int bw = bitmap.width();
+    int bh = bitmap.height();
+
+    SkIRect srcRect = SkIRect::MakeXYWH(x, y, bw, bh);
+    SkIRect clippedSrcRect = DEV_RECT;
+    if (!clippedSrcRect.intersect(srcRect)) {
+        clippedSrcRect.setEmpty();
+    }
+
+    SkAutoLockPixels alp(bitmap);
+    intptr_t pixels = reinterpret_cast<intptr_t>(bitmap.getPixels());
+    for (int by = 0; by < bh; ++by) {
+        for (int bx = 0; bx < bw; ++bx) {
+            int devx = bx + srcRect.fLeft;
+            int devy = by + srcRect.fTop;
+            
+            SkPMColor pixel = *reinterpret_cast<SkPMColor*>(pixels + by * bitmap.rowBytes() + bx * bitmap.bytesPerPixel());
+
+            if (clippedSrcRect.contains(devx, devy)) {
+                REPORTER_ASSERT(reporter, getCanvasColor(devx, devy) == pixel);
+                if (getCanvasColor(devx, devy) != pixel) {
+                    return false;
+                }
+            } else if (preFilledBmp) {
+                REPORTER_ASSERT(reporter, getBitmapColor(bx, by, bw, bh) == pixel);
+                if (getBitmapColor(bx, by, bw, bh) != pixel) {
+                    return false;
+                }
+            }
+        }
+    }
+    return true;
+}
+
+enum BitmapInit {
+    kFirstBitmapInit = 0,
+    
+    kNoPixels_BitmapInit = kFirstBitmapInit,
+    kTight_BitmapInit,
+    kRowBytes_BitmapInit,
+    
+    kBitmapInitCnt
+};
+
+BitmapInit nextBMI(BitmapInit bmi) {
+    int x = bmi;
+    return static_cast<BitmapInit>(++x);
+}
+
+
+void init_bitmap(SkBitmap* bitmap, const SkIRect& rect, BitmapInit init) {
+    int w = rect.width();
+    int h = rect.height();
+    int rowBytes = 0;
+    bool alloc = true;
+    switch (init) {
+        case kNoPixels_BitmapInit:
+            alloc = false;
+        case kTight_BitmapInit:
+            break;
+        case kRowBytes_BitmapInit:
+            rowBytes = w * sizeof(SkPMColor) + 16 * sizeof(SkPMColor);
+            break;
+        default:
+            SkASSERT(0);
+            break;
+    }
+    bitmap->setConfig(SkBitmap::kARGB_8888_Config, w, h, rowBytes);
+    if (alloc) {
+        bitmap->allocPixels();
+    }
+}
+
+void ReadPixelsTest(skiatest::Reporter* reporter, GrContext* context) {
+    SkCanvas canvas;
+    
+    const SkIRect testRects[] = {
+        // entire thing
+        DEV_RECT,
+        // larger on all sides
+        SkIRect::MakeLTRB(-10, -10, DEV_W + 10, DEV_H + 10),
+        // fully contained
+        SkIRect::MakeLTRB(DEV_W / 4, DEV_H / 4, 3 * DEV_W / 4, 3 * DEV_H / 4),
+        // outside top left
+        SkIRect::MakeLTRB(-10, -10, -1, -1),
+        // touching top left corner
+        SkIRect::MakeLTRB(-10, -10, 0, 0),
+        // overlapping top left corner
+        SkIRect::MakeLTRB(-10, -10, DEV_W / 4, DEV_H / 4),
+        // overlapping top left and top right corners
+        SkIRect::MakeLTRB(-10, -10, DEV_W  + 10, DEV_H / 4),
+        // touching entire top edge
+        SkIRect::MakeLTRB(-10, -10, DEV_W  + 10, 0),
+        // overlapping top right corner
+        SkIRect::MakeLTRB(3 * DEV_W / 4, -10, DEV_W  + 10, DEV_H / 4),
+        // contained in x, overlapping top edge
+        SkIRect::MakeLTRB(DEV_W / 4, -10, 3 * DEV_W  / 4, DEV_H / 4),
+        // outside top right corner
+        SkIRect::MakeLTRB(DEV_W + 1, -10, DEV_W + 10, -1),
+        // touching top right corner
+        SkIRect::MakeLTRB(DEV_W, -10, DEV_W + 10, 0),
+        // overlapping top left and bottom left corners
+        SkIRect::MakeLTRB(-10, -10, DEV_W / 4, DEV_H + 10),
+        // touching entire left edge
+        SkIRect::MakeLTRB(-10, -10, 0, DEV_H + 10),
+        // overlapping bottom left corner
+        SkIRect::MakeLTRB(-10, 3 * DEV_H / 4, DEV_W / 4, DEV_H + 10),
+        // contained in y, overlapping left edge
+        SkIRect::MakeLTRB(-10, DEV_H / 4, DEV_W / 4, 3 * DEV_H / 4),
+        // outside bottom left corner
+        SkIRect::MakeLTRB(-10, DEV_H + 1, -1, DEV_H + 10),
+        // touching bottom left corner
+        SkIRect::MakeLTRB(-10, DEV_H, 0, DEV_H + 10),
+        // overlapping bottom left and bottom right corners
+        SkIRect::MakeLTRB(-10, 3 * DEV_H / 4, DEV_W + 10, DEV_H + 10),
+        // touching entire left edge
+        SkIRect::MakeLTRB(0, DEV_H, DEV_W, DEV_H + 10),
+        // overlapping bottom right corner
+        SkIRect::MakeLTRB(3 * DEV_W / 4, 3 * DEV_H / 4, DEV_W + 10, DEV_H + 10),
+        // overlapping top right and bottom right corners
+        SkIRect::MakeLTRB(3 * DEV_W / 4, -10, DEV_W + 10, DEV_H + 10),
+    };
+    for (int dtype = 0; dtype < 2; ++dtype) {
+        if (0 == dtype) {
+            canvas.setDevice(new SkDevice(SkBitmap::kARGB_8888_Config, DEV_W, DEV_H, false))->unref();
+        } else {
+            canvas.setDevice(new SkGpuDevice(context, SkBitmap::kARGB_8888_Config, DEV_W, DEV_H))->unref();
+        }
+        fillCanvas(&canvas);
+
+        for (int rect = 0; rect < SK_ARRAY_COUNT(testRects); ++rect) {
+            SkBitmap bmp;
+            for (BitmapInit bmi = kFirstBitmapInit; bmi < kBitmapInitCnt; bmi = nextBMI(bmi)) {
+
+                const SkIRect& srcRect = testRects[rect];
+
+                init_bitmap(&bmp, srcRect, bmi);
+
+                // if the bitmap has pixels allocated before the readPixels, note
+                // that and fill them with pattern
+                bool startsWithPixels = !bmp.isNull();
+                if (startsWithPixels) {
+                    fillBitmap(&bmp);
+                }
+                
+                bool success = canvas.readPixels(&bmp, srcRect.fLeft, srcRect.fTop);
+                
+                // determine whether we expected the read to succeed.
+                REPORTER_ASSERT(reporter, success == SkIRect::Intersects(srcRect, DEV_RECT));
+
+                if (success || startsWithPixels) {
+                    checkRead(reporter, bmp, srcRect.fLeft, srcRect.fTop, startsWithPixels);
+                } else {
+                    // if we had no pixels beforehand and the readPixels failed then
+                    // our bitmap should still not have any pixels
+                    REPORTER_ASSERT(reporter, bmp.isNull());
+                }
+
+                // check the old webkit version of readPixels that clips the bitmap size
+                SkBitmap wkbmp;
+                success = canvas.readPixels(srcRect, &wkbmp);
+                SkIRect clippedRect = DEV_RECT;
+                if (clippedRect.intersect(srcRect)) {
+                    REPORTER_ASSERT(reporter, success);
+                    checkRead(reporter, wkbmp, clippedRect.fLeft, clippedRect.fTop, false);
+                } else {
+                    REPORTER_ASSERT(reporter, !success);
+                }
+            }
+        }
+    }
+}
+}
+
+#include "TestClassDef.h"
+DEFINE_GPUTESTCLASS("ReadPixels", ReadPixelsTestClass, ReadPixelsTest)
+