Start on new GPU pixel ops implementation.

The new ops attempt to be less optimal by falling back to CPU conversions
rather than relying on intermediate draws and complex coordination between
GrContext and GrGpu to determine how conversions are performed.

This adds the new writePixels implementation.

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

11 files changed, 298 insertions, 79 deletions

diff --git a/BUILD.gn b/BUILD.gn
index 30af60093e..0db8b14097 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -60,6 +60,8 @@ declare_args() {
   skia_use_sfntly = skia_use_icu
   skia_enable_atlas_text = is_skia_dev_build && skia_enable_gpu
 
+  skia_use_legacy_gpu_pixel_ops = is_skia_dev_build && is_win  # Arbitrary to keep old code path tested until deletion.
+
   if (is_android) {
     skia_use_vulkan = defined(ndk_api) && ndk_api >= 24
   } else if (is_fuchsia) {
@@ -605,6 +607,9 @@ optional("gpu") {
       }
     }
   }
+  if (skia_use_legacy_gpu_pixel_ops) {
+    public_defines += [ "SK_LEGACY_GPU_PIXEL_OPS" ]
+  }
   if (skia_enable_spirv_validation) {
     deps += [ "//third_party/spirv-tools" ]
     public_defines += [ "SK_ENABLE_SPIRV_VALIDATION" ]
@@ -1251,13 +1256,13 @@ if (skia_enable_tools) {
       "tools/debugger/SkDrawCommand.cpp",
       "tools/debugger/SkJsonWriteBuffer.cpp",
       "tools/debugger/SkObjectParser.cpp",
+      "tools/fonts/SkRandomScalerContext.cpp",
+      "tools/fonts/SkTestFontMgr.cpp",
+      "tools/fonts/SkTestScalerContext.cpp",
+      "tools/fonts/sk_tool_utils_font.cpp",
       "tools/picture_utils.cpp",
       "tools/random_parse_path.cpp",
       "tools/sk_tool_utils.cpp",
-      "tools/fonts/sk_tool_utils_font.cpp",
-      "tools/fonts/SkRandomScalerContext.cpp",
-      "tools/fonts/SkTestScalerContext.cpp",
-      "tools/fonts/SkTestFontMgr.cpp",
       "tools/timer/Timer.cpp",
       "tools/trace/SkChromeTracingTracer.cpp",
       "tools/trace/SkChromeTracingTracer.h",
diff --git a/include/gpu/GrCaps.h b/include/gpu/GrCaps.h
index e8449bbcb4..de9bc73cf5 100644
--- a/include/gpu/GrCaps.h
+++ b/include/gpu/GrCaps.h
@@ -185,6 +185,23 @@ public:
         return this->getRenderTargetSampleCount(requestedCount, config);
     }
 
+    /**
+     * Some backends have restrictions on what types of render targets for which
+     * GrGpu::writePixels() will succeed. If this returns false then the caller should implement a
+     * fallback where a temporary texture is created, pixels are written to it, and then that is
+     * copied or drawn into the the render target.
+     */
+    virtual bool renderTargetWritePixelsSupported(bool isAlsoTexture, int sampleCnt) const = 0;
+
+    /**
+     * Given a dst pixel config and a src color type what color type must the caller coax the
+     * the data into in order to use GrGpu::writePixels().
+     */
+    virtual GrColorType supportedWritePixelsColorType(GrPixelConfig config,
+                                                      GrColorType /*srcColorType*/) const {
+        return GrPixelConfigToColorType(config);
+    }
+
     bool suppressPrints() const { return fSuppressPrints; }
 
     size_t bufferMapThreshold() const {
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 0af319715b..cc311b6e87 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -5,8 +5,8 @@
  * found in the LICENSE file.
  */
 
-#include "GrBackendSemaphore.h"
 #include "GrContext.h"
+#include "GrBackendSemaphore.h"
 #include "GrClip.h"
 #include "GrContextOptions.h"
 #include "GrContextPriv.h"
@@ -25,7 +25,6 @@
 #include "GrTexture.h"
 #include "GrTextureContext.h"
 #include "GrTracing.h"
-
 #include "SkConvertPixels.h"
 #include "SkDeferredDisplayList.h"
 #include "SkGr.h"
@@ -35,10 +34,9 @@
 #include "SkTaskGroup.h"
 #include "SkUnPreMultiplyPriv.h"
 #include "effects/GrConfigConversionEffect.h"
-#include "text/GrTextBlobCache.h"
-
 #include "gl/GrGLGpu.h"
 #include "mock/GrMockGpu.h"
+#include "text/GrTextBlobCache.h"
 #ifdef SK_METAL
 #include "mtl/GrMtlTrampoline.h"
 #endif
@@ -643,6 +641,11 @@ bool GrContextPriv::writeSurfacePixels(GrSurfaceContext* dst, int left, int top,
                                        int height, GrColorType srcColorType,
                                        SkColorSpace* srcColorSpace, const void* buffer,
                                        size_t rowBytes, uint32_t pixelOpsFlags) {
+#ifndef SK_LEGACY_GPU_PIXEL_OPS
+    return this->writeSurfacePixels2(dst, left, top, width, height, srcColorType, srcColorSpace,
+                                     buffer, rowBytes, pixelOpsFlags);
+#endif
+
     // TODO: Color space conversion
 
     ASSERT_SINGLE_OWNER_PRIV
@@ -934,6 +937,141 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src, int left, int top,
     return true;
 }
 
+bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top, int width,
+                                        int height, GrColorType srcColorType,
+                                        SkColorSpace* srcColorSpace, const void* buffer,
+                                        size_t rowBytes, uint32_t pixelOpsFlags) {
+    ASSERT_SINGLE_OWNER_PRIV
+    RETURN_FALSE_IF_ABANDONED_PRIV
+    SkASSERT(dst);
+    SkASSERT(buffer);
+    ASSERT_OWNED_PROXY_PRIV(dst->asSurfaceProxy());
+    GR_CREATE_TRACE_MARKER_CONTEXT("GrContextPriv", "writeSurfacePixels2", fContext);
+
+    if (GrColorType::kUnknown == srcColorType) {
+        return false;
+    }
+
+    if (!dst->asSurfaceProxy()->instantiate(this->resourceProvider())) {
+        return false;
+    }
+
+    GrSurfaceProxy* dstProxy = dst->asSurfaceProxy();
+    GrSurface* dstSurface = dstProxy->priv().peekSurface();
+
+    if (!GrSurfacePriv::AdjustWritePixelParams(dstSurface->width(), dstSurface->height(),
+                                               GrColorTypeBytesPerPixel(srcColorType), &left, &top,
+                                               &width, &height, &buffer, &rowBytes)) {
+        return false;
+    }
+
+    auto dstRTProxy = dstProxy->asRenderTargetProxy();
+    if (dstRTProxy &&
+        !fContext->caps()->renderTargetWritePixelsSupported(SkToBool(dstProxy->asTextureProxy()),
+                                                            dstRTProxy->numColorSamples())) {
+        GrSurfaceDesc desc;
+        desc.fConfig = dstProxy->config();
+        desc.fWidth = width;
+        desc.fHeight = height;
+        desc.fSampleCnt = 1;
+        desc.fOrigin = kTopLeft_GrSurfaceOrigin;
+        auto tempProxy =
+                this->proxyProvider()->createProxy(desc, SkBackingFit::kApprox, SkBudgeted::kYes);
+        if (!tempProxy) {
+            return false;
+        }
+        auto tempCtx = this->drawingManager()->makeTextureContext(
+                tempProxy, dst->colorSpaceInfo().refColorSpace());
+        if (!tempCtx) {
+            return false;
+        }
+        if (!this->writeSurfacePixels2(tempCtx.get(), 0, 0, width, height, srcColorType,
+                                       srcColorSpace, buffer, rowBytes, pixelOpsFlags)) {
+            return false;
+        }
+        GrPaint paint;
+        paint.setAllowSRGBInputs(true);
+        paint.addColorTextureProcessor(std::move(tempProxy), SkMatrix::I());
+        paint.setPorterDuffXPFactory(SkBlendMode::kSrc);
+        dst->asRenderTargetContext()->drawRect(GrNoClip(), std::move(paint), GrAA::kNo,
+                                               SkMatrix::MakeTrans(left, top),
+                                               SkRect::MakeIWH(width, height));
+        return true;
+    }
+
+    // TODO: Make GrSurfaceContext know its alpha type and pass src buffer's alpha type.
+    bool premul = SkToBool(kUnpremul_PixelOpsFlag & pixelOpsFlags);
+    bool convert = premul;
+
+    if (!valid_pixel_conversion(srcColorType, dstProxy->config(), premul)) {
+        return false;
+    }
+
+    GrColorType allowedColorType =
+            fContext->caps()->supportedWritePixelsColorType(dstProxy->config(), srcColorType);
+    convert = convert || (srcColorType != allowedColorType);
+
+    if (!dst->colorSpaceInfo().colorSpace()) {
+        // "Legacy" mode - no color space conversions.
+        srcColorSpace = nullptr;
+    }
+    convert = convert || !SkColorSpace::Equals(srcColorSpace, dst->colorSpaceInfo().colorSpace());
+
+    std::unique_ptr<char[]> tempBuffer;
+    if (convert) {
+        auto srcSkColorType = GrColorTypeToSkColorType(srcColorType);
+        auto dstSkColorType = GrColorTypeToSkColorType(allowedColorType);
+        if (kUnknown_SkColorType == srcSkColorType || kUnknown_SkColorType == dstSkColorType) {
+            return false;
+        }
+        auto srcAlphaType = premul ? kUnpremul_SkAlphaType : kPremul_SkAlphaType;
+        SkPixmap src(SkImageInfo::Make(width, height, srcSkColorType, srcAlphaType,
+                                       sk_ref_sp(srcColorSpace)),
+                     buffer, rowBytes);
+        auto tempSrcII = SkImageInfo::Make(width, height, dstSkColorType, kPremul_SkAlphaType,
+                                           dst->colorSpaceInfo().refColorSpace());
+        auto size = tempSrcII.computeMinByteSize();
+        if (!size) {
+            return false;
+        }
+        tempBuffer.reset(new char[size]);
+        SkPixmap tempSrc(tempSrcII, tempBuffer.get(), tempSrcII.minRowBytes());
+        if (!src.readPixels(tempSrc)) {
+            return false;
+        }
+        srcColorType = allowedColorType;
+        buffer = tempSrc.addr();
+        rowBytes = tempSrc.rowBytes();
+        if (dstProxy->origin() == kBottomLeft_GrSurfaceOrigin) {
+            std::unique_ptr<char[]> row(new char[rowBytes]);
+            for (int y = 0; y < height / 2; ++y) {
+                memcpy(row.get(), tempSrc.addr(0, y), rowBytes);
+                memcpy(tempSrc.writable_addr(0, y), tempSrc.addr(0, height - 1 - y), rowBytes);
+                memcpy(tempSrc.writable_addr(0, height - 1 - y), row.get(), rowBytes);
+            }
+            top = dstProxy->height() - top - height;
+        }
+    } else if (dstProxy->origin() == kBottomLeft_GrSurfaceOrigin) {
+        size_t trimRowBytes = GrColorTypeBytesPerPixel(srcColorType) * width;
+        tempBuffer.reset(new char[trimRowBytes * height]);
+        char* dst = reinterpret_cast<char*>(tempBuffer.get()) + trimRowBytes * (height - 1);
+        const char* src = reinterpret_cast<const char*>(buffer);
+        for (int i = 0; i < height; ++i, src += rowBytes, dst -= trimRowBytes) {
+            memcpy(dst, src, trimRowBytes);
+        }
+        buffer = tempBuffer.get();
+        rowBytes = trimRowBytes;
+        top = dstProxy->height() - top - height;
+    }
+
+    if (!(kDontFlush_PixelOpsFlag & pixelOpsFlags) && dstSurface->surfacePriv().hasPendingIO()) {
+        this->flush(nullptr);  // MDB TODO: tighten this
+    }
+
+    return this->getGpu()->writePixels(dstSurface, left, top, width, height, srcColorType, buffer,
+                                       rowBytes);
+}
+
 void GrContextPriv::prepareSurfaceForExternalIO(GrSurfaceProxy* proxy) {
     ASSERT_SINGLE_OWNER_PRIV
     RETURN_IF_ABANDONED_PRIV
diff --git a/src/gpu/GrContextPriv.h b/src/gpu/GrContextPriv.h
index a26d15d26d..a2a45b5007 100644
--- a/src/gpu/GrContextPriv.h
+++ b/src/gpu/GrContextPriv.h
@@ -147,13 +147,16 @@ public:
                            size_t rowBytes = 0, uint32_t pixelOpsFlags = 0);
 
     /**
-     * Writes a rectangle of pixels to a surface.
+     * Writes a rectangle of pixels to a surface. There are currently two versions of this.
+     * writeSurfacePixels() is the older version which will be replaced by the more robust and
+     * maintainable (but perhaps slower) writeSurfacePixels2().
+     *
      * @param dst           the surface context to write to.
      * @param left          left edge of the rectangle to write (inclusive)
      * @param top           top edge of the rectangle to write (inclusive)
      * @param width         width of rectangle to write in pixels.
      * @param height        height of rectangle to write in pixels.
-     * @param srcConfig     the pixel config of the source buffer
+     * @param srcColorType  the color type of the source buffer
      * @param srcColorSpace color space of the source buffer
      * @param buffer        memory to read pixels from
      * @param rowBytes      number of bytes between consecutive rows. Zero
@@ -165,6 +168,9 @@ public:
     bool writeSurfacePixels(GrSurfaceContext* dst, int left, int top, int width, int height,
                             GrColorType srcColorType, SkColorSpace* srcColorSpace,
                             const void* buffer, size_t rowBytes, uint32_t pixelOpsFlags = 0);
+    bool writeSurfacePixels2(GrSurfaceContext* dst, int left, int top, int width, int height,
+                             GrColorType srcColorType, SkColorSpace* srcColorSpace,
+                             const void* buffer, size_t rowBytes, uint32_t pixelOpsFlags = 0);
 
     GrBackend getBackend() const { return fContext->fBackend; }
 
diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h
index 96691e3322..7c3b796a52 100644
--- a/src/gpu/GrGpu.h
+++ b/src/gpu/GrGpu.h
@@ -297,6 +297,16 @@ public:
                      GrColorType, const void* buffer, size_t rowBytes);
 
     /**
+     * This version of writePixels doesn't take an origin. TODO: Remove origin handling from
+     * GrGpu::writePixels entirely.
+     */
+    bool writePixels(GrSurface* surface, int left, int top, int width, int height,
+                     GrColorType srcColorType, const void* buffer, size_t rowBytes) {
+        return this->writePixels(surface, kTopLeft_GrSurfaceOrigin, left, top, width, height,
+                                 srcColorType, buffer, rowBytes);
+    }
+
+    /**
      * Updates the pixels in a rectangle of a texture using a buffer
      *
      * There are a couple of assumptions here. First, we only update the top miplevel.
diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h
index 8bac68cd10..f7b4f68b49 100644
--- a/src/gpu/gl/GrGLCaps.h
+++ b/src/gpu/gl/GrGLCaps.h
@@ -316,6 +316,13 @@ public:
     /// Use indices or vertices in CPU arrays rather than VBOs for dynamic content.
     bool useNonVBOVertexAndIndexDynamicData() const { return fUseNonVBOVertexAndIndexDynamicData; }
 
+    bool renderTargetWritePixelsSupported(bool isAlsoTexture, int sampleCnt) const override {
+        if (sampleCnt > 1 && this->usesMSAARenderBuffers()) {
+            return false;
+        }
+        return isAlsoTexture;
+    }
+
     /// Does ReadPixels support reading readConfig pixels from a FBO that is surfaceConfig?
     bool readPixelsSupported(GrPixelConfig surfaceConfig,
                              GrPixelConfig readConfig,
diff --git a/src/gpu/mock/GrMockCaps.h b/src/gpu/mock/GrMockCaps.h
index e75a2c63ff..3ef404f1dc 100644
--- a/src/gpu/mock/GrMockCaps.h
+++ b/src/gpu/mock/GrMockCaps.h
@@ -67,6 +67,10 @@ public:
         return 0;
     }
 
+    bool renderTargetWritePixelsSupported(bool isAlsoTexture, int sampleCnt) const override {
+        return true;
+    }
+
     bool initDescForDstCopy(const GrRenderTargetProxy* src, GrSurfaceDesc* desc,
                             bool* rectsMustMatch, bool* disallowSubrect) const override {
         return false;
diff --git a/src/gpu/mtl/GrMtlCaps.h b/src/gpu/mtl/GrMtlCaps.h
index c23a9028ca..61eaffed64 100644
--- a/src/gpu/mtl/GrMtlCaps.h
+++ b/src/gpu/mtl/GrMtlCaps.h
@@ -31,6 +31,10 @@ public:
     int getRenderTargetSampleCount(int requestedCount, GrPixelConfig) const override;
     int maxRenderTargetSampleCount(GrPixelConfig) const override;
 
+    bool renderTargetWritePixelsSupported(bool isAlsoTexture, int sampleCnt) const override {
+        return true;
+    }
+
     bool isConfigCopyable(GrPixelConfig config) const override {
         return true;
     }
diff --git a/src/gpu/vk/GrVkCaps.h b/src/gpu/vk/GrVkCaps.h
index e4d107cf3e..55ed916807 100644
--- a/src/gpu/vk/GrVkCaps.h
+++ b/src/gpu/vk/GrVkCaps.h
@@ -40,6 +40,10 @@ public:
     int getRenderTargetSampleCount(int requestedCount, GrPixelConfig config) const override;
     int maxRenderTargetSampleCount(GrPixelConfig config) const override;
 
+    bool renderTargetWritePixelsSupported(bool isAlsoTexture, int sampleCnt) const override {
+        return sampleCnt <= 1 && isAlsoTexture;
+    }
+
     bool isConfigTexturableLinearly(GrPixelConfig config) const {
         return SkToBool(ConfigInfo::kTextureable_Flag & fConfigTable[config].fLinearFlags);
     }
diff --git a/tests/SRGBReadWritePixelsTest.cpp b/tests/SRGBReadWritePixelsTest.cpp
index 7ced2ebe97..8b7dd07a57 100644
--- a/tests/SRGBReadWritePixelsTest.cpp
+++ b/tests/SRGBReadWritePixelsTest.cpp
@@ -182,23 +182,21 @@ static const char* encoding_as_str(Encoding encoding) {
     return nullptr;
 }
 
-static void do_test(Encoding contextEncoding, Encoding writeEncoding, Encoding readEncoding,
-                    float error, CheckFn check, GrContext* context, skiatest::Reporter* reporter) {
-#if defined(SK_BUILD_FOR_GOOGLE3)
-    // Stack frame size is limited in SK_BUILD_FOR_GOOGLE3.
-    static const int kW = 63;
-    static const int kH = 63;
-#else
-    static const int kW = 255;
-    static const int kH = 255;
-#endif
-    uint32_t origData[kW * kH];
+static constexpr int kW = 255;
+static constexpr int kH = 255;
+
+static std::unique_ptr<uint32_t[]> make_data() {
+    std::unique_ptr<uint32_t[]> data(new uint32_t[kW * kH]);
     for (int j = 0; j < kH; ++j) {
         for (int i = 0; i < kW; ++i) {
-            origData[j * kW + i] = (j << 24) | (i << 16) | (i << 8) | i;
+            data[j * kW + i] = (j << 24) | (i << 16) | (i << 8) | i;
         }
     }
+    return data;
+}
 
+static sk_sp<GrSurfaceContext> make_surface_context(Encoding contextEncoding, GrContext* context,
+                                                    skiatest::Reporter* reporter) {
     GrSurfaceDesc desc;
     desc.fFlags = kRenderTarget_GrSurfaceFlag;
     desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
@@ -211,12 +209,38 @@ static void do_test(Encoding contextEncoding, Encoding writeEncoding, Encoding r
             encoding_as_color_space(contextEncoding));
     if (!surfaceContext) {
         ERRORF(reporter, "Could not create %s surface context.", encoding_as_str(contextEncoding));
+    }
+    return surfaceContext;
+}
+
+#ifndef SK_LEGACY_GPU_PIXEL_OPS
+static void text_write_fails(Encoding contextEncoding, Encoding writeEncoding, GrContext* context,
+                             skiatest::Reporter* reporter) {
+    auto surfaceContext = make_surface_context(contextEncoding, context, reporter);
+    if (!surfaceContext) {
         return;
     }
     auto writeII = SkImageInfo::Make(kW, kH, kRGBA_8888_SkColorType, kPremul_SkAlphaType,
                                      encoding_as_color_space(writeEncoding));
+    auto data = make_data();
+    if (surfaceContext->writePixels(writeII, data.get(), 0, 0, 0)) {
+        ERRORF(reporter, "Expected %s write to %s surface context to fail.",
+               encoding_as_str(writeEncoding), encoding_as_str(contextEncoding));
+    }
+}
+#endif
 
-    if (!surfaceContext->writePixels(writeII, origData, 0, 0, 0)) {
+static void test_write_read(Encoding contextEncoding, Encoding writeEncoding, Encoding readEncoding,
+                            float error, CheckFn check, GrContext* context,
+                            skiatest::Reporter* reporter) {
+    auto surfaceContext = make_surface_context(contextEncoding, context, reporter);
+    if (!surfaceContext) {
+        return;
+    }
+    auto writeII = SkImageInfo::Make(kW, kH, kRGBA_8888_SkColorType, kPremul_SkAlphaType,
+                                     encoding_as_color_space(writeEncoding));
+    auto data = make_data();
+    if (!surfaceContext->writePixels(writeII, data.get(), 0, 0, 0)) {
         ERRORF(reporter, "Could not write %s to %s surface context.",
                encoding_as_str(writeEncoding), encoding_as_str(contextEncoding));
         return;
@@ -227,7 +251,7 @@ static void do_test(Encoding contextEncoding, Encoding writeEncoding, Encoding r
     SkString testName;
     testName.printf("write %s data to a %s context and read as %s.", encoding_as_str(writeEncoding),
                     encoding_as_str(contextEncoding), encoding_as_str(readEncoding));
-    read_and_check_pixels(reporter, surfaceContext.get(), origData, readII, check, error,
+    read_and_check_pixels(reporter, surfaceContext.get(), data.get(), readII, check, error,
                           testName.c_str());
 }
 
@@ -250,94 +274,104 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) {
     // Write sRGB data to a sRGB context - no conversion on the write.
 
     // back to sRGB no conversion
-    do_test(Encoding::kSRGB, Encoding::kSRGB, Encoding::kSRGB, smallError, check_no_conversion,
-            context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kSRGB, smallError,
+                    check_no_conversion, context, reporter);
     // Untagged read from sRGB is treated as a conversion back to linear. TODO: Fail or don't
     // convert?
-    do_test(Encoding::kSRGB, Encoding::kSRGB, Encoding::kUntagged, error,
-            check_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kUntagged, error,
+                    check_srgb_to_linear_conversion, context, reporter);
     // Converts back to linear
-    do_test(Encoding::kSRGB, Encoding::kSRGB, Encoding::kLinear, error,
-            check_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kLinear, error,
+                    check_srgb_to_linear_conversion, context, reporter);
 
+#ifdef SK_LEGACY_GPU_PIXEL_OPS
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write untagged data to a sRGB context - Currently this treats the untagged data as
     // linear and converts to sRGB during the write. TODO: Fail or passthrough?
 
     // read back to srgb, no additional conversion
-    do_test(Encoding::kSRGB, Encoding::kUntagged, Encoding::kSRGB, error,
-            check_linear_to_srgb_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kUntagged, Encoding::kSRGB, error,
+                    check_linear_to_srgb_conversion, context, reporter);
     // read back to untagged. Currently converts back to linear. TODO: Fail or don't convert?
-    do_test(Encoding::kSRGB, Encoding::kUntagged, Encoding::kUntagged, error,
-            check_linear_to_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kUntagged, Encoding::kUntagged, error,
+                    check_linear_to_srgb_to_linear_conversion, context, reporter);
     // Converts back to linear.
-    do_test(Encoding::kSRGB, Encoding::kUntagged, Encoding::kLinear, error,
-            check_linear_to_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kUntagged, Encoding::kLinear, error,
+                    check_linear_to_srgb_to_linear_conversion, context, reporter);
+#else
+    // Currently writing untagged data to kSRGB fails because SkImageInfoValidConversion fails.
+    text_write_fails(Encoding::kSRGB, Encoding::kUntagged, context, reporter);
+#endif
 
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write linear data to a sRGB context. It gets converted to sRGB on write. The reads
     // are all the same as the above cases where the original data was untagged.
-    do_test(Encoding::kSRGB, Encoding::kLinear, Encoding::kSRGB, error,
-            check_linear_to_srgb_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kSRGB, error,
+                    check_linear_to_srgb_conversion, context, reporter);
     // TODO: Fail or don't convert?
-    do_test(Encoding::kSRGB, Encoding::kLinear, Encoding::kUntagged, error,
-            check_linear_to_srgb_to_linear_conversion, context, reporter);
-    do_test(Encoding::kSRGB, Encoding::kLinear, Encoding::kLinear, error,
-            check_linear_to_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kUntagged, error,
+                    check_linear_to_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kLinear, error,
+                    check_linear_to_srgb_to_linear_conversion, context, reporter);
 
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write data to an untagged context. The write does no conversion no matter what encoding the
     // src data has.
     for (auto writeEncoding : {Encoding::kSRGB, Encoding::kUntagged, Encoding::kLinear}) {
         // The read from untagged to sRGB also does no conversion. TODO: Should it just fail?
-        do_test(Encoding::kUntagged, writeEncoding, Encoding::kSRGB, error, check_no_conversion,
-                context, reporter);
+        test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kSRGB, error,
+                        check_no_conversion, context, reporter);
         // Reading untagged back as untagged should do no conversion.
-        do_test(Encoding::kUntagged, writeEncoding, Encoding::kUntagged, error, check_no_conversion,
-                context, reporter);
+        test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kUntagged, error,
+                        check_no_conversion, context, reporter);
         // Reading untagged back as linear does no conversion. TODO: Should it just fail?
-        do_test(Encoding::kUntagged, writeEncoding, Encoding::kLinear, error, check_no_conversion,
-                context, reporter);
+        test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kLinear, error,
+                        check_no_conversion, context, reporter);
     }
 
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write sRGB data to a linear context - converts to sRGB on the write.
 
     // converts back to sRGB on read.
-    do_test(Encoding::kLinear, Encoding::kSRGB, Encoding::kSRGB, error,
-            check_srgb_to_linear_to_srgb_conversion, context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kSRGB, Encoding::kSRGB, error,
+                    check_srgb_to_linear_to_srgb_conversion, context, reporter);
     // Reading untagged data from linear currently does no conversion. TODO: Should it fail?
-    do_test(Encoding::kLinear, Encoding::kSRGB, Encoding::kUntagged, error,
-            check_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kSRGB, Encoding::kUntagged, error,
+                    check_srgb_to_linear_conversion, context, reporter);
     // Stays linear when read.
-    do_test(Encoding::kLinear, Encoding::kSRGB, Encoding::kLinear, error,
-            check_srgb_to_linear_conversion, context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kSRGB, Encoding::kLinear, error,
+                    check_srgb_to_linear_conversion, context, reporter);
 
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write untagged data to a linear context. Currently does no conversion. TODO: Should this
     // fail?
 
+#ifdef SK_LEGACY_GPU_PIXEL_OPS
     // Reading to sRGB does a conversion.
-    do_test(Encoding::kLinear, Encoding::kUntagged, Encoding::kSRGB, error,
-            check_linear_to_srgb_conversion, context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kUntagged, Encoding::kSRGB, error,
+                    check_linear_to_srgb_conversion, context, reporter);
     // Reading to untagged does no conversion. TODO: Should it fail?
-    do_test(Encoding::kLinear, Encoding::kUntagged, Encoding::kUntagged, error, check_no_conversion,
-            context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kUntagged, Encoding::kUntagged, error,
+                    check_no_conversion, context, reporter);
     // Stays linear when read.
-    do_test(Encoding::kLinear, Encoding::kUntagged, Encoding::kLinear, error, check_no_conversion,
-            context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kUntagged, Encoding::kLinear, error,
+                    check_no_conversion, context, reporter);
+#else
+    // Currently writing untagged data to kLinear fails because SkImageInfoValidConversion fails.
+    text_write_fails(Encoding::kSRGB, Encoding::kUntagged, context, reporter);
+#endif
 
     ///////////////////////////////////////////////////////////////////////////////////////////////
     // Write linear data to a linear context. Does no conversion.
 
     // Reading to sRGB does a conversion.
-    do_test(Encoding::kLinear, Encoding::kLinear, Encoding::kSRGB, error,
-            check_linear_to_srgb_conversion, context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kSRGB, error,
+                    check_linear_to_srgb_conversion, context, reporter);
     // Reading to untagged does no conversion. TODO: Should it fail?
-    do_test(Encoding::kLinear, Encoding::kLinear, Encoding::kUntagged, error, check_no_conversion,
-            context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kUntagged, error,
+                    check_no_conversion, context, reporter);
     // Stays linear when read.
-    do_test(Encoding::kLinear, Encoding::kLinear, Encoding::kLinear, error, check_no_conversion,
-            context, reporter);
+    test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kLinear, error,
+                    check_no_conversion, context, reporter);
 }
 #endif
diff --git a/tests/WritePixelsTest.cpp b/tests/WritePixelsTest.cpp
index f2ad10c4f3..1a0f7a549d 100644
--- a/tests/WritePixelsTest.cpp
+++ b/tests/WritePixelsTest.cpp
@@ -423,22 +423,12 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(WritePixelsNonTexture_Gpu, reporter, ctxInfo)
         for (int sampleCnt : {1, 4}) {
             GrBackendTexture backendTex = gpu->createTestingOnlyBackendTexture(
                     nullptr, DEV_W, DEV_H, kSkia8888_GrPixelConfig, true, GrMipMapped::kNo);
-            SkColorType colorType;
-            if (kRGBA_8888_GrPixelConfig == kSkia8888_GrPixelConfig) {
-                colorType = kRGBA_8888_SkColorType;
-            } else {
-                colorType = kBGRA_8888_SkColorType;
-            }
+            SkColorType colorType = kN32_SkColorType;
             sk_sp<SkSurface> surface(SkSurface::MakeFromBackendTextureAsRenderTarget(
                     context, backendTex, origin, sampleCnt, colorType, nullptr, nullptr));
-            if (!surface) {
-                gpu->deleteTestingOnlyBackendTexture(&backendTex);
-                continue;
+            if (surface) {
+                test_write_pixels(reporter, surface.get());
             }
-
-            test_write_pixels(reporter, surface.get());
-
-            surface.reset();
             gpu->deleteTestingOnlyBackendTexture(&backendTex);
         }
     }