From e44bfee96e97e9d530452e8b4e07dfc1e48dd090 Mon Sep 17 00:00:00 2001 From: Brian Salomon Date: Mon, 15 Jul 2019 17:05:05 -0400 Subject: [PATCH] Remove GrGLGpu/GrGLCaps readPixelsSupported. This code checks the external format/type that will be used with glReadPixels. It tests for the values inherently allowed by the GL and for implementation values. It would need to be refactored to use formats and color types rather than GrPixelConfig. However, the code that calls GrGpu::readPixels and GrGpu::transferPixelsFrom already tests GrCaps::supportedReadPixelsColorType. The GrGLCaps override only allows format/type combinations allowed by ES without implementation queries, which is less permissive than the deleted code. We may get more permissive again but will probably follow a different pattern and do this after the format/colortype tables are built out in GrGLCaps. Bug: skia:6718 Change-Id: I36d9ab496dfe71045dcbce84200a9de3e93440bf Reviewed-on: https://skia-review.googlesource.com/c/skia/+/226840 Commit-Queue: Brian Salomon Reviewed-by: Greg Daniel --- src/gpu/gl/GrGLCaps.cpp | 73 ----------------------------------------- src/gpu/gl/GrGLCaps.h | 7 ---- src/gpu/gl/GrGLGpu.cpp | 72 ---------------------------------------- src/gpu/gl/GrGLGpu.h | 14 -------- 4 files changed, 166 deletions(-) diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index 979d544e7f..802586e7c4 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -969,79 +969,6 @@ bool GrGLCaps::hasPathRenderingSupport(const GrGLContextInfo& ctxInfo, const GrG return true; } -bool GrGLCaps::readPixelsSupported(GrPixelConfig surfaceConfig, - GrPixelConfig readConfig, - std::function getIntegerv, - std::function bindRenderTarget, - std::function unbindRenderTarget) const { - // If it's not possible to even have a color attachment of surfaceConfig then read pixels is - // not supported regardless of readConfig. - if (!this->canConfigBeFBOColorAttachment(surfaceConfig)) { - return false; - } - - GrGLenum readFormat; - GrGLenum readType; - if (!this->getReadPixelsFormat(surfaceConfig, readConfig, &readFormat, &readType)) { - return false; - } - - if (GR_IS_GR_GL(fStandard)) { - // Some OpenGL implementations allow GL_ALPHA as a format to glReadPixels. However, - // the manual (https://www.opengl.org/sdk/docs/man/) says only these formats are allowed: - // GL_STENCIL_INDEX, GL_DEPTH_COMPONENT, GL_DEPTH_STENCIL, GL_RED, GL_GREEN, GL_BLUE, - // GL_RGB, GL_BGR, GL_RGBA, and GL_BGRA. We check for the subset that we would use. - // The manual does not seem to fully match the spec as the spec allows integer formats - // when the bound color buffer is an integer buffer. It doesn't specify which integer - // formats are allowed, so perhaps all of them are. We only use GL_RGBA_INTEGER currently. - if (readFormat != GR_GL_RED && readFormat != GR_GL_RG && readFormat != GR_GL_RGB && - readFormat != GR_GL_RGBA && readFormat != GR_GL_BGRA && - readFormat != GR_GL_RGBA_INTEGER) { - return false; - } - // There is also a set of allowed types, but all the types we use are in the set: - // GL_UNSIGNED_BYTE, GL_BYTE, GL_UNSIGNED_SHORT, GL_SHORT, GL_UNSIGNED_INT, GL_INT, - // GL_HALF_FLOAT, GL_FLOAT, GL_UNSIGNED_BYTE_3_3_2, GL_UNSIGNED_BYTE_2_3_3_REV, - // GL_UNSIGNED_SHORT_5_6_5, GL_UNSIGNED_SHORT_5_6_5_REV, GL_UNSIGNED_SHORT_4_4_4_4, - // GL_UNSIGNED_SHORT_4_4_4_4_REV, GL_UNSIGNED_SHORT_5_5_5_1, GL_UNSIGNED_SHORT_1_5_5_5_REV, - // GL_UNSIGNED_INT_8_8_8_8, GL_UNSIGNED_INT_8_8_8_8_REV,GL_UNSIGNED_INT_10_10_10_2, - // GL_UNSIGNED_INT_2_10_10_10_REV, GL_UNSIGNED_INT_24_8, GL_UNSIGNED_INT_10F_11F_11F_REV, - // GL_UNSIGNED_INT_5_9_9_9_REV, or GL_FLOAT_32_UNSIGNED_INT_24_8_REV. - return true; - } - - // See Section 16.1.2 in the ES 3.2 specification. - switch (fConfigTable[surfaceConfig].fFormatType) { - case kNormalizedFixedPoint_FormatType: - if (GR_GL_RGBA == readFormat && GR_GL_UNSIGNED_BYTE == readType) { - return true; - } - break; - case kFloat_FormatType: - if (GR_GL_RGBA == readFormat && GR_GL_FLOAT == readType) { - return true; - } - break; - } - - if (0 == fConfigTable[surfaceConfig].fSecondReadPixelsFormat.fFormat) { - ReadPixelsFormat* rpFormat = - const_cast(&fConfigTable[surfaceConfig].fSecondReadPixelsFormat); - GrGLint format = 0, type = 0; - if (!bindRenderTarget()) { - return false; - } - getIntegerv(GR_GL_IMPLEMENTATION_COLOR_READ_FORMAT, &format); - getIntegerv(GR_GL_IMPLEMENTATION_COLOR_READ_TYPE, &type); - rpFormat->fFormat = format; - rpFormat->fType = type; - unbindRenderTarget(); - } - - return fConfigTable[surfaceConfig].fSecondReadPixelsFormat.fFormat == readFormat && - fConfigTable[surfaceConfig].fSecondReadPixelsFormat.fType == readType; -} - void GrGLCaps::initFSAASupport(const GrContextOptions& contextOptions, const GrGLContextInfo& ctxInfo, const GrGLInterface* gli) { // We need dual source blending and the ability to disable multisample in order to support mixed diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h index d2b08c5a7d..839a4fa85a 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -337,13 +337,6 @@ public: SupportedRead supportedReadPixelsColorType(GrPixelConfig, const GrBackendFormat&, GrColorType) const override; - /// Does ReadPixels support reading readConfig pixels from a FBO that is surfaceConfig? - bool readPixelsSupported(GrPixelConfig surfaceConfig, - GrPixelConfig readConfig, - std::function getIntegerv, - std::function bindRenderTarget, - std::function unbindRenderTarget) const; - bool isCoreProfile() const { return fIsCoreProfile; } bool bindFragDataLocationSupport() const { return fBindFragDataLocationSupport; } diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index 6a82183b00..fa92dff07f 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -2203,74 +2203,6 @@ void GrGLGpu::clearStencilClip(const GrFixedClip& clip, fHWStencilSettings.invalidate(); } -bool GrGLGpu::readPixelsSupported(GrRenderTarget* target, GrPixelConfig readConfig) { -#ifdef SK_BUILD_FOR_MAC - // Chromium may ask us to read back from locked IOSurfaces. Calling the command buffer's - // glGetIntegerv() with GL_IMPLEMENTATION_COLOR_READ_FORMAT/_TYPE causes the command buffer - // to make a call to check the framebuffer status which can hang the driver. So in Mac Chromium - // we always use a temporary surface to test for read pixels support. - // https://www.crbug.com/662802 - if (this->glContext().driver() == kChromium_GrGLDriver) { - return this->readPixelsSupported(target->config(), readConfig); - } -#endif - auto bindRenderTarget = [this, target]() -> bool { - this->flushRenderTargetNoColorWrites(static_cast(target)); - return true; - }; - auto unbindRenderTarget = []{}; - auto getIntegerv = [this](GrGLenum query, GrGLint* value) { - GR_GL_GetIntegerv(this->glInterface(), query, value); - }; - GrPixelConfig rtConfig = target->config(); - return this->glCaps().readPixelsSupported(rtConfig, readConfig, getIntegerv, bindRenderTarget, - unbindRenderTarget); -} - -bool GrGLGpu::readPixelsSupported(GrPixelConfig rtConfig, GrPixelConfig readConfig) { - sk_sp temp; - auto bindRenderTarget = [this, rtConfig, &temp]() -> bool { - GrSurfaceDesc desc; - desc.fConfig = rtConfig; - desc.fWidth = desc.fHeight = 16; - if (this->glCaps().isConfigRenderable(rtConfig)) { - desc.fFlags = kRenderTarget_GrSurfaceFlag; - temp = this->createTexture(desc, SkBudgeted::kNo); - if (!temp) { - return false; - } - GrGLRenderTarget* glrt = static_cast(temp->asRenderTarget()); - this->flushRenderTargetNoColorWrites(glrt); - return true; - } else if (this->glCaps().canConfigBeFBOColorAttachment(rtConfig)) { - temp = this->createTexture(desc, SkBudgeted::kNo); - if (!temp) { - return false; - } - this->bindSurfaceFBOForPixelOps(temp.get(), GR_GL_FRAMEBUFFER, kDst_TempFBOTarget); - fHWBoundRenderTargetUniqueID.makeInvalid(); - return true; - } - return false; - }; - auto unbindRenderTarget = [this, &temp]() { - this->unbindTextureFBOForPixelOps(GR_GL_FRAMEBUFFER, temp.get()); - }; - auto getIntegerv = [this](GrGLenum query, GrGLint* value) { - GR_GL_GetIntegerv(this->glInterface(), query, value); - }; - return this->glCaps().readPixelsSupported(rtConfig, readConfig, getIntegerv, bindRenderTarget, - unbindRenderTarget); -} - -bool GrGLGpu::readPixelsSupported(GrSurface* surfaceForConfig, GrPixelConfig readConfig) { - if (GrRenderTarget* rt = surfaceForConfig->asRenderTarget()) { - return this->readPixelsSupported(rt, readConfig); - } else { - GrPixelConfig config = surfaceForConfig->config(); - return this->readPixelsSupported(config, readConfig); - } -} bool GrGLGpu::readOrTransferPixelsFrom(GrSurface* surface, int left, int top, int width, int height, GrColorType dstColorType, void* offsetOrPtr, @@ -2285,10 +2217,6 @@ bool GrGLGpu::readOrTransferPixelsFrom(GrSurface* surface, int left, int top, in // TODO: Avoid this conversion by making GrGLCaps work with color types. auto dstAsConfig = GrColorTypeToPixelConfig(dstColorType); - if (!this->readPixelsSupported(surface, dstAsConfig)) { - return false; - } - GrGLenum externalFormat; GrGLenum externalType; if (!this->glCaps().getReadPixelsFormat(surface->config(), dstAsConfig, &externalFormat, diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h index 027d5717f8..50da36639c 100644 --- a/src/gpu/gl/GrGLGpu.h +++ b/src/gpu/gl/GrGLGpu.h @@ -229,20 +229,6 @@ private: GrGLTextureParameters::SamplerOverriddenState* initialState, const void* data); - // Checks whether glReadPixels can be called to get pixel values in readConfig from the - // render target. - bool readPixelsSupported(GrRenderTarget* target, GrPixelConfig readConfig); - - // Checks whether glReadPixels can be called to get pixel values in readConfig from a - // render target that has renderTargetConfig. This may have to create a temporary - // render target and thus is less preferable than the variant that takes a render target. - bool readPixelsSupported(GrPixelConfig renderTargetConfig, GrPixelConfig readConfig); - - // Checks whether glReadPixels can be called to get pixel values in readConfig from a - // render target that has the same config as surfaceForConfig. Calls one of the the two - // variations above, depending on whether the surface is a render target or not. - bool readPixelsSupported(GrSurface* surfaceForConfig, GrPixelConfig readConfig); - bool onReadPixels(GrSurface*, int left, int top, int width, int height, GrColorType, void* buffer, size_t rowBytes) override;