Use SkPathRef gen id for SkPath::getGenerationID
authorcommit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81>
Wed, 30 Oct 2013 18:57:55 +0000 (18:57 +0000)
committercommit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81>
Wed, 30 Oct 2013 18:57:55 +0000 (18:57 +0000)
R=mtklein@google.com, robertphillips@google.com

Author: bsalomon@google.com

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

git-svn-id: http://skia.googlecode.com/svn/trunk@12029 2bbb7eff-a529-9590-31e7-b0007b416f81

include/core/SkPath.h
include/core/SkPathRef.h
src/core/SkPath.cpp
src/core/SkPathRef.cpp
tests/PathTest.cpp

index a6674d9..9b5dc4b 100644 (file)
 #include "SkTDArray.h"
 #include "SkRefCnt.h"
 
-#ifdef SK_BUILD_FOR_ANDROID
-#define GEN_ID_INC              fGenerationID++
-#define GEN_ID_PTR_INC(ptr)     (ptr)->fGenerationID++
-#else
-#define GEN_ID_INC
-#define GEN_ID_PTR_INC(ptr)
-#endif
-
 class SkReader32;
 class SkWriter32;
 class SkAutoPathBoundsUpdate;
@@ -40,10 +32,10 @@ public:
     SK_DECLARE_INST_COUNT_ROOT(SkPath);
 
     SkPath();
-    SkPath(const SkPath&);  // Copies fGenerationID on Android.
+    SkPath(const SkPath&);
     ~SkPath();
 
-    SkPath& operator=(const SkPath&);  // Increments fGenerationID on Android.
+    SkPath& operator=(const SkPath&);
     friend  SK_API bool operator==(const SkPath&, const SkPath&);
     friend bool operator!=(const SkPath& a, const SkPath& b) {
         return !(a == b);
@@ -80,7 +72,6 @@ public:
     */
     void setFillType(FillType ft) {
         fFillType = SkToU8(ft);
-        GEN_ID_INC;
     }
 
     /** Returns true if the filltype is one of the Inverse variants */
@@ -92,7 +83,6 @@ public:
      */
     void toggleInverseFillType() {
         fFillType ^= 2;
-        GEN_ID_INC;
      }
 
     enum Convexity {
@@ -914,16 +904,25 @@ public:
      *  If buffer is NULL, it still returns the number of bytes.
      */
     uint32_t writeToMemory(void* buffer) const;
+
     /**
      *  Initialized the region from the buffer, returning the number
      *  of bytes actually read.
      */
     uint32_t readFromMemory(const void* buffer);
 
-#ifdef SK_BUILD_FOR_ANDROID
+    /** Returns a non-zero, globally unique value corresponding to the set of verbs
+        and points in the path (but not the fill type [except on Android skbug.com/1762]).
+        Each time the path is modified, a different generation ID will be returned. 
+    */
     uint32_t getGenerationID() const;
+
+#ifdef SK_BUILD_FOR_ANDROID
+    static const int kPathRefGenIDBitCnt = 30; // leave room for the fill type (skbug.com/1762)
     const SkPath* getSourcePath() const;
     void setSourcePath(const SkPath* path);
+#else
+    static const int kPathRefGenIDBitCnt = 32;
 #endif
 
     SkDEBUGCODE(void validate() const;)
@@ -953,7 +952,6 @@ private:
     mutable uint8_t     fDirection;
     mutable SkBool8     fIsOval;
 #ifdef SK_BUILD_FOR_ANDROID
-    uint32_t            fGenerationID;
     const SkPath*       fSourcePath;
 #endif
 
index d832944..aea0a91 100644 (file)
@@ -227,6 +227,13 @@ public:
      */
     uint32_t writeSize();
 
+    /**
+     * Gets an ID that uniquely identifies the contents of the path ref. If two path refs have the
+     * same ID then they have the same verbs and points. However, two path refs may have the same
+     * contents but different genIDs.
+     */
+    uint32_t genID() const;
+
 private:
     enum SerializationOffsets {
         kIsFinite_SerializationShift = 25,  // requires 1 bit
@@ -380,14 +387,6 @@ private:
         return reinterpret_cast<intptr_t>(fVerbs) - reinterpret_cast<intptr_t>(fPoints);
     }
 
-    /**
-     * Gets an ID that uniquely identifies the contents of the path ref. If two path refs have the
-     * same ID then they have the same verbs and points. However, two path refs may have the same
-     * contents but different genIDs. Zero is reserved and means an ID has not yet been determined
-     * for the path ref.
-     */
-    int32_t genID() const;
-
     SkDEBUGCODE(void validate() const;)
 
     /**
@@ -413,7 +412,7 @@ private:
     enum {
         kEmptyGenID = 1, // GenID reserved for path ref with zero points and zero verbs.
     };
-    mutable int32_t     fGenerationID;
+    mutable uint32_t    fGenerationID;
     SkDEBUGCODE(int32_t fEditorsAttached;) // assert that only one editor in use at any time.
 
     typedef SkRefCnt INHERITED;
index 8f79fbe..83f481a 100644 (file)
@@ -147,7 +147,7 @@ private:
 SkPath::SkPath()
     : fPathRef(SkPathRef::CreateEmpty())
 #ifdef SK_BUILD_FOR_ANDROID
-    , fGenerationID(0)
+    , fSourcePath(NULL)
 #endif
 {
     this->resetFields();
@@ -161,18 +161,15 @@ void SkPath::resetFields() {
     fConvexity = kUnknown_Convexity;
     fDirection = kUnknown_Direction;
     fIsOval = false;
-#ifdef SK_BUILD_FOR_ANDROID
-    GEN_ID_INC;
-    // We don't touch fSourcePath.  It's used to track texture garbage collection, so we don't
-    // want to muck with it if it's been set to something non-NULL.
-#endif
+
+    // We don't touch Android's fSourcePath.  It's used to track texture garbage collection, so we
+    // don't want to muck with it if it's been set to something non-NULL.
 }
 
 SkPath::SkPath(const SkPath& that)
     : fPathRef(SkRef(that.fPathRef.get())) {
     this->copyFields(that);
 #ifdef SK_BUILD_FOR_ANDROID
-    fGenerationID = that.fGenerationID;
     fSourcePath   = that.fSourcePath;
 #endif
     SkDEBUGCODE(that.validate();)
@@ -189,7 +186,6 @@ SkPath& SkPath::operator=(const SkPath& that) {
         fPathRef.reset(SkRef(that.fPathRef.get()));
         this->copyFields(that);
 #ifdef SK_BUILD_FOR_ANDROID
-        GEN_ID_INC;  // Similar to swap, we can't just copy this or it could go back in time.
         fSourcePath = that.fSourcePath;
 #endif
     }
@@ -232,10 +228,6 @@ void SkPath::swap(SkPath& that) {
         SkTSwap<uint8_t>(fDirection, that.fDirection);
         SkTSwap<SkBool8>(fIsOval, that.fIsOval);
 #ifdef SK_BUILD_FOR_ANDROID
-        // It doesn't really make sense to swap the generation IDs here, because they might go
-        // backwards.  To be safe we increment both to mark them both as changed.
-        GEN_ID_INC;
-        GEN_ID_PTR_INC(&that);
         SkTSwap<const SkPath*>(fSourcePath, that.fSourcePath);
 #endif
     }
@@ -328,11 +320,16 @@ bool SkPath::conservativelyContainsRect(const SkRect& rect) const {
     return check_edge_against_rect(prevPt, firstPt, rect, direction);
 }
 
-#ifdef SK_BUILD_FOR_ANDROID
 uint32_t SkPath::getGenerationID() const {
-    return fGenerationID;
+    uint32_t genID = fPathRef->genID();
+#ifdef SK_BUILD_FOR_ANDROID
+    SkASSERT((unsigned)fFillType < (1 << (32 - kPathRefGenIDBitCnt)));
+    genID |= static_cast<uint32_t>(fFillType) << kPathRefGenIDBitCnt;
+#endif
+    return genID;
 }
 
+#ifdef SK_BUILD_FOR_ANDROID
 const SkPath* SkPath::getSourcePath() const {
     return fSourcePath;
 }
@@ -633,14 +630,12 @@ void SkPath::setLastPt(SkScalar x, SkScalar y) {
         fIsOval = false;
         SkPathRef::Editor ed(&fPathRef);
         ed.atPoint(count-1)->set(x, y);
-        GEN_ID_INC;
     }
 }
 
 void SkPath::setConvexity(Convexity c) {
     if (fConvexity != c) {
         fConvexity = c;
-        GEN_ID_INC;
     }
 }
 
@@ -669,8 +664,6 @@ void SkPath::moveTo(SkScalar x, SkScalar y) {
     fLastMoveToIndex = ed.pathRef()->countPoints();
 
     ed.growForVerb(kMove_Verb)->set(x, y);
-
-    GEN_ID_INC;
 }
 
 void SkPath::rMoveTo(SkScalar x, SkScalar y) {
@@ -702,7 +695,6 @@ void SkPath::lineTo(SkScalar x, SkScalar y) {
     ed.growForVerb(kLine_Verb)->set(x, y);
     fSegmentMask |= kLine_SegmentMask;
 
-    GEN_ID_INC;
     DIRTY_AFTER_EDIT;
 }
 
@@ -724,7 +716,6 @@ void SkPath::quadTo(SkScalar x1, SkScalar y1, SkScalar x2, SkScalar y2) {
     pts[1].set(x2, y2);
     fSegmentMask |= kQuad_SegmentMask;
 
-    GEN_ID_INC;
     DIRTY_AFTER_EDIT;
 }
 
@@ -756,7 +747,6 @@ void SkPath::conicTo(SkScalar x1, SkScalar y1, SkScalar x2, SkScalar y2,
         pts[1].set(x2, y2);
         fSegmentMask |= kConic_SegmentMask;
 
-        GEN_ID_INC;
         DIRTY_AFTER_EDIT;
     }
 }
@@ -782,7 +772,6 @@ void SkPath::cubicTo(SkScalar x1, SkScalar y1, SkScalar x2, SkScalar y2,
     pts[2].set(x3, y3);
     fSegmentMask |= kCubic_SegmentMask;
 
-    GEN_ID_INC;
     DIRTY_AFTER_EDIT;
 }
 
@@ -808,7 +797,6 @@ void SkPath::close() {
             case kMove_Verb: {
                 SkPathRef::Editor ed(&fPathRef);
                 ed.growForVerb(kClose_Verb);
-                GEN_ID_INC;
                 break;
             }
             case kClose_Verb:
@@ -894,7 +882,6 @@ void SkPath::addPoly(const SkPoint pts[], int count, bool close) {
         vb[~count] = kClose_Verb;
     }
 
-    GEN_ID_INC;
     DIRTY_AFTER_EDIT;
     SkDEBUGCODE(this->validate();)
 }
@@ -1720,12 +1707,6 @@ void SkPath::transform(const SkMatrix& matrix, SkPath* dst) const {
             dst->fConvexity = fConvexity;
         }
 
-#ifdef SK_BUILD_FOR_ANDROID
-        if (!matrix.isIdentity() && !dst->hasComputedBounds()) {
-            GEN_ID_PTR_INC(dst);
-        }
-#endif
-
         if (kUnknown_Direction == fDirection) {
             dst->fDirection = kUnknown_Direction;
         } else {
@@ -2134,8 +2115,6 @@ uint32_t SkPath::readFromMemory(const void* storage) {
 
     buffer.skipToAlign4();
 
-    GEN_ID_INC;
-
     SkDEBUGCODE(this->validate();)
     return SkToU32(buffer.pos());
 }
index f635c2a..f811b24 100644 (file)
@@ -289,8 +289,9 @@ SkPoint* SkPathRef::growForVerb(int /* SkPath::Verb*/ verb) {
     return ret;
 }
 
-int32_t SkPathRef::genID() const {
+uint32_t SkPathRef::genID() const {
     SkASSERT(!fEditorsAttached);
+    static const uint32_t kMask = (static_cast<int64_t>(1) << SkPath::kPathRefGenIDBitCnt) - 1;
     if (!fGenerationID) {
         if (0 == fPointCnt && 0 == fVerbCnt) {
             fGenerationID = kEmptyGenID;
@@ -299,7 +300,7 @@ int32_t SkPathRef::genID() const {
             // do a loop in case our global wraps around, as we never want to return a 0 or the
             // empty ID
             do {
-                fGenerationID = sk_atomic_inc(&gPathRefGenerationID) + 1;
+                fGenerationID = (sk_atomic_inc(&gPathRefGenerationID) + 1) & kMask;
             } while (fGenerationID <= kEmptyGenID);
         }
     }
index c47b2c4..6544c0b 100644 (file)
@@ -127,7 +127,6 @@ static void test_android_specific_behavior(skiatest::Reporter* reporter) {
     original.setSourcePath(&source);
     original.moveTo(0, 0);
     original.lineTo(1, 1);
-    REPORTER_ASSERT(reporter, original.getGenerationID() > 0);
     REPORTER_ASSERT(reporter, original.getSourcePath() == &source);
 
     uint32_t copyID, assignID;
@@ -137,37 +136,88 @@ static void test_android_specific_behavior(skiatest::Reporter* reporter) {
     REPORTER_ASSERT(reporter, copy.getGenerationID() == original.getGenerationID());
     REPORTER_ASSERT(reporter, copy.getSourcePath() == original.getSourcePath());
 
-    // Test assigment operator.  Increment generation ID, copy source path.
+    // Test assigment operator.  Change generation ID, copy source path.
     SkPath assign;
     assignID = assign.getGenerationID();
     assign = original;
-    REPORTER_ASSERT(reporter, assign.getGenerationID() > assignID);
+    REPORTER_ASSERT(reporter, assign.getGenerationID() != assignID);
     REPORTER_ASSERT(reporter, assign.getSourcePath() == original.getSourcePath());
 
-    // Test rewind.  Increment generation ID, don't touch source path.
+    // Test rewind.  Change generation ID, don't touch source path.
     copyID = copy.getGenerationID();
     copy.rewind();
-    REPORTER_ASSERT(reporter, copy.getGenerationID() > copyID);
+    REPORTER_ASSERT(reporter, copy.getGenerationID() != copyID);
     REPORTER_ASSERT(reporter, copy.getSourcePath() == original.getSourcePath());
 
-    // Test reset.  Increment generation ID, don't touch source path.
+    // Test reset.  Change generation ID, don't touch source path.
     assignID = assign.getGenerationID();
     assign.reset();
-    REPORTER_ASSERT(reporter, assign.getGenerationID() > assignID);
+    REPORTER_ASSERT(reporter, assign.getGenerationID() != assignID);
     REPORTER_ASSERT(reporter, assign.getSourcePath() == original.getSourcePath());
 
-    // Test swap.  Increment both generation IDs, swap source paths.
+    // Test swap.  Swap the generation IDs, swap source paths.
+    copy.reset();
+    copy.moveTo(2, 2);
     copy.setSourcePath(&anotherSource);
     copyID = copy.getGenerationID();
+    assign.moveTo(3, 3);
     assignID = assign.getGenerationID();
     copy.swap(assign);
-    REPORTER_ASSERT(reporter, copy.getGenerationID() > copyID);
-    REPORTER_ASSERT(reporter, assign.getGenerationID() > assignID);
+    REPORTER_ASSERT(reporter, copy.getGenerationID() != copyID);
+    REPORTER_ASSERT(reporter, assign.getGenerationID() != assignID);
     REPORTER_ASSERT(reporter, copy.getSourcePath() == original.getSourcePath());
     REPORTER_ASSERT(reporter, assign.getSourcePath() == &anotherSource);
 #endif
 }
 
+static void test_gen_id(skiatest::Reporter* reporter) {
+    SkPath a, b;
+    REPORTER_ASSERT(reporter, a.getGenerationID() == b.getGenerationID());
+
+    a.moveTo(0, 0);
+    const uint32_t z = a.getGenerationID();
+    REPORTER_ASSERT(reporter, z != b.getGenerationID());
+
+    a.reset();
+    REPORTER_ASSERT(reporter, a.getGenerationID() == b.getGenerationID());
+
+    a.moveTo(1, 1);
+    const uint32_t y = a.getGenerationID();
+    REPORTER_ASSERT(reporter, z != y);
+
+    b.moveTo(2, 2);
+    const uint32_t x = b.getGenerationID();
+    REPORTER_ASSERT(reporter, x != y && x != z);
+
+    a.swap(b);
+    REPORTER_ASSERT(reporter, b.getGenerationID() == y && a.getGenerationID() == x);
+
+    b = a;
+    REPORTER_ASSERT(reporter, b.getGenerationID() == x);
+
+    SkPath c(a);
+    REPORTER_ASSERT(reporter, c.getGenerationID() == x);
+
+    c.lineTo(3, 3);
+    const uint32_t w = c.getGenerationID();
+    REPORTER_ASSERT(reporter, b.getGenerationID() == x);
+    REPORTER_ASSERT(reporter, a.getGenerationID() == x);
+    REPORTER_ASSERT(reporter, w != x);
+
+#ifdef SK_BUILD_FOR_ANDROID
+    static bool kExpectGenIDToIgnoreFill = false;
+#else
+    static bool kExpectGenIDToIgnoreFill = true;
+#endif
+
+    c.toggleInverseFillType();
+    const uint32_t v = c.getGenerationID();
+    REPORTER_ASSERT(reporter, (v == w) == kExpectGenIDToIgnoreFill);
+
+    c.rewind();
+    REPORTER_ASSERT(reporter, v != c.getGenerationID());
+}
+
 // This used to assert in the debug build, as the edges did not all line-up.
 static void test_bad_cubic_crbug234190() {
     SkPath path;
@@ -2620,6 +2670,7 @@ static void TestPath(skiatest::Reporter* reporter) {
     test_bad_cubic_crbug229478();
     test_bad_cubic_crbug234190();
     test_android_specific_behavior(reporter);
+    test_gen_id(reporter);
     test_path_close_issue1474(reporter);
     test_path_to_region(reporter);
 }