From bbf362d24642ed22fd7d19f428125c8bd9e3f170 Mon Sep 17 00:00:00 2001 From: Melody Hsu Date: Wed, 20 Mar 2024 21:59:06 +0000 Subject: Delete border rendering code from SurfaceFlinger. Removed code is never used and drawing borders is done instead by Window Manager Service. Changes revert ag/16980603 and ag/17496275. Bug: b/227656283 Test: presubmit Test: SurfaceFlinger_test Change-Id: Ib5c8bf74ad6764d65536dc60cc3c458edde86b3f --- libs/gui/SurfaceComposerClient.cpp | 17 ----------------- 1 file changed, 17 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 4f1356bebb..1d2ea3ea02 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2250,23 +2250,6 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDropI return *this; } -SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::enableBorder( - const sp& sc, bool shouldEnable, float width, const half4& color) { - layer_state_t* s = getLayerState(sc); - if (!s) { - mStatus = BAD_INDEX; - return *this; - } - - s->what |= layer_state_t::eRenderBorderChanged; - s->borderEnabled = shouldEnable; - s->borderWidth = width; - s->borderColor = color; - - registerSurfaceControlForCallback(sc); - return *this; -} - // --------------------------------------------------------------------------- DisplayState& SurfaceComposerClient::Transaction::getDisplayState(const sp& token) { -- cgit v1.2.3-59-g8ed1b From a74ba0f4f2e8766751fb3cf23b2c1dbcda476506 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 16 Apr 2024 11:25:06 -0700 Subject: Avoid extra locking in TransactionCompletedListener::getInstance Change-Id: I823da204e3ac94b5ad7fec8a16f16f7de57f2d49 Fixes: 333947892 Test: presubmit --- libs/gui/SurfaceComposerClient.cpp | 39 ++++++++++++++-------------- libs/gui/include/gui/SurfaceComposerClient.h | 2 ++ 2 files changed, 21 insertions(+), 20 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index eb945ccce0..3743025b7b 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -706,6 +706,7 @@ void removeDeadBufferCallback(void* /*context*/, uint64_t graphicBufferId) { SurfaceComposerClient::Transaction::Transaction() { mId = generateId(); + mTransactionCompletedListener = TransactionCompletedListener::getInstance(); } SurfaceComposerClient::Transaction::Transaction(const Transaction& other) @@ -723,6 +724,7 @@ SurfaceComposerClient::Transaction::Transaction(const Transaction& other) mComposerStates = other.mComposerStates; mInputWindowCommands = other.mInputWindowCommands; mListenerCallbacks = other.mListenerCallbacks; + mTransactionCompletedListener = TransactionCompletedListener::getInstance(); } void SurfaceComposerClient::Transaction::sanitize(int pid, int uid) { @@ -1000,8 +1002,8 @@ 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) { - TransactionCompletedListener::getInstance() - ->addSurfaceControlToCallbacks(currentProcessCallbackInfo, surfaceControl); + mTransactionCompletedListener->addSurfaceControlToCallbacks(currentProcessCallbackInfo, + surfaceControl); } } @@ -1354,7 +1356,7 @@ void SurfaceComposerClient::Transaction::registerSurfaceControlForCallback( auto& callbackInfo = mListenerCallbacks[TransactionCompletedListener::getIInstance()]; callbackInfo.surfaceControls.insert(sc); - TransactionCompletedListener::getInstance()->addSurfaceControlToCallbacks(callbackInfo, sc); + mTransactionCompletedListener->addSurfaceControlToCallbacks(callbackInfo, sc); } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setPosition( @@ -1672,7 +1674,7 @@ std::shared_ptr SurfaceComposerClient::Transaction::getAndClearBuffe std::shared_ptr bufferData = std::move(s->bufferData); - TransactionCompletedListener::getInstance()->removeReleaseBufferCallback( + mTransactionCompletedListener->removeReleaseBufferCallback( bufferData->generateReleaseCallbackId()); s->what &= ~layer_state_t::eBufferChanged; s->bufferData = nullptr; @@ -1715,8 +1717,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe bufferData->acquireFence = *fence; bufferData->flags |= BufferData::BufferDataChange::fenceChanged; } - bufferData->releaseBufferEndpoint = - IInterface::asBinder(TransactionCompletedListener::getIInstance()); + bufferData->releaseBufferEndpoint = IInterface::asBinder(mTransactionCompletedListener); setReleaseBufferCallback(bufferData.get(), callback); } @@ -1774,9 +1775,10 @@ void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bu return; } - bufferData->releaseBufferListener = TransactionCompletedListener::getIInstance(); - auto listener = TransactionCompletedListener::getInstance(); - listener->setReleaseBufferCallback(bufferData->generateReleaseCallbackId(), callback); + bufferData->releaseBufferListener = + static_cast>(mTransactionCompletedListener); + mTransactionCompletedListener->setReleaseBufferCallback(bufferData->generateReleaseCallbackId(), + callback); } SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDataspace( @@ -1932,18 +1934,15 @@ SurfaceComposerClient::Transaction::setFrameRateSelectionPriority(const spaddCallbackFunction(callbackWithContext, surfaceControls, callbackType); + mTransactionCompletedListener->addCallbackFunction(callbackWithContext, surfaceControls, + callbackType); - mListenerCallbacks[TransactionCompletedListener::getIInstance()].callbackIds.emplace( - callbackId); + mListenerCallbacks[mTransactionCompletedListener].callbackIds.emplace(callbackId); return *this; } @@ -2333,8 +2332,9 @@ SurfaceComposerClient::Transaction::setTrustedPresentationCallback( const sp& sc, TrustedPresentationCallback cb, const TrustedPresentationThresholds& thresholds, void* context, sp& outCallbackRef) { - auto listener = TransactionCompletedListener::getInstance(); - outCallbackRef = listener->addTrustedPresentationCallback(cb, sc->getLayerId(), context); + outCallbackRef = + mTransactionCompletedListener->addTrustedPresentationCallback(cb, sc->getLayerId(), + context); layer_state_t* s = getLayerState(sc); if (!s) { @@ -2351,8 +2351,7 @@ SurfaceComposerClient::Transaction::setTrustedPresentationCallback( SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::clearTrustedPresentationCallback(const sp& sc) { - auto listener = TransactionCompletedListener::getInstance(); - listener->clearTrustedPresentationCallback(sc->getLayerId()); + mTransactionCompletedListener->clearTrustedPresentationCallback(sc->getLayerId()); layer_state_t* s = getLayerState(sc); if (!s) { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 79224e6782..49b0a7d0c9 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -431,6 +431,8 @@ public: static std::mutex sApplyTokenMutex; void releaseBufferIfOverwriting(const layer_state_t& state); static void mergeFrameTimelineInfo(FrameTimelineInfo& t, const FrameTimelineInfo& other); + // Tracks registered callbacks + sp mTransactionCompletedListener = nullptr; protected: std::unordered_map, ComposerState, IBinderHash> mComposerStates; -- cgit v1.2.3-59-g8ed1b From fc6eab5d5b3c6372118ba5193bcf5ea7e73a0a67 Mon Sep 17 00:00:00 2001 From: Alan Ding Date: Thu, 2 May 2024 18:28:45 -0700 Subject: SF: Propagate uniqueID when creating virtual displays This is part of Trunk Stable effort to upstream SF-ARC screen record capabiliy (undiverging http://ag/20003031) which requires mirroring virtual displays to be associated with screen capture sessions using the package name within the unique ID. Creating virtual display with unique ID specified is optional such that it doesn't affect existing consumers who don't need it (i.e. av). Bug: 137375833 Bug: 194863377 Test: libsurfaceflinger_unittest Change-Id: Ia3cd13df07f701593ddc94c196df0b04844cf502 --- libs/gui/SurfaceComposerClient.cpp | 15 ++++++----- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 18 +++++++------ libs/gui/include/gui/SurfaceComposerClient.h | 11 ++++---- libs/gui/tests/Surface_test.cpp | 4 +-- services/surfaceflinger/DisplayDevice.h | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 24 +++++++++-------- services/surfaceflinger/SurfaceFlinger.h | 24 +++++++++-------- .../unittests/SurfaceFlinger_CreateDisplayTest.cpp | 30 ++++++++++++++++++++++ .../tests/unittests/TestableSurfaceFlinger.h | 11 ++++++-- 9 files changed, 91 insertions(+), 47 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 4f1356bebb..07664ca701 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -89,6 +89,8 @@ int64_t generateId() { void emptyCallback(nsecs_t, const sp&, const std::vector&) {} } // namespace +const std::string SurfaceComposerClient::kEmpty{}; + ComposerService::ComposerService() : Singleton() { Mutex::Autolock _l(mLock); @@ -1276,14 +1278,13 @@ status_t SurfaceComposerClient::Transaction::sendSurfaceFlushJankDataTransaction } // --------------------------------------------------------------------------- -sp SurfaceComposerClient::createDisplay(const String8& displayName, bool secure, - float requestedRefereshRate) { +sp SurfaceComposerClient::createDisplay(const String8& displayName, bool isSecure, + const std::string& uniqueId, + float requestedRefreshRate) { sp display = nullptr; - binder::Status status = - ComposerServiceAIDL::getComposerService()->createDisplay(std::string( - displayName.c_str()), - secure, requestedRefereshRate, - &display); + binder::Status status = ComposerServiceAIDL::getComposerService() + ->createDisplay(std::string(displayName.c_str()), isSecure, + uniqueId, requestedRefreshRate, &display); return status.isOk() ? display : nullptr; } diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index 51e01930d3..cf6752f818 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -73,7 +73,7 @@ interface ISurfaceComposer { void bootFinished(); /** - * Create a display event connection + * Create a display event connection. * * layerHandle * Optional binder handle representing a Layer in SF to associate the new @@ -90,12 +90,14 @@ interface ISurfaceComposer { @nullable ISurfaceComposerClient createConnection(); /** - * Create a virtual display + * Create a virtual display. * * displayName - * The name of the virtual display - * secure - * Whether this virtual display is secure + * The name of the virtual display. + * isSecure + * Whether this virtual display is secure. + * uniqueId + * The unique ID for the display. * requestedRefreshRate * The refresh rate, frames per second, to request on the virtual display. * This is just a request, the actual rate may be adjusted to align well @@ -104,11 +106,11 @@ interface ISurfaceComposer { * * requires ACCESS_SURFACE_FLINGER permission. */ - @nullable IBinder createDisplay(@utf8InCpp String displayName, boolean secure, - float requestedRefreshRate); + @nullable IBinder createDisplay(@utf8InCpp String displayName, boolean isSecure, + @utf8InCpp String uniqueId, float requestedRefreshRate); /** - * Destroy a virtual display + * Destroy a virtual display. * requires ACCESS_SURFACE_FLINGER permission. */ void destroyDisplay(IBinder display); diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 288882695a..82090fa385 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -18,6 +18,7 @@ #include #include + #include #include #include @@ -374,17 +375,15 @@ public: sp mirrorDisplay(DisplayId displayId); - //! Create a virtual display - static sp createDisplay(const String8& displayName, bool secure, - float requestedRefereshRate = 0); + static const std::string kEmpty; + static sp createDisplay(const String8& displayName, bool isSecure, + const std::string& uniqueId = kEmpty, + float requestedRefreshRate = 0); - //! Destroy a virtual display static void destroyDisplay(const sp& display); - //! Get stable IDs for connected physical displays static std::vector getPhysicalDisplayIds(); - //! Get token for a physical display given its stable ID static sp getPhysicalDisplayToken(PhysicalDisplayId displayId); // Returns StalledTransactionInfo if a transaction from the provided pid has not been applied diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 577d2394c6..8a7ecabc16 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -673,8 +673,8 @@ public: return binder::Status::ok(); } - binder::Status createDisplay(const std::string& /*displayName*/, bool /*secure*/, - float /*requestedRefreshRate*/, + binder::Status createDisplay(const std::string& /*displayName*/, bool /*isSecure*/, + const std::string& /*uniqueId*/, float /*requestedRefreshRate*/, sp* /*outDisplay*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h index edd57cce91..543a16a399 100644 --- a/services/surfaceflinger/DisplayDevice.h +++ b/services/surfaceflinger/DisplayDevice.h @@ -329,6 +329,7 @@ struct DisplayDeviceState { uint32_t width = 0; uint32_t height = 0; std::string displayName; + std::string uniqueId; bool isSecure = false; bool isProtected = false; // Refer to DisplayDevice::mRequestedRefreshRate, for virtual display only diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 3ec5f213e0..6b117b8a17 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -577,13 +577,13 @@ void SurfaceFlinger::run() { mScheduler->run(); } -sp SurfaceFlinger::createDisplay(const String8& displayName, bool secure, - float requestedRefreshRate) { +sp SurfaceFlinger::createDisplay(const String8& displayName, bool isSecure, + const std::string& uniqueId, float requestedRefreshRate) { // SurfaceComposerAIDL checks for some permissions, but adding an additional check here. // This is to ensure that only root, system, and graphics can request to create a secure // display. Secure displays can show secure content so we add an additional restriction on it. - const int uid = IPCThreadState::self()->getCallingUid(); - if (secure && uid != AID_ROOT && uid != AID_GRAPHICS && uid != AID_SYSTEM) { + const uid_t uid = IPCThreadState::self()->getCallingUid(); + if (isSecure && uid != AID_ROOT && uid != AID_GRAPHICS && uid != AID_SYSTEM) { ALOGE("Only privileged processes can create a secure display"); return nullptr; } @@ -607,11 +607,12 @@ sp SurfaceFlinger::createDisplay(const String8& displayName, bool secur Mutex::Autolock _l(mStateLock); // Display ID is assigned when virtual display is allocated by HWC. DisplayDeviceState state; - state.isSecure = secure; + state.isSecure = isSecure; // Set display as protected when marked as secure to ensure no behavior change // TODO (b/314820005): separate as a different arg when creating the display. - state.isProtected = secure; + state.isProtected = isSecure; state.displayName = displayName; + state.uniqueId = uniqueId; state.requestedRefreshRate = Fps::fromValue(requestedRefreshRate); mCurrentState.displays.add(token, state); return token; @@ -9530,7 +9531,8 @@ binder::Status SurfaceComposerAIDL::createConnection(sp* outDisplay) { status_t status = checkAccessPermission(); @@ -9538,7 +9540,7 @@ binder::Status SurfaceComposerAIDL::createDisplay(const std::string& displayName return binderStatusFromStatusT(status); } String8 displayName8 = String8::format("%s", displayName.c_str()); - *outDisplay = mFlinger->createDisplay(displayName8, secure, requestedRefreshRate); + *outDisplay = mFlinger->createDisplay(displayName8, isSecure, uniqueId, requestedRefreshRate); return binder::Status::ok(); } @@ -9555,10 +9557,10 @@ binder::Status SurfaceComposerAIDL::getPhysicalDisplayIds(std::vector* std::vector physicalDisplayIds = mFlinger->getPhysicalDisplayIds(); std::vector displayIds; displayIds.reserve(physicalDisplayIds.size()); - for (auto item : physicalDisplayIds) { - displayIds.push_back(static_cast(item.value)); + for (const auto id : physicalDisplayIds) { + displayIds.push_back(static_cast(id.value)); } - *outDisplayIds = displayIds; + *outDisplayIds = std::move(displayIds); return binder::Status::ok(); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index c106abda37..4c53faec7c 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -536,15 +536,16 @@ private: static const size_t MAX_LAYERS = 4096; - // Implements IBinder. + static bool callingThreadHasUnscopedSurfaceFlingerAccess(bool usePermissionCache = true) + EXCLUDES(mStateLock); + + // IBinder overrides: status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) override; status_t dump(int fd, const Vector& args) override { return priorityDump(fd, args); } - bool callingThreadHasUnscopedSurfaceFlingerAccess(bool usePermissionCache = true) - EXCLUDES(mStateLock); - // Implements ISurfaceComposer - sp createDisplay(const String8& displayName, bool secure, - float requestedRefreshRate = 0.0f); + // ISurfaceComposer implementation: + sp createDisplay(const String8& displayName, bool isSecure, + const std::string& uniqueId, float requestedRefreshRate = 0.0f); void destroyDisplay(const sp& displayToken); std::vector getPhysicalDisplayIds() const EXCLUDES(mStateLock) { Mutex::Autolock lock(mStateLock); @@ -665,7 +666,7 @@ private: void updateHdcpLevels(hal::HWDisplayId hwcDisplayId, int32_t connectedLevel, int32_t maxLevel); - // Implements IBinder::DeathRecipient. + // IBinder::DeathRecipient overrides: void binderDied(const wp& who) override; // HWC2::ComposerCallback overrides: @@ -1543,7 +1544,7 @@ private: class SurfaceComposerAIDL : public gui::BnSurfaceComposer { public: - SurfaceComposerAIDL(sp sf) : mFlinger(std::move(sf)) {} + explicit SurfaceComposerAIDL(sp sf) : mFlinger(std::move(sf)) {} binder::Status bootFinished() override; binder::Status createDisplayEventConnection( @@ -1551,8 +1552,9 @@ public: const sp& layerHandle, sp* outConnection) override; binder::Status createConnection(sp* outClient) override; - binder::Status createDisplay(const std::string& displayName, bool secure, - float requestedRefreshRate, sp* outDisplay) override; + binder::Status createDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId, float requestedRefreshRate, + sp* outDisplay) override; binder::Status destroyDisplay(const sp& display) override; binder::Status getPhysicalDisplayIds(std::vector* outDisplayIds) override; binder::Status getPhysicalDisplayToken(int64_t displayId, sp* outDisplay) override; @@ -1675,7 +1677,7 @@ private: gui::DynamicDisplayInfo*& outInfo); private: - sp mFlinger; + const sp mFlinger; }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp index 28162f4d6b..bf5ae21f60 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp @@ -132,6 +132,36 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); } +TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { + const String8 name("virtual.test"); + const std::string uniqueId = "virtual:package:id"; + + // -------------------------------------------------------------------- + // Call Expectations + + // -------------------------------------------------------------------- + // Invocation + + sp displayToken = mFlinger.createDisplay(name, false, uniqueId); + + // -------------------------------------------------------------------- + // Postconditions + + // The display should have been added to the current state + ASSERT_TRUE(hasCurrentDisplayState(displayToken)); + const auto& display = getCurrentDisplayState(displayToken); + EXPECT_TRUE(display.isVirtual()); + EXPECT_FALSE(display.isSecure); + EXPECT_EQ(display.uniqueId, "virtual:package:id"); + EXPECT_EQ(name.c_str(), display.displayName); + + // -------------------------------------------------------------------- + // Cleanup conditions + + // Creating the display commits a display transaction. + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame()).Times(1); +} + // Requesting 0 tells SF not to do anything, i.e., default to refresh as physical displays TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRate0) { const String8 displayName("virtual.test"); diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 82023b092a..3b03f92090 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -411,8 +411,15 @@ public: commit(kComposite); } - auto createDisplay(const String8& displayName, bool secure, float requestedRefreshRate = 0.0f) { - return mFlinger->createDisplay(displayName, secure, requestedRefreshRate); + auto createDisplay(const String8& displayName, bool isSecure, + float requestedRefreshRate = 0.0f) { + const std::string testId = "virtual:libsurfaceflinger_unittest:TestableSurfaceFlinger"; + return mFlinger->createDisplay(displayName, isSecure, testId, requestedRefreshRate); + } + + auto createDisplay(const String8& displayName, bool isSecure, const std::string& uniqueId, + float requestedRefreshRate = 0.0f) { + return mFlinger->createDisplay(displayName, isSecure, uniqueId, requestedRefreshRate); } auto destroyDisplay(const sp& displayToken) { -- cgit v1.2.3-59-g8ed1b From d53801c5273e34bc767e606454d32aa50f38ece8 Mon Sep 17 00:00:00 2001 From: Alan Ding Date: Wed, 8 May 2024 16:45:29 -0700 Subject: SF: Cleanup creating / destroying virtual display APIs Follow-up CL for cleanup requested by SF owner in http://ag/27202517. Misc: - Fix type conversion warnings for print format. - Return status destroying virtual display in SF and add appropriate checks in UT. - Use static constexpr / const for compile-time effciciency. Bug: 339525838 Bug: 137375833 Bug: 194863377 Test: atest libsurfaceflinger_unittest Test: atest SurfaceFlinger_test Flag: EXEMPT refactor Change-Id: I7859720989c9244b26f8764c3323a8495064c888 --- libs/gui/SurfaceComposerClient.cpp | 20 ++-- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 4 +- libs/gui/include/gui/ISurfaceComposer.h | 4 +- libs/gui/include/gui/SurfaceComposerClient.h | 8 +- libs/gui/tests/EndToEndNativeInputTest.cpp | 7 +- libs/gui/tests/Surface_test.cpp | 9 +- services/surfaceflinger/SurfaceFlinger.cpp | 41 ++++---- services/surfaceflinger/SurfaceFlinger.h | 15 +-- services/surfaceflinger/tests/Credentials_test.cpp | 10 +- .../tests/MultiDisplayLayerBounds_test.cpp | 5 +- .../tests/TransactionTestHarnesses.h | 7 +- .../surfaceflinger/tests/VirtualDisplay_test.cpp | 5 +- .../unittests/SurfaceFlinger_ColorMatrixTest.cpp | 3 +- .../unittests/SurfaceFlinger_CreateDisplayTest.cpp | 106 ++++++++++----------- .../SurfaceFlinger_DestroyDisplayTest.cpp | 10 +- .../SurfaceFlinger_GetDisplayStatsTest.cpp | 4 +- .../tests/unittests/TestableSurfaceFlinger.h | 20 ++-- 17 files changed, 146 insertions(+), 132 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 7aaaebbc8e..0a85cf8d8a 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1280,18 +1280,22 @@ status_t SurfaceComposerClient::Transaction::sendSurfaceFlushJankDataTransaction } // --------------------------------------------------------------------------- -sp SurfaceComposerClient::createDisplay(const String8& displayName, bool isSecure, - const std::string& uniqueId, - float requestedRefreshRate) { +sp SurfaceComposerClient::createVirtualDisplay(const std::string& displayName, + bool isSecure, const std::string& uniqueId, + float requestedRefreshRate) { sp display = nullptr; - binder::Status status = ComposerServiceAIDL::getComposerService() - ->createDisplay(std::string(displayName.c_str()), isSecure, - uniqueId, requestedRefreshRate, &display); + binder::Status status = + ComposerServiceAIDL::getComposerService()->createVirtualDisplay(displayName, isSecure, + uniqueId, + requestedRefreshRate, + &display); return status.isOk() ? display : nullptr; } -void SurfaceComposerClient::destroyDisplay(const sp& display) { - ComposerServiceAIDL::getComposerService()->destroyDisplay(display); +status_t SurfaceComposerClient::destroyVirtualDisplay(const sp& displayToken) { + return ComposerServiceAIDL::getComposerService() + ->destroyVirtualDisplay(displayToken) + .transactionError(); } std::vector SurfaceComposerClient::getPhysicalDisplayIds() { diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index c6e7197f24..11ccc9c2fa 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -105,14 +105,14 @@ interface ISurfaceComposer { * * requires ACCESS_SURFACE_FLINGER permission. */ - @nullable IBinder createDisplay(@utf8InCpp String displayName, boolean isSecure, + @nullable IBinder createVirtualDisplay(@utf8InCpp String displayName, boolean isSecure, @utf8InCpp String uniqueId, float requestedRefreshRate); /** * Destroy a virtual display. * requires ACCESS_SURFACE_FLINGER permission. */ - void destroyDisplay(IBinder display); + void destroyVirtualDisplay(IBinder displayToken); /** * Get stable IDs for connected physical displays. diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 738c73a24b..eb4a802c17 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -130,8 +130,8 @@ public: CREATE_CONNECTION, // Deprecated. Autogenerated by .aidl now. GET_STATIC_DISPLAY_INFO, // Deprecated. Autogenerated by .aidl now. CREATE_DISPLAY_EVENT_CONNECTION, // Deprecated. Autogenerated by .aidl now. - CREATE_DISPLAY, // Deprecated. Autogenerated by .aidl now. - DESTROY_DISPLAY, // Deprecated. Autogenerated by .aidl now. + CREATE_VIRTUAL_DISPLAY, // Deprecated. Autogenerated by .aidl now. + DESTROY_VIRTUAL_DISPLAY, // Deprecated. Autogenerated by .aidl now. GET_PHYSICAL_DISPLAY_TOKEN, // Deprecated. Autogenerated by .aidl now. SET_TRANSACTION_STATE, AUTHENTICATE_SURFACE, // Deprecated. Autogenerated by .aidl now. diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 987efe01ba..e2307ed60a 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -376,11 +376,11 @@ public: sp mirrorDisplay(DisplayId displayId); static const std::string kEmpty; - static sp createDisplay(const String8& displayName, bool isSecure, - const std::string& uniqueId = kEmpty, - float requestedRefreshRate = 0); + static sp createVirtualDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId = kEmpty, + float requestedRefreshRate = 0); - static void destroyDisplay(const sp& display); + static status_t destroyVirtualDisplay(const sp& displayToken); static std::vector getPhysicalDisplayIds(); diff --git a/libs/gui/tests/EndToEndNativeInputTest.cpp b/libs/gui/tests/EndToEndNativeInputTest.cpp index c0e79655f8..b0db396872 100644 --- a/libs/gui/tests/EndToEndNativeInputTest.cpp +++ b/libs/gui/tests/EndToEndNativeInputTest.cpp @@ -1186,9 +1186,8 @@ public: MultiDisplayTests() : InputSurfacesTest() { ProcessState::self()->startThreadPool(); } void TearDown() override { - for (auto& token : mVirtualDisplays) { - SurfaceComposerClient::destroyDisplay(token); - } + std::for_each(mVirtualDisplays.begin(), mVirtualDisplays.end(), + SurfaceComposerClient::destroyVirtualDisplay); InputSurfacesTest::TearDown(); } @@ -1203,7 +1202,7 @@ public: std::string name = "VirtualDisplay"; name += std::to_string(mVirtualDisplays.size()); - sp token = SurfaceComposerClient::createDisplay(String8(name.c_str()), isSecure); + sp token = SurfaceComposerClient::createVirtualDisplay(name, isSecure); SurfaceComposerClient::Transaction t; t.setDisplaySurface(token, producer); t.setDisplayFlags(token, receivesInput ? 0x01 /* DisplayDevice::eReceivesInput */ : 0); diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index eee4fb93c9..6c6a849544 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -673,13 +673,14 @@ public: return binder::Status::ok(); } - binder::Status createDisplay(const std::string& /*displayName*/, bool /*isSecure*/, - const std::string& /*uniqueId*/, float /*requestedRefreshRate*/, - sp* /*outDisplay*/) override { + binder::Status createVirtualDisplay(const std::string& /*displayName*/, bool /*isSecure*/, + const std::string& /*uniqueId*/, + float /*requestedRefreshRate*/, + sp* /*outDisplay*/) override { return binder::Status::ok(); } - binder::Status destroyDisplay(const sp& /*display*/) override { + binder::Status destroyVirtualDisplay(const sp& /*displayToken*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 5181fb8b56..9a575ca769 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -56,6 +56,7 @@ #include #include #include +#include #include #include #include @@ -573,8 +574,9 @@ void SurfaceFlinger::run() { mScheduler->run(); } -sp SurfaceFlinger::createDisplay(const String8& displayName, bool isSecure, - const std::string& uniqueId, float requestedRefreshRate) { +sp SurfaceFlinger::createVirtualDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId, + float requestedRefreshRate) { // SurfaceComposerAIDL checks for some permissions, but adding an additional check here. // This is to ensure that only root, system, and graphics can request to create a secure // display. Secure displays can show secure content so we add an additional restriction on it. @@ -614,22 +616,23 @@ sp SurfaceFlinger::createDisplay(const String8& displayName, bool isSec return token; } -void SurfaceFlinger::destroyDisplay(const sp& displayToken) { +status_t SurfaceFlinger::destroyVirtualDisplay(const sp& displayToken) { Mutex::Autolock lock(mStateLock); const ssize_t index = mCurrentState.displays.indexOfKey(displayToken); if (index < 0) { ALOGE("%s: Invalid display token %p", __func__, displayToken.get()); - return; + return NAME_NOT_FOUND; } const DisplayDeviceState& state = mCurrentState.displays.valueAt(index); if (state.physical) { ALOGE("%s: Invalid operation on physical display", __func__); - return; + return INVALID_OPERATION; } mCurrentState.displays.removeItemsAt(index); setTransactionFlags(eDisplayTransactionNeeded); + return NO_ERROR; } void SurfaceFlinger::enableHalVirtualDisplays(bool enable) { @@ -6531,18 +6534,19 @@ void SurfaceFlinger::dumpWideColorInfo(std::string& result) const { StringAppendF(&result, "DisplayColorSetting: %s\n", decodeDisplayColorSetting(mDisplayColorSetting).c_str()); - // TODO: print out if wide-color mode is active or not + // TODO: print out if wide-color mode is active or not. for (const auto& [id, display] : mPhysicalDisplays) { StringAppendF(&result, "Display %s color modes:\n", to_string(id).c_str()); for (const auto mode : display.snapshot().colorModes()) { - StringAppendF(&result, " %s (%d)\n", decodeColorMode(mode).c_str(), mode); + StringAppendF(&result, " %s (%d)\n", decodeColorMode(mode).c_str(), + fmt::underlying(mode)); } if (const auto display = getDisplayDeviceLocked(id)) { ui::ColorMode currentMode = display->getCompositionDisplay()->getState().colorMode; StringAppendF(&result, " Current color mode: %s (%d)\n", - decodeColorMode(currentMode).c_str(), currentMode); + decodeColorMode(currentMode).c_str(), fmt::underlying(currentMode)); } } result.append("\n"); @@ -6999,8 +7003,8 @@ status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) { // Used by apps to hook Choreographer to SurfaceFlinger. case CREATE_DISPLAY_EVENT_CONNECTION: case CREATE_CONNECTION: - case CREATE_DISPLAY: - case DESTROY_DISPLAY: + case CREATE_VIRTUAL_DISPLAY: + case DESTROY_VIRTUAL_DISPLAY: case GET_PRIMARY_PHYSICAL_DISPLAY_ID: case GET_PHYSICAL_DISPLAY_IDS: case GET_PHYSICAL_DISPLAY_TOKEN: @@ -9553,26 +9557,25 @@ binder::Status SurfaceComposerAIDL::createConnection(sp* outDisplay) { +binder::Status SurfaceComposerAIDL::createVirtualDisplay(const std::string& displayName, + bool isSecure, const std::string& uniqueId, + float requestedRefreshRate, + sp* outDisplay) { status_t status = checkAccessPermission(); if (status != OK) { return binderStatusFromStatusT(status); } - String8 displayName8 = String8::format("%s", displayName.c_str()); - *outDisplay = mFlinger->createDisplay(displayName8, isSecure, uniqueId, requestedRefreshRate); + *outDisplay = + mFlinger->createVirtualDisplay(displayName, isSecure, uniqueId, requestedRefreshRate); return binder::Status::ok(); } -binder::Status SurfaceComposerAIDL::destroyDisplay(const sp& display) { +binder::Status SurfaceComposerAIDL::destroyVirtualDisplay(const sp& displayToken) { status_t status = checkAccessPermission(); if (status != OK) { return binderStatusFromStatusT(status); } - mFlinger->destroyDisplay(display); - return binder::Status::ok(); + return binder::Status::fromStatusT(mFlinger->destroyVirtualDisplay(displayToken)); } binder::Status SurfaceComposerAIDL::getPhysicalDisplayIds(std::vector* outDisplayIds) { diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 84745150d0..2363cd7ebd 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -547,9 +547,10 @@ private: status_t dump(int fd, const Vector& args) override { return priorityDump(fd, args); } // ISurfaceComposer implementation: - sp createDisplay(const String8& displayName, bool isSecure, - const std::string& uniqueId, float requestedRefreshRate = 0.0f); - void destroyDisplay(const sp& displayToken); + sp createVirtualDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId, + float requestedRefreshRate = 0.0f); + status_t destroyVirtualDisplay(const sp& displayToken); std::vector getPhysicalDisplayIds() const EXCLUDES(mStateLock) { Mutex::Autolock lock(mStateLock); return getPhysicalDisplayIdsLocked(); @@ -1568,10 +1569,10 @@ public: const sp& layerHandle, sp* outConnection) override; binder::Status createConnection(sp* outClient) override; - binder::Status createDisplay(const std::string& displayName, bool isSecure, - const std::string& uniqueId, float requestedRefreshRate, - sp* outDisplay) override; - binder::Status destroyDisplay(const sp& display) override; + binder::Status createVirtualDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId, float requestedRefreshRate, + sp* outDisplay) override; + binder::Status destroyVirtualDisplay(const sp& displayToken) override; binder::Status getPhysicalDisplayIds(std::vector* outDisplayIds) override; binder::Status getPhysicalDisplayToken(int64_t displayId, sp* outDisplay) override; binder::Status setPowerMode(const sp& display, int mode) override; diff --git a/services/surfaceflinger/tests/Credentials_test.cpp b/services/surfaceflinger/tests/Credentials_test.cpp index c1ef48ec97..ebe11fb0f3 100644 --- a/services/surfaceflinger/tests/Credentials_test.cpp +++ b/services/surfaceflinger/tests/Credentials_test.cpp @@ -39,8 +39,8 @@ using gui::aidl_utils::statusTFromBinderStatus; using ui::ColorMode; namespace { -const String8 DISPLAY_NAME("Credentials Display Test"); -const String8 SURFACE_NAME("Test Surface Name"); +const std::string kDisplayName("Credentials Display Test"); +const String8 kSurfaceName("Test Surface Name"); } // namespace /** @@ -99,7 +99,7 @@ protected: ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getActiveDisplayMode(mDisplay, &mode)); // Background surface - mBGSurfaceControl = mComposerClient->createSurface(SURFACE_NAME, mode.resolution.getWidth(), + mBGSurfaceControl = mComposerClient->createSurface(kSurfaceName, mode.resolution.getWidth(), mode.resolution.getHeight(), PIXEL_FORMAT_RGBA_8888, 0); ASSERT_TRUE(mBGSurfaceControl != nullptr); @@ -232,7 +232,7 @@ TEST_F(CredentialsTest, SetActiveColorModeTest) { TEST_F(CredentialsTest, CreateDisplayTest) { // Only graphics and system processes can create a secure display. std::function condition = [=]() { - sp testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, true); + sp testDisplay = SurfaceComposerClient::createVirtualDisplay(kDisplayName, true); return testDisplay.get() != nullptr; }; @@ -267,7 +267,7 @@ TEST_F(CredentialsTest, CreateDisplayTest) { } condition = [=]() { - sp testDisplay = SurfaceComposerClient::createDisplay(DISPLAY_NAME, false); + sp testDisplay = SurfaceComposerClient::createVirtualDisplay(kDisplayName, false); return testDisplay.get() != nullptr; }; ASSERT_NO_FATAL_FAILURE(checkWithPrivileges(condition, true, false)); diff --git a/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp b/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp index 7fce7e9af9..56cf13d7fe 100644 --- a/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp +++ b/services/surfaceflinger/tests/MultiDisplayLayerBounds_test.cpp @@ -53,14 +53,15 @@ protected: } virtual void TearDown() { - SurfaceComposerClient::destroyDisplay(mVirtualDisplay); + EXPECT_EQ(NO_ERROR, SurfaceComposerClient::destroyVirtualDisplay(mVirtualDisplay)); LayerTransactionTest::TearDown(); mColorLayer = 0; } void createDisplay(const ui::Size& layerStackSize, ui::LayerStack layerStack) { + static const std::string kDisplayName("VirtualDisplay"); mVirtualDisplay = - SurfaceComposerClient::createDisplay(String8("VirtualDisplay"), false /*secure*/); + SurfaceComposerClient::createVirtualDisplay(kDisplayName, false /*isSecure*/); asTransaction([&](Transaction& t) { t.setDisplaySurface(mVirtualDisplay, mProducer); t.setDisplayLayerStack(mVirtualDisplay, layerStack); diff --git a/services/surfaceflinger/tests/TransactionTestHarnesses.h b/services/surfaceflinger/tests/TransactionTestHarnesses.h index 87e6d3ee9f..af3cb9aec1 100644 --- a/services/surfaceflinger/tests/TransactionTestHarnesses.h +++ b/services/surfaceflinger/tests/TransactionTestHarnesses.h @@ -66,8 +66,9 @@ public: sp listener = sp::make(this); itemConsumer->setFrameAvailableListener(listener); - vDisplay = SurfaceComposerClient::createDisplay(String8("VirtualDisplay"), - false /*secure*/); + static const std::string kDisplayName("VirtualDisplay"); + vDisplay = SurfaceComposerClient::createVirtualDisplay(kDisplayName, + false /*isSecure*/); constexpr ui::LayerStack layerStack{ 848472}; // ASCII for TTH (TransactionTestHarnesses) @@ -107,7 +108,7 @@ public: t.setLayerStack(mirrorSc, ui::INVALID_LAYER_STACK); t.apply(true); } - SurfaceComposerClient::destroyDisplay(vDisplay); + SurfaceComposerClient::destroyVirtualDisplay(vDisplay); return sc; } } diff --git a/services/surfaceflinger/tests/VirtualDisplay_test.cpp b/services/surfaceflinger/tests/VirtualDisplay_test.cpp index f31f582ffd..cd66dd20bb 100644 --- a/services/surfaceflinger/tests/VirtualDisplay_test.cpp +++ b/services/surfaceflinger/tests/VirtualDisplay_test.cpp @@ -41,14 +41,15 @@ protected: }; TEST_F(VirtualDisplayTest, VirtualDisplayDestroyedSurfaceReuse) { + static const std::string kDisplayName("VirtualDisplay"); sp virtualDisplay = - SurfaceComposerClient::createDisplay(String8("VirtualDisplay"), false /*secure*/); + SurfaceComposerClient::createVirtualDisplay(kDisplayName, false /*isSecure*/); SurfaceComposerClient::Transaction t; t.setDisplaySurface(virtualDisplay, mProducer); t.apply(true); - SurfaceComposerClient::destroyDisplay(virtualDisplay); + EXPECT_EQ(NO_ERROR, SurfaceComposerClient::destroyVirtualDisplay(virtualDisplay)); virtualDisplay.clear(); // Sync here to ensure the display was completely destroyed in SF t.apply(true); diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_ColorMatrixTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_ColorMatrixTest.cpp index 5852b1c45f..ff7612e064 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_ColorMatrixTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_ColorMatrixTest.cpp @@ -53,7 +53,8 @@ TEST_F(ColorMatrixTest, colorMatrixChangedAfterDisplayTransaction) { mFlinger.commitAndComposite(); EXPECT_COLOR_MATRIX_CHANGED(false, false); - mFlinger.createDisplay(String8("Test Display"), false); + static const std::string kDisplayName("Test Display"); + mFlinger.createVirtualDisplay(kDisplayName, false /*isSecure=*/); mFlinger.commit(); EXPECT_COLOR_MATRIX_CHANGED(false, true); diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp index bf5ae21f60..e5f2a9138d 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_CreateDisplayTest.cpp @@ -27,7 +27,7 @@ namespace { class CreateDisplayTest : public DisplayTransactionTest { public: - void createDisplayWithRequestedRefreshRate(const String8& name, uint64_t displayId, + void createDisplayWithRequestedRefreshRate(const std::string& name, uint64_t displayId, float pacesetterDisplayRefreshRate, float requestedRefreshRate, float expectedAdjustedRefreshRate) { @@ -37,7 +37,7 @@ public: // -------------------------------------------------------------------- // Invocation - sp displayToken = mFlinger.createDisplay(name, false, requestedRefreshRate); + sp displayToken = mFlinger.createVirtualDisplay(name, false, requestedRefreshRate); // -------------------------------------------------------------------- // Postconditions @@ -73,7 +73,7 @@ public: }; TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForNonsecureDisplay) { - const String8 name("virtual.test"); + static const std::string name("virtual.test"); // -------------------------------------------------------------------- // Call Expectations @@ -81,7 +81,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForNonsecureDisplay) { // -------------------------------------------------------------------- // Invocation - sp displayToken = mFlinger.createDisplay(name, false); + sp displayToken = mFlinger.createVirtualDisplay(name, false); // -------------------------------------------------------------------- // Postconditions @@ -101,7 +101,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForNonsecureDisplay) { } TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { - const String8 name("virtual.test"); + static const std::string kDisplayName("virtual.test"); // -------------------------------------------------------------------- // Call Expectations @@ -112,7 +112,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { // Set the calling identity to graphics so captureDisplay with secure is allowed. IPCThreadState::self()->restoreCallingIdentity(static_cast(AID_GRAPHICS) << 32 | AID_GRAPHICS); - sp displayToken = mFlinger.createDisplay(name, true); + sp displayToken = mFlinger.createVirtualDisplay(kDisplayName, true); IPCThreadState::self()->restoreCallingIdentity(oldId); // -------------------------------------------------------------------- @@ -123,7 +123,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { const auto& display = getCurrentDisplayState(displayToken); EXPECT_TRUE(display.isVirtual()); EXPECT_TRUE(display.isSecure); - EXPECT_EQ(name.c_str(), display.displayName); + EXPECT_EQ(kDisplayName.c_str(), display.displayName); // -------------------------------------------------------------------- // Cleanup conditions @@ -133,8 +133,8 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForSecureDisplay) { } TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { - const String8 name("virtual.test"); - const std::string uniqueId = "virtual:package:id"; + static const std::string kDisplayName("virtual.test"); + static const std::string kUniqueId = "virtual:package:id"; // -------------------------------------------------------------------- // Call Expectations @@ -142,7 +142,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { // -------------------------------------------------------------------- // Invocation - sp displayToken = mFlinger.createDisplay(name, false, uniqueId); + sp displayToken = mFlinger.createVirtualDisplay(kDisplayName, false, kUniqueId); // -------------------------------------------------------------------- // Postconditions @@ -153,7 +153,7 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { EXPECT_TRUE(display.isVirtual()); EXPECT_FALSE(display.isSecure); EXPECT_EQ(display.uniqueId, "virtual:package:id"); - EXPECT_EQ(name.c_str(), display.displayName); + EXPECT_EQ(kDisplayName.c_str(), display.displayName); // -------------------------------------------------------------------- // Cleanup conditions @@ -164,78 +164,78 @@ TEST_F(CreateDisplayTest, createDisplaySetsCurrentStateForUniqueId) { // Requesting 0 tells SF not to do anything, i.e., default to refresh as physical displays TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRate0) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 60.f; - const float kRequestedRefreshRate = 0.f; - const float kExpectedAdjustedRefreshRate = 0.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 60.f; + constexpr float kRequestedRefreshRate = 0.f; + constexpr float kExpectedAdjustedRefreshRate = 0.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting negative refresh rate, will be ignored, same as requesting 0 TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateNegative) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 60.f; - const float kRequestedRefreshRate = -60.f; - const float kExpectedAdjustedRefreshRate = 0.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 60.f; + constexpr float kRequestedRefreshRate = -60.f; + constexpr float kExpectedAdjustedRefreshRate = 0.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting a higher refresh rate than the pacesetter TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateHigh) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 60.f; - const float kRequestedRefreshRate = 90.f; - const float kExpectedAdjustedRefreshRate = 60.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 60.f; + constexpr float kRequestedRefreshRate = 90.f; + constexpr float kExpectedAdjustedRefreshRate = 60.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting the same refresh rate as the pacesetter TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateSame) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 60.f; - const float kRequestedRefreshRate = 60.f; - const float kExpectedAdjustedRefreshRate = 60.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 60.f; + constexpr float kRequestedRefreshRate = 60.f; + constexpr float kExpectedAdjustedRefreshRate = 60.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting a divisor (30) of the pacesetter (60) should be honored TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateDivisor) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 60.f; - const float kRequestedRefreshRate = 30.f; - const float kExpectedAdjustedRefreshRate = 30.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 60.f; + constexpr float kRequestedRefreshRate = 30.f; + constexpr float kExpectedAdjustedRefreshRate = 30.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting a non divisor (45) of the pacesetter (120) should round up to a divisor (60) TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateNoneDivisor) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 120.f; - const float kRequestedRefreshRate = 45.f; - const float kExpectedAdjustedRefreshRate = 60.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 120.f; + constexpr float kRequestedRefreshRate = 45.f; + constexpr float kExpectedAdjustedRefreshRate = 60.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } // Requesting a non divisor (75) of the pacesetter (120) should round up to pacesetter (120) TEST_F(CreateDisplayTest, createDisplayWithRequestedRefreshRateNoneDivisorMax) { - const String8 displayName("virtual.test"); - const uint64_t displayId = 123ull; - const float kPacesetterDisplayRefreshRate = 120.f; - const float kRequestedRefreshRate = 75.f; - const float kExpectedAdjustedRefreshRate = 120.f; - createDisplayWithRequestedRefreshRate(displayName, displayId, kPacesetterDisplayRefreshRate, + static const std::string kDisplayName("virtual.test"); + constexpr uint64_t kDisplayId = 123ull; + constexpr float kPacesetterDisplayRefreshRate = 120.f; + constexpr float kRequestedRefreshRate = 75.f; + constexpr float kExpectedAdjustedRefreshRate = 120.f; + createDisplayWithRequestedRefreshRate(kDisplayName, kDisplayId, kPacesetterDisplayRefreshRate, kRequestedRefreshRate, kExpectedAdjustedRefreshRate); } diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp index 93a3811172..f8ad8e1e1b 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DestroyDisplayTest.cpp @@ -43,18 +43,18 @@ TEST_F(DestroyDisplayTest, destroyDisplayClearsCurrentStateForDisplay) { // -------------------------------------------------------------------- // Invocation - mFlinger.destroyDisplay(existing.token()); + EXPECT_EQ(NO_ERROR, mFlinger.destroyVirtualDisplay(existing.token())); // -------------------------------------------------------------------- // Postconditions - // The display should have been removed from the current state + // The display should have been removed from the current state. EXPECT_FALSE(hasCurrentDisplayState(existing.token())); - // Ths display should still exist in the drawing state + // Ths display should still exist in the drawing state. EXPECT_TRUE(hasDrawingDisplayState(existing.token())); - // The display transaction needed flasg should be set + // The display transaction needed flags should be set. EXPECT_TRUE(hasTransactionFlagSet(eDisplayTransactionNeeded)); } @@ -67,7 +67,7 @@ TEST_F(DestroyDisplayTest, destroyDisplayHandlesUnknownDisplay) { // -------------------------------------------------------------------- // Invocation - mFlinger.destroyDisplay(displayToken); + EXPECT_EQ(NAME_NOT_FOUND, mFlinger.destroyVirtualDisplay(displayToken)); } } // namespace diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_GetDisplayStatsTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_GetDisplayStatsTest.cpp index 4e9fba7bda..f424133655 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_GetDisplayStatsTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_GetDisplayStatsTest.cpp @@ -42,8 +42,8 @@ TEST_F(SurfaceFlingerGetDisplayStatsTest, explicitToken) { } TEST_F(SurfaceFlingerGetDisplayStatsTest, invalidToken) { - const String8 displayName("fakeDisplay"); - sp displayToken = mFlinger.createDisplay(displayName, false); + static const std::string kDisplayName("fakeDisplay"); + sp displayToken = mFlinger.createVirtualDisplay(kDisplayName, false /*isSecure*/); DisplayStatInfo info; status_t status = mFlinger.getDisplayStats(displayToken, &info); EXPECT_EQ(status, NAME_NOT_FOUND); diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index b251e2cf0c..265f804fc8 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -420,19 +420,21 @@ public: commit(kComposite); } - auto createDisplay(const String8& displayName, bool isSecure, - float requestedRefreshRate = 0.0f) { - const std::string testId = "virtual:libsurfaceflinger_unittest:TestableSurfaceFlinger"; - return mFlinger->createDisplay(displayName, isSecure, testId, requestedRefreshRate); + auto createVirtualDisplay(const std::string& displayName, bool isSecure, + float requestedRefreshRate = 0.0f) { + static const std::string kTestId = + "virtual:libsurfaceflinger_unittest:TestableSurfaceFlinger"; + return mFlinger->createVirtualDisplay(displayName, isSecure, kTestId, requestedRefreshRate); } - auto createDisplay(const String8& displayName, bool isSecure, const std::string& uniqueId, - float requestedRefreshRate = 0.0f) { - return mFlinger->createDisplay(displayName, isSecure, uniqueId, requestedRefreshRate); + auto createVirtualDisplay(const std::string& displayName, bool isSecure, + const std::string& uniqueId, float requestedRefreshRate = 0.0f) { + return mFlinger->createVirtualDisplay(displayName, isSecure, uniqueId, + requestedRefreshRate); } - auto destroyDisplay(const sp& displayToken) { - return mFlinger->destroyDisplay(displayToken); + auto destroyVirtualDisplay(const sp& displayToken) { + return mFlinger->destroyVirtualDisplay(displayToken); } auto getDisplay(const sp& displayToken) { -- cgit v1.2.3-59-g8ed1b From eafc18ad085dcd727277729aee2f74edc4df589b Mon Sep 17 00:00:00 2001 From: Wenhui Yang Date: Tue, 14 May 2024 17:15:52 +0000 Subject: Capture transaction traces before system reboot Bug: 299937754 Test: bugreport - data/misc/wmtrace/systemRestart_transactions.winscope Test: go/winscope Change-Id: Icfb10a9e69fd2ca7f4657564b109f8da126f7dc7 --- libs/gui/SurfaceComposerClient.cpp | 4 ++++ libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 7 +++++++ libs/gui/include/gui/SurfaceComposerClient.h | 2 ++ libs/gui/tests/Surface_test.cpp | 2 ++ services/surfaceflinger/SurfaceFlinger.cpp | 8 +++++++- services/surfaceflinger/SurfaceFlinger.h | 1 + 6 files changed, 23 insertions(+), 1 deletion(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 3743025b7b..1977ce614c 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -3113,6 +3113,10 @@ status_t SurfaceComposerClient::removeWindowInfosListener( ->removeWindowInfosListener(windowInfosListener, ComposerServiceAIDL::getComposerService()); } + +void SurfaceComposerClient::notifyShutdown() { + ComposerServiceAIDL::getComposerService()->notifyShutdown(); +} // ---------------------------------------------------------------------------- status_t ScreenshotClient::captureDisplay(const DisplayCaptureArgs& captureArgs, diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index a2549e7e3f..854045a17f 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -571,4 +571,11 @@ interface ISurfaceComposer { @nullable StalledTransactionInfo getStalledTransactionInfo(int pid); SchedulingPolicy getSchedulingPolicy(); + + /** + * Notifies the SurfaceFlinger that the ShutdownThread is running. When it is called, + * transaction traces will be captured and writted into a file. + * This method should not block the ShutdownThread therefore it's handled asynchronously. + */ + oneway void notifyShutdown(); } diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 49b0a7d0c9..64fbb4342b 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -825,6 +825,8 @@ public: nullptr); status_t removeWindowInfosListener(const sp& windowInfosListener); + static void notifyShutdown(); + protected: ReleaseCallbackThread mReleaseCallbackThread; diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index f4b059c39b..8aec030ea3 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -984,6 +984,8 @@ public: return binder::Status::ok(); } + binder::Status notifyShutdown() override { return binder::Status::ok(); } + protected: IBinder* onAsBinder() override { return nullptr; } diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 496185e49b..68bd6a2727 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -984,8 +984,9 @@ void SurfaceFlinger::initTransactionTraceWriter() { ALOGD("TransactionTraceWriter: file=%s already exists", filename.c_str()); return; } - mTransactionTracing->flush(); + ALOGD("TransactionTraceWriter: writing file=%s", filename.c_str()); mTransactionTracing->writeToFile(filename); + mTransactionTracing->flush(); }; if (std::this_thread::get_id() == mMainThreadId) { writeFn(); @@ -10343,6 +10344,11 @@ binder::Status SurfaceComposerAIDL::getSchedulingPolicy(gui::SchedulingPolicy* o return gui::getSchedulingPolicy(outPolicy); } +binder::Status SurfaceComposerAIDL::notifyShutdown() { + TransactionTraceWriter::getInstance().invoke("systemShutdown_", /* overwrite= */ false); + return ::android::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 24444241c9..9a2f018b8c 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1677,6 +1677,7 @@ public: binder::Status getStalledTransactionInfo( int pid, std::optional* outInfo) override; binder::Status getSchedulingPolicy(gui::SchedulingPolicy* outPolicy) override; + binder::Status notifyShutdown() override; private: static const constexpr bool kUsePermissionCache = true; -- cgit v1.2.3-59-g8ed1b From 9e0017e9748e6cd1ed681f4d3aa9b888a02bfa3c Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 22 May 2024 19:02:44 +0000 Subject: Allow child layers to unset trusted overlay state 2/2 Test: presubmit Bug: 339701674 Change-Id: I1a94b5e5dc1fa64a9e1eb3330b5c5b03af8d2b16 --- libs/gui/Android.bp | 1 + libs/gui/LayerState.cpp | 12 ++-- libs/gui/SurfaceComposerClient.cpp | 9 ++- libs/gui/android/gui/TrustedOverlay.aidl | 45 ++++++++++++ libs/gui/include/gui/LayerState.h | 3 +- libs/gui/include/gui/SurfaceComposerClient.h | 2 + services/surfaceflinger/FrontEnd/LayerSnapshot.h | 2 +- .../FrontEnd/LayerSnapshotBuilder.cpp | 19 ++++- .../FrontEnd/RequestedLayerState.cpp | 2 +- services/surfaceflinger/Layer.cpp | 2 +- services/surfaceflinger/LayerProtoHelper.cpp | 3 +- services/surfaceflinger/SurfaceFlinger.cpp | 2 +- .../Tracing/TransactionProtoParser.cpp | 6 +- services/surfaceflinger/common/FlagManager.cpp | 2 + .../common/include/common/FlagManager.h | 1 + .../surfaceflinger_flags_new.aconfig | 13 +++- .../tests/unittests/LayerHierarchyTest.h | 4 +- .../tests/unittests/LayerSnapshotTest.cpp | 83 +++++++++++++++++++++- 18 files changed, 190 insertions(+), 21 deletions(-) create mode 100644 libs/gui/android/gui/TrustedOverlay.aidl (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 6c45746335..2547297ff8 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -87,6 +87,7 @@ filegroup { "android/gui/DropInputMode.aidl", "android/gui/StalledTransactionInfo.aidl", "android/**/TouchOcclusionMode.aidl", + "android/gui/TrustedOverlay.aidl", ], } diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 0a2879975b..c82bde9a83 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -89,7 +89,7 @@ layer_state_t::layer_state_t() frameRateSelectionStrategy(ANATIVEWINDOW_FRAME_RATE_SELECTION_STRATEGY_PROPAGATE), fixedTransformHint(ui::Transform::ROT_INVALID), autoRefresh(false), - isTrustedOverlay(false), + trustedOverlay(gui::TrustedOverlay::UNSET), bufferCrop(Rect::INVALID_RECT), destinationFrame(Rect::INVALID_RECT), dropInputMode(gui::DropInputMode::NONE) { @@ -179,7 +179,7 @@ status_t layer_state_t::write(Parcel& output) const SAFE_PARCEL(output.write, stretchEffect); SAFE_PARCEL(output.write, bufferCrop); SAFE_PARCEL(output.write, destinationFrame); - SAFE_PARCEL(output.writeBool, isTrustedOverlay); + SAFE_PARCEL(output.writeInt32, static_cast(trustedOverlay)); SAFE_PARCEL(output.writeUint32, static_cast(dropInputMode)); @@ -308,7 +308,9 @@ status_t layer_state_t::read(const Parcel& input) SAFE_PARCEL(input.read, stretchEffect); SAFE_PARCEL(input.read, bufferCrop); SAFE_PARCEL(input.read, destinationFrame); - SAFE_PARCEL(input.readBool, &isTrustedOverlay); + uint32_t trustedOverlayInt; + SAFE_PARCEL(input.readUint32, &trustedOverlayInt); + trustedOverlay = static_cast(trustedOverlayInt); uint32_t mode; SAFE_PARCEL(input.readUint32, &mode); @@ -674,7 +676,7 @@ void layer_state_t::merge(const layer_state_t& other) { } if (other.what & eTrustedOverlayChanged) { what |= eTrustedOverlayChanged; - isTrustedOverlay = other.isTrustedOverlay; + trustedOverlay = other.trustedOverlay; } if (other.what & eStretchChanged) { what |= eStretchChanged; @@ -779,7 +781,7 @@ uint64_t layer_state_t::diff(const layer_state_t& other) const { CHECK_DIFF(diff, eFrameRateSelectionStrategyChanged, other, frameRateSelectionStrategy); CHECK_DIFF(diff, eFixedTransformHintChanged, other, fixedTransformHint); CHECK_DIFF(diff, eAutoRefreshChanged, other, autoRefresh); - CHECK_DIFF(diff, eTrustedOverlayChanged, other, isTrustedOverlay); + CHECK_DIFF(diff, eTrustedOverlayChanged, other, trustedOverlay); CHECK_DIFF(diff, eStretchChanged, other, stretchEffect); CHECK_DIFF(diff, eBufferCropChanged, other, bufferCrop); CHECK_DIFF(diff, eDestinationFrameChanged, other, destinationFrame); diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 7aaaebbc8e..e0f1b03e26 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -2175,6 +2175,13 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setAutoR SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setTrustedOverlay( const sp& sc, bool isTrustedOverlay) { + return setTrustedOverlay(sc, + isTrustedOverlay ? gui::TrustedOverlay::ENABLED + : gui::TrustedOverlay::UNSET); +} + +SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setTrustedOverlay( + const sp& sc, gui::TrustedOverlay trustedOverlay) { layer_state_t* s = getLayerState(sc); if (!s) { mStatus = BAD_INDEX; @@ -2182,7 +2189,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setTrust } s->what |= layer_state_t::eTrustedOverlayChanged; - s->isTrustedOverlay = isTrustedOverlay; + s->trustedOverlay = trustedOverlay; return *this; } diff --git a/libs/gui/android/gui/TrustedOverlay.aidl b/libs/gui/android/gui/TrustedOverlay.aidl new file mode 100644 index 0000000000..06fb5f0bd5 --- /dev/null +++ b/libs/gui/android/gui/TrustedOverlay.aidl @@ -0,0 +1,45 @@ +/** + * 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. + */ + +package android.gui; + + +/** + * Trusted overlay state prevents layers from being considered as obscuring for + * input occlusion detection purposes. + * + * @hide + */ +@Backing(type="int") +enum TrustedOverlay { + /** + * The default, layer will inherit the state from its parents. If the parent state is also + * unset, the layer will be considered as untrusted. + */ + UNSET, + + /** + * Treats this layer and all its children as an untrusted overlay. This will override any + * state set by its parent layers. + */ + DISABLED, + + /** + * Treats this layer and all its children as a trusted overlay unless the child layer + * explicitly disables its trusted state. + */ + ENABLED +} diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index ebdf2326d6..849f66718a 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -30,6 +30,7 @@ #include #include +#include #include #include @@ -388,7 +389,7 @@ struct layer_state_t { // An inherited state that indicates that this surface control and its children // should be trusted for input occlusion detection purposes - bool isTrustedOverlay; + gui::TrustedOverlay trustedOverlay; // Stretch effect to be applied to this layer StretchEffect stretchEffect; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 987efe01ba..50f2d17f51 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -719,6 +719,8 @@ public: // Sets that this surface control and its children are trusted overlays for input Transaction& setTrustedOverlay(const sp& sc, bool isTrustedOverlay); + Transaction& setTrustedOverlay(const sp& sc, + gui::TrustedOverlay trustedOverlay); // Queues up transactions using this token in SurfaceFlinger. By default, all transactions // from a client are placed on the same queue. This can be used to prevent multiple diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshot.h b/services/surfaceflinger/FrontEnd/LayerSnapshot.h index eef8dff94c..398e64a4f1 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshot.h +++ b/services/surfaceflinger/FrontEnd/LayerSnapshot.h @@ -84,7 +84,7 @@ struct LayerSnapshot : public compositionengine::LayerFECompositionState { // is a mirror root bool ignoreLocalTransform; gui::DropInputMode dropInputMode; - bool isTrustedOverlay; + gui::TrustedOverlay trustedOverlay; gui::GameMode gameMode; scheduler::LayerInfo::FrameRate frameRate; scheduler::LayerInfo::FrameRate inheritedFrameRate; diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp index f10bb33a2f..87706d8491 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp +++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -373,7 +374,7 @@ LayerSnapshot LayerSnapshotBuilder::getRootSnapshot() { snapshot.relativeLayerMetadata.mMap.clear(); snapshot.inputInfo.touchOcclusionMode = gui::TouchOcclusionMode::BLOCK_UNTRUSTED; snapshot.dropInputMode = gui::DropInputMode::NONE; - snapshot.isTrustedOverlay = false; + snapshot.trustedOverlay = gui::TrustedOverlay::UNSET; snapshot.gameMode = gui::GameMode::Unsupported; snapshot.frameRate = {}; snapshot.fixedTransformHint = ui::Transform::ROT_INVALID; @@ -750,7 +751,19 @@ void LayerSnapshotBuilder::updateSnapshot(LayerSnapshot& snapshot, const Args& a } if (forceUpdate || snapshot.clientChanges & layer_state_t::eTrustedOverlayChanged) { - snapshot.isTrustedOverlay = parentSnapshot.isTrustedOverlay || requested.isTrustedOverlay; + switch (requested.trustedOverlay) { + case gui::TrustedOverlay::UNSET: + snapshot.trustedOverlay = parentSnapshot.trustedOverlay; + break; + case gui::TrustedOverlay::DISABLED: + snapshot.trustedOverlay = FlagManager::getInstance().override_trusted_overlay() + ? requested.trustedOverlay + : parentSnapshot.trustedOverlay; + break; + case gui::TrustedOverlay::ENABLED: + snapshot.trustedOverlay = requested.trustedOverlay; + break; + } } if (snapshot.isHiddenByPolicyFromParent && @@ -1125,7 +1138,7 @@ void LayerSnapshotBuilder::updateInput(LayerSnapshot& snapshot, // Inherit the trusted state from the parent hierarchy, but don't clobber the trusted state // if it was set by WM for a known system overlay - if (snapshot.isTrustedOverlay) { + if (snapshot.trustedOverlay == gui::TrustedOverlay::ENABLED) { snapshot.inputInfo.inputConfig |= InputConfig::TRUSTED_OVERLAY; } diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp index f5e5b0233e..3cf45a7fbb 100644 --- a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp +++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp @@ -119,7 +119,7 @@ RequestedLayerState::RequestedLayerState(const LayerCreationArgs& args) shadowRadius = 0.f; fixedTransformHint = ui::Transform::ROT_INVALID; destinationFrame.makeInvalid(); - isTrustedOverlay = false; + trustedOverlay = gui::TrustedOverlay::UNSET; dropInputMode = gui::DropInputMode::NONE; dimmingEnabled = true; defaultFrameRateCompatibility = static_cast(scheduler::FrameRateCompatibility::Default); diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 363b35c4f3..6b97e2f799 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -3902,7 +3902,7 @@ bool Layer::isSimpleBufferUpdate(const layer_state_t& s) const { } if (s.what & layer_state_t::eTrustedOverlayChanged) { - if (mDrawingState.isTrustedOverlay != s.isTrustedOverlay) { + if (mDrawingState.isTrustedOverlay != (s.trustedOverlay == gui::TrustedOverlay::ENABLED)) { ATRACE_FORMAT_INSTANT("%s: false [eTrustedOverlayChanged changed]", __func__); return false; } diff --git a/services/surfaceflinger/LayerProtoHelper.cpp b/services/surfaceflinger/LayerProtoHelper.cpp index 753886ad24..496033b749 100644 --- a/services/surfaceflinger/LayerProtoHelper.cpp +++ b/services/surfaceflinger/LayerProtoHelper.cpp @@ -382,7 +382,8 @@ void LayerProtoHelper::writeSnapshotToProto(perfetto::protos::LayerProto* layerI layerInfo->set_corner_radius( (snapshot.roundedCorner.radius.x + snapshot.roundedCorner.radius.y) / 2.0); layerInfo->set_background_blur_radius(snapshot.backgroundBlurRadius); - layerInfo->set_is_trusted_overlay(snapshot.isTrustedOverlay); + layerInfo->set_is_trusted_overlay(snapshot.trustedOverlay == gui::TrustedOverlay::ENABLED); + // TODO(b/339701674) update protos LayerProtoHelper::writeToProtoDeprecated(transform, layerInfo->mutable_transform()); LayerProtoHelper::writePositionToProto(transform.tx(), transform.ty(), [&]() { return layerInfo->mutable_position(); }); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 28d80188ff..059edad467 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5700,7 +5700,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime if (layer->setHdrMetadata(s.hdrMetadata)) flags |= eTraversalNeeded; } if (what & layer_state_t::eTrustedOverlayChanged) { - if (layer->setTrustedOverlay(s.isTrustedOverlay)) { + if (layer->setTrustedOverlay(s.trustedOverlay == gui::TrustedOverlay::ENABLED)) { flags |= eTraversalNeeded; } } diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp index b3e9fab261..fc496b2982 100644 --- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp +++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp @@ -247,7 +247,8 @@ perfetto::protos::LayerState TransactionProtoParser::toProto( proto.set_auto_refresh(layer.autoRefresh); } if (layer.what & layer_state_t::eTrustedOverlayChanged) { - proto.set_is_trusted_overlay(layer.isTrustedOverlay); + proto.set_is_trusted_overlay(layer.trustedOverlay == gui::TrustedOverlay::ENABLED); + // TODO(b/339701674) update protos } if (layer.what & layer_state_t::eBufferCropChanged) { LayerProtoHelper::writeToProto(layer.bufferCrop, proto.mutable_buffer_crop()); @@ -515,7 +516,8 @@ void TransactionProtoParser::fromProto(const perfetto::protos::LayerState& proto layer.autoRefresh = proto.auto_refresh(); } if (proto.what() & layer_state_t::eTrustedOverlayChanged) { - layer.isTrustedOverlay = proto.is_trusted_overlay(); + layer.trustedOverlay = proto.is_trusted_overlay() ? gui::TrustedOverlay::ENABLED + : gui::TrustedOverlay::UNSET; } if (proto.what() & layer_state_t::eBufferCropChanged) { LayerProtoHelper::readFromProto(proto.buffer_crop(), layer.bufferCrop); diff --git a/services/surfaceflinger/common/FlagManager.cpp b/services/surfaceflinger/common/FlagManager.cpp index 121629fd10..57b170fba3 100644 --- a/services/surfaceflinger/common/FlagManager.cpp +++ b/services/surfaceflinger/common/FlagManager.cpp @@ -147,6 +147,7 @@ void FlagManager::dump(std::string& result) const { DUMP_READ_ONLY_FLAG(detached_mirror); DUMP_READ_ONLY_FLAG(commit_not_composited); DUMP_READ_ONLY_FLAG(local_tonemap_screenshots); + DUMP_READ_ONLY_FLAG(override_trusted_overlay); #undef DUMP_READ_ONLY_FLAG #undef DUMP_SERVER_FLAG @@ -244,6 +245,7 @@ FLAG_MANAGER_READ_ONLY_FLAG(allow_n_vsyncs_in_targeter, ""); FLAG_MANAGER_READ_ONLY_FLAG(detached_mirror, ""); FLAG_MANAGER_READ_ONLY_FLAG(commit_not_composited, ""); FLAG_MANAGER_READ_ONLY_FLAG(local_tonemap_screenshots, "debug.sf.local_tonemap_screenshots"); +FLAG_MANAGER_READ_ONLY_FLAG(override_trusted_overlay, ""); /// Trunk stable server flags /// FLAG_MANAGER_SERVER_FLAG(refresh_rate_overlay_on_external_display, "") diff --git a/services/surfaceflinger/common/include/common/FlagManager.h b/services/surfaceflinger/common/include/common/FlagManager.h index 4cf4453980..9517ff7efb 100644 --- a/services/surfaceflinger/common/include/common/FlagManager.h +++ b/services/surfaceflinger/common/include/common/FlagManager.h @@ -86,6 +86,7 @@ public: bool detached_mirror() const; bool commit_not_composited() const; bool local_tonemap_screenshots() const; + bool override_trusted_overlay() const; protected: // overridden for unit tests diff --git a/services/surfaceflinger/surfaceflinger_flags_new.aconfig b/services/surfaceflinger/surfaceflinger_flags_new.aconfig index 5b94f07dff..02d8819785 100644 --- a/services/surfaceflinger/surfaceflinger_flags_new.aconfig +++ b/services/surfaceflinger/surfaceflinger_flags_new.aconfig @@ -52,7 +52,7 @@ flag { metadata { purpose: PURPOSE_BUGFIX } -} # deprecate_vsync_sf +} # detached_mirror flag { name: "frame_rate_category_mrr" @@ -84,6 +84,17 @@ flag { is_fixed_read_only: true } # local_tonemap_screenshots + flag { + name: "override_trusted_overlay" + namespace: "window_surfaces" + description: "Allow child to disable trusted overlay set by a parent layer" + bug: "339701674" + is_fixed_read_only: true + metadata { + purpose: PURPOSE_BUGFIX + } +} # override_trusted_overlay + flag { name: "vrr_bugfix_24q4" namespace: "core_graphics" diff --git a/services/surfaceflinger/tests/unittests/LayerHierarchyTest.h b/services/surfaceflinger/tests/unittests/LayerHierarchyTest.h index fd15eef54b..8b3303c809 100644 --- a/services/surfaceflinger/tests/unittests/LayerHierarchyTest.h +++ b/services/surfaceflinger/tests/unittests/LayerHierarchyTest.h @@ -494,14 +494,14 @@ protected: mLifecycleManager.applyTransactions(transactions); } - void setTrustedOverlay(uint32_t id, bool isTrustedOverlay) { + void setTrustedOverlay(uint32_t id, gui::TrustedOverlay trustedOverlay) { std::vector transactions; transactions.emplace_back(); transactions.back().states.push_back({}); transactions.back().states.front().state.what = layer_state_t::eTrustedOverlayChanged; transactions.back().states.front().layerId = id; - transactions.back().states.front().state.isTrustedOverlay = isTrustedOverlay; + transactions.back().states.front().state.trustedOverlay = trustedOverlay; mLifecycleManager.applyTransactions(transactions); } diff --git a/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp b/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp index ce4d798dd8..8acabe3b54 100644 --- a/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp +++ b/services/surfaceflinger/tests/unittests/LayerSnapshotTest.cpp @@ -1247,7 +1247,7 @@ TEST_F(LayerSnapshotTest, setShadowRadius) { TEST_F(LayerSnapshotTest, setTrustedOverlayForNonVisibleInput) { hideLayer(1); - setTrustedOverlay(1, true); + setTrustedOverlay(1, gui::TrustedOverlay::ENABLED); Region touch{Rect{0, 0, 1000, 1000}}; setTouchableRegion(1, touch); @@ -1439,4 +1439,85 @@ TEST_F(LayerSnapshotTest, mirroredHierarchyIgnoresLocalTransform) { EXPECT_EQ(getSnapshot({.id = 111})->geomLayerTransform.ty(), 220); } +TEST_F(LayerSnapshotTest, overrideParentTrustedOverlayState) { + SET_FLAG_FOR_TEST(flags::override_trusted_overlay, true); + hideLayer(1); + setTrustedOverlay(1, gui::TrustedOverlay::ENABLED); + + Region touch{Rect{0, 0, 1000, 1000}}; + setTouchableRegion(1, touch); + setTouchableRegion(11, touch); + setTouchableRegion(111, touch); + + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + + // disable trusted overlay and override parent state + setTrustedOverlay(11, gui::TrustedOverlay::DISABLED); + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_FALSE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_FALSE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + + // unset state and go back to default behavior of inheriting + // state + setTrustedOverlay(11, gui::TrustedOverlay::UNSET); + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); +} + +TEST_F(LayerSnapshotTest, doNotOverrideParentTrustedOverlayState) { + SET_FLAG_FOR_TEST(flags::override_trusted_overlay, false); + hideLayer(1); + setTrustedOverlay(1, gui::TrustedOverlay::ENABLED); + + Region touch{Rect{0, 0, 1000, 1000}}; + setTouchableRegion(1, touch); + setTouchableRegion(11, touch); + setTouchableRegion(111, touch); + + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + + // disable trusted overlay but flag is disabled so this behaves + // as UNSET + setTrustedOverlay(11, gui::TrustedOverlay::DISABLED); + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + + // unset state and go back to default behavior of inheriting + // state + setTrustedOverlay(11, gui::TrustedOverlay::UNSET); + UPDATE_AND_VERIFY(mSnapshotBuilder, {2}); + EXPECT_TRUE(getSnapshot(1)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(11)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); + EXPECT_TRUE(getSnapshot(111)->inputInfo.inputConfig.test( + gui::WindowInfo::InputConfig::TRUSTED_OVERLAY)); +} + } // namespace android::surfaceflinger::frontend -- cgit v1.2.3-59-g8ed1b From 39f510fb536d26247997141bdfdcc7d8af890514 Mon Sep 17 00:00:00 2001 From: Nergi Rahardi Date: Thu, 23 May 2024 15:16:54 +0900 Subject: Pass dequeue timestamp along with buffer data Avoids performance penalties associated with metadata. Bug: 342174822 Test: atest libsurfaceflinger_unittest Test: Manually verified dequeueTime sent without triggering metadata change Change-Id: Ifed747818ea252b2551780ffcefd3309eb41edbe --- libs/gui/BLASTBufferQueue.cpp | 24 +++++++++++----------- libs/gui/LayerState.cpp | 2 ++ libs/gui/SurfaceComposerClient.cpp | 3 ++- libs/gui/include/gui/LayerState.h | 2 ++ libs/gui/include/gui/SurfaceComposerClient.h | 3 ++- services/surfaceflinger/Layer.cpp | 8 ++++---- services/surfaceflinger/Layer.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 13 ++---------- .../Tracing/TransactionProtoParser.cpp | 1 + .../tests/unittests/TransactionFrameTracerTest.cpp | 2 +- .../unittests/TransactionSurfaceFrameTest.cpp | 24 +++++++++++----------- 11 files changed, 41 insertions(+), 43 deletions(-) (limited to 'libs/gui/SurfaceComposerClient.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index f317a2eea0..739c3c2a41 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -613,8 +613,19 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( std::bind(releaseBufferCallbackThunk, wp(this) /* callbackContext */, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3); sp fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE; + + nsecs_t dequeueTime = -1; + { + std::lock_guard _lock{mTimestampMutex}; + auto dequeueTimeIt = mDequeueTimestamps.find(buffer->getId()); + if (dequeueTimeIt != mDequeueTimestamps.end()) { + dequeueTime = dequeueTimeIt->second; + mDequeueTimestamps.erase(dequeueTimeIt); + } + } + t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, mProducerId, - releaseBufferCallback); + releaseBufferCallback, dequeueTime); t->setDataspace(mSurfaceControl, static_cast(bufferItem.mDataSpace)); t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata); t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage); @@ -658,17 +669,6 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( mPendingFrameTimelines.pop(); } - { - std::lock_guard _lock{mTimestampMutex}; - auto dequeueTime = mDequeueTimestamps.find(buffer->getId()); - if (dequeueTime != mDequeueTimestamps.end()) { - Parcel p; - p.writeInt64(dequeueTime->second); - t->setMetadata(mSurfaceControl, gui::METADATA_DEQUEUE_TIME, p); - mDequeueTimestamps.erase(dequeueTime); - } - } - mergePendingTransactions(t, bufferItem.mFrameNumber); if (applyTransaction) { // All transactions on our apply token are one-way. See comment on mAppliedLastTransaction diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index c82bde9a83..3745805aa3 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -985,6 +985,7 @@ status_t BufferData::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeBool, hasBarrier); SAFE_PARCEL(output->writeUint64, barrierFrameNumber); SAFE_PARCEL(output->writeUint32, producerId); + SAFE_PARCEL(output->writeInt64, dequeueTime); return NO_ERROR; } @@ -1024,6 +1025,7 @@ status_t BufferData::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readBool, &hasBarrier); SAFE_PARCEL(input->readUint64, &barrierFrameNumber); SAFE_PARCEL(input->readUint32, &producerId); + SAFE_PARCEL(input->readInt64, &dequeueTime); return NO_ERROR; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index b420aabb84..af91bb3ae2 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1702,7 +1702,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer( const sp& sc, const sp& buffer, const std::optional>& fence, const std::optional& optFrameNumber, - uint32_t producerId, ReleaseBufferCallback callback) { + uint32_t producerId, ReleaseBufferCallback callback, nsecs_t dequeueTime) { layer_state_t* s = getLayerState(sc); if (!s) { mStatus = BAD_INDEX; @@ -1718,6 +1718,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe bufferData->frameNumber = frameNumber; bufferData->producerId = producerId; bufferData->flags |= BufferData::BufferDataChange::frameNumberChanged; + bufferData->dequeueTime = dequeueTime; if (fence) { bufferData->acquireFence = *fence; bufferData->flags |= BufferData::BufferDataChange::fenceChanged; diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 82889efe36..ba06101059 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -128,6 +128,8 @@ public: client_cache_t cachedBuffer; + nsecs_t dequeueTime; + // Generates the release callback id based on the buffer id and frame number. // This is used as an identifier when release callbacks are invoked. ReleaseCallbackId generateReleaseCallbackId() const; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 9712907396..0862e03c44 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -568,7 +568,8 @@ public: Transaction& setBuffer(const sp& sc, const sp& buffer, const std::optional>& fence = std::nullopt, const std::optional& frameNumber = std::nullopt, - uint32_t producerId = 0, ReleaseBufferCallback callback = nullptr); + uint32_t producerId = 0, ReleaseBufferCallback callback = nullptr, + nsecs_t dequeueTime = -1); Transaction& unsetBuffer(const sp& sc); std::shared_ptr getAndClearBuffer(const sp& sc); diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 6b97e2f799..d27bfd29ef 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -3149,8 +3149,7 @@ void Layer::resetDrawingStateBufferInfo() { bool Layer::setBuffer(std::shared_ptr& buffer, const BufferData& bufferData, nsecs_t postTime, nsecs_t desiredPresentTime, - bool isAutoTimestamp, std::optional dequeueTime, - const FrameTimelineInfo& info) { + bool isAutoTimestamp, const FrameTimelineInfo& info) { ATRACE_FORMAT("setBuffer %s - hasBuffer=%s", getDebugName(), (buffer ? "true" : "false")); const bool frameNumberChanged = @@ -3224,10 +3223,11 @@ bool Layer::setBuffer(std::shared_ptr& buffer, setFrameTimelineVsyncForBufferTransaction(info, postTime); - if (dequeueTime && *dequeueTime != 0) { + if (bufferData.dequeueTime > 0) { const uint64_t bufferId = mDrawingState.buffer->getId(); mFlinger->mFrameTracer->traceNewLayer(layerId, getName().c_str()); - mFlinger->mFrameTracer->traceTimestamp(layerId, bufferId, frameNumber, *dequeueTime, + mFlinger->mFrameTracer->traceTimestamp(layerId, bufferId, frameNumber, + bufferData.dequeueTime, FrameTracer::FrameEvent::DEQUEUE); mFlinger->mFrameTracer->traceTimestamp(layerId, bufferId, frameNumber, postTime, FrameTracer::FrameEvent::QUEUE); diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 9db7664aa2..b9fcd5c333 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -312,7 +312,7 @@ public: bool setBuffer(std::shared_ptr& /* buffer */, const BufferData& /* bufferData */, nsecs_t /* postTime */, nsecs_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/, - std::optional /* dequeueTime */, const FrameTimelineInfo& /*info*/); + const FrameTimelineInfo& /*info*/); void setDesiredPresentTime(nsecs_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/); bool setDataspace(ui::Dataspace /*dataspace*/); bool setExtendedRangeBrightness(float currentBufferRatio, float desiredRatio); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 5f81cd45cc..c0faf16927 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5618,10 +5618,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime layer->setInputInfo(*s.windowInfoHandle->getInfo()); flags |= eTraversalNeeded; } - std::optional dequeueBufferTimestamp; if (what & layer_state_t::eMetadataChanged) { - dequeueBufferTimestamp = s.metadata.getInt64(gui::METADATA_DEQUEUE_TIME); - if (const int32_t gameMode = s.metadata.getInt32(gui::METADATA_GAME_MODE, -1); gameMode != -1) { // The transaction will be received on the Task layer and needs to be applied to all @@ -5763,8 +5760,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime if (what & layer_state_t::eBufferChanged) { if (layer->setBuffer(composerState.externalTexture, *s.bufferData, postTime, - desiredPresentTime, isAutoTimestamp, dequeueBufferTimestamp, - frameTimelineInfo)) { + desiredPresentTime, isAutoTimestamp, frameTimelineInfo)) { flags |= eTraversalNeeded; } } else if (frameTimelineInfo.vsyncId != FrameTimelineInfo::INVALID_VSYNC_ID) { @@ -5850,10 +5846,6 @@ uint32_t SurfaceFlinger::updateLayerCallbacksAndStats(const FrameTimelineInfo& f if (what & layer_state_t::eProducerDisconnect) { layer->onDisconnect(); } - std::optional dequeueBufferTimestamp; - if (what & layer_state_t::eMetadataChanged) { - dequeueBufferTimestamp = s.metadata.getInt64(gui::METADATA_DEQUEUE_TIME); - } std::vector> callbackHandles; if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!filteredListeners.empty())) { @@ -5900,8 +5892,7 @@ uint32_t SurfaceFlinger::updateLayerCallbacksAndStats(const FrameTimelineInfo& f } layer->setTransformHint(transformHint); if (layer->setBuffer(composerState.externalTexture, *s.bufferData, postTime, - desiredPresentTime, isAutoTimestamp, dequeueBufferTimestamp, - frameTimelineInfo)) { + desiredPresentTime, isAutoTimestamp, frameTimelineInfo)) { flags |= eTraversalNeeded; } mLayersWithQueuedFrames.emplace(layer); diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp index fc496b2982..0bafb71f3f 100644 --- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp +++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp @@ -436,6 +436,7 @@ void TransactionProtoParser::fromProto(const perfetto::protos::LayerState& proto layer.bufferData->flags = ftl::Flags(bufferProto.flags()); layer.bufferData->cachedBuffer.id = bufferProto.cached_buffer_id(); layer.bufferData->acquireFence = Fence::NO_FENCE; + layer.bufferData->dequeueTime = -1; } if (proto.what() & layer_state_t::eApiChanged) { diff --git a/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp b/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp index d4d5b32341..85b61f8fb9 100644 --- a/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp @@ -94,7 +94,7 @@ public: HAL_PIXEL_FORMAT_RGBA_8888, 0ULL /*usage*/); layer->setBuffer(externalTexture, bufferData, postTime, /*desiredPresentTime*/ 30, false, - dequeueTime, FrameTimelineInfo{}); + FrameTimelineInfo{}); commitTransaction(layer.get()); nsecs_t latchTime = 25; diff --git a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp index 5046a86304..46733b9a83 100644 --- a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp @@ -99,7 +99,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture, bufferData, 10, 20, false, ftInfo); acquireFence->signalForTest(12); commitTransaction(layer.get()); @@ -134,7 +134,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture1, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture1, bufferData, 10, 20, false, ftInfo); EXPECT_EQ(0u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); const auto droppedSurfaceFrame = layer->mDrawingState.bufferSurfaceFrameTX; @@ -151,7 +151,7 @@ public: 2ULL /* bufferId */, HAL_PIXEL_FORMAT_RGBA_8888, 0ULL /*usage*/); - layer->setBuffer(externalTexture2, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture2, bufferData, 10, 20, false, ftInfo); nsecs_t end = systemTime(); acquireFence2->signalForTest(12); @@ -197,7 +197,7 @@ public: 1ULL /* bufferId */, HAL_PIXEL_FORMAT_RGBA_8888, 0ULL /*usage*/); - layer->setBuffer(externalTexture, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture, bufferData, 10, 20, false, ftInfo); acquireFence->signalForTest(12); EXPECT_EQ(0u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); @@ -232,7 +232,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture, bufferData, 10, 20, false, ftInfo); EXPECT_EQ(0u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); @@ -275,7 +275,7 @@ public: FrameTimelineInfo ftInfo3; ftInfo3.vsyncId = 3; ftInfo3.inputEventId = 0; - layer->setBuffer(externalTexture, bufferData, 10, 20, false, std::nullopt, ftInfo3); + layer->setBuffer(externalTexture, bufferData, 10, 20, false, ftInfo3); EXPECT_EQ(2u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); const auto bufferSurfaceFrameTX = layer->mDrawingState.bufferSurfaceFrameTX; @@ -320,7 +320,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture1, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture1, bufferData, 10, 20, false, ftInfo); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); const auto droppedSurfaceFrame = layer->mDrawingState.bufferSurfaceFrameTX; @@ -335,7 +335,7 @@ public: 1ULL /* bufferId */, HAL_PIXEL_FORMAT_RGBA_8888, 0ULL /*usage*/); - layer->setBuffer(externalTexture2, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture2, bufferData, 10, 20, false, ftInfo); acquireFence2->signalForTest(12); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); @@ -372,7 +372,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture1, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture1, bufferData, 10, 20, false, ftInfo); EXPECT_EQ(0u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); const auto droppedSurfaceFrame1 = layer->mDrawingState.bufferSurfaceFrameTX; @@ -392,7 +392,7 @@ public: FrameTimelineInfo ftInfoInv; ftInfoInv.vsyncId = FrameTimelineInfo::INVALID_VSYNC_ID; ftInfoInv.inputEventId = 0; - layer->setBuffer(externalTexture2, bufferData, 10, 20, false, std::nullopt, ftInfoInv); + layer->setBuffer(externalTexture2, bufferData, 10, 20, false, ftInfoInv); auto dropEndTime1 = systemTime(); EXPECT_EQ(0u, layer->mDrawingState.bufferlessSurfaceFramesTX.size()); ASSERT_NE(nullptr, layer->mDrawingState.bufferSurfaceFrameTX); @@ -413,7 +413,7 @@ public: FrameTimelineInfo ftInfo2; ftInfo2.vsyncId = 2; ftInfo2.inputEventId = 0; - layer->setBuffer(externalTexture3, bufferData, 10, 20, false, std::nullopt, ftInfo2); + layer->setBuffer(externalTexture3, bufferData, 10, 20, false, ftInfo2); auto dropEndTime2 = systemTime(); acquireFence3->signalForTest(12); @@ -462,7 +462,7 @@ public: FrameTimelineInfo ftInfo; ftInfo.vsyncId = 1; ftInfo.inputEventId = 0; - layer->setBuffer(externalTexture, bufferData, 10, 20, false, std::nullopt, ftInfo); + layer->setBuffer(externalTexture, bufferData, 10, 20, false, ftInfo); FrameTimelineInfo ftInfo2; ftInfo2.vsyncId = 2; ftInfo2.inputEventId = 0; -- cgit v1.2.3-59-g8ed1b