From 179924cbfaf3f4ec073fadac65748da0a5579722 Mon Sep 17 00:00:00 2001 From: Roman Lavrov Date: Thu, 27 Jul 2023 20:45:26 +0000 Subject: [PATCH] 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 > Commit-Queue: Igor Nazarov 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 Bot-Commit: Rubber Stamper --- src/libANGLE/Image.cpp | 14 +++++--------- src/libANGLE/SharedContextMutex.cpp | 27 +++++++++++++-------------- src/libANGLE/SharedContextMutex.h | 16 ++++++++-------- 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/src/libANGLE/Image.cpp b/src/libANGLE/Image.cpp index 1333fdcfa..5d23367e0 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 @@ Image::Image(rx::EGLImplFactory *factory, 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 @@ Image::Image(rx::EGLImplFactory *factory, mSharedContextMutex = context->getContextMutex(); mSharedContextMutex->addRef(); } + else + { + mSharedContextMutex = nullptr; + } mState.source->addImageSource(this); } @@ -497,12 +499,6 @@ rx::ImageImpl *Image::getImplementation() const 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(mState.source); diff --git a/src/libANGLE/SharedContextMutex.cpp b/src/libANGLE/SharedContextMutex.cpp index 694b3ab47..352a6781b 100644 --- a/src/libANGLE/SharedContextMutex.cpp +++ b/src/libANGLE/SharedContextMutex.cpp @@ -241,29 +241,28 @@ void SharedContextMutex::Merge(SharedContextMutex *lockedMutex, // 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 diff --git a/src/libANGLE/SharedContextMutex.h b/src/libANGLE/SharedContextMutex.h index 8d9d15c19..7bb0c0ed8 100644 --- a/src/libANGLE/SharedContextMutex.h +++ b/src/libANGLE/SharedContextMutex.h @@ -219,7 +219,7 @@ class SharedContextMutex final : public ContextMutex // - 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 @@ class SharedContextMutex final : public ContextMutex std::vector 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).