From 64d094d7756534a9b9b0997aab225d9ceba098b6 Mon Sep 17 00:00:00 2001 From: brianosman Date: Fri, 25 Mar 2016 06:01:59 -0700 Subject: [PATCH] Require sRGB write control for sRGB support. Add flag to GrPaint to suppress linear -> sRGB conversion on write. Use that to fix YUV conversion, which directly produces sRGB data. (Technically, it produces data in whatever the color space of the JPEG might be). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1830303002 Review URL: https://codereview.chromium.org/1830303002 --- include/gpu/GrCaps.h | 7 +++++++ include/gpu/GrPaint.h | 9 +++++++++ src/gpu/GrPaint.cpp | 1 + src/gpu/GrPipeline.cpp | 3 +++ src/gpu/GrPipeline.h | 8 ++++++-- src/gpu/GrPipelineBuilder.cpp | 2 ++ src/gpu/GrPipelineBuilder.h | 9 ++++++++- src/gpu/GrYUVProvider.cpp | 3 +++ src/gpu/gl/GrGLCaps.cpp | 8 +++----- src/gpu/gl/GrGLCaps.h | 8 -------- src/gpu/gl/GrGLGpu.cpp | 24 +++++++++++++----------- src/gpu/gl/GrGLGpu.h | 2 +- 12 files changed, 56 insertions(+), 28 deletions(-) diff --git a/include/gpu/GrCaps.h b/include/gpu/GrCaps.h index 1f5b11955b..398517cb68 100644 --- a/include/gpu/GrCaps.h +++ b/include/gpu/GrCaps.h @@ -137,6 +137,13 @@ public: /** To avoid as-yet-unnecessary complexity we don't allow any partial support of MIP Maps (e.g. only for POT textures) */ bool mipMapSupport() const { return fMipMapSupport; } + + /** + * Skia convention is that a device only has sRGB support if it supports sRGB formats for both + * textures and framebuffers. In addition: + * Decoding to linear of an sRGB texture can be disabled. + * Encoding and gamma-correct blending to an sRGB framebuffer can be disabled. + */ bool srgbSupport() const { return fSRGBSupport; } bool twoSidedStencilSupport() const { return fTwoSidedStencilSupport; } bool stencilWrapOpsSupport() const { return fStencilWrapOpsSupport; } diff --git a/include/gpu/GrPaint.h b/include/gpu/GrPaint.h index 152cb51d7e..3f06f092ab 100644 --- a/include/gpu/GrPaint.h +++ b/include/gpu/GrPaint.h @@ -56,6 +56,13 @@ public: void setAntiAlias(bool aa) { fAntiAlias = aa; } bool isAntiAlias() const { return fAntiAlias; } + /** + * Should shader output conversion from linear to sRGB be disabled. + * Only relevant if the destination is sRGB. Defaults to false. + */ + void setDisableOutputConversionToSRGB(bool srgb) { fDisableOutputConversionToSRGB = srgb; } + bool getDisableOutputConversionToSRGB() const { return fDisableOutputConversionToSRGB; } + const GrXPFactory* setXPFactory(const GrXPFactory* xpFactory) { fXPFactory.reset(SkSafeRef(xpFactory)); return xpFactory; @@ -112,6 +119,7 @@ public: GrPaint& operator=(const GrPaint& paint) { fAntiAlias = paint.fAntiAlias; + fDisableOutputConversionToSRGB = paint.fDisableOutputConversionToSRGB; fColor = paint.fColor; this->resetFragmentProcessors(); @@ -154,6 +162,7 @@ private: SkSTArray<2, const GrFragmentProcessor*, true> fCoverageFragmentProcessors; bool fAntiAlias; + bool fDisableOutputConversionToSRGB; GrColor fColor; }; diff --git a/src/gpu/GrPaint.cpp b/src/gpu/GrPaint.cpp index 1ec8e502be..c381562b7c 100644 --- a/src/gpu/GrPaint.cpp +++ b/src/gpu/GrPaint.cpp @@ -15,6 +15,7 @@ GrPaint::GrPaint() : fAntiAlias(false) + , fDisableOutputConversionToSRGB(false) , fColor(GrColor_WHITE) {} void GrPaint::setCoverageSetOpXPFactory(SkRegion::Op regionOp, bool invertCoverage) { diff --git a/src/gpu/GrPipeline.cpp b/src/gpu/GrPipeline.cpp index 8e542d2a70..e3c4864980 100644 --- a/src/gpu/GrPipeline.cpp +++ b/src/gpu/GrPipeline.cpp @@ -81,6 +81,9 @@ GrPipeline* GrPipeline::CreateAt(void* memory, const CreateArgs& args, if (builder.snapVerticesToPixelCenters()) { pipeline->fFlags |= kSnapVertices_Flag; } + if (builder.getDisableOutputConversionToSRGB()) { + pipeline->fFlags |= kDisableOutputConversionToSRGB_Flag; + } int firstColorProcessorIdx = args.fOpts.fColorPOI.firstEffectiveProcessorIndex(); diff --git a/src/gpu/GrPipeline.h b/src/gpu/GrPipeline.h index db7cb5bc62..bdcc7d991b 100644 --- a/src/gpu/GrPipeline.h +++ b/src/gpu/GrPipeline.h @@ -145,6 +145,9 @@ public: bool isHWAntialiasState() const { return SkToBool(fFlags & kHWAA_Flag); } bool snapVerticesToPixelCenters() const { return SkToBool(fFlags & kSnapVertices_Flag); } + bool getDisableOutputConversionToSRGB() const { + return SkToBool(fFlags & kDisableOutputConversionToSRGB_Flag); + } GrXferBarrierType xferBarrierType(const GrCaps& caps) const { return this->getXferProcessor().xferBarrierType(fRenderTarget.get(), caps); @@ -184,8 +187,9 @@ private: const GrCaps&); enum Flags { - kHWAA_Flag = 0x1, - kSnapVertices_Flag = 0x2, + kHWAA_Flag = 0x1, + kSnapVertices_Flag = 0x2, + kDisableOutputConversionToSRGB_Flag = 0x4, }; typedef GrPendingIOResource RenderTarget; diff --git a/src/gpu/GrPipelineBuilder.cpp b/src/gpu/GrPipelineBuilder.cpp index f1d5c26168..2eba5354fa 100644 --- a/src/gpu/GrPipelineBuilder.cpp +++ b/src/gpu/GrPipelineBuilder.cpp @@ -44,6 +44,8 @@ GrPipelineBuilder::GrPipelineBuilder(const GrPaint& paint, GrRenderTarget* rt, c this->setState(GrPipelineBuilder::kHWAntialias_Flag, rt->isUnifiedMultisampled() && paint.isAntiAlias()); + this->setState(GrPipelineBuilder::kDisableOutputConversionToSRGB_Flag, + paint.getDisableOutputConversionToSRGB()); } //////////////////////////////////////////////////////////////////////////////s diff --git a/src/gpu/GrPipelineBuilder.h b/src/gpu/GrPipelineBuilder.h index f66ced3025..83ad05f85f 100644 --- a/src/gpu/GrPipelineBuilder.h +++ b/src/gpu/GrPipelineBuilder.h @@ -283,12 +283,19 @@ public: */ kSnapVerticesToPixelCenters_Flag = 0x02, - kLast_Flag = kSnapVerticesToPixelCenters_Flag, + /** + * Suppress linear -> sRGB conversion when rendering to sRGB render targets. + */ + kDisableOutputConversionToSRGB_Flag = 0x04, + + kLast_Flag = kDisableOutputConversionToSRGB_Flag, }; bool isHWAntialias() const { return SkToBool(fFlags & kHWAntialias_Flag); } bool snapVerticesToPixelCenters() const { return SkToBool(fFlags & kSnapVerticesToPixelCenters_Flag); } + bool getDisableOutputConversionToSRGB() const { + return SkToBool(fFlags & kDisableOutputConversionToSRGB_Flag); } /** * Enable render state settings. diff --git a/src/gpu/GrYUVProvider.cpp b/src/gpu/GrYUVProvider.cpp index f35c6dfe9b..90d553758f 100644 --- a/src/gpu/GrYUVProvider.cpp +++ b/src/gpu/GrYUVProvider.cpp @@ -123,6 +123,9 @@ GrTexture* GrYUVProvider::refAsTexture(GrContext* ctx, const GrSurfaceDesc& desc SkASSERT(renderTarget); GrPaint paint; + // We may be decoding an sRGB image, but the result of our linear math on the YUV planes + // is already in sRGB in that case. Don't convert (which will make the image too bright). + paint.setDisableOutputConversionToSRGB(true); SkAutoTUnref yuvToRgbProcessor( GrYUVEffect::CreateYUVToRGB(yuvTextures[0], yuvTextures[1], diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index 50a74febe7..faccbd1fca 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -46,7 +46,6 @@ GrGLCaps::GrGLCaps(const GrContextOptions& contextOptions, fBindFragDataLocationSupport = false; fRectangleTextureSupport = false; fTextureSwizzleSupport = false; - fSRGBWriteControl = false; fRGBA8888PixelsOpsAreSlow = false; fPartialFBOReadIsSlow = false; fMipMapLevelAndLodControlSupport = false; @@ -1080,7 +1079,6 @@ SkString GrGLCaps::dump() const { r.appendf("Base instance support: %s\n", (fBaseInstanceSupport ? "YES" : "NO")); r.appendf("Use non-VBO for dynamic data: %s\n", (fUseNonVBOVertexAndIndexDynamicData ? "YES" : "NO")); - r.appendf("SRGB write contol: %s\n", (fSRGBWriteControl ? "YES" : "NO")); r.appendf("RGBA 8888 pixel ops are slow: %s\n", (fRGBA8888PixelsOpsAreSlow ? "YES" : "NO")); r.appendf("Partial FBO read is slow: %s\n", (fPartialFBOReadIsSlow ? "YES" : "NO")); r.appendf("Bind uniform location support: %s\n", (fBindUniformLocationSupport ? "YES" : "NO")); @@ -1434,7 +1432,8 @@ void GrGLCaps::initConfigTable(const GrGLContextInfo& ctxInfo, const GrGLInterfa fConfigTable[kBGRA_8888_GrPixelConfig].fSwizzle = GrSwizzle::RGBA(); // We only enable srgb support if both textures and FBOs support srgb, - // *and* we can disable sRGB decode-on-read, to support "legacy" mode. + // *and* we can disable sRGB decode-on-read, to support "legacy" mode, + // *and* we can disable sRGB encode-on-write. if (kGL_GrGLStandard == standard) { if (ctxInfo.version() >= GR_GL_VER(3,0)) { fSRGBSupport = true; @@ -1445,14 +1444,13 @@ void GrGLCaps::initConfigTable(const GrGLContextInfo& ctxInfo, const GrGLInterfa } } // All the above srgb extensions support toggling srgb writes - fSRGBWriteControl = fSRGBSupport; } else { // See https://bug.skia.org/4148 for PowerVR issue. fSRGBSupport = kPowerVRRogue_GrGLRenderer != ctxInfo.renderer() && (ctxInfo.version() >= GR_GL_VER(3,0) || ctxInfo.hasExtension("GL_EXT_sRGB")); // ES through 3.1 requires EXT_srgb_write_control to support toggling // sRGB writing for destinations. - fSRGBWriteControl = ctxInfo.hasExtension("GL_EXT_sRGB_write_control"); + fSRGBSupport = fSRGBSupport && ctxInfo.hasExtension("GL_EXT_sRGB_write_control"); } if (!ctxInfo.hasExtension("GL_EXT_texture_sRGB_decode")) { // To support "legacy" L32 mode, we require the ability to turn off sRGB decode: diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h index bb3e231719..e9a29693de 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -323,13 +323,6 @@ public: /// GL_ARB_texture_swizzle bool textureSwizzleSupport() const { return fTextureSwizzleSupport; } - /** - * Is there support for enabling/disabling sRGB writes for sRGB-capable color attachments? - * If false this does not mean sRGB is not supported but rather that if it is supported - * it cannot be turned off for configs that support it. - */ - bool srgbWriteControl() const { return fSRGBWriteControl; } - bool mipMapLevelAndLodControlSupport() const { return fMipMapLevelAndLodControlSupport; } /** @@ -402,7 +395,6 @@ private: bool fUseNonVBOVertexAndIndexDynamicData : 1; bool fIsCoreProfile : 1; bool fBindFragDataLocationSupport : 1; - bool fSRGBWriteControl : 1; bool fRGBA8888PixelsOpsAreSlow : 1; bool fPartialFBOReadIsSlow : 1; bool fBindUniformLocationSupport : 1; diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index 3a293610f2..3bbc77aa9b 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -2114,7 +2114,7 @@ bool GrGLGpu::flushGLState(const GrPipeline& pipeline, const GrPrimitiveProcesso // This must come after textures are flushed because a texture may need // to be msaa-resolved (which will modify bound FBO state). - this->flushRenderTarget(glRT, nullptr); + this->flushRenderTarget(glRT, nullptr, pipeline.getDisableOutputConversionToSRGB()); return true; } @@ -2833,7 +2833,7 @@ void GrGLGpu::finishDrawTarget() { } } -void GrGLGpu::flushRenderTarget(GrGLRenderTarget* target, const SkIRect* bounds) { +void GrGLGpu::flushRenderTarget(GrGLRenderTarget* target, const SkIRect* bounds, bool disableSRGB) { SkASSERT(target); uint32_t rtID = target->getUniqueID(); @@ -2855,17 +2855,19 @@ void GrGLGpu::flushRenderTarget(GrGLRenderTarget* target, const SkIRect* bounds) #endif fHWBoundRenderTargetUniqueID = rtID; this->flushViewport(target->getViewport()); - if (this->glCaps().srgbWriteControl()) { - bool enableSRGBWrite = GrPixelConfigIsSRGB(target->config()); - if (enableSRGBWrite && kYes_TriState != fHWSRGBFramebuffer) { - GL_CALL(Enable(GR_GL_FRAMEBUFFER_SRGB)); - fHWSRGBFramebuffer = kYes_TriState; - } else if (!enableSRGBWrite && kNo_TriState != fHWSRGBFramebuffer) { - GL_CALL(Disable(GR_GL_FRAMEBUFFER_SRGB)); - fHWSRGBFramebuffer = kNo_TriState; - } + } + + if (this->glCaps().srgbSupport()) { + bool enableSRGBWrite = GrPixelConfigIsSRGB(target->config()) && !disableSRGB; + if (enableSRGBWrite && kYes_TriState != fHWSRGBFramebuffer) { + GL_CALL(Enable(GR_GL_FRAMEBUFFER_SRGB)); + fHWSRGBFramebuffer = kYes_TriState; + } else if (!enableSRGBWrite && kNo_TriState != fHWSRGBFramebuffer) { + GL_CALL(Disable(GR_GL_FRAMEBUFFER_SRGB)); + fHWSRGBFramebuffer = kNo_TriState; } } + this->didWriteToSurface(target, bounds); } diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h index bcb3c193dc..61fdb8fa33 100644 --- a/src/gpu/gl/GrGLGpu.h +++ b/src/gpu/gl/GrGLGpu.h @@ -328,7 +328,7 @@ private: // bounds is region that may be modified. // nullptr means whole target. Can be an empty rect. - void flushRenderTarget(GrGLRenderTarget*, const SkIRect* bounds); + void flushRenderTarget(GrGLRenderTarget*, const SkIRect* bounds, bool disableSRGB = false); // Handles cases where a surface will be updated without a call to flushRenderTarget void didWriteToSurface(GrSurface*, const SkIRect* bounds) const;