From 48a619f8332e06ea1cd96d82719cdf5e05c69630 Mon Sep 17 00:00:00 2001 From: Yi Kong Date: Tue, 5 Jun 2018 16:34:59 -0700 Subject: Replace NULL/0 with nullptr Fixes -Wzero-as-null-pointer-constant warning. clang-tidy -checks=modernize-use-nullptr -p compile_commands.json -fix ... Test: m Bug: 68236239 Change-Id: I3a8e982ba40f9b029bafef78437b146a878f56a9 --- libs/gui/SurfaceControl.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 5eafbb3555..19ad31b8c7 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -86,7 +86,7 @@ void SurfaceControl::clear() } void SurfaceControl::disconnect() { - if (mGraphicBufferProducer != NULL) { + if (mGraphicBufferProducer != nullptr) { mGraphicBufferProducer->disconnect( BufferQueueCore::CURRENTLY_CONNECTED_API); } @@ -95,7 +95,7 @@ void SurfaceControl::disconnect() { bool SurfaceControl::isSameSurface( const sp& lhs, const sp& rhs) { - if (lhs == 0 || rhs == 0) + if (lhs == nullptr || rhs == nullptr) return false; return lhs->mHandle == rhs->mHandle; } @@ -116,7 +116,7 @@ status_t SurfaceControl::getLayerFrameStats(FrameStats* outStats) const { status_t SurfaceControl::validate() const { - if (mHandle==0 || mClient==0) { + if (mHandle==nullptr || mClient==nullptr) { ALOGE("invalid handle (%p) or client (%p)", mHandle.get(), mClient.get()); return NO_INIT; @@ -128,7 +128,7 @@ status_t SurfaceControl::writeSurfaceToParcel( const sp& control, Parcel* parcel) { sp bp; - if (control != NULL) { + if (control != nullptr) { bp = control->mGraphicBufferProducer; } return parcel->writeStrongBinder(IInterface::asBinder(bp)); @@ -146,7 +146,7 @@ sp SurfaceControl::generateSurfaceLocked() const sp SurfaceControl::getSurface() const { Mutex::Autolock _l(mLock); - if (mSurfaceData == 0) { + if (mSurfaceData == nullptr) { return generateSurfaceLocked(); } return mSurfaceData; -- cgit v1.2.3-59-g8ed1b From 7274173ba52f6a40f14b2c064b9651ddabf7166d Mon Sep 17 00:00:00 2001 From: Aleks Rozman Date: Sat, 4 Aug 2018 22:15:13 -0700 Subject: Replace magic 0 with enum in surfacecontrol Test: m Fixes: nothing Change-Id: Iab9348dcf57bfb72069655ec9fc7f443d30f3e1b --- libs/gui/SurfaceControl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 19ad31b8c7..3a6dfdada5 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -102,14 +102,14 @@ bool SurfaceControl::isSameSurface( status_t SurfaceControl::clearLayerFrameStats() const { status_t err = validate(); - if (err < 0) return err; + if (err != NO_ERROR) return err; const sp& client(mClient); return client->clearLayerFrameStats(mHandle); } status_t SurfaceControl::getLayerFrameStats(FrameStats* outStats) const { status_t err = validate(); - if (err < 0) return err; + if (err != NO_ERROR) return err; const sp& client(mClient); return client->getLayerFrameStats(mHandle, outStats); } -- cgit v1.2.3-59-g8ed1b From b89ea9d9533864bc6f73a24a4d33d3edba6d1365 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Mon, 10 Dec 2018 13:01:14 -0800 Subject: SurfaceFlinger: Removed createScopedConnection. Scoped connections existed to constrain clients to only making surfaces with parents. However now that we support off-screen parents this is no longer required and we can use normal connections everywhere. We take however care that only priviledged clients can place layers in the current state. Test: Manual Bug: 62536731 Bug: 111373437 Bug: 111297488 Change-Id: I0a034767e92becec63071d7b1e3e71b95d505b77 --- libs/gui/ISurfaceComposer.cpp | 18 ------- libs/gui/SurfaceComposerClient.cpp | 9 +--- libs/gui/SurfaceControl.cpp | 7 +++ libs/gui/include/gui/ISurfaceComposer.h | 16 +----- libs/gui/include/gui/SurfaceComposerClient.h | 2 - libs/gui/include/gui/SurfaceControl.h | 2 + libs/gui/tests/Surface_test.cpp | 4 -- services/surfaceflinger/Client.cpp | 63 +--------------------- services/surfaceflinger/Client.h | 9 ---- services/surfaceflinger/Layer.cpp | 12 ----- services/surfaceflinger/SurfaceFlinger.cpp | 31 ++++------- services/surfaceflinger/SurfaceFlinger.h | 4 +- services/surfaceflinger/tests/Credentials_test.cpp | 20 +------ 13 files changed, 27 insertions(+), 170 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index 2f6ef79af9..a481e81131 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -63,16 +63,6 @@ public: return interface_cast(reply.readStrongBinder()); } - virtual sp createScopedConnection( - const sp& parent) - { - Parcel data, reply; - data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor()); - data.writeStrongBinder(IInterface::asBinder(parent)); - remote()->transact(BnSurfaceComposer::CREATE_SCOPED_CONNECTION, data, &reply); - return interface_cast(reply.readStrongBinder()); - } - virtual void setTransactionState(const Vector& state, const Vector& displays, uint32_t flags, const sp& applyToken, @@ -711,14 +701,6 @@ status_t BnSurfaceComposer::onTransact( reply->writeStrongBinder(b); return NO_ERROR; } - case CREATE_SCOPED_CONNECTION: { - CHECK_INTERFACE(ISurfaceComposer, data, reply); - sp bufferProducer = - interface_cast(data.readStrongBinder()); - sp b = IInterface::asBinder(createScopedConnection(bufferProducer)); - reply->writeStrongBinder(b); - return NO_ERROR; - } case SET_TRANSACTION_STATE: { CHECK_INTERFACE(ISurfaceComposer, data, reply); diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index e043762653..824e43fb07 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -910,11 +910,6 @@ SurfaceComposerClient::SurfaceComposerClient() { } -SurfaceComposerClient::SurfaceComposerClient(const sp& root) - : mStatus(NO_INIT), mParent(root) -{ -} - SurfaceComposerClient::SurfaceComposerClient(const sp& client) : mStatus(NO_ERROR), mClient(client) { @@ -923,10 +918,8 @@ SurfaceComposerClient::SurfaceComposerClient(const sp& c void SurfaceComposerClient::onFirstRef() { sp sf(ComposerService::getComposerService()); if (sf != nullptr && mStatus == NO_INIT) { - auto rootProducer = mParent.promote(); sp conn; - conn = (rootProducer != nullptr) ? sf->createScopedConnection(rootProducer) : - sf->createConnection(); + conn = sf->createConnection(); if (conn != nullptr) { mClient = conn; mStatus = NO_ERROR; diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 3a6dfdada5..b6ef286fd4 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -54,6 +54,13 @@ SurfaceControl::SurfaceControl( { } +SurfaceControl::SurfaceControl(const sp& other) { + mClient = other->mClient; + mHandle = other->mHandle; + mGraphicBufferProducer = other->mGraphicBufferProducer; + mOwned = false; +} + SurfaceControl::~SurfaceControl() { destroy(); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index e0ff410873..1534fca790 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -87,22 +87,11 @@ public: eVsyncSourceSurfaceFlinger = 1 }; - /* create connection with surface flinger, requires - * ACCESS_SURFACE_FLINGER permission + /* + * Create a connection with SurfaceFlinger. */ virtual sp createConnection() = 0; - /** create a scoped connection with surface flinger. - * Surfaces produced with this connection will act - * as children of the passed in GBP. That is to say - * SurfaceFlinger will draw them relative and confined to - * drawing of buffers from the layer associated with parent. - * As this is graphically equivalent in reach to just drawing - * pixels into the parent buffers, it requires no special permission. - */ - virtual sp createScopedConnection( - const sp& parent) = 0; - /* return an IDisplayEventConnection */ virtual sp createDisplayEventConnection( VsyncSource vsyncSource = eVsyncSourceApp) = 0; @@ -356,7 +345,6 @@ public: ENABLE_VSYNC_INJECTIONS, INJECT_VSYNC, GET_LAYER_DEBUG_INFO, - CREATE_SCOPED_CONNECTION, GET_COMPOSITION_PREFERENCE, GET_COLOR_MANAGEMENT, GET_DISPLAYED_CONTENT_SAMPLING_ATTRIBUTES, diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 9765cdd714..4a8e01bec1 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -90,7 +90,6 @@ class SurfaceComposerClient : public RefBase public: SurfaceComposerClient(); SurfaceComposerClient(const sp& client); - SurfaceComposerClient(const sp& parent); virtual ~SurfaceComposerClient(); // Always make sure we could initialize @@ -402,7 +401,6 @@ private: mutable Mutex mLock; status_t mStatus; sp mClient; - wp mParent; }; // --------------------------------------------------------------------------- diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index ccb30fa8e7..9bba76674d 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -75,6 +75,8 @@ public: status_t getLayerFrameStats(FrameStats* outStats) const; sp getClient() const; + + explicit SurfaceControl(const sp& other); private: // can't be copied diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 13fac7fa04..afc30d17b0 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -545,10 +545,6 @@ public: } sp createConnection() override { return nullptr; } - sp createScopedConnection( - const sp& /* parent */) override { - return nullptr; - } sp createDisplayEventConnection(ISurfaceComposer::VsyncSource) override { return nullptr; diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 0b59147c5a..ee4ec506f7 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -35,13 +35,7 @@ const String16 sAccessSurfaceFlinger("android.permission.ACCESS_SURFACE_FLINGER" // --------------------------------------------------------------------------- Client::Client(const sp& flinger) - : Client(flinger, nullptr) -{ -} - -Client::Client(const sp& flinger, const sp& parentLayer) - : mFlinger(flinger), - mParentLayer(parentLayer) + : mFlinger(flinger) { } @@ -65,25 +59,6 @@ Client::~Client() } } -void Client::updateParent(const sp& parentLayer) { - Mutex::Autolock _l(mLock); - - // If we didn't ever have a parent, then we must instead be - // relying on permissions and we never need a parent. - if (mParentLayer != nullptr) { - mParentLayer = parentLayer; - } -} - -sp Client::getParentLayer(bool* outParentDied) const { - Mutex::Autolock _l(mLock); - sp parent = mParentLayer.promote(); - if (outParentDied != nullptr) { - *outParentDied = (mParentLayer != nullptr && parent == nullptr); - } - return parent; -} - status_t Client::initCheck() const { return NO_ERROR; } @@ -119,32 +94,6 @@ sp Client::getLayerUser(const sp& handle) const } -status_t Client::onTransact( - uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) -{ - // these must be checked - IPCThreadState* ipc = IPCThreadState::self(); - const int pid = ipc->getCallingPid(); - const int uid = ipc->getCallingUid(); - const int self_pid = getpid(); - // If we are called from another non root process without the GRAPHICS, SYSTEM, or ROOT - // uid we require the sAccessSurfaceFlinger permission. - // We grant an exception in the case that the Client has a "parent layer", as its - // effects will be scoped to that layer. - if (CC_UNLIKELY(pid != self_pid && uid != AID_GRAPHICS && uid != AID_SYSTEM && uid != 0) - && (getParentLayer() == nullptr)) { - // we're called from a different process, do the real check - if (!PermissionCache::checkCallingPermission(sAccessSurfaceFlinger)) - { - ALOGE("Permission Denial: " - "can't openGlobalTransaction pid=%d, uid<=%d", pid, uid); - return PERMISSION_DENIED; - } - } - return BnSurfaceComposerClient::onTransact(code, data, reply, flags); -} - - status_t Client::createSurface( const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, @@ -160,16 +109,8 @@ status_t Client::createSurface( return NAME_NOT_FOUND; } } - if (parent == nullptr) { - bool parentDied; - parent = getParentLayer(&parentDied); - // If we had a parent, but it died, we've lost all - // our capabilities. - if (parentDied) { - return NAME_NOT_FOUND; - } - } + // We rely on createLayer to check permissions. return mFlinger->createLayer(name, this, w, h, format, flags, windowType, ownerUid, handle, gbp, &parent); } diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index 49437ed7fd..4a74739a10 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -39,7 +39,6 @@ class Client : public BnSurfaceComposerClient { public: explicit Client(const sp& flinger); - Client(const sp& flinger, const sp& parentLayer); ~Client(); status_t initCheck() const; @@ -51,8 +50,6 @@ public: sp getLayerUser(const sp& handle) const; - void updateParent(const sp& parentLayer); - private: // ISurfaceComposerClient interface virtual status_t createSurface( @@ -68,17 +65,11 @@ private: virtual status_t getLayerFrameStats(const sp& handle, FrameStats* outStats) const; - virtual status_t onTransact( - uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags); - - sp getParentLayer(bool* outParentDied = nullptr) const; - // constant sp mFlinger; // protected by mLock DefaultKeyedVector< wp, wp > mLayers; - wp mParentLayer; // thread-safe mutable Mutex mLock; diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index f4b3cddc63..2d3fd8ecbb 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -1666,11 +1666,6 @@ bool Layer::reparentChildren(const sp& newParentHandle) { } for (const sp& child : mCurrentChildren) { newParent->addChild(child); - - sp client(child->mClientRef.promote()); - if (client != nullptr) { - client->updateParent(newParent); - } } mCurrentChildren.clear(); @@ -1705,13 +1700,6 @@ bool Layer::reparent(const sp& newParentHandle) { addToCurrentState(); } - sp client(mClientRef.promote()); - sp newParentClient(newParent->mClientRef.promote()); - - if (client != newParentClient) { - client->updateParent(newParent); - } - Mutex::Autolock lock(mStateMutex); if (mLayerDetached) { mLayerDetached = false; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 4d685590cb..13997bea21 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -458,19 +458,6 @@ sp SurfaceFlinger::createConnection() { return initClient(new Client(this)); } -sp SurfaceFlinger::createScopedConnection( - const sp& gbp) { - if (authenticateSurfaceTexture(gbp) == false) { - return nullptr; - } - const auto& layer = (static_cast(gbp.get()))->getLayer(); - if (layer == nullptr) { - return nullptr; - } - - return initClient(new Client(this, layer)); -} - sp SurfaceFlinger::createDisplay(const String8& displayName, bool secure) { @@ -3243,7 +3230,8 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const sp& handle, const sp& gbc, const sp& lbc, - const sp& parent) + const sp& parent, + bool addToCurrentState) { // add this layer to the current state list { @@ -3253,13 +3241,12 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, MAX_LAYERS); return NO_MEMORY; } - if (parent == nullptr) { + if (parent == nullptr && addToCurrentState) { mCurrentState.layersSortedByZ.add(lbc); - } else { - if (parent->isRemovedFromCurrentState()) { + } else if (parent == nullptr || parent->isRemovedFromCurrentState()) { ALOGE("addClientLayer called with a removed parent"); lbc->onRemovedFromCurrentState(); - } + } else { parent->addChild(lbc); } @@ -3864,7 +3851,9 @@ status_t SurfaceFlinger::createLayer( layer->setInfo(windowType, ownerUid); - result = addClientLayer(client, *handle, *gbp, layer, *parent); + bool addToCurrentState = callingThreadHasUnscopedSurfaceFlingerAccess(); + result = addClientLayer(client, *handle, *gbp, layer, *parent, + addToCurrentState); if (result != NO_ERROR) { return result; } @@ -4828,7 +4817,6 @@ status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) { // access to SF. case BOOT_FINISHED: case CLEAR_ANIMATION_FRAME_STATS: - case CREATE_CONNECTION: case CREATE_DISPLAY: case DESTROY_DISPLAY: case ENABLE_VSYNC_INJECTIONS: @@ -4875,8 +4863,7 @@ status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) { // Calling setTransactionState is safe, because you need to have been // granted a reference to Client* and Handle* to do anything with it. case SET_TRANSACTION_STATE: - // Creating a scoped connection is safe, as per discussion in ISurfaceComposer.h - case CREATE_SCOPED_CONNECTION: + case CREATE_CONNECTION: case GET_COLOR_MANAGEMENT: case GET_COMPOSITION_PREFERENCE: { return OK; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index c37f159f7b..bfc87a02eb 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -437,7 +437,6 @@ private: * ISurfaceComposer interface */ virtual sp createConnection(); - virtual sp createScopedConnection(const sp& gbp); virtual sp createDisplay(const String8& displayName, bool secure); virtual void destroyDisplay(const sp& displayToken); virtual sp getBuiltInDisplay(int32_t id); @@ -620,7 +619,8 @@ private: const sp& handle, const sp& gbc, const sp& lbc, - const sp& parent); + const sp& parent, + bool addToCurrentState); /* ------------------------------------------------------------------------ * Boot animation, on/off animations and screen capture diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index a73ec6c7ef..8560f5ed3e 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -162,10 +162,10 @@ TEST_F(CredentialsTest, ClientInitTest) { setSystemUID(); ASSERT_NO_FATAL_FAILURE(initClient()); - // No one else can init the client. + // Anyone else can init the client. setBinUID(); mComposerClient = new SurfaceComposerClient; - ASSERT_EQ(NO_INIT, mComposerClient->initCheck()); + ASSERT_NO_FATAL_FAILURE(initClient()); } TEST_F(CredentialsTest, GetBuiltInDisplayAccessTest) { @@ -222,22 +222,6 @@ TEST_F(CredentialsTest, SetActiveColorModeTest) { ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, NO_ERROR, PERMISSION_DENIED)); } -TEST_F(CredentialsTest, CreateSurfaceTest) { - sp display(SurfaceComposerClient::getBuiltInDisplay(ISurfaceComposer::eDisplayIdMain)); - DisplayInfo info; - SurfaceComposerClient::getDisplayInfo(display, &info); - const ssize_t displayWidth = info.w; - const ssize_t displayHeight = info.h; - - std::function condition = [=]() { - mBGSurfaceControl = - mComposerClient->createSurface(SURFACE_NAME, displayWidth, displayHeight, - PIXEL_FORMAT_RGBA_8888, 0); - return mBGSurfaceControl != nullptr && mBGSurfaceControl->isValid(); - }; - ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false)); -} - TEST_F(CredentialsTest, CreateDisplayTest) { std::function condition = [=]() { sp testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true); -- cgit v1.2.3-59-g8ed1b From 6fb1a7e9627df030667304d88a3292f91e790ea9 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 11 Dec 2018 12:07:25 -0800 Subject: SurfaceFlinger: Remove removeLayer We remove explicit layer destruction and replace it with reparent->null, completing the transition to a reference counted model. Test: Manual Bug: 62536731 Bug: 111373437 Bug: 111297488 Change-Id: I8ac7c5c5125e1c8daf84b42db00e1dd93a544bb5 --- cmds/surfacereplayer/replayer/Replayer.cpp | 41 ------------ cmds/surfacereplayer/replayer/Replayer.h | 3 - libs/gui/ISurfaceComposerClient.cpp | 8 --- libs/gui/SurfaceComposerClient.cpp | 34 +++++----- libs/gui/SurfaceControl.cpp | 10 ++- libs/gui/include/gui/ISurfaceComposerClient.h | 5 -- libs/gui/include/gui/SurfaceComposerClient.h | 11 ++-- services/surfaceflinger/Client.cpp | 24 ------- services/surfaceflinger/Client.h | 5 +- services/surfaceflinger/Layer.cpp | 49 +++++++++----- services/surfaceflinger/SurfaceFlinger.cpp | 77 ++++------------------ services/surfaceflinger/SurfaceFlinger.h | 12 +--- services/surfaceflinger/tests/Stress_test.cpp | 2 +- .../tests/SurfaceInterceptor_test.cpp | 12 ---- services/surfaceflinger/tests/Transaction_test.cpp | 8 +-- 15 files changed, 82 insertions(+), 219 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/cmds/surfacereplayer/replayer/Replayer.cpp b/cmds/surfacereplayer/replayer/Replayer.cpp index 384f21ff17..34886a99e9 100644 --- a/cmds/surfacereplayer/replayer/Replayer.cpp +++ b/cmds/surfacereplayer/replayer/Replayer.cpp @@ -286,10 +286,6 @@ status_t Replayer::dispatchEvent(int index) { std::thread(&Replayer::createSurfaceControl, this, increment.surface_creation(), event) .detach(); } break; - case increment.kSurfaceDeletion: { - std::thread(&Replayer::deleteSurfaceControl, this, increment.surface_deletion(), event) - .detach(); - } break; case increment.kBufferUpdate: { std::lock_guard lock1(mLayerLock); std::lock_guard lock2(mBufferQueueSchedulerLock); @@ -628,47 +624,10 @@ status_t Replayer::createSurfaceControl( return NO_ERROR; } -status_t Replayer::deleteSurfaceControl( - const SurfaceDeletion& delete_, const std::shared_ptr& event) { - ALOGV("Deleting %d Surface Control", delete_.id()); - event->readyToExecute(); - - std::lock_guard lock1(mPendingLayersLock); - - mLayersPendingRemoval.push_back(delete_.id()); - - const auto& iterator = mBufferQueueSchedulers.find(delete_.id()); - if (iterator != mBufferQueueSchedulers.end()) { - (*iterator).second->stopScheduling(); - } - - std::lock_guard lock2(mLayerLock); - if (mLayers[delete_.id()] != nullptr) { - mComposerClient->destroySurface(mLayers[delete_.id()]->getHandle()); - } - - return NO_ERROR; -} - -void Replayer::doDeleteSurfaceControls() { - std::lock_guard lock1(mPendingLayersLock); - std::lock_guard lock2(mLayerLock); - if (!mLayersPendingRemoval.empty()) { - for (int id : mLayersPendingRemoval) { - mLayers.erase(id); - mColors.erase(id); - mBufferQueueSchedulers.erase(id); - } - mLayersPendingRemoval.clear(); - } -} - status_t Replayer::injectVSyncEvent( const VSyncEvent& vSyncEvent, const std::shared_ptr& event) { ALOGV("Injecting VSync Event"); - doDeleteSurfaceControls(); - event->readyToExecute(); SurfaceComposerClient::injectVSync(vSyncEvent.when()); diff --git a/cmds/surfacereplayer/replayer/Replayer.h b/cmds/surfacereplayer/replayer/Replayer.h index 120dd9babd..ad807ee950 100644 --- a/cmds/surfacereplayer/replayer/Replayer.h +++ b/cmds/surfacereplayer/replayer/Replayer.h @@ -70,8 +70,6 @@ class Replayer { status_t doTransaction(const Transaction& transaction, const std::shared_ptr& event); status_t createSurfaceControl(const SurfaceCreation& create, const std::shared_ptr& event); - status_t deleteSurfaceControl(const SurfaceDeletion& delete_, - const std::shared_ptr& event); status_t injectVSyncEvent(const VSyncEvent& vsyncEvent, const std::shared_ptr& event); void createDisplay(const DisplayCreation& create, const std::shared_ptr& event); void deleteDisplay(const DisplayDeletion& delete_, const std::shared_ptr& event); @@ -120,7 +118,6 @@ class Replayer { void setDisplayProjection(SurfaceComposerClient::Transaction& t, display_id id, const ProjectionChange& pc); - void doDeleteSurfaceControls(); void waitUntilTimestamp(int64_t timestamp); void waitUntilDeferredTransactionLayerExists( const DeferredTransactionChange& dtc, std::unique_lock& lock); diff --git a/libs/gui/ISurfaceComposerClient.cpp b/libs/gui/ISurfaceComposerClient.cpp index a6890eeb19..369f523ea4 100644 --- a/libs/gui/ISurfaceComposerClient.cpp +++ b/libs/gui/ISurfaceComposerClient.cpp @@ -31,7 +31,6 @@ namespace { // Anonymous enum class Tag : uint32_t { CREATE_SURFACE = IBinder::FIRST_CALL_TRANSACTION, - DESTROY_SURFACE, CLEAR_LAYER_FRAME_STATS, GET_LAYER_FRAME_STATS, LAST = GET_LAYER_FRAME_STATS, @@ -57,11 +56,6 @@ public: handle, gbp); } - status_t destroySurface(const sp& handle) override { - return callRemote(Tag::DESTROY_SURFACE, - handle); - } - status_t clearLayerFrameStats(const sp& handle) const override { return callRemote(Tag::CLEAR_LAYER_FRAME_STATS, @@ -92,8 +86,6 @@ status_t BnSurfaceComposerClient::onTransact(uint32_t code, const Parcel& data, switch (tag) { case Tag::CREATE_SURFACE: return callLocal(data, reply, &ISurfaceComposerClient::createSurface); - case Tag::DESTROY_SURFACE: - return callLocal(data, reply, &ISurfaceComposerClient::destroySurface); case Tag::CLEAR_LAYER_FRAME_STATS: return callLocal(data, reply, &ISurfaceComposerClient::clearLayerFrameStats); case Tag::GET_LAYER_FRAME_STATS: diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 824e43fb07..6c39d6f1b8 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -205,6 +205,22 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Tr return *this; } +void SurfaceComposerClient::doDropReferenceTransaction(const sp& handle, + const sp& client) { + sp sf(ComposerService::getComposerService()); + Vector composerStates; + Vector displayStates; + + ComposerState s; + s.client = client; + s.state.surface = handle; + s.state.what |= layer_state_t::eReparent; + s.state.parentHandleForChild = nullptr; + + composerStates.add(s); + sf->setTransactionState(composerStates, displayStates, 0, nullptr, {}); +} + status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { if (mStatus != NO_ERROR) { return mStatus; @@ -819,17 +835,6 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::transfer #endif -SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::destroySurface( - const sp& sc) { - layer_state_t* s = getLayerState(sc); - if (!s) { - mStatus = BAD_INDEX; - return *this; - } - s->what |= layer_state_t::eDestroySurface; - return *this; -} - SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setColorTransform( const sp& sc, const mat3& matrix, const vec3& translation) { layer_state_t* s = getLayerState(sc); @@ -1004,13 +1009,6 @@ status_t SurfaceComposerClient::createSurfaceChecked( return err; } -status_t SurfaceComposerClient::destroySurface(const sp& sid) { - if (mStatus != NO_ERROR) - return mStatus; - status_t err = mClient->destroySurface(sid); - return err; -} - status_t SurfaceComposerClient::clearLayerFrameStats(const sp& token) const { if (mStatus != NO_ERROR) { return mStatus; diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index b6ef286fd4..8e500a4f9b 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -63,7 +63,13 @@ SurfaceControl::SurfaceControl(const sp& other) { SurfaceControl::~SurfaceControl() { - destroy(); + if (mClient != nullptr && mHandle != nullptr && mOwned) { + SurfaceComposerClient::doDropReferenceTransaction(mHandle, mClient->getClient()); + } + mClient.clear(); + mHandle.clear(); + mGraphicBufferProducer.clear(); + IPCThreadState::self()->flushCommands(); } void SurfaceControl::destroy() @@ -71,7 +77,7 @@ void SurfaceControl::destroy() // Avoid destroying the server-side surface if we are not the owner of it, meaning that we // retrieved it from another process. if (isValid() && mOwned) { - mClient->destroySurface(mHandle); + SurfaceComposerClient::Transaction().reparent(this, nullptr).apply(); } // clear all references and trigger an IPC now, to make sure things // happen without delay, since these resources are quite heavy. diff --git a/libs/gui/include/gui/ISurfaceComposerClient.h b/libs/gui/include/gui/ISurfaceComposerClient.h index 82b01b8cf0..56ca197ff9 100644 --- a/libs/gui/include/gui/ISurfaceComposerClient.h +++ b/libs/gui/include/gui/ISurfaceComposerClient.h @@ -55,11 +55,6 @@ public: int32_t ownerUid, sp* handle, sp* gbp) = 0; - /* - * Requires ACCESS_SURFACE_FLINGER permission - */ - virtual status_t destroySurface(const sp& handle) = 0; - /* * Requires ACCESS_SURFACE_FLINGER permission */ diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 4a8e01bec1..e0cbb705c7 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -145,6 +145,13 @@ public: ui::Dataspace* wideColorGamutDataspace, ui::PixelFormat* wideColorGamutPixelFormat); + /** + * Called from SurfaceControl d'tor to 'destroy' the surface (or rather, reparent it + * to null), but without needing an sp to avoid infinite ressurection. + */ + static void doDropReferenceTransaction(const sp& handle, + const sp& client); + // ------------------------------------------------------------------------ // surface creation / destruction @@ -338,8 +345,6 @@ public: Transaction& transferTouchFocus(const sp& fromToken, const sp& toToken); #endif - Transaction& destroySurface(const sp& sc); - // Set a color transform matrix on the given layer on the built-in display. Transaction& setColorTransform(const sp& sc, const mat3& matrix, const vec3& translation); @@ -368,8 +373,6 @@ public: void setEarlyWakeup(); }; - status_t destroySurface(const sp& id); - status_t clearLayerFrameStats(const sp& token) const; status_t getLayerFrameStats(const sp& token, FrameStats* outStats) const; static status_t clearAnimationFrameStats(); diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index ee4ec506f7..4f6fb1cca6 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -39,26 +39,6 @@ Client::Client(const sp& flinger) { } -Client::~Client() -{ - // We need to post a message to remove our remaining layers rather than - // do so directly by acquiring the SurfaceFlinger lock. If we were to - // attempt to directly call the lock it becomes effectively impossible - // to use sp while holding the SF lock as descoping it could - // then trigger a dead-lock. - - const size_t count = mLayers.size(); - for (size_t i=0 ; i l = mLayers.valueAt(i).promote(); - if (l == nullptr) { - continue; - } - mFlinger->postMessageAsync(new LambdaMessage([flinger = mFlinger, l]() { - flinger->removeLayer(l); - })); - } -} - status_t Client::initCheck() const { return NO_ERROR; } @@ -115,10 +95,6 @@ status_t Client::createSurface( ownerUid, handle, gbp, &parent); } -status_t Client::destroySurface(const sp& handle) { - return mFlinger->onLayerRemoved(this, handle); -} - status_t Client::clearLayerFrameStats(const sp& handle) const { sp layer = getLayerUser(handle); if (layer == nullptr) { diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index 4a74739a10..d0051dee1a 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -39,13 +39,12 @@ class Client : public BnSurfaceComposerClient { public: explicit Client(const sp& flinger); - ~Client(); + ~Client() = default; status_t initCheck() const; // protected by SurfaceFlinger::mStateLock void attachLayer(const sp& handle, const sp& layer); - void detachLayer(const Layer* layer); sp getLayerUser(const sp& handle) const; @@ -59,8 +58,6 @@ private: sp* handle, sp* gbp); - virtual status_t destroySurface(const sp& handle); - virtual status_t clearLayerFrameStats(const sp& handle) const; virtual status_t getLayerFrameStats(const sp& handle, FrameStats* outStats) const; diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index b1827c191c..3f2d10a45b 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -179,6 +179,8 @@ void Layer::onRemovedFromCurrentState() { for (const auto& child : mCurrentChildren) { child->onRemovedFromCurrentState(); } + + mFlinger->markLayerPendingRemovalLocked(this); } void Layer::addToCurrentState() { @@ -1571,6 +1573,7 @@ void Layer::addChild(const sp& layer) { ssize_t Layer::removeChild(const sp& layer) { layer->setParent(nullptr); + return mCurrentChildren.remove(layer); } @@ -1605,14 +1608,14 @@ void Layer::setChildrenDrawingParent(const sp& newParent) { } bool Layer::reparent(const sp& newParentHandle) { - if (newParentHandle == nullptr) { - return false; - } - - auto handle = static_cast(newParentHandle.get()); - sp newParent = handle->owner.promote(); - if (newParent == nullptr) { - ALOGE("Unable to promote Layer handle"); + bool callSetTransactionFlags = false; + + // While layers are detached, we allow most operations + // and simply halt performing the actual transaction. However + // for reparent != null we would enter the mRemovedFromCurrentState + // state, regardless of whether doTransaction was called, and + // so we need to prevent the update here. + if (mLayerDetached && newParentHandle == nullptr) { return false; } @@ -1620,17 +1623,31 @@ bool Layer::reparent(const sp& newParentHandle) { if (parent != nullptr) { parent->removeChild(this); } - newParent->addChild(this); - if (!newParent->isRemovedFromCurrentState()) { - addToCurrentState(); - } + if (newParentHandle != nullptr) { + auto handle = static_cast(newParentHandle.get()); + sp newParent = handle->owner.promote(); + if (newParent == nullptr) { + ALOGE("Unable to promote Layer handle"); + return false; + } - if (mLayerDetached) { - mLayerDetached = false; - setTransactionFlags(eTransactionNeeded); + newParent->addChild(this); + if (!newParent->isRemovedFromCurrentState()) { + addToCurrentState(); + } else { + onRemovedFromCurrentState(); + } + + if (mLayerDetached) { + mLayerDetached = false; + callSetTransactionFlags = true; + } + } else { + onRemovedFromCurrentState(); } - if (attachChildren()) { + + if (callSetTransactionFlags || attachChildren()) { setTransactionFlags(eTransactionNeeded); } return true; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fd25abf1a5..3648be49ec 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2799,10 +2799,8 @@ void SurfaceFlinger::commitTransaction() // showing at its last configured state until we eventually // abandon the buffer queue. if (l->isRemovedFromCurrentState()) { - l->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* child) { - child->destroyHwcLayersForAllDisplays(); - latchAndReleaseBuffer(child); - }); + l->destroyHwcLayersForAllDisplays(); + latchAndReleaseBuffer(l); } } mLayersPendingRemoval.clear(); @@ -3276,30 +3274,6 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, return NO_ERROR; } -status_t SurfaceFlinger::removeLayer(const sp& layer) { - Mutex::Autolock _l(mStateLock); - return removeLayerLocked(mStateLock, layer); -} - -status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp& layer) { - if (layer->isLayerDetached()) { - return NO_ERROR; - } - - const auto& p = layer->getParent(); - ssize_t index; - if (p != nullptr) { - index = p->removeChild(layer); - } else { - index = mCurrentState.layersSortedByZ.remove(layer); - } - - layer->onRemovedFromCurrentState(); - - markLayerPendingRemovalLocked(lock, layer); - return NO_ERROR; -} - uint32_t SurfaceFlinger::peekTransactionFlags() { return mTransactionFlags; } @@ -3434,14 +3408,6 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, } transactionFlags |= clientStateFlags; - // Iterate through all layers again to determine if any need to be destroyed. Marking layers - // as destroyed should only occur after setting all other states. This is to allow for a - // child re-parent to happen before marking its original parent as destroyed (which would - // then mark the child as destroyed). - for (const ComposerState& state : states) { - setDestroyStateLocked(state); - } - transactionFlags |= addInputWindowCommands(inputWindowCommands); // If a synchronous transaction is explicitly requested without any changes, force a transaction @@ -3767,20 +3733,6 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState return flags; } -void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) { - const layer_state_t& state = composerState.state; - sp client(static_cast(composerState.client.get())); - - sp layer(client->getLayerUser(state.surface)); - if (layer == nullptr) { - return; - } - - if (state.what & layer_state_t::eDestroySurface) { - removeLayerLocked(mStateLock, layer); - } -} - uint32_t SurfaceFlinger::addInputWindowCommands(const InputWindowCommands& inputWindowCommands) { uint32_t flags = 0; if (!inputWindowCommands.transferTouchFocusCommands.empty()) { @@ -3960,21 +3912,7 @@ status_t SurfaceFlinger::createContainerLayer(const sp& client, } -status_t SurfaceFlinger::onLayerRemoved(const sp& client, const sp& handle) -{ - // called by a client when it wants to remove a Layer - status_t err = NO_ERROR; - sp l(client->getLayerUser(handle)); - if (l != nullptr) { - mInterceptor->saveSurfaceDeletion(l); - err = removeLayer(l); - ALOGE_IF(err<0 && err != NAME_NOT_FOUND, - "error removing layer=%p (%s)", l.get(), strerror(-err)); - } - return err; -} - -void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp& layer) { +void SurfaceFlinger::markLayerPendingRemovalLocked(const sp& layer) { mLayersPendingRemoval.add(layer); mLayersRemoved = true; setTransactionFlags(eTransactionNeeded); @@ -3983,7 +3921,14 @@ void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp void SurfaceFlinger::onHandleDestroyed(sp& layer) { Mutex::Autolock lock(mStateLock); - markLayerPendingRemovalLocked(mStateLock, layer); + // If a layer has a parent, we allow it to out-live it's handle + // with the idea that the parent holds a reference and will eventually + // be cleaned up. However no one cleans up the top-level so we do so + // here. + if (layer->getParent() == nullptr) { + mCurrentState.layersSortedByZ.remove(layer); + } + markLayerPendingRemovalLocked(layer); layer.clear(); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index bfc87a02eb..b8bfcf1f9e 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -569,7 +569,6 @@ private: bool composerStateContainsUnsignaledFences(const Vector& states); uint32_t setClientStateLocked(const ComposerState& composerState); uint32_t setDisplayStateLocked(const DisplayState& s); - void setDestroyStateLocked(const ComposerState& composerState); uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands); /* ------------------------------------------------------------------------ @@ -599,20 +598,11 @@ private: String8 getUniqueLayerName(const String8& name); - // called in response to the window-manager calling - // ISurfaceComposerClient::destroySurface() - status_t onLayerRemoved(const sp& client, const sp& handle); - - void markLayerPendingRemovalLocked(const Mutex& /* mStateLock */, const sp& layer); - // called when all clients have released all their references to // this layer meaning it is entirely safe to destroy all // resources associated to this layer. void onHandleDestroyed(sp& layer); - - // remove a layer from SurfaceFlinger immediately - status_t removeLayer(const sp& layer); - status_t removeLayerLocked(const Mutex&, const sp& layer); + void markLayerPendingRemovalLocked(const sp& layer); // add a layer to SurfaceFlinger status_t addClientLayer(const sp& client, diff --git a/services/surfaceflinger/tests/Stress_test.cpp b/services/surfaceflinger/tests/Stress_test.cpp index 3e1be8e288..89c26f4a98 100644 --- a/services/surfaceflinger/tests/Stress_test.cpp +++ b/services/surfaceflinger/tests/Stress_test.cpp @@ -34,7 +34,7 @@ TEST(SurfaceFlingerStress, create_and_destroy) { auto surf = client->createSurface(String8("t"), 100, 100, PIXEL_FORMAT_RGBA_8888, 0); ASSERT_TRUE(surf != nullptr); - client->destroySurface(surf->getHandle()); + surf->clear(); } }; diff --git a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp index e506757867..8ec3e154e3 100644 --- a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp +++ b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp @@ -806,18 +806,6 @@ TEST_F(SurfaceInterceptorTest, InterceptSurfaceCreationWorks) { Increment::IncrementCase::kSurfaceCreation); } -TEST_F(SurfaceInterceptorTest, InterceptSurfaceDeletionWorks) { - enableInterceptor(); - sp layerToDelete = mComposerClient->createSurface(String8(LAYER_NAME), - SIZE_UPDATE, SIZE_UPDATE, PIXEL_FORMAT_RGBA_8888, 0); - mComposerClient->destroySurface(layerToDelete->getHandle()); - disableInterceptor(); - - Trace capturedTrace; - ASSERT_EQ(NO_ERROR, readProtoFile(&capturedTrace)); - ASSERT_TRUE(singleIncrementFound(capturedTrace, Increment::IncrementCase::kSurfaceDeletion)); -} - TEST_F(SurfaceInterceptorTest, InterceptDisplayCreationWorks) { captureTest(&SurfaceInterceptorTest::displayCreation, Increment::IncrementCase::kDisplayCreation); diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index d118ad6402..991ea36a3c 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -1023,7 +1023,7 @@ TEST_P(LayerTypeTransactionTest, SetRelativeZBug64572777) { .setRelativeLayer(layerG, layerR->getHandle(), 1) .apply(); - mClient->destroySurface(layerG->getHandle()); + layerG->clear(); // layerG should have been removed screenshot()->expectColor(Rect(0, 0, 32, 32), Color::RED); } @@ -4030,9 +4030,9 @@ TEST_F(ChildLayerTest, ReparentToNoParent) { asTransaction([&](Transaction& t) { t.reparent(mChild, nullptr); }); { mCapture = screenshot(); - // Nothing should have changed. + // The surface should now be offscreen. mCapture->expectFGColor(64, 64); - mCapture->expectChildColor(74, 74); + mCapture->expectFGColor(74, 74); mCapture->expectFGColor(84, 84); } } @@ -4610,7 +4610,7 @@ TEST_F(ScreenCaptureTest, CaptureInvalidLayer) { ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(redLayer, Color::RED, 60, 60)); auto redLayerHandle = redLayer->getHandle(); - mClient->destroySurface(redLayerHandle); + redLayer->clear(); SurfaceComposerClient::Transaction().apply(true); sp outBuffer; -- cgit v1.2.3-59-g8ed1b From a35ef9f5ecec00349bfeb69aedbd21f3342ed140 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Fri, 25 Jan 2019 14:29:21 -0800 Subject: Expose IGBP getter Useful for comparing a SurfaceControl to an existing Surface, but without calling getSurface which may cause the allocation of an as of yet unexisting Surface wrapper. Bug: 122588130 Test: None Change-Id: I5742bf6af06bd48013a2418b583de8eeeecba2df --- libs/gui/SurfaceControl.cpp | 6 ++++++ libs/gui/include/gui/SurfaceControl.h | 2 ++ 2 files changed, 8 insertions(+) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 8e500a4f9b..008f520bb0 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -177,6 +177,12 @@ sp SurfaceControl::getHandle() const return mHandle; } +sp SurfaceControl::getIGraphicBufferProducer() const +{ + Mutex::Autolock _l(mLock); + return mGraphicBufferProducer; +} + sp SurfaceControl::getClient() const { return mClient; diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index 9bba76674d..b584f36e7a 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -71,6 +71,8 @@ public: sp createSurface() const; sp getHandle() const; + sp getIGraphicBufferProducer() const; + status_t clearLayerFrameStats() const; status_t getLayerFrameStats(FrameStats* outStats) const; -- cgit v1.2.3-59-g8ed1b From 8b6b32182d1c4f37e00b2c539f2f81ef2c846e63 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 29 Jan 2019 15:27:55 -0800 Subject: SurfaceControl: Allow calling destroy when mOwned is true mOwned was just put in place to prevent the finalizer in SysUI from destroying leashes, it was never meant to prevent explicit calls to destroy. On another note we should probably remove destroy or call it "release" but we can come back to that. Change-Id: I72b0ab467105dc63094994de5482cec651fffaa3 Fixes: 113820778 Test: Builds --- libs/gui/SurfaceControl.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 008f520bb0..f06d36aea3 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -63,6 +63,8 @@ SurfaceControl::SurfaceControl(const sp& other) { SurfaceControl::~SurfaceControl() { + // Avoid reparenting the server-side surface to null if we are not the owner of it, + // meaning that we retrieved it from another process. if (mClient != nullptr && mHandle != nullptr && mOwned) { SurfaceComposerClient::doDropReferenceTransaction(mHandle, mClient->getClient()); } @@ -74,9 +76,7 @@ SurfaceControl::~SurfaceControl() void SurfaceControl::destroy() { - // Avoid destroying the server-side surface if we are not the owner of it, meaning that we - // retrieved it from another process. - if (isValid() && mOwned) { + if (isValid()) { SurfaceComposerClient::Transaction().reparent(this, nullptr).apply(); } // clear all references and trigger an IPC now, to make sure things -- cgit v1.2.3-59-g8ed1b From 8724653a77f855b7e062541d6e08f3ea659cef7c Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Mon, 4 Feb 2019 15:20:26 -0800 Subject: SurfaceControl life-cycle refactoring. We provide a method called "release" which allows the client to drop its references without reparenting the SurfaceControl to null as the destructor would. The Java and NDK API's already have this method but it's not perfectly functional at the moment. Additionally we remove the strange pass-through of clear to destroy and expose destroy directly. Test: Builds Bug: 123587983 Change-Id: Ia89ada1476daef97e6f30d50a57065c3636a6489 --- libs/gui/SurfaceControl.cpp | 23 +++++++--------------- libs/gui/include/gui/SurfaceControl.h | 9 ++++++--- services/surfaceflinger/tests/Stress_test.cpp | 2 +- services/surfaceflinger/tests/Transaction_test.cpp | 6 +++--- 4 files changed, 17 insertions(+), 23 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 008f520bb0..d2b0e6a220 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -66,10 +66,7 @@ SurfaceControl::~SurfaceControl() if (mClient != nullptr && mHandle != nullptr && mOwned) { SurfaceComposerClient::doDropReferenceTransaction(mHandle, mClient->getClient()); } - mClient.clear(); - mHandle.clear(); - mGraphicBufferProducer.clear(); - IPCThreadState::self()->flushCommands(); + release(); } void SurfaceControl::destroy() @@ -79,7 +76,12 @@ void SurfaceControl::destroy() if (isValid() && mOwned) { SurfaceComposerClient::Transaction().reparent(this, nullptr).apply(); } - // clear all references and trigger an IPC now, to make sure things + release(); +} + +void SurfaceControl::release() +{ + // Trigger an IPC now, to make sure things // happen without delay, since these resources are quite heavy. mClient.clear(); mHandle.clear(); @@ -87,17 +89,6 @@ void SurfaceControl::destroy() IPCThreadState::self()->flushCommands(); } -void SurfaceControl::clear() -{ - // here, the window manager tells us explicitly that we should destroy - // the surface's resource. Soon after this call, it will also release - // its last reference (which will call the dtor); however, it is possible - // that a client living in the same process still holds references which - // would delay the call to the dtor -- that is why we need this explicit - // "clear()" call. - destroy(); -} - void SurfaceControl::disconnect() { if (mGraphicBufferProducer != nullptr) { mGraphicBufferProducer->disconnect( diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index b584f36e7a..55efcbfb3a 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -58,8 +58,12 @@ public: static bool isSameSurface( const sp& lhs, const sp& rhs); - // release surface data from java - void clear(); + // Release the handles assosciated with the SurfaceControl, without reparenting + // them off-screen. At the moment if this isn't executed before ~SurfaceControl + // is called then the destructor will reparent the layer off-screen for you. + void release(); + // Reparent off-screen and release. This is invoked by the destructor. + void destroy(); // disconnect any api that's connected void disconnect(); @@ -98,7 +102,6 @@ private: sp generateSurfaceLocked() const; status_t validate() const; - void destroy(); sp mClient; sp mHandle; diff --git a/services/surfaceflinger/tests/Stress_test.cpp b/services/surfaceflinger/tests/Stress_test.cpp index 89c26f4a98..ee857b01f2 100644 --- a/services/surfaceflinger/tests/Stress_test.cpp +++ b/services/surfaceflinger/tests/Stress_test.cpp @@ -34,7 +34,7 @@ TEST(SurfaceFlingerStress, create_and_destroy) { auto surf = client->createSurface(String8("t"), 100, 100, PIXEL_FORMAT_RGBA_8888, 0); ASSERT_TRUE(surf != nullptr); - surf->clear(); + surf.clear(); } }; diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index 4f0ded3517..50a2e602d7 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -1137,7 +1137,7 @@ TEST_P(LayerTypeAndRenderTypeTransactionTest, SetRelativeZBug64572777) { .setRelativeLayer(layerG, layerR->getHandle(), 1) .apply(); - layerG->clear(); + layerG.clear(); // layerG should have been removed getScreenCapture()->expectColor(Rect(0, 0, 32, 32), Color::RED); } @@ -4360,7 +4360,7 @@ TEST_F(ChildLayerTest, ChildrenSurviveParentDestruction) { mCapture->checkPixel(64, 64, 111, 111, 111); } - mChild->clear(); + mChild.clear(); { SCOPED_TRACE("After destroying child"); @@ -5232,7 +5232,7 @@ TEST_F(ScreenCaptureTest, CaptureInvalidLayer) { ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(redLayer, Color::RED, 60, 60)); auto redLayerHandle = redLayer->getHandle(); - redLayer->clear(); + redLayer.clear(); SurfaceComposerClient::Transaction().apply(true); sp outBuffer; -- cgit v1.2.3-59-g8ed1b