Bug 1456555 - Support Map on multiple threads. r=rhunt
☠☠ backed out by 1a5817b175de ☠ ☠
authorMatt Woodrow <mwoodrow@mozilla.com>
Tue, 06 Nov 2018 00:41:03 +0000
changeset 444511 d46e2737134f292baa56a768e7b9b9b6cd754661
parent 444510 4261b7dc70f79703570b1e38fcea17669299f476
child 444512 afb19dd185566aa0ad509cf40c449923d9302b96
push id34996
push userrgurzau@mozilla.com
push dateTue, 06 Nov 2018 09:53:23 +0000
treeherdermozilla-central@e160f0a60e4f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrhunt
bugs1456555
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1456555 - Support Map on multiple threads. r=rhunt This just makes the existing hack available to all DataSourceSurface implementations by default, since we use different ones with WR. MozReview-Commit-ID: GVR0rIx8wtD Depends on D10036 Differential Revision: https://phabricator.services.mozilla.com/D10038
gfx/2d/2D.h
gfx/2d/SourceSurfaceD2D1.cpp
gfx/2d/SourceSurfaceD2D1.h
gfx/2d/SourceSurfaceRawData.h
gfx/2d/SourceSurfaceSkia.cpp
gfx/2d/SourceSurfaceSkia.h
gfx/layers/SourceSurfaceSharedData.h
gfx/layers/SourceSurfaceVolatileData.h
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -26,16 +26,17 @@
 
 // This RefPtr class isn't ideal for usage in Azure, as it doesn't allow T**
 // outparams using the &-operator. But it will have to do as there's no easy
 // solution.
 #include "mozilla/RefPtr.h"
 #include "mozilla/StaticMutex.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/ThreadSafeWeakPtr.h"
+#include "mozilla/Atomics.h"
 
 #include "mozilla/DebugOnly.h"
 
 #include "nsRegionFwd.h"
 
 #if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GTK)
   #ifndef MOZ_ENABLE_FREETYPE
   #define MOZ_ENABLE_FREETYPE
@@ -440,24 +441,24 @@ protected:
   UserData mUserData;
 };
 
 class DataSourceSurface : public SourceSurface
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DataSourceSurface, override)
   DataSourceSurface()
-    : mIsMapped(false)
+    : mMapCount(0)
   {
   }
 
 #ifdef DEBUG
   virtual ~DataSourceSurface()
   {
-    MOZ_ASSERT(!mIsMapped, "Someone forgot to call Unmap()");
+    MOZ_ASSERT(mMapCount == 0);
   }
 #endif
 
   struct MappedSurface {
     uint8_t *mData;
     int32_t mStride;
   };
 
@@ -552,29 +553,41 @@ public:
    * Stride of the surface, distance in bytes between the start of the image
    * data belonging to row y and row y+1. This may be negative.
    * Can return 0 if there was OOM allocating surface data.
    */
   virtual int32_t Stride() = 0;
 
   /**
    * The caller is responsible for ensuring aMappedSurface is not null.
+  // Althought Map (and Moz2D in general) isn't normally threadsafe,
+  // we want to allow it for SourceSurfaceRawData since it should
+  // always be fine (for reading at least).
+  //
+  // This is the same as the base class implementation except using
+  // mMapCount instead of mIsMapped since that breaks for multithread.
+  //
+  // Once mfbt supports Monitors we should implement proper read/write
+  // locking to prevent write races.
    */
   virtual bool Map(MapType, MappedSurface *aMappedSurface)
   {
     aMappedSurface->mData = GetData();
     aMappedSurface->mStride = Stride();
-    mIsMapped = !!aMappedSurface->mData;
-    return mIsMapped;
+    bool success = !!aMappedSurface->mData;
+    if (success) {
+      mMapCount++;
+    }
+    return success;
   }
 
   virtual void Unmap()
   {
-    MOZ_ASSERT(mIsMapped);
-    mIsMapped = false;
+    mMapCount--;
+    MOZ_ASSERT(mMapCount >= 0);
   }
 
   /**
    * Returns a DataSourceSurface with the same data as this one, but
    * guaranteed to have surface->GetType() == SurfaceType::DATA.
    *
    * The returning surface might be null, because of OOM or gfx device reset.
    * The caller needs to do null-check before using it.
@@ -609,17 +622,17 @@ public:
   }
 
   /**
    * Indicate a region which has changed in the surface.
    */
   virtual void Invalidate(const IntRect& aDirtyRect) { }
 
 protected:
-  bool mIsMapped;
+  Atomic<int32_t> mMapCount;
 };
 
 /** This is an abstract object that accepts path segments. */
 class PathSink : public RefCounted<PathSink>
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(PathSink)
   virtual ~PathSink() {}
--- a/gfx/2d/SourceSurfaceD2D1.cpp
+++ b/gfx/2d/SourceSurfaceD2D1.cpp
@@ -159,23 +159,24 @@ SourceSurfaceD2D1::MarkIndependent()
     mDrawTarget->mSnapshot = nullptr;
     mDrawTarget = nullptr;
   }
 }
 
 DataSourceSurfaceD2D1::DataSourceSurfaceD2D1(ID2D1Bitmap1 *aMappableBitmap, SurfaceFormat aFormat)
   : mBitmap(aMappableBitmap)
   , mFormat(aFormat)
-  , mMapped(false)
+  , mIsMapped(false)
+  , mImplicitMapped(false)
 {
 }
 
 DataSourceSurfaceD2D1::~DataSourceSurfaceD2D1()
 {
-  if (mMapped) {
+  if (mImplicitMapped) {
     mBitmap->Unmap();
   }
 }
 
 IntSize
 DataSourceSurfaceD2D1::GetSize() const
 {
   D2D1_SIZE_F size = mBitmap->GetSize();
@@ -190,17 +191,17 @@ DataSourceSurfaceD2D1::GetData()
 
   return mMap.bits;
 }
 
 bool
 DataSourceSurfaceD2D1::Map(MapType aMapType, MappedSurface *aMappedSurface)
 {
   // DataSourceSurfaces used with the new Map API should not be used with GetData!!
-  MOZ_ASSERT(!mMapped);
+  MOZ_ASSERT(!mImplicitMapped);
   MOZ_ASSERT(!mIsMapped);
 
   D2D1_MAP_OPTIONS options;
   if (aMapType == MapType::READ) {
     options = D2D1_MAP_OPTIONS_READ;
   } else {
     gfxWarning() << "Attempt to map D2D1 DrawTarget for writing.";
     return false;
@@ -235,20 +236,20 @@ DataSourceSurfaceD2D1::Stride()
   return mMap.pitch;
 }
 
 void
 DataSourceSurfaceD2D1::EnsureMapped()
 {
   // Do not use GetData() after having used Map!
   MOZ_ASSERT(!mIsMapped);
-  if (mMapped) {
+  if (mImplicitMapped) {
     return;
   }
   if (FAILED(mBitmap->Map(D2D1_MAP_OPTIONS_READ, &mMap))) {
     gfxCriticalError() << "Failed to map bitmap (EM).";
     return;
   }
-  mMapped = true;
+  mImplicitMapped = true;
 }
 
 }
 }
--- a/gfx/2d/SourceSurfaceD2D1.h
+++ b/gfx/2d/SourceSurfaceD2D1.h
@@ -85,15 +85,16 @@ public:
 
 private:
   friend class SourceSurfaceD2DTarget;
   void EnsureMapped();
 
   mutable RefPtr<ID2D1Bitmap1> mBitmap;
   SurfaceFormat mFormat;
   D2D1_MAPPED_RECT mMap;
-  bool mMapped;
+  bool mIsMapped;
+  bool mImplicitMapped;
 };
 
 }
 }
 
 #endif /* MOZILLA_GFX_SOURCESURFACED2D2TARGET_H_ */
--- a/gfx/2d/SourceSurfaceRawData.h
+++ b/gfx/2d/SourceSurfaceRawData.h
@@ -18,70 +18,41 @@ class SourceSurfaceRawData : public Data
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DataSourceSurfaceRawData, override)
 
   SourceSurfaceRawData()
     : mRawData(0)
     , mStride(0)
     , mFormat(SurfaceFormat::UNKNOWN)
-    , mMapCount(0)
     , mOwnData(false)
     , mDeallocator(nullptr)
     , mClosure(nullptr)
   {
   }
 
   virtual ~SourceSurfaceRawData()
   {
     if (mDeallocator) {
       mDeallocator(mClosure);
     } else if (mOwnData) {
       // The buffer is created from GuaranteePersistance().
       delete [] mRawData;
     }
-
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   virtual uint8_t *GetData() override { return mRawData; }
   virtual int32_t Stride() override { return mStride; }
 
   virtual SurfaceType GetType() const override { return SurfaceType::DATA; }
   virtual IntSize GetSize() const override { return mSize; }
   virtual SurfaceFormat GetFormat() const override { return mFormat; }
 
   virtual void GuaranteePersistance() override;
 
-  // Althought Map (and Moz2D in general) isn't normally threadsafe,
-  // we want to allow it for SourceSurfaceRawData since it should
-  // always be fine (for reading at least).
-  //
-  // This is the same as the base class implementation except using
-  // mMapCount instead of mIsMapped since that breaks for multithread.
-  //
-  // Once mfbt supports Monitors we should implement proper read/write
-  // locking to prevent write races.
-  virtual bool Map(MapType, MappedSurface *aMappedSurface) override
-  {
-    aMappedSurface->mData = GetData();
-    aMappedSurface->mStride = Stride();
-    bool success = !!aMappedSurface->mData;
-    if (success) {
-      mMapCount++;
-    }
-    return success;
-  }
-
-  virtual void Unmap() override
-  {
-    mMapCount--;
-    MOZ_ASSERT(mMapCount >= 0);
-  }
-
 private:
   friend class Factory;
 
   // If we have a custom deallocator, the |aData| will be released using the
   // custom deallocator and |aClosure| in dtor.  The assumption is that the
   // caller will check for valid size and stride before making this call.
   void InitWrappingData(unsigned char *aData,
                         const IntSize &aSize,
@@ -89,35 +60,32 @@ private:
                         SurfaceFormat aFormat,
                         Factory::SourceSurfaceDeallocator aDeallocator,
                         void* aClosure);
 
   uint8_t *mRawData;
   int32_t mStride;
   SurfaceFormat mFormat;
   IntSize mSize;
-  Atomic<int32_t> mMapCount;
 
   bool mOwnData;
   Factory::SourceSurfaceDeallocator mDeallocator;
   void* mClosure;
 };
 
 class SourceSurfaceAlignedRawData : public DataSourceSurface
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(DataSourceSurfaceAlignedRawData, override)
   SourceSurfaceAlignedRawData()
     : mStride(0)
     , mFormat(SurfaceFormat::UNKNOWN)
-    , mMapCount(0)
   {}
   ~SourceSurfaceAlignedRawData()
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   bool Init(const IntSize &aSize,
             SurfaceFormat aFormat,
             bool aClearMem,
             uint8_t aClearValue,
             int32_t aStride = 0);
 
@@ -129,39 +97,21 @@ public:
   virtual SurfaceFormat GetFormat() const override { return mFormat; }
 
   void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
                               size_t& aHeapSizeOut,
                               size_t& aNonHeapSizeOut,
                               size_t& aExtHandlesOut,
                               uint64_t& aExtIdOut) const override;
 
-  virtual bool Map(MapType, MappedSurface *aMappedSurface) override
-  {
-    aMappedSurface->mData = GetData();
-    aMappedSurface->mStride = Stride();
-    bool success = !!aMappedSurface->mData;
-    if (success) {
-      mMapCount++;
-    }
-    return success;
-  }
-
-  virtual void Unmap() override
-  {
-    mMapCount--;
-    MOZ_ASSERT(mMapCount >= 0);
-  }
-
 private:
   friend class Factory;
 
   AlignedArray<uint8_t> mArray;
   int32_t mStride;
   SurfaceFormat mFormat;
   IntSize mSize;
-  Atomic<int32_t> mMapCount;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif /* MOZILLA_GFX_SOURCESURFACERAWDATA_H_ */
--- a/gfx/2d/SourceSurfaceSkia.cpp
+++ b/gfx/2d/SourceSurfaceSkia.cpp
@@ -18,16 +18,17 @@ using namespace std;
 namespace mozilla {
 namespace gfx {
 
 SourceSurfaceSkia::SourceSurfaceSkia()
   : mFormat(SurfaceFormat::UNKNOWN)
   , mStride(0)
   , mDrawTarget(nullptr)
   , mChangeMutex("SourceSurfaceSkia::mChangeMutex")
+  , mIsMapped(false)
 {
 }
 
 SourceSurfaceSkia::~SourceSurfaceSkia()
 {
 }
 
 IntSize
--- a/gfx/2d/SourceSurfaceSkia.h
+++ b/gfx/2d/SourceSurfaceSkia.h
@@ -68,14 +68,15 @@ private:
   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;
   DrawTargetSkia* mDrawTarget;
   Mutex mChangeMutex;
+  bool mIsMapped;
 };
 
 } // namespace gfx
 } // namespace mozilla
 
 #endif /* MOZILLA_GFX_SOURCESURFACESKIA_H_ */
--- a/gfx/layers/SourceSurfaceSharedData.h
+++ b/gfx/layers/SourceSurfaceSharedData.h
@@ -133,17 +133,16 @@ class SourceSurfaceSharedData final : pu
   typedef mozilla::ipc::SharedMemoryBasic SharedMemoryBasic;
 
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(SourceSurfaceSharedData, override)
 
   SourceSurfaceSharedData()
     : mMutex("SourceSurfaceSharedData")
     , mStride(0)
-    , mMapCount(0)
     , mHandleCount(0)
     , mFormat(SurfaceFormat::UNKNOWN)
     , mClosed(false)
     , mFinalized(false)
     , mShared(false)
   {
   }
 
@@ -321,17 +320,16 @@ public:
     RefPtr<SourceSurfaceSharedData> mSurface;
   };
 
 private:
   friend class SourceSurfaceSharedDataWrapper;
 
   ~SourceSurfaceSharedData() override
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   void LockHandle()
   {
     MutexAutoLock lock(mMutex);
     ++mHandleCount;
   }
 
@@ -359,17 +357,16 @@ private:
   /**
    * Attempt to close the handle. Only if the buffer has been both finalized
    * and we have completed sharing will it be released.
    */
   void CloseHandleInternal();
 
   mutable Mutex mMutex;
   int32_t mStride;
-  int32_t mMapCount;
   int32_t mHandleCount;
   Maybe<IntRect> mDirtyRect;
   IntSize mSize;
   RefPtr<SharedMemoryBasic> mBuf;
   RefPtr<SharedMemoryBasic> mOldBuf;
   SurfaceFormat mFormat;
   bool mClosed : 1;
   bool mFinalized : 1;
--- a/gfx/layers/SourceSurfaceVolatileData.h
+++ b/gfx/layers/SourceSurfaceVolatileData.h
@@ -26,17 +26,16 @@ namespace gfx {
 class SourceSurfaceVolatileData : public DataSourceSurface
 {
 public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(SourceSurfaceVolatileData, override)
 
   SourceSurfaceVolatileData()
     : mMutex("SourceSurfaceVolatileData")
     , mStride(0)
-    , mMapCount(0)
     , mFormat(SurfaceFormat::UNKNOWN)
     , mWasPurged(false)
   {
   }
 
   bool Init(const IntSize &aSize,
             int32_t aStride,
             SurfaceFormat aFormat);
@@ -94,22 +93,20 @@ public:
     if (--mMapCount == 0) {
       mVBufPtr = nullptr;
     }
   }
 
 private:
   ~SourceSurfaceVolatileData() override
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   Mutex mMutex;
   int32_t mStride;
-  int32_t mMapCount;
   IntSize mSize;
   RefPtr<VolatileBuffer> mVBuf;
   VolatileBufferPtr<uint8_t> mVBufPtr;
   SurfaceFormat mFormat;
   bool mWasPurged;
 };
 
 } // namespace gfx