Bug 1425257: Introduce a global lock to protect the dependency graph between DrawTargets. r=lsalzman
This patch takes the safest route for securing modifications to the dependency graph for D2D DrawTargets. It's possible a slightly optimal approach is possible, however lock contention should be rare and I believe this is the safest and most upliftable approach.
MozReview-Commit-ID: HGfSdEp2U5N
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -1750,16 +1750,19 @@ private:
static StaticRefPtr<ID3D11Device> mD3D11Device;
static StaticRefPtr<IDWriteFactory> mDWriteFactory;
static bool mDWriteFactoryInitialized;
protected:
// This guards access to the singleton devices above, as well as the
// singleton devices in DrawTargetD2D1.
static StaticMutex mDeviceLock;
+ // This synchronizes access between different D2D drawtargets and their
+ // implied dependency graph.
+ static StaticMutex mDTDependencyLock;
friend class DrawTargetD2D1;
#endif
private:
static DrawEventRecorder *mRecorder;
};
--- a/gfx/2d/DrawTargetD2D1.cpp
+++ b/gfx/2d/DrawTargetD2D1.cpp
@@ -71,26 +71,31 @@ DrawTargetD2D1::~DrawTargetD2D1()
if (mDC && IsDeviceContextValid()) {
// The only way mDC can be null is if Init failed, but it can happen and the
// destructor is the only place where we need to check for it since the
// DrawTarget will destroyed right after Init fails.
mDC->EndDraw();
}
- // Targets depending on us can break that dependency, since we're obviously not going to
- // be modified in the future.
- for (auto iter = mDependentTargets.begin();
- iter != mDependentTargets.end(); iter++) {
- (*iter)->mDependingOnTargets.erase(this);
- }
- // Our dependencies on other targets no longer matter.
- for (TargetSet::iterator iter = mDependingOnTargets.begin();
- iter != mDependingOnTargets.end(); iter++) {
- (*iter)->mDependentTargets.erase(this);
+ {
+ // Until this point in the destructor it -must- still be valid for FlushInternal
+ // to be called on this.
+ StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+ // Targets depending on us can break that dependency, since we're obviously not going to
+ // be modified in the future.
+ for (auto iter = mDependentTargets.begin();
+ iter != mDependentTargets.end(); iter++) {
+ (*iter)->mDependingOnTargets.erase(this);
+ }
+ // Our dependencies on other targets no longer matter.
+ for (TargetSet::iterator iter = mDependingOnTargets.begin();
+ iter != mDependingOnTargets.end(); iter++) {
+ (*iter)->mDependentTargets.erase(this);
+ }
}
}
already_AddRefed<SourceSurface>
DrawTargetD2D1::Snapshot()
{
MutexAutoLock lock(*mSnapshotLock);
if (mSnapshot) {
@@ -151,38 +156,17 @@ DrawTargetD2D1::IntoLuminanceSource(Lumi
// this can cause issues with memory usage (see bug 1238328). EndDraw/BeginDraw
// are expensive though, especially relatively when little work is done, so
// we try to reduce the amount of times we execute these purges.
static const uint32_t kPushedLayersBeforePurge = 25;
void
DrawTargetD2D1::Flush()
{
- if (IsDeviceContextValid()) {
- if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
- mPushedLayers.size() == 1) {
- // It's important to pop all clips as otherwise layers can forget about
- // their clip when doing an EndDraw. When we have layers pushed we cannot
- // easily pop all underlying clips to delay the purge until we have no
- // layers pushed.
- PopAllClips();
- mUsedCommandListsSincePurge = 0;
- mDC->EndDraw();
- mDC->BeginDraw();
- } else {
- mDC->Flush();
- }
- }
-
- // We no longer depend on any target.
- for (TargetSet::iterator iter = mDependingOnTargets.begin();
- iter != mDependingOnTargets.end(); iter++) {
- (*iter)->mDependentTargets.erase(this);
- }
- mDependingOnTargets.clear();
+ FlushInternal();
}
void
DrawTargetD2D1::DrawSurface(SourceSurface *aSurface,
const Rect &aDest,
const Rect &aSource,
const DrawSurfaceOptions &aSurfOptions,
const DrawOptions &aOptions)
@@ -1267,38 +1251,77 @@ DrawTargetD2D1::CleanupD2D()
if (mFactory) {
RadialGradientEffectD2D1::Unregister(mFactory);
ExtendInputEffectD2D1::Unregister(mFactory);
mFactory = nullptr;
}
}
void
+DrawTargetD2D1::FlushInternal(bool aHasDependencyMutex /* = false */)
+{
+ if (IsDeviceContextValid()) {
+ if ((mUsedCommandListsSincePurge >= kPushedLayersBeforePurge) &&
+ mPushedLayers.size() == 1) {
+ // It's important to pop all clips as otherwise layers can forget about
+ // their clip when doing an EndDraw. When we have layers pushed we cannot
+ // easily pop all underlying clips to delay the purge until we have no
+ // layers pushed.
+ PopAllClips();
+ mUsedCommandListsSincePurge = 0;
+ mDC->EndDraw();
+ mDC->BeginDraw();
+ }
+ else {
+ mDC->Flush();
+ }
+ }
+
+ Maybe<StaticMutexAutoLock> lock;
+
+ if (!aHasDependencyMutex) {
+ lock.emplace(Factory::mDTDependencyLock);
+ }
+
+ Factory::mDTDependencyLock.AssertCurrentThreadOwns();
+ // We no longer depend on any target.
+ for (TargetSet::iterator iter = mDependingOnTargets.begin();
+ iter != mDependingOnTargets.end(); iter++) {
+ (*iter)->mDependentTargets.erase(this);
+ }
+ mDependingOnTargets.clear();
+}
+
+void
DrawTargetD2D1::MarkChanged()
{
if (mSnapshot) {
MutexAutoLock lock(*mSnapshotLock);
if (mSnapshot->hasOneRef()) {
// Just destroy it, since no-one else knows about it.
mSnapshot = nullptr;
} else {
mSnapshot->DrawTargetWillChange();
// The snapshot will no longer depend on this target.
MOZ_ASSERT(!mSnapshot);
}
}
- if (mDependentTargets.size()) {
- // Copy mDependentTargets since the Flush()es below will modify it.
- TargetSet tmpTargets = mDependentTargets;
- for (TargetSet::iterator iter = tmpTargets.begin();
- iter != tmpTargets.end(); iter++) {
- (*iter)->Flush();
+
+ {
+ StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+ if (mDependentTargets.size()) {
+ // Copy mDependentTargets since the Flush()es below will modify it.
+ TargetSet tmpTargets = mDependentTargets;
+ for (TargetSet::iterator iter = tmpTargets.begin();
+ iter != tmpTargets.end(); iter++) {
+ (*iter)->FlushInternal(true);
+ }
+ // The Flush() should have broken all dependencies on this target.
+ MOZ_ASSERT(!mDependentTargets.size());
}
- // The Flush() should have broken all dependencies on this target.
- MOZ_ASSERT(!mDependentTargets.size());
}
}
bool
DrawTargetD2D1::ShouldClipTemporarySurfaceDrawing(CompositionOp aOp,
const Pattern& aPattern,
bool aClipIsComplex)
{
@@ -1459,19 +1482,28 @@ DrawTargetD2D1::FinalizeDrawing(Composit
radialGradientEffect->SetInput(0, source);
mDC->DrawImage(radialGradientEffect, D2D1_INTERPOLATION_MODE_NEAREST_NEIGHBOR, D2DCompositionMode(aOp));
}
void
DrawTargetD2D1::AddDependencyOnSource(SourceSurfaceD2D1* aSource)
{
- if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
- aSource->mDrawTarget->mDependentTargets.insert(this);
- mDependingOnTargets.insert(aSource->mDrawTarget);
+ Maybe<MutexAutoLock> snapshotLock;
+ // We grab the SnapshotLock as well, this guaranteeds aSource->mDrawTarget
+ // cannot be cleared in between the if statement and the dereference.
+ if (aSource->mSnapshotLock) {
+ snapshotLock.emplace(*aSource->mSnapshotLock);
+ }
+ {
+ StaticMutexAutoLock lock(Factory::mDTDependencyLock);
+ if (aSource->mDrawTarget && !mDependingOnTargets.count(aSource->mDrawTarget)) {
+ aSource->mDrawTarget->mDependentTargets.insert(this);
+ mDependingOnTargets.insert(aSource->mDrawTarget);
+ }
}
}
static D2D1_RECT_F
IntersectRect(const D2D1_RECT_F& aRect1, const D2D1_RECT_F& aRect2)
{
D2D1_RECT_F result;
result.left = max(aRect1.left, aRect2.left);
--- a/gfx/2d/DrawTargetD2D1.h
+++ b/gfx/2d/DrawTargetD2D1.h
@@ -168,16 +168,18 @@ public:
}
static uint64_t mVRAMUsageDT;
static uint64_t mVRAMUsageSS;
private:
friend class SourceSurfaceD2D1;
+ void FlushInternal(bool aHasDependencyMutex = false);
+
typedef std::unordered_set<DrawTargetD2D1*> TargetSet;
// This function will mark the surface as changing, and make sure any
// copy-on-write snapshots are notified.
void MarkChanged();
bool ShouldClipTemporarySurfaceDrawing(CompositionOp aOp, const Pattern& aPattern, bool aClipIsComplex);
void PrepareForDrawing(CompositionOp aOp, const Pattern &aPattern);
void FinalizeDrawing(CompositionOp aOp, const Pattern &aPattern);
--- a/gfx/2d/Factory.cpp
+++ b/gfx/2d/Factory.cpp
@@ -217,16 +217,17 @@ Mutex* Factory::mFTLock = nullptr;
#ifdef WIN32
// Note: mDeviceLock must be held when mutating these values.
static uint32_t mDeviceSeq = 0;
StaticRefPtr<ID3D11Device> Factory::mD3D11Device;
StaticRefPtr<ID2D1Device> Factory::mD2D1Device;
StaticRefPtr<IDWriteFactory> Factory::mDWriteFactory;
bool Factory::mDWriteFactoryInitialized = false;
StaticMutex Factory::mDeviceLock;
+StaticMutex Factory::mDTDependencyLock;
#endif
DrawEventRecorder *Factory::mRecorder;
mozilla::gfx::Config* Factory::sConfig = nullptr;
void
Factory::Init(const Config& aConfig)