From 2a52ca6274f3ca8b9aea8d22c49ca8fcae4b8455 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Thu, 24 Jun 2021 13:08:53 -0700 Subject: BlastBufferQueue: Fix acquire counts when holding buffers Update the acquire count when ever we release a buffer otherwise when going from 120hz to 60hz we may release multiple buffers and only decrement the acquire count once. This will prevent the adapter from acquiring all the possible buffers. Also make sure we process the shadow queue every time we release a buffer. Bug: 188553729 Test: Run backpressure based game on 60Hz and 90hz and collect traces Change-Id: I23517ee0fe840a9215f368bd85713ba19dbeb2a3 --- libs/gui/BLASTBufferQueue.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 364c939c18..e15e11cc20 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -151,7 +151,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const spsetName(String8(consumerName.c_str())); mBufferItemConsumer->setFrameAvailableListener(this); @@ -361,16 +361,15 @@ void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId, graphicBufferId); return; } - + mNumAcquired--; mBufferItemConsumer->releaseBuffer(it->second, releaseBuffer.releaseFence); mSubmitted.erase(it); + processNextBufferLocked(false /* useNextTransaction */); } ATRACE_INT("PendingRelease", mPendingRelease.size()); - - mNumAcquired--; - ATRACE_INT(mPendingBufferTrace.c_str(), mNumFrameAvailable + mNumAcquired); - processNextBufferLocked(false /* useNextTransaction */); + ATRACE_INT(mQueuedBufferTrace.c_str(), + mNumFrameAvailable + mNumAcquired - mPendingRelease.size()); mCallbackCV.notify_all(); } @@ -538,7 +537,8 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { } // add to shadow queue mNumFrameAvailable++; - ATRACE_INT(mPendingBufferTrace.c_str(), mNumFrameAvailable + mNumAcquired); + ATRACE_INT(mQueuedBufferTrace.c_str(), + mNumFrameAvailable + mNumAcquired - mPendingRelease.size()); BQA_LOGV("onFrameAvailable framenumber=%" PRIu64 " nextTransactionSet=%s", item.mFrameNumber, toString(nextTransactionSet)); -- cgit v1.2.3-59-g8ed1b From 4ba0c2e2a2e8a4870b4d5055bd765d5514a35ebc Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Thu, 24 Jun 2021 11:27:17 -0700 Subject: Blast: Use a unique id to track buffers When buffer queue is configured in shared buffer mode, the client can queue the same buffer over and over again and the consumer can acquire the same buffer repeatedly. BBQ tracks acquired buffers by graphic buffer id and SCC tracks release buffer callbacks by graphic buffer ids. This breaks if a buffer is reused before release. Fix this by using graphic buffer id and framenumber to track acquired buffers and buffer release callbacks. Test: atest CtsDeqpTestCases:dEQP-VK.wsi.android.shared_presentable_image.scale_none.identity.inherit.demand CtsDeqpTestCases:dEQP-VK.wsi.android.colorspace#basic Bug: 191220669 Change-Id: Ibd54df9fa6780c26cd8de769bf9e43163abbed5a --- libs/gui/BLASTBufferQueue.cpp | 40 +++++------ libs/gui/ITransactionCompletedListener.cpp | 24 +++++-- libs/gui/SurfaceComposerClient.cpp | 43 +++++++----- libs/gui/include/gui/BLASTBufferQueue.h | 7 +- .../include/gui/ITransactionCompletedListener.h | 38 +++++++++-- libs/gui/include/gui/LayerState.h | 6 ++ libs/gui/include/gui/SurfaceComposerClient.h | 20 +++--- services/surfaceflinger/BufferLayer.h | 1 + services/surfaceflinger/BufferStateLayer.cpp | 40 +++++------ services/surfaceflinger/BufferStateLayer.h | 9 +-- .../surfaceflinger/TransactionCallbackInvoker.cpp | 3 +- .../surfaceflinger/TransactionCallbackInvoker.h | 2 +- .../tests/ReleaseBufferCallback_test.cpp | 77 ++++++++++++++-------- 13 files changed, 192 insertions(+), 118 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index e15e11cc20..29701168e7 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -132,8 +132,7 @@ void BLASTBufferItemConsumer::onSidebandStreamChanged() { BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp& surface, int width, int height, int32_t format) - : mName(name), - mSurfaceControl(surface), + : mSurfaceControl(surface), mSize(width, height), mRequestedSize(mSize), mFormat(format), @@ -150,6 +149,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp context, uint64_t graphicBufferId, +static void releaseBufferCallbackThunk(wp context, const ReleaseCallbackId& id, const sp& releaseFence, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) { sp blastBufferQueue = context.promote(); - ALOGV("releaseBufferCallbackThunk graphicBufferId=%" PRIu64 " blastBufferQueue=%s", - graphicBufferId, blastBufferQueue ? "alive" : "dead"); if (blastBufferQueue) { - blastBufferQueue->releaseBufferCallback(graphicBufferId, releaseFence, transformHint, + blastBufferQueue->releaseBufferCallback(id, releaseFence, transformHint, currentMaxAcquiredBufferCount); + } else { + ALOGV("releaseBufferCallbackThunk %s blastBufferQueue is dead", id.to_string().c_str()); } } -void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId, +void BLASTBufferQueue::releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) { ATRACE_CALL(); std::unique_lock _lock{mMutex}; - BQA_LOGV("releaseBufferCallback graphicBufferId=%" PRIu64, graphicBufferId); + BQA_LOGV("releaseBufferCallback %s", id.to_string().c_str()); if (mSurfaceControl != nullptr) { mTransformHint = transformHint; @@ -343,25 +343,26 @@ void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId, // on a lower refresh rate than the max supported. We only do that for EGL // clients as others don't care about latency const bool isEGL = [&] { - const auto it = mSubmitted.find(graphicBufferId); + const auto it = mSubmitted.find(id); return it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL; }(); const auto numPendingBuffersToHold = isEGL ? std::max(0u, mMaxAcquiredBuffers - currentMaxAcquiredBufferCount) : 0; - mPendingRelease.emplace_back(ReleasedBuffer{graphicBufferId, releaseFence}); + mPendingRelease.emplace_back(ReleasedBuffer{id, releaseFence}); // Release all buffers that are beyond the ones that we need to hold while (mPendingRelease.size() > numPendingBuffersToHold) { const auto releaseBuffer = mPendingRelease.front(); mPendingRelease.pop_front(); - auto it = mSubmitted.find(releaseBuffer.bufferId); + auto it = mSubmitted.find(releaseBuffer.callbackId); if (it == mSubmitted.end()) { - BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %" PRIu64, - graphicBufferId); + BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %s", + releaseBuffer.callbackId.to_string().c_str()); return; } mNumAcquired--; + BQA_LOGV("released %s", id.to_string().c_str()); mBufferItemConsumer->releaseBuffer(it->second, releaseBuffer.releaseFence); mSubmitted.erase(it); processNextBufferLocked(false /* useNextTransaction */); @@ -428,7 +429,9 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { } mNumAcquired++; - mSubmitted[buffer->getId()] = bufferItem; + mLastAcquiredFrameNumber = bufferItem.mFrameNumber; + ReleaseCallbackId releaseCallbackId(buffer->getId(), mLastAcquiredFrameNumber); + mSubmitted[releaseCallbackId] = bufferItem; bool needsDisconnect = false; mBufferItemConsumer->getConnectionEvents(bufferItem.mFrameNumber, &needsDisconnect); @@ -442,7 +445,6 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { incStrong((void*)transactionCallbackThunk); Rect crop = computeCrop(bufferItem); - mLastAcquiredFrameNumber = bufferItem.mFrameNumber; mLastBufferInfo.update(true /* hasBuffer */, bufferItem.mGraphicBuffer->getWidth(), bufferItem.mGraphicBuffer->getHeight(), bufferItem.mTransform, bufferItem.mScalingMode, crop); @@ -451,7 +453,7 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { std::bind(releaseBufferCallbackThunk, wp(this) /* callbackContext */, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4); - t->setBuffer(mSurfaceControl, buffer, releaseBufferCallback); + t->setBuffer(mSurfaceControl, buffer, releaseCallbackId, releaseBufferCallback); t->setDataspace(mSurfaceControl, static_cast(bufferItem.mDataSpace)); t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata); t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage); @@ -510,11 +512,11 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { BQA_LOGV("processNextBufferLocked size=%dx%d mFrameNumber=%" PRIu64 " applyTransaction=%s mTimestamp=%" PRId64 "%s mPendingTransactions.size=%d" - " graphicBufferId=%" PRIu64, + " graphicBufferId=%" PRIu64 "%s", mSize.width, mSize.height, bufferItem.mFrameNumber, toString(applyTransaction), bufferItem.mTimestamp, bufferItem.mIsAutoTimestamp ? "(auto)" : "", - static_cast(mPendingTransactions.size()), - bufferItem.mGraphicBuffer->getId()); + static_cast(mPendingTransactions.size()), bufferItem.mGraphicBuffer->getId(), + bufferItem.mAutoRefresh ? " mAutoRefresh" : ""); } Rect BLASTBufferQueue::computeCrop(const BufferItem& item) { diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index 17499ecd50..98e8b548bd 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -125,7 +125,7 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { for (const auto& data : jankData) { SAFE_PARCEL(output->writeParcelable, data); } - SAFE_PARCEL(output->writeUint64, previousBufferId); + SAFE_PARCEL(output->writeParcelable, previousReleaseCallbackId); return NO_ERROR; } @@ -149,7 +149,7 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readParcelable, &data); jankData.push_back(data); } - SAFE_PARCEL(input->readUint64, &previousBufferId); + SAFE_PARCEL(input->readParcelable, &previousReleaseCallbackId); return NO_ERROR; } @@ -253,11 +253,11 @@ public: stats); } - void onReleaseBuffer(uint64_t graphicBufferId, sp releaseFence, uint32_t transformHint, - uint32_t currentMaxAcquiredBufferCount) override { + void onReleaseBuffer(ReleaseCallbackId callbackId, sp releaseFence, + uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) override { callRemoteAsync(Tag::ON_RELEASE_BUFFER, - graphicBufferId, releaseFence, + callbackId, releaseFence, transformHint, currentMaxAcquiredBufferCount); } @@ -308,4 +308,18 @@ status_t CallbackId::readFromParcel(const Parcel* input) { return NO_ERROR; } +status_t ReleaseCallbackId::writeToParcel(Parcel* output) const { + SAFE_PARCEL(output->writeUint64, bufferId); + SAFE_PARCEL(output->writeUint64, framenumber); + return NO_ERROR; +} + +status_t ReleaseCallbackId::readFromParcel(const Parcel* input) { + SAFE_PARCEL(input->readUint64, &bufferId); + SAFE_PARCEL(input->readUint64, &framenumber); + return NO_ERROR; +} + +const ReleaseCallbackId ReleaseCallbackId::INVALID_ID = ReleaseCallbackId(0, 0); + }; // namespace android diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index fd78309462..6ea070ea2f 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -197,15 +197,16 @@ void TransactionCompletedListener::removeJankListener(const sp } } -void TransactionCompletedListener::setReleaseBufferCallback(uint64_t graphicBufferId, +void TransactionCompletedListener::setReleaseBufferCallback(const ReleaseCallbackId& callbackId, ReleaseBufferCallback listener) { std::scoped_lock lock(mMutex); - mReleaseBufferCallbacks[graphicBufferId] = listener; + mReleaseBufferCallbacks[callbackId] = listener; } -void TransactionCompletedListener::removeReleaseBufferCallback(uint64_t graphicBufferId) { +void TransactionCompletedListener::removeReleaseBufferCallback( + const ReleaseCallbackId& callbackId) { std::scoped_lock lock(mMutex); - mReleaseBufferCallbacks.erase(graphicBufferId); + mReleaseBufferCallbacks.erase(callbackId); } void TransactionCompletedListener::addSurfaceStatsListener(void* context, void* cookie, @@ -319,14 +320,15 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener // and call them. This is a performance optimization when we have a transaction // callback and a release buffer callback happening at the same time to avoid an // additional ipc call from the server. - if (surfaceStats.previousBufferId) { + if (surfaceStats.previousReleaseCallbackId != ReleaseCallbackId::INVALID_ID) { ReleaseBufferCallback callback; { std::scoped_lock lock(mMutex); - callback = popReleaseBufferCallbackLocked(surfaceStats.previousBufferId); + callback = popReleaseBufferCallbackLocked( + surfaceStats.previousReleaseCallbackId); } if (callback) { - callback(surfaceStats.previousBufferId, + callback(surfaceStats.previousReleaseCallbackId, surfaceStats.previousReleaseFence ? surfaceStats.previousReleaseFence : Fence::NO_FENCE, @@ -362,25 +364,26 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener } } -void TransactionCompletedListener::onReleaseBuffer(uint64_t graphicBufferId, sp releaseFence, - uint32_t transformHint, +void TransactionCompletedListener::onReleaseBuffer(ReleaseCallbackId callbackId, + sp releaseFence, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) { ReleaseBufferCallback callback; { std::scoped_lock lock(mMutex); - callback = popReleaseBufferCallbackLocked(graphicBufferId); + callback = popReleaseBufferCallbackLocked(callbackId); } if (!callback) { - ALOGE("Could not call release buffer callback, buffer not found %" PRIu64, graphicBufferId); + ALOGE("Could not call release buffer callback, buffer not found %s", + callbackId.to_string().c_str()); return; } - callback(graphicBufferId, releaseFence, transformHint, currentMaxAcquiredBufferCount); + callback(callbackId, releaseFence, transformHint, currentMaxAcquiredBufferCount); } ReleaseBufferCallback TransactionCompletedListener::popReleaseBufferCallbackLocked( - uint64_t graphicBufferId) { + const ReleaseCallbackId& callbackId) { ReleaseBufferCallback callback; - auto itr = mReleaseBufferCallbacks.find(graphicBufferId); + auto itr = mReleaseBufferCallbacks.find(callbackId); if (itr == mReleaseBufferCallbacks.end()) { return nullptr; } @@ -1258,7 +1261,7 @@ SurfaceComposerClient::Transaction::setTransformToDisplayInverse(const sp& sc, const sp& buffer, + const sp& sc, const sp& buffer, const ReleaseCallbackId& id, ReleaseBufferCallback callback) { layer_state_t* s = getLayerState(sc); if (!s) { @@ -1271,7 +1274,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe if (mIsAutoTimestamp) { mDesiredPresentTime = systemTime(); } - setReleaseBufferCallback(s, callback); + setReleaseBufferCallback(s, id, callback); registerSurfaceControlForCallback(sc); @@ -1286,10 +1289,13 @@ void SurfaceComposerClient::Transaction::removeReleaseBufferCallback(layer_state s->what &= ~static_cast(layer_state_t::eReleaseBufferListenerChanged); s->releaseBufferListener = nullptr; - TransactionCompletedListener::getInstance()->removeReleaseBufferCallback(s->buffer->getId()); + auto listener = TransactionCompletedListener::getInstance(); + listener->removeReleaseBufferCallback(s->releaseCallbackId); + s->releaseCallbackId = ReleaseCallbackId::INVALID_ID; } void SurfaceComposerClient::Transaction::setReleaseBufferCallback(layer_state_t* s, + const ReleaseCallbackId& id, ReleaseBufferCallback callback) { if (!callback) { return; @@ -1303,8 +1309,9 @@ void SurfaceComposerClient::Transaction::setReleaseBufferCallback(layer_state_t* s->what |= layer_state_t::eReleaseBufferListenerChanged; s->releaseBufferListener = TransactionCompletedListener::getIInstance(); + s->releaseCallbackId = id; auto listener = TransactionCompletedListener::getInstance(); - listener->setReleaseBufferCallback(s->buffer->getId(), callback); + listener->setReleaseBufferCallback(id, callback); } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setAcquireFence( diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 26c7285ab0..0d6a673b02 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -89,7 +89,7 @@ public: void transactionCallback(nsecs_t latchTime, const sp& presentFence, const std::vector& stats); - void releaseBufferCallback(uint64_t graphicBufferId, const sp& releaseFence, + void releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount); void setNextTransaction(SurfaceComposerClient::Transaction *t); void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber); @@ -144,13 +144,14 @@ private: // Keep a reference to the submitted buffers so we can release when surfaceflinger drops the // buffer or the buffer has been presented and a new buffer is ready to be presented. - std::unordered_map mSubmitted GUARDED_BY(mMutex); + std::unordered_map mSubmitted + GUARDED_BY(mMutex); // Keep a queue of the released buffers instead of immediately releasing // the buffers back to the buffer queue. This would be controlled by SF // setting the max acquired buffer count. struct ReleasedBuffer { - uint64_t bufferId; + ReleaseCallbackId callbackId; sp releaseFence; }; std::deque mPendingRelease GUARDED_BY(mMutex); diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index d286c34ec8..937095c543 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -53,6 +53,36 @@ struct CallbackIdHash { std::size_t operator()(const CallbackId& key) const { return std::hash()(key.id); } }; +class ReleaseCallbackId : public Parcelable { +public: + static const ReleaseCallbackId INVALID_ID; + + uint64_t bufferId; + uint64_t framenumber; + ReleaseCallbackId() {} + ReleaseCallbackId(uint64_t bufferId, uint64_t framenumber) + : bufferId(bufferId), framenumber(framenumber) {} + status_t writeToParcel(Parcel* output) const override; + status_t readFromParcel(const Parcel* input) override; + + bool operator==(const ReleaseCallbackId& rhs) const { + return bufferId == rhs.bufferId && framenumber == rhs.framenumber; + } + bool operator!=(const ReleaseCallbackId& rhs) const { return !operator==(rhs); } + std::string to_string() const { + if (*this == INVALID_ID) return "INVALID_ID"; + + return "bufferId:" + std::to_string(bufferId) + + " framenumber:" + std::to_string(framenumber); + } +}; + +struct ReleaseBufferCallbackIdHash { + std::size_t operator()(const ReleaseCallbackId& key) const { + return std::hash()(key.bufferId); + } +}; + class FrameEventHistoryStats : public Parcelable { public: status_t writeToParcel(Parcel* output) const override; @@ -103,7 +133,7 @@ public: SurfaceStats(const sp& sc, nsecs_t time, const sp& prevReleaseFence, uint32_t hint, uint32_t currentMaxAcquiredBuffersCount, FrameEventHistoryStats frameEventStats, std::vector jankData, - uint64_t previousBufferId) + ReleaseCallbackId previousReleaseCallbackId) : surfaceControl(sc), acquireTime(time), previousReleaseFence(prevReleaseFence), @@ -111,7 +141,7 @@ public: currentMaxAcquiredBufferCount(currentMaxAcquiredBuffersCount), eventStats(frameEventStats), jankData(std::move(jankData)), - previousBufferId(previousBufferId) {} + previousReleaseCallbackId(previousReleaseCallbackId) {} sp surfaceControl; nsecs_t acquireTime = -1; @@ -120,7 +150,7 @@ public: uint32_t currentMaxAcquiredBufferCount = 0; FrameEventHistoryStats eventStats; std::vector jankData; - uint64_t previousBufferId; + ReleaseCallbackId previousReleaseCallbackId; }; class TransactionStats : public Parcelable { @@ -161,7 +191,7 @@ public: virtual void onTransactionCompleted(ListenerStats stats) = 0; - virtual void onReleaseBuffer(uint64_t graphicBufferId, sp releaseFence, + virtual void onReleaseBuffer(ReleaseCallbackId callbackId, sp releaseFence, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) = 0; }; diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 16430b324d..8ac1e5d9e9 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -234,6 +234,12 @@ struct layer_state_t { // layers only. The callback includes a release fence as well as the graphic // buffer id to identify the buffer. sp releaseBufferListener; + + // Keeps track of the release callback id associated with the listener. This + // is not sent to the server since the id can be reconstructed there. This + // is used to remove the old callback from the client process map if it is + // overwritten by another setBuffer call. + ReleaseCallbackId releaseCallbackId; }; struct ComposerState { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 13994fd9eb..c2963b58df 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -82,7 +82,7 @@ using TransactionCompletedCallback = std::function& /*presentFence*/, const std::vector& /*stats*/)>; using ReleaseBufferCallback = - std::function& /*releaseFence*/, + std::function& /*releaseFence*/, uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount)>; using SurfaceStatsCallback = @@ -397,8 +397,9 @@ public: void cacheBuffers(); void registerSurfaceControlForCallback(const sp& sc); - void setReleaseBufferCallback(layer_state_t* state, ReleaseBufferCallback callback); - void removeReleaseBufferCallback(layer_state_t* state); + void setReleaseBufferCallback(layer_state_t*, const ReleaseCallbackId&, + ReleaseBufferCallback); + void removeReleaseBufferCallback(layer_state_t*); public: Transaction(); @@ -470,6 +471,7 @@ public: Transaction& setTransformToDisplayInverse(const sp& sc, bool transformToDisplayInverse); Transaction& setBuffer(const sp& sc, const sp& buffer, + const ReleaseCallbackId& id = ReleaseCallbackId::INVALID_ID, ReleaseBufferCallback callback = nullptr); Transaction& setCachedBuffer(const sp& sc, int32_t bufferId); Transaction& setAcquireFence(const sp& sc, const sp& fence); @@ -678,7 +680,7 @@ class TransactionCompletedListener : public BnTransactionCompletedListener { std::unordered_map mCallbacks GUARDED_BY(mMutex); std::multimap, sp> mJankListeners GUARDED_BY(mMutex); - std::unordered_map + std::unordered_map mReleaseBufferCallbacks GUARDED_BY(mMutex); // This is protected by mSurfaceStatsListenerMutex, but GUARDED_BY isn't supported for @@ -717,16 +719,16 @@ public: SurfaceStatsCallback listener); void removeSurfaceStatsListener(void* context, void* cookie); - void setReleaseBufferCallback(uint64_t /* graphicsBufferId */, ReleaseBufferCallback); - void removeReleaseBufferCallback(uint64_t /* graphicsBufferId */); + void setReleaseBufferCallback(const ReleaseCallbackId&, ReleaseBufferCallback); + void removeReleaseBufferCallback(const ReleaseCallbackId&); // BnTransactionCompletedListener overrides void onTransactionCompleted(ListenerStats stats) override; - void onReleaseBuffer(uint64_t /* graphicsBufferId */, sp releaseFence, - uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) override; + void onReleaseBuffer(ReleaseCallbackId, sp releaseFence, uint32_t transformHint, + uint32_t currentMaxAcquiredBufferCount) override; private: - ReleaseBufferCallback popReleaseBufferCallbackLocked(uint64_t /* graphicsBufferId */); + ReleaseBufferCallback popReleaseBufferCallbackLocked(const ReleaseCallbackId&); }; } // namespace android diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h index cd3d80e809..760c8b9f3c 100644 --- a/services/surfaceflinger/BufferLayer.h +++ b/services/surfaceflinger/BufferLayer.h @@ -133,6 +133,7 @@ protected: bool mTransformToDisplayInverse{false}; std::shared_ptr mBuffer; + uint64_t mFrameNumber; int mBufferSlot{BufferQueue::INVALID_BUFFER_SLOT}; bool mFrameLatencyNeeded{false}; diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 6b5cf04536..8cf0d66a08 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -44,25 +44,18 @@ namespace android { using PresentState = frametimeline::SurfaceFrame::PresentState; namespace { void callReleaseBufferCallback(const sp& listener, - const sp& buffer, const sp& releaseFence, - uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) { + const sp& buffer, uint64_t framenumber, + const sp& releaseFence, uint32_t transformHint, + uint32_t currentMaxAcquiredBufferCount) { if (!listener) { return; } - listener->onReleaseBuffer(buffer->getId(), releaseFence ? releaseFence : Fence::NO_FENCE, - transformHint, currentMaxAcquiredBufferCount); + listener->onReleaseBuffer({buffer->getId(), framenumber}, + releaseFence ? releaseFence : Fence::NO_FENCE, transformHint, + currentMaxAcquiredBufferCount); } } // namespace -// clang-format off -const std::array BufferStateLayer::IDENTITY_MATRIX{ - 1, 0, 0, 0, - 0, 1, 0, 0, - 0, 0, 1, 0, - 0, 0, 0, 1 -}; -// clang-format on - BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args) : BufferLayer(args), mHwcSlotGenerator(new HwcSlotGenerator()) { mDrawingState.dataspace = ui::Dataspace::V0_SRGB; @@ -75,8 +68,8 @@ BufferStateLayer::~BufferStateLayer() { // issue with the clone layer trying to use the texture. if (mBufferInfo.mBuffer != nullptr && !isClone()) { callReleaseBufferCallback(mDrawingState.releaseBufferListener, - mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFence, - mTransformHint, + mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFrameNumber, + mBufferInfo.mFence, mTransformHint, mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( mOwnerUid)); } @@ -87,7 +80,7 @@ status_t BufferStateLayer::addReleaseFence(const sp& ch, if (ch == nullptr) { return OK; } - ch->previousBufferId = mPreviousBufferId; + ch->previousReleaseCallbackId = mPreviousReleaseCallbackId; if (!ch->previousReleaseFence.get()) { ch->previousReleaseFence = fence; return OK; @@ -214,7 +207,7 @@ void BufferStateLayer::releasePendingBuffer(nsecs_t dequeueReadyTime) { // see BufferStateLayer::onLayerDisplayed. for (auto& handle : mDrawingState.callbackHandles) { if (handle->releasePreviousBuffer) { - handle->previousBufferId = mPreviousBufferId; + handle->previousReleaseCallbackId = mPreviousReleaseCallbackId; break; } } @@ -438,8 +431,8 @@ bool BufferStateLayer::setBuffer(const std::shared_ptrgetBuffer(), mDrawingState.acquireFence, - mTransformHint, + mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber, + mDrawingState.acquireFence, mTransformHint, mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( mOwnerUid)); decrementPendingBufferCount(); @@ -684,7 +677,7 @@ uint64_t BufferStateLayer::getFrameNumber(nsecs_t /*expectedPresentTime*/) const * DeferTransactionUntil -> frameNumber = 2 * Random other stuff * } - * Now imagine getHeadFrameNumber returned mDrawingState.mFrameNumber (or mCurrentFrameNumber). + * Now imagine mFrameNumber returned mDrawingState.frameNumber (or mCurrentFrameNumber). * Prior to doTransaction SurfaceFlinger will call notifyAvailableFrames, but because we * haven't swapped mDrawingState to mDrawingState yet we will think the sync point * is not ready. So we will return false from applyPendingState and not swap @@ -792,9 +785,10 @@ status_t BufferStateLayer::updateActiveBuffer() { decrementPendingBufferCount(); } - mPreviousBufferId = getCurrentBufferId(); + mPreviousReleaseCallbackId = {getCurrentBufferId(), mBufferInfo.mFrameNumber}; mBufferInfo.mBuffer = s.buffer; mBufferInfo.mFence = s.acquireFence; + mBufferInfo.mFrameNumber = s.frameNumber; return NO_ERROR; } @@ -969,8 +963,8 @@ void BufferStateLayer::bufferMayChange(const sp& newBuffer) { // then we will drop a buffer and should decrement the pending buffer count and // call any release buffer callbacks if set. callReleaseBufferCallback(mDrawingState.releaseBufferListener, - mDrawingState.buffer->getBuffer(), mDrawingState.acquireFence, - mTransformHint, + mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber, + mDrawingState.acquireFence, mTransformHint, mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( mOwnerUid)); decrementPendingBufferCount(); diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index 27470182bf..e5674785af 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -143,15 +143,8 @@ private: bool bufferNeedsFiltering() const override; - static const std::array IDENTITY_MATRIX; - - std::unique_ptr mTextureImage; - - mutable uint64_t mFrameNumber{0}; - uint64_t mFrameCounter{0}; - sp mPreviousReleaseFence; - uint64_t mPreviousBufferId = 0; + ReleaseCallbackId mPreviousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; uint64_t mPreviousReleasedFrameNumber = 0; bool mReleasePreviousBuffer = false; diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp index fdf16a797f..6af69f0ef2 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.cpp +++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp @@ -237,7 +237,8 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle->previousReleaseFence, handle->transformHint, handle->currentMaxAcquiredBufferCount, - eventStats, jankData, handle->previousBufferId); + eventStats, jankData, + handle->previousReleaseCallbackId); } return NO_ERROR; } diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index 444bec646e..6f4d812ec5 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -51,7 +51,7 @@ public: nsecs_t refreshStartTime = 0; nsecs_t dequeueReadyTime = 0; uint64_t frameNumber = 0; - uint64_t previousBufferId = 0; + ReleaseCallbackId previousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; }; class TransactionCallbackInvoker { diff --git a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp index 5aa809dc8b..579a26ebf4 100644 --- a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp +++ b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp @@ -29,7 +29,7 @@ using android::hardware::graphics::common::V1_1::BufferUsage; // b/181132765 - disabled until cuttlefish failures are investigated class ReleaseBufferCallbackHelper { public: - static void function(void* callbackContext, uint64_t graphicsBufferId, + static void function(void* callbackContext, ReleaseCallbackId callbackId, const sp& releaseFence, uint32_t /*currentMaxAcquiredBufferCount*/) { if (!callbackContext) { @@ -38,11 +38,11 @@ public: ReleaseBufferCallbackHelper* helper = static_cast(callbackContext); std::lock_guard lock(helper->mMutex); - helper->mCallbackDataQueue.emplace(graphicsBufferId, releaseFence); + helper->mCallbackDataQueue.emplace(callbackId, releaseFence); helper->mConditionVariable.notify_all(); } - void getCallbackData(uint64_t* bufferId) { + void getCallbackData(ReleaseCallbackId* callbackId) { std::unique_lock lock(mMutex); if (mCallbackDataQueue.empty()) { if (!mConditionVariable.wait_for(lock, std::chrono::seconds(3), @@ -53,7 +53,7 @@ public: auto callbackData = mCallbackDataQueue.front(); mCallbackDataQueue.pop(); - *bufferId = callbackData.first; + *callbackId = callbackData.first; } void verifyNoCallbacks() { @@ -72,7 +72,7 @@ public: std::mutex mMutex; std::condition_variable mConditionVariable; - std::queue>> mCallbackDataQueue; + std::queue>> mCallbackDataQueue; }; class ReleaseBufferCallbackTest : public LayerTransactionTest { @@ -82,10 +82,11 @@ public: } static void submitBuffer(const sp& layer, sp buffer, - sp fence, CallbackHelper& callback, + sp fence, CallbackHelper& callback, const ReleaseCallbackId& id, ReleaseBufferCallbackHelper& releaseCallback) { Transaction t; - t.setBuffer(layer, buffer, releaseCallback.getCallback()); + t.setFrameNumber(layer, id.framenumber); + t.setBuffer(layer, buffer, id, releaseCallback.getCallback()); t.setAcquireFence(layer, fence); t.addTransactionCompletedCallback(callback.function, callback.getContext()); t.apply(); @@ -98,10 +99,10 @@ public: } static void waitForReleaseBufferCallback(ReleaseBufferCallbackHelper& releaseCallback, - uint64_t expectedReleaseBufferId) { - uint64_t actualReleaseBufferId; + const ReleaseCallbackId& expectedCallbackId) { + ReleaseCallbackId actualReleaseBufferId; releaseCallback.getCallbackData(&actualReleaseBufferId); - EXPECT_EQ(expectedReleaseBufferId, actualReleaseBufferId); + EXPECT_EQ(expectedCallbackId, actualReleaseBufferId); releaseCallback.verifyNoCallbacks(); } static ReleaseBufferCallbackHelper* getReleaseBufferCallbackHelper() { @@ -116,6 +117,10 @@ public: BufferUsage::COMPOSER_OVERLAY, "test"); } + static uint64_t generateFrameNumber() { + static uint64_t sFrameNumber = 0; + return ++sFrameNumber; + } }; TEST_F(ReleaseBufferCallbackTest, DISABLED_PresentBuffer) { @@ -125,7 +130,9 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_PresentBuffer) { // If a buffer is being presented, we should not emit a release callback. sp firstBuffer = getBuffer(); - submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback); + ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, firstBufferCallbackId, + *releaseCallback); ExpectedResult expected; expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED); @@ -143,13 +150,15 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_PresentBuffer) { // If a presented buffer is replaced, we should emit a release callback for the // previously presented buffer. sp secondBuffer = getBuffer(); - submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback); + ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, secondBufferCallbackId, + *releaseCallback); expected = ExpectedResult(); expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED, ExpectedResult::PreviousBuffer::RELEASED); ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected)); - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } TEST_F(ReleaseBufferCallbackTest, DISABLED_OffScreenLayer) { @@ -160,7 +169,9 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_OffScreenLayer) { // If a buffer is being presented, we should not emit a release callback. sp firstBuffer = getBuffer(); - submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback); + ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, firstBufferCallbackId, + *releaseCallback); ExpectedResult expected; expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED); @@ -184,23 +195,27 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_OffScreenLayer) { // If a presented buffer is replaced, we should emit a release callback for the // previously presented buffer. sp secondBuffer = getBuffer(); - submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback); + ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, secondBufferCallbackId, + *releaseCallback); expected = ExpectedResult(); expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED, ExpectedResult::PreviousBuffer::NOT_RELEASED); ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected)); - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); // If continue to submit buffer we continue to get release callbacks sp thirdBuffer = getBuffer(); - submitBuffer(layer, thirdBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback); + ReleaseCallbackId thirdBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, thirdBuffer, Fence::NO_FENCE, transactionCallback, thirdBufferCallbackId, + *releaseCallback); expected = ExpectedResult(); expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED, ExpectedResult::PreviousBuffer::NOT_RELEASED); ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected)); - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBufferCallbackId)); } TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_layerdestroy) { @@ -210,7 +225,9 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_layerdestroy) { // If a buffer is being presented, we should not emit a release callback. sp firstBuffer = getBuffer(); - submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback); + ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, firstBufferCallbackId, + *releaseCallback); { ExpectedResult expected; expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, @@ -225,7 +242,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_layerdestroy) { t.apply(); layer = nullptr; - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } // Destroying a never presented layer emits a callback. @@ -242,7 +259,9 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_OffScreenLayerDestroy) // Submitting a buffer does not emit a callback. sp firstBuffer = getBuffer(); - submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback); + ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, firstBufferCallbackId, + *releaseCallback); { ExpectedResult expected; expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, @@ -253,19 +272,21 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_OffScreenLayerDestroy) // Submitting a second buffer will replace the drawing state buffer and emit a callback. sp secondBuffer = getBuffer(); - submitBuffer(layer, secondBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback); + ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); + submitBuffer(layer, secondBuffer, Fence::NO_FENCE, *transactionCallback, secondBufferCallbackId, + *releaseCallback); { ExpectedResult expected; expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer, ExpectedResult::Buffer::NOT_ACQUIRED); ASSERT_NO_FATAL_FAILURE(waitForCallback(*transactionCallback, expected)); ASSERT_NO_FATAL_FAILURE( - waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId())); + waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } // Destroying the offscreen layer emits a callback. layer = nullptr; - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBufferCallbackId)); } TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { @@ -275,12 +296,13 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { // If a buffer is being presented, we should not emit a release callback. sp firstBuffer = getBuffer(); + ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber()); // Try to present 100ms in the future nsecs_t time = systemTime() + std::chrono::nanoseconds(100ms).count(); Transaction t; - t.setBuffer(layer, firstBuffer, releaseCallback->getCallback()); + t.setBuffer(layer, firstBuffer, firstBufferCallbackId, releaseCallback->getCallback()); t.setAcquireFence(layer, Fence::NO_FENCE); t.addTransactionCompletedCallback(transactionCallback.function, transactionCallback.getContext()); @@ -295,7 +317,8 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { // Dropping frames in transaction queue emits a callback sp secondBuffer = getBuffer(); - t.setBuffer(layer, secondBuffer, releaseCallback->getCallback()); + ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); + t.setBuffer(layer, secondBuffer, secondBufferCallbackId, releaseCallback->getCallback()); t.setAcquireFence(layer, Fence::NO_FENCE); t.addTransactionCompletedCallback(transactionCallback.function, transactionCallback.getContext()); @@ -307,7 +330,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { ExpectedResult::Buffer::NOT_ACQUIRED, ExpectedResult::PreviousBuffer::RELEASED); ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected)); - ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId())); + ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } } // namespace android -- cgit v1.2.3-59-g8ed1b From 5fa91c276a6f67a33effceee27ef91d2cd935002 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 29 Jun 2021 14:30:48 -0700 Subject: BBQ: Update the transform hint if the SC layer handle does not change We are now passing the transform hint from WM to the client. In BBQ, we were not getting the updated transform hint in some cases because we did not update the SurfaceControl if the layer handle was the same. Fix this by always updating the SC object in BBQ. Test: rotate phone with messages app and check buffer transforms Fixes: 184842607 Change-Id: I83158f810a9820285c84ecfff294689f63ff3113 --- libs/gui/BLASTBufferQueue.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 29701168e7..ac8feaa0c2 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -197,15 +197,18 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, } SurfaceComposerClient::Transaction t; + const bool setBackpressureFlag = !SurfaceControl::isSameSurface(mSurfaceControl, surface); bool applyTransaction = false; - if (!SurfaceControl::isSameSurface(mSurfaceControl, surface)) { - mSurfaceControl = surface; - t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure, - layer_state_t::eEnableBackpressure); - applyTransaction = true; - } + // Always update the native object even though they might have the same layer handle, so we can + // get the updated transform hint from WM. + mSurfaceControl = surface; if (mSurfaceControl != nullptr) { + if (setBackpressureFlag) { + t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure, + layer_state_t::eEnableBackpressure); + applyTransaction = true; + } mTransformHint = mSurfaceControl->getTransformHint(); mBufferItemConsumer->setTransformHint(mTransformHint); } @@ -221,7 +224,7 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, // We only need to update the scale if we've received at least one buffer. The reason // for this is the scale is calculated based on the requested size and buffer size. // If there's no buffer, the scale will always be 1. - if (mLastBufferInfo.hasBuffer) { + if (mSurfaceControl != nullptr && mLastBufferInfo.hasBuffer) { t.setDestinationFrame(mSurfaceControl, Rect(0, 0, newSize.getWidth(), newSize.getHeight())); } -- cgit v1.2.3-59-g8ed1b From 9c16128241425c91249379aacc3bca31d04ef12f Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 7 Jul 2021 16:52:34 -0700 Subject: BlastBufferQueue: Keep transform hint in Surface consistent We should only update the transform hint in Surface when connecting to the queue or queuing a buffer. Otherwise, the transform hint value used when dequeuing the buffer will not align with the value queried from the Surface. This inconsistency was causing BBQ to reject buffers and we suspect this was causing an issue where BBQ will remain in this inconsistent state rejecting all buffers until the app changed orientation. Test: rotate device, check for rejected buffers, run through scenario in bug Bug: b/191841127 Change-Id: Id84a3a650ce68878e5638b942249c11558fd29eb Merged-In: Id84a3a650ce68878e5638b942249c11558fd29eb --- libs/gui/BLASTBufferQueue.cpp | 21 +++++++-------- libs/gui/tests/BLASTBufferQueue_test.cpp | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 12 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index ac8feaa0c2..7d57d8b02c 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -171,6 +171,8 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp& surface, uint32_t width, uint32_t height, int32_t format) { std::unique_lock _lock{mMutex}; - BQA_LOGV("update width=%d height=%d format=%d", width, height, format); if (mFormat != format) { mFormat = format; mBufferItemConsumer->setDefaultBufferFormat(convertBufferFormat(format)); @@ -212,6 +213,8 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, mTransformHint = mSurfaceControl->getTransformHint(); mBufferItemConsumer->setTransformHint(mTransformHint); } + BQA_LOGV("update width=%d height=%d format=%d mTransformHint=%d", width, height, format, + mTransformHint); ui::Size newSize(width, height); if (mRequestedSize != newSize) { @@ -267,6 +270,7 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const spsetTransformHint(mTransformHint); + BQA_LOGV("updated mTransformHint=%d", mTransformHint); // Update frametime stamps if the frame was latched and presented, indicated by a // valid latch time. if (stat.latchTime > 0) { @@ -339,6 +343,7 @@ void BLASTBufferQueue::releaseBufferCallback(const ReleaseCallbackId& id, mTransformHint = transformHint; mSurfaceControl->setTransformHint(transformHint); mBufferItemConsumer->setTransformHint(mTransformHint); + BQA_LOGV("updated mTransformHint=%d", mTransformHint); } // Calculate how many buffers we need to hold before we release them back @@ -422,7 +427,7 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { } if (rejectBuffer(bufferItem)) { - BQA_LOGE("rejecting buffer:active_size=%dx%d, requested_size=%dx%d" + BQA_LOGE("rejecting buffer:active_size=%dx%d, requested_size=%dx%d " "buffer{size=%dx%d transform=%d}", mSize.width, mSize.height, mRequestedSize.width, mRequestedSize.height, buffer->getWidth(), buffer->getHeight(), bufferItem.mTransform); @@ -515,11 +520,11 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { BQA_LOGV("processNextBufferLocked size=%dx%d mFrameNumber=%" PRIu64 " applyTransaction=%s mTimestamp=%" PRId64 "%s mPendingTransactions.size=%d" - " graphicBufferId=%" PRIu64 "%s", + " graphicBufferId=%" PRIu64 "%s transform=%d", mSize.width, mSize.height, bufferItem.mFrameNumber, toString(applyTransaction), bufferItem.mTimestamp, bufferItem.mIsAutoTimestamp ? "(auto)" : "", static_cast(mPendingTransactions.size()), bufferItem.mGraphicBuffer->getId(), - bufferItem.mAutoRefresh ? " mAutoRefresh" : ""); + bufferItem.mAutoRefresh ? " mAutoRefresh" : "", bufferItem.mTransform); } Rect BLASTBufferQueue::computeCrop(const BufferItem& item) { @@ -646,14 +651,6 @@ public: status_t setFrameTimelineInfo(const FrameTimelineInfo& frameTimelineInfo) override { return mBbq->setFrameTimelineInfo(frameTimelineInfo); } - protected: - uint32_t getTransformHint() const override { - if (mStickyTransform == 0 && !transformToDisplayInverse()) { - return mBbq->getLastTransformHint(); - } else { - return 0; - } - } }; // TODO: Can we coalesce this with frame updates? Need to confirm diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp index 6ff67aa7cb..9082d275a2 100644 --- a/libs/gui/tests/BLASTBufferQueue_test.cpp +++ b/libs/gui/tests/BLASTBufferQueue_test.cpp @@ -71,6 +71,10 @@ public: return mBlastBufferQueueAdapter->mSurfaceControl; } + sp getSurface() { + return mBlastBufferQueueAdapter->getSurface(false /* includeSurfaceControlHandle */); + } + void waitForCallbacks() { std::unique_lock lock{mBlastBufferQueueAdapter->mMutex}; // Wait until all but one of the submitted buffers have been released. @@ -758,6 +762,48 @@ TEST_F(BLASTBufferQueueTest, OutOfOrderTransactionTest) { {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight / 2})); } +TEST_F(BLASTBufferQueueTest, TransformHint) { + // Transform hint is provided to BBQ via the surface control passed by WM + mSurfaceControl->setTransformHint(ui::Transform::ROT_90); + + BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight); + sp igbProducer = adapter.getIGraphicBufferProducer(); + ASSERT_NE(nullptr, igbProducer.get()); + ASSERT_EQ(NO_ERROR, igbProducer->setMaxDequeuedBufferCount(2)); + sp surface = adapter.getSurface(); + + // Before connecting to the surface, we do not get a valid transform hint + int transformHint; + surface->query(NATIVE_WINDOW_TRANSFORM_HINT, &transformHint); + ASSERT_EQ(ui::Transform::ROT_0, transformHint); + + ASSERT_EQ(NO_ERROR, + surface->connect(NATIVE_WINDOW_API_CPU, new TestProducerListener(igbProducer))); + + // After connecting to the surface, we should get the correct hint. + surface->query(NATIVE_WINDOW_TRANSFORM_HINT, &transformHint); + ASSERT_EQ(ui::Transform::ROT_90, transformHint); + + ANativeWindow_Buffer buffer; + surface->lock(&buffer, nullptr /* inOutDirtyBounds */); + + // Transform hint is updated via callbacks or surface control updates + mSurfaceControl->setTransformHint(ui::Transform::ROT_0); + adapter.update(mSurfaceControl, mDisplayWidth, mDisplayHeight); + + // The hint does not change and matches the value used when dequeueing the buffer. + surface->query(NATIVE_WINDOW_TRANSFORM_HINT, &transformHint); + ASSERT_EQ(ui::Transform::ROT_90, transformHint); + + surface->unlockAndPost(); + + // After queuing the buffer, we get the updated transform hint + surface->query(NATIVE_WINDOW_TRANSFORM_HINT, &transformHint); + ASSERT_EQ(ui::Transform::ROT_0, transformHint); + + adapter.waitForCallbacks(); +} + class BLASTBufferQueueTransformTest : public BLASTBufferQueueTest { public: void test(uint32_t tr) { -- cgit v1.2.3-59-g8ed1b From 084514aadca64e44b49eac584a89fef887fbb30c Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Fri, 30 Jul 2021 16:07:42 -0700 Subject: SurfaceView: Synchronize destframe updates with SurfaceView size changes This CL fixes one of the issues with SurfaceView parent frame and content syncing. With BLAST, we have two surface controls each setting a scale. The parent surface control sets a scale based on the requested surface size and the SurfaceView layout size. The BlastBufferQueue surface control scales the buffer to the requested buffer size if the buffer has the appropriate scale mode. The destination frame controls the second scaling and it must be applied with the parent surface scale changes. This cl fixes flickers where the requested fixed surface size changes without any view size changes. This cl allows the caller to pass in a transaction to BLASTBufferQueue#update which is updated with the destination frame changes. This transaction can then be applied with the parent surface changes. This also fixes an issue where destination Frame was being set on every buffer update and when we updated the BlastBufferQueue size. Since buffer transactions can be queued up on the server side, a stale value maybe applied for a few frames causing flickers. Fixes: 194458377 Test: bug repro steps Test: atest SurfaceViewSyncTest#testSurfaceViewSetFixedSize Change-Id: I216586842ff45bfd398659b5cc0c89eaf51394ff --- libs/gui/BLASTBufferQueue.cpp | 25 +++++++++++++++++-------- libs/gui/include/gui/BLASTBufferQueue.h | 3 ++- 2 files changed, 19 insertions(+), 9 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 7d57d8b02c..56a9683773 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -166,9 +166,10 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const spgetTransformHint(); mBufferItemConsumer->setTransformHint(mTransformHint); SurfaceComposerClient::Transaction() - .setFlags(surface, layer_state_t::eEnableBackpressure, - layer_state_t::eEnableBackpressure) - .apply(); + .setFlags(surface, layer_state_t::eEnableBackpressure, + layer_state_t::eEnableBackpressure) + .setApplyToken(mApplyToken) + .apply(); mNumAcquired = 0; mNumFrameAvailable = 0; BQA_LOGV("BLASTBufferQueue created width=%d height=%d format=%d mTransformHint=%d", width, @@ -190,7 +191,7 @@ BLASTBufferQueue::~BLASTBufferQueue() { } void BLASTBufferQueue::update(const sp& surface, uint32_t width, uint32_t height, - int32_t format) { + int32_t format, SurfaceComposerClient::Transaction* outTransaction) { std::unique_lock _lock{mMutex}; if (mFormat != format) { mFormat = format; @@ -227,15 +228,18 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, // We only need to update the scale if we've received at least one buffer. The reason // for this is the scale is calculated based on the requested size and buffer size. // If there's no buffer, the scale will always be 1. + SurfaceComposerClient::Transaction* destFrameTransaction = + (outTransaction) ? outTransaction : &t; if (mSurfaceControl != nullptr && mLastBufferInfo.hasBuffer) { - t.setDestinationFrame(mSurfaceControl, - Rect(0, 0, newSize.getWidth(), newSize.getHeight())); + destFrameTransaction->setDestinationFrame(mSurfaceControl, + Rect(0, 0, newSize.getWidth(), + newSize.getHeight())); } applyTransaction = true; } } if (applyTransaction) { - t.apply(); + t.setApplyToken(mApplyToken).apply(); } } @@ -453,6 +457,9 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { incStrong((void*)transactionCallbackThunk); Rect crop = computeCrop(bufferItem); + const bool updateDestinationFrame = + bufferItem.mScalingMode == NATIVE_WINDOW_SCALING_MODE_FREEZE || + !mLastBufferInfo.hasBuffer; mLastBufferInfo.update(true /* hasBuffer */, bufferItem.mGraphicBuffer->getWidth(), bufferItem.mGraphicBuffer->getHeight(), bufferItem.mTransform, bufferItem.mScalingMode, crop); @@ -470,7 +477,9 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { t->addTransactionCompletedCallback(transactionCallbackThunk, static_cast(this)); mSurfaceControlsWithPendingCallback.push(mSurfaceControl); - t->setDestinationFrame(mSurfaceControl, Rect(0, 0, mSize.getWidth(), mSize.getHeight())); + if (updateDestinationFrame) { + t->setDestinationFrame(mSurfaceControl, Rect(0, 0, mSize.getWidth(), mSize.getHeight())); + } t->setBufferCrop(mSurfaceControl, crop); t->setTransform(mSurfaceControl, bufferItem.mTransform); t->setTransformToDisplayInverse(mSurfaceControl, bufferItem.mTransformToDisplayInverse); diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 0d6a673b02..ea9b1c68af 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -96,7 +96,8 @@ public: void setTransactionCompleteCallback(uint64_t frameNumber, std::function&& transactionCompleteCallback); - void update(const sp& surface, uint32_t width, uint32_t height, int32_t format); + void update(const sp& surface, uint32_t width, uint32_t height, int32_t format, + SurfaceComposerClient::Transaction* outTransaction = nullptr); void flushShadowQueue() {} status_t setFrameRate(float frameRate, int8_t compatibility, bool shouldBeSeamless); -- cgit v1.2.3-59-g8ed1b