Proactive SkPixelRef invalidation

include/core/SkPixelRef.h

Index: include/core/SkPixelRef.h
diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h
index 64e08765bed8a3c1e1b9f3f1a652e9959b0a1865..146416cb3cf5940314345e98f37442115b495ddd 100644
--- a/include/core/SkPixelRef.h
+++ b/include/core/SkPixelRef.h
@@ -14,6 +14,7 @@
 #include "SkRefCnt.h"
 #include "SkString.h"
 #include "SkFlattenable.h"
+#include "SkTDArray.h"
 
 #ifdef SK_DEBUG
     /**
@@ -49,6 +50,7 @@ public:
     SK_DECLARE_INST_COUNT(SkPixelRef)
 
     explicit SkPixelRef(SkBaseMutex* mutex = NULL);
+    virtual ~SkPixelRef();
 
     /** Return the pixel memory returned from lockPixels, or null if the
         lockCount is 0.
@@ -207,6 +209,20 @@ public:
     virtual void globalUnref();
 #endif
 
+    // Register a listener to be called the next time our generation ID changes,
+    // if we're confident that we're the only SkPixelRef with that generation ID.
+    // This can be used to invalidate caches keyed by generation ID.

[scroggo, 2013/10/22 21:04:02]

It's not obvious from this comment that an InvalidationListener is only
meaningful/called if someone else knows this pixel ref's generation ID. Could
you add that to the comment?

[mtklein, 2013/10/23 15:28:10]

Yeah, I was hoping you guys would get tripped up by this API and help me find a
clear way to explain the conditions under which we'll actually call the
listeners.  It's pretty weird.  Does this make more sense?

+    struct InvalidationListener {
+        virtual ~InvalidationListener() {}
+        virtual void onInvalidate() = 0;
+    };
+
+    // Takes ownership of listener.
+    void addInvalidationListener(InvalidationListener* listener);
+
+    // Sometimes it's not worth the effort to add a listener that we know will not be called.
+    bool addingAnInvalidationListenerMakesSense() const { return fUniqueGenerationID; }

[scroggo, 2013/10/22 21:04:02]

It was initially odd to me that this function depends on whether
getGenerationID() has been called (since creation/the last invalidation), but I
think adding a comment to InvalidationListener (as I suggested above) would make
that more clear.

[bsalomon, 2013/10/23 13:52:57]

MakesSense seems ambiguous. invalidationListenerMayBeCalled() ?

Or maybe it's just simpler to allow addInvalidationListener() to fail with a
false return if the listener will never be called?

[mtklein, 2013/10/23 15:28:10]

The idea was that we could guard against an unnecessary "l = new Listener;
delete l", which we can't really do in addInvalidationListener().  I think it
may be simpler to just remove this method.  No correctness difference with or
without it.

+
 protected:
     /** Called when the lockCount goes from 0 to 1. The caller will have already
         acquire a mutex for thread safety, so this method need not do that.
@@ -252,17 +268,17 @@ protected:
     void setPreLocked(void* pixels, SkColorTable* ctable);
 
 private:
-
     SkBaseMutex*    fMutex; // must remain in scope for the life of this object
     void*           fPixels;
     SkColorTable*   fColorTable;    // we do not track ownership, subclass does
     int             fLockCount;
 
+    void needsNewGenID();

[scroggo, 2013/10/22 21:04:02]

Style nit: fields should go before methods (these are interspersed).

[mtklein, 2013/10/23 15:28:10]

Done.  Are you sure this is a good thing to be in our style guide?  I find it
more readable to see the fields and their related methods together when they're
closely coupled like this.

     mutable uint32_t fGenerationID;
+    mutable bool     fUniqueGenerationID;
 
-    // SkBitmap is only a friend so that when copying, it can modify the new SkPixelRef to have the
-    // same fGenerationID as the original.
-    friend class SkBitmap;
+    void invalidate();

[scroggo, 2013/10/22 21:04:02]

This name seems to be lacking some context. It makes sense to the only caller to
have an InvalidationListener, but this private function really means "invalidate
anyone who used my genID as a cache." We also have a pattern of using a function
called "validate()" as a test to make sure that the object is valid, which makes
this name seem out of place.

[bsalomon, 2013/10/23 13:52:57]

sendInvalidationEvents(), notifyInvalidationListeners(), ...

[mtklein, 2013/10/23 15:28:10]

How's that?

[bsalomon, 2013/10/23 15:32:27]

good

+    SkTDArray<InvalidationListener*> fInvalidationListeners;  // pointers are owned
 
     SkString    fURI;
 
@@ -273,6 +289,11 @@ private:
 
     void setMutex(SkBaseMutex* mutex);
 
+    // When copying a bitmap to another with the same shape and config, we can safely
+    // clone the pixelref generation ID too, which makes them equivalent under caching.
+    friend class SkBitmap;  // only for cloneGenID
+    void cloneGenID(const SkPixelRef&);
+
     typedef SkFlattenable INHERITED;
 };