Revert "Fix ExternalImageTarget EGLImage race"
This reverts commit 8aa3ca9d177c0ed54926b769de7d0bce0f8482d3.
Reason for revert: Confirmed to break Android's testDrawingHardwareBitmapNotLeaking in this single-commit roll: https://r.android.com/2679397
Original change's description:
> Fix ExternalImageTarget EGLImage race
>
> Race may happen when ExternalImageTarget EGLImage is destroyed while its
> GLES Texture/Renderbuffer target is modified/destroyed.
>
> Fixed by providing `egl::Image` with `egl::ContextMutex` even when
> `context` is `nullptr`.
>
> This CL also changes `SharedContextMutex` merging rules when `mRank` is
> equal - now priority goes to the `lockedMutex`. This is done to prevent
> unnecessary `mRoot` update of Context mutex when merging with
> `egl::Image` only mutex.
>
> Bug: angleproject:6957
> Change-Id: I823e53b98f70ed3eaca191e8be5b168dc07899f6
> Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4720835
> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
> Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
Bug: angleproject:6957
Change-Id: I860a8bfd6dd66eb549045391755a83483109ebbb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4727621
Commit-Queue: Roman Lavrov <romanl@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
diff --git a/src/libANGLE/Image.cpp b/src/libANGLE/Image.cpp
index 1333fdc..5d23367 100644
--- a/src/libANGLE/Image.cpp
+++ b/src/libANGLE/Image.cpp
@@ -11,7 +11,6 @@
#include "common/debug.h"
#include "common/utilities.h"
#include "libANGLE/Context.h"
-#include "libANGLE/Display.h"
#include "libANGLE/Renderbuffer.h"
#include "libANGLE/Texture.h"
#include "libANGLE/angletypes.h"
@@ -311,8 +310,7 @@
const AttributeMap &attribs)
: mState(id, target, buffer, attribs),
mImplementation(factory->createImage(mState, context, target, attribs)),
- mOrphanedAndNeedsInit(false),
- mSharedContextMutex(nullptr)
+ mOrphanedAndNeedsInit(false)
{
ASSERT(mImplementation != nullptr);
ASSERT(buffer != nullptr);
@@ -323,6 +321,10 @@
mSharedContextMutex = context->getContextMutex();
mSharedContextMutex->addRef();
}
+ else
+ {
+ mSharedContextMutex = nullptr;
+ }
mState.source->addImageSource(this);
}
@@ -497,12 +499,6 @@
Error Image::initialize(const Display *display, const gl::Context *context)
{
- if (kIsSharedContextMutexEnabled && mSharedContextMutex == nullptr)
- {
- mSharedContextMutex = display->getSharedContextMutexManager()->create();
- mSharedContextMutex->addRef();
- }
-
if (IsExternalImageTarget(mState.target))
{
ExternalImageSibling *externalSibling = rx::GetAs<ExternalImageSibling>(mState.source);
diff --git a/src/libANGLE/SharedContextMutex.cpp b/src/libANGLE/SharedContextMutex.cpp
index 694b3ab..352a678 100644
--- a/src/libANGLE/SharedContextMutex.cpp
+++ b/src/libANGLE/SharedContextMutex.cpp
@@ -241,29 +241,28 @@
// Decide the new "root". See mRank comment for more details...
- SharedContextMutex *oldRoot = otherLockedRoot;
- SharedContextMutex *newRoot = lockedRoot;
-
- if (oldRoot->mRank > newRoot->mRank)
+ // Make "otherLockedRoot" the root of the "merged" mutex
+ if (lockedRoot->mRank > otherLockedRoot->mRank)
{
- std::swap(oldRoot, newRoot);
+ // So the "lockedRoot" is lower rank.
+ std::swap(lockedRoot, otherLockedRoot);
}
- else if (oldRoot->mRank == newRoot->mRank)
+ else if (lockedRoot->mRank == otherLockedRoot->mRank)
{
- ++newRoot->mRank;
+ ++otherLockedRoot->mRank;
}
// Update the structure
- for (SharedContextMutex *const leaf : oldRoot->mLeaves)
+ for (SharedContextMutex *const leaf : lockedRoot->mLeaves)
{
- ASSERT(leaf->getRoot() == oldRoot);
- leaf->setNewRoot(newRoot);
+ ASSERT(leaf->getRoot() == lockedRoot);
+ leaf->setNewRoot(otherLockedRoot);
}
- oldRoot->mLeaves.clear();
- oldRoot->setNewRoot(newRoot);
+ lockedRoot->mLeaves.clear();
+ lockedRoot->setNewRoot(otherLockedRoot);
- // Leave only the "merged" mutex locked. "oldRoot" already merged, need to use "doUnlock()"
- oldRoot->doUnlock();
+ // Leave only the "merged" mutex locked. "lockedRoot" already merged, need to use "doUnlock()"
+ lockedRoot->doUnlock();
}
template <class Mutex>
diff --git a/src/libANGLE/SharedContextMutex.h b/src/libANGLE/SharedContextMutex.h
index 8d9d15c..7bb0c0e 100644
--- a/src/libANGLE/SharedContextMutex.h
+++ b/src/libANGLE/SharedContextMutex.h
@@ -219,7 +219,7 @@
// - the mutex_1 has no other references (only in the mutex_2);
// - have other mutex_3 "root";
// - mutex_1 pointer is cached on the stack during locking of mutex_2 (thread A);
- // - merge mutex_3 and mutex_2 (thread B):
+ // - marge mutex_2 and mutex_3 (thread B):
// * now "leaf" mutex_2 stores reference to mutex_3 "root";
// * old "root" mutex_1 becomes a "leaf" of mutex_3;
// * old "root" mutex_1 has no references and gets destroyed.
@@ -228,27 +228,27 @@
std::vector<SharedContextMutex *> mOldRoots;
// mRank is used to fix a problem of indefinite grows of mOldRoots:
- // - merge mutex_2 and mutex_1 -> mutex_2 is "root" of mutex_1 (mOldRoots == 0);
+ // - merge mutex_1 and mutex_2 -> mutex_2 is "root" of mutex_1 (mOldRoots == 0);
// - destroy mutex_2;
- // - merge mutex_3 and mutex_1 -> mutex_3 is "root" of mutex_1 (mOldRoots == 1);
+ // - merge mutex_1 and mutex_3 -> mutex_3 is "root" of mutex_1 (mOldRoots == 1);
// - destroy mutex_3;
- // - merge mutex_4 and mutex_1 -> mutex_4 is "root" of mutex_1 (mOldRoots == 2);
+ // - merge mutex_1 and mutex_4 -> mutex_4 is "root" of mutex_1 (mOldRoots == 2);
// - destroy mutex_4;
// - continuing this pattern can lead to indefinite grows of mOldRoots, while pick number of
// mutexes is only 2.
// Fix details using mRank:
// - initially "mRank == 0" and only relevant for "root" mutexes;
- // - merging mutexes with equal mRank of their "root"s, will use first (lockedMutex) "root"
+ // - merging mutexes with equal mRank of their "root"s, will use second (otherMutex) "root"
// mutex as a new "root" and increase its mRank by 1;
// - otherwise, "root" mutex with a highest rank will be used without changing the mRank;
// - this way, "stronger" (with a higher mRank) "root" mutex will "protect" its "leaves" from
// "mRoot" replacement and therefore - mOldRoots grows.
// Lets look at the problematic pattern with the mRank:
- // - merge mutex_2 and mutex_1 -> mutex_2 is "root" (mRank == 1) of mutex_1 (mOldRoots == 0);
+ // - merge mutex_1 and mutex_2 -> mutex_2 is "root" (mRank == 1) of mutex_1 (mOldRoots == 0);
// - destroy mutex_2;
- // - merge mutex_3 and mutex_1 -> mutex_2 is "root" (mRank == 1) of mutex_3 (mOldRoots == 0);
+ // - merge mutex_1 and mutex_3 -> mutex_2 is "root" (mRank == 1) of mutex_3 (mOldRoots == 0);
// - destroy mutex_3;
- // - merge mutex_4 and mutex_1 -> mutex_2 is "root" (mRank == 1) of mutex_4 (mOldRoots == 0);
+ // - merge mutex_1 and mutex_4 -> mutex_2 is "root" (mRank == 1) of mutex_4 (mOldRoots == 0);
// - destroy mutex_4;
// - no mOldRoots grows at all;
// - minumum number of mutexes to reach mOldRoots size of N => 2^(N+1).
