From d79c549467e5e7be025e38357f179b7965ed2ec3 Mon Sep 17 00:00:00 2001 From: bsalomon Date: Mon, 27 Apr 2015 10:07:04 -0700 Subject: Make non-AA hairline stroke rects snap to pixels centers so they close. BUG=skia:3717 Review URL: https://codereview.chromium.org/1101663007 --- src/gpu/GrClipMaskManager.cpp | 2 +- src/gpu/GrContext.cpp | 5 +++ src/gpu/GrPipeline.cpp | 3 ++ src/gpu/GrPipeline.h | 2 ++ src/gpu/GrPipelineBuilder.cpp | 10 +++--- src/gpu/GrPipelineBuilder.h | 41 +++++++++++++++---------- src/gpu/GrProgramDesc.h | 3 +- src/gpu/gl/GrGLProgramDesc.cpp | 2 +- src/gpu/gl/builders/GrGLVertexShaderBuilder.cpp | 22 ++++++++----- 9 files changed, 57 insertions(+), 33 deletions(-) (limited to 'src/gpu') diff --git a/src/gpu/GrClipMaskManager.cpp b/src/gpu/GrClipMaskManager.cpp index 439733b8d4..2a778bd9af 100644 --- a/src/gpu/GrClipMaskManager.cpp +++ b/src/gpu/GrClipMaskManager.cpp @@ -746,7 +746,7 @@ bool GrClipMaskManager::createStencilClipMask(GrRenderTarget* rt, // if the target is MSAA then we want MSAA enabled when the clip is soft if (rt->isMultisampled()) { - pipelineBuilder.setState(GrPipelineBuilder::kHWAntialias_StateBit, element->isAA()); + pipelineBuilder.setState(GrPipelineBuilder::kHWAntialias_Flag, element->isAA()); } bool fillInverted = false; diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp index 743377d468..4154005f17 100755 --- a/src/gpu/GrContext.cpp +++ b/src/gpu/GrContext.cpp @@ -798,6 +798,11 @@ void GrContext::drawRect(GrRenderTarget* rt, SkScalar rad = SkScalarHalf(width); bounds.outset(rad, rad); viewMatrix.mapRect(&bounds); + // Depending on sub-pixel coordinates and the particular GPU, we may lose a corner of + // hairline rects. We jam all the vertices to pixel centers to avoid this, but not when MSAA + // is enabled because it can cause ugly artifacts. + pipelineBuilder.setState(GrPipelineBuilder::kSnapVerticesToPixelCenters_Flag, + 0 == width && !rt->isMultisampled()); target->drawBatch(&pipelineBuilder, batch, &bounds); } else { // filled BW rect diff --git a/src/gpu/GrPipeline.cpp b/src/gpu/GrPipeline.cpp index c77dfeacc0..551882f971 100644 --- a/src/gpu/GrPipeline.cpp +++ b/src/gpu/GrPipeline.cpp @@ -64,6 +64,9 @@ GrPipeline::GrPipeline(const GrPipelineBuilder& pipelineBuilder, if (pipelineBuilder.isDither()) { fFlags |= kDither_Flag; } + if (pipelineBuilder.snapVerticesToPixelCenters()) { + fFlags |= kSnapVertices_Flag; + } int firstColorStageIdx = colorPOI.firstEffectiveStageIndex(); diff --git a/src/gpu/GrPipeline.h b/src/gpu/GrPipeline.h index 2cb5aab20a..fdba0d764c 100644 --- a/src/gpu/GrPipeline.h +++ b/src/gpu/GrPipeline.h @@ -80,6 +80,7 @@ public: bool isDitherState() const { return SkToBool(fFlags & kDither_Flag); } bool isHWAntialiasState() const { return SkToBool(fFlags & kHWAA_Flag); } + bool snapVerticesToPixelCenters() const { return SkToBool(fFlags & kSnapVertices_Flag); } // Skip any draws that refer to this pipeline (they should be a no-op). bool mustSkip() const { return NULL == this->getRenderTarget(); } @@ -119,6 +120,7 @@ private: enum Flags { kDither_Flag = 0x1, kHWAA_Flag = 0x2, + kSnapVertices_Flag = 0x4, }; typedef GrPendingIOResource RenderTarget; diff --git a/src/gpu/GrPipelineBuilder.cpp b/src/gpu/GrPipelineBuilder.cpp index 1d8aa4f2b4..98deee9bb8 100644 --- a/src/gpu/GrPipelineBuilder.cpp +++ b/src/gpu/GrPipelineBuilder.cpp @@ -16,7 +16,7 @@ #include "effects/GrPorterDuffXferProcessor.h" GrPipelineBuilder::GrPipelineBuilder() - : fFlagBits(0x0) + : fFlags(0x0) , fDrawFace(kBoth_DrawFace) , fColorProcInfoValid(false) , fCoverageProcInfoValid(false) @@ -27,7 +27,7 @@ GrPipelineBuilder::GrPipelineBuilder() GrPipelineBuilder& GrPipelineBuilder::operator=(const GrPipelineBuilder& that) { fRenderTarget.reset(SkSafeRef(that.fRenderTarget.get())); - fFlagBits = that.fFlagBits; + fFlags = that.fFlags; fStencilSettings = that.fStencilSettings; fDrawFace = that.fDrawFace; fXPFactory.reset(SkRef(that.getXPFactory())); @@ -69,12 +69,12 @@ void GrPipelineBuilder::setFromPaint(const GrPaint& paint, GrRenderTarget* rt, c // These have no equivalent in GrPaint, set them to defaults fDrawFace = kBoth_DrawFace; fStencilSettings.setDisabled(); - fFlagBits = 0; + fFlags = 0; fClip = clip; - this->setState(GrPipelineBuilder::kDither_StateBit, paint.isDither()); - this->setState(GrPipelineBuilder::kHWAntialias_StateBit, + this->setState(GrPipelineBuilder::kDither_Flag, paint.isDither()); + this->setState(GrPipelineBuilder::kHWAntialias_Flag, rt->isMultisampled() && paint.isAntiAlias()); fColorProcInfoValid = false; diff --git a/src/gpu/GrPipelineBuilder.h b/src/gpu/GrPipelineBuilder.h index ae9ca9b067..24d72e80aa 100644 --- a/src/gpu/GrPipelineBuilder.h +++ b/src/gpu/GrPipelineBuilder.h @@ -275,49 +275,56 @@ public: * Flags that affect rendering. Controlled using enable/disableState(). All * default to disabled. */ - enum StateBits { + enum Flags { /** * Perform dithering. TODO: Re-evaluate whether we need this bit */ - kDither_StateBit = 0x01, + kDither_Flag = 0x01, /** * Perform HW anti-aliasing. This means either HW FSAA, if supported by the render target, * or smooth-line rendering if a line primitive is drawn and line smoothing is supported by * the 3D API. */ - kHWAntialias_StateBit = 0x02, + kHWAntialias_Flag = 0x02, - kLast_StateBit = kHWAntialias_StateBit, + /** + * Modifies the vertex shader so that vertices will be positioned at pixel centers. + */ + kSnapVerticesToPixelCenters_Flag = 0x04, + + kLast_Flag = kSnapVerticesToPixelCenters_Flag, }; - bool isDither() const { return 0 != (fFlagBits & kDither_StateBit); } - bool isHWAntialias() const { return 0 != (fFlagBits & kHWAntialias_StateBit); } + bool isDither() const { return SkToBool(fFlags & kDither_Flag); } + bool isHWAntialias() const { return SkToBool(fFlags & kHWAntialias_Flag); } + bool snapVerticesToPixelCenters() const { + return SkToBool(fFlags & kSnapVerticesToPixelCenters_Flag); } /** * Enable render state settings. * - * @param stateBits bitfield of StateBits specifying the states to enable + * @param flags bitfield of Flags specifying the states to enable */ - void enableState(uint32_t stateBits) { fFlagBits |= stateBits; } - + void enableState(uint32_t flags) { fFlags |= flags; } + /** * Disable render state settings. * - * @param stateBits bitfield of StateBits specifying the states to disable + * @param flags bitfield of Flags specifying the states to disable */ - void disableState(uint32_t stateBits) { fFlagBits &= ~(stateBits); } + void disableState(uint32_t flags) { fFlags &= ~(flags); } /** - * Enable or disable stateBits based on a boolean. + * Enable or disable flags based on a boolean. * - * @param stateBits bitfield of StateBits to enable or disable + * @param flags bitfield of Flags to enable or disable * @param enable if true enable stateBits, otherwise disable */ - void setState(uint32_t stateBits, bool enable) { + void setState(uint32_t flags, bool enable) { if (enable) { - this->enableState(stateBits); + this->enableState(flags); } else { - this->disableState(stateBits); + this->disableState(flags); } } @@ -422,7 +429,7 @@ private: typedef SkSTArray<4, GrFragmentStage> FragmentStageArray; SkAutoTUnref fRenderTarget; - uint32_t fFlagBits; + uint32_t fFlags; GrStencilSettings fStencilSettings; DrawFace fDrawFace; mutable SkAutoTUnref fXPFactory; diff --git a/src/gpu/GrProgramDesc.h b/src/gpu/GrProgramDesc.h index 9f636a18bc..b306f02e4f 100644 --- a/src/gpu/GrProgramDesc.h +++ b/src/gpu/GrProgramDesc.h @@ -56,10 +56,11 @@ public: uint8_t fFragPosKey; // set by GrGLShaderBuilder if there are // effects that read the fragment position. // Otherwise, 0. - + uint8_t fSnapVerticesToPixelCenters; int8_t fColorEffectCnt; int8_t fCoverageEffectCnt; }; + GR_STATIC_ASSERT(sizeof(KeyHeader) == 4); int numColorEffects() const { return this->header().fColorEffectCnt; diff --git a/src/gpu/gl/GrGLProgramDesc.cpp b/src/gpu/gl/GrGLProgramDesc.cpp index 96a8ada09e..87e7f2d834 100644 --- a/src/gpu/gl/GrGLProgramDesc.cpp +++ b/src/gpu/gl/GrGLProgramDesc.cpp @@ -143,7 +143,7 @@ bool GrGLProgramDescBuilder::Build(GrProgramDesc* desc, } else { header->fFragPosKey = 0; } - + header->fSnapVerticesToPixelCenters = pipeline.snapVerticesToPixelCenters(); header->fColorEffectCnt = pipeline.numColorFragmentStages(); header->fCoverageEffectCnt = pipeline.numCoverageFragmentStages(); glDesc->finalize(); diff --git a/src/gpu/gl/builders/GrGLVertexShaderBuilder.cpp b/src/gpu/gl/builders/GrGLVertexShaderBuilder.cpp index 00e96a5618..20e9f0861a 100644 --- a/src/gpu/gl/builders/GrGLVertexShaderBuilder.cpp +++ b/src/gpu/gl/builders/GrGLVertexShaderBuilder.cpp @@ -42,11 +42,18 @@ void GrGLVertexBuilder::transformToNormalizedDeviceSpace(const GrShaderVar& posV kVec4f_GrSLType, kDefault_GrSLPrecision, fProgramBuilder->rtAdjustment(), &fRtAdjustName); - - // Transform from Skia's device coords to GL's normalized device coords. Note that - // because we want to "nudge" the device space positions we are converting to - // non-homogeneous NDC. - if (kVec3f_GrSLType == posVar.getType()) { + if (this->getProgramBuilder()->desc().header().fSnapVerticesToPixelCenters) { + if (kVec3f_GrSLType == posVar.getType()) { + const char* p = posVar.c_str(); + this->codeAppendf("{vec2 _posTmp = vec2(%s.x/%s.z, %s.y/%s.z);", p, p, p, p); + } else { + SkASSERT(kVec2f_GrSLType == posVar.getType()); + this->codeAppendf("{vec2 _posTmp = %s;", posVar.c_str()); + } + this->codeAppendf("_posTmp = floor(_posTmp) + vec2(0.5, 0.5);" + "gl_Position = vec4(_posTmp.x * %s.x + %s.y, _posTmp.y * %s.z + %s.w, 0, 1);}", + fRtAdjustName, fRtAdjustName, fRtAdjustName, fRtAdjustName); + } else if (kVec3f_GrSLType == posVar.getType()) { this->codeAppendf("gl_Position = vec4(dot(%s.xz, %s.xy)/%s.z, dot(%s.yz, %s.zw)/%s.z, 0, 1);", posVar.c_str(), fRtAdjustName, posVar.c_str(), posVar.c_str(), fRtAdjustName, posVar.c_str()); @@ -56,9 +63,8 @@ void GrGLVertexBuilder::transformToNormalizedDeviceSpace(const GrShaderVar& posV posVar.c_str(), fRtAdjustName, fRtAdjustName, posVar.c_str(), fRtAdjustName, fRtAdjustName); } - - // We could have the GrGeometryProcessor do this, but its just easier to have it performed here. - // If we ever need to set variable pointsize, then we can reinvestigate + // We could have the GrGeometryProcessor do this, but its just easier to have it performed + // here. If we ever need to set variable pointsize, then we can reinvestigate this->codeAppend("gl_PointSize = 1.0;"); } -- cgit v1.2.3