SkBitmap::setPixelRef(): less reference churn

BUG=skia:

Change-Id: I9657e4af5fcc808f9175ff336155374ccc77999d
Reviewed-on: https://skia-review.googlesource.com/5461
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Hal Canary <halcanary@google.com>

16 files changed, 157 insertions, 108 deletions

diff --git a/gn/android_framework_defines.gni b/gn/android_framework_defines.gni
index ea6358d557..40ac342be1 100644
--- a/gn/android_framework_defines.gni
+++ b/gn/android_framework_defines.gni
@@ -12,6 +12,7 @@ android_framework_defines = [
   "SK_SUPPORT_LEGACY_GRADIENT_DITHERING",
   "SK_SUPPORT_LEGACY_DRAWFILTER",
   "SK_IGNORE_GPU_DITHER",
+  "SK_SUPPORT_LEGACY_BITMAP_SETPIXELREF",
   "SK_SUPPORT_LEGACY_IMAGE_ENCODER_CLASS",
   "SK_SUPPORT_LEGACY_SHADER_ISABITMAP",
   "SK_SUPPORT_LEGACY_EMBOSSMASKFILTER",
diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h
index 7c3cd7f9b7..1f195c01ba 100644
--- a/include/core/SkBitmap.h
+++ b/include/core/SkBitmap.h
@@ -117,7 +117,7 @@ public:
      *  dimensions of the bitmap are > 0 (see empty()).
      *  Hey!  Before you use this, see if you really want to know drawsNothing() instead.
      */
-    bool isNull() const { return NULL == fPixelRef; }
+    bool isNull() const { return nullptr == fPixelRef; }
 
     /** Return true iff drawing this bitmap has no effect.
      */
@@ -407,7 +407,7 @@ public:
      *  Return the current pixelref object or NULL if there is none. This does
      *  not affect the refcount of the pixelref.
      */
-    SkPixelRef* pixelRef() const { return fPixelRef; }
+    SkPixelRef* pixelRef() const { return fPixelRef.get(); }
 
     /**
      *  A bitmap can reference a subset of a pixelref's pixels. That means the
@@ -423,6 +423,15 @@ public:
     SkIPoint pixelRefOrigin() const { return fPixelRefOrigin; }
 
     /**
+     * Assign a pixelref and origin to the bitmap.  (dx,dy) specify the offset
+     * within the pixelref's pixels for the top/left corner of the bitmap. For
+     * a bitmap that encompases the entire pixels of the pixelref, these will
+     * be (0,0).
+     */
+    void setPixelRef(sk_sp<SkPixelRef>, int dx, int dy);
+
+#ifdef SK_SUPPORT_LEGACY_BITMAP_SETPIXELREF
+    /**
      *  Assign a pixelref and origin to the bitmap. Pixelrefs are reference,
      *  so the existing one (if any) will be unref'd and the new one will be
      *  ref'd. (x,y) specify the offset within the pixelref's pixels for the
@@ -438,6 +447,7 @@ public:
     SkPixelRef* setPixelRef(SkPixelRef* pr) {
         return this->setPixelRef(pr, 0, 0);
     }
+#endif
 
     /** Call this to ensure that the bitmap points to the current pixel address
         in the pixelref. Balance it with a call to unlockPixels(). These calls
@@ -748,13 +758,13 @@ public:
     SK_TO_STRING_NONVIRT()
 
 private:
-    mutable SkPixelRef* fPixelRef;
-    mutable int         fPixelLockCount;
+    mutable sk_sp<SkPixelRef> fPixelRef;
+    mutable int               fPixelLockCount;
     // These are just caches from the locked pixelref
-    mutable void*       fPixels;
-    mutable SkColorTable* fColorTable;    // only meaningful for kIndex8
+    mutable void*             fPixels;
+    mutable SkColorTable*     fColorTable;    // only meaningful for kIndex8
 
-    SkIPoint    fPixelRefOrigin;
+    SkIPoint                  fPixelRefOrigin;
 
     enum Flags {
         kImageIsVolatile_Flag   = 0x02,
@@ -767,9 +777,9 @@ private:
 #endif
     };
 
-    SkImageInfo fInfo;
-    uint32_t    fRowBytes;
-    uint8_t     fFlags;
+    SkImageInfo               fInfo;
+    uint32_t                  fRowBytes;
+    uint8_t                   fFlags;
 
     /*  Unreference any pixelrefs or colortables
     */
diff --git a/include/core/SkBitmapDevice.h b/include/core/SkBitmapDevice.h
index 17cb9847b1..7ad4abec21 100644
--- a/include/core/SkBitmapDevice.h
+++ b/include/core/SkBitmapDevice.h
@@ -14,6 +14,7 @@
 #include "SkColor.h"
 #include "SkDevice.h"
 #include "SkImageInfo.h"
+#include "SkPixelRef.h"
 #include "SkRect.h"
 #include "SkScalar.h"
 #include "SkSize.h"
@@ -140,10 +141,10 @@ protected:
 
     SkPixelRef* getPixelRef() const { return fBitmap.pixelRef(); }
     // just for subclasses, to assign a custom pixelref
-    SkPixelRef* setPixelRef(SkPixelRef* pr) {
-        fBitmap.setPixelRef(pr);
-        return pr;
-    }
+    void setPixelRef(sk_sp<SkPixelRef> pr) { fBitmap.setPixelRef(std::move(pr), 0, 0); }
+#ifdef SK_SUPPORT_LEGACY_BITMAP_SETPIXELREF
+    SkPixelRef* setPixelRef(SkPixelRef* pr) { return fBitmap.setPixelRef(pr); }
+#endif
 
     bool onReadPixels(const SkImageInfo&, void*, size_t, int x, int y) override;
     bool onWritePixels(const SkImageInfo&, const void*, size_t, int, int) override;
diff --git a/public.bzl b/public.bzl
index 666a598fb0..147f11bc2b 100644
--- a/public.bzl
+++ b/public.bzl
@@ -594,6 +594,7 @@ DEFINES_ALL = [
     # Temporarily Disable analytic AA for Google3
     "SK_NO_ANALYTIC_AA",
     "SK_SUPPORT_LEGACY_CLIPOPS_PLAIN_ENUM",
+    "SK_SUPPORT_LEGACY_BITMAP_SETPIXELREF",
 ]
 
 ################################################################################
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 24ca72adb2..f0ade7017d 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -30,18 +30,48 @@ static bool reset_return_false(SkBitmap* bm) {
     return false;
 }
 
-SkBitmap::SkBitmap() {
-    sk_bzero(this, sizeof(*this));
-}
-
-SkBitmap::SkBitmap(const SkBitmap& src) {
+SkBitmap::SkBitmap()
+    : fPixelLockCount(0)
+    , fPixels        (nullptr)
+    , fColorTable    (nullptr)
+    , fPixelRefOrigin{0, 0}
+    , fRowBytes      (0)
+    , fFlags         (0) {}
+
+// copy pixelref, but don't copy lock.
+SkBitmap::SkBitmap(const SkBitmap& src)
+    : fPixelRef      (src.fPixelRef)
+    , fPixelLockCount(0)
+    , fPixels        (nullptr)
+    , fColorTable    (nullptr)
+    , fPixelRefOrigin(src.fPixelRefOrigin)
+    , fInfo          (src.fInfo)
+    , fRowBytes      (src.fRowBytes)
+    , fFlags         (src.fFlags)
+{
     SkDEBUGCODE(src.validate();)
-    sk_bzero(this, sizeof(*this));
-    *this = src;
     SkDEBUGCODE(this->validate();)
 }
 
-SkBitmap::SkBitmap(SkBitmap&& other) : SkBitmap() { this->swap(other); }
+// take lock and lockcount from other.
+SkBitmap::SkBitmap(SkBitmap&& other)
+    : fPixelRef      (std::move(other.fPixelRef))
+    , fPixelLockCount          (other.fPixelLockCount)
+    , fPixels                  (other.fPixels)
+    , fColorTable              (other.fColorTable)
+    , fPixelRefOrigin          (other.fPixelRefOrigin)
+    , fInfo          (std::move(other.fInfo))
+    , fRowBytes                (other.fRowBytes)
+    , fFlags                   (other.fFlags) {
+    SkASSERT(!other.fPixelRef);
+    other.fInfo.reset();
+    other.fPixelLockCount = 0;
+    other.fPixels         = nullptr;
+    other.fColorTable     = nullptr;
+    other.fPixelRefOrigin = SkIPoint{0, 0};
+    other.fRowBytes       = 0;
+    other.fFlags          = 0;
+}
 
 SkBitmap::~SkBitmap() {
     SkDEBUGCODE(this->validate();)
@@ -51,46 +81,47 @@ SkBitmap::~SkBitmap() {
 SkBitmap& SkBitmap::operator=(const SkBitmap& src) {
     if (this != &src) {
         this->freePixels();
-        this->fPixelRef = SkSafeRef(src.fPixelRef);
-        if (this->fPixelRef) {
-            // ignore the values if we have a pixelRef
-            this->fPixels = nullptr;
-            this->fColorTable = nullptr;
-        } else {
-            this->fPixels = src.fPixels;
-            this->fColorTable = src.fColorTable;
-        }
-        // we reset our locks if we get blown away
-        this->fPixelLockCount = 0;
-
-        this->fPixelRefOrigin = src.fPixelRefOrigin;
-        this->fInfo = src.fInfo;
-        this->fRowBytes = src.fRowBytes;
-        this->fFlags = src.fFlags;
+        SkASSERT(!fPixels);
+        SkASSERT(!fColorTable);
+        SkASSERT(!fPixelLockCount);
+        fPixelRef       = src.fPixelRef;
+        fPixelRefOrigin = src.fPixelRefOrigin;
+        fInfo           = src.fInfo;
+        fRowBytes       = src.fRowBytes;
+        fFlags          = src.fFlags;
     }
-
     SkDEBUGCODE(this->validate();)
     return *this;
 }
 
 SkBitmap& SkBitmap::operator=(SkBitmap&& other) {
     if (this != &other) {
-        this->swap(other);
-        other.reset();
+        this->freePixels();
+        SkASSERT(!fPixels);
+        SkASSERT(!fColorTable);
+        SkASSERT(!fPixelLockCount);
+        fPixelRef       = std::move(other.fPixelRef);
+        fInfo           = std::move(other.fInfo);
+        fPixelLockCount = other.fPixelLockCount;
+        fPixels         = other.fPixels;
+        fColorTable     = other.fColorTable;
+        fPixelRefOrigin = other.fPixelRefOrigin;
+        fRowBytes       = other.fRowBytes;
+        fFlags          = other.fFlags;
+        SkASSERT(!other.fPixelRef);
+        other.fInfo.reset();
+        other.fPixelLockCount = 0;
+        other.fPixels         = nullptr;
+        other.fColorTable     = nullptr;
+        other.fPixelRefOrigin = SkIPoint{0, 0};
+        other.fRowBytes       = 0;
+        other.fFlags          = 0;
     }
     return *this;
 }
 
 void SkBitmap::swap(SkBitmap& other) {
-    SkTSwap(fColorTable, other.fColorTable);
-    SkTSwap(fPixelRef, other.fPixelRef);
-    SkTSwap(fPixelRefOrigin, other.fPixelRefOrigin);
-    SkTSwap(fPixelLockCount, other.fPixelLockCount);
-    SkTSwap(fPixels, other.fPixels);
-    SkTSwap(fInfo, other.fInfo);
-    SkTSwap(fRowBytes, other.fRowBytes);
-    SkTSwap(fFlags, other.fFlags);
-
+    SkTSwap(*this, other);
     SkDEBUGCODE(this->validate();)
 }
 
@@ -182,7 +213,14 @@ void SkBitmap::updatePixelsFromRef() const {
     }
 }
 
+#ifdef SK_SUPPORT_LEGACY_BITMAP_SETPIXELREF
 SkPixelRef* SkBitmap::setPixelRef(SkPixelRef* pr, int dx, int dy) {
+    this->setPixelRef(sk_ref_sp(pr), dx, dy);
+    return pr;
+}
+#endif
+
+void SkBitmap::setPixelRef(sk_sp<SkPixelRef> pr, int dx, int dy) {
 #ifdef SK_DEBUG
     if (pr) {
         if (kUnknown_SkColorType != fInfo.colorType()) {
@@ -218,15 +256,13 @@ SkPixelRef* SkBitmap::setPixelRef(SkPixelRef* pr, int dx, int dy) {
 
     if (fPixelRef != pr) {
         this->freePixels();
-        SkASSERT(nullptr == fPixelRef);
+        SkASSERT(!fPixelRef);
 
-        SkSafeRef(pr);
-        fPixelRef = pr;
+        fPixelRef = std::move(pr);
         this->updatePixelsFromRef();
     }
 
     SkDEBUGCODE(this->validate();)
-    return pr;
 }
 
 void SkBitmap::lockPixels() const {
@@ -238,7 +274,7 @@ void SkBitmap::lockPixels() const {
 }
 
 void SkBitmap::unlockPixels() const {
-    SkASSERT(nullptr == fPixelRef || fPixelLockCount > 0);
+    SkASSERT(!fPixelRef || fPixelLockCount > 0);
 
     if (fPixelRef && 1 == sk_atomic_dec(&fPixelLockCount)) {
         fPixelRef->unlockPixels();
@@ -248,28 +284,25 @@ void SkBitmap::unlockPixels() const {
 }
 
 bool SkBitmap::lockPixelsAreWritable() const {
-    return (fPixelRef) ? fPixelRef->lockPixelsAreWritable() : false;
+    return fPixelRef ? fPixelRef->lockPixelsAreWritable() : false;
 }
 
 void SkBitmap::setPixels(void* p, SkColorTable* ctable) {
     if (nullptr == p) {
-        this->setPixelRef(nullptr);
+        this->setPixelRef(nullptr, 0, 0);
         return;
     }
 
     if (kUnknown_SkColorType == fInfo.colorType()) {
-        this->setPixelRef(nullptr);
+        this->setPixelRef(nullptr, 0, 0);
         return;
     }
 
-    SkPixelRef* pr = SkMallocPixelRef::NewDirect(fInfo, p, fRowBytes, ctable);
-    if (nullptr == pr) {
-        this->setPixelRef(nullptr);
+    sk_sp<SkPixelRef> pr(SkMallocPixelRef::NewDirect(fInfo, p, fRowBytes, ctable));
+    this->setPixelRef(std::move(pr), 0, 0);
+    if (!fPixelRef) {
         return;
     }
-
-    this->setPixelRef(pr)->unref();
-
     // since we're already allocated, we lockPixels right away
     this->lockPixels();
     SkDEBUGCODE(this->validate();)
@@ -301,11 +334,11 @@ bool SkBitmap::tryAllocPixels(const SkImageInfo& requestedInfo, size_t rowBytes)
 
     SkMallocPixelRef::PRFactory defaultFactory;
 
-    SkPixelRef* pr = defaultFactory.create(correctedInfo, rowBytes, nullptr);
-    if (nullptr == pr) {
+    sk_sp<SkPixelRef> pr(defaultFactory.create(correctedInfo, rowBytes, nullptr));
+    if (!pr) {
         return reset_return_false(this);
     }
-    this->setPixelRef(pr)->unref();
+    this->setPixelRef(std::move(pr), 0, 0);
 
     // TODO: lockPixels could/should return bool or void*/nullptr
     this->lockPixels();
@@ -332,11 +365,11 @@ bool SkBitmap::tryAllocPixels(const SkImageInfo& requestedInfo, SkPixelRefFactor
         factory = &defaultFactory;
     }
 
-    SkPixelRef* pr = factory->create(correctedInfo, correctedInfo.minRowBytes(), ctable);
-    if (nullptr == pr) {
+    sk_sp<SkPixelRef> pr(factory->create(correctedInfo, correctedInfo.minRowBytes(), ctable));
+    if (!pr) {
         return reset_return_false(this);
     }
-    this->setPixelRef(pr)->unref();
+    this->setPixelRef(std::move(pr), 0, 0);
 
     // TODO: lockPixels could/should return bool or void*/nullptr
     this->lockPixels();
@@ -368,14 +401,14 @@ bool SkBitmap::installPixels(const SkImageInfo& requestedInfo, void* pixels, siz
     // setInfo may have corrected info (e.g. 565 is always opaque).
     const SkImageInfo& correctedInfo = this->info();
 
-    SkPixelRef* pr = SkMallocPixelRef::NewWithProc(correctedInfo, rb, ct, pixels, releaseProc,
-                                                   context);
+    sk_sp<SkPixelRef> pr(SkMallocPixelRef::NewWithProc(correctedInfo, rb, ct, pixels, releaseProc,
+                                                       context));
     if (!pr) {
         this->reset();
         return false;
     }
 
-    this->setPixelRef(pr)->unref();
+    this->setPixelRef(std::move(pr), 0, 0);
 
     // since we're already allocated, we lockPixels right away
     this->lockPixels();
@@ -406,7 +439,6 @@ void SkBitmap::freePixels() {
         if (fPixelLockCount > 0) {
             fPixelRef->unlockPixels();
         }
-        fPixelRef->unref();
         fPixelRef = nullptr;
         fPixelRefOrigin.setZero();
     }
@@ -416,7 +448,7 @@ void SkBitmap::freePixels() {
 }
 
 uint32_t SkBitmap::getGenerationID() const {
-    return (fPixelRef) ? fPixelRef->getGenerationID() : 0;
+    return fPixelRef ? fPixelRef->getGenerationID() : 0;
 }
 
 void SkBitmap::notifyPixelsChanged() const {
@@ -439,12 +471,12 @@ bool SkBitmap::HeapAllocator::allocPixelRef(SkBitmap* dst,
         return false;
     }
 
-    SkPixelRef* pr = SkMallocPixelRef::NewAllocate(info, dst->rowBytes(), ctable);
-    if (nullptr == pr) {
+    sk_sp<SkPixelRef> pr(SkMallocPixelRef::NewAllocate(info, dst->rowBytes(), ctable));
+    if (!pr) {
         return false;
     }
 
-    dst->setPixelRef(pr)->unref();
+    dst->setPixelRef(std::move(pr), 0, 0);
     // since we're already allocated, we lockPixels right away
     dst->lockPixels();
     return true;
@@ -597,7 +629,7 @@ void SkBitmap::eraseColor(SkColor c) const {
 bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
     SkDEBUGCODE(this->validate();)
 
-    if (nullptr == result || nullptr == fPixelRef) {
+    if (nullptr == result || !fPixelRef) {
         return false;   // no src pixels
     }
 
@@ -621,7 +653,7 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const {
         origin.fX += r.fLeft;
         origin.fY += r.fTop;
         // share the pixelref with a custom offset
-        dst.setPixelRef(fPixelRef, origin);
+        dst.setPixelRef(fPixelRef, origin.x(), origin.y());
     }
     SkDEBUGCODE(dst.validate();)
 
@@ -707,8 +739,9 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc)
             if (tmpSrc.colorType() == dstColorType && nullptr == alloc) {
                 dst->swap(tmpSrc);
                 // If the result is an exact copy, clone the gen ID.
-                if (dst->pixelRef() && dst->pixelRef()->info() == fPixelRef->info()) {
-                    dst->pixelRef()->cloneGenID(*fPixelRef);
+                SkPixelRef* dstPixelRef = dst->pixelRef();
+                if (!dstPixelRef && dstPixelRef->info() == fPixelRef->info()) {
+                    dstPixelRef->cloneGenID(*fPixelRef);
                 }
                 return true;
             }
@@ -895,7 +928,7 @@ static void write_raw_pixels(SkWriteBuffer* buffer, const SkPixmap& pmap) {
 
 void SkBitmap::WriteRawPixels(SkWriteBuffer* buffer, const SkBitmap& bitmap) {
     const SkImageInfo info = bitmap.info();
-    if (0 == info.width() || 0 == info.height() || nullptr == bitmap.pixelRef()) {
+    if (0 == info.width() || 0 == info.height() || bitmap.isNull()) {
         buffer->writeUInt(0); // instead of snugRB, signaling no pixels
         return;
     }
@@ -981,7 +1014,7 @@ bool SkBitmap::ReadRawPixels(SkReadBuffer* buffer, SkBitmap* bitmap) {
         return false;
     }
     bitmap->setInfo(pr->info());
-    bitmap->setPixelRef(pr.get(), 0, 0);
+    bitmap->setPixelRef(std::move(pr), 0, 0);
     return true;
 }
 
@@ -1083,7 +1116,7 @@ void SkBitmap::toString(SkString* str) const {
 bool SkBitmap::requestLock(SkAutoPixmapUnlock* result) const {
     SkASSERT(result);
 
-    SkPixelRef* pr = fPixelRef;
+    SkPixelRef* pr = fPixelRef.get();
     if (nullptr == pr) {
         return false;
     }
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index f8069ee2d3..6a825b5402 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -876,7 +876,7 @@ bool SkCanvas::readPixels(SkBitmap* bitmap, int x, int y) {
     }
 
     if (weAllocated) {
-        bitmap->setPixelRef(nullptr);
+        bitmap->setPixelRef(nullptr, 0, 0);
     }
     return false;
 }
diff --git a/src/core/SkResourceCache.cpp b/src/core/SkResourceCache.cpp
index 4bdc8dd9e0..47d9f6a48f 100644
--- a/src/core/SkResourceCache.cpp
+++ b/src/core/SkResourceCache.cpp
@@ -189,8 +189,8 @@ bool SkResourceCacheDiscardableAllocator::allocPixelRef(SkBitmap* bitmap, SkColo
     }
 
     SkImageInfo info = bitmap->info();
-    bitmap->setPixelRef(new SkOneShotDiscardablePixelRef(info, dm, bitmap->rowBytes(),
-                                                         ctable))->unref();
+    bitmap->setPixelRef(
+            sk_make_sp<SkOneShotDiscardablePixelRef>(info, dm, bitmap->rowBytes(), ctable), 0, 0);
     bitmap->lockPixels();
     return bitmap->readyToDraw();
 }
diff --git a/src/core/SkSpecialSurface.cpp b/src/core/SkSpecialSurface.cpp
index 956300cb3e..e6206a0a19 100644
--- a/src/core/SkSpecialSurface.cpp
+++ b/src/core/SkSpecialSurface.cpp
@@ -63,14 +63,14 @@ sk_sp<SkSpecialImage> SkSpecialSurface::makeImageSnapshot() {
 
 class SkSpecialSurface_Raster : public SkSpecialSurface_Base {
 public:
-    SkSpecialSurface_Raster(SkPixelRef* pr,
+    SkSpecialSurface_Raster(sk_sp<SkPixelRef> pr,
                             const SkIRect& subset,
                             const SkSurfaceProps* props)
         : INHERITED(subset, props) {
         const SkImageInfo& info = pr->info();
 
         fBitmap.setInfo(info, info.minRowBytes());
-        fBitmap.setPixelRef(pr);
+        fBitmap.setPixelRef(std::move(pr), 0, 0);
 
         fCanvas.reset(new SkCanvas(fBitmap, this->props()));
         fCanvas->clipRect(SkRect::Make(subset));
@@ -93,7 +93,7 @@ private:
 
 sk_sp<SkSpecialSurface> SkSpecialSurface::MakeFromBitmap(const SkIRect& subset, SkBitmap& bm,
                                                          const SkSurfaceProps* props) {
-    return sk_make_sp<SkSpecialSurface_Raster>(bm.pixelRef(), subset, props);
+    return sk_make_sp<SkSpecialSurface_Raster>(sk_ref_sp(bm.pixelRef()), subset, props);
 }
 
 sk_sp<SkSpecialSurface> SkSpecialSurface::MakeRaster(const SkImageInfo& info,
@@ -105,7 +105,7 @@ sk_sp<SkSpecialSurface> SkSpecialSurface::MakeRaster(const SkImageInfo& info,
 
     const SkIRect subset = SkIRect::MakeWH(pr->info().width(), pr->info().height());
 
-    return sk_make_sp<SkSpecialSurface_Raster>(pr.get(), subset, props);
+    return sk_make_sp<SkSpecialSurface_Raster>(std::move(pr), subset, props);
 }
 
 #if SK_SUPPORT_GPU
diff --git a/src/effects/gradients/SkGradientShader.cpp b/src/effects/gradients/SkGradientShader.cpp
index df78da9b48..ca64cb77e1 100644
--- a/src/effects/gradients/SkGradientShader.cpp
+++ b/src/effects/gradients/SkGradientShader.cpp
@@ -730,7 +730,7 @@ void SkGradientShaderBase::getGradientTableBitmap(SkBitmap* bitmap,
             // force our cache32pixelref to be built
             (void)cache->getCache32();
             bitmap->setInfo(SkImageInfo::MakeN32Premul(kCache32Count, 1));
-            bitmap->setPixelRef(cache->getCache32PixelRef());
+            bitmap->setPixelRef(sk_ref_sp(cache->getCache32PixelRef()), 0, 0);
         } else {
             // For these cases we use the bitmap cache, but not the GradientShaderCache. So just
             // allocate and populate the bitmap's data directly.
diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp
index b14a06e99d..afd5f90390 100644
--- a/src/image/SkImage_Raster.cpp
+++ b/src/image/SkImage_Raster.cpp
@@ -158,7 +158,7 @@ SkImage_Raster::SkImage_Raster(const Info& info, SkPixelRef* pr, const SkIPoint&
     : INHERITED(info.width(), info.height(), pr->getGenerationID())
 {
     fBitmap.setInfo(info, rowBytes);
-    fBitmap.setPixelRef(pr, pixelRefOrigin);
+    fBitmap.setPixelRef(sk_ref_sp(pr), pixelRefOrigin.x(), pixelRefOrigin.y());
     fBitmap.lockPixels();
     SkASSERT(fBitmap.isImmutable());
 }
@@ -364,7 +364,9 @@ bool SkImage_Raster::onAsLegacyBitmap(SkBitmap* bitmap, LegacyBitmapMode mode) c
         // (thus changing our state).
         if (fBitmap.isImmutable()) {
             bitmap->setInfo(fBitmap.info(), fBitmap.rowBytes());
-            bitmap->setPixelRef(fBitmap.pixelRef(), fBitmap.pixelRefOrigin());
+            bitmap->setPixelRef(sk_ref_sp(fBitmap.pixelRef()),
+                                fBitmap.pixelRefOrigin().x(),
+                                fBitmap.pixelRefOrigin().y());
             return true;
         }
     }
diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp
index 987ff2bbb0..308b072ea4 100644
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -20,7 +20,7 @@ public:
     SkSurface_Raster(const SkImageInfo&, void*, size_t rb,
                      void (*releaseProc)(void* pixels, void* context), void* context,
                      const SkSurfaceProps*);
-    SkSurface_Raster(SkPixelRef*, const SkSurfaceProps*);
+    SkSurface_Raster(sk_sp<SkPixelRef>, const SkSurfaceProps*);
 
     SkCanvas* onNewCanvas() override;
     sk_sp<SkSurface> onNewSurface(const SkImageInfo&) override;
@@ -108,14 +108,14 @@ SkSurface_Raster::SkSurface_Raster(const SkImageInfo& info, void* pixels, size_t
     fWeOwnThePixels = false;    // We are "Direct"
 }
 
-SkSurface_Raster::SkSurface_Raster(SkPixelRef* pr, const SkSurfaceProps* props)
+SkSurface_Raster::SkSurface_Raster(sk_sp<SkPixelRef> pr, const SkSurfaceProps* props)
     : INHERITED(pr->info().width(), pr->info().height(), props)
 {
     const SkImageInfo& info = pr->info();
 
     fBitmap.setInfo(info, pr->rowBytes());
-    fBitmap.setPixelRef(pr);
     fRowBytes = pr->rowBytes(); // we track this, so that subsequent re-allocs will match
+    fBitmap.setPixelRef(std::move(pr), 0, 0);
     fWeOwnThePixels = true;
 }
 
@@ -215,5 +215,5 @@ sk_sp<SkSurface> SkSurface::MakeRaster(const SkImageInfo& info, size_t rowBytes,
     if (rowBytes) {
         SkASSERT(pr->rowBytes() == rowBytes);
     }
-    return sk_make_sp<SkSurface_Raster>(pr.get(), props);
+    return sk_make_sp<SkSurface_Raster>(std::move(pr), props);
 }
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index 4f214897a7..005ce67abb 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -91,7 +91,7 @@ static SkImageSubset make_image_subset(const SkBitmap& bitmap) {
     SkASSERT(bitmap.pixelRef());
     SkBitmap tmp;
     tmp.setInfo(bitmap.pixelRef()->info(), bitmap.rowBytes());
-    tmp.setPixelRef(bitmap.pixelRef());
+    tmp.setPixelRef(sk_ref_sp(bitmap.pixelRef()), 0, 0);
     tmp.lockPixels();
     auto img = SkImage::MakeFromBitmap(tmp);
     if (img) {
diff --git a/tests/DrawBitmapRectTest.cpp b/tests/DrawBitmapRectTest.cpp
index 221f0c9b91..62da1c2af8 100644
--- a/tests/DrawBitmapRectTest.cpp
+++ b/tests/DrawBitmapRectTest.cpp
@@ -38,7 +38,7 @@ static void test_faulty_pixelref(skiatest::Reporter* reporter) {
     SkBitmap bm;
     const SkImageInfo info = SkImageInfo::MakeN32Premul(100, 100);
     bm.setInfo(info);
-    bm.setPixelRef(new FailurePixelRef(info), 0, 0)->unref();
+    bm.setPixelRef(sk_make_sp<FailurePixelRef>(info), 0, 0);
     // now our bitmap has a pixelref, but we know it will fail to lock
 
     auto surface(SkSurface::MakeRasterN32Premul(200, 200));
diff --git a/tests/PDFInvalidBitmapTest.cpp b/tests/PDFInvalidBitmapTest.cpp
index 10292bdb5f..54acf02f40 100644
--- a/tests/PDFInvalidBitmapTest.cpp
+++ b/tests/PDFInvalidBitmapTest.cpp
@@ -32,7 +32,7 @@ private:
 SkBitmap make_invalid_bitmap(const SkImageInfo& imageInfo) {
     SkBitmap bitmap;
     bitmap.setInfo(imageInfo);
-    bitmap.setPixelRef(new InvalidPixelRef(imageInfo))->unref();
+    bitmap.setPixelRef(sk_make_sp<InvalidPixelRef>(imageInfo), 0 ,0);
     return bitmap;
 }
 
diff --git a/tests/WritePixelsTest.cpp b/tests/WritePixelsTest.cpp
index 2937e0ef00..1c5ce81cbf 100644
--- a/tests/WritePixelsTest.cpp
+++ b/tests/WritePixelsTest.cpp
@@ -263,8 +263,8 @@ static bool alloc_row_bytes(SkBitmap* bm, const SkImageInfo& info, size_t rowByt
     if (!bm->setInfo(info, rowBytes)) {
         return false;
     }
-    SkPixelRef* pr = SkMallocPixelRef::NewAllocate(info, rowBytes, nullptr);
-    bm->setPixelRef(pr)->unref();
+    sk_sp<SkPixelRef> pr(SkMallocPixelRef::NewAllocate(info, rowBytes, nullptr));
+    bm->setPixelRef(std::move(pr), 0, 0);
     return true;
 }
 
diff --git a/tools/skdiff/skdiff_main.cpp b/tools/skdiff/skdiff_main.cpp
index b496cadeb6..f03ccb04b0 100644
--- a/tools/skdiff/skdiff_main.cpp
+++ b/tools/skdiff/skdiff_main.cpp
@@ -13,6 +13,7 @@
 #include "SkOSFile.h"
 #include "SkOSPath.h"
 #include "SkStream.h"
+#include "SkPixelRef.h"
 #include "../private/SkTDArray.h"
 #include "../private/SkTSearch.h"
 
@@ -329,10 +330,10 @@ public:
         SkASSERT(drp != nullptr);
     }
     ~AutoReleasePixels() {
-        fDrp->fBase.fBitmap.setPixelRef(nullptr);
-        fDrp->fComparison.fBitmap.setPixelRef(nullptr);
-        fDrp->fDifference.fBitmap.setPixelRef(nullptr);
-        fDrp->fWhite.fBitmap.setPixelRef(nullptr);
+        fDrp->fBase.fBitmap.setPixelRef(nullptr, 0, 0);
+        fDrp->fComparison.fBitmap.setPixelRef(nullptr, 0, 0);
+        fDrp->fDifference.fBitmap.setPixelRef(nullptr, 0, 0);
+        fDrp->fWhite.fBitmap.setPixelRef(nullptr, 0, 0);
     }
 
 private: