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

--HG--
extra : moz-landing-system : lando

gfx/2d/2D.h

@@ -31,6 +31,7 @@
 #include "mozilla/StaticMutex.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/ThreadSafeWeakPtr.h"
+#include "mozilla/Atomics.h"
 
 #include "mozilla/DebugOnly.h"
 
@@ -445,14 +446,14 @@ 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
 
@@ -557,19 +558,31 @@ public:
 
   /**
    * 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);
   }
 
   /**
@@ -614,7 +627,7 @@ public:
   virtual void Invalidate(const IntRect& aDirtyRect) { }
 
 protected:
-  bool mIsMapped;
+  Atomic<int32_t> mMapCount;
 };
 
 /** This is an abstract object that accepts path segments. */

gfx/2d/SourceSurfaceD2D1.cpp

@@ -164,13 +164,14 @@ SourceSurfaceD2D1::MarkIndependent()
 DataSourceSurfaceD2D1::DataSourceSurfaceD2D1(ID2D1Bitmap1 *aMappableBitmap, SurfaceFormat aFormat)
   : mBitmap(aMappableBitmap)
   , mFormat(aFormat)
-  , mMapped(false)
+  , mIsMapped(false)
+  , mImplicitMapped(false)
 {
 }
 
 DataSourceSurfaceD2D1::~DataSourceSurfaceD2D1()
 {
-  if (mMapped) {
+  if (mImplicitMapped) {
     mBitmap->Unmap();
   }
 }
@@ -195,7 +196,7 @@ 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;
@@ -240,14 +241,14 @@ 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;
 }
 
 }

gfx/2d/SourceSurfaceD2D1.h

@@ -90,7 +90,8 @@ private:
   mutable RefPtr<ID2D1Bitmap1> mBitmap;
   SurfaceFormat mFormat;
   D2D1_MAPPED_RECT mMap;
-  bool mMapped;
+  bool mIsMapped;
+  bool mImplicitMapped;
 };
 
 }

gfx/2d/SourceSurfaceRawData.h

@@ -23,7 +23,6 @@ public:
     : mRawData(0)
     , mStride(0)
     , mFormat(SurfaceFormat::UNKNOWN)
-    , mMapCount(0)
     , mOwnData(false)
     , mDeallocator(nullptr)
     , mClosure(nullptr)
@@ -38,8 +37,6 @@ public:
       // The buffer is created from GuaranteePersistance().
       delete [] mRawData;
     }
-
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   virtual uint8_t *GetData() override { return mRawData; }
@@ -51,32 +48,6 @@ public:
 
   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;
 
@@ -94,7 +65,6 @@ private:
   int32_t mStride;
   SurfaceFormat mFormat;
   IntSize mSize;
-  Atomic<int32_t> mMapCount;
 
   bool mOwnData;
   Factory::SourceSurfaceDeallocator mDeallocator;
@@ -108,11 +78,9 @@ public:
   SourceSurfaceAlignedRawData()
     : mStride(0)
     , mFormat(SurfaceFormat::UNKNOWN)
-    , mMapCount(0)
   {}
   ~SourceSurfaceAlignedRawData()
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   bool Init(const IntSize &aSize,
@@ -134,23 +102,6 @@ public:
                               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;
 
@@ -158,7 +109,6 @@ private:
   int32_t mStride;
   SurfaceFormat mFormat;
   IntSize mSize;
-  Atomic<int32_t> mMapCount;
 };
 
 } // namespace gfx

gfx/2d/SourceSurfaceSkia.cpp

@@ -23,6 +23,7 @@ SourceSurfaceSkia::SourceSurfaceSkia()
   , mStride(0)
   , mDrawTarget(nullptr)
   , mChangeMutex("SourceSurfaceSkia::mChangeMutex")
+  , mIsMapped(false)
 {
 }
 

gfx/2d/SourceSurfaceSkia.h

@@ -73,6 +73,7 @@ private:
   int32_t mStride;
   DrawTargetSkia* mDrawTarget;
   Mutex mChangeMutex;
+  bool mIsMapped;
 };
 
 } // namespace gfx

gfx/layers/SourceSurfaceSharedData.h

@@ -138,7 +138,6 @@ public:
   SourceSurfaceSharedData()
     : mMutex("SourceSurfaceSharedData")
     , mStride(0)
-    , mMapCount(0)
     , mHandleCount(0)
     , mFormat(SurfaceFormat::UNKNOWN)
     , mClosed(false)
@@ -326,7 +325,6 @@ private:
 
   ~SourceSurfaceSharedData() override
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   void LockHandle()
@@ -364,7 +362,6 @@ private:
 
   mutable Mutex mMutex;
   int32_t mStride;
-  int32_t mMapCount;
   int32_t mHandleCount;
   Maybe<IntRect> mDirtyRect;
   IntSize mSize;

gfx/layers/SourceSurfaceVolatileData.h

@@ -31,7 +31,6 @@ public:
   SourceSurfaceVolatileData()
     : mMutex("SourceSurfaceVolatileData")
     , mStride(0)
-    , mMapCount(0)
     , mFormat(SurfaceFormat::UNKNOWN)
     , mWasPurged(false)
   {
@@ -99,12 +98,10 @@ public:
 private:
   ~SourceSurfaceVolatileData() override
   {
-    MOZ_ASSERT(mMapCount == 0);
   }
 
   Mutex mMutex;
   int32_t mStride;
-  int32_t mMapCount;
   IntSize mSize;
   RefPtr<VolatileBuffer> mVBuf;
   VolatileBufferPtr<uint8_t> mVBufPtr;