From e8097ca7c5e00ed61d7f604d8eda8d2fe2d2fa9b Mon Sep 17 00:00:00 2001 From: chaviw Date: Fri, 7 Jun 2019 17:08:53 -0700 Subject: Remove SurfaceControl.destroy function The destroy function was only used temporarily instead of all call points calling reparent(null) explicitly. Removing the Java destroy request so no need for the SurfaceControl.destroy method anymore. Test: SurfaceControlTest Change-Id: If69e030f3babf83a6382f85a26f0bb1eb451dc23 --- libs/gui/SurfaceControl.cpp | 8 -------- 1 file changed, 8 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 55488dad0b..011854edbb 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -71,14 +71,6 @@ SurfaceControl::~SurfaceControl() release(); } -void SurfaceControl::destroy() -{ - if (isValid()) { - SurfaceComposerClient::Transaction().reparent(this, nullptr).apply(); - } - release(); -} - void SurfaceControl::release() { // Trigger an IPC now, to make sure things -- cgit v1.2.3-59-g8ed1b From 621102e5d2c97bd00404817191fa5012749a7a0f Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 12 Jun 2019 14:16:57 -0700 Subject: Make SurfaceControl Transaction parcelable 2/2 Allow clients to send SurfaceControl Transactions across processes to enable more advanced synchronization use cases. Bug: 132205507 Test: atest SurfaceFlinger_test Change-Id: I20a33cafc0960e73f9a2c3d740f81319e02b68ff --- libs/gui/SurfaceComposerClient.cpp | 89 ++++++++++++++++++++++++++++ libs/gui/SurfaceControl.cpp | 3 +- libs/gui/include/gui/SurfaceComposerClient.h | 8 ++- libs/gui/include/gui/SurfaceControl.h | 4 +- 4 files changed, 99 insertions(+), 5 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index cc9e468e16..3f97a969bf 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -322,10 +322,99 @@ SurfaceComposerClient::Transaction::Transaction(const Transaction& other) mTransactionNestCount(other.mTransactionNestCount), mAnimation(other.mAnimation), mEarlyWakeup(other.mEarlyWakeup), + mContainsBuffer(other.mContainsBuffer), mDesiredPresentTime(other.mDesiredPresentTime) { mDisplayStates = other.mDisplayStates; mComposerStates = other.mComposerStates; mInputWindowCommands = other.mInputWindowCommands; + mListenerCallbacks = other.mListenerCallbacks; +} + +std::unique_ptr +SurfaceComposerClient::Transaction::createFromParcel(const Parcel* parcel) { + auto transaction = std::make_unique(); + if (transaction->readFromParcel(parcel) == NO_ERROR) { + return transaction; + } + return nullptr; +} + +status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel) { + const uint32_t forceSynchronous = parcel->readUint32(); + const uint32_t transactionNestCount = parcel->readUint32(); + const bool animation = parcel->readBool(); + const bool earlyWakeup = parcel->readBool(); + const bool containsBuffer = parcel->readBool(); + const int64_t desiredPresentTime = parcel->readInt64(); + + size_t count = static_cast(parcel->readUint32()); + if (count > parcel->dataSize()) { + return BAD_VALUE; + } + SortedVector displayStates; + displayStates.setCapacity(count); + for (size_t i = 0; i < count; i++) { + DisplayState displayState; + if (displayState.read(*parcel) == BAD_VALUE) { + return BAD_VALUE; + } + displayStates.add(displayState); + } + + count = static_cast(parcel->readUint32()); + if (count > parcel->dataSize()) { + return BAD_VALUE; + } + std::unordered_map, ComposerState, SCHash> composerStates; + composerStates.reserve(count); + for (size_t i = 0; i < count; i++) { + sp surfaceControl = SurfaceControl::readFromParcel(parcel); + + ComposerState composerState; + if (composerState.read(*parcel) == BAD_VALUE) { + return BAD_VALUE; + } + composerStates[surfaceControl] = composerState; + } + + InputWindowCommands inputWindowCommands; + inputWindowCommands.read(*parcel); + + // Parsing was successful. Update the object. + mForceSynchronous = forceSynchronous; + mTransactionNestCount = transactionNestCount; + mAnimation = animation; + mEarlyWakeup = earlyWakeup; + mContainsBuffer = containsBuffer; + mDesiredPresentTime = desiredPresentTime; + mDisplayStates = displayStates; + mComposerStates = composerStates; + mInputWindowCommands = inputWindowCommands; + // listener callbacks contain function pointer addresses and may not be safe to parcel. + mListenerCallbacks.clear(); + return NO_ERROR; +} + +status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const { + parcel->writeUint32(mForceSynchronous); + parcel->writeUint32(mTransactionNestCount); + parcel->writeBool(mAnimation); + parcel->writeBool(mEarlyWakeup); + parcel->writeBool(mContainsBuffer); + parcel->writeInt64(mDesiredPresentTime); + parcel->writeUint32(static_cast(mDisplayStates.size())); + for (auto const& displayState : mDisplayStates) { + displayState.write(*parcel); + } + + parcel->writeUint32(static_cast(mComposerStates.size())); + for (auto const& [surfaceControl, composerState] : mComposerStates) { + surfaceControl->writeToParcel(parcel); + composerState.write(*parcel); + } + + mInputWindowCommands.write(*parcel); + return NO_ERROR; } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) { diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 55488dad0b..d87a447a97 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -186,8 +186,7 @@ void SurfaceControl::writeToParcel(Parcel* parcel) parcel->writeStrongBinder(IGraphicBufferProducer::asBinder(mGraphicBufferProducer)); } -sp SurfaceControl::readFromParcel(Parcel* parcel) -{ +sp SurfaceControl::readFromParcel(const Parcel* parcel) { sp client = parcel->readStrongBinder(); sp handle = parcel->readStrongBinder(); if (client == nullptr || handle == nullptr) diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 0e17c7b015..1f2947c824 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -285,7 +285,7 @@ public: std::unordered_set, SCHash> surfaceControls; }; - class Transaction { + class Transaction : Parcelable { std::unordered_map, ComposerState, SCHash> mComposerStates; SortedVector mDisplayStates; std::unordered_map, CallbackInfo, TCLHash> @@ -325,6 +325,12 @@ public: virtual ~Transaction() = default; Transaction(Transaction const& other); + // Factory method that creates a new Transaction instance from the parcel. + static std::unique_ptr createFromParcel(const Parcel* parcel); + + status_t writeToParcel(Parcel* parcel) const override; + status_t readFromParcel(const Parcel* parcel) override; + status_t apply(bool synchronous = false); // Merge another transaction in to this one, clearing other // as if it had been applied. diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index 23bfc0256b..ae4a14690f 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -44,7 +44,7 @@ class SurfaceComposerClient; class SurfaceControl : public RefBase { public: - static sp readFromParcel(Parcel* parcel); + static sp readFromParcel(const Parcel* parcel); void writeToParcel(Parcel* parcel); static bool isValid(const sp& surface) { @@ -81,7 +81,7 @@ public: status_t getLayerFrameStats(FrameStats* outStats) const; sp getClient() const; - + explicit SurfaceControl(const sp& other); SurfaceControl(const sp& client, const sp& handle, -- cgit v1.2.3-59-g8ed1b From f03652dd9df88182518a7046cf542076ea10d5ea Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 16 Jul 2019 17:56:56 -0700 Subject: Store layer state changes by layer handle in Transaction objects Layer state changes are stored in an unordered map with the surface control as the key and state changes as the value. This causes a few problems when merging the transactions. If transactions contained state changes from a cloned surface control then it will not be merged. This will cause ordering issues when the transaction is applied since the changes are stored in and unordered map. When parcelling transactions, a new surface control is created from the existing one and this surfaces the problem more frequently. Instead we store the layer changes by the layer handle which is consistent across processes. Test: atest SurfaceFlinger_test Test: go/wm-smoke Change-Id: I2e041d70ae24db2c1f26ada003532ad97f667167 --- libs/gui/LayerState.cpp | 2 -- libs/gui/SurfaceComposerClient.cpp | 40 +++++++++++++--------------- libs/gui/SurfaceControl.cpp | 4 +-- libs/gui/include/gui/LayerState.h | 3 --- libs/gui/include/gui/SurfaceComposerClient.h | 25 +++++++++-------- services/surfaceflinger/SurfaceFlinger.cpp | 28 +------------------ services/surfaceflinger/SurfaceFlinger.h | 1 - 7 files changed, 36 insertions(+), 67 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 6066421faf..a09ceae7c6 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -169,12 +169,10 @@ status_t layer_state_t::read(const Parcel& input) } status_t ComposerState::write(Parcel& output) const { - output.writeStrongBinder(IInterface::asBinder(client)); return state.write(output); } status_t ComposerState::read(const Parcel& input) { - client = interface_cast(input.readStrongBinder()); return state.read(input); } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index c59fddfb9d..de36511a5b 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -202,7 +202,8 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener * callbackIds to generate one super map that contains all the sp to sp * that could possibly exist for the callbacks. */ - std::unordered_map, sp, IBinderHash> surfaceControls; + std::unordered_map, sp, SurfaceComposerClient::IBinderHash> + surfaceControls; for (const auto& transactionStats : listenerStats.transactionStats) { for (auto callbackId : transactionStats.callbackIds) { auto& [callbackFunction, callbackSurfaceControls] = mCallbacks[callbackId]; @@ -365,16 +366,16 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel if (count > parcel->dataSize()) { return BAD_VALUE; } - std::unordered_map, ComposerState, SCHash> composerStates; + std::unordered_map, ComposerState, IBinderHash> composerStates; composerStates.reserve(count); for (size_t i = 0; i < count; i++) { - sp surfaceControl = SurfaceControl::readFromParcel(parcel); + sp surfaceControlHandle = parcel->readStrongBinder(); ComposerState composerState; if (composerState.read(*parcel) == BAD_VALUE) { return BAD_VALUE; } - composerStates[surfaceControl] = composerState; + composerStates[surfaceControlHandle] = composerState; } InputWindowCommands inputWindowCommands; @@ -408,8 +409,8 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const } parcel->writeUint32(static_cast(mComposerStates.size())); - for (auto const& [surfaceControl, composerState] : mComposerStates) { - surfaceControl->writeToParcel(parcel); + for (auto const& [surfaceHandle, composerState] : mComposerStates) { + parcel->writeStrongBinder(surfaceHandle); composerState.write(*parcel); } @@ -418,11 +419,11 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) { - for (auto const& kv : other.mComposerStates) { - if (mComposerStates.count(kv.first) == 0) { - mComposerStates[kv.first] = kv.second; + for (auto const& [surfaceHandle, composerState] : other.mComposerStates) { + if (mComposerStates.count(surfaceHandle) == 0) { + mComposerStates[surfaceHandle] = composerState; } else { - mComposerStates[kv.first].state.merge(kv.second.state); + mComposerStates[surfaceHandle].state.merge(composerState.state); } } @@ -465,14 +466,12 @@ void SurfaceComposerClient::Transaction::clear() { mDesiredPresentTime = -1; } -void SurfaceComposerClient::doDropReferenceTransaction(const sp& handle, - const sp& client) { +void SurfaceComposerClient::doDropReferenceTransaction(const sp& handle) { 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; @@ -499,8 +498,8 @@ void SurfaceComposerClient::Transaction::cacheBuffers() { } size_t count = 0; - for (auto& [sc, cs] : mComposerStates) { - layer_state_t* s = getLayerState(sc); + for (auto& [handle, cs] : mComposerStates) { + layer_state_t* s = getLayerState(handle); if (!(s->what & layer_state_t::eBufferChanged)) { continue; } @@ -639,16 +638,15 @@ void SurfaceComposerClient::Transaction::setEarlyWakeup() { mEarlyWakeup = true; } -layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp& sc) { - if (mComposerStates.count(sc) == 0) { +layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp& handle) { + if (mComposerStates.count(handle) == 0) { // we don't have it, add an initialized layer_state to our list ComposerState s; - s.client = sc->getClient()->mClient; - s.state.surface = sc->getHandle(); - mComposerStates[sc] = s; + s.state.surface = handle; + mComposerStates[handle] = s; } - return &(mComposerStates[sc].state); + return &(mComposerStates[handle].state); } void SurfaceComposerClient::Transaction::registerSurfaceControlForCallback( diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index b9defdd120..071314f082 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -65,8 +65,8 @@ 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()); + if (mHandle != nullptr && mOwned) { + SurfaceComposerClient::doDropReferenceTransaction(mHandle); } release(); } diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index f438eb3d01..cbd1c8553b 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -206,7 +206,6 @@ struct layer_state_t { }; struct ComposerState { - sp client; layer_state_t state; status_t write(Parcel& output) const; status_t read(const Parcel& input); @@ -274,8 +273,6 @@ struct InputWindowCommands { }; static inline int compare_type(const ComposerState& lhs, const ComposerState& rhs) { - if (lhs.client < rhs.client) return -1; - if (lhs.client > rhs.client) return 1; if (lhs.state.surface < rhs.state.surface) return -1; if (lhs.state.surface > rhs.state.surface) return 1; return 0; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 4dda97f5e1..22ab62da44 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -163,8 +163,7 @@ public: * 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); + static void doDropReferenceTransaction(const sp& handle); /** * Uncaches a buffer in ISurfaceComposer. It must be uncached via a transaction so that it is @@ -270,6 +269,12 @@ public: } }; + struct IBinderHash { + std::size_t operator()(const sp& iBinder) const { + return std::hash{}(iBinder.get()); + } + }; + struct TCLHash { std::size_t operator()(const sp& tcl) const { return std::hash{}((tcl) ? IInterface::asBinder(tcl).get() : nullptr); @@ -286,7 +291,7 @@ public: }; class Transaction : Parcelable { - std::unordered_map, ComposerState, SCHash> mComposerStates; + std::unordered_map, ComposerState, IBinderHash> mComposerStates; SortedVector mDisplayStates; std::unordered_map, CallbackInfo, TCLHash> mListenerCallbacks; @@ -314,7 +319,10 @@ public: InputWindowCommands mInputWindowCommands; int mStatus = NO_ERROR; - layer_state_t* getLayerState(const sp& sc); + layer_state_t* getLayerState(const sp& surfaceHandle); + layer_state_t* getLayerState(const sp& sc) { + return getLayerState(sc->getHandle()); + } DisplayState& getDisplayState(const sp& token); void cacheBuffers(); @@ -560,15 +568,10 @@ class TransactionCompletedListener : public BnTransactionCompletedListener { CallbackId mCallbackIdCounter GUARDED_BY(mMutex) = 1; - struct IBinderHash { - std::size_t operator()(const sp& iBinder) const { - return std::hash{}(iBinder.get()); - } - }; - struct CallbackTranslation { TransactionCompletedCallback callbackFunction; - std::unordered_map, sp, IBinderHash> surfaceControls; + std::unordered_map, sp, SurfaceComposerClient::IBinderHash> + surfaceControls; }; std::unordered_map mCallbacks GUARDED_BY(mMutex); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index f1e3971811..9c612b459b 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3655,27 +3655,6 @@ bool SurfaceFlinger::transactionFlushNeeded() { return !mTransactionQueues.empty(); } -bool SurfaceFlinger::containsAnyInvalidClientState(const Vector& states) { - for (const ComposerState& state : states) { - // Here we need to check that the interface we're given is indeed - // one of our own. A malicious client could give us a nullptr - // IInterface, or one of its own or even one of our own but a - // different type. All these situations would cause us to crash. - if (state.client == nullptr) { - return true; - } - - sp binder = IInterface::asBinder(state.client); - if (binder == nullptr) { - return true; - } - - if (binder->queryLocalInterface(ISurfaceComposerClient::descriptor) == nullptr) { - return true; - } - } - return false; -} bool SurfaceFlinger::transactionIsReadyToBeApplied(int64_t desiredPresentTime, const Vector& states) { @@ -3714,10 +3693,6 @@ void SurfaceFlinger::setTransactionState(const Vector& states, Mutex::Autolock _l(mStateLock); - if (containsAnyInvalidClientState(states)) { - return; - } - // If its TransactionQueue already has a pending TransactionState or if it is pending auto itr = mTransactionQueues.find(applyToken); // if this is an animation frame, wait until prior animation frame has @@ -3929,9 +3904,8 @@ uint32_t SurfaceFlinger::setClientStateLocked( const std::vector& listenerCallbacks, int64_t postTime, bool privileged) { const layer_state_t& s = composerState.state; - sp client(static_cast(composerState.client.get())); - sp layer(client->getLayerUser(s.surface)); + sp layer(fromHandle(s.surface)); if (layer == nullptr) { for (auto& listenerCallback : listenerCallbacks) { mTransactionCompletedThread.registerUnpresentedCallbackHandle( diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index fa801afff8..30b6b69b17 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -582,7 +582,6 @@ private: void latchAndReleaseBuffer(const sp& layer); void commitTransaction() REQUIRES(mStateLock); void commitOffscreenLayers(); - bool containsAnyInvalidClientState(const Vector& states); bool transactionIsReadyToBeApplied(int64_t desiredPresentTime, const Vector& states); uint32_t setClientStateLocked(const ComposerState& composerState, int64_t desiredPresentTime, -- cgit v1.2.3-59-g8ed1b From 1acd6961737cc32627d8298999878c2674328b6b Mon Sep 17 00:00:00 2001 From: Valerie Hau Date: Mon, 28 Oct 2019 16:35:48 -0700 Subject: Pass back transformHint on Surface Creation Bug: 141939598 Test: build, boot, SurfaceFlinger_test, libgui_test, libsurfaceflinger_unittest Change-Id: I35a77ac1399ad4248cb1c2357afb869de4c15170 --- libs/gui/ISurfaceComposerClient.cpp | 13 +++++---- libs/gui/SurfaceComposerClient.cpp | 29 +++++++++++++----- libs/gui/SurfaceControl.cpp | 34 +++++++++++++++------- libs/gui/include/gui/ISurfaceComposerClient.h | 5 ++-- libs/gui/include/gui/SurfaceComposerClient.h | 16 +++++----- libs/gui/include/gui/SurfaceControl.h | 7 ++++- services/surfaceflinger/BufferStateLayer.h | 1 + services/surfaceflinger/Client.cpp | 9 +++--- services/surfaceflinger/Client.h | 6 ++-- services/surfaceflinger/SurfaceFlinger.cpp | 11 ++++--- services/surfaceflinger/SurfaceFlinger.h | 6 ++-- .../tests/LayerRenderTypeTransaction_test.cpp | 11 ++++++- .../surfaceflinger/tests/LayerTransactionTest.h | 18 +++++++----- .../tests/TransactionTestHarnesses.h | 6 ++-- 14 files changed, 116 insertions(+), 56 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/ISurfaceComposerClient.cpp b/libs/gui/ISurfaceComposerClient.cpp index b98e48b52a..621cf5950b 100644 --- a/libs/gui/ISurfaceComposerClient.cpp +++ b/libs/gui/ISurfaceComposerClient.cpp @@ -49,25 +49,28 @@ public: status_t createSurface(const String8& name, uint32_t width, uint32_t height, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, - sp* handle, sp* gbp) override { + sp* handle, sp* gbp, + uint32_t* outTransformHint) override { return callRemote(Tag::CREATE_SURFACE, name, width, height, format, flags, parent, std::move(metadata), - handle, gbp); + handle, gbp, + outTransformHint); } status_t createWithSurfaceParent(const String8& name, uint32_t width, uint32_t height, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp) override { + sp* gbp, + uint32_t* outTransformHint) override { return callRemote(Tag::CREATE_WITH_SURFACE_PARENT, name, width, height, format, flags, parent, - std::move(metadata), handle, - gbp); + std::move(metadata), handle, gbp, + outTransformHint); } status_t clearLayerFrameStats(const sp& handle) const override { diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index e9079efd29..3178b6af8d 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -224,6 +224,8 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener surfaceStats.acquireTime, surfaceStats.previousReleaseFence, surfaceStats.transformHint); + surfaceControls[surfaceStats.surfaceControl]->setTransformHint( + surfaceStats.transformHint); } callbackFunction(transactionStats.latchTime, transactionStats.presentFence, @@ -1451,16 +1453,19 @@ void SurfaceComposerClient::dispose() { sp SurfaceComposerClient::createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, SurfaceControl* parent, - LayerMetadata metadata) { + LayerMetadata metadata, + uint32_t* outTransformHint) { sp s; - createSurfaceChecked(name, w, h, format, &s, flags, parent, std::move(metadata)); + createSurfaceChecked(name, w, h, format, &s, flags, parent, std::move(metadata), + outTransformHint); return s; } sp SurfaceComposerClient::createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, Surface* parent, - LayerMetadata metadata) { + LayerMetadata metadata, + uint32_t* outTransformHint) { sp sur; status_t err = mStatus; @@ -1469,8 +1474,12 @@ sp SurfaceComposerClient::createWithSurfaceParent(const String8& sp parentGbp = parent->getIGraphicBufferProducer(); sp gbp; + uint32_t transformHint = 0; err = mClient->createWithSurfaceParent(name, w, h, format, flags, parentGbp, - std::move(metadata), &handle, &gbp); + std::move(metadata), &handle, &gbp, &transformHint); + if (outTransformHint) { + *outTransformHint = transformHint; + } ALOGE_IF(err, "SurfaceComposerClient::createWithSurfaceParent error %s", strerror(-err)); if (err == NO_ERROR) { return new SurfaceControl(this, handle, gbp, true /* owned */); @@ -1482,8 +1491,8 @@ sp SurfaceComposerClient::createWithSurfaceParent(const String8& status_t SurfaceComposerClient::createSurfaceChecked(const String8& name, uint32_t w, uint32_t h, PixelFormat format, sp* outSurface, uint32_t flags, - SurfaceControl* parent, - LayerMetadata metadata) { + SurfaceControl* parent, LayerMetadata metadata, + uint32_t* outTransformHint) { sp sur; status_t err = mStatus; @@ -1496,11 +1505,15 @@ status_t SurfaceComposerClient::createSurfaceChecked(const String8& name, uint32 parentHandle = parent->getHandle(); } + uint32_t transformHint = 0; err = mClient->createSurface(name, w, h, format, flags, parentHandle, std::move(metadata), - &handle, &gbp); + &handle, &gbp, &transformHint); + if (outTransformHint) { + *outTransformHint = transformHint; + } ALOGE_IF(err, "SurfaceComposerClient::createSurface error %s", strerror(-err)); if (err == NO_ERROR) { - *outSurface = new SurfaceControl(this, handle, gbp, true /* owned */); + *outSurface = new SurfaceControl(this, handle, gbp, true /* owned */, transformHint); } } return err; diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 071314f082..6292388ac3 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -45,20 +45,21 @@ namespace android { // SurfaceControl // ============================================================================ -SurfaceControl::SurfaceControl( - const sp& client, - const sp& handle, - const sp& gbp, - bool owned) - : mClient(client), mHandle(handle), mGraphicBufferProducer(gbp), mOwned(owned) -{ -} +SurfaceControl::SurfaceControl(const sp& client, const sp& handle, + const sp& gbp, bool owned, + uint32_t transform) + : mClient(client), + mHandle(handle), + mGraphicBufferProducer(gbp), + mOwned(owned), + mTransformHint(transform) {} SurfaceControl::SurfaceControl(const sp& other) { mClient = other->mClient; mHandle = other->mHandle; mGraphicBufferProducer = other->mGraphicBufferProducer; mOwned = false; + mTransformHint = other->mTransformHint; } SurfaceControl::~SurfaceControl() @@ -171,11 +172,22 @@ sp SurfaceControl::getClient() const return mClient; } +uint32_t SurfaceControl::getTransformHint() const { + Mutex::Autolock _l(mLock); + return mTransformHint; +} + +void SurfaceControl::setTransformHint(uint32_t hint) { + Mutex::Autolock _l(mLock); + mTransformHint = hint; +} + void SurfaceControl::writeToParcel(Parcel* parcel) { parcel->writeStrongBinder(ISurfaceComposerClient::asBinder(mClient->getClient())); parcel->writeStrongBinder(mHandle); parcel->writeStrongBinder(IGraphicBufferProducer::asBinder(mGraphicBufferProducer)); + parcel->writeUint32(mTransformHint); } sp SurfaceControl::readFromParcel(const Parcel* parcel) { @@ -189,10 +201,12 @@ sp SurfaceControl::readFromParcel(const Parcel* parcel) { sp gbp; parcel->readNullableStrongBinder(&gbp); + uint32_t transformHint = parcel->readUint32(); // We aren't the original owner of the surface. return new SurfaceControl(new SurfaceComposerClient( - interface_cast(client)), - handle.get(), interface_cast(gbp), false /* owned */); + interface_cast(client)), + handle.get(), interface_cast(gbp), + false /* owned */, transformHint); } // ---------------------------------------------------------------------------- diff --git a/libs/gui/include/gui/ISurfaceComposerClient.h b/libs/gui/include/gui/ISurfaceComposerClient.h index 5fe7ca5344..2b65d2f42d 100644 --- a/libs/gui/include/gui/ISurfaceComposerClient.h +++ b/libs/gui/include/gui/ISurfaceComposerClient.h @@ -56,7 +56,7 @@ public: virtual status_t createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp) = 0; + sp* gbp, uint32_t* outTransformHint) = 0; /* * Requires ACCESS_SURFACE_FLINGER permission @@ -65,7 +65,8 @@ public: PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp) = 0; + sp* gbp, + uint32_t* outTransformHint) = 0; /* * Requires ACCESS_SURFACE_FLINGER permission diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 08f4e9e9d3..7a9598cddd 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -226,18 +226,18 @@ public: PixelFormat format, // pixel-format desired uint32_t flags = 0, // usage flags SurfaceControl* parent = nullptr, // parent - LayerMetadata metadata = LayerMetadata() // metadata - ); + LayerMetadata metadata = LayerMetadata(), // metadata + uint32_t* outTransformHint = nullptr); status_t createSurfaceChecked(const String8& name, // name of the surface uint32_t w, // width in pixel uint32_t h, // height in pixel PixelFormat format, // pixel-format desired sp* outSurface, - uint32_t flags = 0, // usage flags - SurfaceControl* parent = nullptr, // parent - LayerMetadata metadata = LayerMetadata() // metadata - ); + uint32_t flags = 0, // usage flags + SurfaceControl* parent = nullptr, // parent + LayerMetadata metadata = LayerMetadata(), // metadata + uint32_t* outTransformHint = nullptr); //! Create a surface sp createWithSurfaceParent(const String8& name, // name of the surface @@ -246,8 +246,8 @@ public: PixelFormat format, // pixel-format desired uint32_t flags = 0, // usage flags Surface* parent = nullptr, // parent - LayerMetadata metadata = LayerMetadata() // metadata - ); + LayerMetadata metadata = LayerMetadata(), // metadata + uint32_t* outTransformHint = nullptr); // Creates a mirrored hierarchy for the mirrorFromSurface. This returns a SurfaceControl // which is a parent of the root of the mirrored hierarchy. diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index ae4a14690f..7bc7c686c9 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -82,10 +82,14 @@ public: sp getClient() const; + uint32_t getTransformHint() const; + + void setTransformHint(uint32_t hint); + explicit SurfaceControl(const sp& other); SurfaceControl(const sp& client, const sp& handle, - const sp& gbp, bool owned); + const sp& gbp, bool owned, uint32_t transformHint = 0); private: // can't be copied @@ -106,6 +110,7 @@ private: mutable Mutex mLock; mutable sp mSurfaceData; bool mOwned; + uint32_t mTransformHint; }; }; // namespace android diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index 289bb1756f..8e6a70c748 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -63,6 +63,7 @@ public: } Rect getCrop(const Layer::State& s) const; + uint32_t getTransformHint() const { return mTransformHint; } bool setTransform(uint32_t transform) override; bool setTransformToDisplayInverse(bool transformToDisplayInverse) override; bool setCrop(const Rect& crop) override; diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index c7ed9b0412..f3313645fa 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -75,17 +75,18 @@ sp Client::getLayerUser(const sp& handle) const status_t Client::createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, const sp& parentHandle, LayerMetadata metadata, sp* handle, - sp* gbp) { + sp* gbp, uint32_t* outTransformHint) { // We rely on createLayer to check permissions. return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp, - parentHandle); + parentHandle, nullptr, outTransformHint); } status_t Client::createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp) { + sp* gbp, + uint32_t* outTransformHint) { if (mFlinger->authenticateSurfaceTexture(parent) == false) { ALOGE("failed to authenticate surface texture"); // The extra parent layer check below before returning is to help with debugging @@ -103,7 +104,7 @@ status_t Client::createWithSurfaceParent(const String8& name, uint32_t w, uint32 } return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp, - nullptr, layer); + nullptr, layer, outTransformHint); } status_t Client::mirrorSurface(const sp& mirrorFromHandle, sp* outHandle) { diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index 7d7cef8c50..e9063e5bb6 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -54,13 +54,15 @@ private: virtual status_t createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp); + sp* gbp, + uint32_t* outTransformHint = nullptr); virtual status_t createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, const sp& parent, LayerMetadata metadata, sp* handle, - sp* gbp); + sp* gbp, + uint32_t* outTransformHint = nullptr); status_t mirrorSurface(const sp& mirrorFromHandle, sp* handle); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 057669b376..018d687574 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3432,8 +3432,8 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie uint32_t h, PixelFormat format, uint32_t flags, LayerMetadata metadata, sp* handle, sp* gbp, - const sp& parentHandle, - const sp& parentLayer) { + const sp& parentHandle, const sp& parentLayer, + uint32_t* outTransformHint) { if (int32_t(w|h) < 0) { ALOGE("createLayer() failed, w or h is negative (w=%d, h=%d)", int(w), int(h)); @@ -3470,7 +3470,7 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie break; case ISurfaceComposerClient::eFXSurfaceBufferState: result = createBufferStateLayer(client, std::move(uniqueName), w, h, flags, - std::move(metadata), handle, &layer); + std::move(metadata), handle, outTransformHint, &layer); break; case ISurfaceComposerClient::eFXSurfaceColor: // check if buffer size is set for color layer. @@ -3585,11 +3585,14 @@ status_t SurfaceFlinger::createBufferQueueLayer(const sp& client, std::s status_t SurfaceFlinger::createBufferStateLayer(const sp& client, std::string name, uint32_t w, uint32_t h, uint32_t flags, LayerMetadata metadata, sp* handle, - sp* outLayer) { + uint32_t* outTransformHint, sp* outLayer) { LayerCreationArgs args(this, client, std::move(name), w, h, flags, std::move(metadata)); args.displayDevice = getDefaultDisplayDevice(); args.textureName = getNewTexture(); sp layer = getFactory().createBufferStateLayer(args); + if (outTransformHint) { + *outTransformHint = layer->getTransformHint(); + } *handle = layer->getHandle(); *outLayer = layer; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index b719245caa..a5987b7861 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -605,7 +605,8 @@ private: status_t createLayer(const String8& name, const sp& client, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags, LayerMetadata metadata, sp* handle, sp* gbp, - const sp& parentHandle, const sp& parentLayer = nullptr); + const sp& parentHandle, const sp& parentLayer = nullptr, + uint32_t* outTransformHint = nullptr); status_t createBufferQueueLayer(const sp& client, std::string name, uint32_t w, uint32_t h, uint32_t flags, LayerMetadata metadata, @@ -614,7 +615,8 @@ private: status_t createBufferStateLayer(const sp& client, std::string name, uint32_t w, uint32_t h, uint32_t flags, LayerMetadata metadata, - sp* outHandle, sp* outLayer); + sp* outHandle, uint32_t* outTransformHint, + sp* outLayer); status_t createColorLayer(const sp& client, std::string name, uint32_t w, uint32_t h, uint32_t flags, LayerMetadata metadata, sp* outHandle, diff --git a/services/surfaceflinger/tests/LayerRenderTypeTransaction_test.cpp b/services/surfaceflinger/tests/LayerRenderTypeTransaction_test.cpp index a48f553d74..999e82dbd2 100644 --- a/services/surfaceflinger/tests/LayerRenderTypeTransaction_test.cpp +++ b/services/surfaceflinger/tests/LayerRenderTypeTransaction_test.cpp @@ -15,9 +15,9 @@ */ #include +#include #include #include "TransactionTestHarnesses.h" - namespace android { using android::hardware::graphics::common::V1_1::BufferUsage; @@ -188,6 +188,15 @@ TEST_P(LayerRenderTypeTransactionTest, SetSizeWithScaleToWindow_BufferQueue) { getScreenCapture()->expectColor(Rect(0, 0, 64, 64), Color::RED); } +TEST_P(LayerRenderTypeTransactionTest, CreateLayer_BufferState) { + uint32_t transformHint = ui::Transform::orientation_flags::ROT_INVALID; + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createLayer("test", 32, 32, + ISurfaceComposerClient::eFXSurfaceBufferState, + /*parent*/ nullptr, &transformHint)); + ASSERT_NE(ui::Transform::orientation_flags::ROT_INVALID, transformHint); +} + void LayerRenderTypeTransactionTest::setRelativeZBasicHelper(uint32_t layerType) { sp layerR; sp layerG; diff --git a/services/surfaceflinger/tests/LayerTransactionTest.h b/services/surfaceflinger/tests/LayerTransactionTest.h index 7edddb6ea3..f7a6d964f0 100644 --- a/services/surfaceflinger/tests/LayerTransactionTest.h +++ b/services/surfaceflinger/tests/LayerTransactionTest.h @@ -53,9 +53,10 @@ protected: virtual sp createLayer(const sp& client, const char* name, uint32_t width, uint32_t height, - uint32_t flags = 0, SurfaceControl* parent = nullptr) { - auto layer = - createSurface(client, name, width, height, PIXEL_FORMAT_RGBA_8888, flags, parent); + uint32_t flags = 0, SurfaceControl* parent = nullptr, + uint32_t* outTransformHint = nullptr) { + auto layer = createSurface(client, name, width, height, PIXEL_FORMAT_RGBA_8888, flags, + parent, outTransformHint); Transaction t; t.setLayerStack(layer, mDisplayLayerStack).setLayer(layer, mLayerZBase); @@ -72,15 +73,18 @@ protected: virtual sp createSurface(const sp& client, const char* name, uint32_t width, uint32_t height, PixelFormat format, uint32_t flags, - SurfaceControl* parent = nullptr) { - auto layer = client->createSurface(String8(name), width, height, format, flags, parent); + SurfaceControl* parent = nullptr, + uint32_t* outTransformHint = nullptr) { + auto layer = client->createSurface(String8(name), width, height, format, flags, parent, + LayerMetadata(), outTransformHint); EXPECT_NE(nullptr, layer.get()) << "failed to create SurfaceControl"; return layer; } virtual sp createLayer(const char* name, uint32_t width, uint32_t height, - uint32_t flags = 0, SurfaceControl* parent = nullptr) { - return createLayer(mClient, name, width, height, flags, parent); + uint32_t flags = 0, SurfaceControl* parent = nullptr, + uint32_t* outTransformHint = nullptr) { + return createLayer(mClient, name, width, height, flags, parent, outTransformHint); } sp createColorLayer(const char* name, const Color& color, diff --git a/services/surfaceflinger/tests/TransactionTestHarnesses.h b/services/surfaceflinger/tests/TransactionTestHarnesses.h index 8fdcde40b6..5612bb21c7 100644 --- a/services/surfaceflinger/tests/TransactionTestHarnesses.h +++ b/services/surfaceflinger/tests/TransactionTestHarnesses.h @@ -114,12 +114,14 @@ public: LayerTypeTransactionHarness(uint32_t layerType) : mLayerType(layerType) {} sp createLayer(const char* name, uint32_t width, uint32_t height, - uint32_t flags = 0, SurfaceControl* parent = nullptr) { + uint32_t flags = 0, SurfaceControl* parent = nullptr, + uint32_t* outTransformHint = nullptr) { // if the flags already have a layer type specified, return an error if (flags & ISurfaceComposerClient::eFXSurfaceMask) { return nullptr; } - return LayerTransactionTest::createLayer(name, width, height, flags | mLayerType, parent); + return LayerTransactionTest::createLayer(name, width, height, flags | mLayerType, parent, + outTransformHint); } void fillLayerColor(const sp& layer, const Color& color, int32_t bufferWidth, -- cgit v1.2.3-59-g8ed1b From 0e328f6a641f256c49e2f063207eca6ddd02be60 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Thu, 6 Feb 2020 17:12:08 -0800 Subject: SurfaceControl: C++ Binding Lifetime refactoring First we eliminate the "dropReferenceTransaction" semantic. This semantic reparents the surface to null if the C++ object dies before release() is called. This is a legacy semantic from before SurfaceControls were reference counted. I point that it's unused by noting that all Java code paths will lead to calling release() in the JNI code before dropping the last reference. With dropReferenceTransaction gone we can remove mOwned it has no further uses. With these gone we now remove release() all together on the native side. This means that mClient and mHandle will only be written from the constructor and destructor making access to them thread-safe as long as you hold an sp<> to the SurfaceControl. This should prevent bugs like we've had in the past about who calls release when, no one calls it! The final question is: is removing the call to release on the Java side safe? We still need an explicit Java binding release call so we can drop the native reference in a timely fashion. This then breaks down in to two scenarios: 1. We are the last reference 2. Someone else holds a reference If we are in the first scenario, then calling release or not is equivalent to just dropping the reference. If we are in the second scenario, calling release() will be unsafe. Because we could at any time overwrite mClient/mHandle after the other ref holder had verified it was null. The main path I know of for how native code could acquire a second reference to the JNI owned SurfaceControl is via Transaction::registerSurfaceControlForCallback then if we release while Transaction::writeToParcel is running, it will inevitably segfault. This change could lead to the extension of life-time for SurfaceControl.cpp objects while the Transaction containing them is alive (but previously the SurfaceControl.cpp proxy would have been released). I also argue this is safe since the sp itself was reffed in another place in the Transaction so the lifetime of the actual server side resource isn't extended at all. Only the lightweight proxy object. Bug: 149055469 Bug: 149315421 Test: Existing tests pass. Change-Id: Ibd4d1804ef18a9c389c7f9112d15872cfe44b22e --- libs/gui/SurfaceComposerClient.cpp | 19 ++----------------- libs/gui/SurfaceControl.cpp | 17 ++--------------- libs/gui/include/gui/SurfaceComposerClient.h | 6 ------ libs/gui/include/gui/SurfaceControl.h | 7 +------ .../tests/LayerTypeAndRenderTypeTransaction_test.cpp | 3 ++- services/surfaceflinger/tests/LayerUpdate_test.cpp | 2 ++ 6 files changed, 9 insertions(+), 45 deletions(-) (limited to 'libs/gui/SurfaceControl.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 7017b7c8f3..ff8b719009 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -526,21 +526,6 @@ void SurfaceComposerClient::Transaction::clear() { mDesiredPresentTime = -1; } -void SurfaceComposerClient::doDropReferenceTransaction(const sp& handle) { - sp sf(ComposerService::getComposerService()); - Vector composerStates; - Vector displayStates; - - ComposerState s; - s.state.surface = handle; - s.state.what |= layer_state_t::eReparent; - s.state.parentHandleForChild = nullptr; - - composerStates.add(s); - sp applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance()); - sf->setTransactionState(composerStates, displayStates, 0, applyToken, {}, -1, {}, false, {}); -} - void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { sp sf(ComposerService::getComposerService()); @@ -1558,7 +1543,7 @@ sp SurfaceComposerClient::createWithSurfaceParent(const String8& } ALOGE_IF(err, "SurfaceComposerClient::createWithSurfaceParent error %s", strerror(-err)); if (err == NO_ERROR) { - return new SurfaceControl(this, handle, gbp, true /* owned */, transformHint); + return new SurfaceControl(this, handle, gbp, transformHint); } } return nullptr; @@ -1589,7 +1574,7 @@ status_t SurfaceComposerClient::createSurfaceChecked(const String8& name, uint32 } ALOGE_IF(err, "SurfaceComposerClient::createSurface error %s", strerror(-err)); if (err == NO_ERROR) { - *outSurface = new SurfaceControl(this, handle, gbp, true /* owned */, transformHint); + *outSurface = new SurfaceControl(this, handle, gbp, transformHint); } } return err; diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 6292388ac3..a332a1f2a8 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -46,33 +46,21 @@ namespace android { // ============================================================================ SurfaceControl::SurfaceControl(const sp& client, const sp& handle, - const sp& gbp, bool owned, + const sp& gbp, uint32_t transform) : mClient(client), mHandle(handle), mGraphicBufferProducer(gbp), - mOwned(owned), mTransformHint(transform) {} SurfaceControl::SurfaceControl(const sp& other) { mClient = other->mClient; mHandle = other->mHandle; mGraphicBufferProducer = other->mGraphicBufferProducer; - mOwned = false; mTransformHint = other->mTransformHint; } 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 (mHandle != nullptr && mOwned) { - SurfaceComposerClient::doDropReferenceTransaction(mHandle); - } - release(); -} - -void SurfaceControl::release() { // Trigger an IPC now, to make sure things // happen without delay, since these resources are quite heavy. @@ -157,7 +145,6 @@ sp SurfaceControl::createSurface() const sp SurfaceControl::getHandle() const { - Mutex::Autolock lock(mLock); return mHandle; } @@ -206,7 +193,7 @@ sp SurfaceControl::readFromParcel(const Parcel* parcel) { return new SurfaceControl(new SurfaceComposerClient( interface_cast(client)), handle.get(), interface_cast(gbp), - false /* owned */, transformHint); + transformHint); } // ---------------------------------------------------------------------------- diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index d0bb6a39ec..27877bb174 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -183,12 +183,6 @@ public: */ static bool getProtectedContentSupport(); - /** - * 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); - /** * Uncaches a buffer in ISurfaceComposer. It must be uncached via a transaction so that it is * in order with other transactions that use buffers. diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index 7bc7c686c9..ac2bbccfd2 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -58,10 +58,6 @@ public: static bool isSameSurface( const sp& lhs, const sp& rhs); - // 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(); @@ -89,7 +85,7 @@ public: explicit SurfaceControl(const sp& other); SurfaceControl(const sp& client, const sp& handle, - const sp& gbp, bool owned, uint32_t transformHint = 0); + const sp& gbp, uint32_t transformHint = 0); private: // can't be copied @@ -109,7 +105,6 @@ private: sp mGraphicBufferProducer; mutable Mutex mLock; mutable sp mSurfaceData; - bool mOwned; uint32_t mTransformHint; }; diff --git a/services/surfaceflinger/tests/LayerTypeAndRenderTypeTransaction_test.cpp b/services/surfaceflinger/tests/LayerTypeAndRenderTypeTransaction_test.cpp index dbace11dcb..2fd257945f 100644 --- a/services/surfaceflinger/tests/LayerTypeAndRenderTypeTransaction_test.cpp +++ b/services/surfaceflinger/tests/LayerTypeAndRenderTypeTransaction_test.cpp @@ -92,7 +92,8 @@ TEST_P(LayerTypeAndRenderTypeTransactionTest, SetRelativeZBug64572777) { .setRelativeLayer(layerG, layerR->getHandle(), 1) .apply(); - layerG.clear(); + Transaction().reparent(layerG, nullptr).apply(); + // layerG should have been removed getScreenCapture()->expectColor(Rect(0, 0, 32, 32), Color::RED); } diff --git a/services/surfaceflinger/tests/LayerUpdate_test.cpp b/services/surfaceflinger/tests/LayerUpdate_test.cpp index cf3f8e8865..cdd9d92063 100644 --- a/services/surfaceflinger/tests/LayerUpdate_test.cpp +++ b/services/surfaceflinger/tests/LayerUpdate_test.cpp @@ -542,6 +542,7 @@ TEST_F(ChildLayerTest, ChildrenSurviveParentDestruction) { mCapture->checkPixel(64, 64, 111, 111, 111); } + Transaction().reparent(mChild, nullptr).apply(); mChild.clear(); { @@ -1702,6 +1703,7 @@ TEST_F(ScreenCaptureTest, CaptureInvalidLayer) { ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(redLayer, Color::RED, 60, 60)); auto redLayerHandle = redLayer->getHandle(); + Transaction().reparent(redLayer, nullptr).apply(); redLayer.clear(); SurfaceComposerClient::Transaction().apply(true); -- cgit v1.2.3-59-g8ed1b