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 a6cebca..bdc7ae2 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 7717d06..1c8ab5f 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 8702326..3c63683 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 312312a..1b7aad1 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 640ba8f..98165fc 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 ebcc984..96a97e0 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 df35be0..212e2bc 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 c1b1adb..5ea1dd8 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 b336e58..a645771 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 12d1d55..fe39d40 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) {