Clean up clipping code a bit

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

9 files changed, 56 insertions, 116 deletions

diff --git a/gm/clipdrawdraw.cpp b/gm/clipdrawdraw.cpp
index 0e471dd148..3846dc0b53 100644
--- a/gm/clipdrawdraw.cpp
+++ b/gm/clipdrawdraw.cpp
@@ -11,74 +11,51 @@ namespace skiagm {
 
 // This GM exercises the use case found in crbug.com/423834.
 // The following pattern:
-//    clipRect(r);
-//    drawRect(r, withAA);
-//    drawRect(r, noAA);
+//    save();
+//    clipRect(rect, noAA);
+//    drawRect(bigRect, noAA);
+//    restore();
+//
+//    drawRect(rect, noAA);
 // can leave 1 pixel wide remnants of the first rect.
 class ClipDrawDrawGM : public GM {
 public:
-    ClipDrawDrawGM() {
-        this->setBGColor(0xFFCCCCCC);
-    }
+    ClipDrawDrawGM() { this->setBGColor(0xFFCCCCCC); }
 
 protected:
-    SkString onShortName() SK_OVERRIDE {
-        return SkString("clipdrawdraw");
-    }
+    SkString onShortName() SK_OVERRIDE { return SkString("clipdrawdraw"); }
 
-    SkISize onISize() SK_OVERRIDE {
-        return SkISize::Make(512, 512);
-    }
+    SkISize onISize() SK_OVERRIDE { return SkISize::Make(512, 512); }
 
-    // Vertical remnant
-    static void draw1(SkCanvas* canvas) {
+    static void Draw(SkCanvas* canvas, const SkRect& rect) {
         SkPaint p;
-        p.setAntiAlias(true);
-
-        const SkRect rect = SkRect::MakeXYWH(8, 9, 404, 313);
-
-        canvas->save();
-
-        canvas->scale(0.5f, 0.5f);
-        canvas->translate(265, 265);
-
-        canvas->save();
-        canvas->clipRect(rect);
-        canvas->drawRect(rect, p);
-        canvas->restore();
-
-        p.setColor(SK_ColorWHITE);
         p.setAntiAlias(false);
-        canvas->drawRect(rect, p);
-        canvas->restore();
-    }
-
-    // Horizontal remnant
-    static void draw2(SkCanvas* canvas) {
-        SkPaint p;
-        p.setAntiAlias(true);
-
-        const SkRect rect = SkRect::MakeXYWH(8, 9, 404, 313);
-
-        canvas->save();
 
-        canvas->translate(200.800003f, 172.299988f);
-        canvas->scale(0.8f, 0.8f);
+        const SkRect bigRect = SkRect::MakeWH(600, 600);
 
         canvas->save();
-        canvas->clipRect(rect);
-        canvas->drawRect(rect, p);
-        canvas->restore();
-
-        p.setColor(SK_ColorWHITE);
-        p.setAntiAlias(false);
-        canvas->drawRect(rect, p);
+            // draw a black rect through the clip
+            canvas->save();
+                canvas->clipRect(rect);
+                canvas->drawRect(bigRect, p);
+            canvas->restore();
+
+            // now draw the white rect on top
+            p.setColor(SK_ColorWHITE);
+            canvas->drawRect(rect, p);
         canvas->restore();
     }
 
     void onDraw(SkCanvas* canvas) SK_OVERRIDE {
-        draw1(canvas);
-        draw2(canvas);
+        // Vertical remnant
+        const SkRect rect1 = SkRect::MakeLTRB(136.5f, 137.5f, 338.5f, 293.5f);
+
+        // Horizontal remnant
+        // 179.488 rounds the right way (i.e., 179), 179.499 rounds the wrong way (i.e., 180)
+        const SkRect rect2 = SkRect::MakeLTRB(207.5f, 179.499f, 530.5f, 429.5f);
+
+        Draw(canvas, rect1);
+        Draw(canvas, rect2);
     }
 
 private:
diff --git a/include/core/SkClipStack.h b/include/core/SkClipStack.h
index 3f79f92a5b..b74e47697f 100644
--- a/include/core/SkClipStack.h
+++ b/include/core/SkClipStack.h
@@ -310,13 +310,6 @@ public:
                    bool* isIntersectionOfRects = NULL) const;
 
     /**
-     * Takes an input rect in device space and conservatively clips it to the
-     * clip-stack. If false is returned then the rect does not intersect the
-     * clip and is unmodified.
-     */
-    bool intersectRectWithClip(SkRect* devRect) const;
-
-    /**
      * Returns true if the input rect in device space is entirely contained
      * by the clip. A return value of false does not guarantee that the rect
      * is not contained by the clip.
diff --git a/include/gpu/GrTypesPriv.h b/include/gpu/GrTypesPriv.h
index b22f48026c..1f86197295 100644
--- a/include/gpu/GrTypesPriv.h
+++ b/include/gpu/GrTypesPriv.h
@@ -249,6 +249,11 @@ struct GrScissorState {
                 (false == fEnabled || fRect == other.fRect);
     }
     bool operator!=(const GrScissorState& other) const { return !(*this == other); }
+
+    bool enabled() const { return fEnabled; }
+    const SkIRect& rect() const { return fRect; }
+
+private:
     bool    fEnabled;
     SkIRect fRect;
 };
diff --git a/src/core/SkClipStack.cpp b/src/core/SkClipStack.cpp
index 2d8c94f0f7..e0c3db01ba 100644
--- a/src/core/SkClipStack.cpp
+++ b/src/core/SkClipStack.cpp
@@ -420,12 +420,6 @@ void SkClipStack::Element::updateBoundAndGenID(const Element* prior) {
     }
 
     if (!fDoAA) {
-        // Here we mimic a non-anti-aliased scanline system. If there is
-        // no anti-aliasing we can integerize the bounding box to exclude
-        // fractional parts that won't be rendered.
-        // Note: the left edge is handled slightly differently below. We
-        // are a bit more generous in the rounding since we don't want to
-        // risk missing the left pixels when fLeft is very close to .5
         fFiniteBound.set(SkScalarFloorToScalar(fFiniteBound.fLeft+0.45f),
                          SkScalarRoundToScalar(fFiniteBound.fTop),
                          SkScalarRoundToScalar(fFiniteBound.fRight),
@@ -622,25 +616,6 @@ void SkClipStack::getBounds(SkRect* canvFiniteBound,
     }
 }
 
-bool SkClipStack::intersectRectWithClip(SkRect* rect) const {
-    SkASSERT(rect);
-
-    SkRect bounds;
-    SkClipStack::BoundsType bt;
-    this->getBounds(&bounds, &bt);
-    if (bt == SkClipStack::kInsideOut_BoundsType) {
-        if (bounds.contains(*rect)) {
-            return false;
-        } else {
-            // If rect's x values are both within bound's x range we
-            // could clip here. Same for y. But we don't bother to check.
-            return true;
-        }
-    } else {
-        return rect->intersect(bounds);
-    }
-}
-
 bool SkClipStack::quickContains(const SkRect& rect) const {
 
     Iter iter(*this, Iter::kTop_IterStart);
diff --git a/src/gpu/GrClipMaskManager.cpp b/src/gpu/GrClipMaskManager.cpp
index 0226ac1b76..f69c8717f8 100644
--- a/src/gpu/GrClipMaskManager.cpp
+++ b/src/gpu/GrClipMaskManager.cpp
@@ -23,7 +23,6 @@
 #include "effects/GrRRectEffect.h"
 #include "effects/GrTextureDomain.h"
 
-#define GR_AA_CLIP 1
 typedef SkClipStack::Element Element;
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -153,7 +152,7 @@ bool GrClipMaskManager::installClipEffects(GrPipelineBuilder* pipelineBuilder,
 
         if (!skip) {
             GrPrimitiveEdgeType edgeType;
-            if (GR_AA_CLIP && iter.get()->isAA()) {
+            if (iter.get()->isAA()) {
                 if (rt->isMultisampled()) {
                     // Coverage based AA clips don't place nicely with MSAA.
                     failed = true;
@@ -276,7 +275,6 @@ bool GrClipMaskManager::setupClipping(GrPipelineBuilder* pipelineBuilder,
         }
     }
 
-#if GR_AA_CLIP
     // If MSAA is enabled we can do everything in the stencil buffer.
     if (0 == rt->numSamples() && requiresAA) {
         GrTexture* result = NULL;
@@ -314,7 +312,6 @@ bool GrClipMaskManager::setupClipping(GrPipelineBuilder* pipelineBuilder,
         }
         // if alpha clip mask creation fails fall through to the non-AA code paths
     }
-#endif // GR_AA_CLIP
 
     // Either a hard (stencil buffer) clip was explicitly requested or an anti-aliased clip couldn't
     // be created. In either case, free up the texture in the anti-aliased mask cache.
diff --git a/src/gpu/GrSoftwarePathRenderer.cpp b/src/gpu/GrSoftwarePathRenderer.cpp
index 4fb7538b03..364c214a60 100644
--- a/src/gpu/GrSoftwarePathRenderer.cpp
+++ b/src/gpu/GrSoftwarePathRenderer.cpp
@@ -49,13 +49,11 @@ bool get_path_and_clip_bounds(const GrDrawTarget* target,
     if (NULL == rt) {
         return false;
     }
-    *devPathBounds = SkIRect::MakeWH(rt->width(), rt->height());
 
     target->getClip()->getConservativeBounds(rt, devClipBounds);
 
-    // TODO: getConservativeBounds already intersects with the
-    // render target's bounding box. Remove this next line
-    if (!devPathBounds->intersect(*devClipBounds)) {
+    if (devClipBounds->isEmpty()) {
+        *devPathBounds = SkIRect::MakeWH(rt->width(), rt->height());
         return false;
     }
 
@@ -64,6 +62,7 @@ bool get_path_and_clip_bounds(const GrDrawTarget* target,
         matrix.mapRect(&pathSBounds, path.getBounds());
         SkIRect pathIBounds;
         pathSBounds.roundOut(&pathIBounds);
+        *devPathBounds = *devClipBounds;
         if (!devPathBounds->intersect(pathIBounds)) {
             // set the correct path bounds, as this would be used later.
             *devPathBounds = pathIBounds;
diff --git a/src/gpu/GrTextContext.cpp b/src/gpu/GrTextContext.cpp
index e62136507c..bc544ad4a0 100644
--- a/src/gpu/GrTextContext.cpp
+++ b/src/gpu/GrTextContext.cpp
@@ -24,15 +24,9 @@ GrTextContext::~GrTextContext() {
 void GrTextContext::init(const GrPaint& grPaint, const SkPaint& skPaint) {
     const GrClipData* clipData = fContext->getClip();
 
-    SkRect devConservativeBound;
-    clipData->fClipStack->getConservativeBounds(
-                                     -clipData->fOrigin.fX,
-                                     -clipData->fOrigin.fY,
-                                     fContext->getRenderTarget()->width(),
+    clipData->getConservativeBounds(fContext->getRenderTarget()->width(),
                                      fContext->getRenderTarget()->height(),
-                                     &devConservativeBound);
-
-    devConservativeBound.roundOut(&fClipRect);
+                                     &fClipRect);
 
     fDrawTarget = fContext->getTextTarget();
 
diff --git a/src/gpu/effects/GrConfigConversionEffect.cpp b/src/gpu/effects/GrConfigConversionEffect.cpp
index ec44d08b01..7aad50f5f8 100644
--- a/src/gpu/effects/GrConfigConversionEffect.cpp
+++ b/src/gpu/effects/GrConfigConversionEffect.cpp
@@ -156,6 +156,8 @@ GrGLFragmentProcessor* GrConfigConversionEffect::createGLInstance() const {
     return SkNEW_ARGS(GrGLConfigConversionEffect, (*this));
 }
 
+
+
 void GrConfigConversionEffect::TestForPreservingPMConversions(GrContext* context,
                                                               PMConversion* pmToUPMRule,
                                                               PMConversion* upmToPMRule) {
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 6419aa6b52..21d2f6327b 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -1327,13 +1327,13 @@ GrIndexBuffer* GrGLGpu::onCreateIndexBuffer(size_t size, bool dynamic) {
 void GrGLGpu::flushScissor(const GrScissorState& scissorState,
                            const GrGLIRect& rtViewport,
                            GrSurfaceOrigin rtOrigin) {
-    if (scissorState.fEnabled) {
+    if (scissorState.enabled()) {
         GrGLIRect scissor;
         scissor.setRelativeTo(rtViewport,
-                              scissorState.fRect.fLeft,
-                              scissorState.fRect.fTop,
-                              scissorState.fRect.width(),
-                              scissorState.fRect.height(),
+                              scissorState.rect().fLeft,
+                              scissorState.rect().fTop,
+                              scissorState.rect().width(),
+                              scissorState.rect().height(),
                               rtOrigin);
         // if the scissor fully contains the viewport then we fall through and
         // disable the scissor test.
@@ -1489,9 +1489,8 @@ void GrGLGpu::onClear(GrRenderTarget* target, const SkIRect* rect, GrColor color
 
     this->flushRenderTarget(glRT, rect);
     GrScissorState scissorState;
-    scissorState.fEnabled = SkToBool(rect);
-    if (scissorState.fEnabled) {
-        scissorState.fRect = *rect;
+    if (rect) {
+        scissorState.set(*rect);
     }
     this->flushScissor(scissorState, glRT->getViewport(), glRT->origin());
 
@@ -1602,8 +1601,7 @@ void GrGLGpu::onClearStencilClip(GrRenderTarget* target, const SkIRect& rect, bo
     this->flushRenderTarget(glRT, &SkIRect::EmptyIRect());
 
     GrScissorState scissorState;
-    scissorState.fEnabled = true;
-    scissorState.fRect = rect;
+    scissorState.set(rect);
     this->flushScissor(scissorState, glRT->getViewport(), glRT->origin());
 
     GL_CALL(StencilMask((uint32_t) clipStencilMask));
@@ -1930,18 +1928,18 @@ void GrGLGpu::onResolveRenderTarget(GrRenderTarget* target) {
             fHWBoundRenderTargetUniqueID = SK_InvalidUniqueID;
             const GrGLIRect& vp = rt->getViewport();
             const SkIRect dirtyRect = rt->getResolveRect();
-            GrGLIRect r;
-            r.setRelativeTo(vp, dirtyRect.fLeft, dirtyRect.fTop,
-                            dirtyRect.width(), dirtyRect.height(), target->origin());
 
             if (GrGLCaps::kES_Apple_MSFBOType == this->glCaps().msFBOType()) {
                 // Apple's extension uses the scissor as the blit bounds.
                 GrScissorState scissorState;
-                scissorState.fEnabled = true;
-                scissorState.fRect = dirtyRect;
-                this->flushScissor(scissorState, rt->getViewport(), rt->origin());
+                scissorState.set(dirtyRect);
+                this->flushScissor(scissorState, vp, rt->origin());
                 GL_CALL(ResolveMultisampleFramebuffer());
             } else {
+                GrGLIRect r;
+                r.setRelativeTo(vp, dirtyRect.fLeft, dirtyRect.fTop,
+                                dirtyRect.width(), dirtyRect.height(), target->origin());
+
                 int right = r.fLeft + r.fWidth;
                 int top = r.fBottom + r.fHeight;