Make non-AA hairline stroke rects snap to pixels centers so they close.

BUG=skia:3717

Review URL: https://codereview.chromium.org/1101663007

9 files changed, 57 insertions, 33 deletions

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<GrRenderTarget, kWrite_GrIOType> 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<GrRenderTarget>            fRenderTarget;
-    uint32_t                                fFlagBits;
+    uint32_t                                fFlags;
     GrStencilSettings                       fStencilSettings;
     DrawFace                                fDrawFace;
     mutable SkAutoTUnref<const GrXPFactory> 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;");
 }