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>

5 files changed, 57 insertions, 74 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;
diff --git a/src/core/SkRecorder.cpp b/src/core/SkRecorder.cpp
index 3c4aff37ff..41441864ce 100644
--- a/src/core/SkRecorder.cpp
+++ b/src/core/SkRecorder.cpp
@@ -366,7 +366,7 @@ SkCanvas::SaveLayerStrategy SkRecorder::getSaveLayerStrategy(const SaveLayerRec&
 }
 
 void SkRecorder::didRestore() {
-    APPEND(Restore, this->getDeviceClipBounds(), this->getTotalMatrix());
+    APPEND(Restore, this->getTotalMatrix());
 }
 
 void SkRecorder::didConcat(const SkMatrix& matrix) {
@@ -384,24 +384,24 @@ void SkRecorder::didTranslate(SkScalar dx, SkScalar dy) {
 void SkRecorder::onClipRect(const SkRect& rect, SkClipOp op, ClipEdgeStyle edgeStyle) {
     INHERITED(onClipRect, rect, op, edgeStyle);
     SkRecords::ClipOpAndAA opAA(op, kSoft_ClipEdgeStyle == edgeStyle);
-    APPEND(ClipRect, this->getDeviceClipBounds(), rect, opAA);
+    APPEND(ClipRect, rect, opAA);
 }
 
 void SkRecorder::onClipRRect(const SkRRect& rrect, SkClipOp op, ClipEdgeStyle edgeStyle) {
     INHERITED(onClipRRect, rrect, op, edgeStyle);
     SkRecords::ClipOpAndAA opAA(op, kSoft_ClipEdgeStyle == edgeStyle);
-    APPEND(ClipRRect, this->getDeviceClipBounds(), rrect, opAA);
+    APPEND(ClipRRect, rrect, opAA);
 }
 
 void SkRecorder::onClipPath(const SkPath& path, SkClipOp op, ClipEdgeStyle edgeStyle) {
     INHERITED(onClipPath, path, op, edgeStyle);
     SkRecords::ClipOpAndAA opAA(op, kSoft_ClipEdgeStyle == edgeStyle);
-    APPEND(ClipPath, this->getDeviceClipBounds(), path, opAA);
+    APPEND(ClipPath, path, opAA);
 }
 
 void SkRecorder::onClipRegion(const SkRegion& deviceRgn, SkClipOp op) {
     INHERITED(onClipRegion, deviceRgn, op);
-    APPEND(ClipRegion, this->getDeviceClipBounds(), deviceRgn, op);
+    APPEND(ClipRegion, deviceRgn, op);
 }
 
 sk_sp<SkSurface> SkRecorder::onNewSurface(const SkImageInfo&, const SkSurfaceProps&) {
diff --git a/src/core/SkRecords.h b/src/core/SkRecords.h
index f1e70b6c88..67424aca2a 100644
--- a/src/core/SkRecords.h
+++ b/src/core/SkRecords.h
@@ -177,7 +177,6 @@ struct T {                              \
 RECORD(NoOp, 0);
 RECORD(Flush, 0);
 RECORD(Restore, 0,
-        SkIRect devBounds;
         TypedMatrix matrix);
 RECORD(Save, 0);
 
@@ -212,19 +211,15 @@ private:
 static_assert(sizeof(ClipOpAndAA) == 4, "ClipOpAndAASize");
 
 RECORD(ClipPath, 0,
-        SkIRect devBounds;
         PreCachedPath path;
         ClipOpAndAA opAA);
 RECORD(ClipRRect, 0,
-        SkIRect devBounds;
         SkRRect rrect;
         ClipOpAndAA opAA);
 RECORD(ClipRect, 0,
-        SkIRect devBounds;
         SkRect rect;
         ClipOpAndAA opAA);
 RECORD(ClipRegion, 0,
-        SkIRect devBounds;
         SkRegion region;
         SkClipOp op);
 
diff --git a/tests/PictureBBHTest.cpp b/tests/PictureBBHTest.cpp
index 6a4cd830c8..f01f0fbe93 100644
--- a/tests/PictureBBHTest.cpp
+++ b/tests/PictureBBHTest.cpp
@@ -105,3 +105,33 @@ DEF_TEST(RTreeMakeLargest, r) {
     bbh->insert(rects, SK_ARRAY_COUNT(rects));
     REPORTER_ASSERT(r, bbh->getRootBound() == SkRect::MakeWH(15,15));
 }
+
+DEF_TEST(PictureNegativeSpace, r) {
+    SkRTreeFactory factory;
+    SkPictureRecorder recorder;
+
+    SkRect cull = {-200,-200,+200,+200};
+
+    {
+        auto canvas = recorder.beginRecording(cull, &factory);
+            canvas->save();
+            canvas->clipRect(cull);
+            canvas->drawRect({-20,-20,-10,-10}, SkPaint{});
+            canvas->drawRect({-20,-20,-10,-10}, SkPaint{});
+            canvas->restore();
+        auto pic = recorder.finishRecordingAsPicture();
+        REPORTER_ASSERT(r, pic->approximateOpCount() == 5);
+        REPORTER_ASSERT(r, pic->cullRect() == (SkRect{-20,-20,-10,-10}));
+    }
+
+    // TODO: we should also get the same results without the explicit save/restore
+    if (0) {
+        auto canvas = recorder.beginRecording(cull, &factory);
+            canvas->clipRect(cull);
+            canvas->drawRect({-20,-20,-10,-10}, SkPaint{});
+            canvas->drawRect({-20,-20,-10,-10}, SkPaint{});
+        auto pic = recorder.finishRecordingAsPicture();
+        REPORTER_ASSERT(r, pic->approximateOpCount() == 3);
+        REPORTER_ASSERT(r, pic->cullRect() == (SkRect{-20,-20,-10,-10}));
+    }
+}
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 216865056a..de9d2333cd 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -130,6 +130,8 @@ static bool sloppy_rect_eq(SkRect a, SkRect b) {
     return outset.contains(b) && !inset.contains(b);
 }
 
+// TODO This would be nice, but we can't get it right today.
+#if 0
 DEF_TEST(RecordDraw_BasicBounds, r) {
     SkRecord record;
     SkRecorder recorder(&record, W, H);
@@ -146,6 +148,7 @@ DEF_TEST(RecordDraw_BasicBounds, r) {
         REPORTER_ASSERT(r, sloppy_rect_eq(SkRect::MakeWH(400, 480), bounds[i]));
     }
 }
+#endif
 
 // A regression test for crbug.com/409110.
 DEF_TEST(RecordDraw_TextBounds, r) {
@@ -232,6 +235,8 @@ DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
     REPORTER_ASSERT(r, sloppy_rect_eq(bounds[3], SkRect::MakeLTRB(0, 0, 50, 50)));
 }
 
+// TODO This would be nice, but we can't get it right today.
+#if 0
 // When a saveLayer provides an explicit bound and has a complex paint (e.g., one that
 // affects transparent black), that bound should serve to shrink the area of the required
 // backing store.
@@ -253,6 +258,7 @@ DEF_TEST(RecordDraw_SaveLayerBoundsAffectsClipBounds, r) {
     REPORTER_ASSERT(r, sloppy_rect_eq(bounds[1], SkRect::MakeLTRB(20, 20, 30, 30)));
     REPORTER_ASSERT(r, sloppy_rect_eq(bounds[2], SkRect::MakeLTRB(10, 10, 40, 40)));
 }
+#endif
 
 DEF_TEST(RecordDraw_drawImage, r){
     class SkCanvasMock : public SkCanvas {