Don't use getDeviceClipBounds() to bound pic ops.

The values returned by SkCanvas::getDeviceClipBounds() are in the right
space, but have extra constraints on them that are not desirable for
bounding the logical bounds of draw operations:

  - they are integral
  - they are non-negative

We've been intersecting the bounds of each operation with these bounds,
which means we're mixing these bogus constraints into the bounds of each
recorded operation.  This percolates up to the SkPicutre cull rect too.

The most egregious way to see the problem is to record a draw op
entirely in negative space... it'll come back with empty logical bounds
rather than its correct (negative-space) bounds.  I've added a test
for this, and another test I also think should be passing but left
making it so as a follow up.

I've had to disable a couple tests asserting clips affect the bounds. :/

A possible follow-up might go back to using the clips to tighten the
bounds of the ops, just so long as we take the original user bounds and
map them with the CTM through to device space ourselves, rather than
relying on the recording canvas' clip stack.  I think this means we'd
need to maintain our own stack of device-space float SkRect clip bounds
while calculating these op bounds.

Bug: skia:7735
Change-Id: I6bf15f6b2a9ba4329a4eeae7f9d57aa8729ec1bb
Reviewed-on: https://skia-review.googlesource.com/126002
Commit-Queue: Mike Klein <mtklein@chromium.org>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Mike Klein <mtklein@chromium.org>
Reviewed-by: Brian Osman <brianosman@google.com>

1 files changed, 16 insertions, 64 deletions

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 89eb8a751a..37899e89c3 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -167,7 +167,6 @@ public:
         , fCullRect(cullRect)
         , fBounds(bounds) {
         fCTM = SkMatrix::I();
-        fCurrentClipBounds = fCullRect;
     }
 
     void cleanUp() {
@@ -188,7 +187,6 @@ public:
 
     template <typename T> void operator()(const T& op) {
         this->updateCTM(op);
-        this->updateClipBounds(op);
         this->trackBounds(op);
     }
 
@@ -206,21 +204,21 @@ public:
 
         // Adjust the rect for its own paint.
         if (!AdjustForPaint(paint, &rect)) {
-            // The paint could do anything to our bounds.  The only safe answer is the current clip.
-            return fCurrentClipBounds;
+            // The paint could do anything to our bounds.  The only safe answer is the cull.
+            return fCullRect;
         }
 
         // Adjust rect for all the paints from the SaveLayers we're inside.
         if (!this->adjustForSaveLayerPaints(&rect)) {
             // Same deal as above.
-            return fCurrentClipBounds;
+            return fCullRect;
         }
 
         // Map the rect back to identity space.
         fCTM.mapRect(&rect);
 
-        // Nothing can draw outside the current clip.
-        if (!rect.intersect(fCurrentClipBounds)) {
+        // Nothing can draw outside the cull rect.
+        if (!rect.intersect(fCullRect)) {
             return Bounds::MakeEmpty();
         }
 
@@ -242,50 +240,6 @@ private:
     void updateCTM(const Concat& op)    { fCTM.preConcat(op.matrix); }
     void updateCTM(const Translate& op) { fCTM.preTranslate(op.dx, op.dy); }
 
-    // Most ops don't change the clip.
-    template <typename T> void updateClipBounds(const T&) {}
-
-    // Clip{Path,RRect,Rect,Region} obviously change the clip.  They all know their bounds already.
-    void updateClipBounds(const ClipPath&   op) { this->updateClipBoundsForClipOp(op.devBounds); }
-    void updateClipBounds(const ClipRRect&  op) { this->updateClipBoundsForClipOp(op.devBounds); }
-    void updateClipBounds(const ClipRect&   op) { this->updateClipBoundsForClipOp(op.devBounds); }
-    void updateClipBounds(const ClipRegion& op) { this->updateClipBoundsForClipOp(op.devBounds); }
-
-    // The bounds of clip ops need to be adjusted for the paints of saveLayers they're inside.
-    void updateClipBoundsForClipOp(const SkIRect& devBounds) {
-        Bounds clip = SkRect::Make(devBounds);
-        // We don't call adjustAndMap() because as its last step it would intersect the adjusted
-        // clip bounds with the previous clip, exactly what we can't do when the clip grows.
-        if (this->adjustForSaveLayerPaints(&clip)) {
-            fCurrentClipBounds = clip.intersect(fCullRect) ? clip : Bounds::MakeEmpty();
-        } else {
-            fCurrentClipBounds = fCullRect;
-        }
-    }
-
-    // Restore holds the devBounds for the clip after the {save,saveLayer}/restore block completes.
-    void updateClipBounds(const Restore& op) {
-        // This is just like the clip ops above, but we need to skip the effects (if any) of our
-        // paired saveLayer (if it is one); it has not yet been popped off the save stack.  Our
-        // devBounds reflect the state of the world after the saveLayer/restore block is done,
-        // so they are not affected by the saveLayer's paint.
-        const int kSavesToIgnore = 1;
-        Bounds clip = SkRect::Make(op.devBounds);
-        if (this->adjustForSaveLayerPaints(&clip, kSavesToIgnore)) {
-            fCurrentClipBounds = clip.intersect(fCullRect) ? clip : Bounds::MakeEmpty();
-        } else {
-            fCurrentClipBounds = fCullRect;
-        }
-    }
-
-    // We also take advantage of SaveLayer bounds when present to further cut the clip down.
-    void updateClipBounds(const SaveLayer& op)  {
-        if (op.bounds) {
-            // adjustAndMap() intersects these layer bounds with the previous clip for us.
-            fCurrentClipBounds = this->adjustAndMap(*op.bounds, op.paint);
-        }
-    }
-
     // The bounds of these ops must be calculated when we hit the Restore
     // from the bounds of the ops in the same Save block.
     void trackBounds(const Save&)          { this->pushSaveBlock(nullptr); }
@@ -311,10 +265,10 @@ private:
         // Starting a new Save block.  Push a new entry to represent that.
         SaveBounds sb;
         sb.controlOps = 0;
-        // If the paint affects transparent black, the bound shouldn't be smaller
-        // than the current clip bounds.
+        // If the paint affects transparent black,
+        // the bound shouldn't be smaller than the cull.
         sb.bounds =
-            PaintMayAffectTransparentBlack(paint) ? fCurrentClipBounds : Bounds::MakeEmpty();
+            PaintMayAffectTransparentBlack(paint) ? fCullRect : Bounds::MakeEmpty();
         sb.paint = paint;
         sb.ctm = this->fCTM;
 
@@ -390,12 +344,12 @@ private:
         }
     }
 
-    Bounds bounds(const Flush&) const { return fCurrentClipBounds; }
+    Bounds bounds(const Flush&) const { return fCullRect; }
 
     // FIXME: this method could use better bounds
-    Bounds bounds(const DrawText&) const { return fCurrentClipBounds; }
+    Bounds bounds(const DrawText&) const { return fCullRect; }
 
-    Bounds bounds(const DrawPaint&) const { return fCurrentClipBounds; }
+    Bounds bounds(const DrawPaint&) const { return fCullRect; }
     Bounds bounds(const NoOp&)  const { return Bounds::MakeEmpty(); }    // NoOps don't draw.
 
     Bounds bounds(const DrawRect& op) const { return this->adjustAndMap(op.rect, &op.paint); }
@@ -428,7 +382,7 @@ private:
         return this->adjustAndMap(op.dst, op.paint);
     }
     Bounds bounds(const DrawPath& op) const {
-        return op.path.isInverseFillType() ? fCurrentClipBounds
+        return op.path.isInverseFillType() ? fCullRect
                                            : this->adjustAndMap(op.path.getBounds(), &op.paint);
     }
     Bounds bounds(const DrawPoints& op) const {
@@ -456,7 +410,7 @@ private:
             // for the paint (by the caller)?
             return this->adjustAndMap(*op.cull, op.paint);
         } else {
-            return fCurrentClipBounds;
+            return fCullRect;
         }
     }
 
@@ -518,7 +472,7 @@ private:
         if (op.cull) {
             return this->adjustAndMap(*op.cull, nullptr);
         } else {
-            return fCurrentClipBounds;
+            return fCullRect;
         }
     }
 
@@ -596,12 +550,10 @@ private:
     // Conservative identity-space bounds for each op in the SkRecord.
     Bounds* fBounds;
 
-    // We walk fCurrentOp through the SkRecord, as we go using updateCTM()
-    // and updateClipBounds() to maintain the exact CTM (fCTM) and conservative
-    // identity-space bounds of the current clip (fCurrentClipBounds).
+    // We walk fCurrentOp through the SkRecord,
+    // as we go using updateCTM() to maintain the exact CTM (fCTM).
     int fCurrentOp;
     SkMatrix fCTM;
-    Bounds fCurrentClipBounds;
 
     // Used to track the bounds of Save/Restore blocks and the control ops inside them.
     SkTDArray<SaveBounds> fSaveStack;