From e2ffb426c3c48bbc93bc2cfc28b33b6f6e0ddfa2 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Fri, 12 Oct 2018 11:33:52 -0700 Subject: blast: Send transaction callback Send transaction callback to client via the TransactionCompletedListener. Test: Transaction_test Bug: 80477568 Change-Id: Iac98780b1357b9cc54b93cc3c848013b28fab441 --- .../surfaceflinger/TransactionCompletedThread.cpp | 168 +++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 services/surfaceflinger/TransactionCompletedThread.cpp (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp new file mode 100644 index 0000000000..e83422e23b --- /dev/null +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -0,0 +1,168 @@ +/* + * Copyright 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +//#define LOG_NDEBUG 0 +#undef LOG_TAG +#define LOG_TAG "TransactionCompletedThread" +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + +#include "TransactionCompletedThread.h" + +#include + +#include +#include +#include + +namespace android { + +TransactionCompletedThread::~TransactionCompletedThread() { + { + std::lock_guard lock(mMutex); + mKeepRunning = false; + mConditionVariable.notify_all(); + } + + mThread.join(); + + { + std::lock_guard lock(mMutex); + for (const auto& [listener, listenerStats] : mListenerStats) { + listener->unlinkToDeath(mDeathRecipient); + } + } +} + +void TransactionCompletedThread::run() { + std::lock_guard lock(mMutex); + if (mRunning) { + return; + } + mDeathRecipient = new ThreadDeathRecipient(); + mRunning = true; + mThread = std::thread(&TransactionCompletedThread::threadMain, this); +} + +void TransactionCompletedThread::registerPendingLatchedCallbackHandle( + const sp& handle) { + std::lock_guard lock(mMutex); + + sp listener = IInterface::asBinder(handle->listener); + const auto& callbackIds = handle->callbackIds; + + mPendingTransactions[listener][callbackIds]++; +} + +void TransactionCompletedThread::addLatchedCallbackHandles( + const std::deque>& handles) { + std::lock_guard lock(mMutex); + + for (const auto& handle : handles) { + auto listener = mPendingTransactions.find(IInterface::asBinder(handle->listener)); + auto& pendingCallbacks = listener->second; + auto pendingCallback = pendingCallbacks.find(handle->callbackIds); + + if (pendingCallback != pendingCallbacks.end()) { + auto& pendingCount = pendingCallback->second; + + // Decrease the pending count for this listener + if (--pendingCount == 0) { + pendingCallbacks.erase(pendingCallback); + } + } else { + ALOGW("there are more latched callbacks than there were registered callbacks"); + } + + addCallbackHandle(handle); + } +} + +void TransactionCompletedThread::addUnlatchedCallbackHandle(const sp& handle) { + std::lock_guard lock(mMutex); + addCallbackHandle(handle); +} + +void TransactionCompletedThread::addCallbackHandle(const sp& handle) { + const sp listener = IInterface::asBinder(handle->listener); + + // If we don't already have a reference to this listener, linkToDeath so we get a notification + // if it dies. + if (mListenerStats.count(listener) == 0) { + status_t error = listener->linkToDeath(mDeathRecipient); + if (error != NO_ERROR) { + ALOGE("cannot add callback handle because linkToDeath failed, err: %d", error); + return; + } + } + + auto& listenerStats = mListenerStats[listener]; + listenerStats.listener = handle->listener; + + auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; + transactionStats.surfaceStats.emplace_back(handle->surfaceControl); +} + +void TransactionCompletedThread::sendCallbacks() { + std::lock_guard lock(mMutex); + if (mRunning) { + mConditionVariable.notify_all(); + } +} + +void TransactionCompletedThread::threadMain() { + std::lock_guard lock(mMutex); + + while (mKeepRunning) { + mConditionVariable.wait(mMutex); + + // For each listener + auto it = mListenerStats.begin(); + while (it != mListenerStats.end()) { + auto& [listener, listenerStats] = *it; + + // For each transaction + bool sendCallback = true; + for (auto& [callbackIds, transactionStats] : listenerStats.transactionStats) { + // If we are still waiting on the callback handles for this transaction, skip it + if (mPendingTransactions[listener].count(callbackIds) != 0) { + sendCallback = false; + break; + } + } + // If the listener has no pending transactions and all latched transactions have been + // presented + if (sendCallback) { + // If the listener is still alive + if (listener->isBinderAlive()) { + // Send callback + listenerStats.listener->onTransactionCompleted(listenerStats); + listener->unlinkToDeath(mDeathRecipient); + } + it = mListenerStats.erase(it); + } else { + it++; + } + } + } +} + +// ----------------------------------------------------------------------- + +CallbackHandle::CallbackHandle(const sp& transactionListener, + const std::vector& ids, const sp& sc) + : listener(transactionListener), callbackIds(ids), surfaceControl(sc) {} + +} // namespace android -- cgit v1.2.3-59-g8ed1b From fda30bb8d79f7406e1d9643e5ec1dc2f695febd1 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Fri, 12 Oct 2018 11:34:28 -0700 Subject: blast: send TransactionStats with callback Add TransactionStats to callback so the client knows when the buffers were released and acquired. Also when transaction was presented and latched. Test: Transaction_test Bug: 80477568 Change-Id: I578a7000193a4401783cb2538172167a552b043f --- libs/gui/ITransactionCompletedListener.cpp | 36 +- .../include/gui/ITransactionCompletedListener.h | 7 +- services/surfaceflinger/BufferStateLayer.cpp | 35 +- services/surfaceflinger/BufferStateLayer.h | 6 +- services/surfaceflinger/Layer.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 1 + .../surfaceflinger/TransactionCompletedThread.cpp | 68 ++- .../surfaceflinger/TransactionCompletedThread.h | 15 +- services/surfaceflinger/tests/Transaction_test.cpp | 637 ++++++++++++++++++++- 9 files changed, 784 insertions(+), 23 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index 3ecd13c28c..95b103839b 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -31,18 +31,50 @@ enum class Tag : uint32_t { } // Anonymous namespace status_t SurfaceStats::writeToParcel(Parcel* output) const { - return output->writeStrongBinder(surfaceControl); + status_t err = output->writeStrongBinder(surfaceControl); + if (err != NO_ERROR) { + return err; + } + err = output->writeInt64(acquireTime); + if (err != NO_ERROR) { + return err; + } + return output->writeBool(releasePreviousBuffer); } status_t SurfaceStats::readFromParcel(const Parcel* input) { - return input->readStrongBinder(&surfaceControl); + status_t err = input->readStrongBinder(&surfaceControl); + if (err != NO_ERROR) { + return err; + } + err = input->readInt64(&acquireTime); + if (err != NO_ERROR) { + return err; + } + return input->readBool(&releasePreviousBuffer); } status_t TransactionStats::writeToParcel(Parcel* output) const { + status_t err = output->writeInt64(latchTime); + if (err != NO_ERROR) { + return err; + } + err = output->writeInt64(presentTime); + if (err != NO_ERROR) { + return err; + } return output->writeParcelableVector(surfaceStats); } status_t TransactionStats::readFromParcel(const Parcel* input) { + status_t err = input->readInt64(&latchTime); + if (err != NO_ERROR) { + return err; + } + err = input->readInt64(&presentTime); + if (err != NO_ERROR) { + return err; + } return input->readParcelableVector(&surfaceStats); } diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index 3c4c393172..5c41c21f63 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -51,9 +51,12 @@ public: status_t readFromParcel(const Parcel* input) override; SurfaceStats() = default; - explicit SurfaceStats(const sp& sc) : surfaceControl(sc) {} + SurfaceStats(const sp& sc, nsecs_t time, bool releasePrevBuffer) + : surfaceControl(sc), acquireTime(time), releasePreviousBuffer(releasePrevBuffer) {} sp surfaceControl; + nsecs_t acquireTime = -1; + bool releasePreviousBuffer = false; }; class TransactionStats : public Parcelable { @@ -61,6 +64,8 @@ public: status_t writeToParcel(Parcel* output) const override; status_t readFromParcel(const Parcel* input) override; + nsecs_t latchTime = -1; + nsecs_t presentTime = -1; std::vector surfaceStats; }; diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index adc91a73e0..2ada70103c 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -41,9 +41,11 @@ BufferStateLayer::~BufferStateLayer() = default; // ----------------------------------------------------------------------- // Interface implementation for Layer // ----------------------------------------------------------------------- -void BufferStateLayer::onLayerDisplayed(const sp& /*releaseFence*/) { - // TODO(marissaw): send the release fence back to buffer owner - return; +void BufferStateLayer::onLayerDisplayed(const sp& releaseFence) { + // The transaction completed callback can only be sent if the release fence from the PREVIOUS + // frame has fired. In practice, we should never actually wait on the previous release fence + // but we should store it just in case. + mPreviousReleaseFence = releaseFence; } void BufferStateLayer::setTransformHint(uint32_t /*orientation*/) const { @@ -52,7 +54,6 @@ void BufferStateLayer::setTransformHint(uint32_t /*orientation*/) const { } void BufferStateLayer::releasePendingBuffer(nsecs_t /*dequeueReadyTime*/) { - // TODO(marissaw): use this to signal the buffer owner return; } @@ -125,7 +126,11 @@ bool BufferStateLayer::setCrop(const Rect& crop) { return true; } -bool BufferStateLayer::setBuffer(sp buffer) { +bool BufferStateLayer::setBuffer(const sp& buffer) { + if (mCurrentState.buffer) { + mReleasePreviousBuffer = true; + } + mCurrentState.sequence++; mCurrentState.buffer = buffer; mCurrentState.modified = true; @@ -134,6 +139,9 @@ bool BufferStateLayer::setBuffer(sp buffer) { } bool BufferStateLayer::setAcquireFence(const sp& fence) { + // The acquire fences of BufferStateLayers have already signaled before they are set + mCallbackHandleAcquireTime = fence->getSignalTime(); + mCurrentState.acquireFence = fence; mCurrentState.modified = true; setTransactionFlags(eTransactionNeeded); @@ -191,16 +199,23 @@ bool BufferStateLayer::setSidebandStream(const sp& sidebandStream) bool BufferStateLayer::setTransactionCompletedListeners( const std::vector>& handles) { - // If there is no handle, we will not send a callback + // If there is no handle, we will not send a callback so reset mReleasePreviousBuffer and return if (handles.empty()) { + mReleasePreviousBuffer = false; return false; } const bool willPresent = willPresentCurrentTransaction(); for (const auto& handle : handles) { + // If this transaction set a buffer on this layer, release its previous buffer + handle->releasePreviousBuffer = mReleasePreviousBuffer; + // If this layer will be presented in this frame if (willPresent) { + // If this transaction set an acquire fence on this layer, set its acquire time + handle->acquireTime = mCallbackHandleAcquireTime; + // Notify the transaction completed thread that there is a pending latched callback // handle mFlinger->getTransactionCompletedThread().registerPendingLatchedCallbackHandle(handle); @@ -214,6 +229,9 @@ bool BufferStateLayer::setTransactionCompletedListeners( } } + mReleasePreviousBuffer = false; + mCallbackHandleAcquireTime = -1; + return willPresent; } @@ -438,8 +456,9 @@ status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nse return BAD_VALUE; } - mFlinger->getTransactionCompletedThread().addLatchedCallbackHandles( - getDrawingState().callbackHandles); + mFlinger->getTransactionCompletedThread() + .addLatchedCallbackHandles(getDrawingState().callbackHandles, latchTime, + mPreviousReleaseFence); // Handle sync fences if (SyncFeatures::getInstance().useNativeFenceSync() && releaseFence != Fence::NO_FENCE) { diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index fcea96dae6..47a046d943 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -62,7 +62,7 @@ public: bool setTransform(uint32_t transform) override; bool setTransformToDisplayInverse(bool transformToDisplayInverse) override; bool setCrop(const Rect& crop) override; - bool setBuffer(sp buffer) override; + bool setBuffer(const sp& buffer) override; bool setAcquireFence(const sp& fence) override; bool setDataspace(ui::Dataspace dataspace) override; bool setHdrMetadata(const HdrMetadata& hdrMetadata) override; @@ -138,7 +138,11 @@ private: uint32_t mFrameNumber{0}; + sp mPreviousReleaseFence; + bool mCurrentStateModified = false; + bool mReleasePreviousBuffer = false; + nsecs_t mCallbackHandleAcquireTime = -1; // TODO(marissaw): support sticky transform for LEGACY camera mode }; diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 9235b645a6..dc9ab3d8ec 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -270,7 +270,7 @@ public: virtual bool setTransform(uint32_t /*transform*/) { return false; }; virtual bool setTransformToDisplayInverse(bool /*transformToDisplayInverse*/) { return false; }; virtual bool setCrop(const Rect& /*crop*/) { return false; }; - virtual bool setBuffer(sp /*buffer*/) { return false; }; + virtual bool setBuffer(const sp& /*buffer*/) { return false; }; virtual bool setAcquireFence(const sp& /*fence*/) { return false; }; virtual bool setDataspace(ui::Dataspace /*dataspace*/) { return false; }; virtual bool setHdrMetadata(const HdrMetadata& /*hdrMetadata*/) { return false; }; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 643b1ce336..7871971554 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1943,6 +1943,7 @@ void SurfaceFlinger::postComposition() } } + mTransactionCompletedThread.addPresentFence(mPreviousPresentFence); mTransactionCompletedThread.sendCallbacks(); } diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index e83422e23b..2c81f0ac86 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -67,9 +67,18 @@ void TransactionCompletedThread::registerPendingLatchedCallbackHandle( } void TransactionCompletedThread::addLatchedCallbackHandles( - const std::deque>& handles) { + const std::deque>& handles, nsecs_t latchTime, + const sp& previousReleaseFence) { std::lock_guard lock(mMutex); + // If the previous release fences have not signaled, something as probably gone wrong. + // Store the fences and check them again before sending a callback. + if (previousReleaseFence && + previousReleaseFence->getSignalTime() == Fence::SIGNAL_TIME_PENDING) { + ALOGD("release fence from the previous frame has not signaled"); + mPreviousReleaseFences.push_back(previousReleaseFence); + } + for (const auto& handle : handles) { auto listener = mPendingTransactions.find(IInterface::asBinder(handle->listener)); auto& pendingCallbacks = listener->second; @@ -83,10 +92,10 @@ void TransactionCompletedThread::addLatchedCallbackHandles( pendingCallbacks.erase(pendingCallback); } } else { - ALOGW("there are more latched callbacks than there were registered callbacks"); + ALOGE("there are more latched callbacks than there were registered callbacks"); } - addCallbackHandle(handle); + addCallbackHandle(handle, latchTime); } } @@ -95,7 +104,8 @@ void TransactionCompletedThread::addUnlatchedCallbackHandle(const sp& handle) { +void TransactionCompletedThread::addCallbackHandle(const sp& handle, + nsecs_t latchTime) { const sp listener = IInterface::asBinder(handle->listener); // If we don't already have a reference to this listener, linkToDeath so we get a notification @@ -112,7 +122,14 @@ void TransactionCompletedThread::addCallbackHandle(const sp& han listenerStats.listener = handle->listener; auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; - transactionStats.surfaceStats.emplace_back(handle->surfaceControl); + transactionStats.latchTime = latchTime; + transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, + handle->releasePreviousBuffer); +} + +void TransactionCompletedThread::addPresentFence(const sp& presentFence) { + std::lock_guard lock(mMutex); + mPresentFence = presentFence; } void TransactionCompletedThread::sendCallbacks() { @@ -128,6 +145,27 @@ void TransactionCompletedThread::threadMain() { while (mKeepRunning) { mConditionVariable.wait(mMutex); + // Present fence should fire almost immediately. If the fence has not signaled in 100ms, + // there is a major problem and it will probably never fire. + nsecs_t presentTime = -1; + if (mPresentFence) { + status_t status = mPresentFence->wait(100); + if (status == NO_ERROR) { + presentTime = mPresentFence->getSignalTime(); + } else { + ALOGE("present fence has not signaled, err %d", status); + } + } + + // We should never hit this case. The release fences from the previous frame should have + // signaled long before the current frame is presented. + for (const auto& fence : mPreviousReleaseFences) { + status_t status = fence->wait(100); + if (status != NO_ERROR) { + ALOGE("previous release fence has not signaled, err %d", status); + } + } + // For each listener auto it = mListenerStats.begin(); while (it != mListenerStats.end()) { @@ -141,6 +179,21 @@ void TransactionCompletedThread::threadMain() { sendCallback = false; break; } + + // If the transaction has been latched + if (transactionStats.latchTime >= 0) { + // If the present time is < 0, this transaction has been latched but not + // presented. Skip it for now. This can happen when a new transaction comes + // in between the latch and present steps. sendCallbacks is called by + // SurfaceFlinger when the transaction is received to ensure that if the + // transaction that didn't update state it still got a callback. + if (presentTime < 0) { + sendCallback = false; + break; + } + + transactionStats.presentTime = presentTime; + } } // If the listener has no pending transactions and all latched transactions have been // presented @@ -156,6 +209,11 @@ void TransactionCompletedThread::threadMain() { it++; } } + + if (mPresentFence) { + mPresentFence.clear(); + mPreviousReleaseFences.clear(); + } } } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 39cf7e877c..5af4097665 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -26,6 +26,7 @@ #include #include +#include namespace android { @@ -37,6 +38,9 @@ public: sp listener; std::vector callbackIds; sp surfaceControl; + + bool releasePreviousBuffer = false; + nsecs_t acquireTime = -1; }; class TransactionCompletedThread { @@ -52,18 +56,22 @@ public: // presented. void registerPendingLatchedCallbackHandle(const sp& handle); // Notifies the TransactionCompletedThread that a pending CallbackHandle has been latched. - void addLatchedCallbackHandles(const std::deque>& handles); + void addLatchedCallbackHandles(const std::deque>& handles, nsecs_t latchTime, + const sp& previousReleaseFence); // Adds the Transaction CallbackHandle from a layer that does not need to be relatched and // presented this frame. void addUnlatchedCallbackHandle(const sp& handle); + void addPresentFence(const sp& presentFence); + void sendCallbacks(); private: void threadMain(); - void addCallbackHandle(const sp& handle) REQUIRES(mMutex); + void addCallbackHandle(const sp& handle, nsecs_t latchTime = -1) + REQUIRES(mMutex); class ThreadDeathRecipient : public IBinder::DeathRecipient { public: @@ -97,6 +105,9 @@ private: bool mRunning GUARDED_BY(mMutex) = false; bool mKeepRunning GUARDED_BY(mMutex) = true; + + sp mPresentFence GUARDED_BY(mMutex); + std::vector> mPreviousReleaseFences GUARDED_BY(mMutex); }; } // namespace android diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index 94b33ac2d5..be8d9395b3 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -15,9 +15,12 @@ */ #include +#include +#include #include #include #include +#include #include @@ -64,6 +67,7 @@ const Color Color::BLACK{0, 0, 0, 255}; const Color Color::TRANSPARENT{0, 0, 0, 0}; using android::hardware::graphics::common::V1_1::BufferUsage; +using namespace std::chrono_literals; std::ostream& operator<<(std::ostream& os, const Color& color) { os << int(color.r) << ", " << int(color.g) << ", " << int(color.b) << ", " << int(color.a); @@ -327,10 +331,11 @@ protected: mClient = 0; } - virtual sp createLayer(const char* name, uint32_t width, uint32_t height, + virtual sp createLayer(const sp& client, + const char* name, uint32_t width, uint32_t height, uint32_t flags = 0) { auto layer = - mClient->createSurface(String8(name), width, height, PIXEL_FORMAT_RGBA_8888, flags); + client->createSurface(String8(name), width, height, PIXEL_FORMAT_RGBA_8888, flags); EXPECT_NE(nullptr, layer.get()) << "failed to create SurfaceControl"; status_t error = Transaction() @@ -345,6 +350,11 @@ protected: return layer; } + virtual sp createLayer(const char* name, uint32_t width, uint32_t height, + uint32_t flags = 0) { + return createLayer(mClient, name, width, height, flags); + } + ANativeWindow_Buffer getBufferQueueLayerBuffer(const sp& layer) { // wait for previous transactions (such as setSize) to complete Transaction().apply(true); @@ -1953,7 +1963,7 @@ TEST_F(LayerTransactionTest, SetFenceBasic_BufferState) { "test"); fillGraphicBufferColor(buffer, Rect(0, 0, 32, 32), Color::RED); - sp fence = new Fence(-1); + sp fence = Fence::NO_FENCE; Transaction() .setBuffer(layer, buffer) @@ -2156,6 +2166,627 @@ TEST_F(LayerTransactionTest, SetColorTransformBasic) { } } +class ExpectedResult { +public: + enum Transaction { + NOT_PRESENTED = 0, + PRESENTED, + }; + + enum Buffer { + NOT_ACQUIRED = 0, + ACQUIRED, + }; + + enum PreviousBuffer { + NOT_RELEASED = 0, + RELEASED, + }; + + void reset() { + mTransactionResult = ExpectedResult::Transaction::NOT_PRESENTED; + mExpectedSurfaceResults.clear(); + } + + void addSurface(ExpectedResult::Transaction transactionResult, const sp& layer, + ExpectedResult::Buffer bufferResult = NOT_ACQUIRED, + ExpectedResult::PreviousBuffer previousBufferResult = NOT_RELEASED) { + mTransactionResult = transactionResult; + mExpectedSurfaceResults.emplace(std::piecewise_construct, + std::forward_as_tuple(layer->getHandle()), + std::forward_as_tuple(bufferResult, previousBufferResult)); + } + + void addSurfaces(ExpectedResult::Transaction transactionResult, + const std::vector>& layers, + ExpectedResult::Buffer bufferResult = NOT_ACQUIRED, + ExpectedResult::PreviousBuffer previousBufferResult = NOT_RELEASED) { + for (const auto& layer : layers) { + addSurface(transactionResult, layer, bufferResult, previousBufferResult); + } + } + + void verifyTransactionStats(const TransactionStats& transactionStats) const { + const auto& [latchTime, presentTime, surfaceStats] = transactionStats; + if (mTransactionResult == ExpectedResult::Transaction::PRESENTED) { + ASSERT_GE(latchTime, 0) << "bad latch time"; + ASSERT_GE(presentTime, 0) << "bad present time"; + } else { + ASSERT_EQ(presentTime, -1) << "transaction shouldn't have been presented"; + ASSERT_EQ(latchTime, -1) << "unpresented transactions shouldn't be latched"; + } + + ASSERT_EQ(surfaceStats.size(), mExpectedSurfaceResults.size()) + << "wrong number of surfaces"; + + for (const auto& stats : surfaceStats) { + const auto& expectedSurfaceResult = mExpectedSurfaceResults.find(stats.surfaceControl); + ASSERT_NE(expectedSurfaceResult, mExpectedSurfaceResults.end()) + << "unexpected surface control"; + expectedSurfaceResult->second.verifySurfaceStats(stats, latchTime); + } + } + +private: + class ExpectedSurfaceResult { + public: + ExpectedSurfaceResult(ExpectedResult::Buffer bufferResult, + ExpectedResult::PreviousBuffer previousBufferResult) + : mBufferResult(bufferResult), mPreviousBufferResult(previousBufferResult) {} + + void verifySurfaceStats(const SurfaceStats& surfaceStats, nsecs_t latchTime) const { + const auto& [surfaceControl, acquireTime, releasePreviousBuffer] = surfaceStats; + + ASSERT_EQ(acquireTime > 0, mBufferResult == ExpectedResult::Buffer::ACQUIRED) + << "bad acquire time"; + ASSERT_LE(acquireTime, latchTime) << "acquire time should be <= latch time"; + ASSERT_EQ(releasePreviousBuffer, + mPreviousBufferResult == ExpectedResult::PreviousBuffer::RELEASED) + << "bad previous buffer released"; + } + + private: + ExpectedResult::Buffer mBufferResult; + ExpectedResult::PreviousBuffer mPreviousBufferResult; + }; + + struct IBinderHash { + std::size_t operator()(const sp& strongPointer) const { + return std::hash{}(strongPointer.get()); + } + }; + ExpectedResult::Transaction mTransactionResult = ExpectedResult::Transaction::NOT_PRESENTED; + std::unordered_map, ExpectedSurfaceResult, IBinderHash> mExpectedSurfaceResults; +}; + +class CallbackHelper { +public: + static void function(void* callbackContext, const TransactionStats& transactionStats) { + if (!callbackContext) { + ALOGE("failed to get callback context"); + } + CallbackHelper* helper = static_cast(callbackContext); + std::lock_guard lock(helper->mMutex); + helper->mTransactionStatsQueue.push(transactionStats); + helper->mConditionVariable.notify_all(); + } + + void getTransactionStats(TransactionStats* outStats) { + std::unique_lock lock(mMutex); + + if (mTransactionStatsQueue.empty()) { + ASSERT_NE(mConditionVariable.wait_for(lock, std::chrono::seconds(3)), + std::cv_status::timeout) + << "did not receive callback"; + } + + *outStats = std::move(mTransactionStatsQueue.front()); + mTransactionStatsQueue.pop(); + } + + void verifyFinalState() { + // Wait to see if there are extra callbacks + std::this_thread::sleep_for(500ms); + + std::lock_guard lock(mMutex); + EXPECT_EQ(mTransactionStatsQueue.size(), 0) << "extra callbacks received"; + mTransactionStatsQueue = {}; + } + + void* getContext() { return static_cast(this); } + + std::mutex mMutex; + std::condition_variable mConditionVariable; + std::queue mTransactionStatsQueue; +}; + +class LayerCallbackTest : public LayerTransactionTest { +protected: + virtual sp createBufferStateLayer() { + return createLayer(mClient, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState); + } + + virtual void fillTransaction(Transaction& transaction, CallbackHelper* callbackHelper, + const sp& layer = nullptr) { + if (layer) { + sp buffer = + new GraphicBuffer(mWidth, mHeight, PIXEL_FORMAT_RGBA_8888, 1, + BufferUsage::CPU_READ_OFTEN | BufferUsage::CPU_WRITE_OFTEN | + BufferUsage::COMPOSER_OVERLAY | + BufferUsage::GPU_TEXTURE, + "test"); + fillGraphicBufferColor(buffer, Rect(0, 0, mWidth, mHeight), Color::RED); + + sp fence = new Fence(-1); + + transaction.setBuffer(layer, buffer) + .setAcquireFence(layer, fence) + .setSize(layer, mWidth, mHeight); + } + + transaction.addTransactionCompletedCallback(callbackHelper->function, + callbackHelper->getContext()); + } + + void waitForCallback(CallbackHelper& helper, const ExpectedResult& expectedResult, + bool finalState = false) { + TransactionStats transactionStats; + ASSERT_NO_FATAL_FAILURE(helper.getTransactionStats(&transactionStats)); + EXPECT_NO_FATAL_FAILURE(expectedResult.verifyTransactionStats(transactionStats)); + + if (finalState) { + ASSERT_NO_FATAL_FAILURE(helper.verifyFinalState()); + } + } + + void waitForCallbacks(CallbackHelper& helper, + const std::vector& expectedResults, + bool finalState = false) { + for (const auto& expectedResult : expectedResults) { + waitForCallback(helper, expectedResult); + } + if (finalState) { + ASSERT_NO_FATAL_FAILURE(helper.verifyFinalState()); + } + } + + uint32_t mWidth = 32; + uint32_t mHeight = 32; +}; + +TEST_F(LayerCallbackTest, Basic) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + fillTransaction(transaction, &callback, layer); + + transaction.apply(); + + ExpectedResult expected; + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true)); +} + +TEST_F(LayerCallbackTest, NoBuffer) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + fillTransaction(transaction, &callback); + + transaction.setPosition(layer, mWidth, mHeight).apply(); + + ExpectedResult expected; + expected.addSurface(ExpectedResult::Transaction::NOT_PRESENTED, layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true)); +} + +TEST_F(LayerCallbackTest, NoStateChange) { + Transaction transaction; + CallbackHelper callback; + fillTransaction(transaction, &callback); + + transaction.apply(); + + ExpectedResult expected; + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true)); +} + +TEST_F(LayerCallbackTest, OffScreen) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + fillTransaction(transaction, &callback, layer); + + transaction.setPosition(layer, -100, -100).apply(); + + ExpectedResult expected; + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true)); +} + +TEST_F(LayerCallbackTest, Merge) { + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createBufferStateLayer()); + ASSERT_NO_FATAL_FAILURE(layer2 = createBufferStateLayer()); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, Merge_SameCallback) { + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createBufferStateLayer()); + ASSERT_NO_FATAL_FAILURE(layer2 = createBufferStateLayer()); + + Transaction transaction1, transaction2; + CallbackHelper callback; + fillTransaction(transaction1, &callback, layer1); + fillTransaction(transaction2, &callback, layer2); + + transaction2.merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true)); +} + +TEST_F(LayerCallbackTest, Merge_SameLayer) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + fillTransaction(transaction1, &callback1, layer); + fillTransaction(transaction2, &callback2, layer); + + transaction2.merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, Merge_SingleBuffer) { + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createBufferStateLayer()); + ASSERT_NO_FATAL_FAILURE(layer2 = createBufferStateLayer()); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, Merge_DifferentClients) { + sp client1(new SurfaceComposerClient), + client2(new SurfaceComposerClient); + + ASSERT_EQ(NO_ERROR, client1->initCheck()) << "failed to create SurfaceComposerClient"; + ASSERT_EQ(NO_ERROR, client2->initCheck()) << "failed to create SurfaceComposerClient"; + + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createLayer(client1, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + ASSERT_NO_FATAL_FAILURE(layer2 = createLayer(client2, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, MultipleTransactions) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + for (size_t i = 0; i < 10; i++) { + fillTransaction(transaction, &callback, layer); + + transaction.apply(); + + ExpectedResult expected; + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, + ExpectedResult::Buffer::NOT_ACQUIRED, + (i == 0) ? ExpectedResult::PreviousBuffer::NOT_RELEASED + : ExpectedResult::PreviousBuffer::RELEASED); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected)); + } + ASSERT_NO_FATAL_FAILURE(callback.verifyFinalState()); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_NoStateChange) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + for (size_t i = 0; i < 10; i++) { + ExpectedResult expected; + + if (i == 0) { + fillTransaction(transaction, &callback, layer); + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + } else { + fillTransaction(transaction, &callback); + } + + transaction.apply(); + + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected)); + } + ASSERT_NO_FATAL_FAILURE(callback.verifyFinalState()); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_SameStateChange) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + for (size_t i = 0; i < 10; i++) { + if (i == 0) { + fillTransaction(transaction, &callback, layer); + } else { + fillTransaction(transaction, &callback); + } + + transaction.setPosition(layer, mWidth, mHeight).apply(); + + ExpectedResult expected; + expected.addSurface((i == 0) ? ExpectedResult::Transaction::PRESENTED + : ExpectedResult::Transaction::NOT_PRESENTED, + layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, i == 0)); + } + ASSERT_NO_FATAL_FAILURE(callback.verifyFinalState()); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_Merge) { + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createBufferStateLayer()); + ASSERT_NO_FATAL_FAILURE(layer2 = createBufferStateLayer()); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + for (size_t i = 0; i < 10; i++) { + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}, + ExpectedResult::Buffer::NOT_ACQUIRED, + (i == 0) ? ExpectedResult::PreviousBuffer::NOT_RELEASED + : ExpectedResult::PreviousBuffer::RELEASED); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected)); + } + ASSERT_NO_FATAL_FAILURE(callback1.verifyFinalState()); + ASSERT_NO_FATAL_FAILURE(callback2.verifyFinalState()); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_Merge_DifferentClients) { + sp client1(new SurfaceComposerClient), + client2(new SurfaceComposerClient); + ASSERT_EQ(NO_ERROR, client1->initCheck()) << "failed to create SurfaceComposerClient"; + ASSERT_EQ(NO_ERROR, client2->initCheck()) << "failed to create SurfaceComposerClient"; + + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createLayer(client1, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + ASSERT_NO_FATAL_FAILURE(layer2 = createLayer(client2, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + for (size_t i = 0; i < 10; i++) { + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}, + ExpectedResult::Buffer::NOT_ACQUIRED, + (i == 0) ? ExpectedResult::PreviousBuffer::NOT_RELEASED + : ExpectedResult::PreviousBuffer::RELEASED); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected)); + } + ASSERT_NO_FATAL_FAILURE(callback1.verifyFinalState()); + ASSERT_NO_FATAL_FAILURE(callback2.verifyFinalState()); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_Merge_DifferentClients_NoStateChange) { + sp client1(new SurfaceComposerClient), + client2(new SurfaceComposerClient); + ASSERT_EQ(NO_ERROR, client1->initCheck()) << "failed to create SurfaceComposerClient"; + ASSERT_EQ(NO_ERROR, client2->initCheck()) << "failed to create SurfaceComposerClient"; + + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createLayer(client1, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + ASSERT_NO_FATAL_FAILURE(layer2 = createLayer(client2, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + + // Normal call to set up test + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); + expected.reset(); + + // Test + fillTransaction(transaction1, &callback1); + fillTransaction(transaction2, &callback2); + + transaction2.merge(std::move(transaction1)).apply(); + + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_Merge_DifferentClients_SameStateChange) { + sp client1(new SurfaceComposerClient), + client2(new SurfaceComposerClient); + + ASSERT_EQ(NO_ERROR, client1->initCheck()) << "failed to create SurfaceComposerClient"; + ASSERT_EQ(NO_ERROR, client2->initCheck()) << "failed to create SurfaceComposerClient"; + + sp layer1, layer2; + ASSERT_NO_FATAL_FAILURE(layer1 = createLayer(client1, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + ASSERT_NO_FATAL_FAILURE(layer2 = createLayer(client2, "test", mWidth, mHeight, + ISurfaceComposerClient::eFXSurfaceBufferState)); + + Transaction transaction1, transaction2; + CallbackHelper callback1, callback2; + + // Normal call to set up test + fillTransaction(transaction1, &callback1, layer1); + fillTransaction(transaction2, &callback2, layer2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + ExpectedResult expected; + expected.addSurfaces(ExpectedResult::Transaction::PRESENTED, {layer1, layer2}); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); + expected.reset(); + + // Test + fillTransaction(transaction1, &callback1); + fillTransaction(transaction2, &callback2); + + transaction2.setPosition(layer2, mWidth, mHeight).merge(std::move(transaction1)).apply(); + + expected.addSurface(ExpectedResult::Transaction::NOT_PRESENTED, layer2); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback1, expected, true)); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback2, expected, true)); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_SingleFrame) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + std::vector expectedResults(50); + ExpectedResult::PreviousBuffer previousBufferResult = + ExpectedResult::PreviousBuffer::NOT_RELEASED; + for (auto& expected : expectedResults) { + expected.reset(); + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, + ExpectedResult::Buffer::NOT_ACQUIRED, previousBufferResult); + previousBufferResult = ExpectedResult::PreviousBuffer::RELEASED; + + fillTransaction(transaction, &callback, layer); + + transaction.apply(); + std::this_thread::sleep_for(200ms); + } + EXPECT_NO_FATAL_FAILURE(waitForCallbacks(callback, expectedResults, true)); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_SingleFrame_NoStateChange) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + Transaction transaction; + CallbackHelper callback; + std::vector expectedResults(50); + bool first = true; + for (auto& expected : expectedResults) { + expected.reset(); + + if (first) { + fillTransaction(transaction, &callback, layer); + expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + first = false; + } else { + fillTransaction(transaction, &callback); + } + + transaction.apply(); + std::this_thread::sleep_for(200ms); + } + EXPECT_NO_FATAL_FAILURE(waitForCallbacks(callback, expectedResults, true)); +} + +TEST_F(LayerCallbackTest, MultipleTransactions_SingleFrame_SameStateChange) { + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer()); + + // Normal call to set up test + Transaction transaction; + CallbackHelper callback; + fillTransaction(transaction, &callback, layer); + + transaction.setPosition(layer, mWidth, mHeight).apply(); + + ExpectedResult expectedResult; + expectedResult.addSurface(ExpectedResult::Transaction::PRESENTED, layer); + EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expectedResult, true)); + + // Test + std::vector expectedResults(50); + for (auto& expected : expectedResults) { + expected.reset(); + expected.addSurface(ExpectedResult::Transaction::NOT_PRESENTED, layer); + + fillTransaction(transaction, &callback); + + transaction.setPosition(layer, mWidth, mHeight).apply(); + + std::this_thread::sleep_for(200ms); + } + EXPECT_NO_FATAL_FAILURE(waitForCallbacks(callback, expectedResults, true)); +} + class LayerUpdateTest : public LayerTransactionTest { protected: virtual void SetUp() { -- cgit v1.2.3-59-g8ed1b From 05d9dd3432d9574b01f5bfd6ef1d1970aa00652a Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Tue, 13 Nov 2018 10:05:14 -0800 Subject: blast: fix crash TransactionCompletedThread When TransactionCompletedThread is destroyed, the std::thread is signaled to exit and then joined. If the thread was never started, the join call throws an exception. To prevent the exception, check if the thread is joinable first. Test: libsurfaceflinger_unittest Change-Id: I6372bf4fff4bc04383a9018dac01b36d1c046001 --- services/surfaceflinger/TransactionCompletedThread.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 2c81f0ac86..9b9dc57781 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -36,7 +36,9 @@ TransactionCompletedThread::~TransactionCompletedThread() { mConditionVariable.notify_all(); } - mThread.join(); + if (mThread.joinable()) { + mThread.join(); + } { std::lock_guard lock(mMutex); -- cgit v1.2.3-59-g8ed1b From 99343baea449f07cc2ac0ac473671ce53be7cdd6 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Tue, 13 Nov 2018 10:39:08 -0800 Subject: blast: protect destruction of callback thread Wrap the creation and destruction of TransactionCompletedThread's mThread in a mutex. This will prevent the race condition between the creation of mThread and mThread.join(). Test: libsurfaceflinger_unittest Transaction_test Change-Id: Ic448872cd5418ce7885b8c2e52a7f1bf02afd772 --- services/surfaceflinger/TransactionCompletedThread.cpp | 6 +++++- services/surfaceflinger/TransactionCompletedThread.h | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 9b9dc57781..389118a0d7 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -30,6 +30,8 @@ namespace android { TransactionCompletedThread::~TransactionCompletedThread() { + std::lock_guard lockThread(mThreadMutex); + { std::lock_guard lock(mMutex); mKeepRunning = false; @@ -50,11 +52,13 @@ TransactionCompletedThread::~TransactionCompletedThread() { void TransactionCompletedThread::run() { std::lock_guard lock(mMutex); - if (mRunning) { + if (mRunning || !mKeepRunning) { return; } mDeathRecipient = new ThreadDeathRecipient(); mRunning = true; + + std::lock_guard lockThread(mThreadMutex); mThread = std::thread(&TransactionCompletedThread::threadMain, this); } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 5af4097665..1612f69d00 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -90,7 +90,10 @@ private: } }; - std::thread mThread; + // Protects the creation and destruction of mThread + std::mutex mThreadMutex; + + std::thread mThread GUARDED_BY(mThreadMutex); std::mutex mMutex; std::condition_variable_any mConditionVariable; -- cgit v1.2.3-59-g8ed1b From 63258a15957693a3278263d4ee5929cf27b2ab2d Mon Sep 17 00:00:00 2001 From: Valerie Hau Date: Fri, 14 Dec 2018 14:31:48 -0800 Subject: Modifying TransactionCompletedListener to pass back present fence Bug: 120919468 Test: build, boot, SurfaceFlinger_test Change-Id: Id3d3b34ffa30291f3dd27040bf97ccd1492e7f9d --- libs/gui/ITransactionCompletedListener.cpp | 20 ++++++++++++++++++-- .../include/gui/ITransactionCompletedListener.h | 3 ++- .../surfaceflinger/TransactionCompletedThread.cpp | 22 ++-------------------- services/surfaceflinger/tests/Transaction_test.cpp | 6 +++--- 4 files changed, 25 insertions(+), 26 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index 95b103839b..1be55e6f8a 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -59,7 +59,15 @@ status_t TransactionStats::writeToParcel(Parcel* output) const { if (err != NO_ERROR) { return err; } - err = output->writeInt64(presentTime); + if (presentFence) { + err = output->writeBool(true); + if (err != NO_ERROR) { + return err; + } + err = output->write(*presentFence); + } else { + err = output->writeBool(false); + } if (err != NO_ERROR) { return err; } @@ -71,10 +79,18 @@ status_t TransactionStats::readFromParcel(const Parcel* input) { if (err != NO_ERROR) { return err; } - err = input->readInt64(&presentTime); + bool hasFence = false; + err = input->readBool(&hasFence); if (err != NO_ERROR) { return err; } + if (hasFence) { + presentFence = new Fence(); + err = input->read(*presentFence); + if (err != NO_ERROR) { + return err; + } + } return input->readParcelableVector(&surfaceStats); } diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index 5c41c21f63..8acfa7a8f6 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -65,7 +66,7 @@ public: status_t readFromParcel(const Parcel* input) override; nsecs_t latchTime = -1; - nsecs_t presentTime = -1; + sp presentFence = nullptr; std::vector surfaceStats; }; diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 389118a0d7..a1a86923f3 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -151,18 +151,6 @@ void TransactionCompletedThread::threadMain() { while (mKeepRunning) { mConditionVariable.wait(mMutex); - // Present fence should fire almost immediately. If the fence has not signaled in 100ms, - // there is a major problem and it will probably never fire. - nsecs_t presentTime = -1; - if (mPresentFence) { - status_t status = mPresentFence->wait(100); - if (status == NO_ERROR) { - presentTime = mPresentFence->getSignalTime(); - } else { - ALOGE("present fence has not signaled, err %d", status); - } - } - // We should never hit this case. The release fences from the previous frame should have // signaled long before the current frame is presented. for (const auto& fence : mPreviousReleaseFences) { @@ -188,17 +176,11 @@ void TransactionCompletedThread::threadMain() { // If the transaction has been latched if (transactionStats.latchTime >= 0) { - // If the present time is < 0, this transaction has been latched but not - // presented. Skip it for now. This can happen when a new transaction comes - // in between the latch and present steps. sendCallbacks is called by - // SurfaceFlinger when the transaction is received to ensure that if the - // transaction that didn't update state it still got a callback. - if (presentTime < 0) { + if (!mPresentFence) { sendCallback = false; break; } - - transactionStats.presentTime = presentTime; + transactionStats.presentFence = mPresentFence; } } // If the listener has no pending transactions and all latched transactions have been diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index cef598cb56..e62fc6e6c8 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -2502,12 +2502,12 @@ public: } void verifyTransactionStats(const TransactionStats& transactionStats) const { - const auto& [latchTime, presentTime, surfaceStats] = transactionStats; + const auto& [latchTime, presentFence, surfaceStats] = transactionStats; if (mTransactionResult == ExpectedResult::Transaction::PRESENTED) { ASSERT_GE(latchTime, 0) << "bad latch time"; - ASSERT_GE(presentTime, 0) << "bad present time"; + ASSERT_NE(presentFence, nullptr); } else { - ASSERT_EQ(presentTime, -1) << "transaction shouldn't have been presented"; + ASSERT_EQ(presentFence, nullptr) << "transaction shouldn't have been presented"; ASSERT_EQ(latchTime, -1) << "unpresented transactions shouldn't be latched"; } -- cgit v1.2.3-59-g8ed1b From 5a68a77923216e3f889776d1398880c6532f834a Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Sat, 22 Dec 2018 17:43:42 -0800 Subject: blast: send back previous release fences When sending a transaction callback, send a previous release fence instead of a boolean. Test: Transaction_test Bug: 80477568, 120930690 Change-Id: I608fecc3cd31fd92fcfc2abb5fc084c529ee7806 --- libs/gui/ITransactionCompletedListener.cpp | 25 +++++++++++-- .../include/gui/ITransactionCompletedListener.h | 6 ++-- services/surfaceflinger/BufferStateLayer.cpp | 41 ++++++++++++++++------ .../surfaceflinger/TransactionCompletedThread.cpp | 37 +++++-------------- .../surfaceflinger/TransactionCompletedThread.h | 15 ++++---- services/surfaceflinger/tests/Transaction_test.cpp | 20 ++++++----- 6 files changed, 84 insertions(+), 60 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index 1be55e6f8a..ce88d7b47d 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -39,7 +39,16 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { if (err != NO_ERROR) { return err; } - return output->writeBool(releasePreviousBuffer); + if (previousReleaseFence) { + err = output->writeBool(true); + if (err != NO_ERROR) { + return err; + } + err = output->write(*previousReleaseFence); + } else { + err = output->writeBool(false); + } + return err; } status_t SurfaceStats::readFromParcel(const Parcel* input) { @@ -51,7 +60,19 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { if (err != NO_ERROR) { return err; } - return input->readBool(&releasePreviousBuffer); + bool hasFence = false; + err = input->readBool(&hasFence); + if (err != NO_ERROR) { + return err; + } + if (hasFence) { + previousReleaseFence = new Fence(); + err = input->read(*previousReleaseFence); + if (err != NO_ERROR) { + return err; + } + } + return NO_ERROR; } status_t TransactionStats::writeToParcel(Parcel* output) const { diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index 8acfa7a8f6..29ab026928 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -52,12 +52,12 @@ public: status_t readFromParcel(const Parcel* input) override; SurfaceStats() = default; - SurfaceStats(const sp& sc, nsecs_t time, bool releasePrevBuffer) - : surfaceControl(sc), acquireTime(time), releasePreviousBuffer(releasePrevBuffer) {} + SurfaceStats(const sp& sc, nsecs_t time, const sp& prevReleaseFence) + : surfaceControl(sc), acquireTime(time), previousReleaseFence(prevReleaseFence) {} sp surfaceControl; nsecs_t acquireTime = -1; - bool releasePreviousBuffer = false; + sp previousReleaseFence; }; class TransactionStats : public Parcelable { diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index e0d9d23c5b..2132f5979e 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -48,10 +48,28 @@ BufferStateLayer::~BufferStateLayer() = default; // Interface implementation for Layer // ----------------------------------------------------------------------- void BufferStateLayer::onLayerDisplayed(const sp& releaseFence) { - // The transaction completed callback can only be sent if the release fence from the PREVIOUS - // frame has fired. In practice, we should never actually wait on the previous release fence - // but we should store it just in case. - mPreviousReleaseFence = releaseFence; + // The previous release fence notifies the client that SurfaceFlinger is done with the previous + // buffer that was presented on this layer. The first transaction that came in this frame that + // replaced the previous buffer on this layer needs this release fence, because the fence will + // let the client know when that previous buffer is removed from the screen. + // + // Every other transaction on this layer does not need a release fence because no other + // Transactions that were set on this layer this frame are going to have their preceeding buffer + // removed from the display this frame. + // + // For example, if we have 3 transactions this frame. The first transaction doesn't contain a + // buffer so it doesn't need a previous release fence because the layer still needs the previous + // buffer. The second transaction contains a buffer so it needs a previous release fence because + // the previous buffer will be released this frame. The third transaction also contains a + // buffer. It replaces the buffer in the second transaction. The buffer in the second + // transaction will now no longer be presented so it is released immediately and the third + // transaction doesn't need a previous release fence. + for (auto& handle : mDrawingState.callbackHandles) { + if (handle->releasePreviousBuffer) { + handle->previousReleaseFence = releaseFence; + break; + } + } } void BufferStateLayer::setTransformHint(uint32_t /*orientation*/) const { @@ -60,7 +78,10 @@ void BufferStateLayer::setTransformHint(uint32_t /*orientation*/) const { } void BufferStateLayer::releasePendingBuffer(nsecs_t /*dequeueReadyTime*/) { - return; + mFlinger->getTransactionCompletedThread().addPresentedCallbackHandles( + mDrawingState.callbackHandles); + + mDrawingState.callbackHandles = {}; } bool BufferStateLayer::shouldPresentNow(nsecs_t /*expectedPresentTime*/) const { @@ -257,14 +278,14 @@ bool BufferStateLayer::setTransactionCompletedListeners( // Notify the transaction completed thread that there is a pending latched callback // handle - mFlinger->getTransactionCompletedThread().registerPendingLatchedCallbackHandle(handle); + mFlinger->getTransactionCompletedThread().registerPendingCallbackHandle(handle); // Store so latched time and release fence can be set mCurrentState.callbackHandles.push_back(handle); } else { // If this layer will NOT need to be relatched and presented this frame // Notify the transaction completed thread this handle is done - mFlinger->getTransactionCompletedThread().addUnlatchedCallbackHandle(handle); + mFlinger->getTransactionCompletedThread().addUnpresentedCallbackHandle(handle); } } @@ -504,9 +525,9 @@ status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nse return BAD_VALUE; } - mFlinger->getTransactionCompletedThread() - .addLatchedCallbackHandles(getDrawingState().callbackHandles, latchTime, - mPreviousReleaseFence); + for (auto& handle : mDrawingState.callbackHandles) { + handle->latchTime = latchTime; + } // Handle sync fences if (SyncFeatures::getInstance().useNativeFenceSync() && releaseFence != Fence::NO_FENCE) { diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index a1a86923f3..d2b7fe099f 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -62,8 +62,7 @@ void TransactionCompletedThread::run() { mThread = std::thread(&TransactionCompletedThread::threadMain, this); } -void TransactionCompletedThread::registerPendingLatchedCallbackHandle( - const sp& handle) { +void TransactionCompletedThread::registerPendingCallbackHandle(const sp& handle) { std::lock_guard lock(mMutex); sp listener = IInterface::asBinder(handle->listener); @@ -72,19 +71,10 @@ void TransactionCompletedThread::registerPendingLatchedCallbackHandle( mPendingTransactions[listener][callbackIds]++; } -void TransactionCompletedThread::addLatchedCallbackHandles( - const std::deque>& handles, nsecs_t latchTime, - const sp& previousReleaseFence) { +void TransactionCompletedThread::addPresentedCallbackHandles( + const std::deque>& handles) { std::lock_guard lock(mMutex); - // If the previous release fences have not signaled, something as probably gone wrong. - // Store the fences and check them again before sending a callback. - if (previousReleaseFence && - previousReleaseFence->getSignalTime() == Fence::SIGNAL_TIME_PENDING) { - ALOGD("release fence from the previous frame has not signaled"); - mPreviousReleaseFences.push_back(previousReleaseFence); - } - for (const auto& handle : handles) { auto listener = mPendingTransactions.find(IInterface::asBinder(handle->listener)); auto& pendingCallbacks = listener->second; @@ -101,17 +91,16 @@ void TransactionCompletedThread::addLatchedCallbackHandles( ALOGE("there are more latched callbacks than there were registered callbacks"); } - addCallbackHandle(handle, latchTime); + addCallbackHandle(handle); } } -void TransactionCompletedThread::addUnlatchedCallbackHandle(const sp& handle) { +void TransactionCompletedThread::addUnpresentedCallbackHandle(const sp& handle) { std::lock_guard lock(mMutex); addCallbackHandle(handle); } -void TransactionCompletedThread::addCallbackHandle(const sp& handle, - nsecs_t latchTime) { +void TransactionCompletedThread::addCallbackHandle(const sp& handle) { const sp listener = IInterface::asBinder(handle->listener); // If we don't already have a reference to this listener, linkToDeath so we get a notification @@ -128,9 +117,9 @@ void TransactionCompletedThread::addCallbackHandle(const sp& han listenerStats.listener = handle->listener; auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; - transactionStats.latchTime = latchTime; + transactionStats.latchTime = handle->latchTime; transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, - handle->releasePreviousBuffer); + handle->previousReleaseFence); } void TransactionCompletedThread::addPresentFence(const sp& presentFence) { @@ -151,15 +140,6 @@ void TransactionCompletedThread::threadMain() { while (mKeepRunning) { mConditionVariable.wait(mMutex); - // We should never hit this case. The release fences from the previous frame should have - // signaled long before the current frame is presented. - for (const auto& fence : mPreviousReleaseFences) { - status_t status = fence->wait(100); - if (status != NO_ERROR) { - ALOGE("previous release fence has not signaled, err %d", status); - } - } - // For each listener auto it = mListenerStats.begin(); while (it != mListenerStats.end()) { @@ -200,7 +180,6 @@ void TransactionCompletedThread::threadMain() { if (mPresentFence) { mPresentFence.clear(); - mPreviousReleaseFences.clear(); } } } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 1612f69d00..f49306d70e 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -40,7 +40,9 @@ public: sp surfaceControl; bool releasePreviousBuffer = false; + sp previousReleaseFence; nsecs_t acquireTime = -1; + nsecs_t latchTime = -1; }; class TransactionCompletedThread { @@ -54,14 +56,13 @@ public: // layer has received the CallbackHandle so the TransactionCompletedThread knows not to send // a callback for that Listener/Transaction pair until that CallbackHandle has been latched and // presented. - void registerPendingLatchedCallbackHandle(const sp& handle); - // Notifies the TransactionCompletedThread that a pending CallbackHandle has been latched. - void addLatchedCallbackHandles(const std::deque>& handles, nsecs_t latchTime, - const sp& previousReleaseFence); + void registerPendingCallbackHandle(const sp& handle); + // Notifies the TransactionCompletedThread that a pending CallbackHandle has been presented. + void addPresentedCallbackHandles(const std::deque>& handles); // Adds the Transaction CallbackHandle from a layer that does not need to be relatched and // presented this frame. - void addUnlatchedCallbackHandle(const sp& handle); + void addUnpresentedCallbackHandle(const sp& handle); void addPresentFence(const sp& presentFence); @@ -70,8 +71,7 @@ public: private: void threadMain(); - void addCallbackHandle(const sp& handle, nsecs_t latchTime = -1) - REQUIRES(mMutex); + void addCallbackHandle(const sp& handle) REQUIRES(mMutex); class ThreadDeathRecipient : public IBinder::DeathRecipient { public: @@ -110,7 +110,6 @@ private: bool mKeepRunning GUARDED_BY(mMutex) = true; sp mPresentFence GUARDED_BY(mMutex); - std::vector> mPreviousReleaseFences GUARDED_BY(mMutex); }; } // namespace android diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index ef6999d7b0..9339761837 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -2542,6 +2542,7 @@ public: enum PreviousBuffer { NOT_RELEASED = 0, RELEASED, + UNKNOWN, }; void reset() { @@ -2596,14 +2597,19 @@ private: : mBufferResult(bufferResult), mPreviousBufferResult(previousBufferResult) {} void verifySurfaceStats(const SurfaceStats& surfaceStats, nsecs_t latchTime) const { - const auto& [surfaceControl, acquireTime, releasePreviousBuffer] = surfaceStats; + const auto& [surfaceControl, acquireTime, previousReleaseFence] = surfaceStats; ASSERT_EQ(acquireTime > 0, mBufferResult == ExpectedResult::Buffer::ACQUIRED) << "bad acquire time"; ASSERT_LE(acquireTime, latchTime) << "acquire time should be <= latch time"; - ASSERT_EQ(releasePreviousBuffer, - mPreviousBufferResult == ExpectedResult::PreviousBuffer::RELEASED) - << "bad previous buffer released"; + + if (mPreviousBufferResult == ExpectedResult::PreviousBuffer::RELEASED) { + ASSERT_NE(previousReleaseFence, nullptr) + << "failed to set release prev buffer fence"; + } else if (mPreviousBufferResult == ExpectedResult::PreviousBuffer::NOT_RELEASED) { + ASSERT_EQ(previousReleaseFence, nullptr) + << "should not have set released prev buffer fence"; + } } private: @@ -3177,13 +3183,11 @@ TEST_F(LayerCallbackTest, MultipleTransactions_SingleFrame) { Transaction transaction; CallbackHelper callback; std::vector expectedResults(50); - ExpectedResult::PreviousBuffer previousBufferResult = - ExpectedResult::PreviousBuffer::NOT_RELEASED; for (auto& expected : expectedResults) { expected.reset(); expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, - ExpectedResult::Buffer::ACQUIRED, previousBufferResult); - previousBufferResult = ExpectedResult::PreviousBuffer::RELEASED; + ExpectedResult::Buffer::ACQUIRED, + ExpectedResult::PreviousBuffer::UNKNOWN); int err = fillTransaction(transaction, &callback, layer); if (err) { -- cgit v1.2.3-59-g8ed1b From 3dad52d38519ef4ad6d13095a8fa756bb736ae85 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Fri, 22 Mar 2019 14:03:19 -0700 Subject: blast: transaction callbacks should come in order Previously, if no SurfaceControls where marked to get callbacks, a callback was sent directly from SurfaceComposerClient so we could save a binder call into SurfaceFlinger and a binder call for the callback. Although this saved us 2 binder calls, it made the transactions callbacks come out of order. The public api guarantees that all callbacks must come in order. This patch moves the callback from SurfaceComposerClient into SurfaceFlinger so the callbacks are in order. Bug: 128519264 Test: SurfaceFlinger_test Change-Id: Ia1cadb81adb69b58a4d6d43ae453c96a1572f833 --- libs/gui/ISurfaceComposer.cpp | 23 ++++++++++- libs/gui/LayerState.cpp | 25 +++--------- libs/gui/SurfaceComposerClient.cpp | 19 +++++----- libs/gui/include/gui/ISurfaceComposer.h | 15 +++++--- libs/gui/include/gui/LayerState.h | 6 +-- libs/gui/tests/Surface_test.cpp | 4 +- services/surfaceflinger/SurfaceFlinger.cpp | 40 +++++++++++++------- services/surfaceflinger/SurfaceFlinger.h | 16 +++++--- .../surfaceflinger/TransactionCompletedThread.cpp | 44 ++++++++++++++++------ .../surfaceflinger/TransactionCompletedThread.h | 9 ++++- 10 files changed, 128 insertions(+), 73 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index bc63d315e0..b74d675e4d 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -69,7 +69,8 @@ public: const sp& applyToken, const InputWindowCommands& commands, int64_t desiredPresentTime, - const cached_buffer_t& uncacheBuffer) { + const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks) { Parcel data, reply; data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor()); @@ -89,6 +90,14 @@ public: data.writeInt64(desiredPresentTime); data.writeStrongBinder(uncacheBuffer.token); data.writeUint64(uncacheBuffer.cacheId); + + if (data.writeVectorSize(listenerCallbacks) == NO_ERROR) { + for (const auto& [listener, callbackIds] : listenerCallbacks) { + data.writeStrongBinder(IInterface::asBinder(listener)); + data.writeInt64Vector(callbackIds); + } + } + remote()->transact(BnSurfaceComposer::SET_TRANSACTION_STATE, data, &reply); } @@ -978,8 +987,18 @@ status_t BnSurfaceComposer::onTransact( uncachedBuffer.token = data.readStrongBinder(); uncachedBuffer.cacheId = data.readUint64(); + std::vector listenerCallbacks; + int32_t listenersSize = data.readInt32(); + for (int32_t i = 0; i < listenersSize; i++) { + auto listener = + interface_cast(data.readStrongBinder()); + std::vector callbackIds; + data.readInt64Vector(&callbackIds); + listenerCallbacks.emplace_back(listener, callbackIds); + } + setTransactionState(state, displays, stateFlags, applyToken, inputWindowCommands, - desiredPresentTime, uncachedBuffer); + desiredPresentTime, uncachedBuffer, listenerCallbacks); return NO_ERROR; } case BOOT_FINISHED: { diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index f6ca9e8f0b..3077b21da7 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -86,14 +86,7 @@ status_t layer_state_t::write(Parcel& output) const memcpy(output.writeInplace(16 * sizeof(float)), colorTransform.asArray(), 16 * sizeof(float)); output.writeFloat(cornerRadius); - - if (output.writeVectorSize(listenerCallbacks) == NO_ERROR) { - for (const auto& [listener, callbackIds] : listenerCallbacks) { - output.writeStrongBinder(IInterface::asBinder(listener)); - output.writeInt64Vector(callbackIds); - } - } - + output.writeBool(hasListenerCallbacks); output.writeStrongBinder(cachedBuffer.token); output.writeUint64(cachedBuffer.cacheId); output.writeParcelable(metadata); @@ -163,15 +156,7 @@ status_t layer_state_t::read(const Parcel& input) colorTransform = mat4(static_cast(input.readInplace(16 * sizeof(float)))); cornerRadius = input.readFloat(); - - int32_t listenersSize = input.readInt32(); - for (int32_t i = 0; i < listenersSize; i++) { - auto listener = interface_cast(input.readStrongBinder()); - std::vector callbackIds; - input.readInt64Vector(&callbackIds); - listenerCallbacks.emplace_back(listener, callbackIds); - } - + hasListenerCallbacks = input.readBool(); cachedBuffer.token = input.readStrongBinder(); cachedBuffer.cacheId = input.readUint64(); input.readParcelable(&metadata); @@ -376,9 +361,9 @@ void layer_state_t::merge(const layer_state_t& other) { what |= eColorTransformChanged; colorTransform = other.colorTransform; } - if (other.what & eListenerCallbacksChanged) { - what |= eListenerCallbacksChanged; - listenerCallbacks = other.listenerCallbacks; + if (other.what & eHasListenerCallbacksChanged) { + what |= eHasListenerCallbacksChanged; + hasListenerCallbacks = other.hasListenerCallbacks; } #ifndef NO_INPUT diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 39cd62fa10..84aed5f51c 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -381,7 +381,7 @@ void SurfaceComposerClient::doDropReferenceTransaction(const sp& handle s.state.parentHandleForChild = nullptr; composerStates.add(s); - sf->setTransactionState(composerStates, displayStates, 0, nullptr, {}, -1, {}); + sf->setTransactionState(composerStates, displayStates, 0, nullptr, {}, -1, {}, {}); } void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { @@ -391,7 +391,7 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { uncacheBuffer.token = BufferCache::getInstance().getToken(); uncacheBuffer.cacheId = cacheId; - sf->setTransactionState({}, {}, 0, nullptr, {}, -1, uncacheBuffer); + sf->setTransactionState({}, {}, 0, nullptr, {}, -1, uncacheBuffer, {}); } void SurfaceComposerClient::Transaction::cacheBuffers() { @@ -434,6 +434,8 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { sp sf(ComposerService::getComposerService()); + std::vector listenerCallbacks; + // For every listener with registered callbacks for (const auto& [listener, callbackInfo] : mListenerCallbacks) { auto& [callbackIds, surfaceControls] = callbackInfo; @@ -441,11 +443,7 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { continue; } - // If the listener does not have any SurfaceControls set on this Transaction, send the - // callback now - if (surfaceControls.empty()) { - listener->onTransactionCompleted(ListenerStats::createEmpty(listener, callbackIds)); - } + listenerCallbacks.emplace_back(listener, std::move(callbackIds)); // If the listener has any SurfaceControls set on this Transaction update the surface state for (const auto& surfaceControl : surfaceControls) { @@ -454,8 +452,8 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { ALOGE("failed to get layer state"); continue; } - s->what |= layer_state_t::eListenerCallbacksChanged; - s->listenerCallbacks.emplace_back(listener, std::move(callbackIds)); + s->what |= layer_state_t::eHasListenerCallbacksChanged; + s->hasListenerCallbacks = true; } } mListenerCallbacks.clear(); @@ -494,7 +492,8 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { sp applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance()); sf->setTransactionState(composerStates, displayStates, flags, applyToken, mInputWindowCommands, mDesiredPresentTime, - {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/); + {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/, + listenerCallbacks); mInputWindowCommands.clear(); mStatus = NO_ERROR; return NO_ERROR; diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 0ef5b39641..fe85fdf697 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -20,13 +20,10 @@ #include #include -#include -#include -#include -#include - #include +#include + #include #include #include @@ -34,6 +31,11 @@ #include #include +#include +#include +#include +#include + #include #include @@ -133,7 +135,8 @@ public: const sp& applyToken, const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime, - const cached_buffer_t& uncacheBuffer) = 0; + const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks) = 0; /* signal that we're done booting. * Requires ACCESS_SURFACE_FLINGER permission diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 77bf8f1dc5..2256497751 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -23,7 +23,6 @@ #include #include -#include #include #ifndef NO_INPUT @@ -86,7 +85,7 @@ struct layer_state_t { eApiChanged = 0x04000000, eSidebandStreamChanged = 0x08000000, eColorTransformChanged = 0x10000000, - eListenerCallbacksChanged = 0x20000000, + eHasListenerCallbacksChanged = 0x20000000, eInputInfoChanged = 0x40000000, eCornerRadiusChanged = 0x80000000, eFrameChanged = 0x1'00000000, @@ -120,6 +119,7 @@ struct layer_state_t { surfaceDamageRegion(), api(-1), colorTransform(mat4()), + hasListenerCallbacks(false), bgColorAlpha(0), bgColorDataspace(ui::Dataspace::UNKNOWN), colorSpaceAgnostic(false) { @@ -182,7 +182,7 @@ struct layer_state_t { sp sidebandStream; mat4 colorTransform; - std::vector listenerCallbacks; + bool hasListenerCallbacks; #ifndef NO_INPUT InputWindowInfo inputInfo; #endif diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 94b669dde4..0ee9bff77f 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -561,7 +561,9 @@ public: const sp& /*applyToken*/, const InputWindowCommands& /*inputWindowCommands*/, int64_t /*desiredPresentTime*/, - const cached_buffer_t& /*cachedBuffer*/) override {} + const cached_buffer_t& /*cachedBuffer*/, + const std::vector& /*listenerCallbacks*/) override { + } void bootFinished() override {} bool authenticateSurfaceTexture( diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 547f30f210..45f337766a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3509,14 +3509,14 @@ bool SurfaceFlinger::flushTransactionQueues() { while (!transactionQueue.empty()) { const auto& - [states, displays, flags, desiredPresentTime, uncacheBuffer, postTime, - privileged] = transactionQueue.front(); + [states, displays, flags, desiredPresentTime, uncacheBuffer, listenerCallbacks, + postTime, privileged] = transactionQueue.front(); if (!transactionIsReadyToBeApplied(desiredPresentTime, states)) { break; } applyTransactionState(states, displays, flags, mPendingInputWindowCommands, - desiredPresentTime, uncacheBuffer, postTime, privileged, - /*isMainThread*/ true); + desiredPresentTime, uncacheBuffer, listenerCallbacks, postTime, + privileged, /*isMainThread*/ true); transactionQueue.pop(); } @@ -3574,7 +3574,8 @@ void SurfaceFlinger::setTransactionState(const Vector& states, const sp& applyToken, const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime, - const cached_buffer_t& uncacheBuffer) { + const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks) { ATRACE_CALL(); const int64_t postTime = systemTime(); @@ -3591,13 +3592,14 @@ void SurfaceFlinger::setTransactionState(const Vector& states, if (mTransactionQueues.find(applyToken) != mTransactionQueues.end() || !transactionIsReadyToBeApplied(desiredPresentTime, states)) { mTransactionQueues[applyToken].emplace(states, displays, flags, desiredPresentTime, - uncacheBuffer, postTime, privileged); + uncacheBuffer, listenerCallbacks, postTime, + privileged); setTransactionFlags(eTransactionNeeded); return; } applyTransactionState(states, displays, flags, inputWindowCommands, desiredPresentTime, - uncacheBuffer, postTime, privileged); + uncacheBuffer, listenerCallbacks, postTime, privileged); } void SurfaceFlinger::applyTransactionState(const Vector& states, @@ -3605,6 +3607,7 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, const InputWindowCommands& inputWindowCommands, const int64_t desiredPresentTime, const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks, const int64_t postTime, bool privileged, bool isMainThread) { uint32_t transactionFlags = 0; @@ -3631,7 +3634,15 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, uint32_t clientStateFlags = 0; for (const ComposerState& state : states) { - clientStateFlags |= setClientStateLocked(state, desiredPresentTime, postTime, privileged); + clientStateFlags |= setClientStateLocked(state, desiredPresentTime, listenerCallbacks, + postTime, privileged); + } + + // In case the client has sent a Transaction that should receive callbacks but without any + // SurfaceControls that should be included in the callback, send the listener and callbackIds + // to the callback thread so it can send an empty callback + for (const auto& [listener, callbackIds] : listenerCallbacks) { + mTransactionCompletedThread.addCallback(listener, callbackIds); } // If the state doesn't require a traversal and there are callbacks, send them now if (!(clientStateFlags & eTraversalNeeded)) { @@ -3752,9 +3763,10 @@ bool SurfaceFlinger::callingThreadHasUnscopedSurfaceFlingerAccess() { return true; } -uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState, - int64_t desiredPresentTime, int64_t postTime, - bool privileged) { +uint32_t SurfaceFlinger::setClientStateLocked( + const ComposerState& composerState, int64_t desiredPresentTime, + const std::vector& listenerCallbacks, int64_t postTime, + bool privileged) { const layer_state_t& s = composerState.state; sp client(static_cast(composerState.client.get())); @@ -3980,9 +3992,9 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState } } std::vector> callbackHandles; - if ((what & layer_state_t::eListenerCallbacksChanged) && (!s.listenerCallbacks.empty())) { + if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!listenerCallbacks.empty())) { mTransactionCompletedThread.run(); - for (const auto& [listener, callbackIds] : s.listenerCallbacks) { + for (const auto& [listener, callbackIds] : listenerCallbacks) { callbackHandles.emplace_back(new CallbackHandle(listener, callbackIds, s.surface)); } } @@ -4243,7 +4255,7 @@ void SurfaceFlinger::onInitializeDisplays() { d.width = 0; d.height = 0; displays.add(d); - setTransactionState(state, displays, 0, nullptr, mPendingInputWindowCommands, -1, {}); + setTransactionState(state, displays, 0, nullptr, mPendingInputWindowCommands, -1, {}, {}); setPowerModeInternal(display, HWC_POWER_MODE_NORMAL); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 03146d6f17..b4095e3485 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -431,8 +431,8 @@ private: const Vector& displays, uint32_t flags, const sp& applyToken, const InputWindowCommands& inputWindowCommands, - int64_t desiredPresentTime, - const cached_buffer_t& uncacheBuffer) override; + int64_t desiredPresentTime, const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks) override; void bootFinished() override; bool authenticateSurfaceTexture( const sp& bufferProducer) const override; @@ -579,8 +579,10 @@ private: const Vector& displays, uint32_t flags, const InputWindowCommands& inputWindowCommands, const int64_t desiredPresentTime, - const cached_buffer_t& uncacheBuffer, const int64_t postTime, - bool privileged, bool isMainThread = false) REQUIRES(mStateLock); + const cached_buffer_t& uncacheBuffer, + const std::vector& listenerCallbacks, + const int64_t postTime, bool privileged, bool isMainThread = false) + REQUIRES(mStateLock); bool flushTransactionQueues(); uint32_t getTransactionFlags(uint32_t flags); uint32_t peekTransactionFlags(); @@ -593,6 +595,7 @@ private: bool transactionIsReadyToBeApplied(int64_t desiredPresentTime, const Vector& states); uint32_t setClientStateLocked(const ComposerState& composerState, int64_t desiredPresentTime, + const std::vector& listenerCallbacks, int64_t postTime, bool privileged) REQUIRES(mStateLock); uint32_t setDisplayStateLocked(const DisplayState& s) REQUIRES(mStateLock); uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands) @@ -1061,12 +1064,14 @@ private: TransactionState(const Vector& composerStates, const Vector& displayStates, uint32_t transactionFlags, int64_t desiredPresentTime, const cached_buffer_t& uncacheBuffer, - int64_t postTime, bool privileged) + const std::vector& listenerCallbacks, int64_t postTime, + bool privileged) : states(composerStates), displays(displayStates), flags(transactionFlags), desiredPresentTime(desiredPresentTime), buffer(uncacheBuffer), + callback(listenerCallbacks), postTime(postTime), privileged(privileged) {} @@ -1075,6 +1080,7 @@ private: uint32_t flags; const int64_t desiredPresentTime; cached_buffer_t buffer; + std::vector callback; const int64_t postTime; bool privileged; }; diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index d2b7fe099f..6e0c1e2d97 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -100,26 +100,48 @@ void TransactionCompletedThread::addUnpresentedCallbackHandle(const sp& handle) { +void TransactionCompletedThread::addCallback( + const sp& transactionListener, + const std::vector& callbackIds) { + std::lock_guard lock(mMutex); + addCallbackLocked(transactionListener, callbackIds); +} + +status_t TransactionCompletedThread::addCallbackHandle(const sp& handle) { + status_t err = addCallbackLocked(handle->listener, handle->callbackIds); + if (err != NO_ERROR) { + ALOGE("cannot add callback, err: %d", err); + return err; + } + const sp listener = IInterface::asBinder(handle->listener); + auto& listenerStats = mListenerStats[listener]; + auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; + transactionStats.latchTime = handle->latchTime; + transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, + handle->previousReleaseFence); + return NO_ERROR; +} + +status_t TransactionCompletedThread::addCallbackLocked( + const sp& transactionListener, + const std::vector& callbackIds) { + const sp listener = IInterface::asBinder(transactionListener); // If we don't already have a reference to this listener, linkToDeath so we get a notification // if it dies. if (mListenerStats.count(listener) == 0) { - status_t error = listener->linkToDeath(mDeathRecipient); - if (error != NO_ERROR) { - ALOGE("cannot add callback handle because linkToDeath failed, err: %d", error); - return; + status_t err = listener->linkToDeath(mDeathRecipient); + if (err != NO_ERROR) { + ALOGE("cannot add callback because linkToDeath failed, err: %d", err); + return err; } } auto& listenerStats = mListenerStats[listener]; - listenerStats.listener = handle->listener; - - auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; - transactionStats.latchTime = handle->latchTime; - transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, - handle->previousReleaseFence); + listenerStats.listener = transactionListener; + listenerStats.transactionStats[callbackIds]; + return NO_ERROR; } void TransactionCompletedThread::addPresentFence(const sp& presentFence) { diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index f49306d70e..88808fd0da 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -64,6 +64,11 @@ public: // presented this frame. void addUnpresentedCallbackHandle(const sp& handle); + // Adds listener and callbackIds in case there are no SurfaceControls that are supposed + // to be included in the callback. + void addCallback(const sp& transactionListener, + const std::vector& callbackIds); + void addPresentFence(const sp& presentFence); void sendCallbacks(); @@ -71,7 +76,9 @@ public: private: void threadMain(); - void addCallbackHandle(const sp& handle) REQUIRES(mMutex); + status_t addCallbackHandle(const sp& handle) REQUIRES(mMutex); + status_t addCallbackLocked(const sp& transactionListener, + const std::vector& callbackIds) REQUIRES(mMutex); class ThreadDeathRecipient : public IBinder::DeathRecipient { public: -- cgit v1.2.3-59-g8ed1b From d600d57da59244af8f94145558debe7f1acad998 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Tue, 26 Mar 2019 15:38:50 -0700 Subject: blast: in order no-op transaction callbacks Transactions callbacks were being sent as soon as they were ready instead of in order. To fix this, keep a deque of Transaction callbacks and do not send a callback until all the callbacks before it have been sent. Bug: 128519264 Test: Transaction_test Change-Id: Ia363b3aca85bc1cd71d0fd915de79b44f786e09f --- libs/gui/ITransactionCompletedListener.cpp | 35 ++-- libs/gui/SurfaceComposerClient.cpp | 8 +- .../include/gui/ITransactionCompletedListener.h | 23 ++- services/surfaceflinger/SurfaceFlinger.cpp | 17 +- .../surfaceflinger/TransactionCompletedThread.cpp | 198 ++++++++++++++------- .../surfaceflinger/TransactionCompletedThread.h | 51 ++++-- 6 files changed, 201 insertions(+), 131 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index ce88d7b47d..74cd4f1ede 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -76,7 +76,11 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { } status_t TransactionStats::writeToParcel(Parcel* output) const { - status_t err = output->writeInt64(latchTime); + status_t err = output->writeInt64Vector(callbackIds); + if (err != NO_ERROR) { + return err; + } + err = output->writeInt64(latchTime); if (err != NO_ERROR) { return err; } @@ -96,7 +100,11 @@ status_t TransactionStats::writeToParcel(Parcel* output) const { } status_t TransactionStats::readFromParcel(const Parcel* input) { - status_t err = input->readInt64(&latchTime); + status_t err = input->readInt64Vector(&callbackIds); + if (err != NO_ERROR) { + return err; + } + err = input->readInt64(&latchTime); if (err != NO_ERROR) { return err; } @@ -120,16 +128,11 @@ status_t ListenerStats::writeToParcel(Parcel* output) const { if (err != NO_ERROR) { return err; } - - for (const auto& [callbackIds, stats] : transactionStats) { + for (const auto& stats : transactionStats) { err = output->writeParcelable(stats); if (err != NO_ERROR) { return err; } - err = output->writeInt64Vector(callbackIds); - if (err != NO_ERROR) { - return err; - } } return NO_ERROR; } @@ -139,18 +142,11 @@ status_t ListenerStats::readFromParcel(const Parcel* input) { for (int i = 0; i < transactionStats_size; i++) { TransactionStats stats; - std::vector callbackIds; - status_t err = input->readParcelable(&stats); if (err != NO_ERROR) { return err; } - err = input->readInt64Vector(&callbackIds); - if (err != NO_ERROR) { - return err; - } - - transactionStats.emplace(callbackIds, stats); + transactionStats.push_back(stats); } return NO_ERROR; } @@ -159,11 +155,8 @@ ListenerStats ListenerStats::createEmpty(const sp const std::unordered_set& callbackIds) { ListenerStats listenerStats; listenerStats.listener = listener; - TransactionStats transactionStats; - listenerStats.transactionStats.emplace(std::piecewise_construct, - std::forward_as_tuple(callbackIds.begin(), - callbackIds.end()), - std::forward_as_tuple(transactionStats)); + listenerStats.transactionStats.emplace_back(callbackIds); + return listenerStats; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index c627898ce2..83cf40c8e9 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -203,15 +203,15 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener * that could possibly exist for the callbacks. */ std::unordered_map, sp, IBinderHash> surfaceControls; - for (const auto& [callbackIds, transactionStats] : listenerStats.transactionStats) { - for (auto callbackId : callbackIds) { + for (const auto& transactionStats : listenerStats.transactionStats) { + for (auto callbackId : transactionStats.callbackIds) { auto& [callbackFunction, callbackSurfaceControls] = mCallbacks[callbackId]; surfaceControls.insert(callbackSurfaceControls.begin(), callbackSurfaceControls.end()); } } - for (const auto& [callbackIds, transactionStats] : listenerStats.transactionStats) { - for (auto callbackId : callbackIds) { + for (const auto& transactionStats : listenerStats.transactionStats) { + for (auto callbackId : transactionStats.callbackIds) { auto& [callbackFunction, callbackSurfaceControls] = mCallbacks[callbackId]; if (!callbackFunction) { ALOGE("cannot call null callback function, skipping"); diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index 29ab026928..774ad46b15 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -34,18 +34,6 @@ class ITransactionCompletedListener; using CallbackId = int64_t; -struct CallbackIdsHash { - // CallbackId vectors have several properties that let us get away with this simple hash. - // 1) CallbackIds are never 0 so if something has gone wrong and our CallbackId vector is - // empty we can still hash 0. - // 2) CallbackId vectors for the same listener either are identical or contain none of the - // same members. It is sufficient to just check the first CallbackId in the vectors. If - // they match, they are the same. If they do not match, they are not the same. - std::size_t operator()(const std::vector callbackIds) const { - return std::hash{}((callbackIds.size() == 0) ? 0 : callbackIds.front()); - } -}; - class SurfaceStats : public Parcelable { public: status_t writeToParcel(Parcel* output) const override; @@ -65,6 +53,15 @@ public: status_t writeToParcel(Parcel* output) const override; status_t readFromParcel(const Parcel* input) override; + TransactionStats() = default; + TransactionStats(const std::vector& ids) : callbackIds(ids) {} + TransactionStats(const std::unordered_set& ids) + : callbackIds(ids.begin(), ids.end()) {} + TransactionStats(const std::vector& ids, nsecs_t latch, const sp& present, + const std::vector& surfaces) + : callbackIds(ids), latchTime(latch), presentFence(present), surfaceStats(surfaces) {} + + std::vector callbackIds; nsecs_t latchTime = -1; sp presentFence = nullptr; std::vector surfaceStats; @@ -79,7 +76,7 @@ public: const std::unordered_set& callbackIds); sp listener; - std::unordered_map, TransactionStats, CallbackIdsHash> transactionStats; + std::vector transactionStats; }; class ITransactionCompletedListener : public IInterface { diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fae4b8168e..2aac8f05d1 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3651,18 +3651,22 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, transactionFlags |= setDisplayStateLocked(display); } - uint32_t clientStateFlags = 0; - for (const ComposerState& state : states) { - clientStateFlags |= setClientStateLocked(state, desiredPresentTime, listenerCallbacks, - postTime, privileged); - } - // In case the client has sent a Transaction that should receive callbacks but without any // SurfaceControls that should be included in the callback, send the listener and callbackIds // to the callback thread so it can send an empty callback + if (!listenerCallbacks.empty()) { + mTransactionCompletedThread.run(); + } for (const auto& [listener, callbackIds] : listenerCallbacks) { mTransactionCompletedThread.addCallback(listener, callbackIds); } + + uint32_t clientStateFlags = 0; + for (const ComposerState& state : states) { + clientStateFlags |= setClientStateLocked(state, desiredPresentTime, listenerCallbacks, + postTime, privileged); + } + // If the state doesn't require a traversal and there are callbacks, send them now if (!(clientStateFlags & eTraversalNeeded)) { mTransactionCompletedThread.sendCallbacks(); @@ -4019,7 +4023,6 @@ uint32_t SurfaceFlinger::setClientStateLocked( } std::vector> callbackHandles; if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!listenerCallbacks.empty())) { - mTransactionCompletedThread.run(); for (const auto& [listener, callbackIds] : listenerCallbacks) { callbackHandles.emplace_back(new CallbackHandle(listener, callbackIds, s.surface)); } diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 6e0c1e2d97..1d2217d322 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -29,6 +29,18 @@ namespace android { +// Returns 0 if they are equal +// <0 if the first id that doesn't match is lower in c2 or all ids match but c2 is shorter +// >0 if the first id that doesn't match is greater in c2 or all ids match but c2 is longer +// +// See CallbackIdsHash for a explaniation of why this works +static int compareCallbackIds(const std::vector& c1, const std::vector c2) { + if (c1.empty()) { + return !c2.empty(); + } + return c1.front() - c2.front(); +} + TransactionCompletedThread::~TransactionCompletedThread() { std::lock_guard lockThread(mThreadMutex); @@ -44,8 +56,8 @@ TransactionCompletedThread::~TransactionCompletedThread() { { std::lock_guard lock(mMutex); - for (const auto& [listener, listenerStats] : mListenerStats) { - listener->unlinkToDeath(mDeathRecipient); + for (const auto& [listener, transactionStats] : mCompletedTransactions) { + IInterface::asBinder(listener)->unlinkToDeath(mDeathRecipient); } } } @@ -62,85 +74,127 @@ void TransactionCompletedThread::run() { mThread = std::thread(&TransactionCompletedThread::threadMain, this); } -void TransactionCompletedThread::registerPendingCallbackHandle(const sp& handle) { +status_t TransactionCompletedThread::addCallback(const sp& listener, + const std::vector& callbackIds) { std::lock_guard lock(mMutex); + if (!mRunning) { + ALOGE("cannot add callback because the callback thread isn't running"); + return BAD_VALUE; + } - sp listener = IInterface::asBinder(handle->listener); - const auto& callbackIds = handle->callbackIds; + if (mCompletedTransactions.count(listener) == 0) { + status_t err = IInterface::asBinder(listener)->linkToDeath(mDeathRecipient); + if (err != NO_ERROR) { + ALOGE("cannot add callback because linkToDeath failed, err: %d", err); + return err; + } + } - mPendingTransactions[listener][callbackIds]++; + auto& transactionStatsDeque = mCompletedTransactions[listener]; + transactionStatsDeque.emplace_back(callbackIds); + return NO_ERROR; } -void TransactionCompletedThread::addPresentedCallbackHandles( +status_t TransactionCompletedThread::registerPendingCallbackHandle( + const sp& handle) { + std::lock_guard lock(mMutex); + if (!mRunning) { + ALOGE("cannot register callback handle because the callback thread isn't running"); + return BAD_VALUE; + } + + // If we can't find the transaction stats something has gone wrong. The client should call + // addCallback before trying to register a pending callback handle. + TransactionStats* transactionStats; + status_t err = findTransactionStats(handle->listener, handle->callbackIds, &transactionStats); + if (err != NO_ERROR) { + ALOGE("cannot find transaction stats"); + return err; + } + + mPendingTransactions[handle->listener][handle->callbackIds]++; + return NO_ERROR; +} + +status_t TransactionCompletedThread::addPresentedCallbackHandles( const std::deque>& handles) { std::lock_guard lock(mMutex); + if (!mRunning) { + ALOGE("cannot add presented callback handle because the callback thread isn't running"); + return BAD_VALUE; + } for (const auto& handle : handles) { - auto listener = mPendingTransactions.find(IInterface::asBinder(handle->listener)); - auto& pendingCallbacks = listener->second; - auto pendingCallback = pendingCallbacks.find(handle->callbackIds); + auto listener = mPendingTransactions.find(handle->listener); + if (listener != mPendingTransactions.end()) { + auto& pendingCallbacks = listener->second; + auto pendingCallback = pendingCallbacks.find(handle->callbackIds); - if (pendingCallback != pendingCallbacks.end()) { - auto& pendingCount = pendingCallback->second; + if (pendingCallback != pendingCallbacks.end()) { + auto& pendingCount = pendingCallback->second; - // Decrease the pending count for this listener - if (--pendingCount == 0) { - pendingCallbacks.erase(pendingCallback); + // Decrease the pending count for this listener + if (--pendingCount == 0) { + pendingCallbacks.erase(pendingCallback); + } + } else { + ALOGW("there are more latched callbacks than there were registered callbacks"); } } else { - ALOGE("there are more latched callbacks than there were registered callbacks"); + ALOGW("cannot find listener in mPendingTransactions"); } - addCallbackHandle(handle); + status_t err = addCallbackHandle(handle); + if (err != NO_ERROR) { + ALOGE("could not add callback handle"); + return err; + } } -} -void TransactionCompletedThread::addUnpresentedCallbackHandle(const sp& handle) { - std::lock_guard lock(mMutex); - addCallbackHandle(handle); + return NO_ERROR; } -void TransactionCompletedThread::addCallback( - const sp& transactionListener, - const std::vector& callbackIds) { +status_t TransactionCompletedThread::addUnpresentedCallbackHandle( + const sp& handle) { std::lock_guard lock(mMutex); - addCallbackLocked(transactionListener, callbackIds); + if (!mRunning) { + ALOGE("cannot add unpresented callback handle because the callback thread isn't running"); + return BAD_VALUE; + } + + return addCallbackHandle(handle); } -status_t TransactionCompletedThread::addCallbackHandle(const sp& handle) { - status_t err = addCallbackLocked(handle->listener, handle->callbackIds); - if (err != NO_ERROR) { - ALOGE("cannot add callback, err: %d", err); - return err; +status_t TransactionCompletedThread::findTransactionStats( + const sp& listener, + const std::vector& callbackIds, TransactionStats** outTransactionStats) { + auto& transactionStatsDeque = mCompletedTransactions[listener]; + + // Search back to front because the most recent transactions are at the back of the deque + auto itr = transactionStatsDeque.rbegin(); + for (; itr != transactionStatsDeque.rend(); itr++) { + if (compareCallbackIds(itr->callbackIds, callbackIds) == 0) { + *outTransactionStats = &(*itr); + return NO_ERROR; + } } - const sp listener = IInterface::asBinder(handle->listener); - auto& listenerStats = mListenerStats[listener]; - auto& transactionStats = listenerStats.transactionStats[handle->callbackIds]; - - transactionStats.latchTime = handle->latchTime; - transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, - handle->previousReleaseFence); - return NO_ERROR; + ALOGE("could not find transaction stats"); + return BAD_VALUE; } -status_t TransactionCompletedThread::addCallbackLocked( - const sp& transactionListener, - const std::vector& callbackIds) { - const sp listener = IInterface::asBinder(transactionListener); - // If we don't already have a reference to this listener, linkToDeath so we get a notification - // if it dies. - if (mListenerStats.count(listener) == 0) { - status_t err = listener->linkToDeath(mDeathRecipient); - if (err != NO_ERROR) { - ALOGE("cannot add callback because linkToDeath failed, err: %d", err); - return err; - } +status_t TransactionCompletedThread::addCallbackHandle(const sp& handle) { + // If we can't find the transaction stats something has gone wrong. The client should call + // addCallback before trying to add a presnted callback handle. + TransactionStats* transactionStats; + status_t err = findTransactionStats(handle->listener, handle->callbackIds, &transactionStats); + if (err != NO_ERROR) { + return err; } - auto& listenerStats = mListenerStats[listener]; - listenerStats.listener = transactionListener; - listenerStats.transactionStats[callbackIds]; + transactionStats->latchTime = handle->latchTime; + transactionStats->surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, + handle->previousReleaseFence); return NO_ERROR; } @@ -163,40 +217,46 @@ void TransactionCompletedThread::threadMain() { mConditionVariable.wait(mMutex); // For each listener - auto it = mListenerStats.begin(); - while (it != mListenerStats.end()) { - auto& [listener, listenerStats] = *it; + auto completedTransactionsItr = mCompletedTransactions.begin(); + while (completedTransactionsItr != mCompletedTransactions.end()) { + auto& [listener, transactionStatsDeque] = *completedTransactionsItr; + ListenerStats listenerStats; + listenerStats.listener = listener; // For each transaction - bool sendCallback = true; - for (auto& [callbackIds, transactionStats] : listenerStats.transactionStats) { - // If we are still waiting on the callback handles for this transaction, skip it - if (mPendingTransactions[listener].count(callbackIds) != 0) { - sendCallback = false; + auto transactionStatsItr = transactionStatsDeque.begin(); + while (transactionStatsItr != transactionStatsDeque.end()) { + auto& transactionStats = *transactionStatsItr; + + // If we are still waiting on the callback handles for this transaction, stop + // here because all transaction callbacks for the same listener must come in order + if (mPendingTransactions[listener].count(transactionStats.callbackIds) != 0) { break; } // If the transaction has been latched if (transactionStats.latchTime >= 0) { if (!mPresentFence) { - sendCallback = false; break; } transactionStats.presentFence = mPresentFence; } + + // Remove the transaction from completed to the callback + listenerStats.transactionStats.push_back(std::move(transactionStats)); + transactionStatsItr = transactionStatsDeque.erase(transactionStatsItr); } - // If the listener has no pending transactions and all latched transactions have been - // presented - if (sendCallback) { + // If the listener has completed transactions + if (!listenerStats.transactionStats.empty()) { // If the listener is still alive - if (listener->isBinderAlive()) { + if (IInterface::asBinder(listener)->isBinderAlive()) { // Send callback listenerStats.listener->onTransactionCompleted(listenerStats); - listener->unlinkToDeath(mDeathRecipient); + IInterface::asBinder(listener)->unlinkToDeath(mDeathRecipient); } - it = mListenerStats.erase(it); + completedTransactionsItr = mCompletedTransactions.erase(completedTransactionsItr); } else { - it++; + completedTransactionsItr++; } } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 88808fd0da..09feb75d1a 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -30,6 +30,18 @@ namespace android { +struct CallbackIdsHash { + // CallbackId vectors have several properties that let us get away with this simple hash. + // 1) CallbackIds are never 0 so if something has gone wrong and our CallbackId vector is + // empty we can still hash 0. + // 2) CallbackId vectors for the same listener either are identical or contain none of the + // same members. It is sufficient to just check the first CallbackId in the vectors. If + // they match, they are the same. If they do not match, they are not the same. + std::size_t operator()(const std::vector callbackIds) const { + return std::hash{}((callbackIds.empty()) ? 0 : callbackIds.front()); + } +}; + class CallbackHandle : public RefBase { public: CallbackHandle(const sp& transactionListener, @@ -51,23 +63,24 @@ public: void run(); + // Adds listener and callbackIds in case there are no SurfaceControls that are supposed + // to be included in the callback. This functions should be call before attempting to add any + // callback handles. + status_t addCallback(const sp& transactionListener, + const std::vector& callbackIds); + // Informs the TransactionCompletedThread that there is a Transaction with a CallbackHandle // that needs to be latched and presented this frame. This function should be called once the // layer has received the CallbackHandle so the TransactionCompletedThread knows not to send // a callback for that Listener/Transaction pair until that CallbackHandle has been latched and // presented. - void registerPendingCallbackHandle(const sp& handle); + status_t registerPendingCallbackHandle(const sp& handle); // Notifies the TransactionCompletedThread that a pending CallbackHandle has been presented. - void addPresentedCallbackHandles(const std::deque>& handles); + status_t addPresentedCallbackHandles(const std::deque>& handles); // Adds the Transaction CallbackHandle from a layer that does not need to be relatched and // presented this frame. - void addUnpresentedCallbackHandle(const sp& handle); - - // Adds listener and callbackIds in case there are no SurfaceControls that are supposed - // to be included in the callback. - void addCallback(const sp& transactionListener, - const std::vector& callbackIds); + status_t addUnpresentedCallbackHandle(const sp& handle); void addPresentFence(const sp& presentFence); @@ -76,9 +89,11 @@ public: private: void threadMain(); + status_t findTransactionStats(const sp& listener, + const std::vector& callbackIds, + TransactionStats** outTransactionStats) REQUIRES(mMutex); + status_t addCallbackHandle(const sp& handle) REQUIRES(mMutex); - status_t addCallbackLocked(const sp& transactionListener, - const std::vector& callbackIds) REQUIRES(mMutex); class ThreadDeathRecipient : public IBinder::DeathRecipient { public: @@ -91,9 +106,10 @@ private: }; sp mDeathRecipient; - struct IBinderHash { - std::size_t operator()(const sp& strongPointer) const { - return std::hash{}(strongPointer.get()); + struct ITransactionCompletedListenerHash { + std::size_t operator()(const sp& listener) const { + return std::hash{}((listener) ? IInterface::asBinder(listener).get() + : nullptr); } }; @@ -106,12 +122,13 @@ private: std::condition_variable_any mConditionVariable; std::unordered_map< - sp, + sp, std::unordered_map, uint32_t /*count*/, CallbackIdsHash>, - IBinderHash> + ITransactionCompletedListenerHash> mPendingTransactions GUARDED_BY(mMutex); - std::unordered_map, ListenerStats, IBinderHash> mListenerStats - GUARDED_BY(mMutex); + std::unordered_map, std::deque, + ITransactionCompletedListenerHash> + mCompletedTransactions GUARDED_BY(mMutex); bool mRunning GUARDED_BY(mMutex) = false; bool mKeepRunning GUARDED_BY(mMutex) = true; -- cgit v1.2.3-59-g8ed1b From a9e843ac5f5fce7cebe3938f0c69943bbbfc64d5 Mon Sep 17 00:00:00 2001 From: Greg Kaiser Date: Mon, 1 Apr 2019 06:23:09 -0700 Subject: surfaceflinger: Avoid extra vector copies We pass a std::vector by const reference in a couple places in TransactionCompletedThread to avoid making an extra copy. Test: TreeHugger Change-Id: Ie81a27b80e52a1890d54b4507d0fcbf30e746e7b --- services/surfaceflinger/TransactionCompletedThread.cpp | 3 ++- services/surfaceflinger/TransactionCompletedThread.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 1d2217d322..6b2b58346f 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -34,7 +34,8 @@ namespace android { // >0 if the first id that doesn't match is greater in c2 or all ids match but c2 is longer // // See CallbackIdsHash for a explaniation of why this works -static int compareCallbackIds(const std::vector& c1, const std::vector c2) { +static int compareCallbackIds(const std::vector& c1, + const std::vector& c2) { if (c1.empty()) { return !c2.empty(); } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 09feb75d1a..21e2678701 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -37,7 +37,7 @@ struct CallbackIdsHash { // 2) CallbackId vectors for the same listener either are identical or contain none of the // same members. It is sufficient to just check the first CallbackId in the vectors. If // they match, they are the same. If they do not match, they are not the same. - std::size_t operator()(const std::vector callbackIds) const { + std::size_t operator()(const std::vector& callbackIds) const { return std::hash{}((callbackIds.empty()) ? 0 : callbackIds.front()); } }; -- cgit v1.2.3-59-g8ed1b From 6110e84917ec5894d1b74fb2c9aeb64c27cf161a Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Fri, 12 Apr 2019 13:29:59 -0700 Subject: surfaceflinger: IBinder leak TransactionCompletedThread never deleted its Listener sp tokens from its pending thread which can lead to SurfaceFlinger crashes. This patch adds the support for removing the sp tokens when SurfaceFlinger is done with them. Test: TransactionTest Bug: 130430082 Change-Id: Icd773fc18f1a0440857bc65111c51c890746a42b --- services/surfaceflinger/TransactionCompletedThread.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 6b2b58346f..34df6068ee 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -144,6 +144,9 @@ status_t TransactionCompletedThread::addPresentedCallbackHandles( } else { ALOGW("cannot find listener in mPendingTransactions"); } + if (listener->second.size() == 0) { + mPendingTransactions.erase(listener); + } status_t err = addCallbackHandle(handle); if (err != NO_ERROR) { @@ -231,7 +234,9 @@ void TransactionCompletedThread::threadMain() { // If we are still waiting on the callback handles for this transaction, stop // here because all transaction callbacks for the same listener must come in order - if (mPendingTransactions[listener].count(transactionStats.callbackIds) != 0) { + auto pendingTransactions = mPendingTransactions.find(listener); + if (pendingTransactions != mPendingTransactions.end() && + pendingTransactions->second.count(transactionStats.callbackIds) != 0) { break; } -- cgit v1.2.3-59-g8ed1b From b0022cc49371bfc5d9bd10fb8e60c29b1ff4aef6 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Tue, 16 Apr 2019 14:19:55 -0700 Subject: blast: transaction ordering This patch fixes two issues. 1) When a pending transactions is not found, don't try to use the iterator. This fixes a crash that happens when we are in a bad state. 2) When a transaction doesn't have any callbacks, don't try to send callbacks. Bug: 130643588 Test: SurfaceFlinger_test Change-Id: I6154c31dbf0b958683324c6b45e1a607691b84e8 --- services/surfaceflinger/SurfaceFlinger.cpp | 2 +- services/surfaceflinger/TransactionCompletedThread.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 606355549e..48e1a64c7d 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3617,7 +3617,7 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, } // If the state doesn't require a traversal and there are callbacks, send them now - if (!(clientStateFlags & eTraversalNeeded)) { + if (!(clientStateFlags & eTraversalNeeded) && !listenerCallbacks.empty()) { mTransactionCompletedThread.sendCallbacks(); } transactionFlags |= clientStateFlags; diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 34df6068ee..b1bf4e20d3 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -141,12 +141,12 @@ status_t TransactionCompletedThread::addPresentedCallbackHandles( } else { ALOGW("there are more latched callbacks than there were registered callbacks"); } + if (listener->second.size() == 0) { + mPendingTransactions.erase(listener); + } } else { ALOGW("cannot find listener in mPendingTransactions"); } - if (listener->second.size() == 0) { - mPendingTransactions.erase(listener); - } status_t err = addCallbackHandle(handle); if (err != NO_ERROR) { -- cgit v1.2.3-59-g8ed1b From caa83f5e10c8845c23beed252474b72afabc5e68 Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Wed, 29 May 2019 13:03:25 -0700 Subject: transactions: fix mutex deadlock on layer death We must drop the mMutex on the TransactionCompletedThread when potentially dropping the last sp to a layer. Bug: 133709028 Test: Transaction_tests Change-Id: I8bbfbf16c755aee7d010a0992f7328f50d521c8b --- services/surfaceflinger/TransactionCompletedThread.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index b1bf4e20d3..5cf8eb1a1d 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -219,6 +219,7 @@ void TransactionCompletedThread::threadMain() { while (mKeepRunning) { mConditionVariable.wait(mMutex); + std::vector completedListenerStats; // For each listener auto completedTransactionsItr = mCompletedTransactions.begin(); @@ -264,11 +265,27 @@ void TransactionCompletedThread::threadMain() { } else { completedTransactionsItr++; } + + completedListenerStats.push_back(std::move(listenerStats)); } if (mPresentFence) { mPresentFence.clear(); } + + // If everyone else has dropped their reference to a layer and its listener is dead, + // we are about to cause the layer to be deleted. If this happens at the wrong time and + // we are holding mMutex, we will cause a deadlock. + // + // The deadlock happens because this thread is holding on to mMutex and when we delete + // the layer, it grabs SF's mStateLock. A different SF binder thread grabs mStateLock, + // then call's TransactionCompletedThread::run() which tries to grab mMutex. + // + // To avoid this deadlock, we need to unlock mMutex when dropping our last reference to + // to the layer. + mMutex.unlock(); + completedListenerStats.clear(); + mMutex.lock(); } } -- cgit v1.2.3-59-g8ed1b From 369d1f5d13dc422a44b1eeeba31b118bec0effbf Mon Sep 17 00:00:00 2001 From: Marissa Wall Date: Wed, 10 Jul 2019 15:32:50 -0700 Subject: blast: fix leak on BufferStateLayer death SurfaceFlinger can occasionally leak graphic buffers. The leak happens when: 1) a transaction comes in and is placed in a queue 2) Chrome crashes 3) the parent layer is cleaned up 4) the child layer is told to release its buffer because it is no longer on screen 5) the transaction is applied with sets a callback handle on the layer which has a sp<> to the layer To fix this, the callback handle should not have a sp<> to layer. It is safe for the callback handle can have wp<> to the layer. The client side has a sp<> so during normal operation, SurfaceFlinger can promote the wp<>. The only time the promote will fail is if the client side is dead. If the client side is dead, there is no one to send a callback to so it doesn't matter if the promote fails. Bug: 135951943 Test: https://buganizer.corp.google.com/issues/135951943#comment34 Change-Id: I756ace14c90b03a6499a3187d235b42d91cdd05a (cherry picked from commit 0e24a8385a63be6a799da902e1d5ffcbb7519c2a) --- services/surfaceflinger/TransactionCompletedThread.cpp | 10 ++++++++-- services/surfaceflinger/TransactionCompletedThread.h | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) (limited to 'services/surfaceflinger/TransactionCompletedThread.cpp') diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp index 5cf8eb1a1d..fd466dedff 100644 --- a/services/surfaceflinger/TransactionCompletedThread.cpp +++ b/services/surfaceflinger/TransactionCompletedThread.cpp @@ -197,8 +197,14 @@ status_t TransactionCompletedThread::addCallbackHandle(const sp& } transactionStats->latchTime = handle->latchTime; - transactionStats->surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime, - handle->previousReleaseFence); + // If the layer has already been destroyed, don't add the SurfaceControl to the callback. + // The client side keeps a sp<> to the SurfaceControl so if the SurfaceControl has been + // destroyed the client side is dead and there won't be anyone to send the callback to. + sp surfaceControl = handle->surfaceControl.promote(); + if (surfaceControl) { + transactionStats->surfaceStats.emplace_back(surfaceControl, handle->acquireTime, + handle->previousReleaseFence); + } return NO_ERROR; } diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h index 21e2678701..e849f714d0 100644 --- a/services/surfaceflinger/TransactionCompletedThread.h +++ b/services/surfaceflinger/TransactionCompletedThread.h @@ -49,7 +49,7 @@ public: sp listener; std::vector callbackIds; - sp surfaceControl; + wp surfaceControl; bool releasePreviousBuffer = false; sp previousReleaseFence; -- cgit v1.2.3-59-g8ed1b