Move some parts of onReadPixels up to GrGpu readPixels.

src/gpu/gl/GrGLGpu.cpp

Index: src/gpu/gl/GrGLGpu.cpp
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 781e7721e3d8731f0ac5ce888d896e88ffa6ad43..b16d1fd9f12b6510ba37e0716dd5c1f4233d8d15 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -1793,14 +1793,10 @@ bool GrGLGpu::onReadPixels(GrSurface* surface,
                            int width, int height,
                            GrPixelConfig config,
                            void* buffer,
-                           size_t rowBytes) {
+                           size_t rowBytes,
+                           size_t tightRowBytes) {

[bsalomon, 2015/07/30 18:27:22]

Personally I find the tightRowBytes param is more clutter than is justified by
the cost of recomputing in the subclass (and I think the computation in
GrGpu::readPixels isn't really necessary in that function).

[egdaniel, 2015/07/30 18:47:34]

Done.

     SkASSERT(surface);
 
-    // We cannot read pixels into a compressed buffer
-    if (GrPixelConfigIsCompressed(config)) {
-        return false;
-    }
-
     GrGLRenderTarget* tgt = static_cast<GrGLRenderTarget*>(surface->asRenderTarget());
     if (!tgt) {
         return false;
@@ -1812,13 +1808,6 @@ bool GrGLGpu::onReadPixels(GrSurface* surface,
     if (!this->configToGLFormats(config, false, NULL, &format, &type)) {
         return false;
     }
-    size_t bpp = GrBytesPerPixel(config);
-    if (!GrSurfacePriv::AdjustReadPixelParams(surface->width(), surface->height(), bpp,
-                                              &left, &top, &width, &height,
-                                              &buffer,
-                                              &rowBytes)) {
-        return false;
-    }
 
     // resolve the render target if necessary

[egdaniel, 2015/07/30 18:00:49]

I also wouldn't be against a virtual on Gpu that is like
resolveRT(GrRenderTarget). Then I would just move this up as well (obviously
only resolving if the surface is a RT)

[bsalomon, 2015/07/30 18:27:22]

This is doing more than just resolving (that is handled by the call to
onResolveRenderTarget below). It is ensuring that the correct FBO is bound for
reading. If we want to resolve in the base class we can just call
GrGpu::resolveRenderTarget(). It'd preclude any implementation that could
downsample and blit to CPU accessible memory in one step (though I know of no
such thing). However, we will wind up revisiting this in any event when we
reorganize the GrSurface family of objects.

[egdaniel, 2015/07/30 18:47:34]

Sounds good

     switch (tgt->getResolveType()) {
@@ -1844,10 +1833,6 @@ bool GrGLGpu::onReadPixels(GrSurface* surface,
     GrGLIRect readRect;
     readRect.setRelativeTo(glvp, left, top, width, height, tgt->origin());
 
-    size_t tightRowBytes = bpp * width;
-    if (0 == rowBytes) {
-        rowBytes = tightRowBytes;
-    }
     size_t readDstRowBytes = tightRowBytes;
     void* readDst = buffer;