Reland "Move conservative bounds tracking from SkCanvas to SkNoPixelsDevice"

This reverts commit 8636e13.

Reason for revert: recording canvases with really big float bounds could
produce a non-empty integer rect bounds that became empty after mapping
it to (0,0,w,h) for the device. This meant resetForNextPictures logic of
updating QR bounds directly from the input bounds allowed state to become
inconsistent with computeDevClipBounds().

PS1->PS4 shows the 1-liner to just compute bounds from the device. This
means that, for now, we preserve the behavior of setting the QR bounds to
be actually empty. skbug.com/10997 is added to fix the underlying issue
with recorders and excessively large float bounds. If that change landed
first, I'd be able to reland this w/o any modifications, but have decided
that it's better to have all locations that modify fQuickRejectBounds use
the exact same expression.

Original change's description:
> Revert "Move conservative bounds tracking from SkCanvas to SkNoPixelsDevice"
>
> This reverts commit 11a3947.
>
> Reason for revert: assert during google3 tests.
>
> Original change's description:
> > Move conservative bounds tracking from SkCanvas to SkNoPixelsDevice
> >
> > Change-Id: I56670b4a4159e21eaa1a58a9a3ee439298d5aa8e
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/335863
> > Reviewed-by: Mike Reed <[email protected]>
> > Commit-Queue: Michael Ludwig <[email protected]>
>
> [email protected],[email protected],[email protected],[email protected]
>
> Change-Id: I7c3a8797460113d9a8ef18d82bbbd64aba2f439c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338316
> Reviewed-by: Michael Ludwig <[email protected]>
> Commit-Queue: Michael Ludwig <[email protected]>

[email protected],[email protected],[email protected],[email protected]

# Not skipping CQ checks because this is a reland.

Change-Id: I1b33e128b4fb4e06b8c7a6ee9b9dcc67202674d8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/338322
Commit-Queue: Michael Ludwig <[email protected]>
Reviewed-by: Michael Ludwig <[email protected]>

Author: lhkbob

include/core/SkCanvas.h

@@ -2636,9 +2636,9 @@ class SK_API SkCanvas {
     sk_sp<SkMarkerStack> fMarkerStack;
 
     // the first N recs that can fit here mean we won't call malloc
-    static constexpr int kMCRecSize      = 128;  // most recent measurement
-    static constexpr int kMCRecCount     = 32;   // common depth for save/restores
-    static constexpr int kDeviceCMSize   = 64;   // most recent measurement
+    static constexpr int kMCRecSize      = 96; // most recent measurement
+    static constexpr int kMCRecCount     = 32; // common depth for save/restores
+    static constexpr int kDeviceCMSize   = 64; // most recent measurement
 
     intptr_t fMCRecStorage[kMCRecSize * kMCRecCount / sizeof(intptr_t)];
     intptr_t fDeviceCMStorage[kDeviceCMSize / sizeof(intptr_t)];
@@ -2758,6 +2758,10 @@ class SK_API SkCanvas {
     bool   fIsScaleTranslate;
     SkRect fQuickRejectBounds;
 
+    // Compute the clip's bounds based on all clipped SkDevice's reported device bounds transformed
+    // into the canvas' global space.
+    SkRect computeDeviceClipBounds() const;
+
     class AutoValidateClip;
     void validateClip() const;
 

src/core/SkCanvas.cpp

@@ -230,7 +230,6 @@ class SkCanvas::MCRec {
     */
     DeviceCM* fTopLayer;
     std::unique_ptr<BackImage> fBackImage;
-    SkConservativeClip fRasterClip;
     SkM44 fMatrix;
     int fDeferredSaveCount;
 
@@ -243,7 +242,7 @@ class SkCanvas::MCRec {
         // don't bother initializing fNext
         inc_rec();
     }
-    MCRec(const MCRec& prev) : fRasterClip(prev.fRasterClip), fMatrix(prev.fMatrix) {
+    MCRec(const MCRec& prev) : fMatrix(prev.fMatrix) {
         fLayer = nullptr;
         fTopLayer = prev.fTopLayer;
         fDeferredSaveCount = 0;
@@ -256,13 +255,12 @@ class SkCanvas::MCRec {
         dec_rec();
     }
 
-    void reset(const SkIRect& bounds) {
+    void reset() {
         SkASSERT(fLayer);
         SkASSERT(fDeferredSaveCount == 0);
         fLayer->validate();
 
         fMatrix.setIdentity();
-        fRasterClip.setRect(bounds);
     }
 };
 
@@ -471,37 +469,35 @@ class AutoLayerForImageFilter {
 
 #define DRAW_END    }
 
-// TODO: conservativeUpdate is temporary to stage moving bounds tracking into SkNoPixelsDevice
-#define UPDATE_DEVICE_CLIP(code, conservativeUpdate)                      \
+#define UPDATE_DEVICE_CLIP(code)                                          \
     do {                                                                  \
     AutoValidateClip avc(this);                                           \
     FOR_EACH_TOP_DEVICE(code);                                            \
-    fMCRec->fRasterClip.conservativeUpdate;                               \
-    fQuickRejectBounds = qr_clip_bounds(fMCRec->fRasterClip.getBounds()); \
+    fQuickRejectBounds = qr_clip_bounds(this->computeDeviceClipBounds()); \
     } while(0)
 
 ////////////////////////////////////////////////////////////////////////////
 
-static inline SkRect qr_clip_bounds(const SkIRect& bounds) {
+static inline SkRect qr_clip_bounds(const SkRect& bounds) {
     if (bounds.isEmpty()) {
         return SkRect::MakeEmpty();
     }
 
     // Expand bounds out by 1 in case we are anti-aliasing.  We store the
     // bounds as floats to enable a faster quick reject implementation.
     SkRect dst;
-    SkNx_cast<float>(Sk4i::Load(&bounds.fLeft) + Sk4i(-1,-1,1,1)).store(&dst.fLeft);
+    (Sk4f::Load(&bounds.fLeft) + Sk4f(-1.f, -1.f, 1.f, 1.f)).store(&dst.fLeft);
     return dst;
 }
 
 void SkCanvas::resetForNextPicture(const SkIRect& bounds) {
     this->restoreToCount(1);
-    fMCRec->reset(bounds);
+    fMCRec->reset();
 
     // We're peering through a lot of structs here.  Only at this scope do we
     // know that the device is a SkNoPixelsDevice.
     static_cast<SkNoPixelsDevice*>(fMCRec->fLayer->fDevice.get())->resetForNextPicture(bounds);
-    fQuickRejectBounds = qr_clip_bounds(bounds);
+    fQuickRejectBounds = qr_clip_bounds(this->computeDeviceClipBounds());
     fIsScaleTranslate = true;
 }
 
@@ -519,7 +515,6 @@ void SkCanvas::init(sk_sp<SkBaseDevice> device) {
 
     fMCRec = (MCRec*)fMCStack.push_back();
     new (fMCRec) MCRec;
-    fMCRec->fRasterClip.setDeviceClipRestriction(&fClipRestrictionRect);
     fIsScaleTranslate = true;
 
     fMCRec->fLayer = (DeviceCM*)fDeviceCMStorage;
@@ -528,19 +523,15 @@ void SkCanvas::init(sk_sp<SkBaseDevice> device) {
     fMCRec->fTopLayer = fMCRec->fLayer;
 
     fSurfaceBase = nullptr;
-    fQuickRejectBounds = {0, 0, 0, 0};
 
     if (device) {
         // The root device and the canvas should always have the same pixel geometry
         SkASSERT(fProps.pixelGeometry() == device->surfaceProps().pixelGeometry());
-        fMCRec->fRasterClip.setRect(device->getGlobalBounds());
-        fQuickRejectBounds = qr_clip_bounds(device->getGlobalBounds());
-
         device->androidFramework_setDeviceClipRestriction(&fClipRestrictionRect);
-
         device->setMarkerStack(fMarkerStack.get());
     }
 
+    fQuickRejectBounds = qr_clip_bounds(this->computeDeviceClipBounds());
     fScratchGlyphRunBuilder = std::make_unique<SkGlyphRunBuilder>();
 }
 
@@ -1167,15 +1158,13 @@ void SkCanvas::internalSaveLayer(const SaveLayerRec& rec, SaveLayerStrategy stra
                 //    replaceClip() simulates what a new top-level device's canvas would be in
                 //    the non-recording scenario. This allows the canvas to report the expanding
                 //    effects of image filters on the temporary clip bounds.
-                UPDATE_DEVICE_CLIP(device->replaceClip(ir),
-                                   setRect(ir));
+                UPDATE_DEVICE_CLIP(device->replaceClip(ir));
             } else {
                 // else the layer device failed to be created, so the saveLayer() effectively
                 // becomes just a save(). The clipRegion() explicitly applies the bounds of the
                 // failed layer, without resetting the clip of the prior device that all subsequent
                 // nested draw calls need to respect.
-                UPDATE_DEVICE_CLIP(device->clipRegion(SkRegion(ir), SkClipOp::kIntersect),
-                                   opRegion(SkRegion(ir), SkRegion::kIntersect_Op));
+                UPDATE_DEVICE_CLIP(device->clipRegion(SkRegion(ir), SkClipOp::kIntersect));
             }
         }
         return;
@@ -1196,10 +1185,6 @@ void SkCanvas::internalSaveLayer(const SaveLayerRec& rec, SaveLayerStrategy stra
 
     newDevice->setOrigin(fMCRec->fMatrix, ir.fLeft, ir.fTop);
     newDevice->androidFramework_setDeviceClipRestriction(&fClipRestrictionRect);
-    if (boundsAffectsClip) {
-        fMCRec->fRasterClip.setRect(ir);
-        fQuickRejectBounds = qr_clip_bounds(ir);
-    }
 
     if (layer->fNext) {
         // need to punch a hole in the previous device, so we don't draw there, given that
@@ -1210,6 +1195,8 @@ void SkCanvas::internalSaveLayer(const SaveLayerRec& rec, SaveLayerStrategy stra
             layer->fDevice->clipRegion(hole, SkClipOp::kDifference);
         } while (layer->fNext);
     }
+
+    fQuickRejectBounds = qr_clip_bounds(this->computeDeviceClipBounds());
 }
 
 int SkCanvas::saveLayerAlpha(const SkRect* bounds, U8CPU alpha) {
@@ -1317,7 +1304,10 @@ void SkCanvas::internalRestore() {
 
     if (fMCRec) {
         fIsScaleTranslate = SkMatrixPriv::IsScaleTranslateAsM33(fMCRec->fMatrix);
-        fQuickRejectBounds = qr_clip_bounds(fMCRec->fRasterClip.getBounds());
+        // Update the quick-reject bounds in case the restore changed the top device or the
+        // removed save record had included modifications to the clip stack.
+        fQuickRejectBounds = qr_clip_bounds(this->computeDeviceClipBounds());
+        this->validateClip();
     }
 }
 
@@ -1586,8 +1576,7 @@ void SkCanvas::clipRect(const SkRect& rect, SkClipOp op, bool doAA) {
 void SkCanvas::onClipRect(const SkRect& rect, SkClipOp op, ClipEdgeStyle edgeStyle) {
     SkASSERT(rect.isSorted());
     const bool isAA = kSoft_ClipEdgeStyle == edgeStyle;
-    UPDATE_DEVICE_CLIP(device->clipRect(rect, op, isAA), \
-                       opRect(rect, fMCRec->fMatrix.asM33(), this->getTopLayerBounds(), (SkRegion::Op) op, isAA));
+    UPDATE_DEVICE_CLIP(device->clipRect(rect, op, isAA));
 }
 
 void SkCanvas::androidFramework_setDeviceClipRestriction(const SkIRect& rect) {
@@ -1597,14 +1586,12 @@ void SkCanvas::androidFramework_setDeviceClipRestriction(const SkIRect& rect) {
         // removing it (i.e. rect is empty).
         this->checkForDeferredSave();
     }
-    UPDATE_DEVICE_CLIP(device->androidFramework_setDeviceClipRestriction(&fClipRestrictionRect),\
-                       opIRect(fClipRestrictionRect, SkRegion::kIntersect_Op));
+    UPDATE_DEVICE_CLIP(device->androidFramework_setDeviceClipRestriction(&fClipRestrictionRect));
 }
 
 void SkCanvas::androidFramework_replaceClip(const SkIRect& rect) {
     this->checkForDeferredSave();
-    UPDATE_DEVICE_CLIP(device->replaceClip(rect), \
-                       setRect(rect));
+    UPDATE_DEVICE_CLIP(device->replaceClip(rect));
 }
 
 void SkCanvas::clipRRect(const SkRRect& rrect, SkClipOp op, bool doAA) {
@@ -1619,8 +1606,7 @@ void SkCanvas::clipRRect(const SkRRect& rrect, SkClipOp op, bool doAA) {
 
 void SkCanvas::onClipRRect(const SkRRect& rrect, SkClipOp op, ClipEdgeStyle edgeStyle) {
     bool isAA = kSoft_ClipEdgeStyle == edgeStyle;
-    UPDATE_DEVICE_CLIP(device->clipRRect(rrect, op, isAA), \
-                       opRRect(rrect, fMCRec->fMatrix.asM33(), this->getTopLayerBounds(), (SkRegion::Op)op, isAA));
+    UPDATE_DEVICE_CLIP(device->clipRRect(rrect, op, isAA));
 }
 
 void SkCanvas::clipPath(const SkPath& path, SkClipOp op, bool doAA) {
@@ -1650,8 +1636,7 @@ void SkCanvas::clipPath(const SkPath& path, SkClipOp op, bool doAA) {
 
 void SkCanvas::onClipPath(const SkPath& path, SkClipOp op, ClipEdgeStyle edgeStyle) {
     bool isAA = kSoft_ClipEdgeStyle == edgeStyle;
-    UPDATE_DEVICE_CLIP(device->clipPath(path, op, isAA), \
-                       opPath(path, fMCRec->fMatrix.asM33(), this->getTopLayerBounds(), (SkRegion::Op)op, isAA));
+    UPDATE_DEVICE_CLIP(device->clipPath(path, op, isAA));
 }
 
 void SkCanvas::clipShader(sk_sp<SkShader> sh, SkClipOp op) {
@@ -1672,8 +1657,7 @@ void SkCanvas::clipShader(sk_sp<SkShader> sh, SkClipOp op) {
 }
 
 void SkCanvas::onClipShader(sk_sp<SkShader> sh, SkClipOp op) {
-    UPDATE_DEVICE_CLIP(device->clipShader(sh, op),
-                       opShader(sh));
+    UPDATE_DEVICE_CLIP(device->clipShader(sh, op));
 }
 
 void SkCanvas::clipRegion(const SkRegion& rgn, SkClipOp op) {
@@ -1682,13 +1666,12 @@ void SkCanvas::clipRegion(const SkRegion& rgn, SkClipOp op) {
 }
 
 void SkCanvas::onClipRegion(const SkRegion& rgn, SkClipOp op) {
-    UPDATE_DEVICE_CLIP(device->clipRegion(rgn, op),
-                       opRegion(rgn, (SkRegion::Op) op));
+    UPDATE_DEVICE_CLIP(device->clipRegion(rgn, op));
 }
 
 void SkCanvas::validateClip() const {
 #ifdef SK_DEBUG
-    SkRect tmp = qr_clip_bounds(this->fMCRec->fRasterClip.getBounds());
+    SkRect tmp = qr_clip_bounds(this->computeDeviceClipBounds());
     if (this->isClipEmpty()) {
         SkASSERT(fQuickRejectBounds.isEmpty());
     } else {
@@ -1735,14 +1718,9 @@ void SkCanvas::temporary_internal_getRgnClip(SkRegion* rgn) {
 ///////////////////////////////////////////////////////////////////////////////
 
 bool SkCanvas::isClipEmpty() const {
-    return fMCRec->fRasterClip.isEmpty();
-
-    // TODO: should we only use the conservative answer in a recording canvas?
-#if 0
     SkBaseDevice* dev = this->getTopDevice();
     // if no device we return true
-    return !dev || dev->onGetClipType() == SkBaseDevice::kEmpty_ClipType;
-#endif
+    return !dev || dev->onGetClipType() == SkBaseDevice::ClipType::kEmpty;
 }
 
 bool SkCanvas::isClipRect() const {
@@ -1841,7 +1819,28 @@ SkRect SkCanvas::getLocalClipBounds() const {
 }
 
 SkIRect SkCanvas::getDeviceClipBounds() const {
-    return fMCRec->fRasterClip.getBounds();
+    return this->computeDeviceClipBounds().roundOut();
+}
+
+SkRect SkCanvas::computeDeviceClipBounds() const {
+    if (this->isClipEmpty()) {
+        return SkRect::MakeEmpty();
+    }
+
+    // Compute the union of all top devices' device bounds, mapped into the global coordinate system
+    DeviceCM* layer = fMCRec->fTopLayer;
+    SkRect totalBounds = SkRect::MakeEmpty();
+    while (layer) {
+        if (layer->fDevice) {
+            SkIRect devClipBounds = layer->fDevice->devClipBounds();
+            SkRect layerBounds =
+                    layer->fDevice->deviceToGlobal().mapRect(SkRect::Make(devClipBounds));
+            totalBounds.join(layerBounds);
+        }
+        layer = layer->fNext;
+    }
+
+    return totalBounds;
 }
 
 ///////////////////////////////////////////////////////////////////////
@@ -2453,7 +2452,10 @@ bool SkCanvas::canDrawBitmapAsSprite(SkScalar x, SkScalar y, int w, int h, const
     SkPoint pt;
     ctm.mapXY(x, y, &pt);
     SkIRect ir = SkIRect::MakeXYWH(SkScalarRoundToInt(pt.x()), SkScalarRoundToInt(pt.y()), w, h);
-    return ir.contains(fMCRec->fRasterClip.getBounds());
+    // quick bounds have been outset by 1px compared to overall device bounds, so this makes the
+    // contains check equivalent to between ir and device bounds
+    ir.outset(1, 1);
+    return ir.contains(fQuickRejectBounds);
 }
 
 // Given storage for a real paint, and an optional paint parameter, clean-up the param (if non-null)