Respect kRectsMustMatchForMSAASrc_BlitFramebufferFlag in dst setup

Crurently, when preparing a texture for blitFramebuffer, we ignore the
kRectsMustMatchForMSAASrc_BlitFramebufferFlag, and may attempt to
copy from one src rect to a different dst rect.

This change updates initDescForDstCopy and setupDstTexture to allocate
larger textures if necessary and accomodate this flags requirements.

Bug: 658277
Change-Id: If4489ac3192dcf6f9996494c63821279721d0a12
Reviewed-on: https://skia-review.googlesource.com/11141
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>

include/gpu/GrCaps.h

@@ -189,9 +189,12 @@ public:
      * This is can be called before allocating a texture to be a dst for copySurface. This is only
      * used for doing dst copies needed in blends, thus the src is always a GrRenderTarget. It will
      * populate the origin, config, and flags fields of the desc such that copySurface can
-     * efficiently succeed.
+     * efficiently succeed. rectsMustMatch will be set to true if the copy operation must ensure
+     * that the src and dest rects are identical. disallowSubrect will be set to true if copy rect
+     * must equal src's bounds.
      */
-    virtual bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const = 0;
+    virtual bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc,
+                                    bool* rectsMustMatch, bool* disallowSubrect) const = 0;
 
 protected:
     /** Subclasses must call this at the end of their constructors in order to apply caps

src/gpu/GrRenderTargetContext.cpp

@@ -1751,10 +1751,11 @@ void GrRenderTargetContext::setupDstTexture(GrRenderTarget* rt, const GrClip& cl
     clip.getConservativeBounds(rt->width(), rt->height(), &copyRect);
 
     SkIRect drawIBounds;
+    SkIRect clippedRect;
     opBounds.roundOut(&drawIBounds);
     // Cover up for any precision issues by outsetting the op bounds a pixel in each direction.
     drawIBounds.outset(1, 1);
-    if (!copyRect.intersect(drawIBounds)) {
+    if (!clippedRect.intersect(copyRect, drawIBounds)) {
 #ifdef SK_DEBUG
         GrCapsDebugf(this->caps(), "Missed an early reject. "
                                    "Bailing on draw from setupDstTexture.\n");
@@ -1765,24 +1766,43 @@ void GrRenderTargetContext::setupDstTexture(GrRenderTarget* rt, const GrClip& cl
     // MSAA consideration: When there is support for reading MSAA samples in the shader we could
     // have per-sample dst values by making the copy multisampled.
     GrSurfaceDesc desc;
-    if (!this->caps()->initDescForDstCopy(rt, &desc)) {
+    bool rectsMustMatch = false;
+    bool disallowSubrect = false;
+    if (!this->caps()->initDescForDstCopy(rt, &desc, &rectsMustMatch, &disallowSubrect)) {
         desc.fOrigin = kDefault_GrSurfaceOrigin;
         desc.fFlags = kRenderTarget_GrSurfaceFlag;
         desc.fConfig = rt->config();
     }
 
+    if (!disallowSubrect) {
+        copyRect = clippedRect;
+    }
+
+    SkIPoint dstPoint;
+    SkIPoint dstOffset;
+    static const uint32_t kFlags = 0;
+    sk_sp<GrTexture> copy;
+    if (rectsMustMatch) {
+        SkASSERT(desc.fOrigin == rt->origin());
+        desc.fWidth = rt->width();
+        desc.fHeight = rt->height();
+        dstPoint = {copyRect.fLeft, copyRect.fTop};
+        dstOffset = {0, 0};
+        copy.reset(fContext->resourceProvider()->createTexture(desc, SkBudgeted::kYes, kFlags));
+    } else {
         desc.fWidth = copyRect.width();
         desc.fHeight = copyRect.height();
-
+        dstPoint = {0, 0};
-    static const uint32_t kFlags = 0;
+        dstOffset = {copyRect.fLeft, copyRect.fTop};
-    sk_sp<GrTexture> copy(fContext->resourceProvider()->createApproxTexture(desc, kFlags));
+        copy.reset(fContext->resourceProvider()->createApproxTexture(desc, kFlags));
+    }
 
     if (!copy) {
         SkDebugf("Failed to create temporary copy of destination texture.\n");
         return;
     }
-    SkIPoint dstPoint = {0, 0};
+
     this->getOpList()->copySurface(copy.get(), rt, copyRect, dstPoint);
     dstTexture->setTexture(std::move(copy));
-    dstTexture->setOffset(copyRect.fLeft, copyRect.fTop);
+    dstTexture->setOffset(dstOffset);
 }

src/gpu/gl/GrGLCaps.cpp

@@ -1018,6 +1018,7 @@ void GrGLCaps::initFSAASupport(const GrGLContextInfo& ctxInfo, const GrGLInterfa
         // is available.
         if (ctxInfo.version() >= GR_GL_VER(3, 0)) {
             fBlitFramebufferFlags = kNoFormatConversionForMSAASrc_BlitFramebufferFlag |
+                                    kNoMSAADst_BlitFramebufferFlag |
                                     kRectsMustMatchForMSAASrc_BlitFramebufferFlag;
         } else if (ctxInfo.hasExtension("GL_CHROMIUM_framebuffer_multisample") ||
                    ctxInfo.hasExtension("GL_ANGLE_framebuffer_blit")) {
@@ -1026,7 +1027,8 @@ void GrGLCaps::initFSAASupport(const GrGLContextInfo& ctxInfo, const GrGLInterfa
             fBlitFramebufferFlags = kNoScalingOrMirroring_BlitFramebufferFlag |
                                     kResolveMustBeFull_BlitFrambufferFlag |
                                     kNoMSAADst_BlitFramebufferFlag |
-                                    kNoFormatConversion_BlitFramebufferFlag;
+                                    kNoFormatConversion_BlitFramebufferFlag |
+                                    kRectsMustMatchForMSAASrc_BlitFramebufferFlag;
         }
     } else {
         if (fUsesMixedSamples) {
@@ -2068,7 +2070,14 @@ void GrGLCaps::initConfigTable(const GrContextOptions& contextOptions,
 #endif
 }
 
-bool GrGLCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const {
+bool GrGLCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc,
+                                  bool* rectsMustMatch, bool* disallowSubrect) const {
+    // By default, we don't require rects to match.
+    *rectsMustMatch = false;
+
+    // By default, we allow subrects.
+    *disallowSubrect = false;
+
     // If the src is a texture, we can implement the blit as a draw assuming the config is
     // renderable.
     if (src->asTexture() && this->isConfigRenderable(src->config(), false)) {
@@ -2089,7 +2098,20 @@ bool GrGLCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc
     // texture. This code prefers CopyTexSubImage to fbo blit and avoids triggering temporary fbo
     // creation. It isn't clear that avoiding temporary fbo creation is actually optimal.
     GrSurfaceOrigin originForBlitFramebuffer = kDefault_GrSurfaceOrigin;
-    if (this->blitFramebufferSupportFlags() & kNoScalingOrMirroring_BlitFramebufferFlag) {
+    bool rectsMustMatchForBlitFramebuffer = false;
+    bool disallowSubrectForBlitFramebuffer = false;
+    if (src->numColorSamples() &&
+        (this->blitFramebufferSupportFlags() & kResolveMustBeFull_BlitFrambufferFlag)) {
+        rectsMustMatchForBlitFramebuffer = true;
+        disallowSubrectForBlitFramebuffer = true;
+        // Mirroring causes rects to mismatch later, don't allow it.
+        originForBlitFramebuffer = src->origin();
+    } else if (src->numColorSamples() && (this->blitFramebufferSupportFlags() &
+                                          kRectsMustMatchForMSAASrc_BlitFramebufferFlag)) {
+        rectsMustMatchForBlitFramebuffer = true;
+        // Mirroring causes rects to mismatch later, don't allow it.
+        originForBlitFramebuffer = src->origin();
+    } else if (this->blitFramebufferSupportFlags() & kNoScalingOrMirroring_BlitFramebufferFlag) {
         originForBlitFramebuffer = src->origin();
     }
 
@@ -2100,6 +2122,8 @@ bool GrGLCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc
         if (this->canConfigBeFBOColorAttachment(kBGRA_8888_GrPixelConfig)) {
             desc->fOrigin = originForBlitFramebuffer;
             desc->fConfig = kBGRA_8888_GrPixelConfig;
+            *rectsMustMatch = rectsMustMatchForBlitFramebuffer;
+            *disallowSubrect = disallowSubrectForBlitFramebuffer;
             return true;
         }
         return false;
@@ -2112,6 +2136,8 @@ bool GrGLCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc
         if (this->canConfigBeFBOColorAttachment(src->config())) {
             desc->fOrigin = originForBlitFramebuffer;
             desc->fConfig = src->config();
+            *rectsMustMatch = rectsMustMatchForBlitFramebuffer;
+            *disallowSubrect = disallowSubrectForBlitFramebuffer;
             return true;
         }
         return false;

src/gpu/gl/GrGLCaps.h

@@ -359,7 +359,8 @@ public:
         return fRGBAToBGRAReadbackConversionsAreSlow;
     }
 
-    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override;
+    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch,
+                            bool* disallowSubrect) const override;
 
 private:
     enum ExternalFormatUsage {

src/gpu/gl/GrGLGpu.cpp

@@ -3344,9 +3344,14 @@ static inline bool can_blit_framebuffer_for_copy_surface(const GrSurface* dst,
         }
     }
     if (GrGLCaps::kResolveMustBeFull_BlitFrambufferFlag & blitFramebufferFlags) {
-        if (srcRT && srcRT->numColorSamples() && dstRT && !dstRT->numColorSamples()) {
+        if (srcRT && srcRT->numColorSamples()) {
+            if (dstRT && !dstRT->numColorSamples()) {
                 return false;
             }
+            if (SkRect::Make(srcRect) != srcRT->getBoundsRect()) {
+                return false;
+            }
+        }
     }
     if (GrGLCaps::kNoMSAADst_BlitFramebufferFlag & blitFramebufferFlags) {
         if (dstRT && dstRT->numColorSamples() > 0) {
@@ -3364,10 +3369,14 @@ static inline bool can_blit_framebuffer_for_copy_surface(const GrSurface* dst,
         }
     }
     if (GrGLCaps::kRectsMustMatchForMSAASrc_BlitFramebufferFlag & blitFramebufferFlags) {
-        if (srcRT && srcRT->numColorSamples() &&
+        if (srcRT && srcRT->numColorSamples()) {
-            (dstPoint.fX != srcRect.fLeft || dstPoint.fY != srcRect.fTop)) {
+            if (dstPoint.fX != srcRect.fLeft || dstPoint.fY != srcRect.fTop) {
                 return false;
             }
+            if (dst->origin() != src->origin()) {
+                return false;
+            }
+        }
     }
     return true;
 }

src/gpu/vk/GrVkCaps.cpp

@@ -53,7 +53,12 @@ GrVkCaps::GrVkCaps(const GrContextOptions& contextOptions, const GrVkInterface*
     this->init(contextOptions, vkInterface, physDev, featureFlags, extensionFlags);
 }
 
-bool GrVkCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const {
+bool GrVkCaps::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc,
+                                  bool* rectsMustMatch, bool* disallowSubrect) const {
+    // Vk doesn't use rectsMustMatch or disallowSubrect. Always return false.
+    *rectsMustMatch = false;
+    *disallowSubrect = false;
+
     // We can always succeed here with either a CopyImage (none msaa src) or ResolveImage (msaa).
     // For CopyImage we can make a simple texture, for ResolveImage we require the dst to be a
     // render target as well.

src/gpu/vk/GrVkCaps.h

@@ -104,7 +104,8 @@ public:
         return fPreferedStencilFormat;
     }
 
-    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override;
+    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch,
+                            bool* disallowSubrect) const override;
 
 private:
     enum VkVendor {

tests/BlendTest.cpp

@@ -5,11 +5,24 @@
  * found in the LICENSE file.
  */
 
-#include "Test.h"
+#include <functional>
+#include "SkBitmap.h"
+#include "SkCanvas.h"
 #include "SkColor.h"
 #include "SkColorPriv.h"
+#include "SkSurface.h"
 #include "SkTaskGroup.h"
-#include <functional>
+#include "SkUtils.h"
+#include "Test.h"
+
+#if SK_SUPPORT_GPU
+#include "GrContext.h"
+#include "GrContextPriv.h"
+#include "GrResourceProvider.h"
+#include "GrSurfaceContext.h"
+#include "GrSurfaceProxy.h"
+#include "GrTexture.h"
+#endif
 
 struct Results { int diffs, diffs_0x00, diffs_0xff, diffs_by_1; };
 
@@ -66,3 +79,116 @@ DEF_TEST(Blend_byte_multiply, r) {
     };
     for (auto multiply : perfect) { REPORTER_ASSERT(r, test(multiply).diffs == 0); }
 }
+
+#if SK_SUPPORT_GPU
+namespace {
+static sk_sp<SkSurface> create_gpu_surface_backend_texture_as_render_target(
+        GrContext* context, int sampleCnt, int width, int height, GrPixelConfig config,
+        sk_sp<GrTexture>* backingSurface) {
+    GrSurfaceDesc backingDesc;
+    backingDesc.fHeight = height;
+    backingDesc.fWidth = width;
+    backingDesc.fConfig = config;
+    backingDesc.fOrigin = kDefault_GrSurfaceOrigin;
+    backingDesc.fFlags = kRenderTarget_GrSurfaceFlag;
+
+    (*backingSurface)
+            .reset(context->resourceProvider()->createTexture(backingDesc, SkBudgeted::kNo));
+
+    GrBackendTextureDesc desc;
+    desc.fConfig = config;
+    desc.fWidth = width;
+    desc.fHeight = height;
+    desc.fFlags = kRenderTarget_GrBackendTextureFlag;
+    desc.fTextureHandle = (*backingSurface)->getTextureHandle();
+    desc.fSampleCnt = sampleCnt;
+    sk_sp<SkSurface> surface =
+            SkSurface::MakeFromBackendTextureAsRenderTarget(context, desc, nullptr);
+    return surface;
+}
+}
+
+// Tests blending to a surface with no texture available.
+DEF_GPUTEST_FOR_GL_RENDERING_CONTEXTS(ES2BlendWithNoTexture, reporter, ctxInfo) {
+    GrContext* context = ctxInfo.grContext();
+    const int kWidth = 10;
+    const int kHeight = 10;
+    const GrPixelConfig kConfig = kRGBA_8888_GrPixelConfig;
+    const SkColorType kColorType = kRGBA_8888_SkColorType;
+
+    // Build our test cases:
+    struct RectAndSamplePoint {
+        SkRect rect;
+        SkIPoint outPoint;
+        SkIPoint inPoint;
+    } allRectsAndPoints[3] = {
+            {SkRect::MakeXYWH(0, 0, 5, 5), SkIPoint::Make(7, 7), SkIPoint::Make(2, 2)},
+            {SkRect::MakeXYWH(2, 2, 5, 5), SkIPoint::Make(0, 0), SkIPoint::Make(4, 4)},
+            {SkRect::MakeXYWH(5, 5, 5, 5), SkIPoint::Make(2, 2), SkIPoint::Make(7, 7)},
+    };
+
+    struct TestCase {
+        RectAndSamplePoint rectAndPoints;
+        int sampleCnt;
+    };
+    std::vector<TestCase> testCases;
+
+    for (int sampleCnt : {0, 4}) {
+        for (auto rectAndPoints : allRectsAndPoints) {
+            testCases.push_back({rectAndPoints, sampleCnt});
+        }
+    }
+
+    // Run each test case:
+    for (auto testCase : testCases) {
+        int sampleCnt = testCase.sampleCnt;
+        SkRect paintRect = testCase.rectAndPoints.rect;
+        SkIPoint outPoint = testCase.rectAndPoints.outPoint;
+        SkIPoint inPoint = testCase.rectAndPoints.inPoint;
+
+        sk_sp<GrTexture> backingSurface;
+        // BGRA forces a framebuffer blit on ES2.
+        sk_sp<SkSurface> surface = create_gpu_surface_backend_texture_as_render_target(
+                context, sampleCnt, kWidth, kHeight, kConfig, &backingSurface);
+
+        if (!surface && sampleCnt > 0) {
+            // Some platforms don't support MSAA.
+            continue;
+        }
+        REPORTER_ASSERT(reporter, !!surface);
+
+        // Fill our canvas with 0xFFFF80
+        SkCanvas* canvas = surface->getCanvas();
+        SkPaint black_paint;
+        black_paint.setColor(SkColorSetRGB(0xFF, 0xFF, 0x80));
+        canvas->drawRect(SkRect::MakeXYWH(0, 0, kWidth, kHeight), black_paint);
+
+        // Blend 2x2 pixels at 5,5 with 0x80FFFF. Use multiply blend mode as this will trigger
+        // a copy of the destination.
+        SkPaint white_paint;
+        white_paint.setColor(SkColorSetRGB(0x80, 0xFF, 0xFF));
+        white_paint.setBlendMode(SkBlendMode::kMultiply);
+        canvas->drawRect(paintRect, white_paint);
+
+        // Read the result into a bitmap.
+        SkBitmap bitmap;
+        REPORTER_ASSERT(reporter, bitmap.tryAllocPixels(SkImageInfo::Make(
+                                          kWidth, kHeight, kColorType, kPremul_SkAlphaType)));
+        bitmap.lockPixels();
+        REPORTER_ASSERT(
+                reporter,
+                surface->readPixels(bitmap.info(), bitmap.getPixels(), bitmap.rowBytes(), 0, 0));
+
+        // Check the in/out pixels.
+        REPORTER_ASSERT(reporter, bitmap.getColor(outPoint.x(), outPoint.y()) ==
+                                          SkColorSetRGB(0xFF, 0xFF, 0x80));
+        REPORTER_ASSERT(reporter, bitmap.getColor(inPoint.x(), inPoint.y()) ==
+                                          SkColorSetRGB(0x80, 0xFF, 0x80));
+
+        // Clean up - surface depends on backingSurface and must be released first.
+        bitmap.unlockPixels();
+        surface.reset();
+        backingSurface.reset();
+    }
+}
+#endif

tools/gpu/GrTest.cpp

@@ -272,7 +272,8 @@ public:
     bool isConfigTexturable(GrPixelConfig config) const override { return false; }
     bool isConfigRenderable(GrPixelConfig config, bool withMSAA) const override { return false; }
     bool canConfigBeImageStorage(GrPixelConfig) const override { return false; }
-    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override {
+    bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch,
+                            bool* disallowSubrect) const override {
         return false;
     }