From be5947c2f38a79b7c709accfb1047d8fd06a0227 Mon Sep 17 00:00:00 2001 From: Brian Salomon Date: Mon, 19 Mar 2018 18:42:25 +0000 Subject: [PATCH] Revert "New read pixels implementation that is simpler but does all conversions on CPU." This reverts commit c9a642edf2d1c7f5380fe829adbb1a692f9969a6. Reason for revert: 1010102 gms broke Original change's description: > New read pixels implementation that is simpler but does all conversions on CPU. > > Change-Id: Ia548cd24a8544b35a233311706faf48de353b7cf > Reviewed-on: https://skia-review.googlesource.com/109902 > Commit-Queue: Brian Salomon > Reviewed-by: Robert Phillips TBR=bsalomon@google.com,robertphillips@google.com,brianosman@google.com Change-Id: I7724a6eef79885ba2a32c1ac871e5b2a9a3c0c12 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/115140 Reviewed-by: Brian Salomon Commit-Queue: Brian Salomon --- include/gpu/GrCaps.h | 19 +-- ...5PPYH-GPU-IntelHD405-x86_64-Debug-All.json | 4 - ...-IntelHD405-x86_64-Release-All-Vulkan.json | 4 - ...-IntelIris640-x86_64-Debug-All-Vulkan.json | 4 - ...HE-GPU-IntelBayTrail-x86_64-Debug-All.json | 4 - infra/bots/recipes/test.py | 2 - src/core/SkConvertPixels.cpp | 24 --- src/gpu/GrContext.cpp | 151 +----------------- src/gpu/GrContextPriv.h | 10 +- src/gpu/GrGpu.h | 9 -- src/gpu/gl/GrGLCaps.cpp | 49 +----- src/gpu/gl/GrGLCaps.h | 5 +- src/gpu/mock/GrMockCaps.h | 3 +- src/gpu/mtl/GrMtlCaps.h | 3 +- src/gpu/vk/GrVkCaps.h | 3 +- tests/PackedConfigsTextureTest.cpp | 19 +-- tests/ReadWriteAlphaTest.cpp | 5 - tests/SRGBReadWritePixelsTest.cpp | 29 +--- 18 files changed, 34 insertions(+), 313 deletions(-) diff --git a/include/gpu/GrCaps.h b/include/gpu/GrCaps.h index 02a51a092d..27ad6a164d 100644 --- a/include/gpu/GrCaps.h +++ b/include/gpu/GrCaps.h @@ -186,15 +186,7 @@ public: * 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 surface. */ - virtual bool surfaceSupportsWritePixels(const GrSurface*) const = 0; - - /** - * Backends may have restrictions on what types of surfaces support GrGpu::readPixels(). - * If this returns false then the caller should implement a fallback where a temporary texture - * is created, the surface is drawn or copied into the temporary, and pixels are read from the - * temporary. - */ - virtual bool surfaceSupportsReadPixels(const GrSurface*) const = 0; + virtual bool surfaceSupportsWritePixels(const GrSurface* surface) const = 0; /** * Given a dst pixel config and a src color type what color type must the caller coax the @@ -205,15 +197,6 @@ public: return GrPixelConfigToColorType(config); } - /** - * Given a src pixel config and a dst color type what color type must the caller read to using - * GrGpu::readPixels() and then coax into dstColorType. - */ - virtual GrColorType supportedReadPixelsColorType(GrPixelConfig config, - GrColorType /*dstColorType*/) const { - return GrPixelConfigToColorType(config); - } - bool suppressPrints() const { return fSuppressPrints; } size_t bufferMapThreshold() const { diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Debug-All.json b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Debug-All.json index c89b44b32d..33a33b7f63 100644 --- a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Debug-All.json +++ b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Debug-All.json @@ -282,10 +282,6 @@ "gm", "_", "dftext_blob_persp", - "gltestthreading", - "gm", - "_", - "orientation", "_", "svg", "_", diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Release-All-Vulkan.json b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Release-All-Vulkan.json index ba4c599879..624027c557 100644 --- a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Release-All-Vulkan.json +++ b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Release-All-Vulkan.json @@ -279,10 +279,6 @@ "gm", "_", "dftext_blob_persp", - "gltestthreading", - "gm", - "_", - "orientation", "_", "svg", "_", diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC7i5BNK-GPU-IntelIris640-x86_64-Debug-All-Vulkan.json b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC7i5BNK-GPU-IntelIris640-x86_64-Debug-All-Vulkan.json index c6eb1c471d..52fce15262 100644 --- a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC7i5BNK-GPU-IntelIris640-x86_64-Debug-All-Vulkan.json +++ b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC7i5BNK-GPU-IntelIris640-x86_64-Debug-All-Vulkan.json @@ -279,10 +279,6 @@ "gm", "_", "dftext_blob_persp", - "gltestthreading", - "gm", - "_", - "orientation", "_", "svg", "_", diff --git a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUCDE3815TYKHE-GPU-IntelBayTrail-x86_64-Debug-All.json b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUCDE3815TYKHE-GPU-IntelBayTrail-x86_64-Debug-All.json index 952a27a8c4..6bfa088a95 100644 --- a/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUCDE3815TYKHE-GPU-IntelBayTrail-x86_64-Debug-All.json +++ b/infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUCDE3815TYKHE-GPU-IntelBayTrail-x86_64-Debug-All.json @@ -282,10 +282,6 @@ "gm", "_", "dftext_blob_persp", - "gltestthreading", - "gm", - "_", - "orientation", "_", "svg", "_", diff --git a/infra/bots/recipes/test.py b/infra/bots/recipes/test.py index 910747b3f0..78c02802f6 100644 --- a/infra/bots/recipes/test.py +++ b/infra/bots/recipes/test.py @@ -178,8 +178,6 @@ def dm_flags(api, bot): blacklist('gltestthreading gm _ savelayer_with_backdrop') blacklist('gltestthreading gm _ persp_shaders_bw') blacklist('gltestthreading gm _ dftext_blob_persp') - # skbug.com/7523 - Flaky on various GPUs - blacklist('gltestthreading gm _ orientation') # The following devices do not support glessrgb. if 'glessrgb' in configs: diff --git a/src/core/SkConvertPixels.cpp b/src/core/SkConvertPixels.cpp index 262abb7038..725730a1e4 100644 --- a/src/core/SkConvertPixels.cpp +++ b/src/core/SkConvertPixels.cpp @@ -184,30 +184,6 @@ static void convert_to_alpha8(uint8_t* dst, size_t dstRB, const SkImageInfo& src } break; } - case kRGBA_1010102_SkColorType: { - auto src32 = (const uint32_t*) src; - for (int y = 0; y < srcInfo.height(); y++) { - for (int x = 0; x < srcInfo.width(); x++) { - switch (src32[x] >> 30) { - case 0: - dst[x] = 0; - break; - case 1: - dst[x] = 0x55; - break; - case 2: - dst[x] = 0xAA; - break; - case 3: - dst[x] = 0xFF; - break; - } - } - dst = SkTAddOffset(dst, dstRB); - src32 = SkTAddOffset(src32, srcRB); - } - break; - } case kARGB_4444_SkColorType: { auto src16 = (const uint16_t*) src; for (int y = 0; y < srcInfo.height(); y++) { diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp index 626b3f6ecd..f9c941565c 100644 --- a/src/gpu/GrContext.cpp +++ b/src/gpu/GrContext.cpp @@ -26,7 +26,6 @@ #include "GrTextureContext.h" #include "GrTextureStripAtlas.h" #include "GrTracing.h" -#include "SkAutoPixmapStorage.h" #include "SkConvertPixels.h" #include "SkDeferredDisplayList.h" #include "SkGr.h" @@ -612,10 +611,6 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src, int left, int top, int height, GrColorType dstColorType, SkColorSpace* dstColorSpace, void* buffer, size_t rowBytes, uint32_t flags) { -#ifndef SK_LEGACY_GPU_PIXEL_OPS - return this->readSurfacePixels2(src, left, top, width, height, dstColorType, dstColorSpace, - buffer, rowBytes, flags); -#endif // TODO: Color space conversion ASSERT_SINGLE_OWNER_PRIV @@ -623,10 +618,6 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src, int left, int top, SkASSERT(src); ASSERT_OWNED_PROXY_PRIV(src->asSurfaceProxy()); GR_CREATE_TRACE_MARKER_CONTEXT("GrContextPriv", "readSurfacePixels", fContext); - SkASSERT(!(flags & kDontFlush_PixelOpsFlag)); - if (flags & kDontFlush_PixelOpsFlag) { - return false; - } // MDB TODO: delay this instantiation until later in the method if (!src->asSurfaceProxy()->instantiate(this->resourceProvider())) { @@ -676,7 +667,7 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src, int left, int top, return false; } - if (srcSurface->surfacePriv().hasPendingWrite()) { + if (!(kDontFlush_PixelOpsFlag & flags) && srcSurface->surfacePriv().hasPendingWrite()) { this->flush(nullptr); // MDB TODO: tighten this } @@ -808,12 +799,6 @@ bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top } if (!fContext->caps()->surfaceSupportsWritePixels(dstSurface)) { - // We don't expect callers that are skipping flushes to require an intermediate draw. - SkASSERT(!(pixelOpsFlags & kDontFlush_PixelOpsFlag)); - if (pixelOpsFlags & kDontFlush_PixelOpsFlag) { - return false; - } - GrSurfaceDesc desc; desc.fConfig = dstProxy->config(); desc.fWidth = width; @@ -886,7 +871,7 @@ bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top 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 = dstSurface->height() - top - height; + top = dstProxy->height() - top - height; } } else if (dstProxy->origin() == kBottomLeft_GrSurfaceOrigin) { size_t trimRowBytes = GrColorTypeBytesPerPixel(srcColorType) * width; @@ -898,7 +883,7 @@ bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top } buffer = tempBuffer.get(); rowBytes = trimRowBytes; - top = dstSurface->height() - top - height; + top = dstProxy->height() - top - height; } if (!(kDontFlush_PixelOpsFlag & pixelOpsFlags) && dstSurface->surfacePriv().hasPendingIO()) { @@ -909,136 +894,6 @@ bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top rowBytes); } -bool GrContextPriv::readSurfacePixels2(GrSurfaceContext* src, int left, int top, int width, - int height, GrColorType dstColorType, - SkColorSpace* dstColorSpace, void* buffer, size_t rowBytes, - uint32_t flags) { - SkASSERT(!(flags & kDontFlush_PixelOpsFlag)); - if (flags & kDontFlush_PixelOpsFlag){ - return false; - } - ASSERT_SINGLE_OWNER_PRIV - RETURN_FALSE_IF_ABANDONED_PRIV - SkASSERT(src); - SkASSERT(buffer); - ASSERT_OWNED_PROXY_PRIV(src->asSurfaceProxy()); - GR_CREATE_TRACE_MARKER_CONTEXT("GrContextPriv", "readSurfacePixels2", fContext); - - // MDB TODO: delay this instantiation until later in the method - if (!src->asSurfaceProxy()->instantiate(this->resourceProvider())) { - return false; - } - - GrSurfaceProxy* srcProxy = src->asSurfaceProxy(); - GrSurface* srcSurface = srcProxy->priv().peekSurface(); - - if (!GrSurfacePriv::AdjustReadPixelParams(srcSurface->width(), srcSurface->height(), - GrColorTypeBytesPerPixel(dstColorType), &left, &top, - &width, &height, &buffer, &rowBytes)) { - return false; - } - - // TODO: Make GrSurfaceContext know its alpha type and pass dst buffer's alpha type. - bool unpremul = SkToBool(kUnpremul_PixelOpsFlag & flags); - bool convert = unpremul; - - if (!valid_pixel_conversion(dstColorType, srcProxy->config(), unpremul)) { - return false; - } - - if (!fContext->caps()->surfaceSupportsReadPixels(srcSurface)) { - GrSurfaceDesc desc; - desc.fConfig = srcProxy->config(); - desc.fWidth = width; - desc.fHeight = height; - desc.fSampleCnt = 1; - auto tempProxy = this->proxyProvider()->createProxy( - desc, kTopLeft_GrSurfaceOrigin, SkBackingFit::kApprox, SkBudgeted::kYes); - if (!tempProxy) { - return false; - } - auto tempCtx = this->drawingManager()->makeTextureContext( - tempProxy, src->colorSpaceInfo().refColorSpace()); - if (!tempCtx) { - return false; - } - if (!tempCtx->copy(srcProxy, {left, top, width, height}, {0, 0})) { - return false; - } - return this->readSurfacePixels2(tempCtx.get(), 0, 0, width, height, dstColorType, - dstColorSpace, buffer, rowBytes, flags); - } - - bool flip = srcProxy->origin() == kBottomLeft_GrSurfaceOrigin; - if (flip) { - top = srcSurface->height() - top - height; - } - - GrColorType allowedColorType = - fContext->caps()->supportedReadPixelsColorType(srcProxy->config(), dstColorType); - convert = convert || (dstColorType != allowedColorType); - - if (!src->colorSpaceInfo().colorSpace()) { - // "Legacy" mode - no color space conversions. - dstColorSpace = nullptr; - } - convert = convert || !SkColorSpace::Equals(dstColorSpace, src->colorSpaceInfo().colorSpace()); - - SkAutoPixmapStorage tempPixmap; - SkPixmap finalPixmap; - if (convert) { - SkColorType srcSkColorType = GrColorTypeToSkColorType(allowedColorType); - SkColorType dstSkColorType = GrColorTypeToSkColorType(dstColorType); - if (kUnknown_SkColorType == srcSkColorType || kUnknown_SkColorType == dstSkColorType) { - return false; - } - auto tempAT = SkColorTypeIsAlwaysOpaque(srcSkColorType) ? kOpaque_SkAlphaType - : kPremul_SkAlphaType; - auto tempII = SkImageInfo::Make(width, height, srcSkColorType, tempAT, - src->colorSpaceInfo().refColorSpace()); - SkASSERT(!unpremul || !SkColorTypeIsAlwaysOpaque(dstSkColorType)); - auto finalAT = SkColorTypeIsAlwaysOpaque(srcSkColorType) - ? kOpaque_SkAlphaType - : unpremul ? kUnpremul_SkAlphaType : kPremul_SkAlphaType; - auto finalII = - SkImageInfo::Make(width, height, dstSkColorType, finalAT, sk_ref_sp(dstColorSpace)); - if (!SkImageInfoValidConversion(finalII, tempII)) { - return false; - } - tempPixmap.alloc(tempII); - finalPixmap.reset(finalII, buffer, rowBytes); - buffer = tempPixmap.writable_addr(); - rowBytes = tempPixmap.rowBytes(); - } - - if (srcSurface->surfacePriv().hasPendingWrite()) { - this->flush(nullptr); // MDB TODO: tighten this - } - - if (!fContext->fGpu->readPixels(srcSurface, left, top, width, height, allowedColorType, buffer, - rowBytes)) { - return false; - } - - if (flip) { - size_t trimRowBytes = GrColorTypeBytesPerPixel(allowedColorType) * width; - std::unique_ptr row(new char[trimRowBytes]); - char* upper = reinterpret_cast(buffer); - char* lower = reinterpret_cast(buffer) + (height - 1) * rowBytes; - for (int y = 0; y < height / 2; ++y, upper += rowBytes, lower -= rowBytes) { - memcpy(row.get(), upper, trimRowBytes); - memcpy(upper, lower, trimRowBytes); - memcpy(lower, row.get(), trimRowBytes); - } - } - if (convert) { - if (!tempPixmap.readPixels(finalPixmap)) { - return false; - } - } - return true; -} - 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 61ff3c1e34..f7bd911b1e 100644 --- a/src/gpu/GrContextPriv.h +++ b/src/gpu/GrContextPriv.h @@ -128,16 +128,13 @@ public: }; /** - * Reads a rectangle of pixels from a surface. There are currently two versions of this. - * readSurfacePixels() is the older version which will be replaced by the more robust and - * maintainable (but perhaps slower) readSurfacePixels2(). - * + * Reads a rectangle of pixels from a surface. * @param src the surface context to read from. * @param left left edge of the rectangle to read (inclusive) * @param top top edge of the rectangle to read (inclusive) * @param width width of rectangle to read in pixels. * @param height height of rectangle to read in pixels. - * @param dstColorType the color type of the destination buffer + * @param dstConfig the pixel config of the destination buffer * @param dstColorSpace color space of the destination buffer * @param buffer memory to read the rectangle into. * @param rowBytes number of bytes bewtween consecutive rows. Zero means rows are tightly @@ -150,9 +147,6 @@ public: bool readSurfacePixels(GrSurfaceContext* src, int left, int top, int width, int height, GrColorType dstColorType, SkColorSpace* dstColorSpace, void* buffer, size_t rowBytes = 0, uint32_t pixelOpsFlags = 0); - bool readSurfacePixels2(GrSurfaceContext* src, int left, int top, int width, int height, - GrColorType dstColorType, SkColorSpace* dstColorSpace, void* buffer, - size_t rowBytes = 0, uint32_t pixelOpsFlags = 0); /** * Writes a rectangle of pixels to a surface. There are currently two versions of this. diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h index 95c5117799..16561418aa 100644 --- a/src/gpu/GrGpu.h +++ b/src/gpu/GrGpu.h @@ -272,15 +272,6 @@ public: */ bool readPixels(GrSurface* surface, GrSurfaceOrigin, int left, int top, int width, int height, GrColorType dstColorType, void* buffer, size_t rowBytes); - /** - * This version of readPixels doesn't take an origin. TODO: Remove origin handling from - * GrGpu::readPixels entirely. - */ - bool readPixels(GrSurface* surface, int left, int top, int width, int height, - GrColorType dstColorType, void* buffer, size_t rowBytes) { - return this->readPixels(surface, kTopLeft_GrSurfaceOrigin, left, top, width, height, - dstColorType, buffer, rowBytes); - } /** * Updates the pixels in a rectangle of a surface. No sRGB/linear conversions are performed. diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index e05a4bb970..0a5c2fe7b7 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -890,6 +890,11 @@ bool GrGLCaps::readPixelsSupported(GrPixelConfig surfaceConfig, return true; } break; + case kInteger_FormatType: + if (GR_GL_RGBA_INTEGER == readFormat && GR_GL_INT == readType) { + return true; + } + break; case kFloat_FormatType: if (GR_GL_RGBA == readFormat && GR_GL_FLOAT == readType) { return true; @@ -1221,8 +1226,7 @@ bool GrGLCaps::getExternalFormat(GrPixelConfig surfaceConfig, GrPixelConfig memo bool memoryIsAlphaOnly = GrPixelConfigIsAlphaOnly(memoryConfig); // We don't currently support moving RGBA data into and out of ALPHA surfaces. It could be - // made to work. However, this is complicated by the use of GL_RED for alpha-only textures but - // is not needed currently. + // made to work in many cases using glPixelStore and what not but is not needed currently. if (surfaceIsAlphaOnly && !memoryIsAlphaOnly) { return false; } @@ -2445,7 +2449,8 @@ bool GrGLCaps::surfaceSupportsWritePixels(const GrSurface* surface) const { return false; } } - } if (auto rt = surface->asRenderTarget()) { + } + if (auto rt = surface->asRenderTarget()) { if (fUseDrawInsteadOfAllRenderTargetWrites) { return false; } @@ -2457,44 +2462,6 @@ bool GrGLCaps::surfaceSupportsWritePixels(const GrSurface* surface) const { return true; } -bool GrGLCaps::surfaceSupportsReadPixels(const GrSurface* surface) const { - if (auto tex = static_cast(surface->asTexture())) { - // We don't support reading pixels directly from EXTERNAL textures as it would require - // binding the texture to a FBO. - if (tex->target() == GR_GL_TEXTURE_EXTERNAL) { - return false; - } - } - return true; -} - -GrColorType GrGLCaps::supportedReadPixelsColorType(GrPixelConfig config, - GrColorType dstColorType) const { - // For now, we mostly report the read back format that is required by the ES spec without - // checking for implementation allowed formats or consider laxer rules in non-ES GL. TODO: Relax - // this as makes sense to increase performance and correctness. - switch (fConfigTable[config].fFormatType) { - case kNormalizedFixedPoint_FormatType: - return GrColorType::kRGBA_8888; - case kFloat_FormatType: - // We cheat a little here and allow F16 read back if the src and dst match. - if (kRGBA_half_GrPixelConfig == config && GrColorType::kRGBA_F16 == dstColorType) { - return GrColorType::kRGBA_F16; - } - if ((kAlpha_half_GrPixelConfig == config || - kAlpha_half_as_Red_GrPixelConfig == config) && - GrColorType::kAlpha_F16 == dstColorType) { - return GrColorType::kAlpha_F16; - } - // And similar for full float RG. - if (kRG_float_GrPixelConfig == config && GrColorType::kRG_F32 == dstColorType) { - return GrColorType::kRG_F32; - } - return GrColorType::kRGBA_F32; - } - return GrColorType::kUnknown; -} - bool GrGLCaps::onIsWindowRectanglesSupportedForRT(const GrBackendRenderTarget& backendRT) const { const GrGLFramebufferInfo* fbInfo = backendRT.getGLFramebufferInfo(); SkASSERT(fbInfo); diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h index eed3ab3a17..5d3fa6a6f6 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -316,9 +316,7 @@ public: /// Use indices or vertices in CPU arrays rather than VBOs for dynamic content. bool useNonVBOVertexAndIndexDynamicData() const { return fUseNonVBOVertexAndIndexDynamicData; } - bool surfaceSupportsWritePixels(const GrSurface*) const override; - bool surfaceSupportsReadPixels(const GrSurface*) const override; - GrColorType supportedReadPixelsColorType(GrPixelConfig, GrColorType) const override; + bool surfaceSupportsWritePixels(const GrSurface* surface) const override; /// Does ReadPixels support reading readConfig pixels from a FBO that is surfaceConfig? bool readPixelsSupported(GrPixelConfig surfaceConfig, @@ -505,6 +503,7 @@ private: enum FormatType { kNormalizedFixedPoint_FormatType, kFloat_FormatType, + kInteger_FormatType, }; struct ReadPixelsFormat { diff --git a/src/gpu/mock/GrMockCaps.h b/src/gpu/mock/GrMockCaps.h index 27d295ebd5..4bcd4599c5 100644 --- a/src/gpu/mock/GrMockCaps.h +++ b/src/gpu/mock/GrMockCaps.h @@ -67,8 +67,7 @@ public: return 0; } - bool surfaceSupportsWritePixels(const GrSurface*) const override { return true; } - bool surfaceSupportsReadPixels(const GrSurface*) const override { return true; } + bool surfaceSupportsWritePixels(const GrSurface* surface) const override { return true; } bool initDescForDstCopy(const GrRenderTargetProxy* src, GrSurfaceDesc* desc, GrSurfaceOrigin*, bool* rectsMustMatch, bool* disallowSubrect) const override { diff --git a/src/gpu/mtl/GrMtlCaps.h b/src/gpu/mtl/GrMtlCaps.h index c43d596e7f..130af67c70 100644 --- a/src/gpu/mtl/GrMtlCaps.h +++ b/src/gpu/mtl/GrMtlCaps.h @@ -31,8 +31,7 @@ public: int getRenderTargetSampleCount(int requestedCount, GrPixelConfig) const override; int maxRenderTargetSampleCount(GrPixelConfig) const override; - bool surfaceSupportsWritePixels(const GrSurface*) const override { return true; } - bool surfaceSupportsReadPixels(const GrSurface*) const override { return true; } + bool surfaceSupportsWritePixels(const GrSurface* surface) 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 881aacc9dc..63bd4e90d4 100644 --- a/src/gpu/vk/GrVkCaps.h +++ b/src/gpu/vk/GrVkCaps.h @@ -40,8 +40,7 @@ public: int getRenderTargetSampleCount(int requestedCount, GrPixelConfig config) const override; int maxRenderTargetSampleCount(GrPixelConfig config) const override; - bool surfaceSupportsWritePixels(const GrSurface*) const override; - bool surfaceSupportsReadPixels(const GrSurface*) const override { return true; } + bool surfaceSupportsWritePixels(const GrSurface* surface) const override; bool isConfigTexturableLinearly(GrPixelConfig config) const { return SkToBool(ConfigInfo::kTextureable_Flag & fConfigTable[config].fLinearFlags); diff --git a/tests/PackedConfigsTextureTest.cpp b/tests/PackedConfigsTextureTest.cpp index 5f18ef3d57..b1fbc77977 100644 --- a/tests/PackedConfigsTextureTest.cpp +++ b/tests/PackedConfigsTextureTest.cpp @@ -97,7 +97,7 @@ static void check_565(skiatest::Reporter* reporter, } static void run_test(skiatest::Reporter* reporter, GrContext* context, int arraySize, - SkColorType colorType) { + GrColorType colorType) { SkTDArray controlPixelData; // We will read back into an 8888 buffer since 565/4444 read backs aren't supported SkTDArray readBuffer; @@ -113,24 +113,19 @@ static void run_test(skiatest::Reporter* reporter, GrContext* context, int array kRGBA_8888_SkColorType, kOpaque_SkAlphaType); for (auto origin : { kTopLeft_GrSurfaceOrigin, kBottomLeft_GrSurfaceOrigin }) { - auto proxy = sk_gpu_test::MakeTextureProxyFromData(context, false, DEV_W, DEV_H, - SkColorTypeToGrColorType(colorType), + auto proxy = sk_gpu_test::MakeTextureProxyFromData(context, false, DEV_W, DEV_H, colorType, origin, controlPixelData.begin(), 0); SkASSERT(proxy); sk_sp sContext = context->contextPriv().makeWrappedSurfaceContext( std::move(proxy)); - if (!sContext->readPixels(dstInfo, readBuffer.begin(), 0, 0, 0)) { - // We only require this to succeed if the format is renderable. - REPORTER_ASSERT(reporter, !context->colorTypeSupportedAsSurface(colorType)); - return; - } + SkAssertResult(sContext->readPixels(dstInfo, readBuffer.begin(), 0, 0, 0)); - if (kARGB_4444_SkColorType == colorType) { + if (GrColorType::kABGR_4444 == colorType) { check_4444(reporter, controlPixelData, readBuffer); } else { - SkASSERT(kRGB_565_SkColorType == colorType); + SkASSERT(GrColorType::kRGB_565 == colorType); check_565(reporter, controlPixelData, readBuffer); } } @@ -139,11 +134,11 @@ static void run_test(skiatest::Reporter* reporter, GrContext* context, int array static const int CONTROL_ARRAY_SIZE = DEV_W * DEV_H; DEF_GPUTEST_FOR_RENDERING_CONTEXTS(RGBA4444TextureTest, reporter, ctxInfo) { - run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, kARGB_4444_SkColorType); + run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, GrColorType::kABGR_4444); } DEF_GPUTEST_FOR_RENDERING_CONTEXTS(RGB565TextureTest, reporter, ctxInfo) { - run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, kRGB_565_SkColorType); + run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, GrColorType::kRGB_565); } #endif diff --git a/tests/ReadWriteAlphaTest.cpp b/tests/ReadWriteAlphaTest.cpp index b82c48fd5d..eaad411d5a 100644 --- a/tests/ReadWriteAlphaTest.cpp +++ b/tests/ReadWriteAlphaTest.cpp @@ -99,11 +99,6 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadWriteAlpha, reporter, ctxInfo) { // read the texture back result = sContext->readPixels(ii, readback.get(), rowBytes, 0, 0); - // We don't require reading from kAlpha_8 to be supported. TODO: At least make this work - // when kAlpha_8 is renderable. - if (!result) { - continue; - } REPORTER_ASSERT(reporter, result, "Initial A8 readPixels failed"); // make sure the original & read back versions match diff --git a/tests/SRGBReadWritePixelsTest.cpp b/tests/SRGBReadWritePixelsTest.cpp index 569490cf7a..18ed738787 100644 --- a/tests/SRGBReadWritePixelsTest.cpp +++ b/tests/SRGBReadWritePixelsTest.cpp @@ -139,8 +139,8 @@ void read_and_check_pixels(skiatest::Reporter* reporter, GrSurfaceContext* conte uint32_t read = readData[j * w + i]; if (!checker(orig, read, error)) { - ERRORF(reporter, "Original 0x%08x, read back as 0x%08x in %s at %d, %d).", orig, - read, subtestName, i, j); + ERRORF(reporter, "Expected 0x%08x, read back as 0x%08x in %s at %d, %d).", + orig, read, subtestName, i, j); return; } } @@ -272,20 +272,13 @@ 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. + // back to sRGB no conversion test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kSRGB, smallError, check_no_conversion, context, reporter); -#ifdef SK_LEGACY_GPU_PIXEL_OPS // Untagged read from sRGB is treated as a conversion back to linear. TODO: Fail or don't // convert? test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kUntagged, error, check_srgb_to_linear_conversion, context, reporter); -#else - // Reading back to untagged should be a pass through with no conversion. - test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kUntagged, error, - check_no_conversion, context, reporter); -#endif - // Converts back to linear test_write_read(Encoding::kSRGB, Encoding::kSRGB, Encoding::kLinear, error, check_srgb_to_linear_conversion, context, reporter); @@ -314,15 +307,9 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) { // are all the same as the above cases where the original data was untagged. test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kSRGB, error, check_linear_to_srgb_conversion, context, reporter); -#ifdef SK_LEGACY_GPU_PIXEL_OPS // TODO: Fail or don't convert? test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kUntagged, error, check_linear_to_srgb_to_linear_conversion, context, reporter); -#else - // When the dst buffer is untagged there should be no conversion on the read. - test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kUntagged, error, - check_linear_to_srgb_conversion, context, reporter); -#endif test_write_read(Encoding::kSRGB, Encoding::kLinear, Encoding::kLinear, error, check_linear_to_srgb_to_linear_conversion, context, reporter); @@ -330,13 +317,13 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) { // 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. + // The read from untagged to sRGB also does no conversion. TODO: Should it just fail? test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kSRGB, error, check_no_conversion, context, reporter); // Reading untagged back as untagged should do no conversion. test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kUntagged, error, check_no_conversion, context, reporter); - // Reading untagged back as linear does no conversion. + // Reading untagged back as linear does no conversion. TODO: Should it just fail? test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kLinear, error, check_no_conversion, context, reporter); } @@ -347,18 +334,18 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) { // converts back to sRGB on read. 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. + // Reading untagged data from linear currently does no conversion. TODO: Should it fail? test_write_read(Encoding::kLinear, Encoding::kSRGB, Encoding::kUntagged, error, check_srgb_to_linear_conversion, context, reporter); // Stays linear when read. test_write_read(Encoding::kLinear, Encoding::kSRGB, Encoding::kLinear, error, check_srgb_to_linear_conversion, context, reporter); -#ifdef SK_LEGACY_GPU_PIXEL_OPS /////////////////////////////////////////////////////////////////////////////////////////////// // 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. test_write_read(Encoding::kLinear, Encoding::kUntagged, Encoding::kSRGB, error, check_linear_to_srgb_conversion, context, reporter); @@ -379,7 +366,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SRGBReadWritePixels, reporter, ctxInfo) { // Reading to sRGB does a conversion. test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kSRGB, error, check_linear_to_srgb_conversion, context, reporter); - // Reading to untagged does no conversion. + // Reading to untagged does no conversion. TODO: Should it fail? test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kUntagged, error, check_no_conversion, context, reporter); // Stays linear when read.