path ops stream-lining

The addT() function is a workhorse of pathops.
Make it simpler, removing branches and parameters.

Separate addOpp() into const and modify parts.

Add more debugging that asserts if the function
fails and the data is not extreme (e.g., fuzzer
generated).

TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2273293004

Review-Url: https://codereview.chromium.org/2273293004

diff --git a/src/pathops/SkAddIntersections.cpp b/src/pathops/SkAddIntersections.cpp
index a6cebca4b84daa197b6ea9b43a753766a96f19fe..bdc7ae256556b818cc4f00025d7ecab66cbeb777 100644 (file)
--- a/src/pathops/SkAddIntersections.cpp
+++ b/src/pathops/SkAddIntersections.cpp
@@ -505,11 +505,12 @@ bool AddIntersectTs(SkOpContour* test, SkOpContour* next, SkOpCoincidence* coinc
                 SkASSERT(ts[0][pt] >= 0 && ts[0][pt] <= 1);
                 SkASSERT(ts[1][pt] >= 0 && ts[1][pt] <= 1);
                 wt.segment()->debugValidate();
-                SkOpPtT* testTAt = wt.segment()->addT(ts[swap][pt], nullptr);
+                SkOpPtT* testTAt = wt.segment()->addT(ts[swap][pt]);
                 wn.segment()->debugValidate();
-                SkOpPtT* nextTAt = wn.segment()->addT(ts[!swap][pt], nullptr);
-                if (testTAt->addOpp(nextTAt)) {
-                    testTAt->span()->checkForCollapsedCoincidence();
+                SkOpPtT* nextTAt = wn.segment()->addT(ts[!swap][pt]);
+                SkOpPtT* oppPrev = testTAt->oppPrev(nextTAt);
+                if (oppPrev) {
+                    testTAt->addOpp(nextTAt, oppPrev);
                 }
                 if (testTAt->fPt != nextTAt->fPt) {
                     testTAt->span()->unaligned();

diff --git a/src/pathops/SkOpCoincidence.cpp b/src/pathops/SkOpCoincidence.cpp
index 7717d06a8085190ebbf5e14c00fb911bf30bd328..1c8ab5f524a25828ae83d8fb75134cba3bdb32cf 100755 (executable)
--- a/src/pathops/SkOpCoincidence.cpp
+++ b/src/pathops/SkOpCoincidence.cpp
@@ -299,7 +299,7 @@ bool SkOpCoincidence::addEndMovedSpans(const SkOpSpan* base, const SkOpSpanBase*
                 continue;
             }
             SkOpSegment* writableSeg = const_cast<SkOpSegment*>(testSeg);
-            SkOpPtT* oppStart = writableSeg->addT(t, nullptr);
+            SkOpPtT* oppStart = writableSeg->addT(t);
             SkOpSpan* writableBase = const_cast<SkOpSpan*>(base);
             oppStart->span()->addOppAndMerge(writableBase);
             if (oppStart->deleted()) {
@@ -668,9 +668,9 @@ bool SkOpCoincidence::addOrOverlap(SkOpSegment* coinSeg, SkOpSegment* oppSeg,
     this->debugValidate();
     if (!cs || !os) {
         SkOpPtT* csWritable = cs ? const_cast<SkOpPtT*>(cs)
-            : coinSeg->addT(coinTs, nullptr);
+            : coinSeg->addT(coinTs);
         SkOpPtT* osWritable = os ? const_cast<SkOpPtT*>(os)
-            : oppSeg->addT(oppTs, nullptr);
+            : oppSeg->addT(oppTs);
         RETURN_FALSE_IF(callerAborts, !csWritable || !osWritable);
         csWritable->span()->addOppAndMerge(osWritable->span());
         cs = csWritable;
@@ -679,9 +679,9 @@ bool SkOpCoincidence::addOrOverlap(SkOpSegment* coinSeg, SkOpSegment* oppSeg,
     }
     if (!ce || !oe) {
         SkOpPtT* ceWritable = ce ? const_cast<SkOpPtT*>(ce)
-            : coinSeg->addT(coinTe, nullptr);
+            : coinSeg->addT(coinTe);
         SkOpPtT* oeWritable = oe ? const_cast<SkOpPtT*>(oe)
-            : oppSeg->addT(oppTe, nullptr);
+            : oppSeg->addT(oppTe);
         ceWritable->span()->addOppAndMerge(oeWritable->span());
         ce = ceWritable;
         oe = oeWritable;

diff --git a/src/pathops/SkOpSegment.cpp b/src/pathops/SkOpSegment.cpp
index 87023261c59d159594f21b0ce86e0400c1f295c0..3c63683285a6a7d4bc384f387e71fc89a594360f 100644 (file)
--- a/src/pathops/SkOpSegment.cpp
+++ b/src/pathops/SkOpSegment.cpp
@@ -251,53 +251,39 @@ bool SkOpSegment::addExpanded(double newT, const SkOpSpanBase* test, bool* start
     if (this->contains(newT)) {
         return true;
     }
-    SkOpPtT* newPtT = this->addT(newT, startOver);
+    this->globalState()->resetAllocatedOpSpan();
+    SkOpPtT* newPtT = this->addT(newT);
+    *startOver |= this->globalState()->allocatedOpSpan();
     if (!newPtT) {
         return false;
     }
     newPtT->fPt = this->ptAtT(newT);
     // const cast away to change linked list; pt/t values stays unchanged
-    SkOpSpanBase* writableTest = const_cast<SkOpSpanBase*>(test);
-    if (writableTest->ptT()->addOpp(newPtT)) {
+    SkOpPtT* oppPrev = test->ptT()->oppPrev(newPtT);
+    if (oppPrev) {
+        SkOpSpanBase* writableTest = const_cast<SkOpSpanBase*>(test);
+        writableTest->ptT()->addOpp(newPtT, oppPrev);
         writableTest->checkForCollapsedCoincidence();
     }
     return true;
 }
 
 // Please keep this in sync with debugAddT()
-SkOpPtT* SkOpSegment::addT(double t, bool* allocated) {
+SkOpPtT* SkOpSegment::addT(double t) {
     debugValidate();
     SkPoint pt = this->ptAtT(t);
-    SkOpSpanBase* span = &fHead;
+    SkOpSpanBase* spanBase = &fHead;
     do {
-        SkOpPtT* result = span->ptT();
-        SkOpPtT* loop;
-        bool duplicatePt;
-        if (t == result->fT) {
-            goto bumpSpan;
-        }
-        if (this->match(result, this, t, pt)) {
-            // see if any existing alias matches segment, pt, and t
-            loop = result->next();
-            duplicatePt = false;
-            while (loop != result) {
-                bool ptMatch = loop->fPt == pt;
-                if (loop->segment() == this && loop->fT == t && ptMatch) {
-                    goto bumpSpan;
-                }
-                duplicatePt |= ptMatch;
-                loop = loop->next();
-            }
-    bumpSpan:
-            span->bumpSpanAdds();
+        SkOpPtT* result = spanBase->ptT();
+        if (t == result->fT || this->match(result, this, t, pt)) {
+            spanBase->bumpSpanAdds();
             return result;
         }
         if (t < result->fT) {
             SkOpSpan* prev = result->span()->prev();
-            if (!prev) {
-                return nullptr;
-            }
-            SkOpSpan* span = insert(prev);
+            FAIL_WITH_NULL_IF(!prev);
+            // marks in global state that new op span has been allocated
+            SkOpSpan* span = this->insert(prev);
             span->init(this, prev, t, pt);
             this->debugValidate();
 #if DEBUG_ADD_T
@@ -305,17 +291,12 @@ SkOpPtT* SkOpSegment::addT(double t, bool* allocated) {
                     span->segment()->debugID(), span->debugID());
 #endif
             span->bumpSpanAdds();
-            if (allocated) {
-                *allocated = true;
-            }
             return span->ptT();
         }
-        if (span == &fTail) {
-            return nullptr;
-        }
-    } while ((span = span->upCast()->next()));
+        FAIL_WITH_NULL_IF(spanBase == &fTail);
+    } while ((spanBase = spanBase->upCast()->next()));
     SkASSERT(0);
-    return nullptr;
+    return nullptr;  // we never get here, but need this to satisfy compiler
 }
 
 void SkOpSegment::calcAngles() {

diff --git a/src/pathops/SkOpSegment.h b/src/pathops/SkOpSegment.h
index 312312a153833e1eb5a97144afdf63572e700609..1b7aad199ce5011b61b1a790b1f50d160c2d2688 100644 (file)
--- a/src/pathops/SkOpSegment.h
+++ b/src/pathops/SkOpSegment.h
@@ -92,7 +92,7 @@ public:
         return this;
     }
 
-    SkOpPtT* addT(double t, bool* allocated);
+    SkOpPtT* addT(double t);
 
     template<typename T> T* allocateArray(int count) {
         return SkOpTAllocator<T>::AllocateArray(this->globalState()->allocator(), count);
@@ -128,7 +128,7 @@ public:
     }
 
     void debugAddAngle(double startT, double endT);
-    const SkOpPtT* debugAddT(double t, bool* allocated) const;
+    const SkOpPtT* debugAddT(double t) const;
     const SkOpAngle* debugAngle(int id) const;
 #if DEBUG_ANGLE
     void debugCheckAngleCoin() const;
@@ -231,7 +231,9 @@ public:
     void init(SkPoint pts[], SkScalar weight, SkOpContour* parent, SkPath::Verb verb);
 
     SkOpSpan* insert(SkOpSpan* prev) {
-        SkOpSpan* result = SkOpTAllocator<SkOpSpan>::Allocate(this->globalState()->allocator());
+        SkOpGlobalState* globalState = this->globalState();
+        globalState->setAllocatedOpSpan();
+        SkOpSpan* result = SkOpTAllocator<SkOpSpan>::Allocate(globalState->allocator());
         SkOpSpanBase* next = prev->next();
         result->setPrev(prev);
         prev->setNext(result);

diff --git a/src/pathops/SkOpSpan.cpp b/src/pathops/SkOpSpan.cpp
index 640ba8feb74a0c41dfc5b957edb14295b5b2de30..98165fcfc53c08458d942dfddff78c9a58e468d2 100755 (executable)
--- a/src/pathops/SkOpSpan.cpp
+++ b/src/pathops/SkOpSpan.cpp
@@ -180,7 +180,9 @@ void SkOpPtT::setDeleted() {
 // please keep this in sync with debugAddOppAndMerge
 // If the added points envelop adjacent spans, merge them in.
 void SkOpSpanBase::addOppAndMerge(SkOpSpanBase* opp) {
-    if (this->ptT()->addOpp(opp->ptT())) {
+    SkOpPtT* oppPrev = this->ptT()->oppPrev(opp->ptT());
+    if (oppPrev) {
+        this->ptT()->addOpp(opp->ptT(), oppPrev);
         this->checkForCollapsedCoincidence();
     }
     // compute bounds of points in span

diff --git a/src/pathops/SkOpSpan.h b/src/pathops/SkOpSpan.h
index ebcc984b33fd0346591e7bd3fb6820f80324bacc..96a97e07320547466d2c12b7f436113ba2055114 100644 (file)
--- a/src/pathops/SkOpSpan.h
+++ b/src/pathops/SkOpSpan.h
@@ -29,24 +29,12 @@ public:
     };
 
     // please keep in sync with debugAddOpp()
-    bool addOpp(SkOpPtT* opp) {
-        // find the fOpp ptr to opp
-        SkOpPtT* oppPrev = opp->fNext;
-        if (oppPrev == this) {
-            return false;
-        }
-        while (oppPrev->fNext != opp) {
-            oppPrev = oppPrev->fNext;
-            if (oppPrev == this) {
-                return false;
-            }
-        }
+    void addOpp(SkOpPtT* opp, SkOpPtT* oppPrev) {
         SkOpPtT* oldNext = this->fNext;
         SkASSERT(this != opp);
         this->fNext = opp;
         SkASSERT(oppPrev != oldNext);
         oppPrev->fNext = oldNext;
-        return true;
     }
 
     bool alias() const;
@@ -62,7 +50,7 @@ public:
         return SkDEBUGRELEASE(fID, -1);
     }
 
-    bool debugAddOpp(const SkOpPtT* opp) const;
+    void debugAddOpp(const SkOpPtT* opp, const SkOpPtT* oppPrev) const;
     const SkOpAngle* debugAngle(int id) const;
     const SkOpCoincidence* debugCoincidence() const;
     bool debugContains(const SkOpPtT* ) const;
@@ -70,6 +58,7 @@ public:
     SkOpContour* debugContour(int id);
     int debugLoopLimit(bool report) const;
     bool debugMatchID(int id) const;
+    const SkOpPtT* debugOppPrev(const SkOpPtT* opp) const;
     const SkOpPtT* debugPtT(int id) const;
     void debugResetCoinT() const;
     const SkOpSegment* debugSegment(int id) const;
@@ -109,6 +98,21 @@ public:
 
     bool onEnd() const;
 
+    SkOpPtT* oppPrev(SkOpPtT* opp) const {
+        // find the fOpp ptr to opp
+        SkOpPtT* oppPrev = opp->fNext;
+        if (oppPrev == this) {
+            return nullptr;
+        }
+        while (oppPrev->fNext != opp) {
+            oppPrev = oppPrev->fNext;
+            if (oppPrev == this) {
+                return nullptr;
+            }
+        }
+        return oppPrev;
+    }
+
     static bool Overlaps(const SkOpPtT* s1, const SkOpPtT* e1, const SkOpPtT* s2,
             const SkOpPtT* e2, const SkOpPtT** sOut, const SkOpPtT** eOut) {
         const SkOpPtT* start1 = s1->fT < e1->fT ? s1 : e1;

diff --git a/src/pathops/SkPathOpsDebug.cpp b/src/pathops/SkPathOpsDebug.cpp
index df35be0cfe441e1a5f2b881c92186a03d8990b3e..212e2bcc79637246eb754a68c8e8218e09931e91 100644 (file)
--- a/src/pathops/SkPathOpsDebug.cpp
+++ b/src/pathops/SkPathOpsDebug.cpp
@@ -582,39 +582,21 @@ void SkDRect::debugInit() {
 
 #if DEBUG_COINCIDENCE
 // commented-out lines keep this in sync with addT()
- const SkOpPtT* SkOpSegment::debugAddT(double t, bool* allocated) const {
+ const SkOpPtT* SkOpSegment::debugAddT(double t) const {
     debugValidate();
     SkPoint pt = this->ptAtT(t);
     const SkOpSpanBase* span = &fHead;
     do {
         const SkOpPtT* result = span->ptT();
-        const SkOpPtT* loop;
-        bool duplicatePt;
-        if (t == result->fT) {
-            goto bumpSpan;
-        }
-        if (this->match(result, this, t, pt)) {
-            // see if any existing alias matches segment, pt, and t
-            loop = result->next();
-            duplicatePt = false;
-            while (loop != result) {
-                bool ptMatch = loop->fPt == pt;
-                if (loop->segment() == this && loop->fT == t && ptMatch) {
-                    goto bumpSpan;
-                }
-                duplicatePt |= ptMatch;
-                loop = loop->next();
-            }
-    bumpSpan:
+        if (t == result->fT || this->match(result, this, t, pt)) {
 //             span->bumpSpanAdds();
              return result;
         }
         if (t < result->fT) {
             const SkOpSpan* prev = result->span()->prev();
-            if (!prev) {
-                return nullptr;  // FIXME: this is a fail case; nullptr return elsewhere means result was allocated in non-const version
-            }
-//             SkOpSpan* span = insert(prev, allocator);
+            FAIL_WITH_NULL_IF(!prev);
+            // marks in global state that new op span has been allocated
+            this->globalState()->setAllocatedOpSpan();
 //             span->init(this, prev, t, pt);
             this->debugValidate();
 // #if DEBUG_ADD_T
@@ -622,15 +604,12 @@ void SkDRect::debugInit() {
 //                     span->segment()->debugID(), span->debugID());
 // #endif
 //             span->bumpSpanAdds();
-            if (allocated) {
-                *allocated = true;
-            }
             return nullptr;
         }
-        SkASSERT(span != &fTail);
+        FAIL_WITH_NULL_IF(span != &fTail);
     } while ((span = span->upCast()->next()));
     SkASSERT(0);
-    return nullptr;
+    return nullptr;  // we never get here, but need this to satisfy compiler
 }
 #endif
 
@@ -1493,9 +1472,9 @@ void SkOpCoincidence::debugAddOrOverlap(const SkOpSegment* coinSeg, const SkOpSe
     this->debugValidate();
     if (!cs || !os) {
         if (!cs)
-            cs = coinSeg->debugAddT(coinTs, nullptr);
+            cs = coinSeg->debugAddT(coinTs);
         if (!os)
-            os = oppSeg->debugAddT(oppTs, nullptr);
+            os = oppSeg->debugAddT(oppTs);
         if (cs && os) cs->span()->debugAddOppAndMerge(id, log, os->span(), &csDeleted, &osDeleted);
 //         cs = csWritable;
 //         os = osWritable;
@@ -1505,9 +1484,9 @@ void SkOpCoincidence::debugAddOrOverlap(const SkOpSegment* coinSeg, const SkOpSe
     }
     if (!ce || !oe) {
         if (!ce)
-            ce = coinSeg->debugAddT(coinTe, nullptr);
+            ce = coinSeg->debugAddT(coinTe);
         if (!oe)
-            oe = oppSeg->debugAddT(oppTe, nullptr);
+            oe = oppSeg->debugAddT(oppTe);
         if (ce && oe) ce->span()->debugAddOppAndMerge(id, log, oe->span(), &ceDeleted, &oeDeleted);
 //         ce = ceWritable;
 //         oe = oeWritable;
@@ -2059,7 +2038,9 @@ void SkOpSegment::debugValidate() const {
 // Commented-out lines keep this in sync with addOppAndMerge()
 // If the added points envelop adjacent spans, merge them in.
 void SkOpSpanBase::debugAddOppAndMerge(const char* id, SkPathOpsDebug::GlitchLog* log, const SkOpSpanBase* opp, bool* spanDeleted, bool* oppDeleted) const {
-    if (this->ptT()->debugAddOpp(opp->ptT())) {
+    const SkOpPtT* oppPrev = this->ptT()->debugOppPrev(opp->ptT());
+    if (oppPrev) {
+        this->ptT()->debugAddOpp(opp->ptT(), oppPrev);
         this->debugCheckForCollapsedCoincidence(id, log);
     }
     // compute bounds of points in span
@@ -2315,24 +2296,12 @@ int SkIntersections::debugCoincidentUsed() const {
 #include "SkOpContour.h"
 
 // Commented-out lines keep this in sync with addOpp()
-bool SkOpPtT::debugAddOpp(const SkOpPtT* opp) const {
-    // find the fOpp ptr to opp
-    const SkOpPtT* oppPrev = opp->fNext;
-    if (oppPrev == this) {
-        return false;
-    }
-    while (oppPrev->fNext != opp) {
-        oppPrev = oppPrev->fNext;
-        if (oppPrev == this) {
-            return false;
-        }
-    }
-//    const SkOpPtT* oldNext = this->fNext;
+void SkOpPtT::debugAddOpp(const SkOpPtT* opp, const SkOpPtT* oppPrev) const {
+    SkDEBUGCODE(const SkOpPtT* oldNext = this->fNext);
     SkASSERT(this != opp);
 //    this->fNext = opp;
-//    SkASSERT(oppPrev != oldNext);
+    SkASSERT(oppPrev != oldNext);
 //    oppPrev->fNext = oldNext;
-    return true;
 }
 
 bool SkOpPtT::debugContains(const SkOpPtT* check) const {
@@ -2403,6 +2372,10 @@ int SkOpPtT::debugLoopLimit(bool report) const {
     return 0;
 }
 
+const SkOpPtT* SkOpPtT::debugOppPrev(const SkOpPtT* opp) const {
+    return this->oppPrev(const_cast<SkOpPtT*>(opp));
+}
+
 void SkOpPtT::debugResetCoinT() const {
 #if DEBUG_COINCIDENCE_ORDER
     this->segment()->debugResetCoinT(); 

diff --git a/src/pathops/SkPathOpsDebug.h b/src/pathops/SkPathOpsDebug.h
index c1b1adb7072a750cf3e883cec1f8d3d9f2bf8171..5ea1dd8b2655a9993699140ad56b071d8f0e64ff 100644 (file)
--- a/src/pathops/SkPathOpsDebug.h
+++ b/src/pathops/SkPathOpsDebug.h
@@ -156,12 +156,19 @@
 #include "SkTLS.h"
 #endif
 
-#define FAIL_IF(cond) do { bool fail = (cond); SkOPASSERT(!fail); if (fail) return false; } \
-        while (false)
+// Tests with extreme numbers may fail, but all other tests should never fail.
+#define FAIL_IF(cond) \
+        do { bool fail = (cond); SkOPASSERT(!fail); if (fail) return false; } while (false)
 
+#define FAIL_WITH_NULL_IF(cond) \
+        do { bool fail = (cond); SkOPASSERT(!fail); if (fail) return nullptr; } while (false)
+
+// Some functions serve two masters: one allows the function to fail, the other expects success
+// always. If abort is true, tests with normal numbers may not fail and assert if they do so.
+// If abort is false, both normal and extreme numbers may return false without asserting.
 #define RETURN_FALSE_IF(abort, cond) \
-        do { bool fail = (cond); SkOPASSERT(!(abort) || !fail); if (fail) return false; } \
-        while (false)
+        do { bool fail = (cond); SkOPASSERT(!(abort) || !fail); if (fail) return false; \
+        } while (false)
 
 class SkPathOpsDebug {
 public:

diff --git a/src/pathops/SkPathOpsTypes.h b/src/pathops/SkPathOpsTypes.h
index b336e58855839c5ccb8bb29a7c3b40b06b8a10e3..a6457716340312a1806c2ed576484dcf5f67f21c 100644 (file)
--- a/src/pathops/SkPathOpsTypes.h
+++ b/src/pathops/SkPathOpsTypes.h
@@ -45,6 +45,10 @@ public:
         kMaxWindingTries = 10
     };
 
+    bool allocatedOpSpan() const {
+        return fAllocatedOpSpan;
+    }
+
     SkChunkAlloc* allocator() {
         return fAllocator;
     }
@@ -127,6 +131,14 @@ public:
     Phase phase() const {
         return fPhase;
     }
+    
+    void resetAllocatedOpSpan() {
+        fAllocatedOpSpan = false;
+    }
+
+    void setAllocatedOpSpan() {
+        fAllocatedOpSpan = true;
+    }
 
     void setAngleCoincidence() {
         fAngleCoincidence = true;
@@ -159,6 +171,7 @@ private:
     SkOpCoincidence* fCoincidence;
     SkOpContourHead* fContourHead;
     int fNested;
+    bool fAllocatedOpSpan;
     bool fWindingFailed;
     bool fAngleCoincidence;
     Phase fPhase;

diff --git a/tests/PathOpsAngleTest.cpp b/tests/PathOpsAngleTest.cpp
index 12d1d5579a9826fa789e4b1649ea48bf70a8506a..fe39d40b9cf1f938e13158b692281ab0782253a8 100644 (file)
--- a/tests/PathOpsAngleTest.cpp
+++ b/tests/PathOpsAngleTest.cpp
@@ -478,9 +478,9 @@ DEF_TEST(PathOpsAngleAfter, reporter) {
 
 void SkOpSegment::debugAddAngle(double startT, double endT) {
     SkOpPtT* startPtT = startT == 0 ? fHead.ptT() : startT == 1 ? fTail.ptT()
-            : this->addT(startT, nullptr);
+            : this->addT(startT);
     SkOpPtT* endPtT = endT == 0 ? fHead.ptT() : endT == 1 ? fTail.ptT()
-            : this->addT(endT, nullptr);
+            : this->addT(endT);
     SkOpAngle* angle = SkOpTAllocator<SkOpAngle>::Allocate(this->globalState()->allocator());
     SkOpSpanBase* startSpan = &fHead;
     while (startSpan->ptT() != startPtT) {