Remove the AA requirement for selecting GrEffect-based clipping.

src/gpu/GrClipMaskManager.cpp

 
 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "GrClipMaskManager.h"
 #include "GrAAConvexPathRenderer.h"
@@ skipping 92 matching lines @@
                 return true;
             }
         }
     }
     return false;
 }
 
 bool GrClipMaskManager::installClipEffects(const ElementList& elements,
                                            GrDrawState::AutoRestoreEffects* are,
                                            const SkVector& clipToRTOffset,
-                                           const SkRect* drawBounds) {
+                                           const SkRect* drawBounds,
+                                           SkIRect* scissorRect) {
 

[robertphillips, 2014/07/02 14:44:34]

SkASSERT(NULL != scissorRect && scissorRect->isEmpty()); ?

[bsalomon, 2014/07/02 16:36:46]

Added the NULL assertion, I'm not sure this function should care how the caller
init'ed it.

[robertphillips, 2014/07/02 16:48:28]

Fair enough.

     GrDrawState* drawState = fGpu->drawState();
     SkRect boundsInClipSpace;
     if (NULL != drawBounds) {
         boundsInClipSpace = *drawBounds;
         boundsInClipSpace.offset(-clipToRTOffset.fX, -clipToRTOffset.fY);
     }
 
     are->set(drawState);
     GrRenderTarget* rt = drawState->getRenderTarget();
-    ElementList::Iter iter(elements);
+    // We iterate from the top of the stack to the bottom. We do this because we select the first
+    // BW rectangle as the scissor. Clients performing hierarchical rendering tend to use smaller
+    // clips towards the top of the clip stack. Smaller scissor rects can help tiled architectures
+    // skip processing tiles for draws.
+    ElementList::Iter iter(elements, ElementList::Iter::kTail_IterStart);
 
     bool setARE = false;
     bool failed = false;
 
     while (NULL != iter.get()) {
         SkRegion::Op op = iter.get()->getOp();
         bool invert;
         bool skip = false;
         switch (op) {
             case SkRegion::kReplace_Op:
@@ skipping 15 matching lines @@
                 break;
         }
         if (failed) {
             break;
         }
 
         if (!skip) {
             GrEffectEdgeType edgeType;
             if (GR_AA_CLIP && iter.get()->isAA()) {
                 if (rt->isMultisampled()) {
-                    // Coverage based AA clips don't place nicely with MSAA.
+                    // Coverage based AA clips don't play nicely with MSAA.
                     failed = true;
                     break;
                 }
                 edgeType = invert ? kInverseFillAA_GrEffectEdgeType : kFillAA_GrEffectEdgeType;
             } else {
                 edgeType = invert ? kInverseFillBW_GrEffectEdgeType : kFillBW_GrEffectEdgeType;
             }

[robertphillips, 2014/07/02 14:44:34]

// We don't want to exit if we convert a BW rect clip to a scissor  ?

[bsalomon, 2014/07/02 16:36:47]

Done.

+            bool failIfNoEffect = true;
             SkAutoTUnref<GrEffectRef> effect;
             switch (iter.get()->getType()) {
                 case SkClipStack::Element::kPath_Type:
                     effect.reset(GrConvexPolyEffect::Create(edgeType, iter.get()->getPath(),
                         &clipToRTOffset));
                     break;
                 case SkClipStack::Element::kRRect_Type: {
                     SkRRect rrect = iter.get()->getRRect();
                     rrect.offset(clipToRTOffset.fX, clipToRTOffset.fY);
                     effect.reset(GrRRectEffect::Create(edgeType, rrect));
                     break;
                 }
                 case SkClipStack::Element::kRect_Type: {
                     SkRect rect = iter.get()->getRect();
                     rect.offset(clipToRTOffset.fX, clipToRTOffset.fY);

[robertphillips, 2014/07/02 14:44:34]

Kind of hand wavy here but what about something like:

if (kFillBW_GrEffectEdgeType == edgeType) {
   if (scissorRect->isEmpty()) {
       rect.roundOut(scissorRect);
       failIfNoEffect = false;
   } else if (scissorRect->contains(rect)) {  // new magical IRect contains Rect
method
       rect.roundOut(scissorRect);
       failIfNoEffect = false;
   } else if (rect.contains(scissorRect)) {  // actually want rounded out 'rect'
       failIfNoEffect = false;
   } else {
       effect.reset(GrConvexPolyEffect::Create(edgeType, rect));
   }
} else {
 ...

Also do we have to worry about expanding clips in all of this?

[bsalomon, 2014/07/02 16:36:47]

Expanding how? The round out? I think that's what the raster code does. It's
also what we do for a single BW rect clip.

I was assuming that the the clip stack reducer had caught most of the
containment optimizations for us already.

-                    effect.reset(GrConvexPolyEffect::Create(edgeType, rect));
+                    if (kFillBW_GrEffectEdgeType == edgeType && scissorRect->isEmpty()) {
+                        rect.roundOut(scissorRect);
+                        failIfNoEffect = false;
+                    } else {
+                        effect.reset(GrConvexPolyEffect::Create(edgeType, rect));
+                    }
                     break;
                 }
                 default:
                     break;
             }
             if (effect) {
                 if (!setARE) {
                     are->set(fGpu->drawState());
                     setARE = true;
                 }
                 fGpu->drawState()->addCoverageEffect(effect);
-            } else {
+            } else if (failIfNoEffect) {
                 failed = true;
                 break;
             }
         }
-        iter.next();
+        iter.prev();
     }
 
     if (failed) {
         are->set(NULL);
     }
 
     return !failed;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 45 matching lines @@
 
     // An element count of 4 was chosen because of the common pattern in Blink of:
     //   isect RR
     //   diff  RR
     //   isect convex_poly
     //   isect convex_poly
     // when drawing rounded div borders. This could probably be tuned based on a
     // configuration's relative costs of switching RTs to generate a mask vs
     // longer shaders.
     if (elements.count() <= 4) {
+        SkIRect scissorRect;
+        scissorRect.setEmpty();
         SkVector clipToRTOffset = { SkIntToScalar(-clipDataIn->fOrigin.fX),
                                     SkIntToScalar(-clipDataIn->fOrigin.fY) };
         if (elements.isEmpty() ||
-            (requiresAA && this->installClipEffects(elements, are, clipToRTOffset, devBounds))) {
-            SkIRect scissorSpaceIBounds(clipSpaceIBounds);
-            scissorSpaceIBounds.offset(-clipDataIn->fOrigin);
-            if (NULL == devBounds ||
-                !SkRect::Make(scissorSpaceIBounds).contains(*devBounds)) {
-                fGpu->enableScissor(scissorSpaceIBounds);
+            this->installClipEffects(elements, are, clipToRTOffset, devBounds, &scissorRect)) {
+            if (scissorRect.isEmpty()) {
+                // We may still want to use a scissor, especially on tiled architectures.
+                scissorRect = clipSpaceIBounds;
+                scissorRect.offset(-clipDataIn->fOrigin);
+                if (NULL == devBounds ||

[robertphillips, 2014/07/02 14:44:34]

Add magical SkIRect contains Rect method ?

[bsalomon, 2014/07/02 16:36:47]

This current code is actually overkill because contains does empty checks. Wrote
a static inline helper.

+                    !SkRect::Make(scissorRect).contains(*devBounds)) {
+                    fGpu->enableScissor(scissorRect);
+                } else {

[robertphillips, 2014/07/02 14:44:34]

Why not use devBounds intersected with scissorRect in this case?

[bsalomon, 2014/07/02 16:36:47]

Scissor changes prevent batching. If we don't see an obvious win for using the
scissor, might as well disable it.

[robertphillips, 2014/07/02 16:48:28]

That would make a good comment.

[bsalomon, 2014/07/02 17:28:46]

Done, and added a comment about effects always shrinking the clip.

+                    fGpu->disableScissor();
+                }
             } else {

[robertphillips, 2014/07/02 14:44:34]

Add SkIRect::intersectWH or, failing that, use SkIRect::intersect(0, 0, width,
height) ?

[bsalomon, 2014/07/02 16:36:47]

The intersect() methods on SkIRect all do empty checks which we don't need here.
There are intersectNoEmptyCheck() methods but none that match the requirements
here. I'm not sure adding such a specialized case (e.g. intersectWHNoEmptyCheck)
makes sense.

-                fGpu->disableScissor();
+                scissorRect.fLeft = SkTMax(0, scissorRect.fLeft);
+                scissorRect.fTop = SkTMax(0, scissorRect.fTop);
+                scissorRect.fRight = SkTMin(rt->width(), scissorRect.fRight);
+                scissorRect.fBottom = SkTMin(rt->height(), scissorRect.fBottom);
+                fGpu->enableScissor(scissorRect);
             }
             this->setGpuStencil();
             return true;
         }
     }
 
 #if GR_AA_CLIP
     // If MSAA is enabled we can do everything in the stencil buffer.
     if (0 == rt->numSamples() && requiresAA) {
         GrTexture* result = NULL;
@@ skipping 852 matching lines @@
 
     // TODO: dynamically attach a stencil buffer
     int stencilBits = 0;
     GrStencilBuffer* stencilBuffer =
         drawState.getRenderTarget()->getStencilBuffer();
     if (NULL != stencilBuffer) {
         stencilBits = stencilBuffer->bits();
         this->adjustStencilParams(settings, clipMode, stencilBits);
     }
 }