Remove origin from GrGpu::readPixels

Change-Id: I40d046c66240ab40794aa008861a1974f7f9182c
Reviewed-on: https://skia-review.googlesource.com/131620
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

9 files changed, 44 insertions, 134 deletions

diff --git a/src/gpu/GrGpu.cpp b/src/gpu/GrGpu.cpp
index 7f60129d51..5c2864db3c 100644
--- a/src/gpu/GrGpu.cpp
+++ b/src/gpu/GrGpu.cpp
@@ -214,8 +214,8 @@ bool GrGpu::copySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
                                canDiscardOutsideDstRect);
 }
 
-bool GrGpu::readPixels(GrSurface* surface, GrSurfaceOrigin origin, int left, int top, int width,
-                       int height, GrColorType dstColorType, void* buffer, size_t rowBytes) {
+bool GrGpu::readPixels(GrSurface* surface, int left, int top, int width, int height,
+                       GrColorType dstColorType, void* buffer, size_t rowBytes) {
     SkASSERT(surface);
 
     int bpp = GrColorTypeBytesPerPixel(dstColorType);
@@ -228,8 +228,7 @@ bool GrGpu::readPixels(GrSurface* surface, GrSurfaceOrigin origin, int left, int
 
     this->handleDirtyContext();
 
-    return this->onReadPixels(surface, origin, left, top, width, height, dstColorType, buffer,
-                              rowBytes);
+    return this->onReadPixels(surface, left, top, width, height, dstColorType, buffer, rowBytes);
 }
 
 bool GrGpu::writePixels(GrSurface* surface, int left, int top, int width, int height,
diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h
index 6958ea997f..ad20d52366 100644
--- a/src/gpu/GrGpu.h
+++ b/src/gpu/GrGpu.h
@@ -160,17 +160,8 @@ public:
      *              because of a unsupported pixel config or because no render
      *              target is currently set.
      */
-    bool readPixels(GrSurface* surface, GrSurfaceOrigin, int left, int top, int width, int height,
-                    GrColorType dstColorType, void* buffer, size_t rowBytes);
-    /**
-     * This version of readPixels doesn't take an origin. TODO: Remove origin handling from
-     * GrGpu::readPixels entirely.
-     */
     bool readPixels(GrSurface* surface, int left, int top, int width, int height,
-                    GrColorType dstColorType, void* buffer, size_t rowBytes) {
-        return this->readPixels(surface, kTopLeft_GrSurfaceOrigin, left, top, width, height,
-                                dstColorType, buffer, rowBytes);
-    }
+                    GrColorType dstColorType, void* buffer, size_t rowBytes);
 
     /**
      * Updates the pixels in a rectangle of a surface.  No sRGB/linear conversions are performed.
@@ -436,8 +427,8 @@ private:
                                      const void* data) = 0;
 
     // overridden by backend-specific derived class to perform the surface read
-    virtual bool onReadPixels(GrSurface*, GrSurfaceOrigin, int left, int top, int width, int height,
-                              GrColorType, void* buffer, size_t rowBytes) = 0;
+    virtual bool onReadPixels(GrSurface*, int left, int top, int width, int height, GrColorType,
+                              void* buffer, size_t rowBytes) = 0;
 
     // overridden by backend-specific derived class to perform the surface write
     virtual bool onWritePixels(GrSurface*, int left, int top, int width, int height, GrColorType,
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index aa5d2e7bd4..e89439a07a 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -26,6 +26,7 @@
 #include "GrTexturePriv.h"
 #include "GrTypes.h"
 #include "SkAutoMalloc.h"
+#include "SkConvertPixels.h"
 #include "SkHalf.h"
 #include "SkJSONWriter.h"
 #include "SkMakeUnique.h"
@@ -1063,11 +1064,7 @@ bool GrGLGpu::uploadTexData(GrPixelConfig texConfig, int texWidth, int texHeight
             // copy data into our new storage, skipping the trailing bytes
             const char* src = (const char*)texelsShallowCopy[currentMipLevel].fPixels;
             char* dst = buffer + individualMipOffsets[currentMipLevel];
-            for (int y = 0; y < currentHeight; y++) {
-                memcpy(dst, src, trimRowBytes);
-                src += rowBytes;
-                dst += trimRowBytes;
-            }
+            SkRectMemcpy(dst, trimRowBytes, src, rowBytes, trimRowBytes, currentHeight);
             // now point data to our copied version
             texelsShallowCopy[currentMipLevel].fPixels = buffer +
                 individualMipOffsets[currentMipLevel];
@@ -2025,8 +2022,8 @@ bool GrGLGpu::readPixelsSupported(GrSurface* surfaceForConfig, GrPixelConfig rea
     }
 }
 
-bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left, int top, int width,
-                           int height, GrColorType dstColorType, void* buffer, size_t rowBytes) {
+bool GrGLGpu::onReadPixels(GrSurface* surface, int left, int top, int width, int height,
+                           GrColorType dstColorType, void* buffer, size_t rowBytes) {
     SkASSERT(surface);
 
     GrGLRenderTarget* renderTarget = static_cast<GrGLRenderTarget*>(surface->asRenderTarget());
@@ -2043,8 +2040,8 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
         if (kAlpha_8_GrPixelConfig == dstAsConfig &&
             this->readPixelsSupported(surface, kRGBA_8888_GrPixelConfig)) {
             std::unique_ptr<uint32_t[]> temp(new uint32_t[width * height * 4]);
-            if (this->onReadPixels(surface, origin, left, top, width, height,
-                                   GrColorType::kRGBA_8888, temp.get(), width * 4)) {
+            if (this->onReadPixels(surface, left, top, width, height, GrColorType::kRGBA_8888,
+                                   temp.get(), width * 4)) {
                 uint8_t* dst = reinterpret_cast<uint8_t*>(buffer);
                 for (int j = 0; j < height; ++j) {
                     for (int i = 0; i < width; ++i) {
@@ -2060,8 +2057,8 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
         if (kRGBA_half_GrPixelConfig == dstAsConfig &&
             this->readPixelsSupported(surface, kRGBA_float_GrPixelConfig)) {
             std::unique_ptr<float[]> temp(new float[width * height * 4]);
-            if (this->onReadPixels(surface, origin, left, top, width, height,
-                                   GrColorType::kRGBA_F32, temp.get(), width * sizeof(float) * 4)) {
+            if (this->onReadPixels(surface, left, top, width, height, GrColorType::kRGBA_F32,
+                                   temp.get(), width * sizeof(float) * 4)) {
                 uint8_t* dst = reinterpret_cast<uint8_t*>(buffer);
                 float* src = temp.get();
                 for (int j = 0; j < height; ++j) {
@@ -2085,7 +2082,6 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
                                             &externalType)) {
         return false;
     }
-    bool flipY = kBottomLeft_GrSurfaceOrigin == origin;
 
     GrGLIRect glvp;
     if (renderTarget) {
@@ -2113,7 +2109,7 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
 
     // the read rect is viewport-relative
     GrGLIRect readRect;
-    readRect.setRelativeTo(glvp, left, top, width, height, origin);
+    readRect.setRelativeTo(glvp, left, top, width, height, kTopLeft_GrSurfaceOrigin);
 
     int bytesPerPixel = GrBytesPerPixel(dstAsConfig);
     size_t tightRowBytes = bytesPerPixel * width;
@@ -2121,8 +2117,7 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
     size_t readDstRowBytes = tightRowBytes;
     void* readDst = buffer;
 
-    // determine if GL can read using the passed rowBytes or if we need
-    // a scratch 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 (this->glCaps().packRowLengthSupport() && !(rowBytes % bytesPerPixel)) {
@@ -2134,9 +2129,6 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
             readDst = scratch.get();
         }
     }
-    if (flipY && this->glCaps().packFlipYSupport()) {
-        GL_CALL(PixelStorei(GR_GL_PACK_REVERSE_ROW_ORDER, 1));
-    }
     GL_CALL(PixelStorei(GR_GL_PACK_ALIGNMENT, config_alignment(dstAsConfig)));
 
     GL_CALL(ReadPixels(readRect.fLeft, readRect.fBottom,
@@ -2146,50 +2138,13 @@ bool GrGLGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
         SkASSERT(this->glCaps().packRowLengthSupport());
         GL_CALL(PixelStorei(GR_GL_PACK_ROW_LENGTH, 0));
     }
-    if (flipY && this->glCaps().packFlipYSupport()) {
-        GL_CALL(PixelStorei(GR_GL_PACK_REVERSE_ROW_ORDER, 0));
-        flipY = false;
-    }
-
-    // now reverse the order of the rows, since GL's are bottom-to-top, but our
-    // API presents top-to-bottom. We must preserve the padding contents. Note
-    // that the above readPixels did not overwrite the padding.
-    if (readDst == buffer) {
-        SkASSERT(rowBytes == readDstRowBytes);
-        if (flipY) {
-            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) * rowBytes;
-            for (int y = 0; y < halfY; y++) {
-                memcpy(tmpRow, top, tightRowBytes);
-                memcpy(top, bottom, tightRowBytes);
-                memcpy(bottom, tmpRow, tightRowBytes);
-                top += rowBytes;
-                bottom -= rowBytes;
-            }
-        }
-    } else {
+
+    if (readDst != buffer) {
         SkASSERT(readDst != buffer);
         SkASSERT(rowBytes != tightRowBytes);
-        // 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);
-        if (flipY) {
-            dst += (height-1) * rowBytes;
-        }
-        for (int y = 0; y < height; y++) {
-            memcpy(dst, src, tightRowBytes);
-            src += readDstRowBytes;
-            if (!flipY) {
-                dst += rowBytes;
-            } else {
-                dst -= rowBytes;
-            }
-        }
+        SkRectMemcpy(dst, rowBytes, src, readDstRowBytes, tightRowBytes, height);
     }
     if (!renderTarget) {
         this->unbindTextureFBOForPixelOps(GR_GL_FRAMEBUFFER, surface);
diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h
index 508b0f862d..7240f9d763 100644
--- a/src/gpu/gl/GrGLGpu.h
+++ b/src/gpu/gl/GrGLGpu.h
@@ -231,8 +231,8 @@ private:
     // variations above, depending on whether the surface is a render target or not.
     bool readPixelsSupported(GrSurface* surfaceForConfig, GrPixelConfig readConfig);
 
-    bool onReadPixels(GrSurface*, GrSurfaceOrigin, int left, int top, int width, int height,
-                      GrColorType, void* buffer, size_t rowBytes) override;
+    bool onReadPixels(GrSurface*, int left, int top, int width, int height, GrColorType,
+                      void* buffer, size_t rowBytes) override;
 
     bool onWritePixels(GrSurface*, int left, int top, int width, int height, GrColorType,
                        const GrMipLevel texels[], int mipLevelCount) override;
diff --git a/src/gpu/mock/GrMockGpu.h b/src/gpu/mock/GrMockGpu.h
index 0b874bb7fa..3bb4a85772 100644
--- a/src/gpu/mock/GrMockGpu.h
+++ b/src/gpu/mock/GrMockGpu.h
@@ -71,8 +71,8 @@ private:
     GrBuffer* onCreateBuffer(size_t sizeInBytes, GrBufferType, GrAccessPattern,
                              const void*) override;
 
-    bool onReadPixels(GrSurface* surface, GrSurfaceOrigin, int left, int top, int width, int height,
-                      GrColorType, void* buffer, size_t rowBytes) override {
+    bool onReadPixels(GrSurface* surface, int left, int top, int width, int height, GrColorType,
+                      void* buffer, size_t rowBytes) override {
         return true;
     }
 
diff --git a/src/gpu/mtl/GrMtlGpu.h b/src/gpu/mtl/GrMtlGpu.h
index 4e1c5e7ff4..e325ad61b0 100644
--- a/src/gpu/mtl/GrMtlGpu.h
+++ b/src/gpu/mtl/GrMtlGpu.h
@@ -96,8 +96,8 @@ private:
         return nullptr;
     }
 
-    bool onReadPixels(GrSurface* surface, GrSurfaceOrigin, int left, int top, int width, int height,
-                      GrColorType, void* buffer, size_t rowBytes) override {
+    bool onReadPixels(GrSurface* surface, int left, int top, int width, int height, GrColorType,
+                      void* buffer, size_t rowBytes) override {
         return false;
     }
 
diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp
index f115a51abf..a99931188d 100644
--- a/src/gpu/vk/GrVkGpu.cpp
+++ b/src/gpu/vk/GrVkGpu.cpp
@@ -1776,8 +1776,8 @@ bool GrVkGpu::onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
     return false;
 }
 
-bool GrVkGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left, int top, int width,
-                           int height, GrColorType dstColorType, void* buffer, size_t rowBytes) {
+bool GrVkGpu::onReadPixels(GrSurface* surface, int left, int top, int width, int height,
+                           GrColorType dstColorType, void* buffer, size_t rowBytes) {
     if (GrPixelConfigToColorType(surface->config()) != dstColorType) {
         return false;
     }
@@ -1815,7 +1815,6 @@ bool GrVkGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
 
     int bpp = GrColorTypeBytesPerPixel(dstColorType);
     size_t tightRowBytes = bpp * width;
-    bool flipY = kBottomLeft_GrSurfaceOrigin == origin;
 
     VkBufferImageCopy region;
     memset(&region, 0, sizeof(VkBufferImageCopy));
@@ -1823,16 +1822,9 @@ bool GrVkGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
     bool copyFromOrigin = this->vkCaps().mustDoCopiesFromOrigin();
     if (copyFromOrigin) {
         region.imageOffset = { 0, 0, 0 };
-        region.imageExtent = { (uint32_t)(left + width),
-                               (uint32_t)(flipY ? surface->height() - top : top + height),
-                               1
-                             };
+        region.imageExtent = { (uint32_t)(left + width), (uint32_t)(top + height), 1 };
     } else {
-        VkOffset3D offset = {
-            left,
-            flipY ? surface->height() - top - height : top,
-            0
-        };
+        VkOffset3D offset = { left, top, 0 };
         region.imageOffset = offset;
         region.imageExtent = { (uint32_t)width, (uint32_t)height, 1 };
     }
@@ -1877,17 +1869,7 @@ bool GrVkGpu::onReadPixels(GrSurface* surface, GrSurfaceOrigin origin, int left,
         mappedMemory = (char*)mappedMemory + transBufferRowBytes * skipRows + bpp * left;
     }
 
-    if (flipY) {
-        const char* srcRow = reinterpret_cast<const char*>(mappedMemory);
-        char* dstRow = reinterpret_cast<char*>(buffer)+(height - 1) * rowBytes;
-        for (int y = 0; y < height; y++) {
-            memcpy(dstRow, srcRow, tightRowBytes);
-            srcRow += transBufferRowBytes;
-            dstRow -= rowBytes;
-        }
-    } else {
-        SkRectMemcpy(buffer, rowBytes, mappedMemory, transBufferRowBytes, tightRowBytes, height);
-    }
+    SkRectMemcpy(buffer, rowBytes, mappedMemory, transBufferRowBytes, tightRowBytes, height);
 
     transferBuffer->unmap();
     transferBuffer->unref();
diff --git a/src/gpu/vk/GrVkGpu.h b/src/gpu/vk/GrVkGpu.h
index 4284bbca6a..5df1675794 100644
--- a/src/gpu/vk/GrVkGpu.h
+++ b/src/gpu/vk/GrVkGpu.h
@@ -165,8 +165,8 @@ private:
     GrBuffer* onCreateBuffer(size_t size, GrBufferType type, GrAccessPattern,
                              const void* data) override;
 
-    bool onReadPixels(GrSurface* surface, GrSurfaceOrigin, int left, int top, int width, int height,
-                      GrColorType, void* buffer, size_t rowBytes) override;
+    bool onReadPixels(GrSurface* surface, int left, int top, int width, int height, GrColorType,
+                      void* buffer, size_t rowBytes) override;
 
     bool onWritePixels(GrSurface* surface, int left, int top, int width, int height, GrColorType,
                        const GrMipLevel texels[], int mipLevelCount) override;
diff --git a/tests/TransferPixelsTest.cpp b/tests/TransferPixelsTest.cpp
index 5bb309494e..339a591660 100755
--- a/tests/TransferPixelsTest.cpp
+++ b/tests/TransferPixelsTest.cpp
@@ -41,12 +41,9 @@ bool does_full_buffer_contain_correct_values(GrColor* srcBuffer,
                                              int width,
                                              int height,
                                              int bufferWidth,
-                                             int bufferHeight,
-                                             GrSurfaceOrigin origin) {
+                                             int bufferHeight) {
     GrColor* srcPtr = srcBuffer;
-    bool bottomUp = SkToBool(kBottomLeft_GrSurfaceOrigin == origin);
-    GrColor* dstPtr = bottomUp ? dstBuffer + bufferWidth*(bufferHeight-1) : dstBuffer;
-    int dstIncrement = bottomUp ? -bufferWidth : +bufferWidth;
+    GrColor* dstPtr = dstBuffer;
 
     for (int j = 0; j < height; ++j) {
         for (int i = 0; i < width; ++i) {
@@ -55,13 +52,13 @@ bool does_full_buffer_contain_correct_values(GrColor* srcBuffer,
             }
         }
         srcPtr += bufferWidth;
-        dstPtr += dstIncrement;
+        dstPtr += bufferWidth;
     }
     return true;
 }
 
 void basic_transfer_test(skiatest::Reporter* reporter, GrContext* context, GrColorType colorType,
-                         GrSurfaceOrigin origin, bool renderTarget) {
+                         bool renderTarget) {
     if (GrCaps::kNone_MapFlags == context->contextPriv().caps()->mapBufferFlags()) {
         return;
     }
@@ -125,7 +122,7 @@ void basic_transfer_test(skiatest::Reporter* reporter, GrContext* context, GrCol
         REPORTER_ASSERT(reporter, result);
 
         memset(dstBuffer.get(), 0xCDCD, size);
-        result = gpu->readPixels(tex.get(), origin, 0, 0, kTextureWidth, kTextureHeight, colorType,
+        result = gpu->readPixels(tex.get(), 0, 0, kTextureWidth, kTextureHeight, colorType,
                                  dstBuffer.get(), rowBytes);
         if (result) {
             REPORTER_ASSERT(reporter, does_full_buffer_contain_correct_values(srcBuffer,
@@ -133,8 +130,7 @@ void basic_transfer_test(skiatest::Reporter* reporter, GrContext* context, GrCol
                                                                               kTextureWidth,
                                                                               kTextureHeight,
                                                                               kBufferWidth,
-                                                                              kBufferHeight,
-                                                                              origin));
+                                                                              kBufferHeight));
         }
 
         //////////////////////////
@@ -157,7 +153,7 @@ void basic_transfer_test(skiatest::Reporter* reporter, GrContext* context, GrCol
         REPORTER_ASSERT(reporter, result);
 
         memset(dstBuffer.get(), 0xCDCD, size);
-        result = gpu->readPixels(tex.get(), origin, 0, 0, kTextureWidth, kTextureHeight, colorType,
+        result = gpu->readPixels(tex.get(), 0, 0, kTextureWidth, kTextureHeight, colorType,
                                  dstBuffer.get(), rowBytes);
         if (result) {
             REPORTER_ASSERT(reporter, does_full_buffer_contain_correct_values(srcBuffer,
@@ -165,30 +161,17 @@ void basic_transfer_test(skiatest::Reporter* reporter, GrContext* context, GrCol
                                                                               kTextureWidth,
                                                                               kTextureHeight,
                                                                               kBufferWidth,
-                                                                              kBufferHeight,
-                                                                              origin));
+                                                                              kBufferHeight));
         }
     }
 }
 
 DEF_GPUTEST_FOR_RENDERING_CONTEXTS(TransferPixelsTest, reporter, ctxInfo) {
     // RGBA
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888,
-                        kTopLeft_GrSurfaceOrigin, false);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888,
-                        kTopLeft_GrSurfaceOrigin, true);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888,
-                        kBottomLeft_GrSurfaceOrigin, false);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888,
-                        kBottomLeft_GrSurfaceOrigin, true);
+    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888, false);
+    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kRGBA_8888, true);
 
     // BGRA
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888,
-                        kTopLeft_GrSurfaceOrigin, false);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888,
-                        kTopLeft_GrSurfaceOrigin, true);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888,
-                        kBottomLeft_GrSurfaceOrigin, false);
-    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888,
-                        kBottomLeft_GrSurfaceOrigin, true);
+    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888, false);
+    basic_transfer_test(reporter, ctxInfo.grContext(), GrColorType::kBGRA_8888, true);
 }