diff --git a/gfx/2d/DrawTargetSkia.cpp b/gfx/2d/DrawTargetSkia.cpp index 36c80e02fb56..1436179a20fb 100644 --- a/gfx/2d/DrawTargetSkia.cpp +++ b/gfx/2d/DrawTargetSkia.cpp @@ -278,7 +278,7 @@ GetSkImageForSurface(SourceSurface* aSurface, const Rect* aBounds = nullptr, con DrawTargetSkia::DrawTargetSkia() : mSnapshot(nullptr) - , mSnapshotLock{make_shared("DrawTargetSkia::mSnapshotLock")} + , mSnapshotLock{"DrawTargetSkia::mSnapshotLock"} #ifdef MOZ_WIDGET_COCOA , mCG(nullptr) , mColorSpace(nullptr) @@ -291,6 +291,12 @@ DrawTargetSkia::DrawTargetSkia() 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); @@ -309,7 +315,7 @@ 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 snapshot = mSnapshot; if (mSurface && !snapshot) { snapshot = new SourceSurfaceSkia(); @@ -322,7 +328,7 @@ DrawTargetSkia::Snapshot() } else { image = mSurface->makeImageSnapshot(); } - if (!snapshot->InitFromImage(image, mFormat, this, mSnapshotLock)) { + if (!snapshot->InitFromImage(image, mFormat, this)) { return nullptr; } mSnapshot = snapshot; @@ -2226,8 +2232,17 @@ DrawTargetSkia::CreateFilter(FilterType 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; @@ -2238,11 +2253,5 @@ DrawTargetSkia::MarkChanged() } } -void -DrawTargetSkia::SnapshotDestroyed() -{ - mSnapshot = nullptr; -} - } // namespace gfx } // namespace mozilla diff --git a/gfx/2d/DrawTargetSkia.h b/gfx/2d/DrawTargetSkia.h index 812120cb427b..b63d5fece745 100644 --- a/gfx/2d/DrawTargetSkia.h +++ b/gfx/2d/DrawTargetSkia.h @@ -160,7 +160,6 @@ public: private: friend class SourceSurfaceSkia; - void SnapshotDestroyed(); void MarkChanged(); @@ -205,8 +204,8 @@ private: IntSize mSize; sk_sp mSurface; SkCanvas* mCanvas; - SourceSurfaceSkia* mSnapshot; - std::shared_ptr mSnapshotLock; + RefPtr mSnapshot; + Mutex mSnapshotLock; #ifdef MOZ_WIDGET_COCOA friend class BorrowedCGContext; diff --git a/gfx/2d/SourceSurfaceSkia.cpp b/gfx/2d/SourceSurfaceSkia.cpp index 737f1992b247..2c2e59d43669 100644 --- a/gfx/2d/SourceSurfaceSkia.cpp +++ b/gfx/2d/SourceSurfaceSkia.cpp @@ -26,13 +26,6 @@ SourceSurfaceSkia::SourceSurfaceSkia() SourceSurfaceSkia::~SourceSurfaceSkia() { - if (mSnapshotLock) { - MutexAutoLock lock{*mSnapshotLock}; - if (mDrawTarget) { - mDrawTarget->SnapshotDestroyed(); - mDrawTarget = nullptr; - } - } } IntSize @@ -109,8 +102,7 @@ SourceSurfaceSkia::InitFromData(unsigned char* aData, bool SourceSurfaceSkia::InitFromImage(const sk_sp& aImage, SurfaceFormat aFormat, - DrawTargetSkia* aOwner, - shared_ptr aSnapshotLock) + DrawTargetSkia* aOwner) { if (!aImage) { return false; @@ -143,8 +135,6 @@ SourceSurfaceSkia::InitFromImage(const sk_sp& aImage, mImage = aImage; if (aOwner) { - MOZ_ASSERT(aSnapshotLock); - mSnapshotLock = move(aSnapshotLock); mDrawTarget = aOwner; } @@ -194,10 +184,6 @@ SourceSurfaceSkia::Unmap() 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, diff --git a/gfx/2d/SourceSurfaceSkia.h b/gfx/2d/SourceSurfaceSkia.h index f6b04ce47c3c..b33490fc73bb 100644 --- a/gfx/2d/SourceSurfaceSkia.h +++ b/gfx/2d/SourceSurfaceSkia.h @@ -31,6 +31,12 @@ public: 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 &aSurface) { mSurface = aSurface; mDrawTarget = nullptr; } + sk_sp GetImage(); bool InitFromData(unsigned char* aData, @@ -40,8 +46,7 @@ public: bool InitFromImage(const sk_sp& aImage, SurfaceFormat aFormat = SurfaceFormat::UNKNOWN, - DrawTargetSkia* aOwner = nullptr, - std::shared_ptr aSnapshotLock = std::shared_ptr{}); + DrawTargetSkia* aOwner = nullptr); virtual uint8_t* GetData(); @@ -60,11 +65,12 @@ private: void DrawTargetWillChange(); sk_sp mImage; + // This keeps a surface alive if needed because its DrawTarget has gone away. + sk_sp mSurface; SurfaceFormat mFormat; IntSize mSize; int32_t mStride; - RefPtr mDrawTarget; - std::shared_ptr mSnapshotLock; + DrawTargetSkia* mDrawTarget; Mutex mChangeMutex; };