We need to adjust the bounds of clip ops with SaveLayer paints too.

Before this CL, SkRecord only adjusted the bounds of draw ops for SaveLayers' paints.
That worked fine, but as a final step we intersect the bounds of draw ops with the
bounds of the current clip, essentially undoing all that work.

I think the right fix here is to also adjust the bounds of the clip ops.

BUG=skia:2957, 415468
R=robertphillips@google.com

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

2 files changed, 60 insertions, 15 deletions

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index a94312225..bdebcb7c1 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -7,6 +7,7 @@
 
 #include "SkRecordDraw.h"
 #include "SkPatchUtils.h"
+#include "SkTLogic.h"
 
 void SkRecordDraw(const SkRecord& record,
                   SkCanvas* canvas,
@@ -184,17 +185,27 @@ private:
     void updateCTM(const Restore& op)   { fCTM = &op.matrix; }
     void updateCTM(const SetMatrix& op) { fCTM = &op.matrix; }
 
-    template <typename T> void updateClipBounds(const T&) { /* most ops don't change the clip */ }
-    // Each of these devBounds fields is the state of the device bounds after the op.
-    // So Restore's devBounds are those bounds saved by its paired Save or SaveLayer.
-    void updateClipBounds(const Restore& op)    { fCurrentClipBounds = Bounds::Make(op.devBounds); }
-    void updateClipBounds(const ClipPath& op)   { fCurrentClipBounds = Bounds::Make(op.devBounds); }
-    void updateClipBounds(const ClipRRect& op)  { fCurrentClipBounds = Bounds::Make(op.devBounds); }
-    void updateClipBounds(const ClipRect& op)   { fCurrentClipBounds = Bounds::Make(op.devBounds); }
-    void updateClipBounds(const ClipRegion& op) { fCurrentClipBounds = Bounds::Make(op.devBounds); }
+    // Most ops don't change the clip.  Those that do generally have a field named devBounds.
+    SK_CREATE_MEMBER_DETECTOR(devBounds);
+
+    template <typename T>
+    SK_WHEN(!HasMember_devBounds<T>, void) updateClipBounds(const T& op) {}
+
+    // Each of the devBounds fields holds the state of the device bounds after the op.
+    // (So Restore's devBounds are those bounds saved by its paired Save or SaveLayer.)
+    template <typename T>
+    SK_WHEN(HasMember_devBounds<T>, void) updateClipBounds(const T& op) {
+        Bounds clip = SkRect::Make(op.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.
+        fCurrentClipBounds = this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
+    }
+
+    // We also take advantage of SaveLayer bounds when present to further cut the clip down.
     void updateClipBounds(const SaveLayer& op)  {
         if (op.bounds) {
-            fCurrentClipBounds.intersect(this->adjustAndMap(*op.bounds, op.paint));
+            // adjustAndMap() intersects these layer bounds with the previous clip for us.
+            fCurrentClipBounds = this->adjustAndMap(*op.bounds, op.paint);
         }
     }
 
@@ -467,6 +478,15 @@ private:
         return true;
     }
 
+    bool adjustForSaveLayerPaints(SkRect* rect) const {
+        for (int i = fSaveStack.count() - 1; i >= 0; i--) {
+            if (!AdjustForPaint(fSaveStack[i].paint, rect)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     // Adjust rect for all paints that may affect its geometry, then map it to identity space.
     Bounds adjustAndMap(SkRect rect, const SkPaint* paint) const {
         // Inverted rectangles really confuse our BBHs.
@@ -479,11 +499,9 @@ private:
         }
 
         // Adjust rect for all the paints from the SaveLayers we're inside.
-        for (int i = fSaveStack.count() - 1; i >= 0; i--) {
-            if (!AdjustForPaint(fSaveStack[i].paint, &rect)) {
-                // Same deal as above.
-                return fCurrentClipBounds;
-            }
+        if (!this->adjustForSaveLayerPaints(&rect)) {
+            // Same deal as above.
+            return fCurrentClipBounds;
         }
 
         // Map the rect back to identity space.
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 70f0250cc..1108af6e3 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -10,9 +10,10 @@
 
 #include "SkDebugCanvas.h"
 #include "SkDrawPictureCallback.h"
+#include "SkDropShadowImageFilter.h"
 #include "SkRecord.h"
-#include "SkRecordOpts.h"
 #include "SkRecordDraw.h"
+#include "SkRecordOpts.h"
 #include "SkRecorder.h"
 #include "SkRecords.h"
 
@@ -224,3 +225,29 @@ DEF_TEST(RecordDraw_PartialClear, r) {
     REPORTER_ASSERT(r, drawRect->rect == rect);
     REPORTER_ASSERT(r, drawRect->paint.getColor() == SK_ColorRED);
 }
+
+// A regression test for crbug.com/415468 and skbug.com/2957.
+DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
+    SkRecord record;
+    SkRecorder recorder(&record, 50, 50);
+
+    // We draw a rectangle with a long drop shadow.  We used to not update the clip
+    // bounds based on SaveLayer paints, so the drop shadow could be cut off.
+    SkPaint paint;
+    paint.setImageFilter(SkDropShadowImageFilter::Create(20, 0, 0, 0, SK_ColorBLACK))->unref();
+
+    recorder.saveLayer(NULL, &paint);
+        recorder.clipRect(SkRect::MakeWH(20, 40));
+        recorder.drawRect(SkRect::MakeWH(20, 40), SkPaint());
+    recorder.restore();
+
+    // Under the original bug, all the right edge values would be 20 less than asserted here
+    // because we intersected them with a clip that had not been adjusted for the drop shadow.
+    TestBBH bbh;
+    SkRecordFillBounds(record, &bbh);
+    REPORTER_ASSERT(r, bbh.entries.count() == 4);
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[0].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[1].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[2].bounds, SkRect::MakeLTRB(0, 0, 40, 40)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[3].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
+}