Revert of Revert of allow canvas to force conservative clips (for speed) (patchset #1 id:1 of https://codereview.chromium.org/554033003/)
Reason for revert:
May just rebaseline, plus want to see the results of the chrome tests, so re-trying this CL.
Original issue's description:
> Revert of allow canvas to force conservative clips (for speed) (patchset #7 id:120001 of https://codereview.chromium.org/541593005/)
>
> Reason for revert:
> multipicturedraw failed on nvprmsaa -- don't know why yet
>
> Original issue's description:
> > Allow SkCanvas to be initialized to force conservative rasterclips. This has the following effects:
> >
> > 1. Queries to the current clip will be conservatively large. This can mean the quickReject may return false more often.
> >
> > 2. The conservative clips mean less work is done.
> >
> > 3. Enabled by default for Gpu, Record, and NoSaveLayer canvases.
> >
> > 4. API is private for now.
> >
> > Committed: https://skia.googlesource.com/skia/+/27a5e656c3d6ef22f9cb34de18e1b960da3aa241
>
> TBR=robertphillips@google.com,bsalomon@google.com,mtklein@google.com,junov@google.com
> NOTREECHECKS=true
> NOTRY=true
>
> Committed: https://skia.googlesource.com/skia/+/6f09709519b79a1159f3826645f1c5fbc101ee11
R=robertphillips@google.com, bsalomon@google.com, mtklein@google.com, junov@google.com, reed@google.com
TBR=bsalomon@google.com, junov@google.com, mtklein@google.com, reed@google.com, robertphillips@google.com
NOTREECHECKS=true
NOTRY=true
Author: reed@chromium.org
Review URL: https://codereview.chromium.org/560713002
diff --git a/src/core/SkRasterClip.cpp b/src/core/SkRasterClip.cpp
index dab91d8..f820c5a 100644
--- a/src/core/SkRasterClip.cpp
+++ b/src/core/SkRasterClip.cpp
@@ -6,18 +6,12 @@
*/
#include "SkRasterClip.h"
-
-
-SkRasterClip::SkRasterClip() {
- fIsBW = true;
- fIsEmpty = true;
- fIsRect = false;
- SkDEBUGCODE(this->validate();)
-}
+#include "SkPath.h"
SkRasterClip::SkRasterClip(const SkRasterClip& src) {
AUTO_RASTERCLIP_VALIDATE(src);
+ fForceConservativeRects = src.fForceConservativeRects;
fIsBW = src.fIsBW;
if (fIsBW) {
fBW = src.fBW;
@@ -30,13 +24,22 @@
SkDEBUGCODE(this->validate();)
}
-SkRasterClip::SkRasterClip(const SkIRect& bounds) : fBW(bounds) {
+SkRasterClip::SkRasterClip(const SkIRect& bounds, bool forceConservativeRects) : fBW(bounds) {
+ fForceConservativeRects = forceConservativeRects;
fIsBW = true;
fIsEmpty = this->computeIsEmpty(); // bounds might be empty, so compute
fIsRect = !fIsEmpty;
SkDEBUGCODE(this->validate();)
}
+SkRasterClip::SkRasterClip(bool forceConservativeRects) {
+ fForceConservativeRects = forceConservativeRects;
+ fIsBW = true;
+ fIsEmpty = true;
+ fIsRect = false;
+ SkDEBUGCODE(this->validate();)
+}
+
SkRasterClip::~SkRasterClip() {
SkDEBUGCODE(this->validate();)
}
@@ -70,9 +73,84 @@
return fIsRect;
}
+/////////////////////////////////////////////////////////////////////////////////////
+
+bool SkRasterClip::setConservativeRect(const SkRect& r, const SkIRect& clipR, bool isInverse) {
+ SkIRect ir;
+ r.roundOut(&ir);
+
+ SkRegion::Op op;
+ if (isInverse) {
+ op = SkRegion::kDifference_Op;
+ } else {
+ op = SkRegion::kIntersect_Op;
+ }
+ fBW.setRect(clipR);
+ fBW.op(ir, op);
+ return this->updateCacheAndReturnNonEmpty();
+}
+
+/////////////////////////////////////////////////////////////////////////////////////
+
+enum MutateResult {
+ kDoNothing_MutateResult,
+ kReplaceClippedAgainstGlobalBounds_MutateResult,
+ kContinue_MutateResult,
+};
+
+static MutateResult mutate_conservative_op(SkRegion::Op* op, bool inverseFilled) {
+ if (inverseFilled) {
+ switch (*op) {
+ case SkRegion::kIntersect_Op:
+ case SkRegion::kDifference_Op:
+ // These ops can only shrink the current clip. So leaving
+ // the clip unchanged conservatively respects the contract.
+ return kDoNothing_MutateResult;
+ case SkRegion::kUnion_Op:
+ case SkRegion::kReplace_Op:
+ case SkRegion::kReverseDifference_Op:
+ case SkRegion::kXOR_Op: {
+ // These ops can grow the current clip up to the extents of
+ // the input clip, which is inverse filled, so we just set
+ // the current clip to the device bounds.
+ *op = SkRegion::kReplace_Op;
+ return kReplaceClippedAgainstGlobalBounds_MutateResult;
+ }
+ }
+ } else {
+ // Not inverse filled
+ switch (*op) {
+ case SkRegion::kIntersect_Op:
+ case SkRegion::kUnion_Op:
+ case SkRegion::kReplace_Op:
+ return kContinue_MutateResult;
+ case SkRegion::kDifference_Op:
+ // Difference can only shrink the current clip.
+ // Leaving clip unchanged conservatively fullfills the contract.
+ return kDoNothing_MutateResult;
+ case SkRegion::kReverseDifference_Op:
+ // To reverse, we swap in the bounds with a replace op.
+ // As with difference, leave it unchanged.
+ *op = SkRegion::kReplace_Op;
+ return kContinue_MutateResult;
+ case SkRegion::kXOR_Op:
+ // Be conservative, based on (A XOR B) always included in (A union B),
+ // which is always included in (bounds(A) union bounds(B))
+ *op = SkRegion::kUnion_Op;
+ return kContinue_MutateResult;
+ }
+ }
+ SkFAIL("should not get here");
+ return kDoNothing_MutateResult;
+}
+
bool SkRasterClip::setPath(const SkPath& path, const SkRegion& clip, bool doAA) {
AUTO_RASTERCLIP_VALIDATE(*this);
+ if (fForceConservativeRects) {
+ return this->setConservativeRect(path.getBounds(), clip.getBounds(), path.isInverseFillType());
+ }
+
if (this->isBW() && !doAA) {
(void)fBW.setPath(path, clip);
} else {
@@ -90,7 +168,22 @@
// base is used to limit the size (and therefore memory allocation) of the
// region that results from scan converting devPath.
SkRegion base;
-
+
+ if (fForceConservativeRects) {
+ SkIRect ir;
+ switch (mutate_conservative_op(&op, path.isInverseFillType())) {
+ case kDoNothing_MutateResult:
+ return !this->isEmpty();
+ case kReplaceClippedAgainstGlobalBounds_MutateResult:
+ ir = SkIRect::MakeSize(size);
+ break;
+ case kContinue_MutateResult:
+ path.getBounds().roundOut(&ir);
+ break;
+ }
+ return this->op(ir, op);
+ }
+
if (SkRegion::kIntersect_Op == op) {
// since we are intersect, we can do better (tighter) with currRgn's
// bounds, than just using the device. However, if currRgn is complex,
@@ -102,7 +195,7 @@
return this->setPath(path, this->bwRgn(), doAA);
} else {
base.setRect(this->getBounds());
- SkRasterClip clip;
+ SkRasterClip clip(fForceConservativeRects);
clip.setPath(path, base, doAA);
return this->op(clip, op);
}
@@ -112,7 +205,7 @@
if (SkRegion::kReplace_Op == op) {
return this->setPath(path, base, doAA);
} else {
- SkRasterClip clip;
+ SkRasterClip clip(fForceConservativeRects);
clip.setPath(path, base, doAA);
return this->op(clip, op);
}
@@ -182,9 +275,24 @@
return x - SkScalarFloorToScalar(x) < domain;
}
-bool SkRasterClip::op(const SkRect& r, SkRegion::Op op, bool doAA) {
+bool SkRasterClip::op(const SkRect& r, const SkISize& size, SkRegion::Op op, bool doAA) {
AUTO_RASTERCLIP_VALIDATE(*this);
+ if (fForceConservativeRects) {
+ SkIRect ir;
+ switch (mutate_conservative_op(&op, false)) {
+ case kDoNothing_MutateResult:
+ return !this->isEmpty();
+ case kReplaceClippedAgainstGlobalBounds_MutateResult:
+ ir = SkIRect::MakeSize(size);
+ break;
+ case kContinue_MutateResult:
+ r.roundOut(&ir);
+ break;
+ }
+ return this->op(ir, op);
+ }
+
if (fIsBW && doAA) {
// check that the rect really needs aa, or is it close enought to
// integer boundaries that we can just treat it as a BW rect?
@@ -251,7 +359,9 @@
void SkRasterClip::convertToAA() {
AUTO_RASTERCLIP_VALIDATE(*this);
-
+
+ SkASSERT(!fForceConservativeRects);
+
SkASSERT(fIsBW);
fAA.setRegion(fBW);
fIsBW = false;