From b6ddf525be3c2abbde59ae1533494b18a7961087 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Mon, 24 Jun 2024 18:56:08 -0500 Subject: Fix DisplayState sanitization. Bug: 347307756 Flag: EXEMPT bugfix Test: CredentialsTest Change-Id: I500ff9a5bd356845e2d0be4d1430448f448c4e8a --- libs/gui/SurfaceComposerClient.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index af91bb3ae2..5db539497c 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1059,7 +1059,8 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { uncacheBuffer.token = BufferCache::getInstance().getToken(); uncacheBuffer.id = cacheId; Vector composerStates; - status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, {}, + Vector displayStates; + status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, displayStates, ISurfaceComposer::eOneWay, Transaction::getDefaultApplyToken(), {}, systemTime(), true, {uncacheBuffer}, false, {}, generateId(), {}); -- cgit v1.2.3-59-g8ed1b From d56514e908eb4b0738a56930a4d9e8e81a545bcf Mon Sep 17 00:00:00 2001 From: Pascal Mütschard Date: Fri, 24 May 2024 17:37:13 +0200 Subject: Jank callback API refactor. Removes the old work-arounds for missing jank callbacks. Removes the jank data from the transaction completed callback. Adds new function to ISurfaceComposer to register jank listeners. With the new API, jank data is only sent over binder periodically (every ~50 frames) and on a background thread. It is also only tracked for layers where there is a listener registered. Test: manual, libsurfaceflinger_unittest Bug: http://b/336461947 Flag: EXEMPT refactor Change-Id: I3238ce604571832523525cf098832c7352879826 --- libs/gui/ITransactionCompletedListener.cpp | 48 +---- libs/gui/SurfaceComposerClient.cpp | 224 +++++++++++++++------ libs/gui/aidl/android/gui/IJankListener.aidl | 29 +++ libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 19 ++ libs/gui/aidl/android/gui/JankData.aidl | 35 ++++ .../include/gui/ITransactionCompletedListener.h | 38 +--- libs/gui/include/gui/SurfaceComposerClient.h | 100 ++++++--- libs/gui/tests/Surface_test.cpp | 13 ++ services/surfaceflinger/Android.bp | 3 +- .../surfaceflinger/FrameTimeline/FrameTimeline.cpp | 9 + services/surfaceflinger/Jank/JankTracker.cpp | 206 +++++++++++++++++++ services/surfaceflinger/Jank/JankTracker.h | 99 +++++++++ services/surfaceflinger/Layer.cpp | 66 +----- services/surfaceflinger/Layer.h | 10 - services/surfaceflinger/SurfaceFlinger.cpp | 37 +++- services/surfaceflinger/SurfaceFlinger.h | 6 + .../surfaceflinger/TransactionCallbackInvoker.cpp | 13 +- .../surfaceflinger/TransactionCallbackInvoker.h | 7 +- services/surfaceflinger/tests/unittests/Android.bp | 1 + .../tests/unittests/FrameTimelineTest.cpp | 63 ++++++ .../tests/unittests/JankTrackerTest.cpp | 216 ++++++++++++++++++++ .../unittests/TransactionSurfaceFrameTest.cpp | 91 ++------- 22 files changed, 999 insertions(+), 334 deletions(-) create mode 100644 libs/gui/aidl/android/gui/IJankListener.aidl create mode 100644 libs/gui/aidl/android/gui/JankData.aidl create mode 100644 services/surfaceflinger/Jank/JankTracker.cpp create mode 100644 services/surfaceflinger/Jank/JankTracker.h create mode 100644 services/surfaceflinger/tests/unittests/JankTrackerTest.cpp (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index f5d19aac78..83fc827c5f 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -43,12 +43,6 @@ enum class Tag : uint32_t { } // Anonymous namespace -namespace { // Anonymous - -constexpr int32_t kSerializedCallbackTypeOnCompelteWithJankData = 2; - -} // Anonymous namespace - status_t FrameEventHistoryStats::writeToParcel(Parcel* output) const { status_t err = output->writeUint64(frameNumber); if (err != NO_ERROR) return err; @@ -119,23 +113,6 @@ status_t FrameEventHistoryStats::readFromParcel(const Parcel* input) { return err; } -JankData::JankData() - : frameVsyncId(FrameTimelineInfo::INVALID_VSYNC_ID), jankType(JankType::None) {} - -status_t JankData::writeToParcel(Parcel* output) const { - SAFE_PARCEL(output->writeInt64, frameVsyncId); - SAFE_PARCEL(output->writeInt32, jankType); - SAFE_PARCEL(output->writeInt64, frameIntervalNs); - return NO_ERROR; -} - -status_t JankData::readFromParcel(const Parcel* input) { - SAFE_PARCEL(input->readInt64, &frameVsyncId); - SAFE_PARCEL(input->readInt32, &jankType); - SAFE_PARCEL(input->readInt64, &frameIntervalNs); - return NO_ERROR; -} - status_t SurfaceStats::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeStrongBinder, surfaceControl); if (const auto* acquireFence = std::get_if>(&acquireTimeOrFence)) { @@ -160,10 +137,6 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeUint32, currentMaxAcquiredBufferCount); SAFE_PARCEL(output->writeParcelable, eventStats); - SAFE_PARCEL(output->writeInt32, static_cast(jankData.size())); - for (const auto& data : jankData) { - SAFE_PARCEL(output->writeParcelable, data); - } SAFE_PARCEL(output->writeParcelable, previousReleaseCallbackId); return NO_ERROR; } @@ -200,13 +173,6 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readUint32, ¤tMaxAcquiredBufferCount); SAFE_PARCEL(input->readParcelable, &eventStats); - int32_t jankData_size = 0; - SAFE_PARCEL_READ_SIZE(input->readInt32, &jankData_size, input->dataSize()); - for (int i = 0; i < jankData_size; i++) { - JankData data; - SAFE_PARCEL(input->readParcelable, &data); - jankData.push_back(data); - } SAFE_PARCEL(input->readParcelable, &previousReleaseCallbackId); return NO_ERROR; } @@ -371,11 +337,7 @@ ListenerCallbacks ListenerCallbacks::filter(CallbackId::Type type) const { status_t CallbackId::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeInt64, id); - if (type == Type::ON_COMPLETE && includeJankData) { - SAFE_PARCEL(output->writeInt32, kSerializedCallbackTypeOnCompelteWithJankData); - } else { - SAFE_PARCEL(output->writeInt32, static_cast(type)); - } + SAFE_PARCEL(output->writeInt32, static_cast(type)); return NO_ERROR; } @@ -383,13 +345,7 @@ status_t CallbackId::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readInt64, &id); int32_t typeAsInt; SAFE_PARCEL(input->readInt32, &typeAsInt); - if (typeAsInt == kSerializedCallbackTypeOnCompelteWithJankData) { - type = Type::ON_COMPLETE; - includeJankData = true; - } else { - type = static_cast(typeAsInt); - includeJankData = false; - } + type = static_cast(typeAsInt); return NO_ERROR; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index af91bb3ae2..e2ac1d5de5 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -86,7 +86,8 @@ int64_t generateId() { return (((int64_t)getpid()) << 32) | ++idCounter; } -void emptyCallback(nsecs_t, const sp&, const std::vector&) {} +constexpr int64_t INVALID_VSYNC = -1; + } // namespace const std::string SurfaceComposerClient::kEmpty{}; @@ -207,9 +208,168 @@ sp SurfaceComposerClient::getDefault() { return DefaultComposerClient::getComposerClient(); } +// --------------------------------------------------------------------------- + JankDataListener::~JankDataListener() { } +status_t JankDataListener::flushJankData() { + if (mLayerId == -1) { + return INVALID_OPERATION; + } + + binder::Status status = ComposerServiceAIDL::getComposerService()->flushJankData(mLayerId); + return statusTFromBinderStatus(status); +} + +std::mutex JankDataListenerFanOut::sFanoutInstanceMutex; +std::unordered_map> JankDataListenerFanOut::sFanoutInstances; + +binder::Status JankDataListenerFanOut::onJankData(const std::vector& jankData) { + // Find the highest VSync ID. + int64_t lastVsync = jankData.empty() + ? 0 + : std::max_element(jankData.begin(), jankData.end(), + [](const gui::JankData& jd1, const gui::JankData& jd2) { + return jd1.frameVsyncId < jd2.frameVsyncId; + }) + ->frameVsyncId; + + // Fan out the jank data callback. + std::vector> listenersToRemove; + for (auto listener : getActiveListeners()) { + if (!listener->onJankDataAvailable(jankData) || + (listener->mRemoveAfter >= 0 && listener->mRemoveAfter <= lastVsync)) { + listenersToRemove.push_back(listener); + } + } + + return removeListeners(listenersToRemove) + ? binder::Status::ok() + : binder::Status::fromExceptionCode(binder::Status::EX_NULL_POINTER); +} + +status_t JankDataListenerFanOut::addListener(sp sc, sp listener) { + sp layer = sc->getHandle(); + if (layer == nullptr) { + return UNEXPECTED_NULL; + } + int32_t layerId = sc->getLayerId(); + + sFanoutInstanceMutex.lock(); + auto it = sFanoutInstances.find(layerId); + bool registerNeeded = it == sFanoutInstances.end(); + sp fanout; + if (registerNeeded) { + fanout = sp::make(layerId); + sFanoutInstances.insert({layerId, fanout}); + } else { + fanout = it->second; + } + + fanout->mMutex.lock(); + fanout->mListeners.insert(listener); + fanout->mMutex.unlock(); + + sFanoutInstanceMutex.unlock(); + + if (registerNeeded) { + binder::Status status = + ComposerServiceAIDL::getComposerService()->addJankListener(layer, fanout); + return statusTFromBinderStatus(status); + } + return OK; +} + +status_t JankDataListenerFanOut::removeListener(sp listener) { + int32_t layerId = listener->mLayerId; + if (layerId == -1) { + return INVALID_OPERATION; + } + + int64_t removeAfter = INVALID_VSYNC; + sp fanout; + + sFanoutInstanceMutex.lock(); + auto it = sFanoutInstances.find(layerId); + if (it != sFanoutInstances.end()) { + fanout = it->second; + removeAfter = fanout->updateAndGetRemovalVSync(); + } + + if (removeAfter != INVALID_VSYNC) { + // Remove this instance from the map, so that no new listeners are added + // while we're scheduled to be removed. + sFanoutInstances.erase(layerId); + } + sFanoutInstanceMutex.unlock(); + + if (removeAfter < 0) { + return OK; + } + + binder::Status status = + ComposerServiceAIDL::getComposerService()->removeJankListener(layerId, fanout, + removeAfter); + return statusTFromBinderStatus(status); +} + +std::vector> JankDataListenerFanOut::getActiveListeners() { + std::scoped_lock lock(mMutex); + + std::vector> listeners; + for (auto it = mListeners.begin(); it != mListeners.end();) { + auto listener = it->promote(); + if (!listener) { + it = mListeners.erase(it); + } else { + listeners.push_back(std::move(listener)); + it++; + } + } + return listeners; +} + +bool JankDataListenerFanOut::removeListeners(const std::vector>& listeners) { + std::scoped_lock fanoutLock(sFanoutInstanceMutex); + std::scoped_lock listenersLock(mMutex); + + for (auto listener : listeners) { + mListeners.erase(listener); + } + + if (mListeners.empty()) { + sFanoutInstances.erase(mLayerId); + return false; + } + return true; +} + +int64_t JankDataListenerFanOut::updateAndGetRemovalVSync() { + std::scoped_lock lock(mMutex); + if (mRemoveAfter >= 0) { + // We've already been scheduled to be removed. Don't schedule again. + return INVALID_VSYNC; + } + + int64_t removeAfter = 0; + for (auto it = mListeners.begin(); it != mListeners.end();) { + auto listener = it->promote(); + if (!listener) { + it = mListeners.erase(it); + } else if (listener->mRemoveAfter < 0) { + // We have at least one listener that's still interested. Don't remove. + return INVALID_VSYNC; + } else { + removeAfter = std::max(removeAfter, listener->mRemoveAfter); + it++; + } + } + + mRemoveAfter = removeAfter; + return removeAfter; +} + // --------------------------------------------------------------------------- // TransactionCompletedListener does not use ANDROID_SINGLETON_STATIC_INSTANCE because it needs @@ -256,14 +416,6 @@ CallbackId TransactionCompletedListener::addCallbackFunction( surfaceControls, CallbackId::Type callbackType) { std::lock_guard lock(mMutex); - return addCallbackFunctionLocked(callbackFunction, surfaceControls, callbackType); -} - -CallbackId TransactionCompletedListener::addCallbackFunctionLocked( - const TransactionCompletedCallback& callbackFunction, - const std::unordered_set, SurfaceComposerClient::SCHash>& - surfaceControls, - CallbackId::Type callbackType) { startListeningLocked(); CallbackId callbackId(getNextIdLocked(), callbackType); @@ -272,33 +424,11 @@ CallbackId TransactionCompletedListener::addCallbackFunctionLocked( for (const auto& surfaceControl : surfaceControls) { callbackSurfaceControls[surfaceControl->getHandle()] = surfaceControl; - - if (callbackType == CallbackId::Type::ON_COMPLETE && - mJankListeners.count(surfaceControl->getLayerId()) != 0) { - callbackId.includeJankData = true; - } } return callbackId; } -void TransactionCompletedListener::addJankListener(const sp& listener, - sp surfaceControl) { - std::lock_guard lock(mMutex); - mJankListeners.insert({surfaceControl->getLayerId(), listener}); -} - -void TransactionCompletedListener::removeJankListener(const sp& listener) { - std::lock_guard lock(mMutex); - for (auto it = mJankListeners.begin(); it != mJankListeners.end();) { - if (it->second == listener) { - it = mJankListeners.erase(it); - } else { - it++; - } - } -} - void TransactionCompletedListener::setReleaseBufferCallback(const ReleaseCallbackId& callbackId, ReleaseBufferCallback listener) { std::scoped_lock lock(mMutex); @@ -325,32 +455,20 @@ void TransactionCompletedListener::removeSurfaceStatsListener(void* context, voi } void TransactionCompletedListener::addSurfaceControlToCallbacks( - SurfaceComposerClient::CallbackInfo& callbackInfo, - const sp& surfaceControl) { + const sp& surfaceControl, + const std::unordered_set& callbackIds) { std::lock_guard lock(mMutex); - bool includingJankData = false; - for (auto callbackId : callbackInfo.callbackIds) { + for (auto callbackId : callbackIds) { mCallbacks[callbackId].surfaceControls.emplace(std::piecewise_construct, std::forward_as_tuple( surfaceControl->getHandle()), std::forward_as_tuple(surfaceControl)); - includingJankData = includingJankData || callbackId.includeJankData; - } - - // If no registered callback is requesting jank data, but there is a jank listener registered - // on the new surface control, add a synthetic callback that requests the jank data. - if (!includingJankData && mJankListeners.count(surfaceControl->getLayerId()) != 0) { - CallbackId callbackId = - addCallbackFunctionLocked(&emptyCallback, callbackInfo.surfaceControls, - CallbackId::Type::ON_COMPLETE); - callbackInfo.callbackIds.emplace(callbackId); } } void TransactionCompletedListener::onTransactionCompleted(ListenerStats listenerStats) { std::unordered_map callbacksMap; - std::multimap> jankListenersMap; { std::lock_guard lock(mMutex); @@ -366,7 +484,6 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener * sp that could possibly exist for the callbacks. */ callbacksMap = mCallbacks; - jankListenersMap = mJankListeners; for (const auto& transactionStats : listenerStats.transactionStats) { for (auto& callbackId : transactionStats.callbackIds) { mCallbacks.erase(callbackId); @@ -486,12 +603,6 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener transactionStats.presentFence, surfaceStats); } } - - if (surfaceStats.jankData.empty()) continue; - auto jankRange = jankListenersMap.equal_range(layerId); - for (auto it = jankRange.first; it != jankRange.second; it++) { - it->second->onJankDataAvailable(surfaceStats.jankData); - } } } } @@ -1004,8 +1115,9 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Tr // register all surface controls for all callbackIds for this listener that is merging for (const auto& surfaceControl : currentProcessCallbackInfo.surfaceControls) { - mTransactionCompletedListener->addSurfaceControlToCallbacks(currentProcessCallbackInfo, - surfaceControl); + mTransactionCompletedListener + ->addSurfaceControlToCallbacks(surfaceControl, + currentProcessCallbackInfo.callbackIds); } } @@ -1361,7 +1473,7 @@ void SurfaceComposerClient::Transaction::registerSurfaceControlForCallback( auto& callbackInfo = mListenerCallbacks[TransactionCompletedListener::getIInstance()]; callbackInfo.surfaceControls.insert(sc); - mTransactionCompletedListener->addSurfaceControlToCallbacks(callbackInfo, sc); + mTransactionCompletedListener->addSurfaceControlToCallbacks(sc, callbackInfo.callbackIds); } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setPosition( diff --git a/libs/gui/aidl/android/gui/IJankListener.aidl b/libs/gui/aidl/android/gui/IJankListener.aidl new file mode 100644 index 0000000000..2bfd1af69d --- /dev/null +++ b/libs/gui/aidl/android/gui/IJankListener.aidl @@ -0,0 +1,29 @@ +/* + * Copyright 2024 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. + */ + +package android.gui; + +import android.gui.JankData; + +/** @hide */ +interface IJankListener { + + /** + * Callback reporting jank data of the most recent frames. + * @See {@link ISurfaceComposer#addJankListener(IBinder, IJankListener)} + */ + void onJankData(in JankData[] data); +} diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index 6d018ea7ef..ac14138e8c 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -42,6 +42,7 @@ import android.gui.ISurfaceComposerClient; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; import android.gui.IWindowInfosPublisher; +import android.gui.IJankListener; import android.gui.LayerCaptureArgs; import android.gui.OverlayProperties; import android.gui.PullAtomData; @@ -580,4 +581,22 @@ interface ISurfaceComposer { * This method should not block the ShutdownThread therefore it's handled asynchronously. */ oneway void notifyShutdown(); + + /** + * Registers the jank listener on the given layer to receive jank data of future frames. + */ + void addJankListener(IBinder layer, IJankListener listener); + + /** + * Flushes any pending jank data on the given layer to any registered listeners on that layer. + */ + oneway void flushJankData(int layerId); + + /** + * Schedules the removal of the jank listener from the given layer after the VSync with the + * specified ID. Use a value <= 0 for afterVsync to remove the listener immediately. The given + * listener will not be removed before the given VSync, but may still receive data for frames + * past the provided VSync. + */ + oneway void removeJankListener(int layerId, IJankListener listener, long afterVsync); } diff --git a/libs/gui/aidl/android/gui/JankData.aidl b/libs/gui/aidl/android/gui/JankData.aidl new file mode 100644 index 0000000000..7ea9d22ecf --- /dev/null +++ b/libs/gui/aidl/android/gui/JankData.aidl @@ -0,0 +1,35 @@ +/* + * Copyright 2024 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. + */ + + package android.gui; + + /** @hide */ +parcelable JankData { + /** + * Identifier for the frame submitted with Transaction.setFrameTimelineVsyncId + */ + long frameVsyncId; + + /** + * Bitmask of jank types that occurred. + */ + int jankType; + + /** + * Expected duration in nanoseconds of this frame. + */ + long frameIntervalNs; +} diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index bc97cd0828..014029b257 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -16,8 +16,6 @@ #pragma once -#include "JankInfo.h" - #include #include #include @@ -40,15 +38,10 @@ class ListenerCallbacks; class CallbackId : public Parcelable { public: int64_t id; - enum class Type : int32_t { - ON_COMPLETE = 0, - ON_COMMIT = 1, - /*reserved for serialization = 2*/ - } type; - bool includeJankData; // Only respected for ON_COMPLETE callbacks. + enum class Type : int32_t { ON_COMPLETE = 0, ON_COMMIT = 1 } type; CallbackId() {} - CallbackId(int64_t id, Type type) : id(id), type(type), includeJankData(false) {} + CallbackId(int64_t id, Type type) : id(id), type(type) {} status_t writeToParcel(Parcel* output) const override; status_t readFromParcel(const Parcel* input) override; @@ -113,29 +106,6 @@ public: nsecs_t dequeueReadyTime; }; -/** - * Jank information representing SurfaceFlinger's jank classification about frames for a specific - * surface. - */ -class JankData : public Parcelable { -public: - status_t writeToParcel(Parcel* output) const override; - status_t readFromParcel(const Parcel* input) override; - - JankData(); - JankData(int64_t frameVsyncId, int32_t jankType, nsecs_t frameIntervalNs) - : frameVsyncId(frameVsyncId), jankType(jankType), frameIntervalNs(frameIntervalNs) {} - - // Identifier for the frame submitted with Transaction.setFrameTimelineVsyncId - int64_t frameVsyncId; - - // Bitmask of janks that occurred - int32_t jankType; - - // Expected duration of the frame - nsecs_t frameIntervalNs; -}; - class SurfaceStats : public Parcelable { public: status_t writeToParcel(Parcel* output) const override; @@ -145,14 +115,13 @@ public: SurfaceStats(const sp& sc, std::variant> acquireTimeOrFence, const sp& prevReleaseFence, std::optional hint, uint32_t currentMaxAcquiredBuffersCount, FrameEventHistoryStats frameEventStats, - std::vector jankData, ReleaseCallbackId previousReleaseCallbackId) + ReleaseCallbackId previousReleaseCallbackId) : surfaceControl(sc), acquireTimeOrFence(std::move(acquireTimeOrFence)), previousReleaseFence(prevReleaseFence), transformHint(hint), currentMaxAcquiredBufferCount(currentMaxAcquiredBuffersCount), eventStats(frameEventStats), - jankData(std::move(jankData)), previousReleaseCallbackId(previousReleaseCallbackId) {} sp surfaceControl; @@ -161,7 +130,6 @@ public: std::optional transformHint = 0; uint32_t currentMaxAcquiredBufferCount = 0; FrameEventHistoryStats eventStats; - std::vector jankData; ReleaseCallbackId previousReleaseCallbackId; }; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 0862e03c44..88c0c53e12 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -864,12 +865,82 @@ public: // --------------------------------------------------------------------------- -class JankDataListener : public VirtualLightRefBase { +class JankDataListener; + +// Acts as a representative listener to the composer for a single layer and +// forwards any received jank data to multiple listeners. Will remove itself +// from the composer only once the last listener is removed. +class JankDataListenerFanOut : public gui::BnJankListener { public: - virtual ~JankDataListener() = 0; - virtual void onJankDataAvailable(const std::vector& jankData) = 0; + JankDataListenerFanOut(int32_t layerId) : mLayerId(layerId) {} + + binder::Status onJankData(const std::vector& jankData) override; + + static status_t addListener(sp sc, sp listener); + static status_t removeListener(sp listener); + +private: + std::vector> getActiveListeners(); + bool removeListeners(const std::vector>& listeners); + int64_t updateAndGetRemovalVSync(); + + struct WpJDLHash { + std::size_t operator()(const wp& listener) const { + return std::hash{}(listener.unsafe_get()); + } + }; + + std::mutex mMutex; + std::unordered_set, WpJDLHash> mListeners GUARDED_BY(mMutex); + int32_t mLayerId; + int64_t mRemoveAfter = -1; + + static std::mutex sFanoutInstanceMutex; + static std::unordered_map> sFanoutInstances; }; +// Base class for client listeners interested in jank classification data from +// the composer. Subclasses should override onJankDataAvailable and call the add +// and removal methods to receive jank data. +class JankDataListener : public virtual RefBase { +public: + JankDataListener() {} + virtual ~JankDataListener(); + + virtual bool onJankDataAvailable(const std::vector& jankData) = 0; + + status_t addListener(sp sc) { + if (mLayerId != -1) { + removeListener(0); + mLayerId = -1; + } + + int32_t layerId = sc->getLayerId(); + status_t status = + JankDataListenerFanOut::addListener(std::move(sc), + sp::fromExisting(this)); + if (status == OK) { + mLayerId = layerId; + } + return status; + } + + status_t removeListener(int64_t afterVsync) { + mRemoveAfter = std::max(static_cast(0), afterVsync); + return JankDataListenerFanOut::removeListener(sp::fromExisting(this)); + } + + status_t flushJankData(); + + friend class JankDataListenerFanOut; + +private: + int32_t mLayerId = -1; + int64_t mRemoveAfter = -1; +}; + +// --------------------------------------------------------------------------- + class TransactionCompletedListener : public BnTransactionCompletedListener { public: TransactionCompletedListener(); @@ -904,7 +975,6 @@ protected: std::unordered_map mCallbacks GUARDED_BY(mMutex); - std::multimap> mJankListeners GUARDED_BY(mMutex); std::unordered_map mReleaseBufferCallbacks GUARDED_BY(mMutex); @@ -927,14 +997,10 @@ public: const std::unordered_set, SurfaceComposerClient::SCHash>& surfaceControls, CallbackId::Type callbackType); - CallbackId addCallbackFunctionLocked( - const TransactionCompletedCallback& callbackFunction, - const std::unordered_set, SurfaceComposerClient::SCHash>& - surfaceControls, - CallbackId::Type callbackType) REQUIRES(mMutex); - void addSurfaceControlToCallbacks(SurfaceComposerClient::CallbackInfo& callbackInfo, - const sp& surfaceControl); + void addSurfaceControlToCallbacks( + const sp& surfaceControl, + const std::unordered_set& callbackIds); void addQueueStallListener(std::function stallListener, void* id); void removeQueueStallListener(void *id); @@ -943,18 +1009,6 @@ public: TrustedPresentationCallback tpc, int id, void* context); void clearTrustedPresentationCallback(int id); - /* - * Adds a jank listener to be informed about SurfaceFlinger's jank classification for a specific - * surface. Jank classifications arrive as part of the transaction callbacks about previous - * frames submitted to this Surface. - */ - void addJankListener(const sp& listener, sp surfaceControl); - - /** - * Removes a jank listener previously added to addJankCallback. - */ - void removeJankListener(const sp& listener); - void addSurfaceStatsListener(void* context, void* cookie, sp surfaceControl, SurfaceStatsCallback listener); void removeSurfaceStatsListener(void* context, void* cookie); diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 43cd0f8a7f..ace442339d 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -987,6 +987,19 @@ public: binder::Status notifyShutdown() override { return binder::Status::ok(); } + binder::Status addJankListener(const sp& /*layer*/, + const sp& /*listener*/) override { + return binder::Status::ok(); + } + + binder::Status flushJankData(int32_t /*layerId*/) override { return binder::Status::ok(); } + + binder::Status removeJankListener(int32_t /*layerId*/, + const sp& /*listener*/, + int64_t /*afterVsync*/) override { + return binder::Status::ok(); + } + protected: IBinder* onAsBinder() override { return nullptr; } diff --git a/services/surfaceflinger/Android.bp b/services/surfaceflinger/Android.bp index 1b6c598372..a37433cead 100644 --- a/services/surfaceflinger/Android.bp +++ b/services/surfaceflinger/Android.bp @@ -85,7 +85,7 @@ cc_defaults { "libui", "libutils", "libSurfaceFlingerProp", - "libaconfig_storage_read_api_cc" + "libaconfig_storage_read_api_cc", ], static_libs: [ "iinputflinger_aidl_lib_static", @@ -187,6 +187,7 @@ filegroup { "FrameTracker.cpp", "HdrLayerInfoReporter.cpp", "HdrSdrRatioOverlay.cpp", + "Jank/JankTracker.cpp", "WindowInfosListenerInvoker.cpp", "Layer.cpp", "LayerFE.cpp", diff --git a/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp b/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp index 2596a25d15..f671006149 100644 --- a/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp +++ b/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp @@ -30,6 +30,8 @@ #include #include +#include "../Jank/JankTracker.h" + namespace android::frametimeline { using base::StringAppendF; @@ -685,6 +687,13 @@ void SurfaceFrame::onPresent(nsecs_t presentTime, int32_t displayFrameJankType, mTimeStats->incrementJankyFrames({refreshRate, mRenderRate, mOwnerUid, mLayerName, mGameMode, mJankType, displayDeadlineDelta, displayPresentDelta, deadlineDelta}); + + gui::JankData jd; + jd.frameVsyncId = mToken; + jd.jankType = mJankType; + jd.frameIntervalNs = + (mRenderRate ? *mRenderRate : mDisplayFrameRenderRate).getPeriodNsecs(); + JankTracker::onJankData(mLayerId, jd); } } diff --git a/services/surfaceflinger/Jank/JankTracker.cpp b/services/surfaceflinger/Jank/JankTracker.cpp new file mode 100644 index 0000000000..8e0e084cd2 --- /dev/null +++ b/services/surfaceflinger/Jank/JankTracker.cpp @@ -0,0 +1,206 @@ +/* + * Copyright 2024 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. + */ + +#include "JankTracker.h" + +#include +#include "BackgroundExecutor.h" + +namespace android { + +namespace { + +constexpr size_t kJankDataBatchSize = 50; + +} // anonymous namespace + +std::atomic JankTracker::sListenerCount(0); +std::atomic JankTracker::sCollectAllJankDataForTesting(false); + +JankTracker::~JankTracker() {} + +void JankTracker::addJankListener(int32_t layerId, sp listener) { + // Increment right away, so that if an onJankData call comes in before the background thread has + // added this listener, it will not drop the data. + sListenerCount++; + + BackgroundExecutor::getLowPriorityInstance().sendCallbacks( + {[layerId, listener = std::move(listener)]() { + JankTracker& tracker = getInstance(); + const std::lock_guard _l(tracker.mLock); + tracker.addJankListenerLocked(layerId, listener); + }}); +} + +void JankTracker::flushJankData(int32_t layerId) { + BackgroundExecutor::getLowPriorityInstance().sendCallbacks( + {[layerId]() { getInstance().doFlushJankData(layerId); }}); +} + +void JankTracker::removeJankListener(int32_t layerId, sp listener, int64_t afterVsync) { + BackgroundExecutor::getLowPriorityInstance().sendCallbacks( + {[layerId, listener = std::move(listener), afterVsync]() { + JankTracker& tracker = getInstance(); + const std::lock_guard _l(tracker.mLock); + tracker.markJankListenerForRemovalLocked(layerId, listener, afterVsync); + }}); +} + +void JankTracker::onJankData(int32_t layerId, gui::JankData data) { + if (sListenerCount == 0) { + return; + } + + BackgroundExecutor::getLowPriorityInstance().sendCallbacks( + {[layerId, data = std::move(data)]() { + JankTracker& tracker = getInstance(); + + tracker.mLock.lock(); + bool hasListeners = tracker.mJankListeners.count(layerId) > 0; + tracker.mLock.unlock(); + + if (!hasListeners && !sCollectAllJankDataForTesting) { + return; + } + + tracker.mJankDataLock.lock(); + tracker.mJankData.emplace(layerId, data); + size_t count = tracker.mJankData.count(layerId); + tracker.mJankDataLock.unlock(); + + if (count >= kJankDataBatchSize && !sCollectAllJankDataForTesting) { + tracker.doFlushJankData(layerId); + } + }}); +} + +void JankTracker::addJankListenerLocked(int32_t layerId, sp listener) { + for (auto it = mJankListeners.find(layerId); it != mJankListeners.end(); it++) { + if (it->second.mListener == listener) { + // Undo the duplicate increment in addJankListener. + sListenerCount--; + return; + } + } + + mJankListeners.emplace(layerId, std::move(listener)); +} + +void JankTracker::doFlushJankData(int32_t layerId) { + std::vector jankData; + int64_t maxVsync = transferAvailableJankData(layerId, jankData); + + std::vector> toSend; + + mLock.lock(); + for (auto it = mJankListeners.find(layerId); it != mJankListeners.end();) { + if (!jankData.empty()) { + toSend.emplace_back(it->second.mListener); + } + + int64_t removeAfter = it->second.mRemoveAfter; + if (removeAfter != -1 && removeAfter <= maxVsync) { + it = mJankListeners.erase(it); + sListenerCount--; + } else { + it++; + } + } + mLock.unlock(); + + for (const auto& listener : toSend) { + binder::Status status = interface_cast(listener)->onJankData(jankData); + if (status.exceptionCode() == binder::Status::EX_NULL_POINTER) { + // Remove any listeners, where the App side has gone away, without + // deregistering. + dropJankListener(layerId, listener); + } + } +} + +void JankTracker::markJankListenerForRemovalLocked(int32_t layerId, sp listener, + int64_t afterVysnc) { + for (auto it = mJankListeners.find(layerId); it != mJankListeners.end(); it++) { + if (it->second.mListener == listener) { + it->second.mRemoveAfter = std::max(static_cast(0), afterVysnc); + return; + } + } +} + +int64_t JankTracker::transferAvailableJankData(int32_t layerId, + std::vector& outJankData) { + const std::lock_guard _l(mJankDataLock); + int64_t maxVsync = 0; + auto range = mJankData.equal_range(layerId); + for (auto it = range.first; it != range.second;) { + maxVsync = std::max(it->second.frameVsyncId, maxVsync); + outJankData.emplace_back(std::move(it->second)); + it = mJankData.erase(it); + } + return maxVsync; +} + +void JankTracker::dropJankListener(int32_t layerId, sp listener) { + const std::lock_guard _l(mLock); + for (auto it = mJankListeners.find(layerId); it != mJankListeners.end(); it++) { + if (it->second.mListener == listener) { + mJankListeners.erase(it); + sListenerCount--; + return; + } + } +} + +void JankTracker::clearAndStartCollectingAllJankDataForTesting() { + BackgroundExecutor::getLowPriorityInstance().flushQueue(); + + // Clear all past tracked jank data. + JankTracker& tracker = getInstance(); + const std::lock_guard _l(tracker.mJankDataLock); + tracker.mJankData.clear(); + + // Pretend there's at least one listener. + sListenerCount++; + sCollectAllJankDataForTesting = true; +} + +std::vector JankTracker::getCollectedJankDataForTesting(int32_t layerId) { + JankTracker& tracker = getInstance(); + const std::lock_guard _l(tracker.mJankDataLock); + + auto range = tracker.mJankData.equal_range(layerId); + std::vector result; + std::transform(range.first, range.second, std::back_inserter(result), + [](std::pair layerIdToJankData) { + return layerIdToJankData.second; + }); + + return result; +} + +void JankTracker::clearAndStopCollectingAllJankDataForTesting() { + // Undo startCollectingAllJankDataForTesting. + sListenerCount--; + sCollectAllJankDataForTesting = false; + + // Clear all tracked jank data. + JankTracker& tracker = getInstance(); + const std::lock_guard _l(tracker.mJankDataLock); + tracker.mJankData.clear(); +} + +} // namespace android diff --git a/services/surfaceflinger/Jank/JankTracker.h b/services/surfaceflinger/Jank/JankTracker.h new file mode 100644 index 0000000000..5917358797 --- /dev/null +++ b/services/surfaceflinger/Jank/JankTracker.h @@ -0,0 +1,99 @@ +/* + * Copyright 2024 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. + */ + +#pragma once + +#include +#include +#include + +#include +#include +#include + +namespace android { +namespace frametimeline { +class FrameTimelineTest; +} + +/** + * JankTracker maintains a backlog of frame jank classification and manages and notififies any + * registered jank data listeners. + */ +class JankTracker { +public: + ~JankTracker(); + + static void addJankListener(int32_t layerId, sp listener); + static void flushJankData(int32_t layerId); + static void removeJankListener(int32_t layerId, sp listener, int64_t afterVysnc); + + static void onJankData(int32_t layerId, gui::JankData data); + +protected: + // The following methods can be used to force the tracker to collect all jank data and not + // flush it for a short time period and should *only* be used for testing. Every call to + // clearAndStartCollectingAllJankDataForTesting needs to be followed by a call to + // clearAndStopCollectingAllJankDataForTesting. + static void clearAndStartCollectingAllJankDataForTesting(); + static std::vector getCollectedJankDataForTesting(int32_t layerId); + static void clearAndStopCollectingAllJankDataForTesting(); + + friend class frametimeline::FrameTimelineTest; + +private: + JankTracker() {} + JankTracker(const JankTracker&) = delete; + JankTracker(JankTracker&&) = delete; + + JankTracker& operator=(const JankTracker&) = delete; + JankTracker& operator=(JankTracker&&) = delete; + + static JankTracker& getInstance() { + static JankTracker instance; + return instance; + } + + void addJankListenerLocked(int32_t layerId, sp listener) REQUIRES(mLock); + void doFlushJankData(int32_t layerId); + void markJankListenerForRemovalLocked(int32_t layerId, sp listener, int64_t afterVysnc) + REQUIRES(mLock); + + int64_t transferAvailableJankData(int32_t layerId, std::vector& jankData); + void dropJankListener(int32_t layerId, sp listener); + + struct Listener { + sp mListener; + int64_t mRemoveAfter; + + Listener(sp&& listener) : mListener(listener), mRemoveAfter(-1) {} + }; + + // We keep track of the current listener count, so that the onJankData call, which is on the + // main thread, can short-curcuit the scheduling on the background thread (which involves + // locking) if there are no listeners registered, which is the most common case. + static std::atomic sListenerCount; + static std::atomic sCollectAllJankDataForTesting; + + std::mutex mLock; + std::unordered_multimap mJankListeners GUARDED_BY(mLock); + std::mutex mJankDataLock; + std::unordered_multimap mJankData GUARDED_BY(mJankDataLock); + + friend class JankTrackerTest; +}; + +} // namespace android diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index d27bfd29ef..e758741afb 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -767,41 +767,6 @@ bool Layer::isSecure() const { return (p != nullptr) ? p->isSecure() : false; } -void Layer::transferAvailableJankData(const std::deque>& handles, - std::vector& jankData) { - if (mPendingJankClassifications.empty() || - !mPendingJankClassifications.front()->getJankType()) { - return; - } - - bool includeJankData = false; - for (const auto& handle : handles) { - for (const auto& cb : handle->callbackIds) { - if (cb.includeJankData) { - includeJankData = true; - break; - } - } - - if (includeJankData) { - jankData.reserve(mPendingJankClassifications.size()); - break; - } - } - - while (!mPendingJankClassifications.empty() && - mPendingJankClassifications.front()->getJankType()) { - if (includeJankData) { - std::shared_ptr surfaceFrame = - mPendingJankClassifications.front(); - jankData.emplace_back(JankData(surfaceFrame->getToken(), - surfaceFrame->getJankType().value(), - surfaceFrame->getRenderRate().getPeriodNsecs())); - } - mPendingJankClassifications.pop_front(); - } -} - // ---------------------------------------------------------------------------- // transaction // ---------------------------------------------------------------------------- @@ -1436,7 +1401,6 @@ std::shared_ptr Layer::createSurfaceFrameForTransac if (fps) { surfaceFrame->setRenderRate(*fps); } - onSurfaceFrameCreated(surfaceFrame); return surfaceFrame; } @@ -1453,7 +1417,6 @@ std::shared_ptr Layer::createSurfaceFrameForBuffer( if (fps) { surfaceFrame->setRenderRate(*fps); } - onSurfaceFrameCreated(surfaceFrame); return surfaceFrame; } @@ -1479,7 +1442,6 @@ void Layer::setFrameTimelineVsyncForSkippedFrames(const FrameTimelineInfo& info, if (fps) { surfaceFrame->setRenderRate(*fps); } - onSurfaceFrameCreated(surfaceFrame); addSurfaceFrameDroppedForBuffer(surfaceFrame, postTime); } @@ -2942,25 +2904,6 @@ void Layer::onLayerDisplayed(ftl::SharedFuture futureFenceResult, } } -void Layer::onSurfaceFrameCreated( - const std::shared_ptr& surfaceFrame) { - while (mPendingJankClassifications.size() >= kPendingClassificationMaxSurfaceFrames) { - // Too many SurfaceFrames pending classification. The front of the deque is probably not - // tracked by FrameTimeline and will never be presented. This will only result in a memory - // leak. - if (hasBufferOrSidebandStreamInDrawing()) { - // Only log for layers with a buffer, since we expect the jank data to be drained for - // these, while there may be no jank listeners for bufferless layers. - ALOGW("Removing the front of pending jank deque from layer - %s to prevent memory leak", - mName.c_str()); - std::string miniDump = mPendingJankClassifications.front()->miniDump(); - ALOGD("Head SurfaceFrame mini dump\n%s", miniDump.c_str()); - } - mPendingJankClassifications.pop_front(); - } - mPendingJankClassifications.emplace_back(surfaceFrame); -} - void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { for (const auto& handle : mDrawingState.callbackHandles) { handle->transformHint = mTransformHint; @@ -2978,10 +2921,7 @@ void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { } } - std::vector jankData; - transferAvailableJankData(mDrawingState.callbackHandles, jankData); - mFlinger->getTransactionCallbackInvoker().addCallbackHandles(mDrawingState.callbackHandles, - jankData); + mFlinger->getTransactionCallbackInvoker().addCallbackHandles(mDrawingState.callbackHandles); mDrawingState.callbackHandles = {}; } @@ -3449,9 +3389,7 @@ bool Layer::setTransactionCompletedListeners(const std::vector jankData; - transferAvailableJankData(remainingHandles, jankData); - mFlinger->getTransactionCallbackInvoker().addCallbackHandles(remainingHandles, jankData); + mFlinger->getTransactionCallbackInvoker().addCallbackHandles(remainingHandles); } mReleasePreviousBuffer = false; diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index b9fcd5c333..d3b56f89d1 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -980,7 +980,6 @@ protected: void preparePerFrameBufferCompositionState(); void preparePerFrameEffectsCompositionState(); void gatherBufferInfo(); - void onSurfaceFrameCreated(const std::shared_ptr&); bool isClonedFromAlive() { return getClonedFrom() != nullptr; } @@ -1195,11 +1194,6 @@ private: bool hasSomethingToDraw() const { return hasEffect() || hasBufferOrSidebandStream(); } - // Fills the provided vector with the currently available JankData and removes the processed - // JankData from the pending list. - void transferAvailableJankData(const std::deque>& handles, - std::vector& jankData); - bool shouldOverrideChildrenFrameRate() const { return getDrawingState().frameRateSelectionStrategy == FrameRateSelectionStrategy::OverrideChildren; @@ -1268,10 +1262,6 @@ private: // time. std::variant> mCallbackHandleAcquireTimeOrFence = -1; - std::deque> mPendingJankClassifications; - // An upper bound on the number of SurfaceFrames in the pending classifications deque. - static constexpr int kPendingClassificationMaxSurfaceFrames = 50; - const std::string mBlastTransactionName{"BufferTX - " + mName}; // This integer is incremented everytime a buffer arrives at the server for this layer, // and decremented when a buffer is dropped or latched. When changed the integer is exported diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 5b40acf030..989020daeb 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -143,6 +143,7 @@ #include "FrontEnd/LayerLog.h" #include "FrontEnd/LayerSnapshot.h" #include "HdrLayerInfoReporter.h" +#include "Jank/JankTracker.h" #include "Layer.h" #include "LayerProtoHelper.h" #include "LayerRenderArea.h" @@ -5507,10 +5508,8 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime } if (layer == nullptr) { for (auto& [listener, callbackIds] : s.listeners) { - mTransactionCallbackInvoker.addCallbackHandle(sp::make(listener, - callbackIds, - s.surface), - std::vector()); + mTransactionCallbackInvoker.addCallbackHandle( + sp::make(listener, callbackIds, s.surface)); } return 0; } @@ -5868,10 +5867,8 @@ uint32_t SurfaceFlinger::updateLayerCallbacksAndStats(const FrameTimelineInfo& f } if (layer == nullptr) { for (auto& [listener, callbackIds] : s.listeners) { - mTransactionCallbackInvoker.addCallbackHandle(sp::make(listener, - callbackIds, - s.surface), - std::vector()); + mTransactionCallbackInvoker.addCallbackHandle( + sp::make(listener, callbackIds, s.surface)); } return 0; } @@ -6114,6 +6111,8 @@ void SurfaceFlinger::onHandleDestroyed(BBinder* handle, sp& layer, uint32 mTransactionHandler.onLayerDestroyed(layerId); } + JankTracker::flushJankData(layerId); + Mutex::Autolock lock(mStateLock); markLayerPendingRemovalLocked(layer); layer->onHandleDestroyed(); @@ -10463,6 +10462,28 @@ binder::Status SurfaceComposerAIDL::notifyShutdown() { return ::android::binder::Status::ok(); } +binder::Status SurfaceComposerAIDL::addJankListener(const sp& layerHandle, + const sp& listener) { + sp layer = LayerHandle::getLayer(layerHandle); + if (layer == nullptr) { + return binder::Status::fromExceptionCode(binder::Status::EX_NULL_POINTER); + } + JankTracker::addJankListener(layer->sequence, IInterface::asBinder(listener)); + return binder::Status::ok(); +} + +binder::Status SurfaceComposerAIDL::flushJankData(int32_t layerId) { + JankTracker::flushJankData(layerId); + return binder::Status::ok(); +} + +binder::Status SurfaceComposerAIDL::removeJankListener(int32_t layerId, + const sp& listener, + int64_t afterVsync) { + JankTracker::removeJankListener(layerId, IInterface::asBinder(listener), afterVsync); + return binder::Status::ok(); +} + status_t SurfaceComposerAIDL::checkAccessPermission(bool usePermissionCache) { if (!mFlinger->callingThreadHasUnscopedSurfaceFlingerAccess(usePermissionCache)) { IPCThreadState* ipc = IPCThreadState::self(); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ee541c4364..6838994867 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1693,6 +1694,11 @@ public: int pid, std::optional* outInfo) override; binder::Status getSchedulingPolicy(gui::SchedulingPolicy* outPolicy) override; binder::Status notifyShutdown() override; + binder::Status addJankListener(const sp& layer, + const sp& listener) override; + binder::Status flushJankData(int32_t layerId) override; + binder::Status removeJankListener(int32_t layerId, const sp& listener, + int64_t afterVsync) override; private: static const constexpr bool kUsePermissionCache = true; diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp index 222ae30acb..b9555c04a8 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.cpp +++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp @@ -64,13 +64,12 @@ status_t TransactionCallbackInvoker::addOnCommitCallbackHandles( if (handles.empty()) { return NO_ERROR; } - const std::vector& jankData = std::vector(); for (const auto& handle : handles) { if (!containsOnCommitCallbacks(handle->callbackIds)) { outRemainingHandles.push_back(handle); continue; } - status_t err = addCallbackHandle(handle, jankData); + status_t err = addCallbackHandle(handle); if (err != NO_ERROR) { return err; } @@ -80,12 +79,12 @@ status_t TransactionCallbackInvoker::addOnCommitCallbackHandles( } status_t TransactionCallbackInvoker::addCallbackHandles( - const std::deque>& handles, const std::vector& jankData) { + const std::deque>& handles) { if (handles.empty()) { return NO_ERROR; } for (const auto& handle : handles) { - status_t err = addCallbackHandle(handle, jankData); + status_t err = addCallbackHandle(handle); if (err != NO_ERROR) { return err; } @@ -111,8 +110,7 @@ status_t TransactionCallbackInvoker::findOrCreateTransactionStats( return NO_ERROR; } -status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle, - const std::vector& jankData) { +status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle) { // If we can't find the transaction stats something has gone wrong. The client should call // startRegistration before trying to add a callback handle. TransactionStats* transactionStats; @@ -151,8 +149,7 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle->previousReleaseFence, handle->transformHint, handle->currentMaxAcquiredBufferCount, - eventStats, jankData, - handle->previousReleaseCallbackId); + eventStats, handle->previousReleaseCallbackId); } return NO_ERROR; } diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index cb7150b943..7853a9f359 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -63,8 +63,7 @@ public: class TransactionCallbackInvoker { public: - status_t addCallbackHandles(const std::deque>& handles, - const std::vector& jankData); + status_t addCallbackHandles(const std::deque>& handles); status_t addOnCommitCallbackHandles(const std::deque>& handles, std::deque>& outRemainingHandles); @@ -77,9 +76,7 @@ public: mCompletedTransactions.clear(); } - status_t addCallbackHandle(const sp& handle, - const std::vector& jankData); - + status_t addCallbackHandle(const sp& handle); private: status_t findOrCreateTransactionStats(const sp& listener, diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp index 98d5754271..bc35639a00 100644 --- a/services/surfaceflinger/tests/unittests/Android.bp +++ b/services/surfaceflinger/tests/unittests/Android.bp @@ -84,6 +84,7 @@ cc_test { "FrameTimelineTest.cpp", "GameModeTest.cpp", "HWComposerTest.cpp", + "JankTrackerTest.cpp", "OneShotTimerTest.cpp", "LayerHistoryTest.cpp", "LayerHistoryIntegrationTest.cpp", diff --git a/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp b/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp index dac9265b71..25437ca856 100644 --- a/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp +++ b/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp @@ -15,6 +15,8 @@ */ #include +#include "BackgroundExecutor.h" +#include "Jank/JankTracker.h" #include "com_android_graphics_surfaceflinger_flags.h" #include "gmock/gmock-spec-builders.h" #include "mock/MockTimeStats.h" @@ -90,8 +92,12 @@ public: mTraceCookieCounter = &mFrameTimeline->mTraceCookieCounter; maxDisplayFrames = &mFrameTimeline->mMaxDisplayFrames; maxTokens = mTokenManager->kMaxTokens; + + JankTracker::clearAndStartCollectingAllJankDataForTesting(); } + void TearDown() override { JankTracker::clearAndStopCollectingAllJankDataForTesting(); } + // Each tracing session can be used for a single block of Start -> Stop. static std::unique_ptr getTracingSessionForTest() { perfetto::TraceConfig cfg; @@ -175,6 +181,16 @@ public: [&](FrameTimelineDataSource::TraceContext ctx) { ctx.Flush(); }); } + std::vector getLayerOneJankData() { + BackgroundExecutor::getLowPriorityInstance().flushQueue(); + return JankTracker::getCollectedJankDataForTesting(sLayerIdOne); + } + + std::vector getLayerTwoJankData() { + BackgroundExecutor::getLowPriorityInstance().flushQueue(); + return JankTracker::getCollectedJankDataForTesting(sLayerIdTwo); + } + std::shared_ptr mTimeStats; std::unique_ptr mFrameTimeline; impl::TokenManager* mTokenManager; @@ -339,6 +355,9 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_presentedFramesUpdated) { EXPECT_NE(surfaceFrame1->getJankSeverityType(), std::nullopt); EXPECT_NE(surfaceFrame2->getJankType(), std::nullopt); EXPECT_NE(surfaceFrame2->getJankSeverityType(), std::nullopt); + + EXPECT_EQ(getLayerOneJankData().size(), 1u); + EXPECT_EQ(getLayerTwoJankData().size(), 1u); } TEST_F(FrameTimelineTest, displayFrameSkippedComposition) { @@ -446,6 +465,8 @@ TEST_F(FrameTimelineTest, displayFramesSlidingWindowMovesAfterLimit) { // The window should have slided by 1 now and the previous 0th display frame // should have been removed from the deque EXPECT_EQ(compareTimelineItems(displayFrame0->getActuals(), TimelineItem(52, 57, 62)), true); + + EXPECT_EQ(getLayerOneJankData().size(), *maxDisplayFrames); } TEST_F(FrameTimelineTest, surfaceFrameEndTimeAcquireFenceAfterQueue) { @@ -575,6 +596,8 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_doesNotReportForInvalidTokens) { presentFence1->signalForTest(70); mFrameTimeline->setSfPresent(59, presentFence1); + + EXPECT_EQ(getLayerOneJankData().size(), 0u); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsLongSfCpu) { @@ -603,6 +626,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsLongSfCpu) { presentFence1->signalForTest(70); mFrameTimeline->setSfPresent(62, presentFence1); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::SurfaceFlingerCpuDeadlineMissed); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsLongSfGpu) { @@ -633,6 +660,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsLongSfGpu) { presentFence1->signalForTest(70); mFrameTimeline->setSfPresent(59, presentFence1, gpuFence1); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::SurfaceFlingerGpuDeadlineMissed); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsDisplayMiss) { @@ -661,6 +692,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsDisplayMiss) { mFrameTimeline->setSfPresent(56, presentFence1); EXPECT_EQ(surfaceFrame1->getJankType(), JankType::DisplayHAL); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Full); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::DisplayHAL); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppMiss) { @@ -691,6 +726,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppMiss) { EXPECT_EQ(surfaceFrame1->getJankType(), JankType::AppDeadlineMissed); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Partial); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::AppDeadlineMissed); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsSfScheduling) { @@ -721,6 +760,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsSfScheduling) { EXPECT_EQ(surfaceFrame1->getJankType(), JankType::SurfaceFlingerScheduling); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Full); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::SurfaceFlingerScheduling); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsSfPredictionError) { @@ -751,6 +794,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsSfPredictionError) { EXPECT_EQ(surfaceFrame1->getJankType(), JankType::PredictionError); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Partial); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::PredictionError); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppBufferStuffing) { @@ -782,6 +829,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppBufferStuffing) { EXPECT_EQ(surfaceFrame1->getJankType(), JankType::BufferStuffing); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Full); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::BufferStuffing); } TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppMissWithRenderRate) { @@ -814,6 +865,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_reportsAppMissWithRenderRate) { EXPECT_EQ(surfaceFrame1->getJankType(), JankType::AppDeadlineMissed); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Full); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::AppDeadlineMissed); } TEST_F(FrameTimelineTest, presentFenceSignaled_displayFramePredictionExpiredPresentsSurfaceFrame) { @@ -858,6 +913,10 @@ TEST_F(FrameTimelineTest, presentFenceSignaled_displayFramePredictionExpiredPres EXPECT_EQ(surfaceFrame1->getActuals().presentTime, 90); EXPECT_EQ(surfaceFrame1->getJankType(), JankType::Unknown | JankType::AppDeadlineMissed); EXPECT_EQ(surfaceFrame1->getJankSeverityType(), JankSeverityType::Full); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::Unknown | JankType::AppDeadlineMissed); } /* @@ -1789,6 +1848,10 @@ TEST_F(FrameTimelineTest, jankClassification_presentOnTimeDoesNotClassify) { EXPECT_EQ(displayFrame->getFrameReadyMetadata(), FrameReadyMetadata::OnTimeFinish); EXPECT_EQ(displayFrame->getJankType(), JankType::None); EXPECT_EQ(displayFrame->getJankSeverityType(), JankSeverityType::None); + + auto jankData = getLayerOneJankData(); + EXPECT_EQ(jankData.size(), 1u); + EXPECT_EQ(jankData[0].jankType, JankType::None); } TEST_F(FrameTimelineTest, jankClassification_displayFrameOnTimeFinishEarlyPresent) { diff --git a/services/surfaceflinger/tests/unittests/JankTrackerTest.cpp b/services/surfaceflinger/tests/unittests/JankTrackerTest.cpp new file mode 100644 index 0000000000..2941a14ef9 --- /dev/null +++ b/services/surfaceflinger/tests/unittests/JankTrackerTest.cpp @@ -0,0 +1,216 @@ +/* + * Copyright 2024 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. + */ + +#include +#include +#include "BackgroundExecutor.h" +#include "Jank/JankTracker.h" + +#include +#include + +namespace android { + +namespace { + +using namespace testing; + +class MockJankListener : public gui::BnJankListener { +public: + MockJankListener() = default; + ~MockJankListener() override = default; + + MOCK_METHOD(binder::Status, onJankData, (const std::vector& jankData), + (override)); +}; + +} // anonymous namespace + +class JankTrackerTest : public Test { +public: + JankTrackerTest() {} + + void SetUp() override { mListener = sp>::make(); } + + void addJankListener(int32_t layerId) { + JankTracker::addJankListener(layerId, IInterface::asBinder(mListener)); + } + + void removeJankListener(int32_t layerId, int64_t after) { + JankTracker::removeJankListener(layerId, IInterface::asBinder(mListener), after); + } + + void addJankData(int32_t layerId, int jankType) { + gui::JankData data; + data.frameVsyncId = mVsyncId++; + data.jankType = jankType; + data.frameIntervalNs = 8333333; + JankTracker::onJankData(layerId, data); + } + + void flushBackgroundThread() { BackgroundExecutor::getLowPriorityInstance().flushQueue(); } + + size_t listenerCount() { return JankTracker::sListenerCount; } + + std::vector getCollectedJankData(int32_t layerId) { + return JankTracker::getCollectedJankDataForTesting(layerId); + } + + sp> mListener = nullptr; + int64_t mVsyncId = 1000; +}; + +TEST_F(JankTrackerTest, jankDataIsTrackedAndPropagated) { + ASSERT_EQ(listenerCount(), 0u); + + EXPECT_CALL(*mListener.get(), onJankData(SizeIs(3))) + .WillOnce([](const std::vector& jankData) { + EXPECT_EQ(jankData[0].frameVsyncId, 1000); + EXPECT_EQ(jankData[0].jankType, 1); + EXPECT_EQ(jankData[0].frameIntervalNs, 8333333); + + EXPECT_EQ(jankData[1].frameVsyncId, 1001); + EXPECT_EQ(jankData[1].jankType, 2); + EXPECT_EQ(jankData[1].frameIntervalNs, 8333333); + + EXPECT_EQ(jankData[2].frameVsyncId, 1002); + EXPECT_EQ(jankData[2].jankType, 3); + EXPECT_EQ(jankData[2].frameIntervalNs, 8333333); + return binder::Status::ok(); + }); + EXPECT_CALL(*mListener.get(), onJankData(SizeIs(2))) + .WillOnce([](const std::vector& jankData) { + EXPECT_EQ(jankData[0].frameVsyncId, 1003); + EXPECT_EQ(jankData[0].jankType, 4); + EXPECT_EQ(jankData[0].frameIntervalNs, 8333333); + + EXPECT_EQ(jankData[1].frameVsyncId, 1004); + EXPECT_EQ(jankData[1].jankType, 5); + EXPECT_EQ(jankData[1].frameIntervalNs, 8333333); + + return binder::Status::ok(); + }); + + addJankListener(123); + addJankData(123, 1); + addJankData(123, 2); + addJankData(123, 3); + JankTracker::flushJankData(123); + addJankData(123, 4); + removeJankListener(123, mVsyncId); + addJankData(123, 5); + JankTracker::flushJankData(123); + addJankData(123, 6); + JankTracker::flushJankData(123); + removeJankListener(123, 0); + + flushBackgroundThread(); +} + +TEST_F(JankTrackerTest, jankDataIsAutomaticallyFlushedInBatches) { + ASSERT_EQ(listenerCount(), 0u); + + // needs to be larger than kJankDataBatchSize in JankTracker.cpp. + constexpr size_t kNumberOfJankDataToSend = 234; + + size_t jankDataReceived = 0; + size_t numBatchesReceived = 0; + + EXPECT_CALL(*mListener.get(), onJankData(_)) + .WillRepeatedly([&](const std::vector& jankData) { + jankDataReceived += jankData.size(); + numBatchesReceived++; + return binder::Status::ok(); + }); + + addJankListener(123); + for (size_t i = 0; i < kNumberOfJankDataToSend; i++) { + addJankData(123, 0); + } + + flushBackgroundThread(); + // Check that we got some data, without explicitly flushing. + EXPECT_GT(jankDataReceived, 0u); + EXPECT_GT(numBatchesReceived, 0u); + EXPECT_LT(numBatchesReceived, jankDataReceived); // batches should be > size 1. + + removeJankListener(123, 0); + JankTracker::flushJankData(123); + flushBackgroundThread(); + EXPECT_EQ(jankDataReceived, kNumberOfJankDataToSend); +} + +TEST_F(JankTrackerTest, jankListenerIsRemovedWhenReturningNullError) { + ASSERT_EQ(listenerCount(), 0u); + + EXPECT_CALL(*mListener.get(), onJankData(SizeIs(3))) + .WillOnce(Return(binder::Status::fromExceptionCode(binder::Status::EX_NULL_POINTER))); + + addJankListener(123); + addJankData(123, 1); + addJankData(123, 2); + addJankData(123, 3); + JankTracker::flushJankData(123); + addJankData(123, 4); + addJankData(123, 5); + JankTracker::flushJankData(123); + flushBackgroundThread(); + + EXPECT_EQ(listenerCount(), 0u); +} + +TEST_F(JankTrackerTest, jankDataIsDroppedIfNobodyIsListening) { + ASSERT_EQ(listenerCount(), 0u); + + addJankData(123, 1); + addJankData(123, 2); + addJankData(123, 3); + flushBackgroundThread(); + + EXPECT_EQ(getCollectedJankData(123).size(), 0u); +} + +TEST_F(JankTrackerTest, listenerCountTracksRegistrations) { + ASSERT_EQ(listenerCount(), 0u); + + addJankListener(123); + addJankListener(456); + flushBackgroundThread(); + EXPECT_EQ(listenerCount(), 2u); + + removeJankListener(123, 0); + JankTracker::flushJankData(123); + removeJankListener(456, 0); + JankTracker::flushJankData(456); + flushBackgroundThread(); + EXPECT_EQ(listenerCount(), 0u); +} + +TEST_F(JankTrackerTest, listenerCountIsAccurateOnDuplicateRegistration) { + ASSERT_EQ(listenerCount(), 0u); + + addJankListener(123); + addJankListener(123); + flushBackgroundThread(); + EXPECT_EQ(listenerCount(), 1u); + + removeJankListener(123, 0); + JankTracker::flushJankData(123); + flushBackgroundThread(); + EXPECT_EQ(listenerCount(), 0u); +} + +} // namespace android \ No newline at end of file diff --git a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp index 46733b9a83..0745f870e4 100644 --- a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp @@ -302,58 +302,6 @@ public: EXPECT_EQ(PresentState::Presented, bufferSurfaceFrameTX->getPresentState()); } - void PendingSurfaceFramesRemovedAfterClassification() { - sp layer = createLayer(); - - sp fence1(sp::make()); - auto acquireFence1 = fenceFactory.createFenceTimeForTest(fence1); - BufferData bufferData; - bufferData.acquireFence = fence1; - bufferData.frameNumber = 1; - bufferData.flags |= BufferData::BufferDataChange::fenceChanged; - bufferData.flags |= BufferData::BufferDataChange::frameNumberChanged; - std::shared_ptr externalTexture1 = std::make_shared< - renderengine::mock::FakeExternalTexture>(1U /*width*/, 1U /*height*/, - 1ULL /* bufferId */, - HAL_PIXEL_FORMAT_RGBA_8888, - 0ULL /*usage*/); - FrameTimelineInfo ftInfo; - ftInfo.vsyncId = 1; - ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture1, bufferData, 10, 20, false, ftInfo); - ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); - const auto droppedSurfaceFrame = layer->mDrawingState.bufferSurfaceFrameTX; - - sp fence2(sp::make()); - auto acquireFence2 = fenceFactory.createFenceTimeForTest(fence2); - bufferData.acquireFence = fence2; - bufferData.frameNumber = 1; - bufferData.flags |= BufferData::BufferDataChange::fenceChanged; - bufferData.flags |= BufferData::BufferDataChange::frameNumberChanged; - std::shared_ptr externalTexture2 = std::make_shared< - renderengine::mock::FakeExternalTexture>(1U /*width*/, 1U /*height*/, - 1ULL /* bufferId */, - HAL_PIXEL_FORMAT_RGBA_8888, - 0ULL /*usage*/); - layer->setBuffer(externalTexture2, bufferData, 10, 20, false, ftInfo); - acquireFence2->signalForTest(12); - - ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); - auto presentedSurfaceFrame = layer->mDrawingState.bufferSurfaceFrameTX; - - commitTransaction(layer.get()); - layer->updateTexImage(15); - - // Both the droppedSurfaceFrame and presentedSurfaceFrame should be in - // pendingJankClassifications. - EXPECT_EQ(2u, layer->mPendingJankClassifications.size()); - presentedSurfaceFrame->onPresent(20, JankType::None, 90_Hz, 90_Hz, - /*displayDeadlineDelta*/ 0, /*displayPresentDelta*/ 0); - layer->releasePendingBuffer(25); - - EXPECT_EQ(0u, layer->mPendingJankClassifications.size()); - } - void BufferSurfaceFrame_ReplaceValidTokenBufferWithInvalidTokenBuffer() { sp layer = createLayer(); @@ -445,8 +393,7 @@ public: void MultipleCommitsBeforeLatch() { sp layer = createLayer(); - uint32_t surfaceFramesPendingClassification = 0; - std::vector> bufferlessSurfaceFrames; + std::vector> surfaceFrames; for (int i = 0; i < 10; i += 2) { sp fence(sp::make()); BufferData bufferData; @@ -469,51 +416,43 @@ public: layer->setFrameTimelineVsyncForBufferlessTransaction(ftInfo2, 10); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); EXPECT_EQ(1u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); - auto& bufferlessSurfaceFrame = - layer->mDrawingState.bufferlessSurfaceFramesTX.at(/*vsyncId*/ 2); - bufferlessSurfaceFrames.push_back(bufferlessSurfaceFrame); + + surfaceFrames.push_back(layer->mDrawingState.bufferSurfaceFrameTX); + surfaceFrames.push_back( + layer->mDrawingState.bufferlessSurfaceFramesTX.at(/*vsyncId*/ 2)); commitTransaction(layer.get()); - surfaceFramesPendingClassification += 2; - EXPECT_EQ(surfaceFramesPendingClassification, - layer->mPendingJankClassifications.size()); } auto presentedBufferSurfaceFrame = layer->mDrawingState.bufferSurfaceFrameTX; layer->updateTexImage(15); // BufferlessSurfaceFrames are immediately set to presented and added to the DisplayFrame. // Since we don't have access to DisplayFrame here, trigger an onPresent directly. - for (auto& surfaceFrame : bufferlessSurfaceFrames) { - surfaceFrame->onPresent(20, JankType::None, 90_Hz, 90_Hz, - /*displayDeadlineDelta*/ 0, /*displayPresentDelta*/ 0); + // The odd indices are the bufferless frames. + for (uint32_t i = 1; i < 10; i += 2) { + surfaceFrames[i]->onPresent(20, JankType::None, 90_Hz, 90_Hz, + /*displayDeadlineDelta*/ 0, /*displayPresentDelta*/ 0); } presentedBufferSurfaceFrame->onPresent(20, JankType::None, 90_Hz, 90_Hz, /*displayDeadlineDelta*/ 0, /*displayPresentDelta*/ 0); - // There should be 10 bufferlessSurfaceFrames and 1 bufferSurfaceFrame - ASSERT_EQ(10u, surfaceFramesPendingClassification); - ASSERT_EQ(surfaceFramesPendingClassification, layer->mPendingJankClassifications.size()); - // For the frames upto 8, the bufferSurfaceFrame should have been dropped while the // bufferlessSurfaceFrame presented for (uint32_t i = 0; i < 8; i += 2) { - auto& bufferSurfaceFrame = layer->mPendingJankClassifications[i]; - auto& bufferlessSurfaceFrame = layer->mPendingJankClassifications[i + 1]; + auto bufferSurfaceFrame = surfaceFrames[i]; + auto bufferlessSurfaceFrame = surfaceFrames[i + 1]; EXPECT_EQ(bufferSurfaceFrame->getPresentState(), PresentState::Dropped); EXPECT_EQ(bufferlessSurfaceFrame->getPresentState(), PresentState::Presented); } { - auto& bufferSurfaceFrame = layer->mPendingJankClassifications[8u]; - auto& bufferlessSurfaceFrame = layer->mPendingJankClassifications[9u]; + auto bufferSurfaceFrame = surfaceFrames[8]; + auto bufferlessSurfaceFrame = surfaceFrames[9]; EXPECT_EQ(bufferSurfaceFrame->getPresentState(), PresentState::Presented); EXPECT_EQ(bufferlessSurfaceFrame->getPresentState(), PresentState::Presented); } layer->releasePendingBuffer(25); - - // There shouldn't be any pending classifications. Everything should have been cleared. - EXPECT_EQ(0u, layer->mPendingJankClassifications.size()); } }; @@ -541,10 +480,6 @@ TEST_F(TransactionSurfaceFrameTest, MultipleSurfaceFramesPresentedTogether) { MultipleSurfaceFramesPresentedTogether(); } -TEST_F(TransactionSurfaceFrameTest, PendingSurfaceFramesRemovedAfterClassification) { - PendingSurfaceFramesRemovedAfterClassification(); -} - TEST_F(TransactionSurfaceFrameTest, BufferSurfaceFrame_ReplaceValidTokenBufferWithInvalidTokenBuffer) { BufferSurfaceFrame_ReplaceValidTokenBufferWithInvalidTokenBuffer(); -- cgit v1.2.3-59-g8ed1b From dcc9d9b5cf697d238cbdca93a99a7a3aa48281fe Mon Sep 17 00:00:00 2001 From: Marzia Favaro Date: Wed, 10 Jan 2024 10:17:00 +0000 Subject: Edge extension effect: shader reimplementation X-axis activity transitions require the translation of the surfaces involved. As this movement would create unwanted see-through, we would have added side windows anchored to the moving activities, and textured them by clamping their parents. We are replacing the additional windows with the extension of the surfaces, and filling the area without buffer with a shader. See go/edge-extension-sksl for more info. Bug: 322036393 Test: LayerSnapshotTest Flag: EXEMPT (calls will be protected by wm shell with com.android.window.flags.edge_extension_shader) Change-Id: I3682efd16a1b311d67a522bb85960f100948b2ea --- libs/gui/LayerState.cpp | 7 ++ libs/gui/SurfaceComposerClient.cpp | 14 ++++ .../aidl/android/gui/EdgeExtensionParameters.aidl | 27 ++++++ libs/gui/include/gui/LayerState.h | 7 +- libs/gui/include/gui/SurfaceComposerClient.h | 12 +++ libs/renderengine/Android.bp | 1 + .../include/renderengine/LayerSettings.h | 14 +++- libs/renderengine/skia/SkiaRenderEngine.cpp | 47 ++++++----- libs/renderengine/skia/SkiaRenderEngine.h | 3 + .../skia/filters/EdgeExtensionShaderFactory.cpp | 80 ++++++++++++++++++ .../skia/filters/EdgeExtensionShaderFactory.h | 42 ++++++++++ libs/ui/include/ui/EdgeExtensionEffect.h | 95 ++++++++++++++++++++++ .../compositionengine/LayerFECompositionState.h | 5 ++ .../src/ClientCompositionRequestCache.cpp | 3 +- services/surfaceflinger/FrontEnd/LayerSnapshot.cpp | 10 ++- .../FrontEnd/LayerSnapshotBuilder.cpp | 67 +++++++++++++-- .../surfaceflinger/FrontEnd/LayerSnapshotBuilder.h | 4 + .../FrontEnd/RequestedLayerState.cpp | 5 +- services/surfaceflinger/LayerFE.cpp | 1 + 19 files changed, 413 insertions(+), 31 deletions(-) create mode 100644 libs/gui/aidl/android/gui/EdgeExtensionParameters.aidl create mode 100644 libs/renderengine/skia/filters/EdgeExtensionShaderFactory.cpp create mode 100644 libs/renderengine/skia/filters/EdgeExtensionShaderFactory.h create mode 100644 libs/ui/include/ui/EdgeExtensionEffect.h (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 3745805aa3..307ae3990e 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -177,6 +177,7 @@ status_t layer_state_t::write(Parcel& output) const } SAFE_PARCEL(output.write, stretchEffect); + SAFE_PARCEL(output.writeParcelable, edgeExtensionParameters); SAFE_PARCEL(output.write, bufferCrop); SAFE_PARCEL(output.write, destinationFrame); SAFE_PARCEL(output.writeInt32, static_cast(trustedOverlay)); @@ -306,6 +307,7 @@ status_t layer_state_t::read(const Parcel& input) } SAFE_PARCEL(input.read, stretchEffect); + SAFE_PARCEL(input.readParcelable, &edgeExtensionParameters); SAFE_PARCEL(input.read, bufferCrop); SAFE_PARCEL(input.read, destinationFrame); uint32_t trustedOverlayInt; @@ -682,6 +684,10 @@ void layer_state_t::merge(const layer_state_t& other) { what |= eStretchChanged; stretchEffect = other.stretchEffect; } + if (other.what & eEdgeExtensionChanged) { + what |= eEdgeExtensionChanged; + edgeExtensionParameters = other.edgeExtensionParameters; + } if (other.what & eBufferCropChanged) { what |= eBufferCropChanged; bufferCrop = other.bufferCrop; @@ -783,6 +789,7 @@ uint64_t layer_state_t::diff(const layer_state_t& other) const { CHECK_DIFF(diff, eAutoRefreshChanged, other, autoRefresh); CHECK_DIFF(diff, eTrustedOverlayChanged, other, trustedOverlay); CHECK_DIFF(diff, eStretchChanged, other, stretchEffect); + CHECK_DIFF(diff, eEdgeExtensionChanged, other, edgeExtensionParameters); CHECK_DIFF(diff, eBufferCropChanged, other, bufferCrop); CHECK_DIFF(diff, eDestinationFrameChanged, other, destinationFrame); if (other.what & eProducerDisconnect) diff |= eProducerDisconnect; diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 902684cf03..88d3a7cb1d 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -2330,6 +2331,19 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setStret return *this; } +SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setEdgeExtensionEffect( + const sp& sc, const gui::EdgeExtensionParameters& effect) { + layer_state_t* s = getLayerState(sc); + if (!s) { + mStatus = BAD_INDEX; + return *this; + } + + s->what |= layer_state_t::eEdgeExtensionChanged; + s->edgeExtensionParameters = effect; + return *this; +} + SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBufferCrop( const sp& sc, const Rect& bufferCrop) { layer_state_t* s = getLayerState(sc); diff --git a/libs/gui/aidl/android/gui/EdgeExtensionParameters.aidl b/libs/gui/aidl/android/gui/EdgeExtensionParameters.aidl new file mode 100644 index 0000000000..44f4259f74 --- /dev/null +++ b/libs/gui/aidl/android/gui/EdgeExtensionParameters.aidl @@ -0,0 +1,27 @@ +/* + * Copyright 2024 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. + */ + +package android.gui; + + +/** @hide */ +parcelable EdgeExtensionParameters { + // These represent the translation of the window as requested by the animation + boolean extendRight; + boolean extendLeft; + boolean extendTop; + boolean extendBottom; +} \ No newline at end of file diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 5f2f8dc013..3fb1894585 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -218,6 +219,7 @@ struct layer_state_t { eTrustedOverlayChanged = 0x4000'00000000, eDropInputModeChanged = 0x8000'00000000, eExtendedRangeBrightnessChanged = 0x10000'00000000, + eEdgeExtensionChanged = 0x20000'00000000, }; layer_state_t(); @@ -241,7 +243,7 @@ struct layer_state_t { layer_state_t::eCropChanged | layer_state_t::eDestinationFrameChanged | layer_state_t::eMatrixChanged | layer_state_t::ePositionChanged | layer_state_t::eTransformToDisplayInverseChanged | - layer_state_t::eTransparentRegionChanged; + layer_state_t::eTransparentRegionChanged | layer_state_t::eEdgeExtensionChanged; // Buffer and related updates. static constexpr uint64_t BUFFER_CHANGES = layer_state_t::eApiChanged | @@ -393,6 +395,9 @@ struct layer_state_t { // Stretch effect to be applied to this layer StretchEffect stretchEffect; + // Edge extension effect to be applied to this layer + gui::EdgeExtensionParameters edgeExtensionParameters; + Rect bufferCrop; Rect destinationFrame; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 88c0c53e12..a10283b371 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -744,6 +745,17 @@ public: Transaction& setStretchEffect(const sp& sc, const StretchEffect& stretchEffect); + /** + * Provides the edge extension effect configured on a container that the + * surface is rendered within. + * @param sc target surface the edge extension should be applied to + * @param effect the corresponding EdgeExtensionParameters to be applied + * to the surface. + * @return The transaction being constructed + */ + Transaction& setEdgeExtensionEffect(const sp& sc, + const gui::EdgeExtensionParameters& effect); + Transaction& setBufferCrop(const sp& sc, const Rect& bufferCrop); Transaction& setDestinationFrame(const sp& sc, const Rect& destinationFrame); diff --git a/libs/renderengine/Android.bp b/libs/renderengine/Android.bp index 4a04467308..ecf98c6696 100644 --- a/libs/renderengine/Android.bp +++ b/libs/renderengine/Android.bp @@ -105,6 +105,7 @@ filegroup { "skia/filters/LinearEffect.cpp", "skia/filters/MouriMap.cpp", "skia/filters/StretchShaderFactory.cpp", + "skia/filters/EdgeExtensionShaderFactory.cpp", ], } diff --git a/libs/renderengine/include/renderengine/LayerSettings.h b/libs/renderengine/include/renderengine/LayerSettings.h index 8ac0af47c6..859ae8b6e2 100644 --- a/libs/renderengine/include/renderengine/LayerSettings.h +++ b/libs/renderengine/include/renderengine/LayerSettings.h @@ -31,6 +31,7 @@ #include #include #include +#include "ui/EdgeExtensionEffect.h" #include @@ -134,6 +135,7 @@ struct LayerSettings { mat4 blurRegionTransform = mat4(); StretchEffect stretchEffect; + EdgeExtensionEffect edgeExtensionEffect; // Name associated with the layer for debugging purposes. std::string name; @@ -183,7 +185,9 @@ static inline bool operator==(const LayerSettings& lhs, const LayerSettings& rhs lhs.skipContentDraw == rhs.skipContentDraw && lhs.shadow == rhs.shadow && lhs.backgroundBlurRadius == rhs.backgroundBlurRadius && lhs.blurRegionTransform == rhs.blurRegionTransform && - lhs.stretchEffect == rhs.stretchEffect && lhs.whitePointNits == rhs.whitePointNits; + lhs.stretchEffect == rhs.stretchEffect && + lhs.edgeExtensionEffect == rhs.edgeExtensionEffect && + lhs.whitePointNits == rhs.whitePointNits; } static inline void PrintTo(const Buffer& settings, ::std::ostream* os) { @@ -254,6 +258,10 @@ static inline void PrintTo(const StretchEffect& effect, ::std::ostream* os) { *os << "\n}"; } +static inline void PrintTo(const EdgeExtensionEffect& effect, ::std::ostream* os) { + *os << effect; +} + static inline void PrintTo(const LayerSettings& settings, ::std::ostream* os) { *os << "LayerSettings for '" << settings.name.c_str() << "' {"; *os << "\n .geometry = "; @@ -285,6 +293,10 @@ static inline void PrintTo(const LayerSettings& settings, ::std::ostream* os) { *os << "\n .stretchEffect = "; PrintTo(settings.stretchEffect, os); } + + if (settings.edgeExtensionEffect.hasEffect()) { + *os << "\n .edgeExtensionEffect = " << settings.edgeExtensionEffect; + } *os << "\n .whitePointNits = " << settings.whitePointNits; *os << "\n}"; } diff --git a/libs/renderengine/skia/SkiaRenderEngine.cpp b/libs/renderengine/skia/SkiaRenderEngine.cpp index 5f37125d16..a609f2d2b3 100644 --- a/libs/renderengine/skia/SkiaRenderEngine.cpp +++ b/libs/renderengine/skia/SkiaRenderEngine.cpp @@ -507,16 +507,24 @@ sk_sp SkiaRenderEngine::createRuntimeEffectShader( const RuntimeEffectShaderParameters& parameters) { // The given surface will be stretched by HWUI via matrix transformation // which gets similar results for most surfaces - // Determine later on if we need to leverage the stertch shader within + // Determine later on if we need to leverage the stretch shader within // surface flinger const auto& stretchEffect = parameters.layer.stretchEffect; const auto& targetBuffer = parameters.layer.source.buffer.buffer; + const auto graphicBuffer = targetBuffer ? targetBuffer->getBuffer() : nullptr; + auto shader = parameters.shader; - if (stretchEffect.hasEffect()) { - const auto graphicBuffer = targetBuffer ? targetBuffer->getBuffer() : nullptr; - if (graphicBuffer && shader) { + if (graphicBuffer && parameters.shader) { + if (stretchEffect.hasEffect()) { shader = mStretchShaderFactory.createSkShader(shader, stretchEffect); } + // The given surface requires to be filled outside of its buffer bounds if the edge + // extension is required + const auto& edgeExtensionEffect = parameters.layer.edgeExtensionEffect; + if (edgeExtensionEffect.hasEffect()) { + shader = mEdgeExtensionShaderFactory.createSkShader(shader, parameters.layer, + parameters.imageBounds); + } } if (parameters.requiresLinearEffect) { @@ -566,8 +574,6 @@ sk_sp SkiaRenderEngine::createRuntimeEffectShader( parameters.layerDimmingRatio, 1.f)); } - const auto targetBuffer = parameters.layer.source.buffer.buffer; - const auto graphicBuffer = targetBuffer ? targetBuffer->getBuffer() : nullptr; const auto hardwareBuffer = graphicBuffer ? graphicBuffer->toAHardwareBuffer() : nullptr; return createLinearEffectShader(shader, effect, runtimeEffect, std::move(colorTransform), parameters.display.maxLuminance, @@ -1039,18 +1045,20 @@ void SkiaRenderEngine::drawLayersInternal( toSkColorSpace(layerDataspace))); } - paint.setShader(createRuntimeEffectShader( - RuntimeEffectShaderParameters{.shader = shader, - .layer = layer, - .display = display, - .undoPremultipliedAlpha = !item.isOpaque && - item.usePremultipliedAlpha, - .requiresLinearEffect = requiresLinearEffect, - .layerDimmingRatio = dimInLinearSpace - ? layerDimmingRatio - : 1.f, - .outputDataSpace = display.outputDataspace, - .fakeOutputDataspace = fakeDataspace})); + SkRect imageBounds; + matrix.mapRect(&imageBounds, SkRect::Make(image->bounds())); + + paint.setShader(createRuntimeEffectShader(RuntimeEffectShaderParameters{ + .shader = shader, + .layer = layer, + .display = display, + .undoPremultipliedAlpha = !item.isOpaque && item.usePremultipliedAlpha, + .requiresLinearEffect = requiresLinearEffect, + .layerDimmingRatio = dimInLinearSpace ? layerDimmingRatio : 1.f, + .outputDataSpace = display.outputDataspace, + .fakeOutputDataspace = fakeDataspace, + .imageBounds = imageBounds, + })); // Turn on dithering when dimming beyond this (arbitrary) threshold... static constexpr float kDimmingThreshold = 0.9f; @@ -1118,7 +1126,8 @@ void SkiaRenderEngine::drawLayersInternal( .requiresLinearEffect = requiresLinearEffect, .layerDimmingRatio = layerDimmingRatio, .outputDataSpace = display.outputDataspace, - .fakeOutputDataspace = fakeDataspace})); + .fakeOutputDataspace = fakeDataspace, + .imageBounds = SkRect::MakeEmpty()})); } if (layer.disableBlending) { diff --git a/libs/renderengine/skia/SkiaRenderEngine.h b/libs/renderengine/skia/SkiaRenderEngine.h index c8f9241257..224a1cad92 100644 --- a/libs/renderengine/skia/SkiaRenderEngine.h +++ b/libs/renderengine/skia/SkiaRenderEngine.h @@ -38,6 +38,7 @@ #include "compat/SkiaGpuContext.h" #include "debug/SkiaCapture.h" #include "filters/BlurFilter.h" +#include "filters/EdgeExtensionShaderFactory.h" #include "filters/LinearEffect.h" #include "filters/StretchShaderFactory.h" @@ -156,6 +157,7 @@ private: float layerDimmingRatio; const ui::Dataspace outputDataSpace; const ui::Dataspace fakeOutputDataspace; + const SkRect& imageBounds; }; sk_sp createRuntimeEffectShader(const RuntimeEffectShaderParameters&); @@ -175,6 +177,7 @@ private: AutoBackendTexture::CleanupManager mTextureCleanupMgr GUARDED_BY(mRenderingMutex); StretchShaderFactory mStretchShaderFactory; + EdgeExtensionShaderFactory mEdgeExtensionShaderFactory; sp mLastDrawFence; BlurFilter* mBlurFilter = nullptr; diff --git a/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.cpp b/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.cpp new file mode 100644 index 0000000000..1dbcc29190 --- /dev/null +++ b/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.cpp @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2024 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. + */ + +#include "EdgeExtensionShaderFactory.h" +#include +#include +#include +#include +#include "log/log_main.h" + +namespace android::renderengine::skia { + +static const SkString edgeShader = SkString(R"( + uniform shader uContentTexture; + uniform vec2 uImgSize; + + // TODO(b/214232209) oobTolerance is temporary and will be removed when the scrollbar will be + // hidden during the animation + const float oobTolerance = 15; + const int blurRadius = 3; + const float blurArea = float((2 * blurRadius + 1) * (2 * blurRadius + 1)); + + vec4 boxBlur(vec2 p) { + vec4 sumColors = vec4(0); + + for (int i = -blurRadius; i <= blurRadius; i++) { + for (int j = -blurRadius; j <= blurRadius; j++) { + sumColors += uContentTexture.eval(p + vec2(i, j)); + } + } + return sumColors / blurArea; + } + + vec4 main(vec2 coord) { + vec2 nearestTexturePoint = clamp(coord, vec2(0, 0), uImgSize); + if (coord == nearestTexturePoint) { + return uContentTexture.eval(coord); + } else { + vec2 samplePoint = nearestTexturePoint + oobTolerance * normalize( + nearestTexturePoint - coord); + return boxBlur(samplePoint); + } + } +)"); + +sk_sp EdgeExtensionShaderFactory::createSkShader(const sk_sp& inputShader, + const LayerSettings& layer, + const SkRect& imageBounds) { + if (mBuilder == nullptr) { + const static SkRuntimeEffect::Result instance = SkRuntimeEffect::MakeForShader(edgeShader); + if (!instance.errorText.isEmpty()) { + ALOGE("EdgeExtensionShaderFactory terminated with an error: %s", + instance.errorText.c_str()); + return nullptr; + } + mBuilder = std::make_unique(instance.effect); + } + mBuilder->child("uContentTexture") = inputShader; + if (imageBounds.isEmpty()) { + mBuilder->uniform("uImgSize") = SkPoint{layer.geometry.boundaries.getWidth(), + layer.geometry.boundaries.getHeight()}; + } else { + mBuilder->uniform("uImgSize") = SkPoint{imageBounds.width(), imageBounds.height()}; + } + return mBuilder->makeShader(); +} +} // namespace android::renderengine::skia \ No newline at end of file diff --git a/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.h b/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.h new file mode 100644 index 0000000000..b0a8a9357e --- /dev/null +++ b/libs/renderengine/skia/filters/EdgeExtensionShaderFactory.h @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2024 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. + */ + +#pragma once + +#include +#include +#include +#include +#include +#include + +namespace android::renderengine::skia { + +/** + * This shader is designed to prolong the texture of a surface whose bounds have been extended over + * the size of the texture. This shader is similar to the default clamp, but adds a blur effect and + * samples from close to the edge (compared to on the edge) to avoid weird artifacts when elements + * (in particular, scrollbars) touch the edge. + */ +class EdgeExtensionShaderFactory { +public: + sk_sp createSkShader(const sk_sp& inputShader, const LayerSettings& layer, + const SkRect& imageBounds); + +private: + std::unique_ptr mBuilder; +}; +} // namespace android::renderengine::skia diff --git a/libs/ui/include/ui/EdgeExtensionEffect.h b/libs/ui/include/ui/EdgeExtensionEffect.h new file mode 100644 index 0000000000..02126b12be --- /dev/null +++ b/libs/ui/include/ui/EdgeExtensionEffect.h @@ -0,0 +1,95 @@ +/* + * Copyright (C) 2024 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. + */ + +#pragma once + +/** + * Stores the edge that will be extended + */ +namespace android { + +enum CanonicalDirections { NONE = 0, LEFT = 1, RIGHT = 2, TOP = 4, BOTTOM = 8 }; + +inline std::string to_string(CanonicalDirections direction) { + switch (direction) { + case LEFT: + return "LEFT"; + case RIGHT: + return "RIGHT"; + case TOP: + return "TOP"; + case BOTTOM: + return "BOTTOM"; + case NONE: + return "NONE"; + } +} + +struct EdgeExtensionEffect { + EdgeExtensionEffect(bool left, bool right, bool top, bool bottom) { + extensionEdges = NONE; + if (left) { + extensionEdges |= LEFT; + } + if (right) { + extensionEdges |= RIGHT; + } + if (top) { + extensionEdges |= TOP; + } + if (bottom) { + extensionEdges |= BOTTOM; + } + } + + EdgeExtensionEffect() { EdgeExtensionEffect(false, false, false, false); } + + bool extendsEdge(CanonicalDirections edge) const { return extensionEdges & edge; } + + bool hasEffect() const { return extensionEdges != NONE; }; + + void reset() { extensionEdges = NONE; } + + bool operator==(const EdgeExtensionEffect& other) const { + return extensionEdges == other.extensionEdges; + } + + bool operator!=(const EdgeExtensionEffect& other) const { return !(*this == other); } + +private: + int extensionEdges; +}; + +inline std::string to_string(const EdgeExtensionEffect& effect) { + std::string s = "EdgeExtensionEffect={edges=["; + if (effect.hasEffect()) { + for (CanonicalDirections edge : {LEFT, RIGHT, TOP, BOTTOM}) { + if (effect.extendsEdge(edge)) { + s += to_string(edge) + ", "; + } + } + } else { + s += to_string(NONE); + } + s += "]}"; + return s; +} + +inline std::ostream& operator<<(std::ostream& os, const EdgeExtensionEffect effect) { + os << to_string(effect); + return os; +} +} // namespace android diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/LayerFECompositionState.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/LayerFECompositionState.h index 11759b855f..d1429a2ec6 100644 --- a/services/surfaceflinger/CompositionEngine/include/compositionengine/LayerFECompositionState.h +++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/LayerFECompositionState.h @@ -35,6 +35,7 @@ #pragma clang diagnostic ignored "-Wextra" #include +#include #include #include #include @@ -133,12 +134,16 @@ struct LayerFECompositionState { // The bounds of the layer in layer local coordinates FloatRect geomLayerBounds; + // The crop to apply to the layer in layer local coordinates + FloatRect geomLayerCrop; + ShadowSettings shadowSettings; // List of regions that require blur std::vector blurRegions; StretchEffect stretchEffect; + EdgeExtensionEffect edgeExtensionEffect; /* * Geometry state diff --git a/services/surfaceflinger/CompositionEngine/src/ClientCompositionRequestCache.cpp b/services/surfaceflinger/CompositionEngine/src/ClientCompositionRequestCache.cpp index bdaa1d0ae1..d9018bc3ab 100644 --- a/services/surfaceflinger/CompositionEngine/src/ClientCompositionRequestCache.cpp +++ b/services/surfaceflinger/CompositionEngine/src/ClientCompositionRequestCache.cpp @@ -37,7 +37,8 @@ inline bool equalIgnoringSource(const renderengine::LayerSettings& lhs, lhs.colorTransform == rhs.colorTransform && lhs.disableBlending == rhs.disableBlending && lhs.shadow == rhs.shadow && lhs.backgroundBlurRadius == rhs.backgroundBlurRadius && - lhs.stretchEffect == rhs.stretchEffect; + lhs.stretchEffect == rhs.stretchEffect && + lhs.edgeExtensionEffect == rhs.edgeExtensionEffect; } inline bool equalIgnoringBuffer(const renderengine::Buffer& lhs, const renderengine::Buffer& rhs) { diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp index 70e3c64a0f..e5f6b7bcd1 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp +++ b/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp @@ -322,6 +322,10 @@ std::ostream& operator<<(std::ostream& out, const LayerSnapshot& obj) { << touchableRegion.bottom << "," << touchableRegion.right << "}" << "}"; } + + if (obj.edgeExtensionEffect.hasEffect()) { + out << obj.edgeExtensionEffect; + } return out; } @@ -492,8 +496,10 @@ void LayerSnapshot::merge(const RequestedLayerState& requested, bool forceUpdate requested.what & (layer_state_t::eBufferChanged | layer_state_t::eDataspaceChanged | layer_state_t::eApiChanged | layer_state_t::eShadowRadiusChanged | - layer_state_t::eBlurRegionsChanged | layer_state_t::eStretchChanged)) { - forceClientComposition = shadowSettings.length > 0 || stretchEffect.hasEffect(); + layer_state_t::eBlurRegionsChanged | layer_state_t::eStretchChanged | + layer_state_t::eEdgeExtensionChanged)) { + forceClientComposition = shadowSettings.length > 0 || stretchEffect.hasEffect() || + edgeExtensionEffect.hasEffect(); } if (forceUpdate || diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp index 2b1b2c85b3..4e093816dd 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp +++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp @@ -367,6 +367,7 @@ LayerSnapshot LayerSnapshotBuilder::getRootSnapshot() { snapshot.geomLayerBounds = getMaxDisplayBounds({}); snapshot.roundedCorner = RoundedCornerState(); snapshot.stretchEffect = {}; + snapshot.edgeExtensionEffect = {}; snapshot.outputFilter.layerStack = ui::DEFAULT_LAYER_STACK; snapshot.outputFilter.toInternalDisplay = false; snapshot.isSecure = false; @@ -811,6 +812,32 @@ void LayerSnapshotBuilder::updateSnapshot(LayerSnapshot& snapshot, const Args& a : parentSnapshot.stretchEffect; } + if (forceUpdate || + (snapshot.clientChanges | parentSnapshot.clientChanges) & + layer_state_t::eEdgeExtensionChanged) { + if (requested.edgeExtensionParameters.extendLeft || + requested.edgeExtensionParameters.extendRight || + requested.edgeExtensionParameters.extendTop || + requested.edgeExtensionParameters.extendBottom) { + // This is the root layer to which the extension is applied + snapshot.edgeExtensionEffect = + EdgeExtensionEffect(requested.edgeExtensionParameters.extendLeft, + requested.edgeExtensionParameters.extendRight, + requested.edgeExtensionParameters.extendTop, + requested.edgeExtensionParameters.extendBottom); + } else if (parentSnapshot.clientChanges & layer_state_t::eEdgeExtensionChanged) { + // Extension is inherited + snapshot.edgeExtensionEffect = parentSnapshot.edgeExtensionEffect; + } else { + // There is no edge extension + snapshot.edgeExtensionEffect.reset(); + } + if (snapshot.edgeExtensionEffect.hasEffect()) { + snapshot.clientChanges |= layer_state_t::eEdgeExtensionChanged; + snapshot.changes |= RequestedLayerState::Changes::Geometry; + } + } + if (forceUpdate || snapshot.clientChanges & layer_state_t::eColorTransformChanged) { if (!parentSnapshot.colorTransformIsIdentity) { snapshot.colorTransform = parentSnapshot.colorTransform * requested.colorTransform; @@ -899,6 +926,10 @@ void LayerSnapshotBuilder::updateSnapshot(LayerSnapshot& snapshot, const Args& a updateLayerBounds(snapshot, requested, parentSnapshot, primaryDisplayRotationFlags); } + if (snapshot.edgeExtensionEffect.hasEffect()) { + updateBoundsForEdgeExtension(snapshot); + } + if (forceUpdate || snapshot.clientChanges & layer_state_t::eCornerRadiusChanged || snapshot.changes.any(RequestedLayerState::Changes::Geometry | RequestedLayerState::Changes::BufferUsageFlags)) { @@ -917,8 +948,8 @@ void LayerSnapshotBuilder::updateSnapshot(LayerSnapshot& snapshot, const Args& a } // computed snapshot properties - snapshot.forceClientComposition = - snapshot.shadowSettings.length > 0 || snapshot.stretchEffect.hasEffect(); + snapshot.forceClientComposition = snapshot.shadowSettings.length > 0 || + snapshot.stretchEffect.hasEffect() || snapshot.edgeExtensionEffect.hasEffect(); snapshot.contentOpaque = snapshot.isContentOpaque(); snapshot.isOpaque = snapshot.contentOpaque && !snapshot.roundedCorner.hasRoundedCorners() && snapshot.color.a == 1.f; @@ -973,6 +1004,31 @@ void LayerSnapshotBuilder::updateRoundedCorner(LayerSnapshot& snapshot, } } +/** + * According to the edges that we are requested to extend, we increase the bounds to the maximum + * extension allowed by the crop (parent crop + requested crop). The animation that called + * Transition#setEdgeExtensionEffect is in charge of setting the requested crop. + * @param snapshot + */ +void LayerSnapshotBuilder::updateBoundsForEdgeExtension(LayerSnapshot& snapshot) { + EdgeExtensionEffect& effect = snapshot.edgeExtensionEffect; + + if (effect.extendsEdge(LEFT)) { + snapshot.geomLayerBounds.left = snapshot.geomLayerCrop.left; + } + if (effect.extendsEdge(RIGHT)) { + snapshot.geomLayerBounds.right = snapshot.geomLayerCrop.right; + } + if (effect.extendsEdge(TOP)) { + snapshot.geomLayerBounds.top = snapshot.geomLayerCrop.top; + } + if (effect.extendsEdge(BOTTOM)) { + snapshot.geomLayerBounds.bottom = snapshot.geomLayerCrop.bottom; + } + + snapshot.transformedBounds = snapshot.geomLayerTransform.transform(snapshot.geomLayerBounds); +} + void LayerSnapshotBuilder::updateLayerBounds(LayerSnapshot& snapshot, const RequestedLayerState& requested, const LayerSnapshot& parentSnapshot, @@ -1012,11 +1068,12 @@ void LayerSnapshotBuilder::updateLayerBounds(LayerSnapshot& snapshot, FloatRect parentBounds = parentSnapshot.geomLayerBounds; parentBounds = snapshot.localTransform.inverse().transform(parentBounds); snapshot.geomLayerBounds = - (requested.externalTexture) ? snapshot.bufferSize.toFloatRect() : parentBounds; + requested.externalTexture ? snapshot.bufferSize.toFloatRect() : parentBounds; + snapshot.geomLayerCrop = parentBounds; if (!requested.crop.isEmpty()) { - snapshot.geomLayerBounds = snapshot.geomLayerBounds.intersect(requested.crop.toFloatRect()); + snapshot.geomLayerCrop = snapshot.geomLayerCrop.intersect(requested.crop.toFloatRect()); } - snapshot.geomLayerBounds = snapshot.geomLayerBounds.intersect(parentBounds); + snapshot.geomLayerBounds = snapshot.geomLayerBounds.intersect(snapshot.geomLayerCrop); snapshot.transformedBounds = snapshot.geomLayerTransform.transform(snapshot.geomLayerBounds); const Rect geomLayerBoundsWithoutTransparentRegion = RequestedLayerState::reduce(Rect(snapshot.geomLayerBounds), diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h index dbbad7664a..f3c56a4ef0 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h +++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h @@ -113,6 +113,10 @@ private: static void resetRelativeState(LayerSnapshot& snapshot); static void updateRoundedCorner(LayerSnapshot& snapshot, const RequestedLayerState& layerState, const LayerSnapshot& parentSnapshot, const Args& args); + static bool extensionEdgeSharedWithParent(LayerSnapshot& snapshot, + const RequestedLayerState& requested, + const LayerSnapshot& parentSnapshot); + static void updateBoundsForEdgeExtension(LayerSnapshot& snapshot); void updateLayerBounds(LayerSnapshot& snapshot, const RequestedLayerState& layerState, const LayerSnapshot& parentSnapshot, uint32_t displayRotationFlags); static void updateShadows(LayerSnapshot& snapshot, const RequestedLayerState& requested, diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp index 17d3f4bf38..17d2610d7a 100644 --- a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp +++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp @@ -609,8 +609,9 @@ bool RequestedLayerState::isSimpleBufferUpdate(const layer_state_t& s) const { layer_state_t::eSidebandStreamChanged | layer_state_t::eColorSpaceAgnosticChanged | layer_state_t::eShadowRadiusChanged | layer_state_t::eFixedTransformHintChanged | layer_state_t::eTrustedOverlayChanged | layer_state_t::eStretchChanged | - layer_state_t::eBufferCropChanged | layer_state_t::eDestinationFrameChanged | - layer_state_t::eDimmingEnabledChanged | layer_state_t::eExtendedRangeBrightnessChanged | + layer_state_t::eEdgeExtensionChanged | layer_state_t::eBufferCropChanged | + layer_state_t::eDestinationFrameChanged | layer_state_t::eDimmingEnabledChanged | + layer_state_t::eExtendedRangeBrightnessChanged | layer_state_t::eDesiredHdrHeadroomChanged | (FlagManager::getInstance().latch_unsignaled_with_auto_refresh_changed() ? layer_state_t::eFlagsChanged diff --git a/services/surfaceflinger/LayerFE.cpp b/services/surfaceflinger/LayerFE.cpp index 8f819b25e0..b05f0eecc4 100644 --- a/services/surfaceflinger/LayerFE.cpp +++ b/services/surfaceflinger/LayerFE.cpp @@ -173,6 +173,7 @@ std::optional LayerFE::prepareClientC break; } layerSettings.stretchEffect = mSnapshot->stretchEffect; + layerSettings.edgeExtensionEffect = mSnapshot->edgeExtensionEffect; // Record the name of the layer for debugging further down the stack. layerSettings.name = mSnapshot->name; -- cgit v1.2.3-59-g8ed1b From b3daf1a1398fca55eb936e1920f190c51b86fdf5 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Wed, 17 Jul 2024 20:17:36 +0000 Subject: Revert "Fix DisplayState sanitization." This reverts commit b6ddf525be3c2abbde59ae1533494b18a7961087. Reason for revert: b/352098820 Change-Id: Ie505ebeda960412e79327fddc8ac7c6d3900727d --- libs/gui/ISurfaceComposer.cpp | 2 +- libs/gui/SurfaceComposerClient.cpp | 3 +- libs/gui/include/gui/ISurfaceComposer.h | 2 +- libs/gui/tests/Surface_test.cpp | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 4 +- services/surfaceflinger/SurfaceFlinger.h | 2 +- services/surfaceflinger/tests/Credentials_test.cpp | 53 +--------------------- .../tests/unittests/TestableSurfaceFlinger.h | 2 +- 8 files changed, 9 insertions(+), 61 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index 269936858a..ff6b558d41 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -62,7 +62,7 @@ public: status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - Vector& displays, uint32_t flags, const sp& applyToken, + const Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands commands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, uint64_t transactionId, diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 5db539497c..af91bb3ae2 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1059,8 +1059,7 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { uncacheBuffer.token = BufferCache::getInstance().getToken(); uncacheBuffer.id = cacheId; Vector composerStates; - Vector displayStates; - status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, displayStates, + status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, {}, ISurfaceComposer::eOneWay, Transaction::getDefaultApplyToken(), {}, systemTime(), true, {uncacheBuffer}, false, {}, generateId(), {}); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 1ecc216dff..eb4a802c17 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -112,7 +112,7 @@ public: /* open/close transactions. requires ACCESS_SURFACE_FLINGER permission */ virtual status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - Vector& displays, uint32_t flags, const sp& applyToken, + const Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffer, bool hasListenerCallbacks, const std::vector& listenerCallbacks, diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 5e91088378..43cd0f8a7f 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -636,7 +636,7 @@ public: status_t setTransactionState( const FrameTimelineInfo& /*frameTimelineInfo*/, Vector& /*state*/, - Vector& /*displays*/, uint32_t /*flags*/, + const Vector& /*displays*/, uint32_t /*flags*/, const sp& /*applyToken*/, InputWindowCommands /*inputWindowCommands*/, int64_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/, const std::vector& /*cachedBuffer*/, bool /*hasListenerCallbacks*/, diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index f9262a0a9c..596ec12d9e 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5167,7 +5167,7 @@ bool SurfaceFlinger::shouldLatchUnsignaled(const layer_state_t& state, size_t nu status_t SurfaceFlinger::setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& states, - Vector& displays, uint32_t flags, const sp& applyToken, + const Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, uint64_t transactionId, @@ -5182,7 +5182,7 @@ status_t SurfaceFlinger::setTransactionState( composerState.state.sanitize(permissions); } - for (DisplayState& display : displays) { + for (DisplayState display : displays) { display.sanitize(permissions); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 7762bbea9f..ee541c4364 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -559,7 +559,7 @@ private: sp getPhysicalDisplayToken(PhysicalDisplayId displayId) const; status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - Vector& displays, uint32_t flags, const sp& applyToken, + const Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index d355e720d1..ebe11fb0f3 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -277,7 +276,7 @@ TEST_F(CredentialsTest, CreateDisplayTest) { TEST_F(CredentialsTest, CaptureLayersTest) { setupBackgroundSurface(); sp outBuffer; - std::function condition = [=, this]() { + std::function condition = [=]() { LayerCaptureArgs captureArgs; captureArgs.layerHandle = mBGSurfaceControl->getHandle(); captureArgs.sourceCrop = {0, 0, 1, 1}; @@ -397,56 +396,6 @@ TEST_F(CredentialsTest, TransactionPermissionTest) { } } -TEST_F(CredentialsTest, DisplayTransactionPermissionTest) { - const auto display = getFirstDisplayToken(); - - ui::DisplayState displayState; - ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); - const ui::Rotation initialOrientation = displayState.orientation; - - // Set display orientation from an untrusted process. This should fail silently. - { - UIDFaker f{AID_BIN}; - Transaction transaction; - Rect layerStackRect; - Rect displayRect; - transaction.setDisplayProjection(display, initialOrientation + ui::ROTATION_90, - layerStackRect, displayRect); - transaction.apply(/*synchronous=*/true); - } - - // Verify that the display orientation did not change. - ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); - ASSERT_EQ(initialOrientation, displayState.orientation); - - // Set display orientation from a trusted process. - { - UIDFaker f{AID_SYSTEM}; - Transaction transaction; - Rect layerStackRect; - Rect displayRect; - transaction.setDisplayProjection(display, initialOrientation + ui::ROTATION_90, - layerStackRect, displayRect); - transaction.apply(/*synchronous=*/true); - } - - // Verify that the display orientation did change. - ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); - ASSERT_EQ(initialOrientation + ui::ROTATION_90, displayState.orientation); - - // Reset orientation - { - UIDFaker f{AID_SYSTEM}; - Transaction transaction; - Rect layerStackRect; - Rect displayRect; - transaction.setDisplayProjection(display, initialOrientation, layerStackRect, displayRect); - transaction.apply(/*synchronous=*/true); - } - ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); - ASSERT_EQ(initialOrientation, displayState.orientation); -} - } // namespace android // TODO(b/129481165): remove the #pragma below and fix conversion issues diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 7d1ec253c4..007383b3d9 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -528,7 +528,7 @@ public: auto setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& states, - Vector& displays, uint32_t flags, const sp& applyToken, + const Vector& displays, uint32_t flags, const sp& applyToken, const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, std::vector& listenerCallbacks, -- cgit v1.2.3-59-g8ed1b From c9918bf180ca72718cf0249f4d27a69b5254dc93 Mon Sep 17 00:00:00 2001 From: Marzia Favaro Date: Wed, 10 Jul 2024 14:45:35 +0000 Subject: Introduce com::android::graphics::libgui::flags::edge_extension_shader() Bug: 322036393 Test: N/A Flag: com.android.graphics.libgui.flags.edge_extension_shader Change-Id: I982bb8aa3848ce8503bf0150b272c60d7db2b63f --- libs/gui/SurfaceComposerClient.cpp | 6 ++++++ libs/gui/include/gui/SurfaceComposerClient.h | 2 ++ libs/gui/libgui_flags.aconfig | 8 ++++++++ 3 files changed, 16 insertions(+) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index db8786d656..2821b510c3 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -20,6 +20,8 @@ #include #include +#include + #include #include #include @@ -2330,6 +2332,10 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setStret return *this; } +bool SurfaceComposerClient::flagEdgeExtensionEffectUseShader() { + return com::android::graphics::libgui::flags::edge_extension_shader(); +} + SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setEdgeExtensionEffect( const sp& sc, const gui::EdgeExtensionParameters& effect) { layer_state_t* s = getLayerState(sc); diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index a10283b371..95574ee30e 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -339,6 +339,8 @@ public: static std::optional getDisplayDecorationSupport(const sp& displayToken); + static bool flagEdgeExtensionEffectUseShader(); + // ------------------------------------------------------------------------ // surface creation / destruction diff --git a/libs/gui/libgui_flags.aconfig b/libs/gui/libgui_flags.aconfig index 2d70052166..74d806ec79 100644 --- a/libs/gui/libgui_flags.aconfig +++ b/libs/gui/libgui_flags.aconfig @@ -43,3 +43,11 @@ flag { bug: "340933794" is_fixed_read_only: true } # wb_platform_api_improvements + +flag { + name: "edge_extension_shader" + namespace: "windowing_frontend" + description: "Enable edge extension via shader" + bug: "322036393" + is_fixed_read_only: true +} # edge_extension_shader -- cgit v1.2.3-59-g8ed1b From ac70bc579028cdc00a2f14b77718080b26b93c7d Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Tue, 9 Jul 2024 17:11:28 -0500 Subject: Optimize BLAST buffer releases via Unix sockets Bug: 294133380 Flag: com.android.graphics.libgui.flags.buffer_release_channel Test: BLASTBufferQueueTest Change-Id: Ia183452198dadc7f8e540f7219bd44d8b5823458 --- libs/gui/Android.bp | 1 + libs/gui/BLASTBufferQueue.cpp | 326 ++++++++++++++++-- libs/gui/BufferReleaseChannel.cpp | 364 +++++++++++++++++++++ libs/gui/LayerState.cpp | 17 + libs/gui/SurfaceComposerClient.cpp | 16 + libs/gui/include/gui/BLASTBufferQueue.h | 56 +++- libs/gui/include/gui/BufferReleaseChannel.h | 127 +++++++ libs/gui/include/gui/LayerState.h | 4 + libs/gui/include/gui/SurfaceComposerClient.h | 5 + libs/gui/libgui_flags.aconfig | 8 + libs/gui/tests/Android.bp | 1 + libs/gui/tests/BufferReleaseChannel_test.cpp | 121 +++++++ services/surfaceflinger/Layer.cpp | 15 +- services/surfaceflinger/Layer.h | 3 + services/surfaceflinger/SurfaceFlinger.cpp | 4 + .../surfaceflinger/TransactionCallbackInvoker.cpp | 11 + .../surfaceflinger/TransactionCallbackInvoker.h | 9 + 17 files changed, 1062 insertions(+), 26 deletions(-) create mode 100644 libs/gui/BufferReleaseChannel.cpp create mode 100644 libs/gui/include/gui/BufferReleaseChannel.h create mode 100644 libs/gui/tests/BufferReleaseChannel_test.cpp (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 51d2e5305a..1243b214d3 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -255,6 +255,7 @@ filegroup { "BitTube.cpp", "BLASTBufferQueue.cpp", "BufferItemConsumer.cpp", + "BufferReleaseChannel.cpp", "Choreographer.cpp", "CompositorTiming.cpp", "ConsumerBase.cpp", diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 739c3c2a41..f13d499b50 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -38,13 +38,17 @@ #include #include +#include #include +#include +#include #include #include using namespace com::android::graphics::libgui; using namespace std::chrono_literals; +using android::base::unique_fd; namespace { inline const char* boolToString(bool b) { @@ -179,8 +183,6 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati // explicitly so that dequeueBuffer will block mProducer->setDequeueTimeout(std::numeric_limits::max()); - // safe default, most producers are expected to override this - mProducer->setMaxDequeuedBufferCount(2); mBufferItemConsumer = new BLASTBufferItemConsumer(mConsumer, GraphicBuffer::USAGE_HW_COMPOSER | GraphicBuffer::USAGE_HW_TEXTURE, @@ -210,6 +212,12 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati }, this); +#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL) + std::unique_ptr bufferReleaseConsumer; + gui::BufferReleaseChannel::open(mName, bufferReleaseConsumer, mBufferReleaseProducer); + mBufferReleaseReader.emplace(std::move(bufferReleaseConsumer)); +#endif + BQA_LOGV("BLASTBufferQueue created"); } @@ -259,6 +267,9 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, if (surfaceControlChanged) { t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure, layer_state_t::eEnableBackpressure); + if (mBufferReleaseProducer) { + t.setBufferReleaseChannel(mSurfaceControl, mBufferReleaseProducer); + } applyTransaction = true; } mTransformHint = mSurfaceControl->getTransformHint(); @@ -439,6 +450,21 @@ void BLASTBufferQueue::releaseBufferCallback( BBQ_TRACE(); releaseBufferCallbackLocked(id, releaseFence, currentMaxAcquiredBufferCount, false /* fakeRelease */); + if (!mBufferReleaseReader) { + return; + } + // Drain the buffer release channel socket + while (true) { + ReleaseCallbackId releaseCallbackId; + sp releaseFence; + if (status_t status = + mBufferReleaseReader->readNonBlocking(releaseCallbackId, releaseFence); + status != OK) { + break; + } + releaseBufferCallbackLocked(releaseCallbackId, releaseFence, std::nullopt, + false /* fakeRelease */); + } } void BLASTBufferQueue::releaseBufferCallbackLocked( @@ -495,11 +521,11 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, const sp& releaseFence) { auto it = mSubmitted.find(callbackId); if (it == mSubmitted.end()) { - BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %s", - callbackId.to_string().c_str()); return; } mNumAcquired--; + updateDequeueShouldBlockLocked(); + mBufferReleaseReader->interruptBlockingRead(); BBQ_TRACE("frame=%" PRIu64, callbackId.framenumber); BQA_LOGV("released %s", callbackId.to_string().c_str()); mBufferItemConsumer->releaseBuffer(it->second, releaseFence); @@ -564,6 +590,7 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( auto buffer = bufferItem.mGraphicBuffer; mNumFrameAvailable--; + updateDequeueShouldBlockLocked(); BBQ_TRACE("frame=%" PRIu64, bufferItem.mFrameNumber); if (buffer == nullptr) { @@ -582,6 +609,7 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( } mNumAcquired++; + updateDequeueShouldBlockLocked(); mLastAcquiredFrameNumber = bufferItem.mFrameNumber; ReleaseCallbackId releaseCallbackId(buffer->getId(), mLastAcquiredFrameNumber); mSubmitted[releaseCallbackId] = bufferItem; @@ -708,6 +736,7 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { return; } mNumFrameAvailable--; + updateDequeueShouldBlockLocked(); mBufferItemConsumer->releaseBuffer(bufferItem, bufferItem.mFence); } @@ -761,7 +790,9 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { } // add to shadow queue + mNumDequeued--; mNumFrameAvailable++; + updateDequeueShouldBlockLocked(); if (waitForTransactionCallback && mNumFrameAvailable >= 2) { acquireAndReleaseBuffer(); } @@ -812,11 +843,21 @@ void BLASTBufferQueue::onFrameReplaced(const BufferItem& item) { void BLASTBufferQueue::onFrameDequeued(const uint64_t bufferId) { std::lock_guard _lock{mTimestampMutex}; mDequeueTimestamps[bufferId] = systemTime(); -}; + mNumDequeued++; +} void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { - std::lock_guard _lock{mTimestampMutex}; - mDequeueTimestamps.erase(bufferId); + { + std::lock_guard _lock{mTimestampMutex}; + mDequeueTimestamps.erase(bufferId); + } + + { + std::lock_guard lock{mMutex}; + mNumDequeued--; + updateDequeueShouldBlockLocked(); + } + mBufferReleaseReader->interruptBlockingRead(); }; bool BLASTBufferQueue::syncNextTransaction( @@ -888,6 +929,22 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { return mSize != bufferSize; } +void BLASTBufferQueue::updateDequeueShouldBlockLocked() { + int32_t buffersInUse = mNumDequeued + mNumFrameAvailable + mNumAcquired; + int32_t maxBufferCount = std::min(mMaxAcquiredBuffers + mMaxDequeuedBuffers, kMaxBufferCount); + bool bufferAvailable = buffersInUse < maxBufferCount; + // BLASTBufferQueueProducer should block until a buffer is released if + // (1) There are no free buffers available. + // (2) We're not in async mode. In async mode, BufferQueueProducer::dequeueBuffer returns + // WOULD_BLOCK instead of blocking when there are no free buffers. + // (3) We're not in shared buffer mode. In shared buffer mode, both the producer and consumer + // can access the same buffer simultaneously. BufferQueueProducer::dequeueBuffer returns + // the shared buffer immediately instead of blocking. + mDequeueShouldBlock = !(bufferAvailable || mAsyncMode || mSharedBufferMode); + ATRACE_INT("Dequeued", mNumDequeued); + ATRACE_INT("DequeueShouldBlock", mDequeueShouldBlock); +} + class BBQSurface : public Surface { private: std::mutex mMutex; @@ -1116,24 +1173,58 @@ public: producerControlledByApp, output); } + status_t disconnect(int api, DisconnectMode mode) override { + if (status_t status = BufferQueueProducer::disconnect(api, mode); status != OK) { + return status; + } + + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq) { + return OK; + } + + { + std::lock_guard lock{bbq->mMutex}; + bbq->mNumDequeued = 0; + bbq->mNumFrameAvailable = 0; + bbq->mNumAcquired = 0; + bbq->mSubmitted.clear(); + bbq->updateDequeueShouldBlockLocked(); + } + bbq->mBufferReleaseReader->interruptBlockingRead(); + + return OK; + } + // We want to resize the frame history when changing the size of the buffer queue status_t setMaxDequeuedBufferCount(int maxDequeuedBufferCount) override { int maxBufferCount; status_t status = BufferQueueProducer::setMaxDequeuedBufferCount(maxDequeuedBufferCount, &maxBufferCount); - // if we can't determine the max buffer count, then just skip growing the history size - if (status == OK) { - size_t newFrameHistorySize = maxBufferCount + 2; // +2 because triple buffer rendering - // optimize away resizing the frame history unless it will grow - if (newFrameHistorySize > FrameEventHistory::INITIAL_MAX_FRAME_HISTORY) { - sp bbq = mBLASTBufferQueue.promote(); - if (bbq != nullptr) { - ALOGV("increasing frame history size to %zu", newFrameHistorySize); - bbq->resizeFrameEventHistory(newFrameHistorySize); - } - } + if (status != OK) { + return status; } - return status; + + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq) { + return OK; + } + + { + std::lock_guard lock{bbq->mMutex}; + bbq->mMaxDequeuedBuffers = maxDequeuedBufferCount; + bbq->updateDequeueShouldBlockLocked(); + } + bbq->mBufferReleaseReader->interruptBlockingRead(); + + size_t newFrameHistorySize = maxBufferCount + 2; // +2 because triple buffer rendering + // optimize away resizing the frame history unless it will grow + if (newFrameHistorySize > FrameEventHistory::INITIAL_MAX_FRAME_HISTORY) { + ALOGV("increasing frame history size to %zu", newFrameHistorySize); + bbq->resizeFrameEventHistory(newFrameHistorySize); + } + + return OK; } int query(int what, int* value) override { @@ -1144,6 +1235,125 @@ public: return BufferQueueProducer::query(what, value); } + status_t setAsyncMode(bool asyncMode) override { + if (status_t status = BufferQueueProducer::setAsyncMode(asyncMode); status != NO_ERROR) { + return status; + } + + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq) { + return NO_ERROR; + } + + { + std::lock_guard lock{bbq->mMutex}; + bbq->mAsyncMode = asyncMode; + bbq->updateDequeueShouldBlockLocked(); + } + + bbq->mBufferReleaseReader->interruptBlockingRead(); + return NO_ERROR; + } + + status_t setSharedBufferMode(bool sharedBufferMode) override { + if (status_t status = BufferQueueProducer::setSharedBufferMode(sharedBufferMode); + status != NO_ERROR) { + return status; + } + + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq) { + return NO_ERROR; + } + + { + std::lock_guard lock{bbq->mMutex}; + bbq->mSharedBufferMode = sharedBufferMode; + bbq->updateDequeueShouldBlockLocked(); + } + + bbq->mBufferReleaseReader->interruptBlockingRead(); + return NO_ERROR; + } + + status_t detachBuffer(int slot) override { + if (status_t status = BufferQueueProducer::detachBuffer(slot); status != NO_ERROR) { + return status; + } + + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq) { + return NO_ERROR; + } + + { + std::lock_guard lock{bbq->mMutex}; + bbq->mNumDequeued--; + bbq->updateDequeueShouldBlockLocked(); + } + + bbq->mBufferReleaseReader->interruptBlockingRead(); + return NO_ERROR; + } + + // Override dequeueBuffer to block if there are no free buffers. + // + // Buffer releases are communicated via the BufferReleaseChannel. When dequeueBuffer determines + // a free buffer is not available, it blocks on an epoll file descriptor. Epoll is configured to + // detect messages on the BufferReleaseChannel's socket and an eventfd. The eventfd is signaled + // whenever an event other than a buffer release occurs that may change the number of free + // buffers. dequeueBuffer uses epoll in a similar manner as a condition variable by testing for + // the availability of a free buffer in a loop, breaking the loop once a free buffer is + // available. + // + // This is an optimization implemented to reduce thread scheduling delays in the previously + // existing binder release callback. The binder buffer release callback is still used and there + // are no guarantees around order between buffer releases via binder and the + // BufferReleaseChannel. If we attempt to a release a buffer here that has already been released + // via binder, the release is ignored. + status_t dequeueBuffer(int* outSlot, sp* outFence, uint32_t width, uint32_t height, + PixelFormat format, uint64_t usage, uint64_t* outBufferAge, + FrameEventHistoryDelta* outTimestamps) { + sp bbq = mBLASTBufferQueue.promote(); + if (!bbq || !bbq->mBufferReleaseReader) { + return BufferQueueProducer::dequeueBuffer(outSlot, outFence, width, height, format, + usage, outBufferAge, outTimestamps); + } + + if (bbq->mDequeueShouldBlock) { + ATRACE_FORMAT("waiting for free buffer"); + auto maxWaitTime = std::chrono::steady_clock::now() + 1s; + do { + auto timeout = std::chrono::duration_cast( + maxWaitTime - std::chrono::steady_clock::now()); + if (timeout <= 0ms) { + break; + } + + ReleaseCallbackId releaseCallbackId; + sp releaseFence; + status_t status = bbq->mBufferReleaseReader->readBlocking(releaseCallbackId, + releaseFence, timeout); + if (status == WOULD_BLOCK) { + // readBlocking was interrupted. The loop will test if we have a free buffer. + continue; + } + + if (status != OK) { + // An error occurred or readBlocking timed out. + break; + } + + std::lock_guard lock{bbq->mMutex}; + bbq->releaseBufferCallbackLocked(releaseCallbackId, releaseFence, std::nullopt, + false); + } while (bbq->mDequeueShouldBlock); + } + + return BufferQueueProducer::dequeueBuffer(outSlot, outFence, width, height, format, usage, + outBufferAge, outTimestamps); + } + private: const wp mBLASTBufferQueue; }; @@ -1173,6 +1383,16 @@ void BLASTBufferQueue::createBufferQueue(sp* outProducer *outConsumer = consumer; } +void BLASTBufferQueue::onFirstRef() { + // safe default, most producers are expected to override this + // + // This is done in onFirstRef instead of BLASTBufferQueue's constructor because + // BBQBufferQueueProducer::setMaxDequeuedBufferCount promotes a weak pointer to BLASTBufferQueue + // to a strong pointer. If this is done in the constructor, then when the strong pointer goes + // out of scope, it's the last reference so BLASTBufferQueue is deleted. + mProducer->setMaxDequeuedBufferCount(2); +} + void BLASTBufferQueue::resizeFrameEventHistory(size_t newSize) { // This can be null during creation of the buffer queue, but resizing won't do anything at that // point in time, so just ignore. This can go away once the class relationships and lifetimes of @@ -1222,4 +1442,72 @@ void BLASTBufferQueue::setTransactionHangCallback( mTransactionHangCallback = callback; } +BLASTBufferQueue::BufferReleaseReader::BufferReleaseReader( + std::unique_ptr endpoint) + : mEndpoint(std::move(endpoint)) { + mEpollFd = android::base::unique_fd(epoll_create1(0)); + if (!mEpollFd.ok()) { + ALOGE("Failed to create buffer release epoll file descriptor. errno=%d message='%s'", errno, + strerror(errno)); + } + + epoll_event event; + event.events = EPOLLIN; + event.data.fd = mEndpoint->getFd(); + if (epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mEndpoint->getFd(), &event) == -1) { + ALOGE("Failed to register buffer release consumer file descriptor with epoll. errno=%d " + "message='%s'", + errno, strerror(errno)); + } + + mEventFd = android::base::unique_fd(eventfd(0, EFD_NONBLOCK)); + event.data.fd = mEventFd.get(); + if (epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mEventFd.get(), &event) == -1) { + ALOGE("Failed to register buffer release eventfd with epoll. errno=%d message='%s'", errno, + strerror(errno)); + } +} + +status_t BLASTBufferQueue::BufferReleaseReader::readNonBlocking(ReleaseCallbackId& outId, + sp& outFence) { + std::lock_guard lock{mMutex}; + return mEndpoint->readReleaseFence(outId, outFence); +} + +status_t BLASTBufferQueue::BufferReleaseReader::readBlocking(ReleaseCallbackId& outId, + sp& outFence, + std::chrono::milliseconds timeout) { + epoll_event event; + int eventCount = epoll_wait(mEpollFd.get(), &event, 1 /* maxevents */, timeout.count()); + + if (eventCount == -1) { + ALOGE("epoll_wait error while waiting for buffer release. errno=%d message='%s'", errno, + strerror(errno)); + return UNKNOWN_ERROR; + } + + if (eventCount == 0) { + return TIMED_OUT; + } + + if (event.data.fd == mEventFd.get()) { + uint64_t value; + if (read(mEventFd.get(), &value, sizeof(uint64_t)) == -1 && errno != EWOULDBLOCK) { + ALOGE("error while reading from eventfd. errno=%d message='%s'", errno, + strerror(errno)); + } + return WOULD_BLOCK; + } + + std::lock_guard lock{mMutex}; + return mEndpoint->readReleaseFence(outId, outFence); +} + +void BLASTBufferQueue::BufferReleaseReader::interruptBlockingRead() { + uint64_t value = 1; + if (write(mEventFd.get(), &value, sizeof(uint64_t)) == -1) { + ALOGE("failed to notify dequeue event. errno=%d message='%s'", errno, strerror(errno)); + } +} + } // namespace android diff --git a/libs/gui/BufferReleaseChannel.cpp b/libs/gui/BufferReleaseChannel.cpp new file mode 100644 index 0000000000..183b25a4cb --- /dev/null +++ b/libs/gui/BufferReleaseChannel.cpp @@ -0,0 +1,364 @@ +/* + * Copyright (C) 2024 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_TAG "BufferReleaseChannel" +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include + +using android::base::Result; + +namespace android::gui { + +namespace { + +template +static void readAligned(const void*& buffer, size_t& size, T& value) { + size -= FlattenableUtils::align(buffer); + FlattenableUtils::read(buffer, size, value); +} + +template +static void writeAligned(void*& buffer, size_t& size, T value) { + size -= FlattenableUtils::align(buffer); + FlattenableUtils::write(buffer, size, value); +} + +template +static void addAligned(size_t& size, T /* value */) { + size = FlattenableUtils::align(size); + size += sizeof(T); +} + +template +static inline constexpr uint32_t low32(const T n) { + return static_cast(static_cast(n)); +} + +template +static inline constexpr uint32_t high32(const T n) { + return static_cast(static_cast(n) >> 32); +} + +template +static inline constexpr T to64(const uint32_t lo, const uint32_t hi) { + return static_cast(static_cast(hi) << 32 | lo); +} + +} // namespace + +size_t BufferReleaseChannel::Message::getPodSize() const { + size_t size = 0; + addAligned(size, low32(releaseCallbackId.bufferId)); + addAligned(size, high32(releaseCallbackId.bufferId)); + addAligned(size, low32(releaseCallbackId.framenumber)); + addAligned(size, high32(releaseCallbackId.framenumber)); + return size; +} + +size_t BufferReleaseChannel::Message::getFlattenedSize() const { + size_t size = releaseFence->getFlattenedSize(); + size = FlattenableUtils::align<4>(size); + size += getPodSize(); + return size; +} + +status_t BufferReleaseChannel::Message::flatten(void*& buffer, size_t& size, int*& fds, + size_t& count) const { + if (status_t err = releaseFence->flatten(buffer, size, fds, count); err != OK) { + return err; + } + size -= FlattenableUtils::align<4>(buffer); + + // Check we still have enough space + if (size < getPodSize()) { + return NO_MEMORY; + } + + writeAligned(buffer, size, low32(releaseCallbackId.bufferId)); + writeAligned(buffer, size, high32(releaseCallbackId.bufferId)); + writeAligned(buffer, size, low32(releaseCallbackId.framenumber)); + writeAligned(buffer, size, high32(releaseCallbackId.framenumber)); + return OK; +} + +status_t BufferReleaseChannel::Message::unflatten(void const*& buffer, size_t& size, + int const*& fds, size_t& count) { + releaseFence = new Fence(); + if (status_t err = releaseFence->unflatten(buffer, size, fds, count); err != OK) { + return err; + } + size -= FlattenableUtils::align<4>(buffer); + + // Check we still have enough space + if (size < getPodSize()) { + return OK; + } + + uint32_t bufferIdLo = 0, bufferIdHi = 0; + uint32_t frameNumberLo = 0, frameNumberHi = 0; + + readAligned(buffer, size, bufferIdLo); + readAligned(buffer, size, bufferIdHi); + releaseCallbackId.bufferId = to64(bufferIdLo, bufferIdHi); + readAligned(buffer, size, frameNumberLo); + readAligned(buffer, size, frameNumberHi); + releaseCallbackId.framenumber = to64(frameNumberLo, frameNumberHi); + + return NO_ERROR; +} + +status_t BufferReleaseChannel::ConsumerEndpoint::readReleaseFence( + ReleaseCallbackId& outReleaseCallbackId, sp& outReleaseFence) { + Message releaseBufferMessage; + mFlattenedBuffer.resize(releaseBufferMessage.getFlattenedSize()); + std::array controlMessageBuffer; + + iovec iov{ + .iov_base = mFlattenedBuffer.data(), + .iov_len = mFlattenedBuffer.size(), + }; + + msghdr msg{ + .msg_iov = &iov, + .msg_iovlen = 1, + .msg_control = controlMessageBuffer.data(), + .msg_controllen = controlMessageBuffer.size(), + }; + + int result; + do { + result = recvmsg(mFd, &msg, 0); + } while (result == -1 && errno == EINTR); + if (result == -1) { + if (errno == EWOULDBLOCK || errno == EAGAIN) { + return WOULD_BLOCK; + } + ALOGE("Error reading release fence from socket: error %#x (%s)", errno, strerror(errno)); + return -errno; + } + + if (msg.msg_iovlen != 1) { + ALOGE("Error reading release fence from socket: bad data length"); + return UNKNOWN_ERROR; + } + + if (msg.msg_controllen % sizeof(int) != 0) { + ALOGE("Error reading release fence from socket: bad fd length"); + return UNKNOWN_ERROR; + } + + size_t dataLen = msg.msg_iov->iov_len; + const void* data = static_cast(msg.msg_iov->iov_base); + if (!data) { + ALOGE("Error reading release fence from socket: no buffer data"); + return UNKNOWN_ERROR; + } + + size_t fdCount = 0; + const int* fdData = nullptr; + if (cmsghdr* cmsg = CMSG_FIRSTHDR(&msg)) { + fdData = reinterpret_cast(CMSG_DATA(cmsg)); + fdCount = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); + } + + if (status_t err = releaseBufferMessage.unflatten(data, dataLen, fdData, fdCount); err != OK) { + return err; + } + + outReleaseCallbackId = releaseBufferMessage.releaseCallbackId; + outReleaseFence = std::move(releaseBufferMessage.releaseFence); + + return OK; +} + +int BufferReleaseChannel::ProducerEndpoint::writeReleaseFence(const ReleaseCallbackId& callbackId, + const sp& fence) { + Message releaseBufferMessage(callbackId, fence ? fence : Fence::NO_FENCE); + mFlattenedBuffer.resize(releaseBufferMessage.getFlattenedSize()); + int flattenedFd; + { + // Make copies of needed items since flatten modifies them, and we don't + // want to send anything if there's an error during flatten. + void* flattenedBufferPtr = mFlattenedBuffer.data(); + size_t flattenedBufferSize = mFlattenedBuffer.size(); + int* flattenedFdPtr = &flattenedFd; + size_t flattenedFdCount = 1; + if (status_t err = releaseBufferMessage.flatten(flattenedBufferPtr, flattenedBufferSize, + flattenedFdPtr, flattenedFdCount); + err != OK) { + ALOGE("Failed to flatten BufferReleaseChannel message."); + return err; + } + } + + iovec iov{ + .iov_base = mFlattenedBuffer.data(), + .iov_len = mFlattenedBuffer.size(), + }; + + msghdr msg{ + .msg_iov = &iov, + .msg_iovlen = 1, + }; + + std::array controlMessageBuffer; + if (fence && fence->isValid()) { + msg.msg_control = controlMessageBuffer.data(); + msg.msg_controllen = controlMessageBuffer.size(); + + cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(int)); + memcpy(CMSG_DATA(cmsg), &flattenedFd, sizeof(int)); + } + + int result; + do { + result = sendmsg(mFd, &msg, 0); + } while (result == -1 && errno == EINTR); + if (result == -1) { + ALOGD("Error writing release fence to socket: error %#x (%s)", errno, strerror(errno)); + return -errno; + } + + return OK; +} + +status_t BufferReleaseChannel::ProducerEndpoint::readFromParcel(const android::Parcel* parcel) { + if (!parcel) return STATUS_BAD_VALUE; + SAFE_PARCEL(parcel->readUtf8FromUtf16, &mName); + SAFE_PARCEL(parcel->readUniqueFileDescriptor, &mFd); + return STATUS_OK; +} + +status_t BufferReleaseChannel::ProducerEndpoint::writeToParcel(android::Parcel* parcel) const { + if (!parcel) return STATUS_BAD_VALUE; + SAFE_PARCEL(parcel->writeUtf8AsUtf16, mName); + SAFE_PARCEL(parcel->writeUniqueFileDescriptor, mFd); + return STATUS_OK; +} + +status_t BufferReleaseChannel::open(std::string name, + std::unique_ptr& outConsumer, + std::shared_ptr& outProducer) { + outConsumer.reset(); + outProducer.reset(); + + int sockets[2]; + if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, sockets)) { + ALOGE("[%s] Failed to create socket pair. errorno=%d message='%s'", name.c_str(), errno, + strerror(errno)); + return -errno; + } + + android::base::unique_fd consumerFd(sockets[0]); + android::base::unique_fd producerFd(sockets[1]); + + // Socket buffer size. The default is typically about 128KB, which is much larger than + // we really need. So we make it smaller. It just needs to be big enough to hold + // a few dozen release fences. + const int bufferSize = 32 * 1024; + if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set consumer socket send buffer size. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set consumer socket receive buffer size. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set producer socket send buffer size. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set producer socket receive buffer size. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Configure the consumer socket to be non-blocking. + int flags = fcntl(consumerFd.get(), F_GETFL, 0); + if (flags == -1) { + ALOGE("[%s] Failed to get consumer socket flags. errno=%d message='%s'", name.c_str(), + errno, strerror(errno)); + return -errno; + } + if (fcntl(consumerFd.get(), F_SETFL, flags | O_NONBLOCK) == -1) { + ALOGE("[%s] Failed to set consumer socket to non-blocking mode. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Configure a timeout for the producer socket. + const timeval timeout{.tv_sec = 1, .tv_usec = 0}; + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeval)) == -1) { + ALOGE("[%s] Failed to set producer socket timeout. errno=%d message='%s'", name.c_str(), + errno, strerror(errno)); + return -errno; + } + + // Make the consumer read-only + if (shutdown(consumerFd.get(), SHUT_WR) == -1) { + ALOGE("[%s] Failed to shutdown writing on consumer socket. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Make the producer write-only + if (shutdown(producerFd.get(), SHUT_RD) == -1) { + ALOGE("[%s] Failed to shutdown reading on producer socket. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + outConsumer = std::make_unique(name, std::move(consumerFd)); + outProducer = std::make_shared(std::move(name), std::move(producerFd)); + return STATUS_OK; +} + +} // namespace android::gui diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 307ae3990e..adf1646b83 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -194,6 +194,12 @@ status_t layer_state_t::write(Parcel& output) const SAFE_PARCEL(output.writeFloat, currentHdrSdrRatio); SAFE_PARCEL(output.writeFloat, desiredHdrSdrRatio); SAFE_PARCEL(output.writeInt32, static_cast(cachingHint)); + + const bool hasBufferReleaseChannel = (bufferReleaseChannel != nullptr); + SAFE_PARCEL(output.writeBool, hasBufferReleaseChannel); + if (hasBufferReleaseChannel) { + SAFE_PARCEL(output.writeParcelable, *bufferReleaseChannel); + } return NO_ERROR; } @@ -339,6 +345,12 @@ status_t layer_state_t::read(const Parcel& input) SAFE_PARCEL(input.readInt32, &tmpInt32); cachingHint = static_cast(tmpInt32); + bool hasBufferReleaseChannel; + SAFE_PARCEL(input.readBool, &hasBufferReleaseChannel); + if (hasBufferReleaseChannel) { + bufferReleaseChannel = std::make_shared(); + SAFE_PARCEL(input.readParcelable, bufferReleaseChannel.get()); + } return NO_ERROR; } @@ -718,6 +730,10 @@ void layer_state_t::merge(const layer_state_t& other) { if (other.what & eFlushJankData) { what |= eFlushJankData; } + if (other.what & eBufferReleaseChannelChanged) { + what |= eBufferReleaseChannelChanged; + bufferReleaseChannel = other.bufferReleaseChannel; + } if ((other.what & what) != other.what) { ALOGE("Unmerged SurfaceComposer Transaction properties. LayerState::merge needs updating? " "other.what=0x%" PRIX64 " what=0x%" PRIX64 " unmerged flags=0x%" PRIX64, @@ -797,6 +813,7 @@ uint64_t layer_state_t::diff(const layer_state_t& other) const { CHECK_DIFF(diff, eColorChanged, other, color.rgb); CHECK_DIFF(diff, eColorSpaceAgnosticChanged, other, colorSpaceAgnostic); CHECK_DIFF(diff, eDimmingEnabledChanged, other, dimmingEnabled); + if (other.what & eBufferReleaseChannelChanged) diff |= eBufferReleaseChannelChanged; return diff; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 2821b510c3..5c87772587 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2394,6 +2394,22 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDropI return *this; } +SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBufferReleaseChannel( + const sp& sc, + const std::shared_ptr& channel) { + layer_state_t* s = getLayerState(sc); + if (!s) { + mStatus = BAD_INDEX; + return *this; + } + + s->what |= layer_state_t::eBufferReleaseChannelChanged; + s->bufferReleaseChannel = channel; + + registerSurfaceControlForCallback(sc); + return *this; +} + // --------------------------------------------------------------------------- DisplayState& SurfaceComposerClient::Transaction::getDisplayState(const sp& token) { diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 0e1a505c69..e9a350c301 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -19,7 +19,7 @@ #include #include - +#include #include #include @@ -28,7 +28,6 @@ #include #include -#include #include #include @@ -131,6 +130,8 @@ public: virtual ~BLASTBufferQueue(); + void onFirstRef() override; + private: friend class BLASTBufferQueueHelper; friend class BBQBufferQueueProducer; @@ -170,11 +171,23 @@ private: // BufferQueue internally allows 1 more than // the max to be acquired - int32_t mMaxAcquiredBuffers = 1; - + int32_t mMaxAcquiredBuffers GUARDED_BY(mMutex) = 1; + int32_t mMaxDequeuedBuffers GUARDED_BY(mMutex) = 1; + static constexpr int32_t kMaxBufferCount = BufferQueueDefs::NUM_BUFFER_SLOTS; + + // mNumDequeued is an atomic instead of being guarded by mMutex so that it can be incremented in + // onFrameDequeued while avoiding lock contention. updateDequeueShouldBlockLocked is not called + // after mNumDequeued is incremented for this reason. This means mDequeueShouldBlock may be + // temporarily false when it should be true. This can happen if multiple threads are dequeuing + // buffers or if dequeueBuffers is called multiple times in a row without queuing a buffer in + // between. As mDequeueShouldBlock is only used for optimization, this is OK. + std::atomic_int mNumDequeued; int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0; int32_t mNumAcquired GUARDED_BY(mMutex) = 0; + bool mAsyncMode GUARDED_BY(mMutex) = false; + bool mSharedBufferMode GUARDED_BY(mMutex) = false; + // A value used to identify if a producer has been changed for the same SurfaceControl. // This is needed to know when the frame number has been reset to make sure we don't // latch stale buffers and that we don't wait on barriers from an old producer. @@ -300,6 +313,41 @@ private: std::function mTransactionHangCallback; std::unordered_set mSyncedFrameNumbers GUARDED_BY(mMutex); + + class BufferReleaseReader { + public: + BufferReleaseReader(std::unique_ptr); + + BufferReleaseReader(const BufferReleaseReader&) = delete; + BufferReleaseReader& operator=(const BufferReleaseReader&) = delete; + + // Block until we can release a buffer. + // + // Returns: + // * OK if a ReleaseCallbackId and Fence were successfully read. + // * TIMED_OUT if the specified timeout was reached. + // * WOULD_BLOCK if the blocking read was interrupted by interruptBlockingRead. + // * UNKNOWN_ERROR if something went wrong. + status_t readBlocking(ReleaseCallbackId&, sp&, std::chrono::milliseconds timeout); + + status_t readNonBlocking(ReleaseCallbackId&, sp&); + + void interruptBlockingRead(); + + private: + std::mutex mMutex; + std::unique_ptr mEndpoint GUARDED_BY(mMutex); + android::base::unique_fd mEpollFd; + android::base::unique_fd mEventFd; + }; + + // BufferReleaseChannel is used to communicate buffer releases from SurfaceFlinger to + // the client. See BBQBufferQueueProducer::dequeueBuffer for details. + std::optional mBufferReleaseReader; + std::shared_ptr mBufferReleaseProducer; + + std::atomic_bool mDequeueShouldBlock{false}; + void updateDequeueShouldBlockLocked() REQUIRES(mMutex); }; } // namespace android diff --git a/libs/gui/include/gui/BufferReleaseChannel.h b/libs/gui/include/gui/BufferReleaseChannel.h new file mode 100644 index 0000000000..cb0b261240 --- /dev/null +++ b/libs/gui/include/gui/BufferReleaseChannel.h @@ -0,0 +1,127 @@ +/* + * Copyright (C) 2024 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. + */ + +#pragma once + +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace android::gui { + +/** + * IPC wrapper to pass release fences from SurfaceFlinger to apps via a local unix domain socket. + */ +class BufferReleaseChannel { +private: + class Endpoint { + public: + Endpoint(std::string name, android::base::unique_fd fd) + : mName(std::move(name)), mFd(std::move(fd)) {} + Endpoint() {} + + Endpoint(Endpoint&&) noexcept = default; + Endpoint& operator=(Endpoint&&) noexcept = default; + + Endpoint(const Endpoint&) = delete; + void operator=(const Endpoint&) = delete; + + const android::base::unique_fd& getFd() const { return mFd; } + + protected: + std::string mName; + android::base::unique_fd mFd; + }; + +public: + class ConsumerEndpoint : public Endpoint { + public: + ConsumerEndpoint(std::string name, android::base::unique_fd fd) + : Endpoint(std::move(name), std::move(fd)) {} + + /** + * Reads a release fence from the BufferReleaseChannel. + * + * Returns OK on success. + * Returns WOULD_BLOCK if there is no fence present. + * Other errors probably indicate that the channel is broken. + */ + status_t readReleaseFence(ReleaseCallbackId& outReleaseCallbackId, + sp& outReleaseFence); + + private: + std::vector mFlattenedBuffer; + }; + + class ProducerEndpoint : public Endpoint, public Parcelable { + public: + ProducerEndpoint(std::string name, android::base::unique_fd fd) + : Endpoint(std::move(name), std::move(fd)) {} + ProducerEndpoint() {} + + status_t readFromParcel(const android::Parcel* parcel) override; + status_t writeToParcel(android::Parcel* parcel) const override; + + status_t writeReleaseFence(const ReleaseCallbackId&, const sp& releaseFence); + + private: + std::vector mFlattenedBuffer; + }; + + /** + * Create two endpoints that make up the BufferReleaseChannel. + * + * Return OK on success. + */ + static status_t open(const std::string name, std::unique_ptr& outConsumer, + std::shared_ptr& outProducer); + + struct Message : public Flattenable { + ReleaseCallbackId releaseCallbackId; + sp releaseFence = Fence::NO_FENCE; + + Message() = default; + Message(ReleaseCallbackId releaseCallbackId, sp releaseFence) + : releaseCallbackId(releaseCallbackId), releaseFence(std::move(releaseFence)) {} + + // Flattenable protocol + size_t getFlattenedSize() const; + + size_t getFdCount() const { return releaseFence->getFdCount(); } + + status_t flatten(void*& buffer, size_t& size, int*& fds, size_t& count) const; + + status_t unflatten(void const*& buffer, size_t& size, int const*& fds, size_t& count); + + private: + size_t getPodSize() const; + }; +}; + +} // namespace android::gui diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 3fb1894585..d41994589f 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -220,6 +221,7 @@ struct layer_state_t { eDropInputModeChanged = 0x8000'00000000, eExtendedRangeBrightnessChanged = 0x10000'00000000, eEdgeExtensionChanged = 0x20000'00000000, + eBufferReleaseChannelChanged = 0x40000'00000000, }; layer_state_t(); @@ -412,6 +414,8 @@ struct layer_state_t { TrustedPresentationThresholds trustedPresentationThresholds; TrustedPresentationListener trustedPresentationListener; + + std::shared_ptr bufferReleaseChannel; }; class ComposerState { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 95574ee30e..8c1d6443c5 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -45,6 +45,7 @@ #include #include +#include #include #include #include @@ -763,6 +764,10 @@ public: const Rect& destinationFrame); Transaction& setDropInputMode(const sp& sc, gui::DropInputMode mode); + Transaction& setBufferReleaseChannel( + const sp& sc, + const std::shared_ptr& channel); + status_t setDisplaySurface(const sp& token, const sp& bufferProducer); diff --git a/libs/gui/libgui_flags.aconfig b/libs/gui/libgui_flags.aconfig index 74d806ec79..e8f2b76b9b 100644 --- a/libs/gui/libgui_flags.aconfig +++ b/libs/gui/libgui_flags.aconfig @@ -51,3 +51,11 @@ flag { bug: "322036393" is_fixed_read_only: true } # edge_extension_shader + +flag { + name: "buffer_release_channel" + namespace: "window_surfaces" + description: "Enable BufferReleaseChannel to optimize buffer releases" + bug: "294133380" + is_fixed_read_only: true +} # buffer_release_channel diff --git a/libs/gui/tests/Android.bp b/libs/gui/tests/Android.bp index ea8acbbb72..daaddfbc15 100644 --- a/libs/gui/tests/Android.bp +++ b/libs/gui/tests/Android.bp @@ -32,6 +32,7 @@ cc_test { "BLASTBufferQueue_test.cpp", "BufferItemConsumer_test.cpp", "BufferQueue_test.cpp", + "BufferReleaseChannel_test.cpp", "Choreographer_test.cpp", "CompositorTiming_test.cpp", "CpuConsumer_test.cpp", diff --git a/libs/gui/tests/BufferReleaseChannel_test.cpp b/libs/gui/tests/BufferReleaseChannel_test.cpp new file mode 100644 index 0000000000..f3e962c192 --- /dev/null +++ b/libs/gui/tests/BufferReleaseChannel_test.cpp @@ -0,0 +1,121 @@ +/* + * Copyright (C) 2024 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. + */ + +#include +#include + +#include +#include + +using namespace std::string_literals; +using android::gui::BufferReleaseChannel; + +namespace android { + +namespace { + +// Helper function to check if two file descriptors point to the same file. +bool is_same_file(int fd1, int fd2) { + struct stat stat1; + if (fstat(fd1, &stat1) != 0) { + return false; + } + struct stat stat2; + if (fstat(fd2, &stat2) != 0) { + return false; + } + return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino); +} + +} // namespace + +TEST(BufferReleaseChannelTest, MessageFlattenable) { + ReleaseCallbackId releaseCallbackId{1, 2}; + sp releaseFence = sp::make(memfd_create("fake-fence-fd", 0)); + + std::vector dataBuffer; + std::vector fdBuffer; + + // Verify that we can flatten a message + { + BufferReleaseChannel::Message message{releaseCallbackId, releaseFence}; + + dataBuffer.resize(message.getFlattenedSize()); + void* dataPtr = dataBuffer.data(); + size_t dataSize = dataBuffer.size(); + + fdBuffer.resize(message.getFdCount()); + int* fdPtr = fdBuffer.data(); + size_t fdSize = fdBuffer.size(); + + ASSERT_EQ(OK, message.flatten(dataPtr, dataSize, fdPtr, fdSize)); + + // Fence's unique_fd uses fdsan to check ownership of the file descriptor. Normally the file + // descriptor is passed through the Unix socket and duplicated (and sent to another process) + // so there's no problem with duplicate file descriptor ownership. For this unit test, we + // need to set up a duplicate file descriptor to avoid crashing due to duplicate ownership. + ASSERT_EQ(releaseFence->get(), fdBuffer[0]); + fdBuffer[0] = message.releaseFence->dup(); + } + + // Verify that we can unflatten a message + { + BufferReleaseChannel::Message message; + + const void* dataPtr = dataBuffer.data(); + size_t dataSize = dataBuffer.size(); + + const int* fdPtr = fdBuffer.data(); + size_t fdSize = fdBuffer.size(); + + ASSERT_EQ(OK, message.unflatten(dataPtr, dataSize, fdPtr, fdSize)); + ASSERT_EQ(releaseCallbackId, message.releaseCallbackId); + ASSERT_TRUE(is_same_file(releaseFence->get(), message.releaseFence->get())); + } +} + +// Verify that the BufferReleaseChannel consume returns WOULD_BLOCK when there's no message +// available. +TEST(BufferReleaseChannelTest, ConsumerEndpointIsNonBlocking) { + std::unique_ptr consumer; + std::shared_ptr producer; + ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); + + ReleaseCallbackId releaseCallbackId; + sp releaseFence; + ASSERT_EQ(WOULD_BLOCK, consumer->readReleaseFence(releaseCallbackId, releaseFence)); +} + +// Verify that we can write a message to the BufferReleaseChannel producer and read that message +// using the BufferReleaseChannel consumer. +TEST(BufferReleaseChannelTest, ProduceAndConsume) { + std::unique_ptr consumer; + std::shared_ptr producer; + ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); + + ReleaseCallbackId producerReleaseCallbackId{1, 2}; + sp producerReleaseFence = sp::make(memfd_create("fake-fence-fd", 0)); + ASSERT_EQ(OK, producer->writeReleaseFence(producerReleaseCallbackId, producerReleaseFence)); + + ReleaseCallbackId consumerReleaseCallbackId; + sp consumerReleaseFence; + ASSERT_EQ(OK, consumer->readReleaseFence(consumerReleaseCallbackId, consumerReleaseFence)); + + ASSERT_EQ(producerReleaseCallbackId, consumerReleaseCallbackId); + ASSERT_TRUE(is_same_file(producerReleaseFence->get(), consumerReleaseFence->get())); +} + +} // namespace android diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index a6d2b1529a..082d8aa424 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -2783,11 +2783,14 @@ void Layer::callReleaseBufferCallback(const sp& l return; } SFTRACE_FORMAT_INSTANT("callReleaseBufferCallback %s - %" PRIu64, getDebugName(), framenumber); + ReleaseCallbackId callbackId{buffer->getId(), framenumber}; + const sp& fence = releaseFence ? releaseFence : Fence::NO_FENCE; uint32_t currentMaxAcquiredBufferCount = mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid); - listener->onReleaseBuffer({buffer->getId(), framenumber}, - releaseFence ? releaseFence : Fence::NO_FENCE, - currentMaxAcquiredBufferCount); + listener->onReleaseBuffer(callbackId, fence, currentMaxAcquiredBufferCount); + if (mBufferReleaseChannel) { + mBufferReleaseChannel->writeReleaseFence(callbackId, fence); + } } sp Layer::findCallbackHandle() { @@ -2905,6 +2908,7 @@ void Layer::onLayerDisplayed(ftl::SharedFuture futureFenceResult, void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { for (const auto& handle : mDrawingState.callbackHandles) { + handle->bufferReleaseChannel = mBufferReleaseChannel; handle->transformHint = mTransformHint; handle->dequeueReadyTime = dequeueReadyTime; handle->currentMaxAcquiredBufferCount = @@ -4376,6 +4380,11 @@ bool Layer::setTrustedPresentationInfo(TrustedPresentationThresholds const& thre return haveTrustedPresentationListener; } +void Layer::setBufferReleaseChannel( + const std::shared_ptr& channel) { + mBufferReleaseChannel = channel; +} + void Layer::updateLastLatchTime(nsecs_t latchTime) { mLastLatchTime = latchTime; } diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 52f169ebee..d4707388b2 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -550,6 +550,7 @@ public: }; BufferInfo mBufferInfo; + std::shared_ptr mBufferReleaseChannel; // implements compositionengine::LayerFE const compositionengine::LayerFECompositionState* getCompositionState() const; @@ -813,6 +814,8 @@ public: bool setTrustedPresentationInfo(TrustedPresentationThresholds const& thresholds, TrustedPresentationListener const& listener); + void setBufferReleaseChannel( + const std::shared_ptr& channel); // Creates a new handle each time, so we only expect // this to be called once. diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 1d3573017d..beb05b9780 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5951,6 +5951,10 @@ uint32_t SurfaceFlinger::updateLayerCallbacksAndStats(const FrameTimelineInfo& f } } + if (what & layer_state_t::eBufferReleaseChannelChanged) { + layer->setBufferReleaseChannel(s.bufferReleaseChannel); + } + const auto& requestedLayerState = mLayerLifecycleManager.getLayerFromId(layer->getSequence()); bool willPresentCurrentTransaction = requestedLayerState && (requestedLayerState->hasReadyFrame() || diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp index 881bf35b58..9ea0f5f4ca 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.cpp +++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp @@ -149,6 +149,12 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle->transformHint, handle->currentMaxAcquiredBufferCount, eventStats, handle->previousReleaseCallbackId); + if (handle->bufferReleaseChannel && + handle->previousReleaseCallbackId != ReleaseCallbackId::INVALID_ID) { + mBufferReleases.emplace_back(handle->bufferReleaseChannel, + handle->previousReleaseCallbackId, + handle->previousReleaseFence); + } } return NO_ERROR; } @@ -158,6 +164,11 @@ void TransactionCallbackInvoker::addPresentFence(sp presentFence) { } void TransactionCallbackInvoker::sendCallbacks(bool onCommitOnly) { + for (const auto& bufferRelease : mBufferReleases) { + bufferRelease.channel->writeReleaseFence(bufferRelease.callbackId, bufferRelease.fence); + } + mBufferReleases.clear(); + // For each listener auto completedTransactionsItr = mCompletedTransactions.begin(); ftl::SmallVector listenerStatsToSend; diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index 7853a9f359..f74f9644b6 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -59,6 +60,7 @@ public: uint64_t frameNumber = 0; uint64_t previousFrameNumber = 0; ReleaseCallbackId previousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; + std::shared_ptr bufferReleaseChannel; }; class TransactionCallbackInvoker { @@ -86,6 +88,13 @@ private: std::unordered_map, std::deque, IListenerHash> mCompletedTransactions; + struct BufferRelease { + std::shared_ptr channel; + ReleaseCallbackId callbackId; + sp fence; + }; + std::vector mBufferReleases; + sp mPresentFence; }; -- cgit v1.2.3-59-g8ed1b From f65da8a9d7b6869dc2ad5cdd3b48763b8e18ac46 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Wed, 24 Jul 2024 17:11:19 +0000 Subject: Fix DisplayState sanitization. Bug: 347307756 Flag: EXEMPT bugfix Test: CredentialsTest Change-Id: I7d227dedaa13a1a31ebf9ace073c792287f35305 --- libs/gui/ISurfaceComposer.cpp | 2 +- libs/gui/SurfaceComposerClient.cpp | 3 +- libs/gui/include/gui/ISurfaceComposer.h | 2 +- libs/gui/tests/Surface_test.cpp | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 4 +- services/surfaceflinger/SurfaceFlinger.h | 2 +- services/surfaceflinger/tests/Credentials_test.cpp | 53 +++++++++++++++++++++- .../tests/unittests/TestableSurfaceFlinger.h | 2 +- 8 files changed, 61 insertions(+), 9 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index ff6b558d41..269936858a 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -62,7 +62,7 @@ public: status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - const Vector& displays, uint32_t flags, const sp& applyToken, + Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands commands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, uint64_t transactionId, diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index af91bb3ae2..5db539497c 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1059,7 +1059,8 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { uncacheBuffer.token = BufferCache::getInstance().getToken(); uncacheBuffer.id = cacheId; Vector composerStates; - status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, {}, + Vector displayStates; + status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, displayStates, ISurfaceComposer::eOneWay, Transaction::getDefaultApplyToken(), {}, systemTime(), true, {uncacheBuffer}, false, {}, generateId(), {}); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index eb4a802c17..1ecc216dff 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -112,7 +112,7 @@ public: /* open/close transactions. requires ACCESS_SURFACE_FLINGER permission */ virtual status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - const Vector& displays, uint32_t flags, const sp& applyToken, + Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffer, bool hasListenerCallbacks, const std::vector& listenerCallbacks, diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 43cd0f8a7f..5e91088378 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -636,7 +636,7 @@ public: status_t setTransactionState( const FrameTimelineInfo& /*frameTimelineInfo*/, Vector& /*state*/, - const Vector& /*displays*/, uint32_t /*flags*/, + Vector& /*displays*/, uint32_t /*flags*/, const sp& /*applyToken*/, InputWindowCommands /*inputWindowCommands*/, int64_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/, const std::vector& /*cachedBuffer*/, bool /*hasListenerCallbacks*/, diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 596ec12d9e..f9262a0a9c 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5167,7 +5167,7 @@ bool SurfaceFlinger::shouldLatchUnsignaled(const layer_state_t& state, size_t nu status_t SurfaceFlinger::setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& states, - const Vector& displays, uint32_t flags, const sp& applyToken, + Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, uint64_t transactionId, @@ -5182,7 +5182,7 @@ status_t SurfaceFlinger::setTransactionState( composerState.state.sanitize(permissions); } - for (DisplayState display : displays) { + for (DisplayState& display : displays) { display.sanitize(permissions); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index ee541c4364..7762bbea9f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -559,7 +559,7 @@ private: sp getPhysicalDisplayToken(PhysicalDisplayId displayId) const; status_t setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& state, - const Vector& displays, uint32_t flags, const sp& applyToken, + Vector& displays, uint32_t flags, const sp& applyToken, InputWindowCommands inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, const std::vector& listenerCallbacks, diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index ebe11fb0f3..d355e720d1 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -276,7 +277,7 @@ TEST_F(CredentialsTest, CreateDisplayTest) { TEST_F(CredentialsTest, CaptureLayersTest) { setupBackgroundSurface(); sp outBuffer; - std::function condition = [=]() { + std::function condition = [=, this]() { LayerCaptureArgs captureArgs; captureArgs.layerHandle = mBGSurfaceControl->getHandle(); captureArgs.sourceCrop = {0, 0, 1, 1}; @@ -396,6 +397,56 @@ TEST_F(CredentialsTest, TransactionPermissionTest) { } } +TEST_F(CredentialsTest, DisplayTransactionPermissionTest) { + const auto display = getFirstDisplayToken(); + + ui::DisplayState displayState; + ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); + const ui::Rotation initialOrientation = displayState.orientation; + + // Set display orientation from an untrusted process. This should fail silently. + { + UIDFaker f{AID_BIN}; + Transaction transaction; + Rect layerStackRect; + Rect displayRect; + transaction.setDisplayProjection(display, initialOrientation + ui::ROTATION_90, + layerStackRect, displayRect); + transaction.apply(/*synchronous=*/true); + } + + // Verify that the display orientation did not change. + ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); + ASSERT_EQ(initialOrientation, displayState.orientation); + + // Set display orientation from a trusted process. + { + UIDFaker f{AID_SYSTEM}; + Transaction transaction; + Rect layerStackRect; + Rect displayRect; + transaction.setDisplayProjection(display, initialOrientation + ui::ROTATION_90, + layerStackRect, displayRect); + transaction.apply(/*synchronous=*/true); + } + + // Verify that the display orientation did change. + ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); + ASSERT_EQ(initialOrientation + ui::ROTATION_90, displayState.orientation); + + // Reset orientation + { + UIDFaker f{AID_SYSTEM}; + Transaction transaction; + Rect layerStackRect; + Rect displayRect; + transaction.setDisplayProjection(display, initialOrientation, layerStackRect, displayRect); + transaction.apply(/*synchronous=*/true); + } + ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(display, &displayState)); + ASSERT_EQ(initialOrientation, displayState.orientation); +} + } // namespace android // TODO(b/129481165): remove the #pragma below and fix conversion issues diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 007383b3d9..7d1ec253c4 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -528,7 +528,7 @@ public: auto setTransactionState( const FrameTimelineInfo& frameTimelineInfo, Vector& states, - const Vector& displays, uint32_t flags, const sp& applyToken, + Vector& displays, uint32_t flags, const sp& applyToken, const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime, bool isAutoTimestamp, const std::vector& uncacheBuffers, bool hasListenerCallbacks, std::vector& listenerCallbacks, -- cgit v1.2.3-59-g8ed1b From 4b9507d5a4586ecd2e14db88eb8674e204387ddd Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Thu, 25 Jul 2024 09:55:52 -0500 Subject: Revert "Optimize BLAST buffer releases via Unix sockets" Reverting due to b/355260320 Change-Id: I8a32f73b6805d3f2bceb2948925be6a9baaa3015 --- libs/gui/Android.bp | 1 - libs/gui/BLASTBufferQueue.cpp | 343 ++----------------- libs/gui/BufferReleaseChannel.cpp | 364 --------------------- libs/gui/LayerState.cpp | 17 - libs/gui/SurfaceComposerClient.cpp | 16 - libs/gui/include/gui/BLASTBufferQueue.h | 56 +--- libs/gui/include/gui/BufferReleaseChannel.h | 127 ------- libs/gui/include/gui/LayerState.h | 4 - libs/gui/include/gui/SurfaceComposerClient.h | 5 - libs/gui/tests/Android.bp | 1 - libs/gui/tests/BufferReleaseChannel_test.cpp | 121 ------- services/surfaceflinger/Layer.cpp | 15 +- services/surfaceflinger/Layer.h | 3 - services/surfaceflinger/SurfaceFlinger.cpp | 4 - .../surfaceflinger/TransactionCallbackInvoker.cpp | 11 - .../surfaceflinger/TransactionCallbackInvoker.h | 9 - 16 files changed, 26 insertions(+), 1071 deletions(-) delete mode 100644 libs/gui/BufferReleaseChannel.cpp delete mode 100644 libs/gui/include/gui/BufferReleaseChannel.h delete mode 100644 libs/gui/tests/BufferReleaseChannel_test.cpp (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 1243b214d3..51d2e5305a 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -255,7 +255,6 @@ filegroup { "BitTube.cpp", "BLASTBufferQueue.cpp", "BufferItemConsumer.cpp", - "BufferReleaseChannel.cpp", "Choreographer.cpp", "CompositorTiming.cpp", "ConsumerBase.cpp", diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 767f3e8c16..739c3c2a41 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -38,17 +38,13 @@ #include #include -#include #include -#include -#include #include #include using namespace com::android::graphics::libgui; using namespace std::chrono_literals; -using android::base::unique_fd; namespace { inline const char* boolToString(bool b) { @@ -183,6 +179,8 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati // explicitly so that dequeueBuffer will block mProducer->setDequeueTimeout(std::numeric_limits::max()); + // safe default, most producers are expected to override this + mProducer->setMaxDequeuedBufferCount(2); mBufferItemConsumer = new BLASTBufferItemConsumer(mConsumer, GraphicBuffer::USAGE_HW_COMPOSER | GraphicBuffer::USAGE_HW_TEXTURE, @@ -212,12 +210,6 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati }, this); -#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(BUFFER_RELEASE_CHANNEL) - std::unique_ptr bufferReleaseConsumer; - gui::BufferReleaseChannel::open(mName, bufferReleaseConsumer, mBufferReleaseProducer); - mBufferReleaseReader.emplace(std::move(bufferReleaseConsumer)); -#endif - BQA_LOGV("BLASTBufferQueue created"); } @@ -267,9 +259,6 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, if (surfaceControlChanged) { t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure, layer_state_t::eEnableBackpressure); - if (mBufferReleaseProducer) { - t.setBufferReleaseChannel(mSurfaceControl, mBufferReleaseProducer); - } applyTransaction = true; } mTransformHint = mSurfaceControl->getTransformHint(); @@ -450,21 +439,6 @@ void BLASTBufferQueue::releaseBufferCallback( BBQ_TRACE(); releaseBufferCallbackLocked(id, releaseFence, currentMaxAcquiredBufferCount, false /* fakeRelease */); - if (!mBufferReleaseReader) { - return; - } - // Drain the buffer release channel socket - while (true) { - ReleaseCallbackId releaseCallbackId; - sp releaseFence; - if (status_t status = - mBufferReleaseReader->readNonBlocking(releaseCallbackId, releaseFence); - status != OK) { - break; - } - releaseBufferCallbackLocked(releaseCallbackId, releaseFence, std::nullopt, - false /* fakeRelease */); - } } void BLASTBufferQueue::releaseBufferCallbackLocked( @@ -521,13 +495,11 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, const sp& releaseFence) { auto it = mSubmitted.find(callbackId); if (it == mSubmitted.end()) { + BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %s", + callbackId.to_string().c_str()); return; } mNumAcquired--; - updateDequeueShouldBlockLocked(); - if (mBufferReleaseReader) { - mBufferReleaseReader->interruptBlockingRead(); - } BBQ_TRACE("frame=%" PRIu64, callbackId.framenumber); BQA_LOGV("released %s", callbackId.to_string().c_str()); mBufferItemConsumer->releaseBuffer(it->second, releaseFence); @@ -592,7 +564,6 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( auto buffer = bufferItem.mGraphicBuffer; mNumFrameAvailable--; - updateDequeueShouldBlockLocked(); BBQ_TRACE("frame=%" PRIu64, bufferItem.mFrameNumber); if (buffer == nullptr) { @@ -611,7 +582,6 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( } mNumAcquired++; - updateDequeueShouldBlockLocked(); mLastAcquiredFrameNumber = bufferItem.mFrameNumber; ReleaseCallbackId releaseCallbackId(buffer->getId(), mLastAcquiredFrameNumber); mSubmitted[releaseCallbackId] = bufferItem; @@ -738,7 +708,6 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { return; } mNumFrameAvailable--; - updateDequeueShouldBlockLocked(); mBufferItemConsumer->releaseBuffer(bufferItem, bufferItem.mFence); } @@ -792,9 +761,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { } // add to shadow queue - mNumDequeued--; mNumFrameAvailable++; - updateDequeueShouldBlockLocked(); if (waitForTransactionCallback && mNumFrameAvailable >= 2) { acquireAndReleaseBuffer(); } @@ -845,24 +812,11 @@ void BLASTBufferQueue::onFrameReplaced(const BufferItem& item) { void BLASTBufferQueue::onFrameDequeued(const uint64_t bufferId) { std::lock_guard _lock{mTimestampMutex}; mDequeueTimestamps[bufferId] = systemTime(); - mNumDequeued++; -} +}; void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { - { - std::lock_guard _lock{mTimestampMutex}; - mDequeueTimestamps.erase(bufferId); - } - - { - std::lock_guard lock{mMutex}; - mNumDequeued--; - updateDequeueShouldBlockLocked(); - } - - if (mBufferReleaseReader) { - mBufferReleaseReader->interruptBlockingRead(); - } + std::lock_guard _lock{mTimestampMutex}; + mDequeueTimestamps.erase(bufferId); }; bool BLASTBufferQueue::syncNextTransaction( @@ -934,22 +888,6 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { return mSize != bufferSize; } -void BLASTBufferQueue::updateDequeueShouldBlockLocked() { - int32_t buffersInUse = mNumDequeued + mNumFrameAvailable + mNumAcquired; - int32_t maxBufferCount = std::min(mMaxAcquiredBuffers + mMaxDequeuedBuffers, kMaxBufferCount); - bool bufferAvailable = buffersInUse < maxBufferCount; - // BLASTBufferQueueProducer should block until a buffer is released if - // (1) There are no free buffers available. - // (2) We're not in async mode. In async mode, BufferQueueProducer::dequeueBuffer returns - // WOULD_BLOCK instead of blocking when there are no free buffers. - // (3) We're not in shared buffer mode. In shared buffer mode, both the producer and consumer - // can access the same buffer simultaneously. BufferQueueProducer::dequeueBuffer returns - // the shared buffer immediately instead of blocking. - mDequeueShouldBlock = !(bufferAvailable || mAsyncMode || mSharedBufferMode); - ATRACE_INT("Dequeued", mNumDequeued); - ATRACE_INT("DequeueShouldBlock", mDequeueShouldBlock); -} - class BBQSurface : public Surface { private: std::mutex mMutex; @@ -1178,64 +1116,24 @@ public: producerControlledByApp, output); } - status_t disconnect(int api, DisconnectMode mode) override { - if (status_t status = BufferQueueProducer::disconnect(api, mode); status != OK) { - return status; - } - - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq) { - return OK; - } - - { - std::lock_guard lock{bbq->mMutex}; - bbq->mNumDequeued = 0; - bbq->mNumFrameAvailable = 0; - bbq->mNumAcquired = 0; - bbq->mSubmitted.clear(); - bbq->updateDequeueShouldBlockLocked(); - } - - if (bbq->mBufferReleaseReader) { - bbq->mBufferReleaseReader->interruptBlockingRead(); - } - - return OK; - } - // We want to resize the frame history when changing the size of the buffer queue status_t setMaxDequeuedBufferCount(int maxDequeuedBufferCount) override { int maxBufferCount; status_t status = BufferQueueProducer::setMaxDequeuedBufferCount(maxDequeuedBufferCount, &maxBufferCount); - if (status != OK) { - return status; - } - - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq) { - return OK; - } - - { - std::lock_guard lock{bbq->mMutex}; - bbq->mMaxDequeuedBuffers = maxDequeuedBufferCount; - bbq->updateDequeueShouldBlockLocked(); - } - - if (bbq->mBufferReleaseReader) { - bbq->mBufferReleaseReader->interruptBlockingRead(); - } - - size_t newFrameHistorySize = maxBufferCount + 2; // +2 because triple buffer rendering - // optimize away resizing the frame history unless it will grow - if (newFrameHistorySize > FrameEventHistory::INITIAL_MAX_FRAME_HISTORY) { - ALOGV("increasing frame history size to %zu", newFrameHistorySize); - bbq->resizeFrameEventHistory(newFrameHistorySize); + // if we can't determine the max buffer count, then just skip growing the history size + if (status == OK) { + size_t newFrameHistorySize = maxBufferCount + 2; // +2 because triple buffer rendering + // optimize away resizing the frame history unless it will grow + if (newFrameHistorySize > FrameEventHistory::INITIAL_MAX_FRAME_HISTORY) { + sp bbq = mBLASTBufferQueue.promote(); + if (bbq != nullptr) { + ALOGV("increasing frame history size to %zu", newFrameHistorySize); + bbq->resizeFrameEventHistory(newFrameHistorySize); + } + } } - - return OK; + return status; } int query(int what, int* value) override { @@ -1246,131 +1144,6 @@ public: return BufferQueueProducer::query(what, value); } - status_t setAsyncMode(bool asyncMode) override { - if (status_t status = BufferQueueProducer::setAsyncMode(asyncMode); status != NO_ERROR) { - return status; - } - - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq) { - return NO_ERROR; - } - - { - std::lock_guard lock{bbq->mMutex}; - bbq->mAsyncMode = asyncMode; - bbq->updateDequeueShouldBlockLocked(); - } - - if (bbq->mBufferReleaseReader) { - bbq->mBufferReleaseReader->interruptBlockingRead(); - } - return NO_ERROR; - } - - status_t setSharedBufferMode(bool sharedBufferMode) override { - if (status_t status = BufferQueueProducer::setSharedBufferMode(sharedBufferMode); - status != NO_ERROR) { - return status; - } - - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq) { - return NO_ERROR; - } - - { - std::lock_guard lock{bbq->mMutex}; - bbq->mSharedBufferMode = sharedBufferMode; - bbq->updateDequeueShouldBlockLocked(); - } - - if (bbq->mBufferReleaseReader) { - bbq->mBufferReleaseReader->interruptBlockingRead(); - } - return NO_ERROR; - } - - status_t detachBuffer(int slot) override { - if (status_t status = BufferQueueProducer::detachBuffer(slot); status != NO_ERROR) { - return status; - } - - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq) { - return NO_ERROR; - } - - { - std::lock_guard lock{bbq->mMutex}; - bbq->mNumDequeued--; - bbq->updateDequeueShouldBlockLocked(); - } - - if (bbq->mBufferReleaseReader) { - bbq->mBufferReleaseReader->interruptBlockingRead(); - } - return NO_ERROR; - } - - // Override dequeueBuffer to block if there are no free buffers. - // - // Buffer releases are communicated via the BufferReleaseChannel. When dequeueBuffer determines - // a free buffer is not available, it blocks on an epoll file descriptor. Epoll is configured to - // detect messages on the BufferReleaseChannel's socket and an eventfd. The eventfd is signaled - // whenever an event other than a buffer release occurs that may change the number of free - // buffers. dequeueBuffer uses epoll in a similar manner as a condition variable by testing for - // the availability of a free buffer in a loop, breaking the loop once a free buffer is - // available. - // - // This is an optimization implemented to reduce thread scheduling delays in the previously - // existing binder release callback. The binder buffer release callback is still used and there - // are no guarantees around order between buffer releases via binder and the - // BufferReleaseChannel. If we attempt to a release a buffer here that has already been released - // via binder, the release is ignored. - status_t dequeueBuffer(int* outSlot, sp* outFence, uint32_t width, uint32_t height, - PixelFormat format, uint64_t usage, uint64_t* outBufferAge, - FrameEventHistoryDelta* outTimestamps) { - sp bbq = mBLASTBufferQueue.promote(); - if (!bbq || !bbq->mBufferReleaseReader) { - return BufferQueueProducer::dequeueBuffer(outSlot, outFence, width, height, format, - usage, outBufferAge, outTimestamps); - } - - if (bbq->mDequeueShouldBlock) { - ATRACE_FORMAT("waiting for free buffer"); - auto maxWaitTime = std::chrono::steady_clock::now() + 1s; - do { - auto timeout = std::chrono::duration_cast( - maxWaitTime - std::chrono::steady_clock::now()); - if (timeout <= 0ms) { - break; - } - - ReleaseCallbackId releaseCallbackId; - sp releaseFence; - status_t status = bbq->mBufferReleaseReader->readBlocking(releaseCallbackId, - releaseFence, timeout); - if (status == WOULD_BLOCK) { - // readBlocking was interrupted. The loop will test if we have a free buffer. - continue; - } - - if (status != OK) { - // An error occurred or readBlocking timed out. - break; - } - - std::lock_guard lock{bbq->mMutex}; - bbq->releaseBufferCallbackLocked(releaseCallbackId, releaseFence, std::nullopt, - false); - } while (bbq->mDequeueShouldBlock); - } - - return BufferQueueProducer::dequeueBuffer(outSlot, outFence, width, height, format, usage, - outBufferAge, outTimestamps); - } - private: const wp mBLASTBufferQueue; }; @@ -1400,16 +1173,6 @@ void BLASTBufferQueue::createBufferQueue(sp* outProducer *outConsumer = consumer; } -void BLASTBufferQueue::onFirstRef() { - // safe default, most producers are expected to override this - // - // This is done in onFirstRef instead of BLASTBufferQueue's constructor because - // BBQBufferQueueProducer::setMaxDequeuedBufferCount promotes a weak pointer to BLASTBufferQueue - // to a strong pointer. If this is done in the constructor, then when the strong pointer goes - // out of scope, it's the last reference so BLASTBufferQueue is deleted. - mProducer->setMaxDequeuedBufferCount(2); -} - void BLASTBufferQueue::resizeFrameEventHistory(size_t newSize) { // This can be null during creation of the buffer queue, but resizing won't do anything at that // point in time, so just ignore. This can go away once the class relationships and lifetimes of @@ -1459,72 +1222,4 @@ void BLASTBufferQueue::setTransactionHangCallback( mTransactionHangCallback = callback; } -BLASTBufferQueue::BufferReleaseReader::BufferReleaseReader( - std::unique_ptr endpoint) - : mEndpoint(std::move(endpoint)) { - mEpollFd = android::base::unique_fd(epoll_create1(0)); - if (!mEpollFd.ok()) { - ALOGE("Failed to create buffer release epoll file descriptor. errno=%d message='%s'", errno, - strerror(errno)); - } - - epoll_event event; - event.events = EPOLLIN; - event.data.fd = mEndpoint->getFd(); - if (epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mEndpoint->getFd(), &event) == -1) { - ALOGE("Failed to register buffer release consumer file descriptor with epoll. errno=%d " - "message='%s'", - errno, strerror(errno)); - } - - mEventFd = android::base::unique_fd(eventfd(0, EFD_NONBLOCK)); - event.data.fd = mEventFd.get(); - if (epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mEventFd.get(), &event) == -1) { - ALOGE("Failed to register buffer release eventfd with epoll. errno=%d message='%s'", errno, - strerror(errno)); - } -} - -status_t BLASTBufferQueue::BufferReleaseReader::readNonBlocking(ReleaseCallbackId& outId, - sp& outFence) { - std::lock_guard lock{mMutex}; - return mEndpoint->readReleaseFence(outId, outFence); -} - -status_t BLASTBufferQueue::BufferReleaseReader::readBlocking(ReleaseCallbackId& outId, - sp& outFence, - std::chrono::milliseconds timeout) { - epoll_event event; - int eventCount = epoll_wait(mEpollFd.get(), &event, 1 /* maxevents */, timeout.count()); - - if (eventCount == -1) { - ALOGE("epoll_wait error while waiting for buffer release. errno=%d message='%s'", errno, - strerror(errno)); - return UNKNOWN_ERROR; - } - - if (eventCount == 0) { - return TIMED_OUT; - } - - if (event.data.fd == mEventFd.get()) { - uint64_t value; - if (read(mEventFd.get(), &value, sizeof(uint64_t)) == -1 && errno != EWOULDBLOCK) { - ALOGE("error while reading from eventfd. errno=%d message='%s'", errno, - strerror(errno)); - } - return WOULD_BLOCK; - } - - std::lock_guard lock{mMutex}; - return mEndpoint->readReleaseFence(outId, outFence); -} - -void BLASTBufferQueue::BufferReleaseReader::interruptBlockingRead() { - uint64_t value = 1; - if (write(mEventFd.get(), &value, sizeof(uint64_t)) == -1) { - ALOGE("failed to notify dequeue event. errno=%d message='%s'", errno, strerror(errno)); - } -} - } // namespace android diff --git a/libs/gui/BufferReleaseChannel.cpp b/libs/gui/BufferReleaseChannel.cpp deleted file mode 100644 index 183b25a4cb..0000000000 --- a/libs/gui/BufferReleaseChannel.cpp +++ /dev/null @@ -1,364 +0,0 @@ -/* - * Copyright (C) 2024 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_TAG "BufferReleaseChannel" -#define ATRACE_TAG ATRACE_TAG_GRAPHICS - -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include - -#include - -using android::base::Result; - -namespace android::gui { - -namespace { - -template -static void readAligned(const void*& buffer, size_t& size, T& value) { - size -= FlattenableUtils::align(buffer); - FlattenableUtils::read(buffer, size, value); -} - -template -static void writeAligned(void*& buffer, size_t& size, T value) { - size -= FlattenableUtils::align(buffer); - FlattenableUtils::write(buffer, size, value); -} - -template -static void addAligned(size_t& size, T /* value */) { - size = FlattenableUtils::align(size); - size += sizeof(T); -} - -template -static inline constexpr uint32_t low32(const T n) { - return static_cast(static_cast(n)); -} - -template -static inline constexpr uint32_t high32(const T n) { - return static_cast(static_cast(n) >> 32); -} - -template -static inline constexpr T to64(const uint32_t lo, const uint32_t hi) { - return static_cast(static_cast(hi) << 32 | lo); -} - -} // namespace - -size_t BufferReleaseChannel::Message::getPodSize() const { - size_t size = 0; - addAligned(size, low32(releaseCallbackId.bufferId)); - addAligned(size, high32(releaseCallbackId.bufferId)); - addAligned(size, low32(releaseCallbackId.framenumber)); - addAligned(size, high32(releaseCallbackId.framenumber)); - return size; -} - -size_t BufferReleaseChannel::Message::getFlattenedSize() const { - size_t size = releaseFence->getFlattenedSize(); - size = FlattenableUtils::align<4>(size); - size += getPodSize(); - return size; -} - -status_t BufferReleaseChannel::Message::flatten(void*& buffer, size_t& size, int*& fds, - size_t& count) const { - if (status_t err = releaseFence->flatten(buffer, size, fds, count); err != OK) { - return err; - } - size -= FlattenableUtils::align<4>(buffer); - - // Check we still have enough space - if (size < getPodSize()) { - return NO_MEMORY; - } - - writeAligned(buffer, size, low32(releaseCallbackId.bufferId)); - writeAligned(buffer, size, high32(releaseCallbackId.bufferId)); - writeAligned(buffer, size, low32(releaseCallbackId.framenumber)); - writeAligned(buffer, size, high32(releaseCallbackId.framenumber)); - return OK; -} - -status_t BufferReleaseChannel::Message::unflatten(void const*& buffer, size_t& size, - int const*& fds, size_t& count) { - releaseFence = new Fence(); - if (status_t err = releaseFence->unflatten(buffer, size, fds, count); err != OK) { - return err; - } - size -= FlattenableUtils::align<4>(buffer); - - // Check we still have enough space - if (size < getPodSize()) { - return OK; - } - - uint32_t bufferIdLo = 0, bufferIdHi = 0; - uint32_t frameNumberLo = 0, frameNumberHi = 0; - - readAligned(buffer, size, bufferIdLo); - readAligned(buffer, size, bufferIdHi); - releaseCallbackId.bufferId = to64(bufferIdLo, bufferIdHi); - readAligned(buffer, size, frameNumberLo); - readAligned(buffer, size, frameNumberHi); - releaseCallbackId.framenumber = to64(frameNumberLo, frameNumberHi); - - return NO_ERROR; -} - -status_t BufferReleaseChannel::ConsumerEndpoint::readReleaseFence( - ReleaseCallbackId& outReleaseCallbackId, sp& outReleaseFence) { - Message releaseBufferMessage; - mFlattenedBuffer.resize(releaseBufferMessage.getFlattenedSize()); - std::array controlMessageBuffer; - - iovec iov{ - .iov_base = mFlattenedBuffer.data(), - .iov_len = mFlattenedBuffer.size(), - }; - - msghdr msg{ - .msg_iov = &iov, - .msg_iovlen = 1, - .msg_control = controlMessageBuffer.data(), - .msg_controllen = controlMessageBuffer.size(), - }; - - int result; - do { - result = recvmsg(mFd, &msg, 0); - } while (result == -1 && errno == EINTR); - if (result == -1) { - if (errno == EWOULDBLOCK || errno == EAGAIN) { - return WOULD_BLOCK; - } - ALOGE("Error reading release fence from socket: error %#x (%s)", errno, strerror(errno)); - return -errno; - } - - if (msg.msg_iovlen != 1) { - ALOGE("Error reading release fence from socket: bad data length"); - return UNKNOWN_ERROR; - } - - if (msg.msg_controllen % sizeof(int) != 0) { - ALOGE("Error reading release fence from socket: bad fd length"); - return UNKNOWN_ERROR; - } - - size_t dataLen = msg.msg_iov->iov_len; - const void* data = static_cast(msg.msg_iov->iov_base); - if (!data) { - ALOGE("Error reading release fence from socket: no buffer data"); - return UNKNOWN_ERROR; - } - - size_t fdCount = 0; - const int* fdData = nullptr; - if (cmsghdr* cmsg = CMSG_FIRSTHDR(&msg)) { - fdData = reinterpret_cast(CMSG_DATA(cmsg)); - fdCount = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); - } - - if (status_t err = releaseBufferMessage.unflatten(data, dataLen, fdData, fdCount); err != OK) { - return err; - } - - outReleaseCallbackId = releaseBufferMessage.releaseCallbackId; - outReleaseFence = std::move(releaseBufferMessage.releaseFence); - - return OK; -} - -int BufferReleaseChannel::ProducerEndpoint::writeReleaseFence(const ReleaseCallbackId& callbackId, - const sp& fence) { - Message releaseBufferMessage(callbackId, fence ? fence : Fence::NO_FENCE); - mFlattenedBuffer.resize(releaseBufferMessage.getFlattenedSize()); - int flattenedFd; - { - // Make copies of needed items since flatten modifies them, and we don't - // want to send anything if there's an error during flatten. - void* flattenedBufferPtr = mFlattenedBuffer.data(); - size_t flattenedBufferSize = mFlattenedBuffer.size(); - int* flattenedFdPtr = &flattenedFd; - size_t flattenedFdCount = 1; - if (status_t err = releaseBufferMessage.flatten(flattenedBufferPtr, flattenedBufferSize, - flattenedFdPtr, flattenedFdCount); - err != OK) { - ALOGE("Failed to flatten BufferReleaseChannel message."); - return err; - } - } - - iovec iov{ - .iov_base = mFlattenedBuffer.data(), - .iov_len = mFlattenedBuffer.size(), - }; - - msghdr msg{ - .msg_iov = &iov, - .msg_iovlen = 1, - }; - - std::array controlMessageBuffer; - if (fence && fence->isValid()) { - msg.msg_control = controlMessageBuffer.data(); - msg.msg_controllen = controlMessageBuffer.size(); - - cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(int)); - memcpy(CMSG_DATA(cmsg), &flattenedFd, sizeof(int)); - } - - int result; - do { - result = sendmsg(mFd, &msg, 0); - } while (result == -1 && errno == EINTR); - if (result == -1) { - ALOGD("Error writing release fence to socket: error %#x (%s)", errno, strerror(errno)); - return -errno; - } - - return OK; -} - -status_t BufferReleaseChannel::ProducerEndpoint::readFromParcel(const android::Parcel* parcel) { - if (!parcel) return STATUS_BAD_VALUE; - SAFE_PARCEL(parcel->readUtf8FromUtf16, &mName); - SAFE_PARCEL(parcel->readUniqueFileDescriptor, &mFd); - return STATUS_OK; -} - -status_t BufferReleaseChannel::ProducerEndpoint::writeToParcel(android::Parcel* parcel) const { - if (!parcel) return STATUS_BAD_VALUE; - SAFE_PARCEL(parcel->writeUtf8AsUtf16, mName); - SAFE_PARCEL(parcel->writeUniqueFileDescriptor, mFd); - return STATUS_OK; -} - -status_t BufferReleaseChannel::open(std::string name, - std::unique_ptr& outConsumer, - std::shared_ptr& outProducer) { - outConsumer.reset(); - outProducer.reset(); - - int sockets[2]; - if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, sockets)) { - ALOGE("[%s] Failed to create socket pair. errorno=%d message='%s'", name.c_str(), errno, - strerror(errno)); - return -errno; - } - - android::base::unique_fd consumerFd(sockets[0]); - android::base::unique_fd producerFd(sockets[1]); - - // Socket buffer size. The default is typically about 128KB, which is much larger than - // we really need. So we make it smaller. It just needs to be big enough to hold - // a few dozen release fences. - const int bufferSize = 32 * 1024; - if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == - -1) { - ALOGE("[%s] Failed to set consumer socket send buffer size. errno=%d message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == - -1) { - ALOGE("[%s] Failed to set consumer socket receive buffer size. errno=%d " - "message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - if (setsockopt(producerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == - -1) { - ALOGE("[%s] Failed to set producer socket send buffer size. errno=%d message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == - -1) { - ALOGE("[%s] Failed to set producer socket receive buffer size. errno=%d " - "message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - - // Configure the consumer socket to be non-blocking. - int flags = fcntl(consumerFd.get(), F_GETFL, 0); - if (flags == -1) { - ALOGE("[%s] Failed to get consumer socket flags. errno=%d message='%s'", name.c_str(), - errno, strerror(errno)); - return -errno; - } - if (fcntl(consumerFd.get(), F_SETFL, flags | O_NONBLOCK) == -1) { - ALOGE("[%s] Failed to set consumer socket to non-blocking mode. errno=%d " - "message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - - // Configure a timeout for the producer socket. - const timeval timeout{.tv_sec = 1, .tv_usec = 0}; - if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeval)) == -1) { - ALOGE("[%s] Failed to set producer socket timeout. errno=%d message='%s'", name.c_str(), - errno, strerror(errno)); - return -errno; - } - - // Make the consumer read-only - if (shutdown(consumerFd.get(), SHUT_WR) == -1) { - ALOGE("[%s] Failed to shutdown writing on consumer socket. errno=%d message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - - // Make the producer write-only - if (shutdown(producerFd.get(), SHUT_RD) == -1) { - ALOGE("[%s] Failed to shutdown reading on producer socket. errno=%d message='%s'", - name.c_str(), errno, strerror(errno)); - return -errno; - } - - outConsumer = std::make_unique(name, std::move(consumerFd)); - outProducer = std::make_shared(std::move(name), std::move(producerFd)); - return STATUS_OK; -} - -} // namespace android::gui diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index adf1646b83..307ae3990e 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -194,12 +194,6 @@ status_t layer_state_t::write(Parcel& output) const SAFE_PARCEL(output.writeFloat, currentHdrSdrRatio); SAFE_PARCEL(output.writeFloat, desiredHdrSdrRatio); SAFE_PARCEL(output.writeInt32, static_cast(cachingHint)); - - const bool hasBufferReleaseChannel = (bufferReleaseChannel != nullptr); - SAFE_PARCEL(output.writeBool, hasBufferReleaseChannel); - if (hasBufferReleaseChannel) { - SAFE_PARCEL(output.writeParcelable, *bufferReleaseChannel); - } return NO_ERROR; } @@ -345,12 +339,6 @@ status_t layer_state_t::read(const Parcel& input) SAFE_PARCEL(input.readInt32, &tmpInt32); cachingHint = static_cast(tmpInt32); - bool hasBufferReleaseChannel; - SAFE_PARCEL(input.readBool, &hasBufferReleaseChannel); - if (hasBufferReleaseChannel) { - bufferReleaseChannel = std::make_shared(); - SAFE_PARCEL(input.readParcelable, bufferReleaseChannel.get()); - } return NO_ERROR; } @@ -730,10 +718,6 @@ void layer_state_t::merge(const layer_state_t& other) { if (other.what & eFlushJankData) { what |= eFlushJankData; } - if (other.what & eBufferReleaseChannelChanged) { - what |= eBufferReleaseChannelChanged; - bufferReleaseChannel = other.bufferReleaseChannel; - } if ((other.what & what) != other.what) { ALOGE("Unmerged SurfaceComposer Transaction properties. LayerState::merge needs updating? " "other.what=0x%" PRIX64 " what=0x%" PRIX64 " unmerged flags=0x%" PRIX64, @@ -813,7 +797,6 @@ uint64_t layer_state_t::diff(const layer_state_t& other) const { CHECK_DIFF(diff, eColorChanged, other, color.rgb); CHECK_DIFF(diff, eColorSpaceAgnosticChanged, other, colorSpaceAgnostic); CHECK_DIFF(diff, eDimmingEnabledChanged, other, dimmingEnabled); - if (other.what & eBufferReleaseChannelChanged) diff |= eBufferReleaseChannelChanged; return diff; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index cdf57ffd61..e86e13d3ee 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2395,22 +2395,6 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDropI return *this; } -SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBufferReleaseChannel( - const sp& sc, - const std::shared_ptr& channel) { - layer_state_t* s = getLayerState(sc); - if (!s) { - mStatus = BAD_INDEX; - return *this; - } - - s->what |= layer_state_t::eBufferReleaseChannelChanged; - s->bufferReleaseChannel = channel; - - registerSurfaceControlForCallback(sc); - return *this; -} - // --------------------------------------------------------------------------- DisplayState& SurfaceComposerClient::Transaction::getDisplayState(const sp& token) { diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index e9a350c301..0e1a505c69 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -19,7 +19,7 @@ #include #include -#include + #include #include @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -130,8 +131,6 @@ public: virtual ~BLASTBufferQueue(); - void onFirstRef() override; - private: friend class BLASTBufferQueueHelper; friend class BBQBufferQueueProducer; @@ -171,23 +170,11 @@ private: // BufferQueue internally allows 1 more than // the max to be acquired - int32_t mMaxAcquiredBuffers GUARDED_BY(mMutex) = 1; - int32_t mMaxDequeuedBuffers GUARDED_BY(mMutex) = 1; - static constexpr int32_t kMaxBufferCount = BufferQueueDefs::NUM_BUFFER_SLOTS; - - // mNumDequeued is an atomic instead of being guarded by mMutex so that it can be incremented in - // onFrameDequeued while avoiding lock contention. updateDequeueShouldBlockLocked is not called - // after mNumDequeued is incremented for this reason. This means mDequeueShouldBlock may be - // temporarily false when it should be true. This can happen if multiple threads are dequeuing - // buffers or if dequeueBuffers is called multiple times in a row without queuing a buffer in - // between. As mDequeueShouldBlock is only used for optimization, this is OK. - std::atomic_int mNumDequeued; + int32_t mMaxAcquiredBuffers = 1; + int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0; int32_t mNumAcquired GUARDED_BY(mMutex) = 0; - bool mAsyncMode GUARDED_BY(mMutex) = false; - bool mSharedBufferMode GUARDED_BY(mMutex) = false; - // A value used to identify if a producer has been changed for the same SurfaceControl. // This is needed to know when the frame number has been reset to make sure we don't // latch stale buffers and that we don't wait on barriers from an old producer. @@ -313,41 +300,6 @@ private: std::function mTransactionHangCallback; std::unordered_set mSyncedFrameNumbers GUARDED_BY(mMutex); - - class BufferReleaseReader { - public: - BufferReleaseReader(std::unique_ptr); - - BufferReleaseReader(const BufferReleaseReader&) = delete; - BufferReleaseReader& operator=(const BufferReleaseReader&) = delete; - - // Block until we can release a buffer. - // - // Returns: - // * OK if a ReleaseCallbackId and Fence were successfully read. - // * TIMED_OUT if the specified timeout was reached. - // * WOULD_BLOCK if the blocking read was interrupted by interruptBlockingRead. - // * UNKNOWN_ERROR if something went wrong. - status_t readBlocking(ReleaseCallbackId&, sp&, std::chrono::milliseconds timeout); - - status_t readNonBlocking(ReleaseCallbackId&, sp&); - - void interruptBlockingRead(); - - private: - std::mutex mMutex; - std::unique_ptr mEndpoint GUARDED_BY(mMutex); - android::base::unique_fd mEpollFd; - android::base::unique_fd mEventFd; - }; - - // BufferReleaseChannel is used to communicate buffer releases from SurfaceFlinger to - // the client. See BBQBufferQueueProducer::dequeueBuffer for details. - std::optional mBufferReleaseReader; - std::shared_ptr mBufferReleaseProducer; - - std::atomic_bool mDequeueShouldBlock{false}; - void updateDequeueShouldBlockLocked() REQUIRES(mMutex); }; } // namespace android diff --git a/libs/gui/include/gui/BufferReleaseChannel.h b/libs/gui/include/gui/BufferReleaseChannel.h deleted file mode 100644 index cb0b261240..0000000000 --- a/libs/gui/include/gui/BufferReleaseChannel.h +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright (C) 2024 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. - */ - -#pragma once - -#include - -#include -#include -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -namespace android::gui { - -/** - * IPC wrapper to pass release fences from SurfaceFlinger to apps via a local unix domain socket. - */ -class BufferReleaseChannel { -private: - class Endpoint { - public: - Endpoint(std::string name, android::base::unique_fd fd) - : mName(std::move(name)), mFd(std::move(fd)) {} - Endpoint() {} - - Endpoint(Endpoint&&) noexcept = default; - Endpoint& operator=(Endpoint&&) noexcept = default; - - Endpoint(const Endpoint&) = delete; - void operator=(const Endpoint&) = delete; - - const android::base::unique_fd& getFd() const { return mFd; } - - protected: - std::string mName; - android::base::unique_fd mFd; - }; - -public: - class ConsumerEndpoint : public Endpoint { - public: - ConsumerEndpoint(std::string name, android::base::unique_fd fd) - : Endpoint(std::move(name), std::move(fd)) {} - - /** - * Reads a release fence from the BufferReleaseChannel. - * - * Returns OK on success. - * Returns WOULD_BLOCK if there is no fence present. - * Other errors probably indicate that the channel is broken. - */ - status_t readReleaseFence(ReleaseCallbackId& outReleaseCallbackId, - sp& outReleaseFence); - - private: - std::vector mFlattenedBuffer; - }; - - class ProducerEndpoint : public Endpoint, public Parcelable { - public: - ProducerEndpoint(std::string name, android::base::unique_fd fd) - : Endpoint(std::move(name), std::move(fd)) {} - ProducerEndpoint() {} - - status_t readFromParcel(const android::Parcel* parcel) override; - status_t writeToParcel(android::Parcel* parcel) const override; - - status_t writeReleaseFence(const ReleaseCallbackId&, const sp& releaseFence); - - private: - std::vector mFlattenedBuffer; - }; - - /** - * Create two endpoints that make up the BufferReleaseChannel. - * - * Return OK on success. - */ - static status_t open(const std::string name, std::unique_ptr& outConsumer, - std::shared_ptr& outProducer); - - struct Message : public Flattenable { - ReleaseCallbackId releaseCallbackId; - sp releaseFence = Fence::NO_FENCE; - - Message() = default; - Message(ReleaseCallbackId releaseCallbackId, sp releaseFence) - : releaseCallbackId(releaseCallbackId), releaseFence(std::move(releaseFence)) {} - - // Flattenable protocol - size_t getFlattenedSize() const; - - size_t getFdCount() const { return releaseFence->getFdCount(); } - - status_t flatten(void*& buffer, size_t& size, int*& fds, size_t& count) const; - - status_t unflatten(void const*& buffer, size_t& size, int const*& fds, size_t& count); - - private: - size_t getPodSize() const; - }; -}; - -} // namespace android::gui diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index d41994589f..3fb1894585 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -34,7 +34,6 @@ #include #include -#include #include #include #include @@ -221,7 +220,6 @@ struct layer_state_t { eDropInputModeChanged = 0x8000'00000000, eExtendedRangeBrightnessChanged = 0x10000'00000000, eEdgeExtensionChanged = 0x20000'00000000, - eBufferReleaseChannelChanged = 0x40000'00000000, }; layer_state_t(); @@ -414,8 +412,6 @@ struct layer_state_t { TrustedPresentationThresholds trustedPresentationThresholds; TrustedPresentationListener trustedPresentationListener; - - std::shared_ptr bufferReleaseChannel; }; class ComposerState { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 8c1d6443c5..95574ee30e 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -45,7 +45,6 @@ #include #include -#include #include #include #include @@ -764,10 +763,6 @@ public: const Rect& destinationFrame); Transaction& setDropInputMode(const sp& sc, gui::DropInputMode mode); - Transaction& setBufferReleaseChannel( - const sp& sc, - const std::shared_ptr& channel); - status_t setDisplaySurface(const sp& token, const sp& bufferProducer); diff --git a/libs/gui/tests/Android.bp b/libs/gui/tests/Android.bp index daaddfbc15..ea8acbbb72 100644 --- a/libs/gui/tests/Android.bp +++ b/libs/gui/tests/Android.bp @@ -32,7 +32,6 @@ cc_test { "BLASTBufferQueue_test.cpp", "BufferItemConsumer_test.cpp", "BufferQueue_test.cpp", - "BufferReleaseChannel_test.cpp", "Choreographer_test.cpp", "CompositorTiming_test.cpp", "CpuConsumer_test.cpp", diff --git a/libs/gui/tests/BufferReleaseChannel_test.cpp b/libs/gui/tests/BufferReleaseChannel_test.cpp deleted file mode 100644 index f3e962c192..0000000000 --- a/libs/gui/tests/BufferReleaseChannel_test.cpp +++ /dev/null @@ -1,121 +0,0 @@ -/* - * Copyright (C) 2024 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. - */ - -#include -#include - -#include -#include - -using namespace std::string_literals; -using android::gui::BufferReleaseChannel; - -namespace android { - -namespace { - -// Helper function to check if two file descriptors point to the same file. -bool is_same_file(int fd1, int fd2) { - struct stat stat1; - if (fstat(fd1, &stat1) != 0) { - return false; - } - struct stat stat2; - if (fstat(fd2, &stat2) != 0) { - return false; - } - return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino); -} - -} // namespace - -TEST(BufferReleaseChannelTest, MessageFlattenable) { - ReleaseCallbackId releaseCallbackId{1, 2}; - sp releaseFence = sp::make(memfd_create("fake-fence-fd", 0)); - - std::vector dataBuffer; - std::vector fdBuffer; - - // Verify that we can flatten a message - { - BufferReleaseChannel::Message message{releaseCallbackId, releaseFence}; - - dataBuffer.resize(message.getFlattenedSize()); - void* dataPtr = dataBuffer.data(); - size_t dataSize = dataBuffer.size(); - - fdBuffer.resize(message.getFdCount()); - int* fdPtr = fdBuffer.data(); - size_t fdSize = fdBuffer.size(); - - ASSERT_EQ(OK, message.flatten(dataPtr, dataSize, fdPtr, fdSize)); - - // Fence's unique_fd uses fdsan to check ownership of the file descriptor. Normally the file - // descriptor is passed through the Unix socket and duplicated (and sent to another process) - // so there's no problem with duplicate file descriptor ownership. For this unit test, we - // need to set up a duplicate file descriptor to avoid crashing due to duplicate ownership. - ASSERT_EQ(releaseFence->get(), fdBuffer[0]); - fdBuffer[0] = message.releaseFence->dup(); - } - - // Verify that we can unflatten a message - { - BufferReleaseChannel::Message message; - - const void* dataPtr = dataBuffer.data(); - size_t dataSize = dataBuffer.size(); - - const int* fdPtr = fdBuffer.data(); - size_t fdSize = fdBuffer.size(); - - ASSERT_EQ(OK, message.unflatten(dataPtr, dataSize, fdPtr, fdSize)); - ASSERT_EQ(releaseCallbackId, message.releaseCallbackId); - ASSERT_TRUE(is_same_file(releaseFence->get(), message.releaseFence->get())); - } -} - -// Verify that the BufferReleaseChannel consume returns WOULD_BLOCK when there's no message -// available. -TEST(BufferReleaseChannelTest, ConsumerEndpointIsNonBlocking) { - std::unique_ptr consumer; - std::shared_ptr producer; - ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); - - ReleaseCallbackId releaseCallbackId; - sp releaseFence; - ASSERT_EQ(WOULD_BLOCK, consumer->readReleaseFence(releaseCallbackId, releaseFence)); -} - -// Verify that we can write a message to the BufferReleaseChannel producer and read that message -// using the BufferReleaseChannel consumer. -TEST(BufferReleaseChannelTest, ProduceAndConsume) { - std::unique_ptr consumer; - std::shared_ptr producer; - ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); - - ReleaseCallbackId producerReleaseCallbackId{1, 2}; - sp producerReleaseFence = sp::make(memfd_create("fake-fence-fd", 0)); - ASSERT_EQ(OK, producer->writeReleaseFence(producerReleaseCallbackId, producerReleaseFence)); - - ReleaseCallbackId consumerReleaseCallbackId; - sp consumerReleaseFence; - ASSERT_EQ(OK, consumer->readReleaseFence(consumerReleaseCallbackId, consumerReleaseFence)); - - ASSERT_EQ(producerReleaseCallbackId, consumerReleaseCallbackId); - ASSERT_TRUE(is_same_file(producerReleaseFence->get(), consumerReleaseFence->get())); -} - -} // namespace android diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 0682fdb639..0eb6cc32c5 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -2775,14 +2775,11 @@ void Layer::callReleaseBufferCallback(const sp& l return; } SFTRACE_FORMAT_INSTANT("callReleaseBufferCallback %s - %" PRIu64, getDebugName(), framenumber); - ReleaseCallbackId callbackId{buffer->getId(), framenumber}; - const sp& fence = releaseFence ? releaseFence : Fence::NO_FENCE; uint32_t currentMaxAcquiredBufferCount = mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid); - listener->onReleaseBuffer(callbackId, fence, currentMaxAcquiredBufferCount); - if (mBufferReleaseChannel) { - mBufferReleaseChannel->writeReleaseFence(callbackId, fence); - } + listener->onReleaseBuffer({buffer->getId(), framenumber}, + releaseFence ? releaseFence : Fence::NO_FENCE, + currentMaxAcquiredBufferCount); } sp Layer::findCallbackHandle() { @@ -2900,7 +2897,6 @@ void Layer::onLayerDisplayed(ftl::SharedFuture futureFenceResult, void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { for (const auto& handle : mDrawingState.callbackHandles) { - handle->bufferReleaseChannel = mBufferReleaseChannel; handle->transformHint = mTransformHint; handle->dequeueReadyTime = dequeueReadyTime; handle->currentMaxAcquiredBufferCount = @@ -4372,11 +4368,6 @@ bool Layer::setTrustedPresentationInfo(TrustedPresentationThresholds const& thre return haveTrustedPresentationListener; } -void Layer::setBufferReleaseChannel( - const std::shared_ptr& channel) { - mBufferReleaseChannel = channel; -} - void Layer::updateLastLatchTime(nsecs_t latchTime) { mLastLatchTime = latchTime; } diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index d4707388b2..52f169ebee 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -550,7 +550,6 @@ public: }; BufferInfo mBufferInfo; - std::shared_ptr mBufferReleaseChannel; // implements compositionengine::LayerFE const compositionengine::LayerFECompositionState* getCompositionState() const; @@ -814,8 +813,6 @@ public: bool setTrustedPresentationInfo(TrustedPresentationThresholds const& thresholds, TrustedPresentationListener const& listener); - void setBufferReleaseChannel( - const std::shared_ptr& channel); // Creates a new handle each time, so we only expect // this to be called once. diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index f61763880e..84553cab39 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5789,10 +5789,6 @@ uint32_t SurfaceFlinger::updateLayerCallbacksAndStats(const FrameTimelineInfo& f } } - if (what & layer_state_t::eBufferReleaseChannelChanged) { - layer->setBufferReleaseChannel(s.bufferReleaseChannel); - } - const auto& requestedLayerState = mLayerLifecycleManager.getLayerFromId(layer->getSequence()); bool willPresentCurrentTransaction = requestedLayerState && (requestedLayerState->hasReadyFrame() || diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp index 9ea0f5f4ca..881bf35b58 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.cpp +++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp @@ -149,12 +149,6 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle->transformHint, handle->currentMaxAcquiredBufferCount, eventStats, handle->previousReleaseCallbackId); - if (handle->bufferReleaseChannel && - handle->previousReleaseCallbackId != ReleaseCallbackId::INVALID_ID) { - mBufferReleases.emplace_back(handle->bufferReleaseChannel, - handle->previousReleaseCallbackId, - handle->previousReleaseFence); - } } return NO_ERROR; } @@ -164,11 +158,6 @@ void TransactionCallbackInvoker::addPresentFence(sp presentFence) { } void TransactionCallbackInvoker::sendCallbacks(bool onCommitOnly) { - for (const auto& bufferRelease : mBufferReleases) { - bufferRelease.channel->writeReleaseFence(bufferRelease.callbackId, bufferRelease.fence); - } - mBufferReleases.clear(); - // For each listener auto completedTransactionsItr = mCompletedTransactions.begin(); ftl::SmallVector listenerStatsToSend; diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index f74f9644b6..7853a9f359 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -60,7 +59,6 @@ public: uint64_t frameNumber = 0; uint64_t previousFrameNumber = 0; ReleaseCallbackId previousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; - std::shared_ptr bufferReleaseChannel; }; class TransactionCallbackInvoker { @@ -88,13 +86,6 @@ private: std::unordered_map, std::deque, IListenerHash> mCompletedTransactions; - struct BufferRelease { - std::shared_ptr channel; - ReleaseCallbackId callbackId; - sp fence; - }; - std::vector mBufferReleases; - sp mPresentFence; }; -- cgit v1.2.3-59-g8ed1b From 62f0c2021c7d0a9ff62fd048746c6317aa91af78 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Thu, 1 Aug 2024 22:23:38 +0000 Subject: Remove unused mTransactionNestCount Flag: EXEMPT bug fix Bug: 356936695 Test: presubmit Change-Id: I80e9eb94d2d877580661fb5927ef70a090565a93 --- libs/gui/SurfaceComposerClient.cpp | 5 ----- libs/gui/include/gui/SurfaceComposerClient.h | 1 - 2 files changed, 6 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index e86e13d3ee..f663600f9e 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -827,7 +827,6 @@ SurfaceComposerClient::Transaction::Transaction() { SurfaceComposerClient::Transaction::Transaction(const Transaction& other) : mId(other.mId), - mTransactionNestCount(other.mTransactionNestCount), mAnimation(other.mAnimation), mEarlyWakeupStart(other.mEarlyWakeupStart), mEarlyWakeupEnd(other.mEarlyWakeupEnd), @@ -867,7 +866,6 @@ SurfaceComposerClient::Transaction::createFromParcel(const Parcel* parcel) { status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel) { const uint64_t transactionId = parcel->readUint64(); - const uint32_t transactionNestCount = parcel->readUint32(); const bool animation = parcel->readBool(); const bool earlyWakeupStart = parcel->readBool(); const bool earlyWakeupEnd = parcel->readBool(); @@ -964,7 +962,6 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel // Parsing was successful. Update the object. mId = transactionId; - mTransactionNestCount = transactionNestCount; mAnimation = animation; mEarlyWakeupStart = earlyWakeupStart; mEarlyWakeupEnd = earlyWakeupEnd; @@ -996,7 +993,6 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const const_cast(this)->cacheBuffers(); parcel->writeUint64(mId); - parcel->writeUint32(mTransactionNestCount); parcel->writeBool(mAnimation); parcel->writeBool(mEarlyWakeupStart); parcel->writeBool(mEarlyWakeupEnd); @@ -1148,7 +1144,6 @@ void SurfaceComposerClient::Transaction::clear() { mInputWindowCommands.clear(); mUncacheBuffers.clear(); mMayContainBuffer = false; - mTransactionNestCount = 0; mAnimation = false; mEarlyWakeupStart = false; mEarlyWakeupEnd = false; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 95574ee30e..61a65bb29f 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -451,7 +451,6 @@ public: uint64_t mId; - uint32_t mTransactionNestCount = 0; bool mAnimation = false; bool mEarlyWakeupStart = false; bool mEarlyWakeupEnd = false; -- cgit v1.2.3-59-g8ed1b From f693bcf2ee13391f99b8ede5562047981e1f5968 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Fri, 2 Aug 2024 09:55:23 -0500 Subject: Add BufferReleaseChannel BufferReleaseChannel will be used to communicate buffer releases from SurfaceFlinger to BLASTBufferQueue. Bug: 294133380 Flag: com.android.graphics.libgui.flags.buffer_release_channel Test: BufferReleaseChannelTest Change-Id: Ic38e8eefc96abc0b2bbe780115b7628413e8b829 --- libs/gui/Android.bp | 1 + libs/gui/BufferReleaseChannel.cpp | 358 +++++++++++++++++++++++++++ libs/gui/LayerState.cpp | 20 +- libs/gui/SurfaceComposerClient.cpp | 16 ++ libs/gui/include/gui/BufferReleaseChannel.h | 125 ++++++++++ libs/gui/include/gui/LayerState.h | 4 + libs/gui/include/gui/SurfaceComposerClient.h | 5 + libs/gui/tests/Android.bp | 1 + libs/gui/tests/BufferReleaseChannel_test.cpp | 138 +++++++++++ 9 files changed, 667 insertions(+), 1 deletion(-) create mode 100644 libs/gui/BufferReleaseChannel.cpp create mode 100644 libs/gui/include/gui/BufferReleaseChannel.h create mode 100644 libs/gui/tests/BufferReleaseChannel_test.cpp (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 51d2e5305a..1243b214d3 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -255,6 +255,7 @@ filegroup { "BitTube.cpp", "BLASTBufferQueue.cpp", "BufferItemConsumer.cpp", + "BufferReleaseChannel.cpp", "Choreographer.cpp", "CompositorTiming.cpp", "ConsumerBase.cpp", diff --git a/libs/gui/BufferReleaseChannel.cpp b/libs/gui/BufferReleaseChannel.cpp new file mode 100644 index 0000000000..27367aa83f --- /dev/null +++ b/libs/gui/BufferReleaseChannel.cpp @@ -0,0 +1,358 @@ +/* + * Copyright (C) 2024 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_TAG "BufferReleaseChannel" + +#include +#include +#include + +#include +#include +#include +#include + +#include +#include + +using android::base::Result; + +namespace android::gui { + +namespace { + +template +static void readAligned(const void*& buffer, size_t& size, T& value) { + size -= FlattenableUtils::align(buffer); + FlattenableUtils::read(buffer, size, value); +} + +template +static void writeAligned(void*& buffer, size_t& size, T value) { + size -= FlattenableUtils::align(buffer); + FlattenableUtils::write(buffer, size, value); +} + +template +static void addAligned(size_t& size, T /* value */) { + size = FlattenableUtils::align(size); + size += sizeof(T); +} + +template +static inline constexpr uint32_t low32(const T n) { + return static_cast(static_cast(n)); +} + +template +static inline constexpr uint32_t high32(const T n) { + return static_cast(static_cast(n) >> 32); +} + +template +static inline constexpr T to64(const uint32_t lo, const uint32_t hi) { + return static_cast(static_cast(hi) << 32 | lo); +} + +} // namespace + +size_t BufferReleaseChannel::Message::getPodSize() const { + size_t size = 0; + addAligned(size, low32(releaseCallbackId.bufferId)); + addAligned(size, high32(releaseCallbackId.bufferId)); + addAligned(size, low32(releaseCallbackId.framenumber)); + addAligned(size, high32(releaseCallbackId.framenumber)); + addAligned(size, maxAcquiredBufferCount); + return size; +} + +size_t BufferReleaseChannel::Message::getFlattenedSize() const { + size_t size = releaseFence->getFlattenedSize(); + size = FlattenableUtils::align<4>(size); + size += getPodSize(); + return size; +} + +status_t BufferReleaseChannel::Message::flatten(void*& buffer, size_t& size, int*& fds, + size_t& count) const { + if (status_t err = releaseFence->flatten(buffer, size, fds, count); err != OK) { + return err; + } + size -= FlattenableUtils::align<4>(buffer); + + // Check we still have enough space + if (size < getPodSize()) { + return NO_MEMORY; + } + + writeAligned(buffer, size, low32(releaseCallbackId.bufferId)); + writeAligned(buffer, size, high32(releaseCallbackId.bufferId)); + writeAligned(buffer, size, low32(releaseCallbackId.framenumber)); + writeAligned(buffer, size, high32(releaseCallbackId.framenumber)); + writeAligned(buffer, size, maxAcquiredBufferCount); + return OK; +} + +status_t BufferReleaseChannel::Message::unflatten(void const*& buffer, size_t& size, + int const*& fds, size_t& count) { + releaseFence = new Fence(); + if (status_t err = releaseFence->unflatten(buffer, size, fds, count); err != OK) { + return err; + } + size -= FlattenableUtils::align<4>(buffer); + + // Check we still have enough space + if (size < getPodSize()) { + return OK; + } + + uint32_t bufferIdLo = 0, bufferIdHi = 0; + uint32_t frameNumberLo = 0, frameNumberHi = 0; + + readAligned(buffer, size, bufferIdLo); + readAligned(buffer, size, bufferIdHi); + releaseCallbackId.bufferId = to64(bufferIdLo, bufferIdHi); + readAligned(buffer, size, frameNumberLo); + readAligned(buffer, size, frameNumberHi); + releaseCallbackId.framenumber = to64(frameNumberLo, frameNumberHi); + readAligned(buffer, size, maxAcquiredBufferCount); + + return OK; +} + +status_t BufferReleaseChannel::ConsumerEndpoint::readReleaseFence( + ReleaseCallbackId& outReleaseCallbackId, sp& outReleaseFence, + uint32_t& outMaxAcquiredBufferCount) { + Message message; + mFlattenedBuffer.resize(message.getFlattenedSize()); + std::array controlMessageBuffer; + + iovec iov{ + .iov_base = mFlattenedBuffer.data(), + .iov_len = mFlattenedBuffer.size(), + }; + + msghdr msg{ + .msg_iov = &iov, + .msg_iovlen = 1, + .msg_control = controlMessageBuffer.data(), + .msg_controllen = controlMessageBuffer.size(), + }; + + int result; + do { + result = recvmsg(mFd, &msg, 0); + } while (result == -1 && errno == EINTR); + if (result == -1) { + if (errno == EWOULDBLOCK || errno == EAGAIN) { + return WOULD_BLOCK; + } + ALOGE("Error reading release fence from socket: error %#x (%s)", errno, strerror(errno)); + return UNKNOWN_ERROR; + } + + if (msg.msg_iovlen != 1) { + ALOGE("Error reading release fence from socket: bad data length"); + return UNKNOWN_ERROR; + } + + if (msg.msg_controllen % sizeof(int) != 0) { + ALOGE("Error reading release fence from socket: bad fd length"); + return UNKNOWN_ERROR; + } + + size_t dataLen = msg.msg_iov->iov_len; + const void* data = static_cast(msg.msg_iov->iov_base); + if (!data) { + ALOGE("Error reading release fence from socket: no buffer data"); + return UNKNOWN_ERROR; + } + + size_t fdCount = 0; + const int* fdData = nullptr; + if (cmsghdr* cmsg = CMSG_FIRSTHDR(&msg)) { + fdData = reinterpret_cast(CMSG_DATA(cmsg)); + fdCount = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); + } + + if (status_t err = message.unflatten(data, dataLen, fdData, fdCount); err != OK) { + return err; + } + + outReleaseCallbackId = message.releaseCallbackId; + outReleaseFence = std::move(message.releaseFence); + outMaxAcquiredBufferCount = message.maxAcquiredBufferCount; + + return OK; +} + +int BufferReleaseChannel::ProducerEndpoint::writeReleaseFence(const ReleaseCallbackId& callbackId, + const sp& fence, + uint32_t maxAcquiredBufferCount) { + Message message{callbackId, fence ? fence : Fence::NO_FENCE, maxAcquiredBufferCount}; + mFlattenedBuffer.resize(message.getFlattenedSize()); + int flattenedFd; + { + // Make copies of needed items since flatten modifies them, and we don't + // want to send anything if there's an error during flatten. + void* flattenedBufferPtr = mFlattenedBuffer.data(); + size_t flattenedBufferSize = mFlattenedBuffer.size(); + int* flattenedFdPtr = &flattenedFd; + size_t flattenedFdCount = 1; + if (status_t err = message.flatten(flattenedBufferPtr, flattenedBufferSize, flattenedFdPtr, + flattenedFdCount); + err != OK) { + ALOGE("Failed to flatten BufferReleaseChannel message."); + return err; + } + } + + iovec iov{ + .iov_base = mFlattenedBuffer.data(), + .iov_len = mFlattenedBuffer.size(), + }; + + msghdr msg{ + .msg_iov = &iov, + .msg_iovlen = 1, + }; + + std::array controlMessageBuffer; + if (fence && fence->isValid()) { + msg.msg_control = controlMessageBuffer.data(); + msg.msg_controllen = controlMessageBuffer.size(); + + cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(int)); + memcpy(CMSG_DATA(cmsg), &flattenedFd, sizeof(int)); + } + + int result; + do { + result = sendmsg(mFd, &msg, 0); + } while (result == -1 && errno == EINTR); + if (result == -1) { + ALOGD("Error writing release fence to socket: error %#x (%s)", errno, strerror(errno)); + return -errno; + } + + return OK; +} + +status_t BufferReleaseChannel::ProducerEndpoint::readFromParcel(const android::Parcel* parcel) { + if (!parcel) return STATUS_BAD_VALUE; + SAFE_PARCEL(parcel->readUtf8FromUtf16, &mName); + SAFE_PARCEL(parcel->readUniqueFileDescriptor, &mFd); + return STATUS_OK; +} + +status_t BufferReleaseChannel::ProducerEndpoint::writeToParcel(android::Parcel* parcel) const { + if (!parcel) return STATUS_BAD_VALUE; + SAFE_PARCEL(parcel->writeUtf8AsUtf16, mName); + SAFE_PARCEL(parcel->writeUniqueFileDescriptor, mFd); + return STATUS_OK; +} + +status_t BufferReleaseChannel::open(std::string name, + std::unique_ptr& outConsumer, + std::shared_ptr& outProducer) { + outConsumer.reset(); + outProducer.reset(); + + int sockets[2]; + if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, sockets)) { + ALOGE("[%s] Failed to create socket pair. errorno=%d message='%s'", name.c_str(), errno, + strerror(errno)); + return -errno; + } + + android::base::unique_fd consumerFd(sockets[0]); + android::base::unique_fd producerFd(sockets[1]); + + // Socket buffer size. The default is typically about 128KB, which is much larger than + // we really need. + size_t bufferSize = 32 * 1024; + if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set consumer socket send buffer size. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(consumerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set consumer socket receive buffer size. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_SNDBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set producer socket send buffer size. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVBUF, &bufferSize, sizeof(bufferSize)) == + -1) { + ALOGE("[%s] Failed to set producer socket receive buffer size. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Configure the consumer socket to be non-blocking. + int flags = fcntl(consumerFd.get(), F_GETFL, 0); + if (flags == -1) { + ALOGE("[%s] Failed to get consumer socket flags. errno=%d message='%s'", name.c_str(), + errno, strerror(errno)); + return -errno; + } + if (fcntl(consumerFd.get(), F_SETFL, flags | O_NONBLOCK) == -1) { + ALOGE("[%s] Failed to set consumer socket to non-blocking mode. errno=%d " + "message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Configure a timeout for the producer socket. + const timeval timeout{.tv_sec = 1, .tv_usec = 0}; + if (setsockopt(producerFd.get(), SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeval)) == -1) { + ALOGE("[%s] Failed to set producer socket timeout. errno=%d message='%s'", name.c_str(), + errno, strerror(errno)); + return -errno; + } + + // Make the consumer read-only + if (shutdown(consumerFd.get(), SHUT_WR) == -1) { + ALOGE("[%s] Failed to shutdown writing on consumer socket. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + // Make the producer write-only + if (shutdown(producerFd.get(), SHUT_RD) == -1) { + ALOGE("[%s] Failed to shutdown reading on producer socket. errno=%d message='%s'", + name.c_str(), errno, strerror(errno)); + return -errno; + } + + outConsumer = std::make_unique(name, std::move(consumerFd)); + outProducer = std::make_shared(std::move(name), std::move(producerFd)); + return STATUS_OK; +} + +} // namespace android::gui \ No newline at end of file diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 307ae3990e..0714fd9501 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -17,7 +17,6 @@ #define LOG_TAG "LayerState" #include -#include #include #include @@ -194,6 +193,13 @@ status_t layer_state_t::write(Parcel& output) const SAFE_PARCEL(output.writeFloat, currentHdrSdrRatio); SAFE_PARCEL(output.writeFloat, desiredHdrSdrRatio); SAFE_PARCEL(output.writeInt32, static_cast(cachingHint)); + + const bool hasBufferReleaseChannel = (bufferReleaseChannel != nullptr); + SAFE_PARCEL(output.writeBool, hasBufferReleaseChannel); + if (hasBufferReleaseChannel) { + SAFE_PARCEL(output.writeParcelable, *bufferReleaseChannel); + } + return NO_ERROR; } @@ -339,6 +345,13 @@ status_t layer_state_t::read(const Parcel& input) SAFE_PARCEL(input.readInt32, &tmpInt32); cachingHint = static_cast(tmpInt32); + bool hasBufferReleaseChannel; + SAFE_PARCEL(input.readBool, &hasBufferReleaseChannel); + if (hasBufferReleaseChannel) { + bufferReleaseChannel = std::make_shared(); + SAFE_PARCEL(input.readParcelable, bufferReleaseChannel.get()); + } + return NO_ERROR; } @@ -718,6 +731,10 @@ void layer_state_t::merge(const layer_state_t& other) { if (other.what & eFlushJankData) { what |= eFlushJankData; } + if (other.what & eBufferReleaseChannelChanged) { + what |= eBufferReleaseChannelChanged; + bufferReleaseChannel = other.bufferReleaseChannel; + } if ((other.what & what) != other.what) { ALOGE("Unmerged SurfaceComposer Transaction properties. LayerState::merge needs updating? " "other.what=0x%" PRIX64 " what=0x%" PRIX64 " unmerged flags=0x%" PRIX64, @@ -797,6 +814,7 @@ uint64_t layer_state_t::diff(const layer_state_t& other) const { CHECK_DIFF(diff, eColorChanged, other, color.rgb); CHECK_DIFF(diff, eColorSpaceAgnosticChanged, other, colorSpaceAgnostic); CHECK_DIFF(diff, eDimmingEnabledChanged, other, dimmingEnabled); + if (other.what & eBufferReleaseChannelChanged) diff |= eBufferReleaseChannelChanged; return diff; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index f663600f9e..b5d9366185 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2390,6 +2390,22 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDropI return *this; } +SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBufferReleaseChannel( + const sp& sc, + const std::shared_ptr& channel) { + layer_state_t* s = getLayerState(sc); + if (!s) { + mStatus = BAD_INDEX; + return *this; + } + + s->what |= layer_state_t::eBufferReleaseChannelChanged; + s->bufferReleaseChannel = channel; + + registerSurfaceControlForCallback(sc); + return *this; +} + // --------------------------------------------------------------------------- DisplayState& SurfaceComposerClient::Transaction::getDisplayState(const sp& token) { diff --git a/libs/gui/include/gui/BufferReleaseChannel.h b/libs/gui/include/gui/BufferReleaseChannel.h new file mode 100644 index 0000000000..51fe0b6fab --- /dev/null +++ b/libs/gui/include/gui/BufferReleaseChannel.h @@ -0,0 +1,125 @@ +/* + * Copyright (C) 2024 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. + */ + +#pragma once + +#include +#include + +#include + +#include +#include +#include +#include + +namespace android::gui { + +/** + * IPC wrapper to pass release fences from SurfaceFlinger to apps via a local unix domain socket. + */ +class BufferReleaseChannel { +private: + class Endpoint { + public: + Endpoint(std::string name, android::base::unique_fd fd) + : mName(std::move(name)), mFd(std::move(fd)) {} + Endpoint() {} + + Endpoint(Endpoint&&) noexcept = default; + Endpoint& operator=(Endpoint&&) noexcept = default; + + Endpoint(const Endpoint&) = delete; + void operator=(const Endpoint&) = delete; + + const android::base::unique_fd& getFd() const { return mFd; } + + protected: + std::string mName; + android::base::unique_fd mFd; + }; + +public: + class ConsumerEndpoint : public Endpoint { + public: + ConsumerEndpoint(std::string name, android::base::unique_fd fd) + : Endpoint(std::move(name), std::move(fd)) {} + + /** + * Reads a release fence from the BufferReleaseChannel. + * + * Returns OK on success. + * Returns WOULD_BLOCK if there is no fence present. + * Other errors probably indicate that the channel is broken. + */ + status_t readReleaseFence(ReleaseCallbackId& outReleaseCallbackId, + sp& outReleaseFence, uint32_t& maxAcquiredBufferCount); + + private: + std::vector mFlattenedBuffer; + }; + + class ProducerEndpoint : public Endpoint, public Parcelable { + public: + ProducerEndpoint(std::string name, android::base::unique_fd fd) + : Endpoint(std::move(name), std::move(fd)) {} + ProducerEndpoint() {} + + status_t readFromParcel(const android::Parcel* parcel) override; + status_t writeToParcel(android::Parcel* parcel) const override; + + status_t writeReleaseFence(const ReleaseCallbackId&, const sp& releaseFence, + uint32_t maxAcquiredBufferCount); + + private: + std::vector mFlattenedBuffer; + }; + + /** + * Create two endpoints that make up the BufferReleaseChannel. + * + * Return OK on success. + */ + static status_t open(const std::string name, std::unique_ptr& outConsumer, + std::shared_ptr& outProducer); + + struct Message : public Flattenable { + ReleaseCallbackId releaseCallbackId; + sp releaseFence = Fence::NO_FENCE; + uint32_t maxAcquiredBufferCount; + + Message() = default; + Message(ReleaseCallbackId releaseCallbackId, sp releaseFence, + uint32_t maxAcquiredBufferCount) + : releaseCallbackId{releaseCallbackId}, + releaseFence{std::move(releaseFence)}, + maxAcquiredBufferCount{maxAcquiredBufferCount} {} + + // Flattenable protocol + size_t getFlattenedSize() const; + + size_t getFdCount() const { return releaseFence->getFdCount(); } + + status_t flatten(void*& buffer, size_t& size, int*& fds, size_t& count) const; + + status_t unflatten(void const*& buffer, size_t& size, int const*& fds, size_t& count); + + private: + size_t getPodSize() const; + }; +}; + +} // namespace android::gui diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 3fb1894585..d41994589f 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -220,6 +221,7 @@ struct layer_state_t { eDropInputModeChanged = 0x8000'00000000, eExtendedRangeBrightnessChanged = 0x10000'00000000, eEdgeExtensionChanged = 0x20000'00000000, + eBufferReleaseChannelChanged = 0x40000'00000000, }; layer_state_t(); @@ -412,6 +414,8 @@ struct layer_state_t { TrustedPresentationThresholds trustedPresentationThresholds; TrustedPresentationListener trustedPresentationListener; + + std::shared_ptr bufferReleaseChannel; }; class ComposerState { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 61a65bb29f..4f9af16826 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -45,6 +45,7 @@ #include #include +#include #include #include #include @@ -762,6 +763,10 @@ public: const Rect& destinationFrame); Transaction& setDropInputMode(const sp& sc, gui::DropInputMode mode); + Transaction& setBufferReleaseChannel( + const sp& sc, + const std::shared_ptr& channel); + status_t setDisplaySurface(const sp& token, const sp& bufferProducer); diff --git a/libs/gui/tests/Android.bp b/libs/gui/tests/Android.bp index b342a7db0d..9558edab87 100644 --- a/libs/gui/tests/Android.bp +++ b/libs/gui/tests/Android.bp @@ -33,6 +33,7 @@ cc_test { "BLASTBufferQueue_test.cpp", "BufferItemConsumer_test.cpp", "BufferQueue_test.cpp", + "BufferReleaseChannel_test.cpp", "Choreographer_test.cpp", "CompositorTiming_test.cpp", "CpuConsumer_test.cpp", diff --git a/libs/gui/tests/BufferReleaseChannel_test.cpp b/libs/gui/tests/BufferReleaseChannel_test.cpp new file mode 100644 index 0000000000..11d122b525 --- /dev/null +++ b/libs/gui/tests/BufferReleaseChannel_test.cpp @@ -0,0 +1,138 @@ +/* + * Copyright (C) 2024 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. + */ + +#include +#include + +#include +#include + +using namespace std::string_literals; +using android::gui::BufferReleaseChannel; + +namespace android { + +namespace { + +// Helper function to check if two file descriptors point to the same file. +bool is_same_file(int fd1, int fd2) { + struct stat stat1; + if (fstat(fd1, &stat1) != 0) { + return false; + } + struct stat stat2; + if (fstat(fd2, &stat2) != 0) { + return false; + } + return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino); +} + +} // namespace + +TEST(BufferReleaseChannelTest, MessageFlattenable) { + ReleaseCallbackId releaseCallbackId{1, 2}; + sp releaseFence = sp::make(memfd_create("fake-fence-fd", 0)); + uint32_t maxAcquiredBufferCount = 5; + + std::vector dataBuffer; + std::vector fdBuffer; + + // Verify that we can flatten a message + { + BufferReleaseChannel::Message message{releaseCallbackId, releaseFence, + maxAcquiredBufferCount}; + + dataBuffer.resize(message.getFlattenedSize()); + void* dataPtr = dataBuffer.data(); + size_t dataSize = dataBuffer.size(); + + fdBuffer.resize(message.getFdCount()); + int* fdPtr = fdBuffer.data(); + size_t fdSize = fdBuffer.size(); + + ASSERT_EQ(OK, message.flatten(dataPtr, dataSize, fdPtr, fdSize)); + + // Fence's unique_fd uses fdsan to check ownership of the file descriptor. Normally the file + // descriptor is passed through the Unix socket and duplicated (and sent to another process) + // so there's no problem with duplicate file descriptor ownership. For this unit test, we + // need to set up a duplicate file descriptor to avoid crashing due to duplicate ownership. + ASSERT_EQ(releaseFence->get(), fdBuffer[0]); + fdBuffer[0] = message.releaseFence->dup(); + } + + // Verify that we can unflatten a message + { + BufferReleaseChannel::Message message; + + const void* dataPtr = dataBuffer.data(); + size_t dataSize = dataBuffer.size(); + + const int* fdPtr = fdBuffer.data(); + size_t fdSize = fdBuffer.size(); + + ASSERT_EQ(OK, message.unflatten(dataPtr, dataSize, fdPtr, fdSize)); + ASSERT_EQ(releaseCallbackId, message.releaseCallbackId); + ASSERT_TRUE(is_same_file(releaseFence->get(), message.releaseFence->get())); + ASSERT_EQ(maxAcquiredBufferCount, message.maxAcquiredBufferCount); + } +} + +// Verify that the BufferReleaseChannel consume returns WOULD_BLOCK when there's no message +// available. +TEST(BufferReleaseChannelTest, ConsumerEndpointIsNonBlocking) { + std::unique_ptr consumer; + std::shared_ptr producer; + ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); + + ReleaseCallbackId releaseCallbackId; + sp releaseFence; + uint32_t maxAcquiredBufferCount; + ASSERT_EQ(WOULD_BLOCK, + consumer->readReleaseFence(releaseCallbackId, releaseFence, maxAcquiredBufferCount)); +} + +// Verify that we can write a message to the BufferReleaseChannel producer and read that message +// using the BufferReleaseChannel consumer. +TEST(BufferReleaseChannelTest, ProduceAndConsume) { + std::unique_ptr consumer; + std::shared_ptr producer; + ASSERT_EQ(OK, BufferReleaseChannel::open("test-channel"s, consumer, producer)); + + sp fence = sp::make(memfd_create("fake-fence-fd", 0)); + + for (uint64_t i = 0; i < 64; i++) { + ReleaseCallbackId producerId{i, i + 1}; + uint32_t maxAcquiredBufferCount = i + 2; + ASSERT_EQ(OK, producer->writeReleaseFence(producerId, fence, maxAcquiredBufferCount)); + } + + for (uint64_t i = 0; i < 64; i++) { + ReleaseCallbackId expectedId{i, i + 1}; + uint32_t expectedMaxAcquiredBufferCount = i + 2; + + ReleaseCallbackId consumerId; + sp consumerFence; + uint32_t maxAcquiredBufferCount; + ASSERT_EQ(OK, + consumer->readReleaseFence(consumerId, consumerFence, maxAcquiredBufferCount)); + + ASSERT_EQ(expectedId, consumerId); + ASSERT_TRUE(is_same_file(fence->get(), consumerFence->get())); + ASSERT_EQ(expectedMaxAcquiredBufferCount, maxAcquiredBufferCount); + } +} + +} // namespace android \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From 924f9500f771a93a7a45bd92bf0d1bd0d8b8e583 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 15 Aug 2024 20:01:08 +0000 Subject: Rename AidlStatusUtil.h to AidlUtil.h Shoving some utility methods to help with transitioning between AIDL types and the types we use internally Bug: 329465218 Flag: EXEMPT mechanical refactor Test: builds Change-Id: I07cdfb7c6bfe89e42c1eb7169e15329916df448a --- libs/gui/Surface.cpp | 2 +- libs/gui/SurfaceComposerClient.cpp | 2 +- libs/gui/WindowInfosListenerReporter.cpp | 2 +- libs/gui/include/gui/AidlStatusUtil.h | 138 --------------------- libs/gui/include/gui/AidlUtil.h | 138 +++++++++++++++++++++ libs/gui/tests/BLASTBufferQueue_test.cpp | 2 +- libs/gui/tests/RegionSampling_test.cpp | 2 +- libs/gui/tests/Surface_test.cpp | 2 +- libs/gui/tests/TestServer_test.cpp | 2 +- services/surfaceflinger/Client.cpp | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 2 +- .../surfaceflinger/tests/BootDisplayMode_test.cpp | 2 +- services/surfaceflinger/tests/Credentials_test.cpp | 2 +- .../surfaceflinger/tests/LayerTransactionTest.h | 2 +- .../tests/LayerTypeTransaction_test.cpp | 2 +- services/surfaceflinger/tests/MirrorLayer_test.cpp | 2 +- .../surfaceflinger/tests/ScreenCapture_test.cpp | 2 +- .../surfaceflinger/tests/TextureFiltering_test.cpp | 2 +- .../SurfaceFlinger_HdrOutputControlTest.cpp | 2 +- .../surfaceflinger/tests/utils/ScreenshotUtils.h | 2 +- 20 files changed, 156 insertions(+), 156 deletions(-) delete mode 100644 libs/gui/include/gui/AidlStatusUtil.h create mode 100644 libs/gui/include/gui/AidlUtil.h (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp index 4a8df9e1bb..da3886ccf0 100644 --- a/libs/gui/Surface.cpp +++ b/libs/gui/Surface.cpp @@ -43,7 +43,7 @@ #include #include -#include +#include #include #include diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index b5d9366185..7d3e5c166f 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -43,7 +43,7 @@ #include -#include +#include #include #include #include diff --git a/libs/gui/WindowInfosListenerReporter.cpp b/libs/gui/WindowInfosListenerReporter.cpp index 0929b8e120..91c9a85149 100644 --- a/libs/gui/WindowInfosListenerReporter.cpp +++ b/libs/gui/WindowInfosListenerReporter.cpp @@ -15,7 +15,7 @@ */ #include -#include +#include #include #include "gui/WindowInfosUpdate.h" diff --git a/libs/gui/include/gui/AidlStatusUtil.h b/libs/gui/include/gui/AidlStatusUtil.h deleted file mode 100644 index 159e0329ac..0000000000 --- a/libs/gui/include/gui/AidlStatusUtil.h +++ /dev/null @@ -1,138 +0,0 @@ -/* - * Copyright (C) 2022 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. - */ - -#pragma once - -#include -#include -#include - -// Originally extracted from frameworks/av/media/libaudioclient/include/media/AidlConversionUtil.h -namespace android::gui::aidl_utils { - -/** - * Return the equivalent Android status_t from a binder exception code. - * - * Generally one should use statusTFromBinderStatus() instead. - * - * Exception codes can be generated from a remote Java service exception, translate - * them for use on the Native side. - * - * Note: for EX_TRANSACTION_FAILED and EX_SERVICE_SPECIFIC a more detailed error code - * can be found from transactionError() or serviceSpecificErrorCode(). - */ -static inline status_t statusTFromExceptionCode(int32_t exceptionCode) { - using namespace ::android::binder; - switch (exceptionCode) { - case Status::EX_NONE: - return OK; - case Status::EX_SECURITY: // Java SecurityException, rethrows locally in Java - return PERMISSION_DENIED; - case Status::EX_BAD_PARCELABLE: // Java BadParcelableException, rethrows in Java - case Status::EX_ILLEGAL_ARGUMENT: // Java IllegalArgumentException, rethrows in Java - case Status::EX_NULL_POINTER: // Java NullPointerException, rethrows in Java - return BAD_VALUE; - case Status::EX_ILLEGAL_STATE: // Java IllegalStateException, rethrows in Java - case Status::EX_UNSUPPORTED_OPERATION: // Java UnsupportedOperationException, rethrows - return INVALID_OPERATION; - case Status::EX_HAS_REPLY_HEADER: // Native strictmode violation - case Status::EX_PARCELABLE: // Java bootclass loader (not standard exception), rethrows - case Status::EX_NETWORK_MAIN_THREAD: // Java NetworkOnMainThreadException, rethrows - case Status::EX_TRANSACTION_FAILED: // Native - see error code - case Status::EX_SERVICE_SPECIFIC: // Java ServiceSpecificException, - // rethrows in Java with integer error code - return UNKNOWN_ERROR; - } - return UNKNOWN_ERROR; -} - -/** - * Return the equivalent Android status_t from a binder status. - * - * Used to handle errors from a AIDL method declaration - * - * [oneway] void method(type0 param0, ...) - * - * or the following (where return_type is not a status_t) - * - * return_type method(type0 param0, ...) - */ -static inline status_t statusTFromBinderStatus(const ::android::binder::Status &status) { - return status.isOk() ? OK // check OK, - : status.serviceSpecificErrorCode() // service-side error, not standard Java exception - // (fromServiceSpecificError) - ?: status.transactionError() // a native binder transaction error (fromStatusT) - ?: statusTFromExceptionCode(status.exceptionCode()); // a service-side error with a - // standard Java exception (fromExceptionCode) -} - -/** - * Return a binder::Status from native service status. - * - * This is used for methods not returning an explicit status_t, - * where Java callers expect an exception, not an integer return value. - */ -static inline ::android::binder::Status binderStatusFromStatusT( - status_t status, const char *optionalMessage = nullptr) { - const char *const emptyIfNull = optionalMessage == nullptr ? "" : optionalMessage; - // From binder::Status instructions: - // Prefer a generic exception code when possible, then a service specific - // code, and finally a status_t for low level failures or legacy support. - // Exception codes and service specific errors map to nicer exceptions for - // Java clients. - - using namespace ::android::binder; - switch (status) { - case OK: - return Status::ok(); - case PERMISSION_DENIED: // throw SecurityException on Java side - return Status::fromExceptionCode(Status::EX_SECURITY, emptyIfNull); - case BAD_VALUE: // throw IllegalArgumentException on Java side - return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT, emptyIfNull); - case INVALID_OPERATION: // throw IllegalStateException on Java side - return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE, emptyIfNull); - } - - // A service specific error will not show on status.transactionError() so - // be sure to use statusTFromBinderStatus() for reliable error handling. - - // throw a ServiceSpecificException. - return Status::fromServiceSpecificError(status, emptyIfNull); -} - -static inline Rect fromARect(ARect rect) { - return Rect(rect.left, rect.top, rect.right, rect.bottom); -} - -static inline ARect toARect(Rect rect) { - ARect aRect; - - aRect.left = rect.left; - aRect.top = rect.top; - aRect.right = rect.right; - aRect.bottom = rect.bottom; - return aRect; -} - -static inline ARect toARect(int32_t left, int32_t top, int32_t right, int32_t bottom) { - return toARect(Rect(left, top, right, bottom)); -} - -static inline ARect toARect(int32_t width, int32_t height) { - return toARect(Rect(width, height)); -} - -} // namespace android::gui::aidl_utils diff --git a/libs/gui/include/gui/AidlUtil.h b/libs/gui/include/gui/AidlUtil.h new file mode 100644 index 0000000000..a3ecd84ba1 --- /dev/null +++ b/libs/gui/include/gui/AidlUtil.h @@ -0,0 +1,138 @@ +/* + * Copyright (C) 2022 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. + */ + +#pragma once + +#include +#include +#include + +// Originally extracted from frameworks/av/media/libaudioclient/include/media/AidlConversionUtil.h +namespace android::gui::aidl_utils { + +/** + * Return the equivalent Android status_t from a binder exception code. + * + * Generally one should use statusTFromBinderStatus() instead. + * + * Exception codes can be generated from a remote Java service exception, translate + * them for use on the Native side. + * + * Note: for EX_TRANSACTION_FAILED and EX_SERVICE_SPECIFIC a more detailed error code + * can be found from transactionError() or serviceSpecificErrorCode(). + */ +static inline status_t statusTFromExceptionCode(int32_t exceptionCode) { + using namespace ::android::binder; + switch (exceptionCode) { + case Status::EX_NONE: + return OK; + case Status::EX_SECURITY: // Java SecurityException, rethrows locally in Java + return PERMISSION_DENIED; + case Status::EX_BAD_PARCELABLE: // Java BadParcelableException, rethrows in Java + case Status::EX_ILLEGAL_ARGUMENT: // Java IllegalArgumentException, rethrows in Java + case Status::EX_NULL_POINTER: // Java NullPointerException, rethrows in Java + return BAD_VALUE; + case Status::EX_ILLEGAL_STATE: // Java IllegalStateException, rethrows in Java + case Status::EX_UNSUPPORTED_OPERATION: // Java UnsupportedOperationException, rethrows + return INVALID_OPERATION; + case Status::EX_HAS_REPLY_HEADER: // Native strictmode violation + case Status::EX_PARCELABLE: // Java bootclass loader (not standard exception), rethrows + case Status::EX_NETWORK_MAIN_THREAD: // Java NetworkOnMainThreadException, rethrows + case Status::EX_TRANSACTION_FAILED: // Native - see error code + case Status::EX_SERVICE_SPECIFIC: // Java ServiceSpecificException, + // rethrows in Java with integer error code + return UNKNOWN_ERROR; + } + return UNKNOWN_ERROR; +} + +/** + * Return the equivalent Android status_t from a binder status. + * + * Used to handle errors from a AIDL method declaration + * + * [oneway] void method(type0 param0, ...) + * + * or the following (where return_type is not a status_t) + * + * return_type method(type0 param0, ...) + */ +static inline status_t statusTFromBinderStatus(const ::android::binder::Status& status) { + return status.isOk() ? OK // check OK, + : status.serviceSpecificErrorCode() // service-side error, not standard Java exception + // (fromServiceSpecificError) + ?: status.transactionError() // a native binder transaction error (fromStatusT) + ?: statusTFromExceptionCode(status.exceptionCode()); // a service-side error with a + // standard Java exception (fromExceptionCode) +} + +/** + * Return a binder::Status from native service status. + * + * This is used for methods not returning an explicit status_t, + * where Java callers expect an exception, not an integer return value. + */ +static inline ::android::binder::Status binderStatusFromStatusT( + status_t status, const char* optionalMessage = nullptr) { + const char* const emptyIfNull = optionalMessage == nullptr ? "" : optionalMessage; + // From binder::Status instructions: + // Prefer a generic exception code when possible, then a service specific + // code, and finally a status_t for low level failures or legacy support. + // Exception codes and service specific errors map to nicer exceptions for + // Java clients. + + using namespace ::android::binder; + switch (status) { + case OK: + return Status::ok(); + case PERMISSION_DENIED: // throw SecurityException on Java side + return Status::fromExceptionCode(Status::EX_SECURITY, emptyIfNull); + case BAD_VALUE: // throw IllegalArgumentException on Java side + return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT, emptyIfNull); + case INVALID_OPERATION: // throw IllegalStateException on Java side + return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE, emptyIfNull); + } + + // A service specific error will not show on status.transactionError() so + // be sure to use statusTFromBinderStatus() for reliable error handling. + + // throw a ServiceSpecificException. + return Status::fromServiceSpecificError(status, emptyIfNull); +} + +static inline Rect fromARect(ARect rect) { + return Rect(rect.left, rect.top, rect.right, rect.bottom); +} + +static inline ARect toARect(Rect rect) { + ARect aRect; + + aRect.left = rect.left; + aRect.top = rect.top; + aRect.right = rect.right; + aRect.bottom = rect.bottom; + return aRect; +} + +static inline ARect toARect(int32_t left, int32_t top, int32_t right, int32_t bottom) { + return toARect(Rect(left, top, right, bottom)); +} + +static inline ARect toARect(int32_t width, int32_t height) { + return toARect(Rect(width, height)); +} + +} // namespace android::gui::aidl_utils diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp index 46a19c273d..eb2a61d151 100644 --- a/libs/gui/tests/BLASTBufferQueue_test.cpp +++ b/libs/gui/tests/BLASTBufferQueue_test.cpp @@ -20,7 +20,7 @@ #include #include -#include +#include #include #include #include diff --git a/libs/gui/tests/RegionSampling_test.cpp b/libs/gui/tests/RegionSampling_test.cpp index 223e4b6cbd..a0d8c53385 100644 --- a/libs/gui/tests/RegionSampling_test.cpp +++ b/libs/gui/tests/RegionSampling_test.cpp @@ -19,7 +19,7 @@ #include #include -#include +#include #include #include #include diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 4232443308..ab09dfc58d 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -28,7 +28,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/libs/gui/tests/TestServer_test.cpp b/libs/gui/tests/TestServer_test.cpp index 8712988e52..d6407820ff 100644 --- a/libs/gui/tests/TestServer_test.cpp +++ b/libs/gui/tests/TestServer_test.cpp @@ -24,7 +24,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 6b4215e2f4..abeb2a92eb 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -21,7 +21,7 @@ #include -#include +#include #include #include "Client.h" diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 1e374c0a05..e8d64d92ae 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -64,7 +64,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/tests/BootDisplayMode_test.cpp b/services/surfaceflinger/tests/BootDisplayMode_test.cpp index 4f41a81011..222642f50c 100644 --- a/services/surfaceflinger/tests/BootDisplayMode_test.cpp +++ b/services/surfaceflinger/tests/BootDisplayMode_test.cpp @@ -18,7 +18,7 @@ #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index babbcd4e35..e6fed63d96 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -20,7 +20,7 @@ #include #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/tests/LayerTransactionTest.h b/services/surfaceflinger/tests/LayerTransactionTest.h index 5b056d0765..03f900526d 100644 --- a/services/surfaceflinger/tests/LayerTransactionTest.h +++ b/services/surfaceflinger/tests/LayerTransactionTest.h @@ -23,7 +23,7 @@ #include #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/tests/LayerTypeTransaction_test.cpp b/services/surfaceflinger/tests/LayerTypeTransaction_test.cpp index e655df7695..76bae4116c 100644 --- a/services/surfaceflinger/tests/LayerTypeTransaction_test.cpp +++ b/services/surfaceflinger/tests/LayerTypeTransaction_test.cpp @@ -15,10 +15,10 @@ */ // TODO(b/129481165): remove the #pragma below and fix conversion issues -#include "gui/AidlStatusUtil.h" #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wconversion" +#include #include #include #include "TransactionTestHarnesses.h" diff --git a/services/surfaceflinger/tests/MirrorLayer_test.cpp b/services/surfaceflinger/tests/MirrorLayer_test.cpp index 2a535881b2..6cc1c51cde 100644 --- a/services/surfaceflinger/tests/MirrorLayer_test.cpp +++ b/services/surfaceflinger/tests/MirrorLayer_test.cpp @@ -20,7 +20,7 @@ #include #include -#include +#include #include #include "LayerTransactionTest.h" #include "utils/TransactionUtils.h" diff --git a/services/surfaceflinger/tests/ScreenCapture_test.cpp b/services/surfaceflinger/tests/ScreenCapture_test.cpp index 069f199cab..c62f493941 100644 --- a/services/surfaceflinger/tests/ScreenCapture_test.cpp +++ b/services/surfaceflinger/tests/ScreenCapture_test.cpp @@ -20,7 +20,7 @@ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wconversion" -#include +#include #include #include diff --git a/services/surfaceflinger/tests/TextureFiltering_test.cpp b/services/surfaceflinger/tests/TextureFiltering_test.cpp index f8c536430a..3f39cf6eea 100644 --- a/services/surfaceflinger/tests/TextureFiltering_test.cpp +++ b/services/surfaceflinger/tests/TextureFiltering_test.cpp @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HdrOutputControlTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HdrOutputControlTest.cpp index db6df229d5..4bc134fae8 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HdrOutputControlTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HdrOutputControlTest.cpp @@ -18,7 +18,7 @@ #define LOG_TAG "LibSurfaceFlingerUnittests" #include -#include +#include #include #include diff --git a/services/surfaceflinger/tests/utils/ScreenshotUtils.h b/services/surfaceflinger/tests/utils/ScreenshotUtils.h index efce6b6b44..0bedcd174e 100644 --- a/services/surfaceflinger/tests/utils/ScreenshotUtils.h +++ b/services/surfaceflinger/tests/utils/ScreenshotUtils.h @@ -15,7 +15,7 @@ */ #pragma once -#include +#include #include #include #include -- cgit v1.2.3-59-g8ed1b From 5312ec10e10f9cb0c2b91ac85b402de4e92fe4e6 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Fri, 23 Aug 2024 16:11:10 -0500 Subject: Replace BBQ callback thunks with lambdas The lambdas allows for better StrongPointer/RefBase safety by replacing explicit incStrong/decStrong calls with RAII. Captured StrongPointers are created using sp::fromExisting which helps detect RefBase errors such as creating a StrongPointer from an already deleted RefBase object. Bug: 361588984 Flag: EXEMPT refactor Test: presubmits Change-Id: I1a0c0df882ec2a3f91f857c446418f3fb51689c1 --- libs/gui/BLASTBufferQueue.cpp | 65 ++++++++++++++------------------- libs/gui/SurfaceComposerClient.cpp | 11 ++++-- libs/gui/include/gui/BLASTBufferQueue.h | 6 +++ 3 files changed, 41 insertions(+), 41 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 1fb59fd8e0..f8e3fd0a5c 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -306,14 +306,12 @@ static std::optional findMatchingStat( return std::nullopt; } -static void transactionCommittedCallbackThunk(void* context, nsecs_t latchTime, - const sp& presentFence, - const std::vector& stats) { - if (context == nullptr) { - return; - } - sp bq = static_cast(context); - bq->transactionCommittedCallback(latchTime, presentFence, stats); +TransactionCompletedCallbackTakesContext BLASTBufferQueue::makeTransactionCommittedCallbackThunk() { + return [bbq = sp::fromExisting( + this)](void* /*context*/, nsecs_t latchTime, const sp& presentFence, + const std::vector& stats) { + bbq->transactionCommittedCallback(latchTime, presentFence, stats); + }; } void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, @@ -346,19 +344,15 @@ void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, BQA_LOGE("No matching SurfaceControls found: mSurfaceControlsWithPendingCallback was " "empty."); } - decStrong((void*)transactionCommittedCallbackThunk); } } -static void transactionCallbackThunk(void* context, nsecs_t latchTime, - const sp& presentFence, - const std::vector& stats) { - if (context == nullptr) { - return; - } - auto bq = static_cast(context); - bq->transactionCallback(latchTime, presentFence, stats); - bq->decStrong((void*)transactionCallbackThunk); +TransactionCompletedCallbackTakesContext BLASTBufferQueue::makeTransactionCallbackThunk() { + return [bbq = sp::fromExisting( + this)](void* /*context*/, nsecs_t latchTime, const sp& presentFence, + const std::vector& stats) { + bbq->transactionCallback(latchTime, presentFence, stats); + }; } void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp& /*presentFence*/, @@ -421,15 +415,17 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp context, const ReleaseCallbackId& id, - const sp& releaseFence, - std::optional currentMaxAcquiredBufferCount) { - sp blastBufferQueue = context.promote(); - if (blastBufferQueue) { - blastBufferQueue->releaseBufferCallback(id, releaseFence, currentMaxAcquiredBufferCount); - } else { - ALOGV("releaseBufferCallbackThunk %s blastBufferQueue is dead", id.to_string().c_str()); - } +ReleaseBufferCallback BLASTBufferQueue::makeReleaseBufferCallbackThunk() { + return [weakBbq = wp::fromExisting( + this)](const ReleaseCallbackId& id, const sp& releaseFence, + std::optional currentMaxAcquiredBufferCount) { + sp bbq = weakBbq.promote(); + if (!bbq) { + ALOGV("releaseBufferCallbackThunk %s blastBufferQueue is dead", id.to_string().c_str()); + return; + } + bbq->releaseBufferCallback(id, releaseFence, currentMaxAcquiredBufferCount); + }; } void BLASTBufferQueue::flushShadowQueue() { @@ -609,9 +605,6 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( t->notifyProducerDisconnect(mSurfaceControl); } - // Ensure BLASTBufferQueue stays alive until we receive the transaction complete callback. - incStrong((void*)transactionCallbackThunk); - // Only update mSize for destination bounds if the incoming buffer matches the requested size. // Otherwise, it could cause stretching since the destination bounds will update before the // buffer with the new size is acquired. @@ -624,9 +617,7 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( bufferItem.mGraphicBuffer->getHeight(), bufferItem.mTransform, bufferItem.mScalingMode, crop); - auto releaseBufferCallback = - std::bind(releaseBufferCallbackThunk, wp(this) /* callbackContext */, - std::placeholders::_1, std::placeholders::_2, std::placeholders::_3); + auto releaseBufferCallback = makeReleaseBufferCallbackThunk(); sp fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE; nsecs_t dequeueTime = -1; @@ -644,7 +635,7 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( t->setDataspace(mSurfaceControl, static_cast(bufferItem.mDataSpace)); t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata); t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage); - t->addTransactionCompletedCallback(transactionCallbackThunk, static_cast(this)); + t->addTransactionCompletedCallback(makeTransactionCallbackThunk(), nullptr); mSurfaceControlsWithPendingCallback.push(mSurfaceControl); @@ -804,9 +795,9 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { // Only need a commit callback when syncing to ensure the buffer that's synced has been // sent to SF - incStrong((void*)transactionCommittedCallbackThunk); - mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk, - static_cast(this)); + mSyncTransaction + ->addTransactionCommittedCallback(makeTransactionCommittedCallbackThunk(), + nullptr); if (mAcquireSingleBuffer) { prevCallback = mTransactionReadyCallback; prevTransaction = mSyncTransaction; diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 7d3e5c166f..df58df43be 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2051,8 +2051,9 @@ SurfaceComposerClient::Transaction::setFrameRateSelectionPriority(const sp& presentFence, const std::vector& stats); + + TransactionCompletedCallbackTakesContext makeTransactionCallbackThunk(); virtual void transactionCallback(nsecs_t latchTime, const sp& presentFence, const std::vector& stats); + + ReleaseBufferCallback makeReleaseBufferCallbackThunk(); void releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount); void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount, bool fakeRelease) REQUIRES(mMutex); + bool syncNextTransaction(std::function callback, bool acquireSingleBuffer = true); void stopContinuousSyncTransaction(); -- cgit v1.2.3-59-g8ed1b