Upgrade GPUOptimize

This will be a bit hairy to review.

The FillBounds and CollectLayers code has diverged significantly resulting in the rendering path seeing different bounds than the hoisting path. This CL merges the FillBounds changes into CollectLayers. A follow on CL will, hopefully, find a way to layer CollectLayers on top of FillBounds.

The only code in CollectLayers that is different from FillBounds is bracketed by "LAYER HOISTING" comments.

NOTREECHECKS=true

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

5 files changed, 454 insertions, 92 deletions

diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index a83490dcbf..d353a2a91c 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -33,6 +33,10 @@ struct SkPictInfo;
 
 class SkRecord;
 
+namespace SkRecords {
+    class CollectLayers;
+};
+
 /** \class SkPicture
 
     The SkPicture class records the drawing commands made to a canvas, to
@@ -276,7 +280,7 @@ private:
     friend class SkPictureRecorder;            // SkRecord-based constructor.
     friend class SkGpuDevice;                  // for fData access
     friend class GrLayerHoister;               // access to fRecord
-    friend class CollectLayers;                // access to fRecord
+    friend class SkRecords::CollectLayers;     // access to fRecord
     friend class SkPicturePlayback;            // to get fData
     friend class ReplaceDraw;
 
diff --git a/src/gpu/GrLayerHoister.cpp b/src/gpu/GrLayerHoister.cpp
index 00576710cd..5d3a383b47 100644
--- a/src/gpu/GrLayerHoister.cpp
+++ b/src/gpu/GrLayerHoister.cpp
@@ -111,7 +111,7 @@ void GrLayerHoister::FindLayersToAtlas(GrContext* context,
             continue;
         }
 
-        SkRect layerRect = SkRect::Make(info.fBounds);
+        SkRect layerRect = info.fBounds;
         if (!layerRect.intersect(query)) {
             continue;
         }
@@ -157,7 +157,7 @@ void GrLayerHoister::FindLayersToHoist(GrContext* context,
             continue;
         }
 
-        SkRect layerRect = SkRect::Make(info.fBounds);
+        SkRect layerRect = info.fBounds;
         if (!layerRect.intersect(query)) {
             continue;
         }
diff --git a/src/gpu/GrPictureUtils.cpp b/src/gpu/GrPictureUtils.cpp
index 4cb6dda18b..9e67fe55df 100644
--- a/src/gpu/GrPictureUtils.cpp
+++ b/src/gpu/GrPictureUtils.cpp
@@ -8,6 +8,7 @@
 #include "GrPictureUtils.h"
 
 #include "SkPaintPriv.h"
+#include "SkPatchUtils.h"
 #include "SkRecord.h"
 #include "SkRecords.h"
 
@@ -17,90 +18,410 @@ SkPicture::AccelData::Key GrAccelData::ComputeAccelDataKey() {
     return gGPUID;
 }
 
+// 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 };
+
+namespace SkRecords {
+
 // SkRecord visitor to gather saveLayer/restore information.
-class CollectLayers {
+class CollectLayers : SkNoncopyable {
 public:
     CollectLayers(const SkPicture* pict, GrAccelData* accelData)
         : fPictureID(pict->uniqueID())
-        , fCTM(&SkMatrix::I())
         , fSaveLayersInStack(0)
         , fAccelData(accelData) {
 
-        pict->cullRect().roundOut(&fCurrentClipBounds);
+        // 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;
 
         if (NULL == pict->fRecord.get()) {
             return;
         }
 
+        fBounds.reset(pict->fRecord->count());
+
         for (fCurrentOp = 0; fCurrentOp < pict->fRecord->count(); ++fCurrentOp) {
             pict->fRecord->visit<void>(fCurrentOp, *this);
         }
 
+        // If we have any lingering unpaired Saves, simulate restores to make
+        // sure all ops in those Save blocks have their bounds calculated.
         while (!fSaveStack.isEmpty()) {
             this->popSaveBlock();
         }
+
+        // Any control ops not part of any Save/Restore block draw everywhere.
+        while (!fControlIndices.isEmpty()) {
+            this->popControl(kUnbounded);
+        }
+
+        //--------- LAYER HOISTING
+        while (!fSaveLayerStack.isEmpty()) {
+            this->popSaveLayerInfo();
+        }
+        //--------- LAYER HOISTING
     }
 
     template <typename T> void operator()(const T& op) {
         this->updateCTM(op);
         this->updateClipBounds(op);
+        this->trackBounds(op);
+        //--------- LAYER HOISTING
         this->trackSaveLayers(op);
+        //--------- LAYER HOISTING
     }
 
 private:
+    // In this file, SkRect are in local coordinates, Bounds are translated back to identity space.
+    typedef SkRect Bounds;
+
+    struct SaveBounds {
+        int controlOps;        // Number of control ops in this Save block, including the Save.
+        Bounds bounds;         // Bounds of everything in the block.
+        const SkPaint* paint;  // Unowned.  If set, adjusts the bounds of all ops in this block.
+    };
 
-    class SaveInfo {
+    //--------- LAYER HOISTING
+    class SaveLayerInfo {
     public:
-        SaveInfo() { }
-        SaveInfo(int opIndex, bool isSaveLayer, const SkPaint* paint, const SkIRect& bounds) 
+        SaveLayerInfo() { }
+        SaveLayerInfo(int opIndex, bool isSaveLayer, const SkPaint* paint, const Bounds& clipBound)
             : fStartIndex(opIndex)
             , fIsSaveLayer(isSaveLayer)
             , fHasNestedSaveLayer(false)
             , fPaint(paint)
-            , fBounds(bounds) {
-
+            , fClipBound(clipBound) {
         }
 
         int            fStartIndex;
         bool           fIsSaveLayer;
         bool           fHasNestedSaveLayer;
         const SkPaint* fPaint;
-        SkIRect        fBounds;
+        Bounds         fClipBound;
     };
+    //--------- LAYER HOISTING
+
+    // Only Restore and SetMatrix change the CTM.
+    template <typename T> void updateCTM(const T&) {}
+    void updateCTM(const Restore& op)   { fCTM = &op.matrix; }
+    void updateCTM(const SetMatrix& op) { fCTM = &op.matrix; }
+
+    // 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.
+        fCurrentClipBounds = this->adjustForSaveLayerPaints(&clip) ? clip : kUnbounded;
+    }
+
+    // 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);
+        fCurrentClipBounds =
+            this->adjustForSaveLayerPaints(&clip, kSavesToIgnore) ? clip : kUnbounded;
+    }
 
-    uint32_t            fPictureID;
-    unsigned int        fCurrentOp;
-    const SkMatrix*     fCTM;
-    SkIRect             fCurrentClipBounds;
-    int                 fSaveLayersInStack;
-    SkTDArray<SaveInfo> fSaveStack;
-    GrAccelData*        fAccelData;
-
-    template <typename T> void updateCTM(const T&) { /* most ops don't change the CTM */ }
-    void updateCTM(const SkRecords::Restore& op)   { fCTM = &op.matrix; }
-    void updateCTM(const SkRecords::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 SkRecords::Restore& op)    { fCurrentClipBounds = op.devBounds; }
-    void updateClipBounds(const SkRecords::ClipPath& op)   { fCurrentClipBounds = op.devBounds; }
-    void updateClipBounds(const SkRecords::ClipRRect& op)  { fCurrentClipBounds = op.devBounds; }
-    void updateClipBounds(const SkRecords::ClipRect& op)   { fCurrentClipBounds = op.devBounds; }
-    void updateClipBounds(const SkRecords::ClipRegion& op) { fCurrentClipBounds = op.devBounds; }
-    void updateClipBounds(const SkRecords::SaveLayer& op)  {
+    // 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);
+        }
+    }
+
+    // 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(NULL); }
+    void trackBounds(const SaveLayer& op)  { this->pushSaveBlock(op.paint); }
+    void trackBounds(const Restore&) { fBounds[fCurrentOp] = this->popSaveBlock(); }
+
+    void trackBounds(const SetMatrix&)         { this->pushControl(); }
+    void trackBounds(const ClipRect&)          { this->pushControl(); }
+    void trackBounds(const ClipRRect&)         { this->pushControl(); }
+    void trackBounds(const ClipPath&)          { this->pushControl(); }
+    void trackBounds(const ClipRegion&)        { this->pushControl(); }
+    void trackBounds(const PushCull&)          { this->pushControl(); }
+    void trackBounds(const PopCull&)           { this->pushControl(); }
+    void trackBounds(const BeginCommentGroup&) { this->pushControl(); }
+    void trackBounds(const AddComment&)        { this->pushControl(); }
+    void trackBounds(const EndCommentGroup&)   { this->pushControl(); }
+    void trackBounds(const DrawData&)          { this->pushControl(); }
+
+    // For all other ops, we can calculate and store the bounds directly now.
+    template <typename T> void trackBounds(const T& op) {
+        fBounds[fCurrentOp] = this->bounds(op);
+        this->updateSaveBounds(fBounds[fCurrentOp]);
+    }
+
+    void pushSaveBlock(const SkPaint* paint) {
+        // Starting a new Save block.  Push a new entry to represent that.
+        SaveBounds sb = { 0, Bounds::MakeEmpty(), paint };
+        fSaveStack.push(sb);
+        this->pushControl();
+    }
+
+    static bool PaintMayAffectTransparentBlack(const SkPaint* paint) {
+        if (paint) {
+            // FIXME: this is very conservative
+            if (paint->getImageFilter() || paint->getColorFilter()) {
+                return true;
+            }
+
+            // Unusual Xfermodes require us to process a saved layer
+            // even with operations outisde the clip.
+            // For example, DstIn is used by masking layers.
+            // https://code.google.com/p/skia/issues/detail?id=1291
+            // https://crbug.com/401593
+            SkXfermode* xfermode = paint->getXfermode();
+            SkXfermode::Mode mode;
+            // SrcOver is ok, and is also the common case with a NULL xfermode.
+            // So we should make that the fast path and bypass the mode extraction
+            // and test.
+            if (xfermode && xfermode->asMode(&mode)) {
+                switch (mode) {
+                    // For each of the following transfer modes, if the source
+                    // alpha is zero (our transparent black), the resulting
+                    // blended alpha is not necessarily equal to the original
+                    // destination alpha.
+                    case SkXfermode::kClear_Mode:
+                    case SkXfermode::kSrc_Mode:
+                    case SkXfermode::kSrcIn_Mode:
+                    case SkXfermode::kDstIn_Mode:
+                    case SkXfermode::kSrcOut_Mode:
+                    case SkXfermode::kDstATop_Mode:
+                    case SkXfermode::kModulate_Mode:
+                        return true;
+                        break;
+                    default:
+                        break;
+                }
+            }
+        }
+        return false;
+    }
+
+    Bounds popSaveBlock() {
+        // We're done the Save block.  Apply the block's bounds to all control ops inside it.
+        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 whole Save block may be part another Save block.
+        this->updateSaveBounds(bounds);
+
+        // If called from a real Restore (not a phony one for balance), it'll need the bounds.
+        return bounds;
+    }
+
+    void pushControl() {
+        fControlIndices.push(fCurrentOp);
+        if (!fSaveStack.isEmpty()) {
+            fSaveStack.top().controlOps++;
+        }
+    }
+
+    void popControl(const Bounds& bounds) {
+        fBounds[fControlIndices.top()] = bounds;
+        fControlIndices.pop();
+    }
+
+    void updateSaveBounds(const Bounds& bounds) {
+        // If we're in a Save block, expand its bounds to cover these bounds too.
+        if (!fSaveStack.isEmpty()) {
+            fSaveStack.top().bounds.join(bounds);
         }
     }
 
-    template <typename T> void trackSaveLayers(const T& op) { 
-        /* most ops aren't involved in saveLayers */ 
+    // 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 DrawPaint&) const { return fCurrentClipBounds; }
+    Bounds bounds(const NoOp&)  const { return Bounds::MakeEmpty(); }    // NoOps don't draw.
+
+    Bounds bounds(const DrawSprite& op) const {
+        const SkBitmap& bm = op.bitmap;
+
+        return Bounds::Make(SkIRect::MakeXYWH(op.left, op.top, bm.width(), bm.height()));  // Ignores the matrix.
+    }
+
+    Bounds bounds(const DrawRect& op) const { return this->adjustAndMap(op.rect, &op.paint); }
+    Bounds bounds(const DrawOval& op) const { return this->adjustAndMap(op.oval, &op.paint); }
+    Bounds bounds(const DrawRRect& op) const {
+        return this->adjustAndMap(op.rrect.rect(), &op.paint);
+    }
+    Bounds bounds(const DrawDRRect& op) const {
+        return this->adjustAndMap(op.outer.rect(), &op.paint);
+    }
+    Bounds bounds(const DrawImage& op) const {
+        const SkImage* image = op.image;
+        SkRect rect = SkRect::MakeXYWH(op.left, op.top,
+                                        SkIntToScalar(image->width()), SkIntToScalar(image->height()));
+
+        return this->adjustAndMap(rect, op.paint);
     }
-    void trackSaveLayers(const SkRecords::Save& s) { this->pushSaveBlock(); }
-    void trackSaveLayers(const SkRecords::SaveLayer& sl) { this->pushSaveLayerBlock(sl.paint); }
-    void trackSaveLayers(const SkRecords::Restore& r) { this->popSaveBlock(); }
-    void trackSaveLayers(const SkRecords::DrawPicture& dp) {
+    Bounds bounds(const DrawImageRect& op) const {
+        return this->adjustAndMap(op.dst, op.paint);
+    }
+    Bounds bounds(const DrawBitmapRectToRect& op) const {
+        return this->adjustAndMap(op.dst, op.paint);
+    }
+    Bounds bounds(const DrawBitmapNine& op) const {
+        return this->adjustAndMap(op.dst, op.paint);
+    }
+    Bounds bounds(const DrawBitmap& op) const {
+        const SkBitmap& bm = op.bitmap;
+        return this->adjustAndMap(SkRect::MakeXYWH(op.left, op.top, SkIntToScalar(bm.width()), SkIntToScalar(bm.height())),
+                                    op.paint);
+    }
+    Bounds bounds(const DrawBitmapMatrix& op) const {
+        const SkBitmap& bm = op.bitmap;
+        SkRect dst = SkRect::Make(SkIRect::MakeWH(bm.width(), bm.height()));
+        op.matrix.mapRect(&dst);
+        return this->adjustAndMap(dst, op.paint);
+    }
+
+    Bounds bounds(const DrawPath& op) const {
+        return op.path.isInverseFillType() ? fCurrentClipBounds
+            : this->adjustAndMap(op.path.getBounds(), &op.paint);
+    }
+    Bounds bounds(const DrawPoints& op) const {
+        SkRect dst;
+        dst.set(op.pts, op.count);
+
+        // Pad the bounding box a little to make sure hairline points' bounds aren't empty.
+        SkScalar stroke = SkMaxScalar(op.paint.getStrokeWidth(), 0.01f);
+        dst.outset(stroke / 2, stroke / 2);
+
+        return this->adjustAndMap(dst, &op.paint);
+    }
+    Bounds bounds(const DrawPatch& op) const {
+        SkRect dst;
+        dst.set(op.cubics, SkPatchUtils::kNumCtrlPts);
+        return this->adjustAndMap(dst, &op.paint);
+    }
+    Bounds bounds(const DrawVertices& op) const {
+        SkRect dst;
+        dst.set(op.vertices, op.vertexCount);
+        return this->adjustAndMap(dst, &op.paint);
+    }
+
+    Bounds bounds(const DrawPicture& op) const {
+        SkRect dst = op.picture->cullRect();
+        if (op.matrix) {
+            op.matrix->mapRect(&dst);
+        }
+        return this->adjustAndMap(dst, op.paint);
+    }
+
+    Bounds bounds(const DrawPosText& op) const {
+        const int N = op.paint.countText(op.text, op.byteLength);
+        if (N == 0) {
+            return Bounds::MakeEmpty();
+        }
+
+        SkRect dst;
+        dst.set(op.pos, N);
+        AdjustTextForFontMetrics(&dst, op.paint);
+        return this->adjustAndMap(dst, &op.paint);
+    }
+    Bounds bounds(const DrawPosTextH& op) const {
+        const int N = op.paint.countText(op.text, op.byteLength);
+        if (N == 0) {
+            return Bounds::MakeEmpty();
+        }
+
+        SkScalar left = op.xpos[0], right = op.xpos[0];
+        for (int i = 1; i < N; i++) {
+            left = SkMinScalar(left, op.xpos[i]);
+            right = SkMaxScalar(right, op.xpos[i]);
+        }
+        SkRect dst = { left, op.y, right, op.y };
+        AdjustTextForFontMetrics(&dst, op.paint);
+        return this->adjustAndMap(dst, &op.paint);
+    }
+    Bounds bounds(const DrawTextOnPath& op) const {
+        SkRect dst = op.path.getBounds();
+
+        // Pad all sides by the maximum padding in any direction we'd normally apply.
+        SkRect pad = { 0, 0, 0, 0 };
+        AdjustTextForFontMetrics(&pad, op.paint);
+
+        // That maximum padding happens to always be the right pad today.
+        SkASSERT(pad.fLeft == -pad.fRight);
+        SkASSERT(pad.fTop == -pad.fBottom);
+        SkASSERT(pad.fRight > pad.fBottom);
+        dst.outset(pad.fRight, pad.fRight);
+
+        return this->adjustAndMap(dst, &op.paint);
+    }
+
+    Bounds bounds(const DrawTextBlob& op) const {
+        SkRect dst = op.blob->bounds();
+        dst.offset(op.x, op.y);
+        return this->adjustAndMap(dst, &op.paint);
+    }
+
+    static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint) {
+#ifdef SK_DEBUG
+        SkRect correct = *rect;
+#endif
+        // crbug.com/373785 ~~> xPad = 4x yPad
+        // crbug.com/424824 ~~> bump yPad from 2x text size to 2.5x
+        const SkScalar yPad = 2.5f * paint.getTextSize(),
+            xPad = 4.0f * yPad;
+        rect->outset(xPad, yPad);
+#ifdef SK_DEBUG
+        SkPaint::FontMetrics metrics;
+        paint.getFontMetrics(&metrics);
+        correct.fLeft += metrics.fXMin;
+        correct.fTop += metrics.fTop;
+        correct.fRight += metrics.fXMax;
+        correct.fBottom += metrics.fBottom;
+        // See skia:2862 for why we ignore small text sizes.
+        SkASSERTF(paint.getTextSize() < 0.001f || rect->contains(correct),
+                    "%f %f %f %f vs. %f %f %f %f\n",
+                    -xPad, -yPad, +xPad, +yPad,
+                    metrics.fXMin, metrics.fTop, metrics.fXMax, metrics.fBottom);
+#endif
+    }
+
+    //--------- LAYER HOISTING
+    template <typename T> void trackSaveLayers(const T& op) {
+        /* most ops aren't involved in saveLayers */
+    }
+    void trackSaveLayers(const Save& s) { this->pushSaveLayerInfo(false, NULL); }
+    void trackSaveLayers(const SaveLayer& sl) {
+        this->pushSaveLayerInfo(true, sl.paint);
+    }
+    void trackSaveLayers(const Restore& r) { this->popSaveLayerInfo(); }
+    void trackSaveLayers(const DrawPicture& dp) {
         // For sub-pictures, we wrap their layer information within the parent
         // picture's rendering hierarchy
         const GrAccelData* childData = GPUOptimize(dp.picture);
@@ -108,10 +429,9 @@ private:
         for (int i = 0; i < childData->numSaveLayers(); ++i) {
             const GrAccelData::SaveLayerInfo& src = childData->saveLayerInfo(i);
 
-            SkRect srcRect = SkRect::Make(src.fBounds);
-            SkIRect newClip(fCurrentClipBounds);
+            Bounds newClip(fCurrentClipBounds);
 
-            if (!newClip.intersect(this->adjustAndMap(srcRect, dp.paint))) {
+            if (!newClip.intersect(this->adjustAndMap(src.fBounds, dp.paint))) {
                 continue;
             }
 
@@ -137,41 +457,39 @@ private:
         }
     }
 
-    void pushSaveBlock() {
-        fSaveStack.push(SaveInfo(fCurrentOp, false, NULL, SkIRect::MakeEmpty()));
-    }
-
     // Inform all the saveLayers already on the stack that they now have a
     // nested saveLayer inside them
     void updateStackForSaveLayer() {
-        for (int index = fSaveStack.count() - 1; index >= 0; --index) {
-            if (fSaveStack[index].fHasNestedSaveLayer) {
+        for (int index = fSaveLayerStack.count() - 1; index >= 0; --index) {
+            if (fSaveLayerStack[index].fHasNestedSaveLayer) {
                 break;
             }
-            fSaveStack[index].fHasNestedSaveLayer = true;
-            if (fSaveStack[index].fIsSaveLayer) {
+            fSaveLayerStack[index].fHasNestedSaveLayer = true;
+            if (fSaveLayerStack[index].fIsSaveLayer) {
                 break;
             }
         }
     }
 
-    void pushSaveLayerBlock(const SkPaint* paint) {
-        this->updateStackForSaveLayer();
+    void pushSaveLayerInfo(bool isSaveLayer, const SkPaint* paint) {
+        if (isSaveLayer) {
+            this->updateStackForSaveLayer();
+            ++fSaveLayersInStack;
+        }
 
-        fSaveStack.push(SaveInfo(fCurrentOp, true, paint, fCurrentClipBounds));
-        ++fSaveLayersInStack;
+        fSaveLayerStack.push(SaveLayerInfo(fCurrentOp, isSaveLayer, paint, fCurrentClipBounds));
     }
 
-    void popSaveBlock() {
-        if (fSaveStack.count() <= 0) {
+    void popSaveLayerInfo() {
+        if (fSaveLayerStack.count() <= 0) {
             SkASSERT(false);
             return;
         }
 
-        SaveInfo si;
-        fSaveStack.pop(&si);
+        SaveLayerInfo sli;
+        fSaveLayerStack.pop(&sli);
 
-        if (!si.fIsSaveLayer) {
+        if (!sli.fIsSaveLayer) {
             return;
         }
 
@@ -180,17 +498,20 @@ private:
         GrAccelData::SaveLayerInfo& slInfo = fAccelData->addSaveLayerInfo();
 
         SkASSERT(NULL == slInfo.fPicture);  // This layer is in the top-most picture
-        slInfo.fBounds = si.fBounds;
+
+        slInfo.fBounds = fBounds[sli.fStartIndex];
+        slInfo.fBounds.intersect(sli.fClipBound);
         slInfo.fLocalMat = *fCTM;
         slInfo.fPreMat = SkMatrix::I();
-        if (si.fPaint) {
-            slInfo.fPaint = SkNEW_ARGS(SkPaint, (*si.fPaint));
+        if (sli.fPaint) {
+            slInfo.fPaint = SkNEW_ARGS(SkPaint, (*sli.fPaint));
         }
-        slInfo.fSaveLayerOpID = si.fStartIndex;
+        slInfo.fSaveLayerOpID = sli.fStartIndex;
         slInfo.fRestoreOpID = fCurrentOp;
-        slInfo.fHasNestedLayers = si.fHasNestedSaveLayer;
+        slInfo.fHasNestedLayers = sli.fHasNestedSaveLayer;
         slInfo.fIsNested = fSaveLayersInStack > 0;
     }
+    //--------- LAYER HOISTING
 
     // Returns true if rect was meaningfully adjusted for the effects of paint,
     // false if the paint could affect the rect in unknown ways.
@@ -205,8 +526,17 @@ private:
         return true;
     }
 
-    // Adjust rect for all paints that may affect its geometry, then map it to device space.
-    SkIRect adjustAndMap(SkRect rect, const SkPaint* paint) const {
+    bool adjustForSaveLayerPaints(SkRect* rect, int savesToIgnore = 0) const {
+        for (int i = fSaveStack.count() - 1 - savesToIgnore; 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.
         rect.sort();
 
@@ -217,25 +547,44 @@ 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].fPaint, &rect)) {
-                // Same deal as above.
-                return fCurrentClipBounds;
-            }
+        if (!this->adjustForSaveLayerPaints(&rect)) {
+            // Same deal as above.
+            return fCurrentClipBounds;
         }
 
-        // Map the rect back to device space.
+        // Map the rect back to identity space.
         fCTM->mapRect(&rect);
-        SkIRect devRect;
-        rect.roundOut(&devRect);
 
         // Nothing can draw outside the current clip.
         // (Only bounded ops call into this method, so oddballs like Clear don't matter here.)
-        devRect.intersect(fCurrentClipBounds);
-        return devRect;
+        rect.intersect(fCurrentClipBounds);
+        return rect;
     }
+
+    // Conservative identity-space bounds for each op in the SkRecord.
+    SkAutoTMalloc<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).
+    unsigned fCurrentOp;
+    const SkMatrix* fCTM;
+    Bounds fCurrentClipBounds;
+
+    // Used to track the bounds of Save/Restore blocks and the control ops inside them.
+    SkTDArray<SaveBounds> fSaveStack;
+    SkTDArray<unsigned>   fControlIndices;
+
+    //--------- LAYER HOISTING
+    // Used to collect saveLayer information for layer hoisting
+    uint32_t              fPictureID;
+    int                   fSaveLayersInStack;
+    SkTDArray<SaveLayerInfo> fSaveLayerStack;
+    GrAccelData*          fAccelData;
+    //--------- LAYER HOISTING
 };
 
+} // namespace SkRecords
 
 // GPUOptimize is only intended to be called within the context of SkGpuDevice's
 // EXPERIMENTAL_optimize method.
@@ -256,7 +605,7 @@ const GrAccelData* GPUOptimize(const SkPicture* pict) {
 
     pict->EXPERIMENTAL_addAccelData(data);
 
-    CollectLayers collector(pict, data);
+    SkRecords::CollectLayers collector(pict, data);
 
     return data;
 }
diff --git a/src/gpu/GrPictureUtils.h b/src/gpu/GrPictureUtils.h
index 2018b0d18d..3b557341ed 100644
--- a/src/gpu/GrPictureUtils.h
+++ b/src/gpu/GrPictureUtils.h
@@ -27,7 +27,7 @@ public:
         // is non-NULL and owns a ref on the picture.
         const SkPicture* fPicture;
         // The device space bounds of this layer.
-        SkIRect fBounds;
+        SkRect fBounds;
         // The pre-matrix begins as the identity and accumulates the transforms
         // of the containing SkPictures (if any). This matrix state has to be
         // part of the initial matrix during replay so that it will be 
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 7c3a4d0fe0..ea32805b49 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -8,6 +8,7 @@
 #include "SkBBoxHierarchy.h"
 #include "SkBlurImageFilter.h"
 #include "SkCanvas.h"
+#include "SkColorMatrixFilter.h"
 #include "SkColorPriv.h"
 #include "SkDashPathEffect.h"
 #include "SkData.h"
@@ -866,6 +867,16 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
         static const int kWidth = 100;
         static const int kHeight = 100;
 
+        // Create complex paint that the bounding box computation code can't
+        // optimize away
+        SkScalar blueToRedMatrix[20] = { 0 };
+        blueToRedMatrix[2] = blueToRedMatrix[18] = SK_Scalar1;
+        SkAutoTUnref<SkColorFilter> blueToRed(SkColorMatrixFilter::Create(blueToRedMatrix));
+        SkAutoTUnref<SkImageFilter> filter(SkColorFilterImageFilter::Create(blueToRed.get()));
+
+        SkPaint complexPaint;
+        complexPaint.setImageFilter(filter);
+
         SkAutoTUnref<SkPicture> pict, child;
 
         {
@@ -873,7 +884,7 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
 
             SkCanvas* c = recorder.beginRecording(SkIntToScalar(kWidth), SkIntToScalar(kHeight));
 
-            c->saveLayer(NULL, NULL);
+            c->saveLayer(NULL, &complexPaint);
             c->restore();
 
             child.reset(recorder.endRecording());
@@ -906,22 +917,20 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
             SkCanvas* c = recorder.beginRecording(SkIntToScalar(kWidth),
                                                   SkIntToScalar(kHeight));
             // 1)
-            c->saveLayer(NULL, NULL); // layer #0
+            c->saveLayer(NULL, &complexPaint); // layer #0
             c->restore();
 
             // 2)
             c->saveLayer(NULL, NULL); // layer #1
                 c->translate(kWidth/2.0f, kHeight/2.0f);
                 SkRect r = SkRect::MakeXYWH(0, 0, kWidth/2, kHeight/2);
-                c->saveLayer(&r, NULL); // layer #2
+                c->saveLayer(&r, &complexPaint); // layer #2
                 c->restore();
             c->restore();
 
             // 3)
             {
-                SkPaint p;
-                p.setColor(SK_ColorRED);
-                c->saveLayer(NULL, &p); // layer #3
+                c->saveLayer(NULL, &complexPaint); // layer #3
                 c->restore();
             }
 
@@ -986,7 +995,7 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
             REPORTER_ASSERT(reporter, info0.fLocalMat.isIdentity());
             REPORTER_ASSERT(reporter, info0.fPreMat.isIdentity());
             REPORTER_ASSERT(reporter, 0 == info0.fBounds.fLeft && 0 == info0.fBounds.fTop);
-            REPORTER_ASSERT(reporter, NULL == info0.fPaint);
+            REPORTER_ASSERT(reporter, NULL != info0.fPaint);
             REPORTER_ASSERT(reporter, !info0.fIsNested && !info0.fHasNestedLayers);
 
             REPORTER_ASSERT(reporter, NULL == info1.fPicture);
@@ -1006,7 +1015,7 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
             REPORTER_ASSERT(reporter, info2.fPreMat.isIdentity());
             REPORTER_ASSERT(reporter, kWidth / 2 == info2.fBounds.fLeft &&   // translated
                                       kHeight / 2 == info2.fBounds.fTop);
-            REPORTER_ASSERT(reporter, NULL == info1.fPaint);
+            REPORTER_ASSERT(reporter, NULL != info2.fPaint);
             REPORTER_ASSERT(reporter, info2.fIsNested && !info2.fHasNestedLayers); // is nested
 
             REPORTER_ASSERT(reporter, NULL == info3.fPicture);
@@ -1034,13 +1043,13 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
             REPORTER_ASSERT(reporter, 0 == info5.fBounds.fLeft && 0 == info5.fBounds.fTop);
             REPORTER_ASSERT(reporter, info5.fLocalMat.isIdentity());
             REPORTER_ASSERT(reporter, info5.fPreMat.isIdentity());
-            REPORTER_ASSERT(reporter, NULL == info5.fPaint);
+            REPORTER_ASSERT(reporter, NULL != info5.fPaint);
             REPORTER_ASSERT(reporter, info5.fIsNested && !info5.fHasNestedLayers); // is nested
 
             REPORTER_ASSERT(reporter, NULL == info6.fPicture);
-            REPORTER_ASSERT(reporter, kWidth == info6.fBounds.width() &&
-                                      kHeight == info6.fBounds.height());
-            REPORTER_ASSERT(reporter, 0 == info6.fBounds.fLeft && 0 == info6.fBounds.fTop);
+            REPORTER_ASSERT(reporter, kWidth-10 == info6.fBounds.width() &&
+                                      kHeight-10 == info6.fBounds.height());
+            REPORTER_ASSERT(reporter, 10 == info6.fBounds.fLeft && 10 == info6.fBounds.fTop);
             REPORTER_ASSERT(reporter, info6.fLocalMat.isIdentity());
             REPORTER_ASSERT(reporter, info6.fPreMat.isIdentity());
             REPORTER_ASSERT(reporter, info6.fPaint);
@@ -1053,7 +1062,7 @@ static void test_gpu_picture_optimization(skiatest::Reporter* reporter,
             REPORTER_ASSERT(reporter, 0 == info7.fBounds.fLeft && 0 == info7.fBounds.fTop);
             REPORTER_ASSERT(reporter, info7.fLocalMat.isIdentity());
             REPORTER_ASSERT(reporter, info7.fPreMat.isIdentity());
-            REPORTER_ASSERT(reporter, NULL == info7.fPaint);
+            REPORTER_ASSERT(reporter, NULL != info7.fPaint);
             REPORTER_ASSERT(reporter, info7.fIsNested && !info7.fHasNestedLayers); // is nested
         }
     }