diff options
author | Brian Salomon <bsalomon@google.com> | 2018-03-19 16:06:44 -0400 |
---|---|---|
committer | Skia Commit-Bot <skia-commit-bot@chromium.org> | 2018-03-19 21:17:15 +0000 |
commit | 19eaf2dbe785a06b76f11c2066c302f0aa89d5d2 (patch) | |
tree | 9bf1bcf8fe794da174ba19b1c5cfed84f0f274e5 | |
parent | 56dc04bdc160b71a1e77fdb9e30fde4e860077ae (diff) |
Revert "Revert "New read pixels implementation that is simpler but does all conversions on CPU.""
This reverts commit be5947c2f38a79b7c709accfb1047d8fd06a0227.
Bug: skia:
Change-Id: I06dc15b31042d7827511d0ac2a7f4262c3f09622
Reviewed-on: https://skia-review.googlesource.com/115079
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
-rw-r--r-- | include/gpu/GrCaps.h | 19 | ||||
-rw-r--r-- | infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Debug-All.json | 4 | ||||
-rw-r--r-- | infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC5PPYH-GPU-IntelHD405-x86_64-Release-All-Vulkan.json | 4 | ||||
-rw-r--r-- | infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUC7i5BNK-GPU-IntelIris640-x86_64-Debug-All-Vulkan.json | 4 | ||||
-rw-r--r-- | infra/bots/recipes/test.expected/Test-Ubuntu16-Clang-NUCDE3815TYKHE-GPU-IntelBayTrail-x86_64-Debug-All.json | 4 | ||||
-rw-r--r-- | infra/bots/recipes/test.py | 2 | ||||
-rw-r--r-- | src/core/SkConvertPixels.cpp | 59 | ||||
-rw-r--r-- | src/gpu/GrContext.cpp | 151 | ||||
-rw-r--r-- | src/gpu/GrContextPriv.h | 10 | ||||
-rw-r--r-- | src/gpu/GrGpu.h | 9 | ||||
-rw-r--r-- | src/gpu/gl/GrGLCaps.cpp | 49 | ||||
-rw-r--r-- | src/gpu/gl/GrGLCaps.h | 5 | ||||
-rw-r--r-- | src/gpu/mock/GrMockCaps.h | 3 | ||||
-rw-r--r-- | src/gpu/mtl/GrMtlCaps.h | 3 | ||||
-rw-r--r-- | src/gpu/vk/GrVkCaps.h | 3 | ||||
-rw-r--r-- | tests/PackedConfigsTextureTest.cpp | 19 | ||||
-rw-r--r-- | tests/ReadWriteAlphaTest.cpp | 5 | ||||
-rw-r--r-- | tests/SRGBReadWritePixelsTest.cpp | 29 |
18 files changed, 347 insertions, 35 deletions
diff --git a/include/gpu/GrCaps.h b/include/gpu/GrCaps.h index 27ad6a164d..02a51a092d 100644 --- a/include/gpu/GrCaps.h +++ b/include/gpu/GrCaps.h @@ -186,7 +186,15 @@ 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* surface) const = 0; + 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; /** * Given a dst pixel config and a src color type what color type must the caller coax the @@ -197,6 +205,15 @@ 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 33a33b7f63..c89b44b32d 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,6 +282,10 @@ "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 624027c557..ba4c599879 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,6 +279,10 @@ "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 52fce15262..c6eb1c471d 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,6 +279,10 @@ "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 6bfa088a95..952a27a8c4 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,6 +282,10 @@ "gm", "_", "dftext_blob_persp", + "gltestthreading", + "gm", + "_", + "orientation", "_", "svg", "_", diff --git a/infra/bots/recipes/test.py b/infra/bots/recipes/test.py index 78c02802f6..910747b3f0 100644 --- a/infra/bots/recipes/test.py +++ b/infra/bots/recipes/test.py @@ -178,6 +178,8 @@ 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 725730a1e4..8a8f2ba153 100644 --- a/src/core/SkConvertPixels.cpp +++ b/src/core/SkConvertPixels.cpp @@ -184,6 +184,30 @@ 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<uint8_t>(dst, dstRB); + src32 = SkTAddOffset<const uint32_t>(src32, srcRB); + } + break; + } case kARGB_4444_SkColorType: { auto src16 = (const uint16_t*) src; for (int y = 0; y < srcInfo.height(); y++) { @@ -225,9 +249,20 @@ static void convert_with_pipeline(const SkImageInfo& dstInfo, void* dstRow, size case kRGBA_8888_SkColorType: pipeline.append(SkRasterPipeline::load_8888, &src); break; + case kRGB_888x_SkColorType: + pipeline.append(SkRasterPipeline::load_8888, &src); + pipeline.append(SkRasterPipeline::force_opaque); + break; case kBGRA_8888_SkColorType: pipeline.append(SkRasterPipeline::load_bgra, &src); break; + case kRGBA_1010102_SkColorType: + pipeline.append(SkRasterPipeline::load_1010102, &src); + break; + case kRGB_101010x_SkColorType: + pipeline.append(SkRasterPipeline::load_1010102, &src); + pipeline.append(SkRasterPipeline::force_opaque); + break; case kRGB_565_SkColorType: pipeline.append(SkRasterPipeline::load_565, &src); break; @@ -325,9 +360,20 @@ static void convert_with_pipeline(const SkImageInfo& dstInfo, void* dstRow, size case kRGBA_8888_SkColorType: pipeline.append(SkRasterPipeline::store_8888, &dst); break; + case kRGB_888x_SkColorType: + pipeline.append(SkRasterPipeline::force_opaque); + pipeline.append(SkRasterPipeline::store_8888, &dst); + break; case kBGRA_8888_SkColorType: pipeline.append(SkRasterPipeline::store_bgra, &dst); break; + case kRGBA_1010102_SkColorType: + pipeline.append(SkRasterPipeline::store_1010102, &dst); + break; + case kRGB_101010x_SkColorType: + pipeline.append(SkRasterPipeline::force_opaque); + pipeline.append(SkRasterPipeline::store_1010102, &dst); + break; case kRGB_565_SkColorType: pipeline.append(SkRasterPipeline::store_565, &dst); break; @@ -345,6 +391,16 @@ static void convert_with_pipeline(const SkImageInfo& dstInfo, void* dstRow, size pipeline.run(0,0, srcInfo.width(), srcInfo.height()); } +static bool swizzle_and_multiply_color_type(SkColorType ct) { + switch (ct) { + case kRGBA_8888_SkColorType: + case kBGRA_8888_SkColorType: + return true; + default: + return false; + } +} + void SkConvertPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB, const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRB, SkColorTable* ctable, SkTransferFunctionBehavior behavior) { @@ -361,7 +417,8 @@ void SkConvertPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB, SkASSERT(srcInfo.colorSpace() || !isColorAware); // Fast Path 2: Simple swizzles and premuls. - if (4 == srcInfo.bytesPerPixel() && 4 == dstInfo.bytesPerPixel() && !isColorAware) { + if (swizzle_and_multiply_color_type(srcInfo.colorType()) && + swizzle_and_multiply_color_type(dstInfo.colorType()) && !isColorAware) { swizzle_and_multiply(dstInfo, dstPixels, dstRB, srcInfo, srcPixels, srcRB); return; } diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp index f9c941565c..626b3f6ecd 100644 --- a/src/gpu/GrContext.cpp +++ b/src/gpu/GrContext.cpp @@ -26,6 +26,7 @@ #include "GrTextureContext.h" #include "GrTextureStripAtlas.h" #include "GrTracing.h" +#include "SkAutoPixmapStorage.h" #include "SkConvertPixels.h" #include "SkDeferredDisplayList.h" #include "SkGr.h" @@ -611,6 +612,10 @@ 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 @@ -618,6 +623,10 @@ 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())) { @@ -667,7 +676,7 @@ bool GrContextPriv::readSurfacePixels(GrSurfaceContext* src, int left, int top, return false; } - if (!(kDontFlush_PixelOpsFlag & flags) && srcSurface->surfacePriv().hasPendingWrite()) { + if (srcSurface->surfacePriv().hasPendingWrite()) { this->flush(nullptr); // MDB TODO: tighten this } @@ -799,6 +808,12 @@ 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; @@ -871,7 +886,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 = dstProxy->height() - top - height; + top = dstSurface->height() - top - height; } } else if (dstProxy->origin() == kBottomLeft_GrSurfaceOrigin) { size_t trimRowBytes = GrColorTypeBytesPerPixel(srcColorType) * width; @@ -883,7 +898,7 @@ bool GrContextPriv::writeSurfacePixels2(GrSurfaceContext* dst, int left, int top } buffer = tempBuffer.get(); rowBytes = trimRowBytes; - top = dstProxy->height() - top - height; + top = dstSurface->height() - top - height; } if (!(kDontFlush_PixelOpsFlag & pixelOpsFlags) && dstSurface->surfacePriv().hasPendingIO()) { @@ -894,6 +909,136 @@ 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<char[]> row(new char[trimRowBytes]); + char* upper = reinterpret_cast<char*>(buffer); + char* lower = reinterpret_cast<char*>(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 f7bd911b1e..61ff3c1e34 100644 --- a/src/gpu/GrContextPriv.h +++ b/src/gpu/GrContextPriv.h @@ -128,13 +128,16 @@ public: }; /** - * Reads a rectangle of pixels from a surface. + * 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(). + * * @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 dstConfig the pixel config of the destination buffer + * @param dstColorType the color type 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 @@ -147,6 +150,9 @@ 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 16561418aa..95c5117799 100644 --- a/src/gpu/GrGpu.h +++ b/src/gpu/GrGpu.h @@ -272,6 +272,15 @@ 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 0a5c2fe7b7..e05a4bb970 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -890,11 +890,6 @@ 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; @@ -1226,7 +1221,8 @@ 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 in many cases using glPixelStore and what not but is not needed currently. + // made to work. However, this is complicated by the use of GL_RED for alpha-only textures but + // is not needed currently. if (surfaceIsAlphaOnly && !memoryIsAlphaOnly) { return false; } @@ -2449,8 +2445,7 @@ bool GrGLCaps::surfaceSupportsWritePixels(const GrSurface* surface) const { return false; } } - } - if (auto rt = surface->asRenderTarget()) { + } if (auto rt = surface->asRenderTarget()) { if (fUseDrawInsteadOfAllRenderTargetWrites) { return false; } @@ -2462,6 +2457,44 @@ bool GrGLCaps::surfaceSupportsWritePixels(const GrSurface* surface) const { return true; } +bool GrGLCaps::surfaceSupportsReadPixels(const GrSurface* surface) const { + if (auto tex = static_cast<const GrGLTexture*>(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 5d3fa6a6f6..eed3ab3a17 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -316,7 +316,9 @@ public: /// Use indices or vertices in CPU arrays rather than VBOs for dynamic content. bool useNonVBOVertexAndIndexDynamicData() const { return fUseNonVBOVertexAndIndexDynamicData; } - bool surfaceSupportsWritePixels(const GrSurface* surface) const override; + bool surfaceSupportsWritePixels(const GrSurface*) const override; + bool surfaceSupportsReadPixels(const GrSurface*) const override; + GrColorType supportedReadPixelsColorType(GrPixelConfig, GrColorType) const override; /// Does ReadPixels support reading readConfig pixels from a FBO that is surfaceConfig? bool readPixelsSupported(GrPixelConfig surfaceConfig, @@ -503,7 +505,6 @@ 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 4bcd4599c5..27d295ebd5 100644 --- a/src/gpu/mock/GrMockCaps.h +++ b/src/gpu/mock/GrMockCaps.h @@ -67,7 +67,8 @@ public: return 0; } - bool surfaceSupportsWritePixels(const GrSurface* surface) const override { return true; } + bool surfaceSupportsWritePixels(const GrSurface*) const override { return true; } + bool surfaceSupportsReadPixels(const GrSurface*) 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 130af67c70..c43d596e7f 100644 --- a/src/gpu/mtl/GrMtlCaps.h +++ b/src/gpu/mtl/GrMtlCaps.h @@ -31,7 +31,8 @@ public: int getRenderTargetSampleCount(int requestedCount, GrPixelConfig) const override; int maxRenderTargetSampleCount(GrPixelConfig) const override; - bool surfaceSupportsWritePixels(const GrSurface* surface) const override { return true; } + bool surfaceSupportsWritePixels(const GrSurface*) const override { return true; } + bool surfaceSupportsReadPixels(const GrSurface*) 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 63bd4e90d4..881aacc9dc 100644 --- a/src/gpu/vk/GrVkCaps.h +++ b/src/gpu/vk/GrVkCaps.h @@ -40,7 +40,8 @@ public: int getRenderTargetSampleCount(int requestedCount, GrPixelConfig config) const override; int maxRenderTargetSampleCount(GrPixelConfig config) const override; - bool surfaceSupportsWritePixels(const GrSurface* surface) const override; + bool surfaceSupportsWritePixels(const GrSurface*) const override; + bool surfaceSupportsReadPixels(const GrSurface*) const override { return true; } bool isConfigTexturableLinearly(GrPixelConfig config) const { return SkToBool(ConfigInfo::kTextureable_Flag & fConfigTable[config].fLinearFlags); diff --git a/tests/PackedConfigsTextureTest.cpp b/tests/PackedConfigsTextureTest.cpp index b1fbc77977..5f18ef3d57 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, - GrColorType colorType) { + SkColorType colorType) { SkTDArray<uint16_t> controlPixelData; // We will read back into an 8888 buffer since 565/4444 read backs aren't supported SkTDArray<GrColor> readBuffer; @@ -113,19 +113,24 @@ 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, colorType, + auto proxy = sk_gpu_test::MakeTextureProxyFromData(context, false, DEV_W, DEV_H, + SkColorTypeToGrColorType(colorType), origin, controlPixelData.begin(), 0); SkASSERT(proxy); sk_sp<GrSurfaceContext> sContext = context->contextPriv().makeWrappedSurfaceContext( std::move(proxy)); - SkAssertResult(sContext->readPixels(dstInfo, readBuffer.begin(), 0, 0, 0)); + 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; + } - if (GrColorType::kABGR_4444 == colorType) { + if (kARGB_4444_SkColorType == colorType) { check_4444(reporter, controlPixelData, readBuffer); } else { - SkASSERT(GrColorType::kRGB_565 == colorType); + SkASSERT(kRGB_565_SkColorType == colorType); check_565(reporter, controlPixelData, readBuffer); } } @@ -134,11 +139,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, GrColorType::kABGR_4444); + run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, kARGB_4444_SkColorType); } DEF_GPUTEST_FOR_RENDERING_CONTEXTS(RGB565TextureTest, reporter, ctxInfo) { - run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, GrColorType::kRGB_565); + run_test(reporter, ctxInfo.grContext(), CONTROL_ARRAY_SIZE, kRGB_565_SkColorType); } #endif diff --git a/tests/ReadWriteAlphaTest.cpp b/tests/ReadWriteAlphaTest.cpp index eaad411d5a..b82c48fd5d 100644 --- a/tests/ReadWriteAlphaTest.cpp +++ b/tests/ReadWriteAlphaTest.cpp @@ -99,6 +99,11 @@ 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 18ed738787..569490cf7a 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, "Expected 0x%08x, read back as 0x%08x in %s at %d, %d).", - orig, read, subtestName, i, j); + ERRORF(reporter, "Original 0x%08x, read back as 0x%08x in %s at %d, %d).", orig, + read, subtestName, i, j); return; } } @@ -272,13 +272,20 @@ 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); @@ -307,9 +314,15 @@ 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); @@ -317,13 +330,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. TODO: Should it just fail? + // The read from untagged to sRGB also does no conversion. 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. TODO: Should it just fail? + // Reading untagged back as linear does no conversion. test_write_read(Encoding::kUntagged, writeEncoding, Encoding::kLinear, error, check_no_conversion, context, reporter); } @@ -334,18 +347,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. TODO: Should it fail? + // Reading untagged data from linear currently does no conversion. 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); @@ -366,7 +379,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. TODO: Should it fail? + // Reading to untagged does no conversion. test_write_read(Encoding::kLinear, Encoding::kLinear, Encoding::kUntagged, error, check_no_conversion, context, reporter); // Stays linear when read. |