Shrink saveLayer device bounds when it supplies an explicit bounds and has a complex paint

This CL shrinks the bound computed for saveLayers that possess both an explicit
bound and a complex paint (e.g., one that affects transparent black). In this
case the bound of the layer should be the clipped explicit bound rather then
the clip prior/after the saveLayer/restore block.

In the following the first bound is the currently computed bound
while the second is the new/desired one:

For a 100x100 picture

saveLayer (no bound, no paint) [ 0 0 100 100 ]  [ 50 50 100 100 ]
  setMatrix (translate 50, 50) [ 0 0 100 100 ] [ 50 50 100 100 ]
  saveLayer (bound of 0, 0, 50, 50 - complex paint) [ 0 0 100 100 ] [ 50 50 100 100 ]
  restore [ 0 0 100 100 ] [ 50 50 100 100 ]
restore [ 0 0 100 100 ] [ 50 50 100 100 ]

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

4 files changed, 54 insertions, 31 deletions

diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index 98c28e61af..dccb822c82 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -631,7 +631,7 @@ SkPicture::SkPicture(SkScalar width, SkScalar height, SkRecord* record, SkBBoxHi
     , fAnalysis(*fRecord) {
     // TODO: delay as much of this work until just before first playback?
     if (fBBH.get()) {
-        SkRecordFillBounds(*fRecord, fBBH.get());
+        SkRecordFillBounds(this->cullRect(), *fRecord, fBBH.get());
     }
     this->needsNewGenID();
 }
diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index ad1327016b..07c02b6a3d 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -116,14 +116,6 @@ DRAW(DrawVertices, drawVertices(r.vmode, r.vertexCount, r.vertices, r.texs, r.co
 DRAW(DrawData, drawData(r.data, r.length));
 #undef DRAW
 
-
-// This looks silly, I know.  Why not just use SkRect::MakeLargest()?
-// In practice, this is well large enough, and it has a few extra advantages:
-// it fits in an SkIRect, and we can munge it a little in both SkRect and
-// SKIRect space without worrying about overflow.
-static const SkRect kUnbounded = { -2e9f, -2e9f, 2e9f, 2e9f };
-
-
 // This is an SkRecord visitor that fills an SkBBoxHierarchy.
 //
 // The interesting part here is how to calculate bounds for ops which don't
@@ -144,11 +136,13 @@ static const SkRect kUnbounded = { -2e9f, -2e9f, 2e9f, 2e9f };
 // in for all the control ops we stashed away.
 class FillBounds : SkNoncopyable {
 public:
-    FillBounds(const SkRecord& record, SkBBoxHierarchy* bbh) : fBounds(record.count()) {
+    FillBounds(const SkRect& cullRect, const SkRecord& record, SkBBoxHierarchy* bbh) 
+        : fCullRect(cullRect)
+        , fBounds(record.count()) {
         // Calculate bounds for all ops.  This won't go quite in order, so we'll need
         // to store the bounds separately then feed them in to the BBH later in order.
         fCTM = &SkMatrix::I();
-        fCurrentClipBounds = kUnbounded;
+        fCurrentClipBounds = fCullRect;
         for (fCurrentOp = 0; fCurrentOp < record.count(); fCurrentOp++) {
             record.visit<void>(fCurrentOp, *this);
         }
@@ -161,7 +155,7 @@ public:
 
         // Any control ops not part of any Save/Restore block draw everywhere.
         while (!fControlIndices.isEmpty()) {
-            this->popControl(kUnbounded);
+            this->popControl(fCullRect);
         }
 
         // Finally feed all stored bounds into the BBH.  They'll be returned in this order.
@@ -204,7 +198,7 @@ private:
         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.
-        fCurrentClipBounds = this->adjustForSaveLayerPaints(&clip) ? clip : kUnbounded;
+        fCurrentClipBounds = this->adjustForSaveLayerPaints(&clip) ? clip : fCullRect;
     }
 
     // Restore holds the devBounds for the clip after the {save,saveLayer}/restore block completes.
@@ -216,7 +210,7 @@ private:
         const int kSavesToIgnore = 1;
         Bounds clip = SkRect::Make(op.devBounds);
         fCurrentClipBounds =
-            this->adjustForSaveLayerPaints(&clip, kSavesToIgnore) ? clip : kUnbounded;
+            this->adjustForSaveLayerPaints(&clip, kSavesToIgnore) ? clip : fCullRect;
     }
 
     // We also take advantage of SaveLayer bounds when present to further cut the clip down.
@@ -253,7 +247,14 @@ private:
 
     void pushSaveBlock(const SkPaint* paint) {
         // Starting a new Save block.  Push a new entry to represent that.
-        SaveBounds sb = { 0, Bounds::MakeEmpty(), paint };
+        SaveBounds sb;
+        sb.controlOps = 0;
+        // If the paint affects transparent black, the bound shouldn't be smaller
+        // than the current clip bounds.
+        sb.bounds =
+            PaintMayAffectTransparentBlack(paint) ? fCurrentClipBounds : Bounds::MakeEmpty();
+        sb.paint = paint;
+
         fSaveStack.push(sb);
         this->pushControl();
     }
@@ -303,19 +304,15 @@ private:
         SaveBounds sb;
         fSaveStack.pop(&sb);
 
-        // If the paint affects transparent black, we can't trust any of our calculated bounds.
-        const Bounds& bounds =
-            PaintMayAffectTransparentBlack(sb.paint) ? fCurrentClipBounds : sb.bounds;
-
         while (sb.controlOps --> 0) {
-            this->popControl(bounds);
+            this->popControl(sb.bounds);
         }
 
         // This whole Save block may be part another Save block.
-        this->updateSaveBounds(bounds);
+        this->updateSaveBounds(sb.bounds);
 
         // If called from a real Restore (not a phony one for balance), it'll need the bounds.
-        return bounds;
+        return sb.bounds;
     }
 
     void pushControl() {
@@ -340,7 +337,7 @@ private:
     // FIXME: this method could use better bounds
     Bounds bounds(const DrawText&) const { return fCurrentClipBounds; }
 
-    Bounds bounds(const Clear&) const { return kUnbounded; }             // Ignores the clip.
+    Bounds bounds(const Clear&) const { return fCullRect; }             // Ignores the clip.
     Bounds bounds(const DrawPaint&) const { return fCurrentClipBounds; }
     Bounds bounds(const NoOp&)  const { return Bounds::MakeEmpty(); }    // NoOps don't draw.
 
@@ -537,6 +534,9 @@ private:
         return rect;
     }
 
+    // We do not guarantee anything for operations outside of the cull rect
+    const SkRect fCullRect;
+
     // Conservative identity-space bounds for each op in the SkRecord.
     SkAutoTMalloc<Bounds> fBounds;
 
@@ -554,6 +554,6 @@ private:
 
 }  // namespace SkRecords
 
-void SkRecordFillBounds(const SkRecord& record, SkBBoxHierarchy* bbh) {
-    SkRecords::FillBounds(record, bbh);
+void SkRecordFillBounds(const SkRect& cullRect, const SkRecord& record, SkBBoxHierarchy* bbh) {
+    SkRecords::FillBounds(cullRect, record, bbh);
 }
diff --git a/src/core/SkRecordDraw.h b/src/core/SkRecordDraw.h
index 75921d182f..701e8352c5 100644
--- a/src/core/SkRecordDraw.h
+++ b/src/core/SkRecordDraw.h
@@ -15,7 +15,7 @@
 #include "SkRecord.h"
 
 // Fill a BBH to be used by SkRecordDraw to accelerate playback.
-void SkRecordFillBounds(const SkRecord&, SkBBoxHierarchy*);
+void SkRecordFillBounds(const SkRect& cullRect, const SkRecord&, SkBBoxHierarchy*);
 
 // Draw an SkRecord into an SkCanvas.  A convenience wrapper around SkRecords::Draw.
 void SkRecordDraw(const SkRecord&, SkCanvas*, const SkBBoxHierarchy*, SkDrawPictureCallback*);
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index 6105c00f7f..1dcd133131 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -137,7 +137,7 @@ DEF_TEST(RecordDraw_BBH, r) {
     recorder.restore();
 
     TestBBH bbh;
-    SkRecordFillBounds(record, &bbh);
+    SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh);
 
     REPORTER_ASSERT(r, bbh.fEntries.count() == 5);
     for (int i = 0; i < bbh.fEntries.count(); i++) {
@@ -163,14 +163,14 @@ DEF_TEST(RecordDraw_TextBounds, r) {
     recorder.drawPosText(text, bytes, pos, SkPaint());
 
     TestBBH bbh;
-    SkRecordFillBounds(record, &bbh);
+    SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh);
     REPORTER_ASSERT(r, bbh.fEntries.count() == 2);
 
     // We can make these next assertions confidently because SkRecordFillBounds
     // builds its bounds by overestimating font metrics in a platform-independent way.
     // If that changes, these tests will need to be more flexible.
-    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(-110,  0, 140, 60)));
-    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(-80, 20, 180, 100)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(0,  0, 140, 60)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 20, 180, 100)));
 }
 
 // Base test to ensure start/stop range is respected
@@ -251,7 +251,7 @@ DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
     // The second bug showed up as adjusting the picture bounds (0,0,50,50) by the drop shadow too.
     // The saveLayer, clipRect, and restore bounds were incorrectly (0,0,70,50).
     TestBBH bbh;
-    SkRecordFillBounds(record, &bbh);
+    SkRecordFillBounds(SkRect::MakeWH(50, 50), record, &bbh);
     REPORTER_ASSERT(r, bbh.fEntries.count() == 4);
     REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
     REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
@@ -259,6 +259,29 @@ DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
     REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[3].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
 }
 
+// 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.
+DEF_TEST(RecordDraw_SaveLayerBoundsAffectsClipBounds, r) {
+    SkRecord record;
+    SkRecorder recorder(&record, 50, 50);
+
+    SkPaint p;
+    p.setXfermodeMode(SkXfermode::kSrc_Mode);
+
+    SkRect bounds = SkRect::MakeLTRB(10, 10, 40, 40);
+    recorder.saveLayer(&bounds, &p);
+    recorder.drawRect(SkRect::MakeLTRB(20, 20, 30, 30), SkPaint());
+    recorder.restore();
+
+    TestBBH bbh;
+    SkRecordFillBounds(SkRect::MakeWH(50, 50), record, &bbh);
+    REPORTER_ASSERT(r, bbh.fEntries.count() == 3);
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(10, 10, 40, 40)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(20, 20, 30, 30)));
+    REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[2].bounds, SkRect::MakeLTRB(10, 10, 40, 40)));
+}
+
 DEF_TEST(RecordDraw_drawImage, r){
     class SkCanvasMock : public SkCanvas {
     public: