Fix some bugs with read/writePixels

- On both GL and Vulkan, we must draw if writing to an MSAA surface.
  Otherwise we just write to the resolve target texture, which gets
  overwritten on the next resolve.
- On Vulkan, we must draw if the target isn't a texture. (This check
  was already present in onWritePixels).
- On Vulkan, when reading from an MSAA surface as a different config,
  we don't need the readConfig to be renderable with MSAA - the temp
  surface is always created non-MSAA.

- Added tests for these fixes, verified that they failed previously.

Bug: skia:
Change-Id: Ia2d5025d7a8f8de8630413453f83b58028dd41aa
Reviewed-on: https://skia-review.googlesource.com/13691
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

4 files changed, 91 insertions, 30 deletions

diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 714ded0e7b..d585e097f9 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -665,6 +665,11 @@ bool GrGLGpu::onGetWritePixelsInfo(GrSurface* dstSurface, int width, int height,
         }
     }
 
+    // If the dst is MSAA, we have to draw, or we'll just be writing to the resolve target.
+    if (dstSurface->asRenderTarget() && dstSurface->asRenderTarget()->numColorSamples() > 0) {
+        ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
+    }
+
     if (GrPixelConfigIsSRGB(dstSurface->config()) != GrPixelConfigIsSRGB(srcConfig)) {
         ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
     }
@@ -2277,7 +2282,7 @@ bool GrGLGpu::onGetReadPixelsInfo(GrSurface* srcSurface, int width, int height,
 
     // Depends on why we need/want a temp draw. Start off assuming no change, the surface we read
     // from will be srcConfig and we will read readConfig pixels from it.
-    // Not that if we require a draw and return a non-renderable format for the temp surface the
+    // Note that if we require a draw and return a non-renderable format for the temp surface the
     // base class will fail for us.
     tempDrawInfo->fTempSurfaceDesc.fConfig = srcConfig;
     tempDrawInfo->fReadConfig = readConfig;
diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp
index cf7a39bb9d..c9f590397a 100644
--- a/src/gpu/vk/GrVkGpu.cpp
+++ b/src/gpu/vk/GrVkGpu.cpp
@@ -295,27 +295,28 @@ bool GrVkGpu::onGetWritePixelsInfo(GrSurface* dstSurface, int width, int height,
     tempDrawInfo->fTempSurfaceDesc.fOrigin = kTopLeft_GrSurfaceOrigin;
 
     if (dstSurface->config() == srcConfig) {
+        // We only support writing pixels to textures. Forcing a draw lets us write to pure RTs.
+        if (!dstSurface->asTexture()) {
+            ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
+        }
+        // If the dst is MSAA, we have to draw, or we'll just be writing to the resolve target.
+        if (renderTarget && renderTarget->numColorSamples() > 1) {
+            ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
+        }
         return true;
     }
 
-    if (renderTarget && this->vkCaps().isConfigRenderable(renderTarget->config(),
-                                                          renderTarget->numColorSamples() > 1)) {
-        ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
+    // Any config change requires a draw
+    ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
 
-        bool configsAreRBSwaps = GrPixelConfigSwapRAndB(srcConfig) == dstSurface->config();
+    bool configsAreRBSwaps = GrPixelConfigSwapRAndB(srcConfig) == dstSurface->config();
 
-        if (!this->vkCaps().isConfigTexturable(srcConfig) && configsAreRBSwaps) {
-            if (!this->vkCaps().isConfigTexturable(dstSurface->config())) {
-                return false;
-            }
-            tempDrawInfo->fTempSurfaceDesc.fConfig = dstSurface->config();
-            tempDrawInfo->fSwizzle = GrSwizzle::BGRA();
-            tempDrawInfo->fWriteConfig = dstSurface->config();
-        }
-        return true;
+    if (!this->vkCaps().isConfigTexturable(srcConfig) && configsAreRBSwaps) {
+        tempDrawInfo->fTempSurfaceDesc.fConfig = dstSurface->config();
+        tempDrawInfo->fSwizzle = GrSwizzle::BGRA();
+        tempDrawInfo->fWriteConfig = dstSurface->config();
     }
-
-    return false;
+    return true;
 }
 
 bool GrVkGpu::onWritePixels(GrSurface* surface,
@@ -848,6 +849,28 @@ sk_sp<GrRenderTarget> GrVkGpu::onWrapBackendRenderTarget(const GrBackendRenderTa
     return tgt;
 }
 
+sk_sp<GrRenderTarget> GrVkGpu::onWrapBackendTextureAsRenderTarget(
+        const GrBackendTextureDesc& wrapDesc){
+
+    const GrVkImageInfo* info =
+        reinterpret_cast<const GrVkImageInfo*>(wrapDesc.fTextureHandle);
+    if (VK_NULL_HANDLE == info->fImage) {
+        return nullptr;
+    }
+
+    GrSurfaceDesc desc;
+    desc.fFlags = (GrSurfaceFlags) wrapDesc.fFlags;
+    desc.fConfig = wrapDesc.fConfig;
+    desc.fWidth = wrapDesc.fWidth;
+    desc.fHeight = wrapDesc.fHeight;
+    desc.fSampleCnt = SkTMin(wrapDesc.fSampleCnt, this->caps()->maxSampleCount());
+
+    desc.fOrigin = resolve_origin(wrapDesc.fOrigin);
+
+    sk_sp<GrVkRenderTarget> tgt = GrVkRenderTarget::MakeWrappedRenderTarget(this, desc, info);
+    return tgt;
+}
+
 void GrVkGpu::generateMipmap(GrVkTexture* tex) {
     // don't do anything for linearly tiled textures (can't have mipmaps)
     if (tex->isLinearTiled()) {
@@ -1653,7 +1676,7 @@ bool GrVkGpu::onGetReadPixelsInfo(GrSurface* srcSurface, int width, int height,
 
     // Depends on why we need/want a temp draw. Start off assuming no change, the surface we read
     // from will be srcConfig and we will read readConfig pixels from it.
-    // Not that if we require a draw and return a non-renderable format for the temp surface the
+    // Note that if we require a draw and return a non-renderable format for the temp surface the
     // base class will fail for us.
     tempDrawInfo->fTempSurfaceDesc.fConfig = srcSurface->config();
     tempDrawInfo->fReadConfig = readConfig;
@@ -1662,14 +1685,12 @@ bool GrVkGpu::onGetReadPixelsInfo(GrSurface* srcSurface, int width, int height,
         return true;
     }
 
-    if (this->vkCaps().isConfigRenderable(readConfig, srcSurface->desc().fSampleCnt > 1)) {
-        ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
-        tempDrawInfo->fTempSurfaceDesc.fConfig = readConfig;
-        tempDrawInfo->fReadConfig = readConfig;
-        return true;
-    }
+    // Any config change requires a draw
+    ElevateDrawPreference(drawPreference, kRequireDraw_DrawPreference);
+    tempDrawInfo->fTempSurfaceDesc.fConfig = readConfig;
+    tempDrawInfo->fReadConfig = readConfig;
 
-    return false;
+    return true;
 }
 
 bool GrVkGpu::onReadPixels(GrSurface* surface,
diff --git a/src/gpu/vk/GrVkGpu.h b/src/gpu/vk/GrVkGpu.h
index 914a2ba5db..84772713a9 100644
--- a/src/gpu/vk/GrVkGpu.h
+++ b/src/gpu/vk/GrVkGpu.h
@@ -178,9 +178,7 @@ private:
     sk_sp<GrTexture> onWrapBackendTexture(const GrBackendTextureDesc&, GrWrapOwnership) override;
 
     sk_sp<GrRenderTarget> onWrapBackendRenderTarget(const GrBackendRenderTargetDesc&) override;
-    sk_sp<GrRenderTarget> onWrapBackendTextureAsRenderTarget(const GrBackendTextureDesc&) override {
-        return nullptr;
-    }
+    sk_sp<GrRenderTarget> onWrapBackendTextureAsRenderTarget(const GrBackendTextureDesc&) override;
 
     GrBuffer* onCreateBuffer(size_t size, GrBufferType type, GrAccessPattern,
                              const void* data) override;
diff --git a/tests/WritePixelsTest.cpp b/tests/WritePixelsTest.cpp
index f5cdddccd0..e26e134b2d 100644
--- a/tests/WritePixelsTest.cpp
+++ b/tests/WritePixelsTest.cpp
@@ -14,6 +14,7 @@
 
 #if SK_SUPPORT_GPU
 #include "GrContext.h"
+#include "GrGpu.h"
 #endif
 
 #include <initializer_list>
@@ -410,9 +411,45 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(WritePixels_Gpu, reporter, ctxInfo) {
     const SkImageInfo ii = SkImageInfo::MakeN32Premul(DEV_W, DEV_H);
 
     for (auto& origin : { kTopLeft_GrSurfaceOrigin, kBottomLeft_GrSurfaceOrigin }) {
-        sk_sp<SkSurface> surface(SkSurface::MakeRenderTarget(ctxInfo.grContext(), SkBudgeted::kNo,
-                                                             ii, 0, origin, nullptr));
-        test_write_pixels(reporter, surface.get());
+        for (int sampleCnt : {0, 4}) {
+            sk_sp<SkSurface> surface(SkSurface::MakeRenderTarget(ctxInfo.grContext(),
+                                                                 SkBudgeted::kNo, ii, sampleCnt,
+                                                                 origin, nullptr));
+            if (!surface && sampleCnt > 0) {
+                // Some platforms don't support MSAA
+                continue;
+            }
+            test_write_pixels(reporter, surface.get());
+        }
+    }
+}
+
+DEF_GPUTEST_FOR_RENDERING_CONTEXTS(WritePixelsNonTexture_Gpu, reporter, ctxInfo) {
+    GrContext* context = ctxInfo.grContext();
+
+    for (auto& origin : { kTopLeft_GrSurfaceOrigin, kBottomLeft_GrSurfaceOrigin }) {
+        for (int sampleCnt : {0, 4}) {
+            GrBackendTextureDesc desc;
+            desc.fConfig = kSkia8888_GrPixelConfig;
+            desc.fWidth = DEV_W;
+            desc.fHeight = DEV_H;
+            desc.fFlags = kRenderTarget_GrBackendTextureFlag;
+            desc.fSampleCnt = sampleCnt;
+            desc.fOrigin = origin;
+            desc.fTextureHandle = context->getGpu()->createTestingOnlyBackendTexture(
+                nullptr, DEV_W, DEV_H, kSkia8888_GrPixelConfig, true);
+            sk_sp<SkSurface> surface(SkSurface::MakeFromBackendTextureAsRenderTarget(context, desc,
+                                                                                     nullptr));
+            if (!surface) {
+                context->getGpu()->deleteTestingOnlyBackendTexture(desc.fTextureHandle);
+                continue;
+            }
+
+            test_write_pixels(reporter, surface.get());
+
+            surface.reset();
+            context->getGpu()->deleteTestingOnlyBackendTexture(desc.fTextureHandle);
+        }
     }
 }
 #endif