Bug 1416862 - Reverse DrawTargetSkia snapshot ownership model. r=dvander, a=jcristau
authorBas Schouten <bschouten@mozilla.com>
Wed, 06 Dec 2017 04:59:19 +0100
changeset 445294 996b3de167b7097dfa198cb79412f3d914f5f949
parent 445293 051581a5ece8983b8f8ee3d13bd56e8eb63b84b8
child 445295 786d521ee926b7df598f581bb07846c03fca2247
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander, jcristau
bugs1416862
milestone58.0
Bug 1416862 - Reverse DrawTargetSkia snapshot ownership model. r=dvander, a=jcristau MozReview-Commit-ID: 3hpeYteEPlA
gfx/2d/DrawTargetSkia.cpp
gfx/2d/DrawTargetSkia.h
gfx/2d/SourceSurfaceSkia.cpp
gfx/2d/SourceSurfaceSkia.h
--- a/gfx/2d/DrawTargetSkia.cpp
+++ b/gfx/2d/DrawTargetSkia.cpp
@@ -273,29 +273,35 @@ GetSkImageForSurface(SourceSurface* aSur
   MOZ_ASSERT(VerifyRGBXCorners(surf->GetData(), surf->GetSize(),
                                surf->Stride(), surf->GetFormat(),
                                aBounds, aMatrix));
   return image;
 }
 
 DrawTargetSkia::DrawTargetSkia()
   : mSnapshot(nullptr)
-  , mSnapshotLock{make_shared<Mutex>("DrawTargetSkia::mSnapshotLock")}
+  , mSnapshotLock{"DrawTargetSkia::mSnapshotLock"}
 #ifdef MOZ_WIDGET_COCOA
   , mCG(nullptr)
   , mColorSpace(nullptr)
   , mCanvasData(nullptr)
   , mCGSize(0, 0)
   , mNeedLayer(false)
 #endif
 {
 }
 
 DrawTargetSkia::~DrawTargetSkia()
 {
+  if (mSnapshot) {
+    MutexAutoLock lock(mSnapshotLock);
+    // We're going to go away, hand our SkSurface to the SourceSurface.
+    mSnapshot->GiveSurface(mSurface);
+  }
+
 #ifdef MOZ_WIDGET_COCOA
   if (mCG) {
     CGContextRelease(mCG);
     mCG = nullptr;
   }
 
   if (mColorSpace) {
     CGColorSpaceRelease(mColorSpace);
@@ -304,30 +310,30 @@ DrawTargetSkia::~DrawTargetSkia()
 #endif
 }
 
 already_AddRefed<SourceSurface>
 DrawTargetSkia::Snapshot()
 {
   // Without this lock, this could cause us to get out a snapshot and race with
   // Snapshot::~Snapshot() actually destroying itself.
-  MutexAutoLock lock(*mSnapshotLock);
+  MutexAutoLock lock(mSnapshotLock);
   RefPtr<SourceSurfaceSkia> snapshot = mSnapshot;
   if (mSurface && !snapshot) {
     snapshot = new SourceSurfaceSkia();
     sk_sp<SkImage> image;
     // If the surface is raster, making a snapshot may trigger a pixel copy.
     // Instead, try to directly make a raster image referencing the surface pixels.
     SkPixmap pixmap;
     if (mSurface->peekPixels(&pixmap)) {
       image = SkImage::MakeFromRaster(pixmap, nullptr, nullptr);
     } else {
       image = mSurface->makeImageSnapshot();
     }
-    if (!snapshot->InitFromImage(image, mFormat, this, mSnapshotLock)) {
+    if (!snapshot->InitFromImage(image, mFormat, this)) {
       return nullptr;
     }
     mSnapshot = snapshot;
   }
 
   return snapshot.forget();
 }
 
@@ -2221,28 +2227,31 @@ already_AddRefed<FilterNode>
 DrawTargetSkia::CreateFilter(FilterType aType)
 {
   return FilterNodeSoftware::Create(aType);
 }
 
 void
 DrawTargetSkia::MarkChanged()
 {
-  MutexAutoLock lock(*mSnapshotLock);
+  // I'm not entirely certain whether this lock is needed, as multiple threads
+  // should never modify the DrawTarget at the same time anyway, but this seems
+  // like the safest.
+  MutexAutoLock lock(mSnapshotLock);
   if (mSnapshot) {
+    if (mSnapshot->hasOneRef()) {
+      // No owners outside of this DrawTarget's own reference. Just dump it.
+      mSnapshot = nullptr;
+      return;
+    }
+
     mSnapshot->DrawTargetWillChange();
     mSnapshot = nullptr;
 
     // Handle copying of any image snapshots bound to the surface.
     if (mSurface) {
       mSurface->notifyContentWillChange(SkSurface::kRetain_ContentChangeMode);
     }
   }
 }
 
-void
-DrawTargetSkia::SnapshotDestroyed()
-{
-  mSnapshot = nullptr;
-}
-
 } // namespace gfx
 } // namespace mozilla
--- a/gfx/2d/DrawTargetSkia.h
+++ b/gfx/2d/DrawTargetSkia.h
@@ -155,17 +155,16 @@ public:
   operator std::string() const {
     std::stringstream stream;
     stream << "DrawTargetSkia(" << this << ")";
     return stream.str();
   }
 
 private:
   friend class SourceSurfaceSkia;
-  void SnapshotDestroyed();
 
   void MarkChanged();
 
   bool ShouldLCDRenderText(FontType aFontType, AntialiasMode aAntialiasMode);
 
   void DrawGlyphs(ScaledFont* aFont,
                   const GlyphBuffer& aBuffer,
                   const Pattern& aPattern,
@@ -200,18 +199,18 @@ private:
 
 #ifdef USE_SKIA_GPU
   sk_sp<GrContext> mGrContext;
 #endif
 
   IntSize mSize;
   sk_sp<SkSurface> mSurface;
   SkCanvas* mCanvas;
-  SourceSurfaceSkia* mSnapshot;
-  std::shared_ptr<Mutex> mSnapshotLock;
+  RefPtr<SourceSurfaceSkia> mSnapshot;
+  Mutex mSnapshotLock;
 
 #ifdef MOZ_WIDGET_COCOA
   friend class BorrowedCGContext;
 
   CGContextRef BorrowCGContext(const DrawOptions &aOptions);
   void ReturnCGContext(CGContextRef);
   bool FillGlyphsWithCG(ScaledFont* aFont,
                         const GlyphBuffer& aBuffer,
--- a/gfx/2d/SourceSurfaceSkia.cpp
+++ b/gfx/2d/SourceSurfaceSkia.cpp
@@ -21,23 +21,16 @@ namespace gfx {
 SourceSurfaceSkia::SourceSurfaceSkia()
   : mDrawTarget(nullptr)
   , mChangeMutex("SourceSurfaceSkia::mChangeMutex")
 {
 }
 
 SourceSurfaceSkia::~SourceSurfaceSkia()
 {
-  if (mSnapshotLock) {
-    MutexAutoLock lock{*mSnapshotLock};
-    if (mDrawTarget) {
-      mDrawTarget->SnapshotDestroyed();
-      mDrawTarget = nullptr;
-    }
-  }
 }
 
 IntSize
 SourceSurfaceSkia::GetSize() const
 {
   return mSize;
 }
 
@@ -104,18 +97,17 @@ SourceSurfaceSkia::InitFromData(unsigned
   mFormat = aFormat;
   mStride = aStride;
   return true;
 }
 
 bool
 SourceSurfaceSkia::InitFromImage(const sk_sp<SkImage>& aImage,
                                  SurfaceFormat aFormat,
-                                 DrawTargetSkia* aOwner,
-                                 shared_ptr<Mutex> aSnapshotLock)
+                                 DrawTargetSkia* aOwner)
 {
   if (!aImage) {
     return false;
   }
 
   mSize = IntSize(aImage->width(), aImage->height());
 
   // For the raster image case, we want to use the format and stride
@@ -138,18 +130,16 @@ SourceSurfaceSkia::InitFromImage(const s
     mStride = SkAlign4(info.minRowBytes());
   } else {
     return false;
   }
 
   mImage = aImage;
 
   if (aOwner) {
-    MOZ_ASSERT(aSnapshotLock);
-    mSnapshotLock = move(aSnapshotLock);
     mDrawTarget = aOwner;
   }
 
   return true;
 }
 
 uint8_t*
 SourceSurfaceSkia::GetData()
@@ -189,20 +179,16 @@ SourceSurfaceSkia::Unmap()
   mChangeMutex.Unlock();
   MOZ_ASSERT(mIsMapped);
   mIsMapped = false;
 }
 
 void
 SourceSurfaceSkia::DrawTargetWillChange()
 {
-  // In this case synchronisation on destroy should be guaranteed!
-  MOZ_ASSERT(mSnapshotLock);
-  mSnapshotLock->AssertCurrentThreadOwns();
-
   MutexAutoLock lock(mChangeMutex);
   if (mDrawTarget) {
     // Raster snapshots do not use Skia's internal copy-on-write mechanism,
     // so we need to do an explicit copy here.
     // GPU snapshots, for which peekPixels is false, will already be dealt
     // with automatically via the internal copy-on-write mechanism, so we
     // don't need to do anything for them here.
     SkPixmap pixmap;
--- a/gfx/2d/SourceSurfaceSkia.h
+++ b/gfx/2d/SourceSurfaceSkia.h
@@ -26,27 +26,32 @@ public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DataSourceSurfaceSkia)
   SourceSurfaceSkia();
   ~SourceSurfaceSkia();
 
   virtual SurfaceType GetType() const { return SurfaceType::SKIA; }
   virtual IntSize GetSize() const;
   virtual SurfaceFormat GetFormat() const;
 
+  // This is only ever called by the DT destructor, which can only ever happen
+  // from one place at a time. Therefore it doesn't need to hold the ChangeMutex
+  // as mSurface is never read to directly and is just there to keep the object
+  // alive, which itself is refcounted in a thread-safe manner.
+  void GiveSurface(sk_sp<SkSurface> &aSurface) { mSurface = aSurface; mDrawTarget = nullptr; }
+
   sk_sp<SkImage> GetImage();
 
   bool InitFromData(unsigned char* aData,
                     const IntSize &aSize,
                     int32_t aStride,
                     SurfaceFormat aFormat);
 
   bool InitFromImage(const sk_sp<SkImage>& aImage,
                      SurfaceFormat aFormat = SurfaceFormat::UNKNOWN,
-                     DrawTargetSkia* aOwner = nullptr,
-                     std::shared_ptr<Mutex> aSnapshotLock = std::shared_ptr<Mutex>{});
+                     DrawTargetSkia* aOwner = nullptr);
 
   virtual uint8_t* GetData();
 
   /**
    * The caller is responsible for ensuring aMappedSurface is not null.
    */
   virtual bool Map(MapType, MappedSurface *aMappedSurface);
 
@@ -55,20 +60,21 @@ public:
   virtual int32_t Stride() { return mStride; }
 
 private:
   friend class DrawTargetSkia;
 
   void DrawTargetWillChange();
 
   sk_sp<SkImage> mImage;
+  // This keeps a surface alive if needed because its DrawTarget has gone away.
+  sk_sp<SkSurface> mSurface;
   SurfaceFormat mFormat;
   IntSize mSize;
   int32_t mStride;
-  RefPtr<DrawTargetSkia> mDrawTarget;
-  std::shared_ptr<Mutex> mSnapshotLock;
+  DrawTargetSkia* mDrawTarget;
   Mutex mChangeMutex;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif /* MOZILLA_GFX_SOURCESURFACESKIA_H_ */