From 4c81ba6ba3a3270db809bf7d4c3bc782694a56a4 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Tue, 11 Apr 2017 13:38:29 +0000 Subject: [PATCH] Revert "Support Canvas Clip on Blit Framebuffer" This reverts commit 1f1646dd861c4b78f0a935b45f7802c787dac198. Reason for revert: Vulkan build failures, many non-Vulkan test failures. Original change's description: > Support Canvas Clip on Blit Framebuffer > > The previous fix to blit framebuffer didn't take cases where > the canvas had a clip applied into account. Fix and update > the unit test to add this case. > > AND > > 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, 658277 > > NOTREECHECKS=true > NOTRY=true > NOPRESUBMIT=true > > Change-Id: I36a54072ee15d035249ef0b0f6cf512e791dd218 > Reviewed-on: https://skia-review.googlesource.com/13085 > Reviewed-by: Brian Salomon > TBR=bsalomon@google.com,ericrk@chromium.org,mtklein@chromium.org,reviews@skia.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I12d8f4eb0e089b1ed7f38620395066b4e1dcefdd Reviewed-on: https://skia-review.googlesource.com/13135 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- src/gpu/GrGpu.h | 7 +- src/gpu/GrRenderTargetOpList.cpp | 47 ++++---------- src/gpu/gl/GrGLCaps.cpp | 4 +- src/gpu/gl/GrGLGpu.cpp | 54 ++++------------ src/gpu/gl/GrGLGpu.h | 3 +- src/gpu/vk/GrVkGpu.cpp | 10 +-- src/gpu/vk/GrVkGpu.h | 3 +- tests/BlendTest.cpp | 134 +-------------------------------------- tools/gpu/GrTest.cpp | 3 +- 9 files changed, 36 insertions(+), 229 deletions(-) diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h index faf1dce..201b22c 100644 --- a/src/gpu/GrGpu.h +++ b/src/gpu/GrGpu.h @@ -311,12 +311,9 @@ 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. 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. + * efficiently succeed. */ - virtual bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, - bool* rectsMustMatch, bool* disallowSubrect) const = 0; + virtual bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const = 0; // After the client interacts directly with the 3D context state the GrGpu // must resync its internal state and assumptions about 3D context state. diff --git a/src/gpu/GrRenderTargetOpList.cpp b/src/gpu/GrRenderTargetOpList.cpp index 5c1deab..4fc898c 100644 --- a/src/gpu/GrRenderTargetOpList.cpp +++ b/src/gpu/GrRenderTargetOpList.cpp @@ -99,6 +99,9 @@ void GrRenderTargetOpList::setupDstTexture(GrRenderTarget* rt, const GrClip& clip, const SkRect& opBounds, GrXferProcessor::DstTexture* dstTexture) { + SkRect bounds = opBounds; + bounds.outset(0.5f, 0.5f); + if (this->caps()->textureBarrierSupport()) { if (GrTexture* rtTex = rt->asTexture()) { // The render target is a texture, so we can read from it directly in the shader. The XP @@ -109,15 +112,12 @@ void GrRenderTargetOpList::setupDstTexture(GrRenderTarget* rt, } } - SkIRect copyRect = SkIRect::MakeWH(rt->width(), rt->height()); + SkIRect copyRect; + clip.getConservativeBounds(rt->width(), rt->height(), ©Rect); - SkIRect clippedRect; - clip.getConservativeBounds(rt->width(), rt->height(), &clippedRect); SkIRect drawIBounds; - opBounds.roundOut(&drawIBounds); - // Cover up for any precision issues by outsetting the op bounds a pixel in each direction. - drawIBounds.outset(1, 1); - if (!clippedRect.intersect(drawIBounds)) { + bounds.roundOut(&drawIBounds); + if (!copyRect.intersect(drawIBounds)) { #ifdef SK_DEBUG GrCapsDebugf(this->caps(), "Missed an early reject. " "Bailing on draw from setupDstTexture.\n"); @@ -128,45 +128,26 @@ void GrRenderTargetOpList::setupDstTexture(GrRenderTarget* rt, // 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; - bool rectsMustMatch = false; - bool disallowSubrect = false; - if (!fGpu->initDescForDstCopy(rt, &desc, &rectsMustMatch, &disallowSubrect)) { + if (!fGpu->initDescForDstCopy(rt, &desc)) { desc.fOrigin = kDefault_GrSurfaceOrigin; desc.fFlags = kRenderTarget_GrSurfaceFlag; desc.fConfig = rt->config(); } - if (!disallowSubrect) { - copyRect = clippedRect; - } + desc.fWidth = copyRect.width(); + desc.fHeight = copyRect.height(); - SkIPoint dstPoint; - SkIPoint dstOffset; static const uint32_t kFlags = 0; - sk_sp 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}; - dstOffset = {copyRect.fLeft, copyRect.fTop}; - copy.reset(fContext->resourceProvider()->createApproxTexture(desc, kFlags)); - } + sk_sp copy(fResourceProvider->createApproxTexture(desc, kFlags)); if (!copy) { SkDebugf("Failed to create temporary copy of destination texture.\n"); return; } - - fGpu->copySurface(copy.get(), rt, copyRect, dstPoint); + SkIPoint dstPoint = {0, 0}; + this->copySurface(copy.get(), rt, copyRect, dstPoint); dstTexture->setTexture(std::move(copy)); - dstTexture->setOffset(dstOffset); + dstTexture->setOffset(copyRect.fLeft, copyRect.fTop); } void GrRenderTargetOpList::prepareOps(GrOpFlushState* flushState) { diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index 3d0cebf..0bcb7e8 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -1029,7 +1029,6 @@ 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")) { @@ -1038,8 +1037,7 @@ void GrGLCaps::initFSAASupport(const GrGLContextInfo& ctxInfo, const GrGLInterfa fBlitFramebufferFlags = kNoScalingOrMirroring_BlitFramebufferFlag | kResolveMustBeFull_BlitFrambufferFlag | kNoMSAADst_BlitFramebufferFlag | - kNoFormatConversion_BlitFramebufferFlag | - kRectsMustMatchForMSAASrc_BlitFramebufferFlag; + kNoFormatConversion_BlitFramebufferFlag; } } else { if (fUsesMixedSamples) { diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index bc4e7d6..cfbef9f 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -3574,13 +3574,8 @@ static inline bool can_blit_framebuffer_for_copy_surface(const GrSurface* dst, } } if (GrGLCaps::kResolveMustBeFull_BlitFrambufferFlag & blitFramebufferFlags) { - if (srcRT && srcRT->numColorSamples()) { - if (dstRT && !dstRT->numColorSamples()) { - return false; - } - if (SkRect::Make(srcRect) != srcRT->getBoundsRect()) { - return false; - } + if (srcRT && srcRT->numColorSamples() && dstRT && !dstRT->numColorSamples()) { + return false; } } if (GrGLCaps::kNoMSAADst_BlitFramebufferFlag & blitFramebufferFlags) { @@ -3599,13 +3594,9 @@ static inline bool can_blit_framebuffer_for_copy_surface(const GrSurface* dst, } } if (GrGLCaps::kRectsMustMatchForMSAASrc_BlitFramebufferFlag & blitFramebufferFlags) { - if (srcRT && srcRT->numColorSamples()) { - if (dstPoint.fX != srcRect.fLeft || dstPoint.fY != srcRect.fTop) { - return false; - } - if (dst->origin() != src->origin()) { - return false; - } + if (srcRT && srcRT->numColorSamples() && + (dstPoint.fX != srcRect.fLeft || dstPoint.fY != srcRect.fTop)) { + return false; } } return true; @@ -3702,14 +3693,7 @@ void GrGLGpu::unbindTextureFBOForPixelOps(GrGLenum fboTarget, GrSurface* surface } } -bool GrGLGpu::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; - +bool GrGLGpu::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const { // If the src is a texture, we can implement the blit as a draw assuming the config is // renderable. if (src->asTexture() && this->caps()->isConfigRenderable(src->config(), false)) { @@ -3729,35 +3713,21 @@ bool GrGLGpu::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, // possible and we return false to fallback to creating a render target dst for render-to- // 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; - bool rectsMustMatchForBlitFramebuffer = false; - bool disallowSubrectForBlitFramebuffer = false; - if (src->numColorSamples() && (this->glCaps().blitFramebufferSupportFlags() & - GrGLCaps::kResolveMustBeFull_BlitFrambufferFlag)) { - rectsMustMatchForBlitFramebuffer = true; - disallowSubrectForBlitFramebuffer = true; - // Mirroring causes rects to mismatch later, don't allow it. - originForBlitFramebuffer = src->origin(); - } else if (src->numColorSamples() && - (this->glCaps().blitFramebufferSupportFlags() & - GrGLCaps::kRectsMustMatchForMSAASrc_BlitFramebufferFlag)) { - rectsMustMatchForBlitFramebuffer = true; - // Mirroring causes rects to mismatch later, don't allow it. - originForBlitFramebuffer = src->origin(); - } else if (this->glCaps().blitFramebufferSupportFlags() & - GrGLCaps::kNoScalingOrMirroring_BlitFramebufferFlag) { + if (this->glCaps().blitFramebufferSupportFlags() & + GrGLCaps::kNoScalingOrMirroring_BlitFramebufferFlag) { originForBlitFramebuffer = src->origin(); } // Check for format issues with glCopyTexSubImage2D - if (this->glCaps().bgraIsInternalFormat() && kBGRA_8888_GrPixelConfig == src->config()) { + if (kGLES_GrGLStandard == this->glStandard() && this->glCaps().bgraIsInternalFormat() && + kBGRA_8888_GrPixelConfig == src->config()) { // glCopyTexSubImage2D doesn't work with this config. If the bgra can be used with fbo blit // then we set up for that, otherwise fail. if (this->glCaps().canConfigBeFBOColorAttachment(kBGRA_8888_GrPixelConfig)) { desc->fOrigin = originForBlitFramebuffer; desc->fConfig = kBGRA_8888_GrPixelConfig; - *rectsMustMatch = rectsMustMatchForBlitFramebuffer; - *disallowSubrect = disallowSubrectForBlitFramebuffer; return true; } return false; @@ -3770,8 +3740,6 @@ bool GrGLGpu::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, if (this->glCaps().canConfigBeFBOColorAttachment(src->config())) { desc->fOrigin = originForBlitFramebuffer; desc->fConfig = src->config(); - *rectsMustMatch = rectsMustMatchForBlitFramebuffer; - *disallowSubrect = disallowSubrectForBlitFramebuffer; return true; } return false; diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h index a4f3784..57068fb 100644 --- a/src/gpu/gl/GrGLGpu.h +++ b/src/gpu/gl/GrGLGpu.h @@ -75,8 +75,7 @@ public: GrPixelConfig srcConfig, DrawPreference*, WritePixelTempDrawInfo*) override; - bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch, - bool* disallowSubrect) const override; + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override; // These functions should be used to bind GL objects. They track the GL state and skip redundant // bindings. Making the equivalent glBind calls directly will confuse the state tracking. diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp index 24a6dcd..b85aa2d 100644 --- a/src/gpu/vk/GrVkGpu.cpp +++ b/src/gpu/vk/GrVkGpu.cpp @@ -1590,18 +1590,14 @@ bool GrVkGpu::onCopySurface(GrSurface* dst, return false; } -bool GrVkGpu::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; - +bool GrVkGpu::initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const { // 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. desc->fOrigin = src->origin(); desc->fConfig = src->config(); - if (src->numColorSamples() > 1 || (src->asTexture() && this->supportsCopiesAsDraws())) { + if (src->numColorSamples() > 1 || + (src->asTexture() && this->vkCaps().supportsCopiesAsDraws())) { desc->fFlags = kRenderTarget_GrSurfaceFlag; } else { // Just going to use CopyImage here diff --git a/src/gpu/vk/GrVkGpu.h b/src/gpu/vk/GrVkGpu.h index 4bfd5de..7203bf1 100644 --- a/src/gpu/vk/GrVkGpu.h +++ b/src/gpu/vk/GrVkGpu.h @@ -79,8 +79,7 @@ public: void onQueryMultisampleSpecs(GrRenderTarget* rt, const GrStencilSettings&, int* effectiveSampleCnt, SamplePattern*) override; - bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch, - bool* disallowSubrect) const override; + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override; void xferBarrier(GrRenderTarget*, GrXferBarrierType) override {} diff --git a/tests/BlendTest.cpp b/tests/BlendTest.cpp index 193c37d..785acb8 100644 --- a/tests/BlendTest.cpp +++ b/tests/BlendTest.cpp @@ -5,24 +5,11 @@ * found in the LICENSE file. */ -#include -#include "SkBitmap.h" -#include "SkCanvas.h" +#include "Test.h" #include "SkColor.h" #include "SkColorPriv.h" -#include "SkSurface.h" #include "SkTaskGroup.h" -#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 +#include struct Results { int diffs, diffs_0x00, diffs_0xff, diffs_by_1; }; @@ -79,120 +66,3 @@ 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 create_gpu_surface_backend_texture_as_render_target( - GrContext* context, int sampleCnt, int width, int height, GrPixelConfig config, - sk_sp* 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 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(1, 1), SkIPoint::Make(4, 4)}, - {SkRect::MakeXYWH(5, 5, 5, 5), SkIPoint::Make(2, 2), SkIPoint::Make(7, 7)}, - }; - - struct TestCase { - RectAndSamplePoint rectAndPoints; - SkRect clip; - int sampleCnt; - }; - std::vector testCases; - - for (int sampleCnt : {0, 4}) { - for (auto rectAndPoints : allRectsAndPoints) { - for (auto clip : {SkRect::MakeXYWH(0, 0, 10, 10), SkRect::MakeXYWH(1, 1, 8, 8)}) { - testCases.push_back({rectAndPoints, clip, 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 backingSurface; - // BGRA forces a framebuffer blit on ES2. - sk_sp 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(); - canvas->clipRect(testCase.clip, false); - 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 diff --git a/tools/gpu/GrTest.cpp b/tools/gpu/GrTest.cpp index f23f40d..4df328c 100644 --- a/tools/gpu/GrTest.cpp +++ b/tools/gpu/GrTest.cpp @@ -310,8 +310,7 @@ public: *effectiveSampleCnt = rt->desc().fSampleCnt; } - bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch, - bool* disallowSubrect) const override { + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override { return false; } -- 2.7.4