From ec61785bbb989a1901b063923da30c04ed41332f Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Sat, 1 Apr 2017 01:53:59 +0000 Subject: [PATCH] Revert "Respect kRectsMustMatchForMSAASrc_BlitFramebufferFlag in dst setup" This reverts commit d58f040532f2f5a63d24bd17d7c588e52c0b99c3. Reason for revert: tests/BlendTest is failing on the Nexus Player: https://chromium-swarm.appspot.com/task?id=353ffc638e202210 https://chromium-swarm.appspot.com/task?id=353ff5e35819ab10 Original change's description: > 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: I9f45a03d4055e0ad87c01e1d826287695096e609 > Reviewed-on: https://skia-review.googlesource.com/10941 > Commit-Queue: Brian Salomon > Reviewed-by: Brian Salomon > TBR=bsalomon@google.com,ericrk@chromium.org,reviews@skia.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I0fd6ca95bbc342f21978783b0103073179017795 Reviewed-on: https://skia-review.googlesource.com/11016 Reviewed-by: Mike Klein Commit-Queue: Mike Klein --- include/gpu/GrCaps.h | 7 +- src/gpu/GrRenderTargetContext.cpp | 34 ++------ src/gpu/gl/GrGLCaps.cpp | 32 +------- src/gpu/gl/GrGLCaps.h | 3 +- src/gpu/gl/GrGLGpu.cpp | 19 ++--- src/gpu/vk/GrVkCaps.cpp | 7 +- src/gpu/vk/GrVkCaps.h | 3 +- tests/BlendTest.cpp | 130 +----------------------------- tools/gpu/GrTest.cpp | 3 +- 9 files changed, 23 insertions(+), 215 deletions(-) diff --git a/include/gpu/GrCaps.h b/include/gpu/GrCaps.h index a7096e3fd9..239d4a4ea9 100644 --- a/include/gpu/GrCaps.h +++ b/include/gpu/GrCaps.h @@ -189,12 +189,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; protected: /** Subclasses must call this at the end of their constructors in order to apply caps diff --git a/src/gpu/GrRenderTargetContext.cpp b/src/gpu/GrRenderTargetContext.cpp index 49482ed8ed..3061074a08 100644 --- a/src/gpu/GrRenderTargetContext.cpp +++ b/src/gpu/GrRenderTargetContext.cpp @@ -1751,11 +1751,10 @@ void GrRenderTargetContext::setupDstTexture(GrRenderTarget* rt, const GrClip& cl clip.getConservativeBounds(rt->width(), rt->height(), ©Rect); 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 (!clippedRect.intersect(copyRect, drawIBounds)) { + if (!copyRect.intersect(drawIBounds)) { #ifdef SK_DEBUG GrCapsDebugf(this->caps(), "Missed an early reject. " "Bailing on draw from setupDstTexture.\n"); @@ -1766,43 +1765,24 @@ 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; - bool rectsMustMatch = false; - bool disallowSubrect = false; - if (!this->caps()->initDescForDstCopy(rt, &desc, &rectsMustMatch, &disallowSubrect)) { + if (!this->caps()->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(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(dstOffset); + dstTexture->setOffset(copyRect.fLeft, copyRect.fTop); } diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index 6b3e3a72dd..06085b5e86 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -1018,7 +1018,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")) { @@ -1027,8 +1026,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) { @@ -2071,14 +2069,7 @@ void GrGLCaps::initConfigTable(const GrContextOptions& contextOptions, #endif } -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; - +bool GrGLCaps::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->isConfigRenderable(src->config(), false)) { @@ -2099,20 +2090,7 @@ 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; - 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) { + if (this->blitFramebufferSupportFlags() & kNoScalingOrMirroring_BlitFramebufferFlag) { originForBlitFramebuffer = src->origin(); } @@ -2123,8 +2101,6 @@ 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; @@ -2137,8 +2113,6 @@ 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; diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h index b8c4745a50..a659435784 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -359,8 +359,7 @@ public: return fRGBAToBGRAReadbackConversionsAreSlow; } - bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, bool* rectsMustMatch, - bool* disallowSubrect) const override; + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override; private: enum ExternalFormatUsage { diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index 714ded0e7b..17572a9bb3 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -3344,13 +3344,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) { @@ -3369,13 +3364,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; diff --git a/src/gpu/vk/GrVkCaps.cpp b/src/gpu/vk/GrVkCaps.cpp index bf7e5ad3ce..bf343a9ae9 100644 --- a/src/gpu/vk/GrVkCaps.cpp +++ b/src/gpu/vk/GrVkCaps.cpp @@ -53,12 +53,7 @@ GrVkCaps::GrVkCaps(const GrContextOptions& contextOptions, const GrVkInterface* this->init(contextOptions, vkInterface, physDev, featureFlags, extensionFlags); } -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; - +bool GrVkCaps::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. diff --git a/src/gpu/vk/GrVkCaps.h b/src/gpu/vk/GrVkCaps.h index 0fb95247f2..73ed367896 100644 --- a/src/gpu/vk/GrVkCaps.h +++ b/src/gpu/vk/GrVkCaps.h @@ -104,8 +104,7 @@ public: return fPreferedStencilFormat; } - bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc, - bool* rectsMustMatch, bool* disallowSubrect) const override; + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override; private: enum VkVendor { diff --git a/tests/BlendTest.cpp b/tests/BlendTest.cpp index 8ac8fea838..785acb8681 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,116 +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(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 testCases; - - for (int sampleCnt : {0, 4, 8}) { - 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 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(); - 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 6949f6fd3d..28c1738a0d 100644 --- a/tools/gpu/GrTest.cpp +++ b/tools/gpu/GrTest.cpp @@ -272,8 +272,7 @@ 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, bool* rectsMustMatch, - bool* disallowSubrect) const override { + bool initDescForDstCopy(const GrRenderTarget* src, GrSurfaceDesc* desc) const override { return false; }