Proactive SkPixelRef invalidation

include/core/SkPixelRef.h

 
 /*
  * Copyright 2008 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkPixelRef_DEFINED
 #define SkPixelRef_DEFINED
 
 #include "SkBitmap.h"
 #include "SkRefCnt.h"
 #include "SkString.h"
 #include "SkFlattenable.h"
+#include "SkTDArray.h"
 
 #ifdef SK_DEBUG
     /**
      *  Defining SK_IGNORE_PIXELREF_SETPRELOCKED will force all pixelref
      *  subclasses to correctly handle lock/unlock pixels. For performance
      *  reasons, simple malloc-based subclasses call setPreLocked() to skip
      *  the overhead of implementing these calls.
      *
      *  This build-flag disables that optimization, to add in debugging our
      *  call-sites, to ensure that they correctly balance their calls of
@@ skipping 15 matching lines @@
     SkBitmap. A pixelref is installed into a bitmap, and then the bitmap can
     access the actual pixel memory by calling lockPixels/unlockPixels.
 
     This class can be shared/accessed between multiple threads.
 */
 class SK_API SkPixelRef : public SkFlattenable {
 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.
     */
     void* pixels() const { return fPixels; }
 
     /** Return the current colorTable (if any) if pixels are locked, or null.
     */
     SkColorTable* colorTable() const { return fColorTable; }
 
@@ skipping 138 matching lines @@
     virtual void globalRef(void* data=NULL);
 
     /**
      *  Release a "global" ref on this object.
      *  The default implementation just calls unref(), but subclasses can override
      *  this method to implement additional behavior.
      */
     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.
     */
     virtual void* onLockPixels(SkColorTable**) = 0;
     /** Called when the lock count goes from 1 to 0. The caller will have
         already acquire a mutex for thread safety, so this method need not do
         that.
     */
     virtual void onUnlockPixels() = 0;
@@ skipping 25 matching lines @@
     // serialization
     SkPixelRef(SkFlattenableReadBuffer&, SkBaseMutex*);
     virtual void flatten(SkFlattenableWriteBuffer&) const SK_OVERRIDE;
 
     // only call from constructor. Flags this to always be locked, removing
     // the need to grab the mutex and call onLockPixels/onUnlockPixels.
     // Performance tweak to avoid those calls (esp. in multi-thread use case).
     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;
 
     // can go from false to true, but never from true to false
     bool    fIsImmutable;
     // only ever set in constructor, const after that
     bool    fPreLocked;
 
     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;
 };
 
 #endif