Revert of Make copySurface work for texture dsts, return a bool, & add unit test. (patchset #6 id:100001 of https://codereview.chromium.org/1684313002/ )

Reason for revert:
Copy surface tests are not happy for Windows, Android and probably others:
https://uberchromegw.corp.google.com/i/client.skia.android/builders/Test-Android-GCC-Nexus7-GPU-Tegra3-Arm7-Release/builds/4161
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-ShuttleA-GPU-GTX960-x86_64-Release/builds/3694

Original issue's description:
> Make copySurface work for texture dsts, return a bool, & add unit test.
> GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1684313002
>
> Committed: https://skia.googlesource.com/skia/+/7ea5e28065e5eb797e95f5d81c1a65cf3209d741

TBR=robertphillips@google.com,bsalomon@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

11 files changed, 51 insertions, 224 deletions

diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h
index 4245b7fa94..00a92836c5 100644
--- a/include/gpu/GrContext.h
+++ b/include/gpu/GrContext.h
@@ -278,17 +278,24 @@ public:
      * @param src           the surface to copy from.
      * @param srcRect       the rectangle of the src that should be copied.
      * @param dstPoint      the translation applied when writing the srcRect's pixels to the dst.
+     * @param pixelOpsFlags see PixelOpsFlags enum above. (kUnpremul_PixelOpsFlag is not allowed).
      */
-    bool copySurface(GrSurface* dst,
+    void copySurface(GrSurface* dst,
                      GrSurface* src,
                      const SkIRect& srcRect,
-                     const SkIPoint& dstPoint);
+                     const SkIPoint& dstPoint,
+                     uint32_t pixelOpsFlags = 0);
 
     /** Helper that copies the whole surface but fails when the two surfaces are not identically
         sized. */
     bool copySurface(GrSurface* dst, GrSurface* src) {
-        return this->copySurface(dst, src, SkIRect::MakeWH(dst->width(), dst->height()),
-                                 SkIPoint::Make(0,0));
+        if (NULL == dst || NULL == src || dst->width() != src->width() ||
+            dst->height() != src->height()) {
+            return false;
+        }
+        this->copySurface(dst, src, SkIRect::MakeWH(dst->width(), dst->height()),
+                          SkIPoint::Make(0,0));
+        return true;
     }
 
     /**
diff --git a/include/gpu/GrDrawContext.h b/include/gpu/GrDrawContext.h
index 4c884154e3..24e38b8494 100644
--- a/include/gpu/GrDrawContext.h
+++ b/include/gpu/GrDrawContext.h
@@ -47,7 +47,7 @@ class SK_API GrDrawContext : public SkRefCnt {
 public:
     ~GrDrawContext() override;
 
-    bool copySurface(GrSurface* src, const SkIRect& srcRect, const SkIPoint& dstPoint);
+    void copySurface(GrSurface* src, const SkIRect& srcRect, const SkIPoint& dstPoint);
 
     // TODO: it is odd that we need both the SkPaint in the following 3 methods.
     // We should extract the text parameters from SkPaint and pass them separately
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 296dfb9e70..187a3ca37a 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -18,7 +18,6 @@
 #include "SkConfig8888.h"
 #include "SkGrPriv.h"
 
-#include "batches/GrCopySurfaceBatch.h"
 #include "effects/GrConfigConversionEffect.h"
 #include "text/GrTextBlobCache.h"
 
@@ -510,42 +509,34 @@ void GrContext::prepareSurfaceForExternalIO(GrSurface* surface) {
     }
 }
 
-bool GrContext::copySurface(GrSurface* dst, GrSurface* src, const SkIRect& srcRect,
-                            const SkIPoint& dstPoint) {
+void GrContext::copySurface(GrSurface* dst, GrSurface* src, const SkIRect& srcRect,
+                            const SkIPoint& dstPoint, uint32_t pixelOpsFlags) {
     ASSERT_SINGLE_OWNER
-    RETURN_FALSE_IF_ABANDONED
+    RETURN_IF_ABANDONED
     GR_AUDIT_TRAIL_AUTO_FRAME(&fAuditTrail, "GrContext::copySurface");
 
     if (!src || !dst) {
-        return false;
+        return;
     }
     ASSERT_OWNED_RESOURCE(src);
     ASSERT_OWNED_RESOURCE(dst);
 
+    // Since we're going to the draw target and not GPU, no need to check kNoFlush
+    // here.
     if (!dst->asRenderTarget()) {
-        SkIRect clippedSrcRect;
-        SkIPoint clippedDstPoint;
-        if (!GrCopySurfaceBatch::ClipSrcRectAndDstPoint(dst, src, srcRect, dstPoint,
-                                                        &clippedSrcRect, &clippedDstPoint)) {
-            return false;
-        }
-        // If we don't have an RT for the dst then we won't have a GrDrawContext to insert the
-        // the copy surface into. In the future we plan to have a more limited Context type
-        // (GrCopyContext?) that has the subset of GrDrawContext operations that should be
-        // allowed on textures that aren't render targets.
-        // For now we just flush any writes to the src and issue an immediate copy to the dst.
-        src->flushWrites();
-        return fGpu->copySurface(dst, src, clippedSrcRect, clippedDstPoint);
+        return;
     }
+
     SkAutoTUnref<GrDrawContext> drawContext(this->drawContext(dst->asRenderTarget()));
     if (!drawContext) {
-        return false;
+        return;
     }
 
-    if (!drawContext->copySurface(src, srcRect, dstPoint)) {
-        return false;
+    drawContext->copySurface(src, srcRect, dstPoint);
+
+    if (kFlushWrites_PixelOp & pixelOpsFlags) {
+        this->flush();
     }
-    return true;
 }
 
 void GrContext::flushSurfaceWrites(GrSurface* surface) {
diff --git a/src/gpu/GrDrawContext.cpp b/src/gpu/GrDrawContext.cpp
index d971f58ece..f37c48c9f3 100644
--- a/src/gpu/GrDrawContext.cpp
+++ b/src/gpu/GrDrawContext.cpp
@@ -97,13 +97,13 @@ GrDrawTarget* GrDrawContext::getDrawTarget() {
     return fDrawTarget;
 }
 
-bool GrDrawContext::copySurface(GrSurface* src, const SkIRect& srcRect, const SkIPoint& dstPoint) {
+void GrDrawContext::copySurface(GrSurface* src, const SkIRect& srcRect, const SkIPoint& dstPoint) {
     ASSERT_SINGLE_OWNER
-    RETURN_FALSE_IF_ABANDONED
+    RETURN_IF_ABANDONED
     SkDEBUGCODE(this->validate();)
     GR_AUDIT_TRAIL_AUTO_FRAME(fAuditTrail, "GrDrawContext::copySurface");
 
-    return this->getDrawTarget()->copySurface(fRenderTarget, src, srcRect, dstPoint);
+    this->getDrawTarget()->copySurface(fRenderTarget, src, srcRect, dstPoint);
 }
 
 void GrDrawContext::drawText(const GrClip& clip, const GrPaint& grPaint,
diff --git a/src/gpu/GrDrawTarget.cpp b/src/gpu/GrDrawTarget.cpp
index 9f15c11508..b9dc794526 100644
--- a/src/gpu/GrDrawTarget.cpp
+++ b/src/gpu/GrDrawTarget.cpp
@@ -406,21 +406,19 @@ void GrDrawTarget::discard(GrRenderTarget* renderTarget) {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-bool GrDrawTarget::copySurface(GrSurface* dst,
+void GrDrawTarget::copySurface(GrSurface* dst,
                                GrSurface* src,
                                const SkIRect& srcRect,
                                const SkIPoint& dstPoint) {
     GrBatch* batch = GrCopySurfaceBatch::Create(dst, src, srcRect, dstPoint);
-    if (!batch) {
-        return false;
-    }
+    if (batch) {
 #ifdef ENABLE_MDB
-    this->addDependency(src);
+        this->addDependency(src);
 #endif
 
-    this->recordBatch(batch);
-    batch->unref();
-    return true;
+        this->recordBatch(batch);
+        batch->unref();
+    }
 }
 
 template <class Left, class Right> static bool intersect(const Left& a, const Right& b) {
diff --git a/src/gpu/GrDrawTarget.h b/src/gpu/GrDrawTarget.h
index a850efd842..55c11da667 100644
--- a/src/gpu/GrDrawTarget.h
+++ b/src/gpu/GrDrawTarget.h
@@ -144,7 +144,7 @@ public:
      * depending on the type of surface, configs, etc, and the backend-specific
      * limitations.
      */
-    bool copySurface(GrSurface* dst,
+    void copySurface(GrSurface* dst,
                      GrSurface* src,
                      const SkIRect& srcRect,
                      const SkIPoint& dstPoint);
diff --git a/src/gpu/SkGrPixelRef.cpp b/src/gpu/SkGrPixelRef.cpp
index e48cbf5d15..3876f17411 100644
--- a/src/gpu/SkGrPixelRef.cpp
+++ b/src/gpu/SkGrPixelRef.cpp
@@ -84,10 +84,10 @@ static SkGrPixelRef* copy_to_new_texture_pixelref(GrTexture* texture, SkColorTyp
     }
 
     // Blink is relying on the above copy being sent to GL immediately in the case when the source
-    // is a WebGL canvas backing store. We could have a TODO to remove this flush, but we have
+    // is a WebGL canvas backing store. We could have a TODO to remove this flush flag, but we have
     // a larger TODO to remove SkGrPixelRef entirely.
-    context->copySurface(dst, texture, srcRect, SkIPoint::Make(0,0));
-    context->flushSurfaceWrites(dst);
+    context->copySurface(dst->asRenderTarget(), texture, srcRect, SkIPoint::Make(0,0),
+                         GrContext::kFlushWrites_PixelOp);
 
     SkImageInfo info = SkImageInfo::Make(desc.fWidth, desc.fHeight, dstCT, kPremul_SkAlphaType,
                                          dstPT);
diff --git a/src/gpu/batches/GrCopySurfaceBatch.cpp b/src/gpu/batches/GrCopySurfaceBatch.cpp
index a59ed38f51..098f7c7704 100644
--- a/src/gpu/batches/GrCopySurfaceBatch.cpp
+++ b/src/gpu/batches/GrCopySurfaceBatch.cpp
@@ -9,12 +9,12 @@
 #include "GrCopySurfaceBatch.h"
 
 // returns true if the read/written rect intersects the src/dst and false if not.
-bool GrCopySurfaceBatch::ClipSrcRectAndDstPoint(const GrSurface* dst,
-                                                const GrSurface* src,
-                                                const SkIRect& srcRect,
-                                                const SkIPoint& dstPoint,
-                                                SkIRect* clippedSrcRect,
-                                                SkIPoint* clippedDstPoint) {
+static bool clip_srcrect_and_dstpoint(const GrSurface* dst,
+                                      const GrSurface* src,
+                                      const SkIRect& srcRect,
+                                      const SkIPoint& dstPoint,
+                                      SkIRect* clippedSrcRect,
+                                      SkIPoint* clippedDstPoint) {
     *clippedSrcRect = srcRect;
     *clippedDstPoint = dstPoint;
 
@@ -67,7 +67,12 @@ GrBatch* GrCopySurfaceBatch::Create(GrSurface* dst, GrSurface* src, const SkIRec
     SkIRect clippedSrcRect;
     SkIPoint clippedDstPoint;
     // If the rect is outside the src or dst then we've already succeeded.
-    if (!ClipSrcRectAndDstPoint(dst, src, srcRect, dstPoint, &clippedSrcRect, &clippedDstPoint)) {
+    if (!clip_srcrect_and_dstpoint(dst,
+                                    src,
+                                    srcRect,
+                                    dstPoint,
+                                    &clippedSrcRect,
+                                    &clippedDstPoint)) {
         return nullptr;
     }
     return new GrCopySurfaceBatch(dst, src, clippedSrcRect, clippedDstPoint);
diff --git a/src/gpu/batches/GrCopySurfaceBatch.h b/src/gpu/batches/GrCopySurfaceBatch.h
index 3926643f8a..7bf8d8d8c2 100644
--- a/src/gpu/batches/GrCopySurfaceBatch.h
+++ b/src/gpu/batches/GrCopySurfaceBatch.h
@@ -17,16 +17,6 @@ class GrCopySurfaceBatch final : public GrBatch {
 public:
     DEFINE_BATCH_CLASS_ID
 
-    /** This should not really be exposed as Create() will apply this clipping, but there is
-     *  currently a workaround in GrContext::copySurface() for non-render target dsts that relies
-     *  on it. */
-    static bool ClipSrcRectAndDstPoint(const GrSurface* dst,
-                                       const GrSurface* src,
-                                       const SkIRect& srcRect,
-                                       const SkIPoint& dstPoint,
-                                       SkIRect* clippedSrcRect,
-                                       SkIPoint* clippedDstPoint);
-
     static GrBatch* Create(GrSurface* dst, GrSurface* src, const SkIRect& srcRect,
                            const SkIPoint& dstPoint);
 
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index c502fc0092..04939e36f1 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -326,8 +326,7 @@ GrTexture* GrDeepCopyTexture(GrTexture* src, bool budgeted) {
 
     const SkIRect srcR = SkIRect::MakeWH(desc.fWidth, desc.fHeight);
     const SkIPoint dstP = SkIPoint::Make(0, 0);
-    ctx->copySurface(dst, src, srcR, dstP);
-    ctx->flushSurfaceWrites(dst);
+    ctx->copySurface(dst, src, srcR, dstP, GrContext::kFlushWrites_PixelOp);
     return dst;
 }
 
diff --git a/tests/CopySurfaceTest.cpp b/tests/CopySurfaceTest.cpp
deleted file mode 100644
index 4b64537143..0000000000
--- a/tests/CopySurfaceTest.cpp
+++ /dev/null
@@ -1,163 +0,0 @@
-/*
- * Copyright 2016 Google Inc.
- *
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-#include <initializer_list>
-#include "Test.h"
-
-#if SK_SUPPORT_GPU
-#include "GrContext.h"
-#include "GrTexture.h"
-#include "GrTextureProvider.h"
-
-#include "SkUtils.h"
-
-DEF_GPUTEST_FOR_RENDERING_CONTEXTS(CopySurface, reporter, context) {
-    static const int kW = 10;
-    static const int kH = 10;
-    static const size_t kRowBytes = sizeof(uint32_t) * kW;
-
-    GrSurfaceDesc baseDesc;
-    baseDesc.fConfig = kRGBA_8888_GrPixelConfig;
-    baseDesc.fWidth = kW;
-    baseDesc.fHeight = kH;
-
-    SkAutoTMalloc<uint32_t> srcPixels(kW * kH);
-    for (int i = 0; i < kW * kH; ++i) {
-        srcPixels.get()[i] = i;
-    }
-
-    SkAutoTMalloc<uint32_t> dstPixels(kW * kH);
-    for (int i = 0; i < kW * kH; ++i) {
-        dstPixels.get()[i] = ~i;
-    }
-
-    static const SkIRect kSrcRects[] {
-        { 0,  0, kW  , kH  },
-        {-1, -1, kW+1, kH+1},
-        { 1,  1, kW-1, kH-1},
-        { 5,  5, 6   , 6   },
-    };
-
-    static const SkIPoint kDstPoints[] {
-        { 0   ,  0   },
-        { 1   ,  1   },
-        { kW/2,  kH/4},
-        { kW-1,  kH-1},
-        { kW  ,  kH  },
-        { kW+1,  kH+2},
-        {-1   , -1   },
-    };
-
-    SkAutoTMalloc<uint32_t> read(kW * kH);
-
-    for (auto sOrigin : {kBottomLeft_GrSurfaceOrigin, kTopLeft_GrSurfaceOrigin}) {
-        for (auto dOrigin : {kBottomLeft_GrSurfaceOrigin, kTopLeft_GrSurfaceOrigin}) {
-            for (auto sFlags: {kRenderTarget_GrSurfaceFlag, kNone_GrSurfaceFlags}) {
-                for (auto dFlags: {kRenderTarget_GrSurfaceFlag, kNone_GrSurfaceFlags}) {
-                    for (auto srcRect : kSrcRects) {
-                        for (auto dstPoint : kDstPoints) {
-                            GrSurfaceDesc srcDesc = baseDesc;
-                            srcDesc.fOrigin = sOrigin;
-                            srcDesc.fFlags = sFlags;
-                            GrSurfaceDesc dstDesc = baseDesc;
-                            dstDesc.fOrigin = dOrigin;
-                            dstDesc.fFlags = dFlags;
-
-                            SkAutoTUnref<GrTexture> src(
-                                context->textureProvider()->createTexture(srcDesc, false,
-                                                                          srcPixels.get(),
-                                                                          kRowBytes));
-                            SkAutoTUnref<GrTexture> dst(
-                                context->textureProvider()->createTexture(dstDesc, false,
-                                                                          dstPixels.get(),
-                                                                          kRowBytes));
-                            if (!src || !dst) {
-                                ERRORF(reporter,
-                                       "Could not create surfaces for copy surface test.");
-                                continue;
-                            }
-
-                            bool result = context->copySurface(dst, src, srcRect, dstPoint);
-
-                            bool expectedResult = true;
-                            SkIPoint dstOffset = { dstPoint.fX - srcRect.fLeft,
-                                                   dstPoint.fY - srcRect.fTop };
-                            SkIRect copiedDstRect = SkIRect::MakeXYWH(dstPoint.fX,
-                                                                      dstPoint.fY,
-                                                                      srcRect.width(),
-                                                                      srcRect.height());
-
-                            SkIRect copiedSrcRect;
-                            if (!copiedSrcRect.intersect(srcRect, SkIRect::MakeWH(kW, kH))) {
-                                expectedResult = false;
-                            } else {
-                                // If the src rect was clipped, apply same clipping to each side of
-                                // copied dst rect.
-                                copiedDstRect.fLeft += copiedSrcRect.fLeft - srcRect.fLeft;
-                                copiedDstRect.fTop += copiedSrcRect.fTop - srcRect.fTop;
-                                copiedDstRect.fRight -= copiedSrcRect.fRight - srcRect.fRight;
-                                copiedDstRect.fBottom -= copiedSrcRect.fBottom - srcRect.fBottom;
-                            }
-                            if (copiedDstRect.isEmpty() ||
-                                !copiedDstRect.intersect(SkIRect::MakeWH(kW, kH))) {
-                                expectedResult = false;
-                            }
-                            // To make the copied src rect correct we would apply any dst clipping
-                            // back to the src rect, but we don't use it again so don't bother.
-                            if (expectedResult != result) {
-                                ERRORF(reporter, "Expected return value %d from copySurface, got "
-                                       "%d.", expectedResult, result);
-                                continue;
-                            }
-
-                            if (!expectedResult || !result) {
-                                continue;
-                            }
-
-                            sk_memset32(read.get(), 0, kW * kH);
-                            if (!dst->readPixels(0, 0, kW, kH, baseDesc.fConfig, read.get(),
-                                                 kRowBytes)) {
-                                ERRORF(reporter, "Error calling readPixels");
-                                continue;
-                            }
-
-                            bool abort = false;
-                            // Validate that pixels inside copiedDstRect received the correct value
-                            // from src and that those outside were not modified.
-                            for (int y = 0; y < kH && !abort; ++y) {
-                                for (int x = 0; x < kW; ++x) {
-                                    uint32_t r = read.get()[y * kW + x];
-                                    if (copiedDstRect.contains(x, y)) {
-                                        int sx = x - dstOffset.fX;
-                                        int sy = y - dstOffset.fY;
-                                        uint32_t s = srcPixels.get()[sy * kW + sx];
-                                        if (s != r) {
-                                            ERRORF(reporter, "Expected dst %d,%d to contain "
-                                                   "0x%08x copied from src location %d,%d. Got "
-                                                   "0x%08x", x, y, s, sx, sy, r);
-                                            abort = true;
-                                            break;
-                                        }
-                                    } else {
-                                        uint32_t d = dstPixels.get()[y * kW + x];
-                                        if (d != r) {
-                                            ERRORF(reporter, "Expected dst %d,%d to be unmodified ("
-                                                   "0x%08x). Got 0x%08x", x, y, d, r);
-                                            abort = true;
-                                            break;
-                                        }
-                                    }
-                                }
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    }
-}
-#endif