Attempt to somewhat simplify GrContext::readSurfacePixels interaction with GrGpu.

Review URL: https://codereview.chromium.org/1255483005

5 files changed, 192 insertions, 200 deletions

diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 17b4b74a39..ba02d60b11 100755
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -450,16 +450,6 @@ bool GrContext::writeSurfacePixels(GrSurface* surface,
     return true;
 }
 
-// toggles between RGBA and BGRA
-static SkColorType toggle_colortype32(SkColorType ct) {
-    if (kRGBA_8888_SkColorType == ct) {
-        return kBGRA_8888_SkColorType;
-    } else {
-        SkASSERT(kBGRA_8888_SkColorType == ct);
-        return kRGBA_8888_SkColorType;
-    }
-}
-
 bool GrContext::readSurfacePixels(GrSurface* src,
                                   int left, int top, int width, int height,
                                   GrPixelConfig dstConfig, void* buffer, size_t rowBytes,
@@ -480,126 +470,87 @@ bool GrContext::readSurfacePixels(GrSurface* src,
         this->flush();
     }
 
-    // Determine which conversions have to be applied: flipY, swapRAnd, and/or unpremul.
-
-    // We ignore the preferred config if it is different than our config unless it is an R/B swap.
-    // In that case we'll perform an R and B swap while drawing to a scratch texture of the swapped
-    // config. Then we will call readPixels on the scratch with the swapped config. The swaps during
-    // the draw cancels out the fact that we call readPixels with a config that is R/B swapped from
-    // dstConfig.
-    GrPixelConfig readConfig = dstConfig;
-    bool swapRAndB = false;
-    if (GrPixelConfigSwapRAndB(dstConfig) ==
-        fGpu->preferredReadPixelsConfig(dstConfig, src->config())) {
-        readConfig = GrPixelConfigSwapRAndB(readConfig);
-        swapRAndB = true;
-    }
-
-    bool flipY = false;
-    GrRenderTarget* srcAsRT = src->asRenderTarget();
-    if (srcAsRT) {
-        // If fGpu->readPixels would incur a y-flip cost then we will read the pixels upside down.
-        // We'll either do the flipY by drawing into a scratch with a matrix or on the cpu after the
-        // read.
-        flipY = fGpu->readPixelsWillPayForYFlip(srcAsRT, left, top,
-                                                width, height, dstConfig,
-                                                rowBytes);
-    }
-
     bool unpremul = SkToBool(kUnpremul_PixelOpsFlag & flags);
     if (unpremul && !GrPixelConfigIs8888(dstConfig)) {
-        // The unpremul flag is only allowed for these two configs.
+        // The unpremul flag is only allowed for 8888 configs.
         return false;
     }
 
-    SkAutoTUnref<GrTexture> tempTexture;
-
-    // If the src is a texture and we would have to do conversions after read pixels, we instead
-    // do the conversions by drawing the src to a scratch texture. If we handle any of the
-    // conversions in the draw we set the corresponding bool to false so that we don't reapply it
-    // on the read back pixels. We also do an intermediate draw if the src is not a render target as
-    // GrGpu currently supports reading from render targets but not textures.
-    GrTexture* srcAsTex = src->asTexture();
-    GrRenderTarget* rtToRead = srcAsRT;
-    if (srcAsTex && (swapRAndB || unpremul || flipY || !srcAsRT)) {
-        // Make the scratch a render so we can read its pixels.
-        GrSurfaceDesc desc;
-        desc.fFlags = kRenderTarget_GrSurfaceFlag;
-        desc.fWidth = width;
-        desc.fHeight = height;
-        desc.fConfig = readConfig;
-        desc.fOrigin = kTopLeft_GrSurfaceOrigin;
-
-        // When a full read back is faster than a partial we could always make the scratch exactly
-        // match the passed rect. However, if we see many different size rectangles we will trash
-        // our texture cache and pay the cost of creating and destroying many textures. So, we only
-        // request an exact match when the caller is reading an entire RT.
+    GrGpu::DrawPreference drawPreference = unpremul ? GrGpu::kCallerPrefersDraw_DrawPreference :
+                                                      GrGpu::kNoDraw_DrawPreference;
+    GrGpu::ReadPixelTempDrawInfo tempDrawInfo;
+    if (!fGpu->getReadPixelsInfo(src, width, height, rowBytes, dstConfig, &drawPreference,
+                                 &tempDrawInfo)) {
+        return false;
+    }
+
+    GrRenderTarget* rtToRead = src->asRenderTarget();
+    bool didTempDraw = false;
+    if (GrGpu::kNoDraw_DrawPreference != drawPreference) {
         GrTextureProvider::ScratchTexMatch match = GrTextureProvider::kApprox_ScratchTexMatch;
-        if (0 == left &&
-            0 == top &&
-            src->width() == width &&
-            src->height() == height &&
-            fGpu->fullReadPixelsIsFasterThanPartial()) {
-            match = GrTextureProvider::kExact_ScratchTexMatch;
+        if (tempDrawInfo.fUseExactScratch) {
+            // We only respect this when the entire src is being read. Otherwise we can trigger too
+            // many odd ball texture sizes and trash the cache.
+            if (width == src->width() && height == src->height()) {
+                match = GrTextureProvider::kExact_ScratchTexMatch;
+            }
         }
-        tempTexture.reset(this->textureProvider()->refScratchTexture(desc, match));
-        if (tempTexture) {
-            // compute a matrix to perform the draw
+        SkAutoTUnref<GrTexture> temp;
+        temp.reset(this->textureProvider()->refScratchTexture(tempDrawInfo.fTempSurfaceDesc, match));
+        if (temp) {
             SkMatrix textureMatrix;
-            textureMatrix.setTranslate(SK_Scalar1 *left, SK_Scalar1 *top);
+            textureMatrix.setTranslate(SkIntToScalar(left), SkIntToScalar(top));
             textureMatrix.postIDiv(src->width(), src->height());
-
             GrPaint paint;
             SkAutoTUnref<const GrFragmentProcessor> fp;
             if (unpremul) {
-                fp.reset(this->createPMToUPMEffect(paint.getProcessorDataManager(), srcAsTex,
-                                                   swapRAndB, textureMatrix));
+                fp.reset(this->createPMToUPMEffect(
+                    paint.getProcessorDataManager(), src->asTexture(), tempDrawInfo.fSwapRAndB,
+                    textureMatrix));
                 if (fp) {
                     unpremul = false; // we no longer need to do this on CPU after the read back.
+                } else if (GrGpu::kCallerPrefersDraw_DrawPreference == drawPreference) {
+                    // We only wanted to do the draw in order to perform the unpremul so don't
+                    // bother.
+                    temp.reset(NULL);
                 }
             }
-            // If we failed to create a PM->UPM effect and have no other conversions to perform then
-            // there is no longer any point to using the scratch.
-            if (fp || flipY || swapRAndB || !srcAsRT) {
-                if (!fp) {
-                    fp.reset(GrConfigConversionEffect::Create(paint.getProcessorDataManager(),
-                            srcAsTex, swapRAndB, GrConfigConversionEffect::kNone_PMConversion,
-                            textureMatrix));
-                }
-                swapRAndB = false; // we will handle the swap in the draw.
-
-                // We protect the existing geometry here since it may not be
-                // clear to the caller that a draw operation (i.e., drawSimpleRect)
-                // can be invoked in this method
-                {
-                    GrDrawContext* drawContext = this->drawContext();
-                    if (!drawContext) {
-                        return false;
-                    }
-
-                    paint.addColorProcessor(fp);
-
-                    SkRect rect = SkRect::MakeWH(SkIntToScalar(width), SkIntToScalar(height));
-
-                    drawContext->drawRect(tempTexture->asRenderTarget(), GrClip::WideOpen(), paint,
-                                          SkMatrix::I(), rect, NULL);                    
-
-                    // we want to read back from the scratch's origin
-                    left = 0;
-                    top = 0;
-                    rtToRead = tempTexture->asRenderTarget();
-                }
-                this->flushSurfaceWrites(tempTexture);
+            if (!fp && temp) {
+                fp.reset(GrConfigConversionEffect::Create(
+                    paint.getProcessorDataManager(), src->asTexture(), tempDrawInfo.fSwapRAndB,
+                    GrConfigConversionEffect::kNone_PMConversion, textureMatrix));
+            }
+            if (fp) {
+                paint.addColorProcessor(fp);
+                SkRect rect = SkRect::MakeWH(SkIntToScalar(width), SkIntToScalar(height));
+                GrDrawContext* drawContext = this->drawContext();
+                drawContext->drawRect(temp->asRenderTarget(), GrClip::WideOpen(), paint,
+                                      SkMatrix::I(), rect, NULL);
+                rtToRead = temp->asRenderTarget();
+                left = 0;
+                top = 0;
+                didTempDraw = true;
             }
         }
     }
 
-    if (!rtToRead ||
-        !fGpu->readPixels(rtToRead, left, top, width, height, readConfig, buffer, rowBytes)) {
+    if (GrGpu::kRequireDraw_DrawPreference == drawPreference && !didTempDraw) {
         return false;
     }
-    // Perform any conversions we weren't able to perform using a scratch texture.
-    if (unpremul || swapRAndB) {
+    GrPixelConfig configToRead = dstConfig;
+    if (didTempDraw) {
+        this->flushSurfaceWrites(rtToRead);
+        // We swapped R and B while doing the temp draw. Swap back on the read.
+        if (tempDrawInfo.fSwapRAndB) {
+            configToRead = GrPixelConfigSwapRAndB(dstConfig);
+        }
+    }
+    if (!fGpu->readPixels(rtToRead, left, top, width, height, configToRead, buffer, rowBytes)) {
+        return false;
+    }
+
+    // Perform umpremul conversion if we weren't able to perform it as a draw.
+    if (unpremul) {
         SkDstPixelInfo dstPI;
         if (!GrPixelConfig2ColorAndProfileType(dstConfig, &dstPI.fColorType, NULL)) {
             return false;
@@ -609,7 +560,7 @@ bool GrContext::readSurfacePixels(GrSurface* src,
         dstPI.fRowBytes = rowBytes;
 
         SkSrcPixelInfo srcPI;
-        srcPI.fColorType = swapRAndB ? toggle_colortype32(dstPI.fColorType) : dstPI.fColorType;
+        srcPI.fColorType = dstPI.fColorType;
         srcPI.fAlphaType = kPremul_SkAlphaType;
         srcPI.fPixels = buffer;
         srcPI.fRowBytes = rowBytes;
diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h
index 9365d8927f..857efd0d65 100644
--- a/src/gpu/GrGpu.h
+++ b/src/gpu/GrGpu.h
@@ -133,15 +133,58 @@ public:
      */
     void resolveRenderTarget(GrRenderTarget* target);
 
+    /** Info struct returned by getReadPixelsInfo about performing intermediate draws before 
+        reading pixels for performance or correctness. */
+    struct ReadPixelTempDrawInfo {
+        /** If the GrGpu is requesting that the caller do a draw to an intermediate surface then
+            this is descriptor for the temp surface. The draw should always be a rect with
+            dst 0,0,w,h. */
+        GrSurfaceDesc   fTempSurfaceDesc;
+        /** Indicates whether there is a performance advantage to using an exact match texture
+            (in terms of width and height) for the intermediate texture instead of approximate. */
+        bool            fUseExactScratch;
+        /** The caller should swap the R and B channel in the temp draw and then instead of reading
+            the desired config back it should read GrPixelConfigSwapRAndB(readConfig). The swap
+            during the draw and the swap at readback time cancel and the client gets the correct
+            data. The swapped read back is either faster for or required by the underlying backend
+            3D API. */
+        bool            fSwapRAndB;
+    };
+    /** Describes why an intermediate draw must/should be performed before readPixels. */
+    enum DrawPreference {
+        /** On input means that the caller would proceed without draw if the GrGpu doesn't request
+            one.
+            On output means that the GrGpu is not requesting a draw. */
+        kNoDraw_DrawPreference,
+        /** Means that the client would prefer a draw for performance of the readback but
+            can satisfy a straight readPixels call on the inputs without an intermediate draw.
+            getReadPixelsInfo will never set the draw preference to this value but may leave
+            it set. */
+        kCallerPrefersDraw_DrawPreference,
+        /** On output means that GrGpu would prefer a draw for performance of the readback but
+            can satisfy a straight readPixels call on the inputs without an intermediate draw. The
+            caller of getReadPixelsInfo should never specify this on intput. */
+        kGpuPrefersDraw_DrawPreference,
+        /** On input means that the caller requires a draw to do a transformation and there is no
+            CPU fallback.
+            On output means that GrGpu can only satisfy the readPixels request if the intermediate
+            draw is performed.
+          */
+        kRequireDraw_DrawPreference
+    };
+
+    /** Used to negotiates whether and how an intermediate draw should or must be performed before
+        a readPixels call. If this returns false then GrGpu could not deduce an intermediate draw
+        that would allow a successful readPixels call. */
+    virtual bool getReadPixelsInfo(GrSurface* srcSurface, int readWidth, int readHeight,
+                                   size_t rowBytes, GrPixelConfig readConfig, DrawPreference*,
+                                   ReadPixelTempDrawInfo *) = 0;
+
     /**
-     * Gets a preferred 8888 config to use for writing/reading pixel data to/from a surface with
+     * Gets a preferred 8888 config to use for writing pixel data to a surface with
      * config surfaceConfig. The returned config must have at least as many bits per channel as the
-     * readConfig or writeConfig param.
+     * writeConfig param.
      */
-    virtual GrPixelConfig preferredReadPixelsConfig(GrPixelConfig readConfig,
-                                                    GrPixelConfig surfaceConfig) const {
-        return readConfig;
-    }
     virtual GrPixelConfig preferredWritePixelsConfig(GrPixelConfig writeConfig,
                                                      GrPixelConfig surfaceConfig) const {
         return writeConfig;
@@ -154,35 +197,6 @@ public:
     virtual bool canWriteTexturePixels(const GrTexture*, GrPixelConfig srcConfig) const = 0;
 
     /**
-     * OpenGL's readPixels returns the result bottom-to-top while the skia
-     * API is top-to-bottom. Thus we have to do a y-axis flip. The obvious
-     * solution is to have the subclass do the flip using either the CPU or GPU.
-     * However, the caller (GrContext) may have transformations to apply and can
-     * simply fold in the y-flip for free. On the other hand, the subclass may
-     * be able to do it for free itself. For example, the subclass may have to
-     * do memcpys to handle rowBytes that aren't tight. It could do the y-flip
-     * concurrently.
-     *
-     * This function returns true if a y-flip is required to put the pixels in
-     * top-to-bottom order and the subclass cannot do it for free.
-     *
-     * See read pixels for the params
-     * @return true if calling readPixels with the same set of params will
-     *              produce bottom-to-top data
-     */
-     virtual bool readPixelsWillPayForYFlip(GrRenderTarget* renderTarget,
-                                            int left, int top,
-                                            int width, int height,
-                                            GrPixelConfig config,
-                                            size_t rowBytes) const = 0;
-     /**
-      * This should return true if reading a NxM rectangle of pixels from a
-      * render target is faster if the target has dimensons N and M and the read
-      * rectangle has its top-left at 0,0.
-      */
-     virtual bool fullReadPixelsIsFasterThanPartial() const { return false; };
-
-    /**
      * Reads a rectangle of pixels from a render target.
      *
      * @param renderTarget  the render target to read from. NULL means the
diff --git a/src/gpu/GrTest.cpp b/src/gpu/GrTest.cpp
index 34d75a84f5..b2fd99f6c3 100644
--- a/src/gpu/GrTest.cpp
+++ b/src/gpu/GrTest.cpp
@@ -150,11 +150,10 @@ public:
         return true;
     }
 
-    bool readPixelsWillPayForYFlip(GrRenderTarget* renderTarget,
-                                   int left, int top,
-                                   int width, int height,
-                                   GrPixelConfig config,
-                                   size_t rowBytes) const override { return false; }
+    bool getReadPixelsInfo(GrSurface* srcSurface, int readWidth, int readHeight, size_t rowBytes,
+                           GrPixelConfig readConfig, DrawPreference*,
+                           ReadPixelTempDrawInfo*) override { return false; }
+
     void buildProgramDesc(GrProgramDesc*,const GrPrimitiveProcessor&,
                           const GrPipeline&,
                           const GrBatchTracker&) const override {}
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 784a62fa27..e8dc788415 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -267,28 +267,6 @@ void GrGLGpu::contextAbandoned() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-GrPixelConfig GrGLGpu::preferredReadPixelsConfig(GrPixelConfig readConfig,
-                                                 GrPixelConfig surfaceConfig) const {
-    if (GR_GL_RGBA_8888_PIXEL_OPS_SLOW && kRGBA_8888_GrPixelConfig == readConfig) {
-        return kBGRA_8888_GrPixelConfig;
-    } else if (kMesa_GrGLDriver == this->glContext().driver() &&
-               GrBytesPerPixel(readConfig) == 4 &&
-               GrPixelConfigSwapRAndB(readConfig) == surfaceConfig) {
-        // Mesa 3D takes a slow path on when reading back  BGRA from an RGBA surface and vice-versa.
-        // Perhaps this should be guarded by some compiletime or runtime check.
-        return surfaceConfig;
-    } else if (readConfig == kBGRA_8888_GrPixelConfig
-            && !this->glCaps().readPixelsSupported(
-                this->glInterface(),
-                GR_GL_BGRA,
-                GR_GL_UNSIGNED_BYTE,
-                surfaceConfig
-            )) {
-        return kRGBA_8888_GrPixelConfig;
-    } else {
-        return readConfig;
-    }
-}
 
 GrPixelConfig GrGLGpu::preferredWritePixelsConfig(GrPixelConfig writeConfig,
                                                   GrPixelConfig surfaceConfig) const {
@@ -322,10 +300,6 @@ bool GrGLGpu::canWriteTexturePixels(const GrTexture* texture, GrPixelConfig srcC
     }
 }
 
-bool GrGLGpu::fullReadPixelsIsFasterThanPartial() const {
-    return SkToBool(GR_GL_FULL_READPIXELS_FASTER_THAN_PARTIAL);
-}
-
 void GrGLGpu::onResetContext(uint32_t resetBits) {
     // we don't use the zb at all
     if (resetBits & kMisc_GrGLBackendState) {
@@ -1651,7 +1625,6 @@ void GrGLGpu::discard(GrRenderTarget* renderTarget) {
     renderTarget->flagAsResolved();
 }
 
-
 void GrGLGpu::clearStencil(GrRenderTarget* target) {
     if (NULL == target) {
         return;
@@ -1705,35 +1678,91 @@ void GrGLGpu::onClearStencilClip(GrRenderTarget* target, const SkIRect& rect, bo
     fHWStencilSettings.invalidate();
 }
 
-bool GrGLGpu::readPixelsWillPayForYFlip(GrRenderTarget* renderTarget,
-                                        int left, int top,
-                                        int width, int height,
-                                        GrPixelConfig config,
-                                        size_t rowBytes) const {
-    // If this rendertarget is aready TopLeft, we don't need to flip.
+static bool read_pixels_pays_for_y_flip(GrRenderTarget* renderTarget, const GrGLCaps& caps,
+                                        int width, int height,  GrPixelConfig config,
+                                        size_t rowBytes) {
+    // If this render target is already TopLeft, we don't need to flip.
     if (kTopLeft_GrSurfaceOrigin == renderTarget->origin()) {
         return false;
     }
 
     // if GL can do the flip then we'll never pay for it.
-    if (this->glCaps().packFlipYSupport()) {
+    if (caps.packFlipYSupport()) {
         return false;
     }
 
     // If we have to do memcpy to handle non-trim rowBytes then we
     // get the flip for free. Otherwise it costs.
-    if (this->glCaps().packRowLengthSupport()) {
-        return true;
+    // Note that we're assuming that 0 rowBytes has already been handled and that the width has been
+    // clipped.
+    return caps.packRowLengthSupport() || GrBytesPerPixel(config) * width == rowBytes;
+}
+
+void elevate_draw_preference(GrGpu::DrawPreference* preference, GrGpu::DrawPreference elevation) {
+    GR_STATIC_ASSERT(GrGpu::kCallerPrefersDraw_DrawPreference > GrGpu::kNoDraw_DrawPreference);
+    GR_STATIC_ASSERT(GrGpu::kGpuPrefersDraw_DrawPreference >
+                     GrGpu::kCallerPrefersDraw_DrawPreference);
+    GR_STATIC_ASSERT(GrGpu::kRequireDraw_DrawPreference > GrGpu::kGpuPrefersDraw_DrawPreference);
+    *preference = SkTMax(*preference, elevation);
+}
+
+bool GrGLGpu::getReadPixelsInfo(GrSurface* srcSurface, int width, int height, size_t rowBytes,
+                                GrPixelConfig readConfig, DrawPreference* drawPreference,
+                                ReadPixelTempDrawInfo* tempDrawInfo) {
+    SkASSERT(drawPreference);
+    SkASSERT(tempDrawInfo);
+    SkASSERT(kGpuPrefersDraw_DrawPreference != *drawPreference);
+
+    if (GrPixelConfigIsCompressed(readConfig)) {
+        return false;
     }
-    // If we have to do memcpys to handle rowBytes then y-flip is free
-    // Note the rowBytes might be tight to the passed in data, but if data
-    // gets clipped in x to the target the rowBytes will no longer be tight.
-    if (left >= 0 && (left + width) < renderTarget->width()) {
-           return 0 == rowBytes ||
-                  GrBytesPerPixel(config) * width == rowBytes;
-    } else {
+
+    tempDrawInfo->fSwapRAndB = false;
+
+    // These settings we will always want if a temp draw is performed. The config is set below
+    // depending on whether we want to do a R/B swap or not.
+    tempDrawInfo->fTempSurfaceDesc.fFlags = kRenderTarget_GrSurfaceFlag;
+    tempDrawInfo->fTempSurfaceDesc.fWidth = width;
+    tempDrawInfo->fTempSurfaceDesc.fHeight = height;
+    tempDrawInfo->fTempSurfaceDesc.fSampleCnt = 0;
+    tempDrawInfo->fTempSurfaceDesc.fOrigin = kTopLeft_GrSurfaceOrigin; // no CPU y-flip for TL.
+    tempDrawInfo->fUseExactScratch = SkToBool(GR_GL_FULL_READPIXELS_FASTER_THAN_PARTIAL);
+
+    // Start off assuming that any temp draw should be to the readConfig, then check if that will
+    // be inefficient.
+    GrPixelConfig srcConfig = srcSurface->config();
+    tempDrawInfo->fTempSurfaceDesc.fConfig = readConfig;
+
+    if (GR_GL_RGBA_8888_PIXEL_OPS_SLOW && kRGBA_8888_GrPixelConfig == readConfig) {
+        tempDrawInfo->fTempSurfaceDesc.fConfig = kBGRA_8888_GrPixelConfig;
+    } else if (kMesa_GrGLDriver == this->glContext().driver() &&
+               GrBytesPerPixel(readConfig) == 4 &&
+               GrPixelConfigSwapRAndB(readConfig) == srcConfig) {
+        // Mesa 3D takes a slow path on when reading back  BGRA from an RGBA surface and vice-versa.
+        // Better to do a draw with a R/B swap and then read as the original config.
+        tempDrawInfo->fTempSurfaceDesc.fConfig = srcConfig;
+        tempDrawInfo->fSwapRAndB = true;
+        elevate_draw_preference(drawPreference, kGpuPrefersDraw_DrawPreference);
+    } else if (readConfig == kBGRA_8888_GrPixelConfig &&
+               !this->glCaps().readPixelsSupported(this->glInterface(), GR_GL_BGRA,
+                                                   GR_GL_UNSIGNED_BYTE, srcConfig)) {
+        tempDrawInfo->fTempSurfaceDesc.fConfig = kRGBA_8888_GrPixelConfig;
+        tempDrawInfo->fSwapRAndB = true;
+        elevate_draw_preference(drawPreference, kRequireDraw_DrawPreference);
+    }
+
+    GrRenderTarget* srcAsRT = srcSurface->asRenderTarget();
+    if (!srcAsRT) {
+        elevate_draw_preference(drawPreference, kRequireDraw_DrawPreference);
+    } else if (read_pixels_pays_for_y_flip(srcAsRT, this->glCaps(), width, height, readConfig,
+                                           rowBytes)) {
+        elevate_draw_preference(drawPreference, kGpuPrefersDraw_DrawPreference);
+    }
+
+    if (kRequireDraw_DrawPreference == *drawPreference && !srcSurface->asTexture()) {
         return false;
     }
+    return true;
 }
 
 bool GrGLGpu::onReadPixels(GrRenderTarget* target,
@@ -1742,6 +1771,8 @@ bool GrGLGpu::onReadPixels(GrRenderTarget* target,
                            GrPixelConfig config,
                            void* buffer,
                            size_t rowBytes) {
+    SkASSERT(target);
+
     // We cannot read pixels into a compressed buffer
     if (GrPixelConfigIsCompressed(config)) {
         return false;
diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h
index 45337ddb3c..066a237e59 100644
--- a/src/gpu/gl/GrGLGpu.h
+++ b/src/gpu/gl/GrGLGpu.h
@@ -57,18 +57,15 @@ public:
     // Used by GrGLProgram to configure OpenGL state.
     void bindTexture(int unitIdx, const GrTextureParams& params, GrGLTexture* texture);
 
+    bool getReadPixelsInfo(GrSurface* srcSurface, int readWidth, int readHeight, size_t rowBytes,
+                           GrPixelConfig readConfig, DrawPreference*,
+                           ReadPixelTempDrawInfo*) override;
+
+
     // GrGpu overrides
-    GrPixelConfig preferredReadPixelsConfig(GrPixelConfig readConfig,
-                                            GrPixelConfig surfaceConfig) const override;
     GrPixelConfig preferredWritePixelsConfig(GrPixelConfig writeConfig,
                                              GrPixelConfig surfaceConfig) const override;
     bool canWriteTexturePixels(const GrTexture*, GrPixelConfig srcConfig) const override;
-    bool readPixelsWillPayForYFlip(GrRenderTarget* renderTarget,
-                                   int left, int top,
-                                   int width, int height,
-                                   GrPixelConfig config,
-                                   size_t rowBytes) const override;
-    bool fullReadPixelsIsFasterThanPartial() const override;
 
     bool initCopySurfaceDstDesc(const GrSurface* src, GrSurfaceDesc* desc) const override;