From 83d1c72376a9223d36ed685c3f88b40ddcb76b8f Mon Sep 17 00:00:00 2001 From: Evan Severson Date: Tue, 4 Apr 2023 14:46:06 -0700 Subject: Add AppOps overload to be able to watch foreground changes. We have never offered the native API to register mode watchers that are invoked for foregroundness changes when the raw mode is MODE_FOREGROUND. Test: Add logging to verify invocation Bug: 247768581 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:949cb3d098fd98715826fc92ea3c26a51aa2d976) Merged-In: I89af46de557fbfc31d69613367a4e26a5222430a Change-Id: I89af46de557fbfc31d69613367a4e26a5222430a --- libs/permission/AppOpsManager.cpp | 8 ++++++++ libs/permission/IAppOpsService.cpp | 11 +++++++++++ libs/permission/include/binder/AppOpsManager.h | 6 ++++++ libs/permission/include/binder/IAppOpsService.h | 3 +++ 4 files changed, 28 insertions(+) diff --git a/libs/permission/AppOpsManager.cpp b/libs/permission/AppOpsManager.cpp index baa9d75116..695927418d 100644 --- a/libs/permission/AppOpsManager.cpp +++ b/libs/permission/AppOpsManager.cpp @@ -146,6 +146,14 @@ void AppOpsManager::startWatchingMode(int32_t op, const String16& packageName, } } +void AppOpsManager::startWatchingMode(int32_t op, const String16& packageName, int32_t flags, + const sp& callback) { + sp service = getService(); + if (service != nullptr) { + service->startWatchingModeWithFlags(op, packageName, flags, callback); + } +} + void AppOpsManager::stopWatchingMode(const sp& callback) { sp service = getService(); if (service != nullptr) { diff --git a/libs/permission/IAppOpsService.cpp b/libs/permission/IAppOpsService.cpp index d59f44562e..7f235a4541 100644 --- a/libs/permission/IAppOpsService.cpp +++ b/libs/permission/IAppOpsService.cpp @@ -166,6 +166,17 @@ public: } return reply.readBool(); } + + virtual void startWatchingModeWithFlags(int32_t op, const String16& packageName, + int32_t flags, const sp& callback) { + Parcel data, reply; + data.writeInterfaceToken(IAppOpsService::getInterfaceDescriptor()); + data.writeInt32(op); + data.writeString16(packageName); + data.writeInt32(flags); + data.writeStrongBinder(IInterface::asBinder(callback)); + remote()->transact(START_WATCHING_MODE_WITH_FLAGS_TRANSACTION, data, &reply); + } }; IMPLEMENT_META_INTERFACE(AppOpsService, "com.android.internal.app.IAppOpsService") diff --git a/libs/permission/include/binder/AppOpsManager.h b/libs/permission/include/binder/AppOpsManager.h index abcd527966..243532bc4d 100644 --- a/libs/permission/include/binder/AppOpsManager.h +++ b/libs/permission/include/binder/AppOpsManager.h @@ -151,6 +151,10 @@ public: _NUM_OP = 117 }; + enum { + WATCH_FOREGROUND_CHANGES = 1 << 0 + }; + AppOpsManager(); int32_t checkOp(int32_t op, int32_t uid, const String16& callingPackage); @@ -174,6 +178,8 @@ public: const std::optional& attributionTag); void startWatchingMode(int32_t op, const String16& packageName, const sp& callback); + void startWatchingMode(int32_t op, const String16& packageName, int32_t flags, + const sp& callback); void stopWatchingMode(const sp& callback); int32_t permissionToOpCode(const String16& permission); void setCameraAudioRestriction(int32_t mode); diff --git a/libs/permission/include/binder/IAppOpsService.h b/libs/permission/include/binder/IAppOpsService.h index 22f056b235..918fcdbce1 100644 --- a/libs/permission/include/binder/IAppOpsService.h +++ b/libs/permission/include/binder/IAppOpsService.h @@ -52,6 +52,8 @@ public: const String16& packageName) = 0; virtual void setCameraAudioRestriction(int32_t mode) = 0; virtual bool shouldCollectNotes(int32_t opCode) = 0; + virtual void startWatchingModeWithFlags(int32_t op, const String16& packageName, + int32_t flags, const sp& callback) = 0; enum { CHECK_OPERATION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION, @@ -64,6 +66,7 @@ public: CHECK_AUDIO_OPERATION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+7, SHOULD_COLLECT_NOTES_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+8, SET_CAMERA_AUDIO_RESTRICTION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+9, + START_WATCHING_MODE_WITH_FLAGS_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+10, }; enum { -- cgit v1.2.3 From 130ff33dfeb3c0702636db090399f114fde86174 Mon Sep 17 00:00:00 2001 From: Evan Severson Date: Tue, 4 Apr 2023 14:46:06 -0700 Subject: Add AppOps overload to be able to watch foreground changes. We have never offered the native API to register mode watchers that are invoked for foregroundness changes when the raw mode is MODE_FOREGROUND. Test: Add logging to verify invocation Bug: 247768581 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:949cb3d098fd98715826fc92ea3c26a51aa2d976) Merged-In: I89af46de557fbfc31d69613367a4e26a5222430a Change-Id: I89af46de557fbfc31d69613367a4e26a5222430a --- libs/permission/AppOpsManager.cpp | 8 ++++++++ libs/permission/IAppOpsService.cpp | 11 +++++++++++ libs/permission/include/binder/AppOpsManager.h | 6 ++++++ libs/permission/include/binder/IAppOpsService.h | 3 +++ 4 files changed, 28 insertions(+) diff --git a/libs/permission/AppOpsManager.cpp b/libs/permission/AppOpsManager.cpp index baa9d75116..695927418d 100644 --- a/libs/permission/AppOpsManager.cpp +++ b/libs/permission/AppOpsManager.cpp @@ -146,6 +146,14 @@ void AppOpsManager::startWatchingMode(int32_t op, const String16& packageName, } } +void AppOpsManager::startWatchingMode(int32_t op, const String16& packageName, int32_t flags, + const sp& callback) { + sp service = getService(); + if (service != nullptr) { + service->startWatchingModeWithFlags(op, packageName, flags, callback); + } +} + void AppOpsManager::stopWatchingMode(const sp& callback) { sp service = getService(); if (service != nullptr) { diff --git a/libs/permission/IAppOpsService.cpp b/libs/permission/IAppOpsService.cpp index d59f44562e..7f235a4541 100644 --- a/libs/permission/IAppOpsService.cpp +++ b/libs/permission/IAppOpsService.cpp @@ -166,6 +166,17 @@ public: } return reply.readBool(); } + + virtual void startWatchingModeWithFlags(int32_t op, const String16& packageName, + int32_t flags, const sp& callback) { + Parcel data, reply; + data.writeInterfaceToken(IAppOpsService::getInterfaceDescriptor()); + data.writeInt32(op); + data.writeString16(packageName); + data.writeInt32(flags); + data.writeStrongBinder(IInterface::asBinder(callback)); + remote()->transact(START_WATCHING_MODE_WITH_FLAGS_TRANSACTION, data, &reply); + } }; IMPLEMENT_META_INTERFACE(AppOpsService, "com.android.internal.app.IAppOpsService") diff --git a/libs/permission/include/binder/AppOpsManager.h b/libs/permission/include/binder/AppOpsManager.h index e3d705fa35..97b91ed523 100644 --- a/libs/permission/include/binder/AppOpsManager.h +++ b/libs/permission/include/binder/AppOpsManager.h @@ -150,6 +150,10 @@ public: _NUM_OP = 116 }; + enum { + WATCH_FOREGROUND_CHANGES = 1 << 0 + }; + AppOpsManager(); int32_t checkOp(int32_t op, int32_t uid, const String16& callingPackage); @@ -173,6 +177,8 @@ public: const std::optional& attributionTag); void startWatchingMode(int32_t op, const String16& packageName, const sp& callback); + void startWatchingMode(int32_t op, const String16& packageName, int32_t flags, + const sp& callback); void stopWatchingMode(const sp& callback); int32_t permissionToOpCode(const String16& permission); void setCameraAudioRestriction(int32_t mode); diff --git a/libs/permission/include/binder/IAppOpsService.h b/libs/permission/include/binder/IAppOpsService.h index 22f056b235..918fcdbce1 100644 --- a/libs/permission/include/binder/IAppOpsService.h +++ b/libs/permission/include/binder/IAppOpsService.h @@ -52,6 +52,8 @@ public: const String16& packageName) = 0; virtual void setCameraAudioRestriction(int32_t mode) = 0; virtual bool shouldCollectNotes(int32_t opCode) = 0; + virtual void startWatchingModeWithFlags(int32_t op, const String16& packageName, + int32_t flags, const sp& callback) = 0; enum { CHECK_OPERATION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION, @@ -64,6 +66,7 @@ public: CHECK_AUDIO_OPERATION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+7, SHOULD_COLLECT_NOTES_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+8, SET_CAMERA_AUDIO_RESTRICTION_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+9, + START_WATCHING_MODE_WITH_FLAGS_TRANSACTION = IBinder::FIRST_CALL_TRANSACTION+10, }; enum { -- cgit v1.2.3 From e9cb5b159c6bf1c76576845d4bacd752ce79f8e1 Mon Sep 17 00:00:00 2001 From: Vladimir Komsiyski Date: Fri, 28 Jul 2023 07:13:46 +0000 Subject: Revert "Handle runtime sensor events even if there are no real ones." This reverts commit 3b9574a68afb242d573f062ce8e0a99a452d5842. Reason for revert: potential culprit for b/291461814 Bug: 291461814 Change-Id: Ib92a05135a05978d8d4e6a938df1c6fb5b8169e2 Test: m --- services/sensorservice/SensorService.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/services/sensorservice/SensorService.cpp b/services/sensorservice/SensorService.cpp index 90d75414d6..398d60242b 100644 --- a/services/sensorservice/SensorService.cpp +++ b/services/sensorservice/SensorService.cpp @@ -1055,12 +1055,7 @@ bool SensorService::threadLoop() { if (count < 0) { if(count == DEAD_OBJECT && device.isReconnecting()) { device.reconnect(); - // There are no "real" events at this point, but do not skip the rest of the loop - // if there are pending runtime events. - Mutex::Autolock _l(&mLock); - if (mRuntimeSensorEventQueue.empty()) { - continue; - } + continue; } else { ALOGE("sensor poll failed (%s)", strerror(-count)); break; -- cgit v1.2.3 From 82f3463d449eb13e28c5dbffeee16e10721c71d2 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Wed, 12 Jul 2023 13:47:28 -0500 Subject: Improve updateInputFlinger performance This change improves the performance of the WindowInfosListenerInvoker work done on SurfaceFlinger's background executor thread. The primary optimization made is not sending a WindowInfosReportedListener with every call to WindowInfosListener.onWindowInfosChanged. Instead, we send a new interface, WindowInfosPublisher, and a unique listener id to listeners when they're added. Listeners call WindowInfosPublisher.ackWindowInfosReceived with their id after processing each update. From traces taken during development, the new code is a major improvement, taking about 15% of the time spent previously on SurfaceFlinger's background thread for sending window infos. Performance with this change seems roughly in line with the performance in T. Bug: 290377931 Test: atest WindowInfosListenerTest Test: atest WindowInfosListenerInvokerTest Test: manually killing system server and checking valid state on restart Change-Id: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 (cherry picked from commit acd2258a5492a9e289fd7f4b8ea90543d6843a23) (cherry picked from commit e8a7ab25b2f2f17571279a2c2bf2ea0dff66c8e6) Merged-In: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 --- libs/gui/Android.bp | 2 + libs/gui/WindowInfosListenerReporter.cpp | 20 +- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 4 +- .../aidl/android/gui/WindowInfosListenerInfo.aidl | 25 ++ libs/gui/android/gui/IWindowInfosListener.aidl | 4 +- libs/gui/android/gui/IWindowInfosPublisher.aidl | 23 ++ libs/gui/fuzzer/libgui_fuzzer_utils.h | 4 +- libs/gui/include/gui/ISurfaceComposer.h | 1 + libs/gui/include/gui/WindowInfosListenerReporter.h | 8 +- libs/gui/tests/Surface_test.cpp | 3 +- services/surfaceflinger/BackgroundExecutor.cpp | 14 ++ services/surfaceflinger/BackgroundExecutor.h | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 14 +- services/surfaceflinger/SurfaceFlinger.h | 7 +- .../surfaceflinger/WindowInfosListenerInvoker.cpp | 253 +++++++++++---------- .../surfaceflinger/WindowInfosListenerInvoker.h | 35 +-- .../unittests/WindowInfosListenerInvokerTest.cpp | 156 ++++++------- 17 files changed, 332 insertions(+), 242 deletions(-) create mode 100644 libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl create mode 100644 libs/gui/android/gui/IWindowInfosPublisher.aidl diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index bf34987b9e..3c8df2bb29 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -73,6 +73,7 @@ filegroup { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfo.aidl", "android/gui/WindowInfosUpdate.aidl", @@ -90,6 +91,7 @@ cc_library_static { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfosUpdate.aidl", "android/gui/WindowInfo.aidl", diff --git a/libs/gui/WindowInfosListenerReporter.cpp b/libs/gui/WindowInfosListenerReporter.cpp index 76e7b6e162..0929b8e120 100644 --- a/libs/gui/WindowInfosListenerReporter.cpp +++ b/libs/gui/WindowInfosListenerReporter.cpp @@ -22,7 +22,6 @@ namespace android { using gui::DisplayInfo; -using gui::IWindowInfosReportedListener; using gui::WindowInfo; using gui::WindowInfosListener; using gui::aidl_utils::statusTFromBinderStatus; @@ -40,8 +39,13 @@ status_t WindowInfosListenerReporter::addWindowInfosListener( { std::scoped_lock lock(mListenersMutex); if (mWindowInfosListeners.empty()) { - binder::Status s = surfaceComposer->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + binder::Status s = surfaceComposer->addWindowInfosListener(this, &listenerInfo); status = statusTFromBinderStatus(s); + if (status == OK) { + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; + } } if (status == OK) { @@ -85,8 +89,7 @@ status_t WindowInfosListenerReporter::removeWindowInfosListener( } binder::Status WindowInfosListenerReporter::onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& windowInfosReportedListener) { + const gui::WindowInfosUpdate& update) { std::unordered_set, gui::SpHash> windowInfosListeners; @@ -104,9 +107,7 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( listener->onWindowInfosChanged(update); } - if (windowInfosReportedListener) { - windowInfosReportedListener->onWindowInfosReported(); - } + mWindowInfosPublisher->ackWindowInfosReceived(update.vsyncId, mListenerId); return binder::Status::ok(); } @@ -114,7 +115,10 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( void WindowInfosListenerReporter::reconnect(const sp& composerService) { std::scoped_lock lock(mListenersMutex); if (!mWindowInfosListeners.empty()) { - composerService->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + composerService->addWindowInfosListener(this, &listenerInfo); + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; } } diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index ec3266ca83..539a1c140e 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -40,12 +40,14 @@ import android.gui.IScreenCaptureListener; import android.gui.ISurfaceComposerClient; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; +import android.gui.IWindowInfosPublisher; import android.gui.LayerCaptureArgs; import android.gui.LayerDebugInfo; import android.gui.OverlayProperties; import android.gui.PullAtomData; import android.gui.ARect; import android.gui.StaticDisplayInfo; +import android.gui.WindowInfosListenerInfo; /** @hide */ interface ISurfaceComposer { @@ -500,7 +502,7 @@ interface ISurfaceComposer { */ int getMaxAcquiredBufferCount(); - void addWindowInfosListener(IWindowInfosListener windowInfosListener); + WindowInfosListenerInfo addWindowInfosListener(IWindowInfosListener windowInfosListener); void removeWindowInfosListener(IWindowInfosListener windowInfosListener); diff --git a/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl new file mode 100644 index 0000000000..0ca13b768a --- /dev/null +++ b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl @@ -0,0 +1,25 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +import android.gui.IWindowInfosPublisher; + +/** @hide */ +parcelable WindowInfosListenerInfo { + long listenerId; + IWindowInfosPublisher windowInfosPublisher; +} \ No newline at end of file diff --git a/libs/gui/android/gui/IWindowInfosListener.aidl b/libs/gui/android/gui/IWindowInfosListener.aidl index 400229d99f..07cb5ed0e6 100644 --- a/libs/gui/android/gui/IWindowInfosListener.aidl +++ b/libs/gui/android/gui/IWindowInfosListener.aidl @@ -16,11 +16,9 @@ package android.gui; -import android.gui.IWindowInfosReportedListener; import android.gui.WindowInfosUpdate; /** @hide */ oneway interface IWindowInfosListener { - void onWindowInfosChanged( - in WindowInfosUpdate update, in @nullable IWindowInfosReportedListener windowInfosReportedListener); + void onWindowInfosChanged(in WindowInfosUpdate update); } diff --git a/libs/gui/android/gui/IWindowInfosPublisher.aidl b/libs/gui/android/gui/IWindowInfosPublisher.aidl new file mode 100644 index 0000000000..5a9c32845e --- /dev/null +++ b/libs/gui/android/gui/IWindowInfosPublisher.aidl @@ -0,0 +1,23 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +/** @hide */ +oneway interface IWindowInfosPublisher +{ + void ackWindowInfosReceived(long vsyncId, long listenerId); +} diff --git a/libs/gui/fuzzer/libgui_fuzzer_utils.h b/libs/gui/fuzzer/libgui_fuzzer_utils.h index 8c003d8ad4..4c7d0562af 100644 --- a/libs/gui/fuzzer/libgui_fuzzer_utils.h +++ b/libs/gui/fuzzer/libgui_fuzzer_utils.h @@ -153,8 +153,8 @@ public: MOCK_METHOD(binder::Status, setOverrideFrameRate, (int32_t, float), (override)); MOCK_METHOD(binder::Status, getGpuContextPriority, (int32_t*), (override)); MOCK_METHOD(binder::Status, getMaxAcquiredBufferCount, (int32_t*), (override)); - MOCK_METHOD(binder::Status, addWindowInfosListener, (const sp&), - (override)); + MOCK_METHOD(binder::Status, addWindowInfosListener, + (const sp&, gui::WindowInfosListenerInfo*), (override)); MOCK_METHOD(binder::Status, removeWindowInfosListener, (const sp&), (override)); MOCK_METHOD(binder::Status, getOverlaySupport, (gui::OverlayProperties*), (override)); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 7c150d53d9..3ff6735926 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include diff --git a/libs/gui/include/gui/WindowInfosListenerReporter.h b/libs/gui/include/gui/WindowInfosListenerReporter.h index 38cb108912..684e21ad96 100644 --- a/libs/gui/include/gui/WindowInfosListenerReporter.h +++ b/libs/gui/include/gui/WindowInfosListenerReporter.h @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -30,8 +30,7 @@ namespace android { class WindowInfosListenerReporter : public gui::BnWindowInfosListener { public: static sp getInstance(); - binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update, - const sp&) override; + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override; status_t addWindowInfosListener( const sp& windowInfosListener, const sp&, @@ -47,5 +46,8 @@ private: std::vector mLastWindowInfos GUARDED_BY(mListenersMutex); std::vector mLastDisplayInfos GUARDED_BY(mListenersMutex); + + sp mWindowInfosPublisher; + int64_t mListenerId; }; } // namespace android diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 096a43cd95..8d7cf07b96 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -998,7 +998,8 @@ public: } binder::Status addWindowInfosListener( - const sp& /*windowInfosListener*/) override { + const sp& /*windowInfosListener*/, + gui::WindowInfosListenerInfo* /*outInfo*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/BackgroundExecutor.cpp b/services/surfaceflinger/BackgroundExecutor.cpp index 6ddf790d47..5a1ec6f501 100644 --- a/services/surfaceflinger/BackgroundExecutor.cpp +++ b/services/surfaceflinger/BackgroundExecutor.cpp @@ -20,6 +20,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS #include +#include #include "BackgroundExecutor.h" @@ -60,4 +61,17 @@ void BackgroundExecutor::sendCallbacks(Callbacks&& tasks) { LOG_ALWAYS_FATAL_IF(sem_post(&mSemaphore), "sem_post failed"); } +void BackgroundExecutor::flushQueue() { + std::mutex mutex; + std::condition_variable cv; + bool flushComplete = false; + sendCallbacks({[&]() { + std::scoped_lock lock{mutex}; + flushComplete = true; + cv.notify_one(); + }}); + std::unique_lock lock{mutex}; + cv.wait(lock, [&]() { return flushComplete; }); +} + } // namespace android diff --git a/services/surfaceflinger/BackgroundExecutor.h b/services/surfaceflinger/BackgroundExecutor.h index 0fae5a5c93..66b7d7a1fc 100644 --- a/services/surfaceflinger/BackgroundExecutor.h +++ b/services/surfaceflinger/BackgroundExecutor.h @@ -34,6 +34,7 @@ public: // Queues callbacks onto a work queue to be executed by a background thread. // This is safe to call from multiple threads. void sendCallbacks(Callbacks&& tasks); + void flushQueue(); private: sem_t mSemaphore; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fe2db940f7..db205b8a95 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -6158,8 +6158,7 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp windowInfosDebug.maxSendDelayVsyncId.value); StringAppendF(&result, " max send delay (ns): %" PRId64 " ns\n", windowInfosDebug.maxSendDelayDuration); - StringAppendF(&result, " unsent messages: %" PRIu32 "\n", - windowInfosDebug.pendingMessageCount); + StringAppendF(&result, " unsent messages: %zu\n", windowInfosDebug.pendingMessageCount); result.append("\n"); } @@ -7992,9 +7991,9 @@ void SurfaceFlinger::onActiveDisplayChangedLocked(const DisplayDevice* inactiveD forceApplyPolicy); } -status_t SurfaceFlinger::addWindowInfosListener( - const sp& windowInfosListener) { - mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener); +status_t SurfaceFlinger::addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { + mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener, outInfo); setTransactionFlags(eInputInfoUpdateNeeded); return NO_ERROR; } @@ -9076,7 +9075,8 @@ binder::Status SurfaceComposerAIDL::getMaxAcquiredBufferCount(int32_t* buffers) } binder::Status SurfaceComposerAIDL::addWindowInfosListener( - const sp& windowInfosListener) { + const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { status_t status; const int pid = IPCThreadState::self()->getCallingPid(); const int uid = IPCThreadState::self()->getCallingUid(); @@ -9084,7 +9084,7 @@ binder::Status SurfaceComposerAIDL::addWindowInfosListener( // WindowInfosListeners if (uid == AID_SYSTEM || uid == AID_GRAPHICS || checkPermission(sAccessSurfaceFlinger, pid, uid)) { - status = mFlinger->addWindowInfosListener(windowInfosListener); + status = mFlinger->addWindowInfosListener(windowInfosListener, outInfo); } else { status = PERMISSION_DENIED; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 0bc506f1fe..d4700a4e25 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -612,7 +612,8 @@ private: status_t getMaxAcquiredBufferCount(int* buffers) const; - status_t addWindowInfosListener(const sp& windowInfosListener); + status_t addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outResult); status_t removeWindowInfosListener( const sp& windowInfosListener) const; @@ -1556,8 +1557,8 @@ public: binder::Status setOverrideFrameRate(int32_t uid, float frameRate) override; binder::Status getGpuContextPriority(int32_t* outPriority) override; binder::Status getMaxAcquiredBufferCount(int32_t* buffers) override; - binder::Status addWindowInfosListener( - const sp& windowInfosListener) override; + binder::Status addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) override; binder::Status removeWindowInfosListener( const sp& windowInfosListener) override; diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp index 20699ef123..7062a4e3a7 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp +++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp @@ -14,7 +14,9 @@ * limitations under the License. */ -#include +#include +#include +#include #include #include #include @@ -23,162 +25,130 @@ #include "BackgroundExecutor.h" #include "WindowInfosListenerInvoker.h" +#undef ATRACE_TAG +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + namespace android { using gui::DisplayInfo; using gui::IWindowInfosListener; using gui::WindowInfo; -using WindowInfosListenerVector = ftl::SmallVector, 3>; - -struct WindowInfosReportedListenerInvoker : gui::BnWindowInfosReportedListener, - IBinder::DeathRecipient { - WindowInfosReportedListenerInvoker(WindowInfosListenerVector windowInfosListeners, - WindowInfosReportedListenerSet windowInfosReportedListeners) - : mCallbacksPending(windowInfosListeners.size()), - mWindowInfosListeners(std::move(windowInfosListeners)), - mWindowInfosReportedListeners(std::move(windowInfosReportedListeners)) {} +void WindowInfosListenerInvoker::addWindowInfosListener(sp listener, + gui::WindowInfosListenerInfo* outInfo) { + int64_t listenerId = mNextListenerId++; + outInfo->listenerId = listenerId; + outInfo->windowInfosPublisher = sp::fromExisting(this); - binder::Status onWindowInfosReported() override { - if (--mCallbacksPending == 0) { - for (const auto& listener : mWindowInfosReportedListeners) { + BackgroundExecutor::getInstance().sendCallbacks( + {[this, listener = std::move(listener), listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::addWindowInfosListener"); sp asBinder = IInterface::asBinder(listener); - if (asBinder->isBinderAlive()) { - listener->onWindowInfosReported(); - } - } - - auto wpThis = wp::fromExisting(this); - for (const auto& listener : mWindowInfosListeners) { - sp binder = IInterface::asBinder(listener); - binder->unlinkToDeath(wpThis); - } - } - return binder::Status::ok(); - } - - void binderDied(const wp&) { onWindowInfosReported(); } - -private: - std::atomic mCallbacksPending; - static constexpr size_t kStaticCapacity = 3; - const WindowInfosListenerVector mWindowInfosListeners; - WindowInfosReportedListenerSet mWindowInfosReportedListeners; -}; - -void WindowInfosListenerInvoker::addWindowInfosListener(sp listener) { - sp asBinder = IInterface::asBinder(listener); - asBinder->linkToDeath(sp::fromExisting(this)); - - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.try_emplace(asBinder, std::move(listener)); + asBinder->linkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.try_emplace(asBinder, + std::make_pair(listenerId, std::move(listener))); + }}); } void WindowInfosListenerInvoker::removeWindowInfosListener( const sp& listener) { - sp asBinder = IInterface::asBinder(listener); - - std::scoped_lock lock(mListenersMutex); - asBinder->unlinkToDeath(sp::fromExisting(this)); - mWindowInfosListeners.erase(asBinder); + BackgroundExecutor::getInstance().sendCallbacks({[this, listener]() { + ATRACE_NAME("WindowInfosListenerInvoker::removeWindowInfosListener"); + sp asBinder = IInterface::asBinder(listener); + asBinder->unlinkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.erase(asBinder); + }}); } void WindowInfosListenerInvoker::binderDied(const wp& who) { - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.erase(who); + BackgroundExecutor::getInstance().sendCallbacks({[this, who]() { + ATRACE_NAME("WindowInfosListenerInvoker::binderDied"); + auto it = mWindowInfosListeners.find(who); + int64_t listenerId = it->second.first; + mWindowInfosListeners.erase(who); + + std::vector vsyncIds; + for (auto& [vsyncId, state] : mUnackedState) { + if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(), + listenerId) != state.unackedListenerIds.end()) { + vsyncIds.push_back(vsyncId); + } + } + + for (int64_t vsyncId : vsyncIds) { + ackWindowInfosReceived(vsyncId, listenerId); + } + }}); } void WindowInfosListenerInvoker::windowInfosChanged( gui::WindowInfosUpdate update, WindowInfosReportedListenerSet reportedListeners, bool forceImmediateCall) { - WindowInfosListenerVector listeners; - { - std::scoped_lock lock{mMessagesMutex}; - - if (!mDelayInfo) { - mDelayInfo = DelayInfo{ - .vsyncId = update.vsyncId, - .frameTime = update.timestamp, - }; - } - - // If there are unacked messages and this isn't a forced call, then return immediately. - // If a forced window infos change doesn't happen first, the update will be sent after - // the WindowInfosReportedListeners are called. If a forced window infos change happens or - // if there are subsequent delayed messages before this update is sent, then this message - // will be dropped and the listeners will only be called with the latest info. This is done - // to reduce the amount of binder memory used. - if (mActiveMessageCount > 0 && !forceImmediateCall) { - mDelayedUpdate = std::move(update); - mReportedListeners.merge(reportedListeners); - return; - } - - if (mDelayedUpdate) { - mDelayedUpdate.reset(); - } + if (!mDelayInfo) { + mDelayInfo = DelayInfo{ + .vsyncId = update.vsyncId, + .frameTime = update.timestamp, + }; + } - { - std::scoped_lock lock{mListenersMutex}; - for (const auto& [_, listener] : mWindowInfosListeners) { - listeners.push_back(listener); - } - } - if (CC_UNLIKELY(listeners.empty())) { - mReportedListeners.merge(reportedListeners); - mDelayInfo.reset(); - return; - } + // If there are unacked messages and this isn't a forced call, then return immediately. + // If a forced window infos change doesn't happen first, the update will be sent after + // the WindowInfosReportedListeners are called. If a forced window infos change happens or + // if there are subsequent delayed messages before this update is sent, then this message + // will be dropped and the listeners will only be called with the latest info. This is done + // to reduce the amount of binder memory used. + if (!mUnackedState.empty() && !forceImmediateCall) { + mDelayedUpdate = std::move(update); + mReportedListeners.merge(reportedListeners); + return; + } - reportedListeners.insert(sp::fromExisting(this)); - reportedListeners.merge(mReportedListeners); - mReportedListeners.clear(); + if (mDelayedUpdate) { + mDelayedUpdate.reset(); + } - mActiveMessageCount++; - updateMaxSendDelay(); + if (CC_UNLIKELY(mWindowInfosListeners.empty())) { + mReportedListeners.merge(reportedListeners); mDelayInfo.reset(); + return; } - auto reportedInvoker = - sp::make(listeners, std::move(reportedListeners)); - - for (const auto& listener : listeners) { - sp asBinder = IInterface::asBinder(listener); + reportedListeners.merge(mReportedListeners); + mReportedListeners.clear(); + + // Update mUnackedState to include the message we're about to send + auto [it, _] = mUnackedState.try_emplace(update.vsyncId, + UnackedState{.reportedListeners = + std::move(reportedListeners)}); + auto& unackedState = it->second; + for (auto& pair : mWindowInfosListeners) { + int64_t listenerId = pair.second.first; + unackedState.unackedListenerIds.push_back(listenerId); + } - // linkToDeath is used here to ensure that the windowInfosReportedListeners - // are called even if one of the windowInfosListeners dies before - // calling onWindowInfosReported. - asBinder->linkToDeath(reportedInvoker); + mDelayInfo.reset(); + updateMaxSendDelay(); - auto status = listener->onWindowInfosChanged(update, reportedInvoker); + // Call the listeners + for (auto& pair : mWindowInfosListeners) { + auto& [listenerId, listener] = pair.second; + auto status = listener->onWindowInfosChanged(update); if (!status.isOk()) { - reportedInvoker->onWindowInfosReported(); + ackWindowInfosReceived(update.vsyncId, listenerId); } } } -binder::Status WindowInfosListenerInvoker::onWindowInfosReported() { - BackgroundExecutor::getInstance().sendCallbacks({[this]() { - gui::WindowInfosUpdate update; - { - std::scoped_lock lock{mMessagesMutex}; - mActiveMessageCount--; - if (!mDelayedUpdate || mActiveMessageCount > 0) { - return; - } - update = std::move(*mDelayedUpdate); - mDelayedUpdate.reset(); - } - windowInfosChanged(std::move(update), {}, false); - }}); - return binder::Status::ok(); -} - WindowInfosListenerInvoker::DebugInfo WindowInfosListenerInvoker::getDebugInfo() { - std::scoped_lock lock{mMessagesMutex}; - updateMaxSendDelay(); - mDebugInfo.pendingMessageCount = mActiveMessageCount; - return mDebugInfo; + DebugInfo result; + BackgroundExecutor::getInstance().sendCallbacks({[&, this]() { + ATRACE_NAME("WindowInfosListenerInvoker::getDebugInfo"); + updateMaxSendDelay(); + result = mDebugInfo; + result.pendingMessageCount = mUnackedState.size(); + }}); + BackgroundExecutor::getInstance().flushQueue(); + return result; } void WindowInfosListenerInvoker::updateMaxSendDelay() { @@ -192,4 +162,41 @@ void WindowInfosListenerInvoker::updateMaxSendDelay() { } } +binder::Status WindowInfosListenerInvoker::ackWindowInfosReceived(int64_t vsyncId, + int64_t listenerId) { + BackgroundExecutor::getInstance().sendCallbacks({[this, vsyncId, listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::ackWindowInfosReceived"); + auto it = mUnackedState.find(vsyncId); + if (it == mUnackedState.end()) { + return; + } + + auto& state = it->second; + state.unackedListenerIds.unstable_erase(std::find(state.unackedListenerIds.begin(), + state.unackedListenerIds.end(), + listenerId)); + if (!state.unackedListenerIds.empty()) { + return; + } + + WindowInfosReportedListenerSet reportedListeners{std::move(state.reportedListeners)}; + mUnackedState.erase(vsyncId); + + for (const auto& reportedListener : reportedListeners) { + sp asBinder = IInterface::asBinder(reportedListener); + if (asBinder->isBinderAlive()) { + reportedListener->onWindowInfosReported(); + } + } + + if (!mDelayedUpdate || !mUnackedState.empty()) { + return; + } + gui::WindowInfosUpdate update{std::move(*mDelayedUpdate)}; + mDelayedUpdate.reset(); + windowInfosChanged(std::move(update), {}, false); + }}); + return binder::Status::ok(); +} + } // namespace android diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h index bc465a3a2b..f36b0edd7d 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.h +++ b/services/surfaceflinger/WindowInfosListenerInvoker.h @@ -19,11 +19,12 @@ #include #include -#include +#include #include #include #include #include +#include #include #include @@ -35,22 +36,22 @@ using WindowInfosReportedListenerSet = std::unordered_set, gui::SpHash>; -class WindowInfosListenerInvoker : public gui::BnWindowInfosReportedListener, +class WindowInfosListenerInvoker : public gui::BnWindowInfosPublisher, public IBinder::DeathRecipient { public: - void addWindowInfosListener(sp); + void addWindowInfosListener(sp, gui::WindowInfosListenerInfo*); void removeWindowInfosListener(const sp& windowInfosListener); void windowInfosChanged(gui::WindowInfosUpdate update, WindowInfosReportedListenerSet windowInfosReportedListeners, bool forceImmediateCall); - binder::Status onWindowInfosReported() override; + binder::Status ackWindowInfosReceived(int64_t, int64_t) override; struct DebugInfo { VsyncId maxSendDelayVsyncId; nsecs_t maxSendDelayDuration; - uint32_t pendingMessageCount; + size_t pendingMessageCount; }; DebugInfo getDebugInfo(); @@ -58,24 +59,28 @@ protected: void binderDied(const wp& who) override; private: - std::mutex mListenersMutex; - static constexpr size_t kStaticCapacity = 3; - ftl::SmallMap, const sp, kStaticCapacity> - mWindowInfosListeners GUARDED_BY(mListenersMutex); + std::atomic mNextListenerId{0}; + ftl::SmallMap, const std::pair>, + kStaticCapacity> + mWindowInfosListeners; - std::mutex mMessagesMutex; - uint32_t mActiveMessageCount GUARDED_BY(mMessagesMutex) = 0; - std::optional mDelayedUpdate GUARDED_BY(mMessagesMutex); + std::optional mDelayedUpdate; WindowInfosReportedListenerSet mReportedListeners; - DebugInfo mDebugInfo GUARDED_BY(mMessagesMutex); + struct UnackedState { + ftl::SmallVector unackedListenerIds; + WindowInfosReportedListenerSet reportedListeners; + }; + ftl::SmallMap mUnackedState; + + DebugInfo mDebugInfo; struct DelayInfo { int64_t vsyncId; nsecs_t frameTime; }; - std::optional mDelayInfo GUARDED_BY(mMessagesMutex); - void updateMaxSendDelay() REQUIRES(mMessagesMutex); + std::optional mDelayInfo; + void updateMaxSendDelay(); }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp index af4971b063..c7b845e668 100644 --- a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp +++ b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp @@ -15,35 +15,23 @@ protected: WindowInfosListenerInvokerTest() : mInvoker(sp::make()) {} ~WindowInfosListenerInvokerTest() { - std::mutex mutex; - std::condition_variable cv; - bool flushComplete = false; // Flush the BackgroundExecutor thread to ensure any scheduled tasks are complete. // Otherwise, references those tasks hold may go out of scope before they are done // executing. - BackgroundExecutor::getInstance().sendCallbacks({[&]() { - std::scoped_lock lock{mutex}; - flushComplete = true; - cv.notify_one(); - }}); - std::unique_lock lock{mutex}; - cv.wait(lock, [&]() { return flushComplete; }); + BackgroundExecutor::getInstance().flushQueue(); } sp mInvoker; }; -using WindowInfosUpdateConsumer = std::function&)>; +using WindowInfosUpdateConsumer = std::function; class Listener : public gui::BnWindowInfosListener { public: Listener(WindowInfosUpdateConsumer consumer) : mConsumer(std::move(consumer)) {} - binder::Status onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& reportedListener) override { - mConsumer(update, reportedListener); + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override { + mConsumer(update); return binder::Status::ok(); } @@ -58,15 +46,17 @@ TEST_F(WindowInfosListenerInvokerTest, callsSingleListener) { int callCount = 0; - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); - reportedListener->onWindowInfosReported(); - })); + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo.listenerId); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks( {[this]() { mInvoker->windowInfosChanged({}, {}, false); }}); @@ -81,21 +71,27 @@ TEST_F(WindowInfosListenerInvokerTest, callsMultipleListeners) { std::mutex mutex; std::condition_variable cv; - int callCount = 0; - const int expectedCallCount = 3; - - for (int i = 0; i < expectedCallCount; i++) { - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - - reportedListener->onWindowInfosReported(); - })); + size_t callCount = 0; + const size_t expectedCallCount = 3; + std::vector listenerInfos{expectedCallCount, + gui::WindowInfosListenerInfo{}}; + + for (size_t i = 0; i < expectedCallCount; i++) { + mInvoker->addWindowInfosListener(sp::make([&, &listenerInfo = listenerInfos[i]]( + const gui::WindowInfosUpdate& + update) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo + .listenerId); + }), + &listenerInfos[i]); } BackgroundExecutor::getInstance().sendCallbacks( @@ -114,17 +110,20 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { int callCount = 0; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, + false); }}); { @@ -134,7 +133,7 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { EXPECT_EQ(callCount, 1); // Ack the first message. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(0, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -152,19 +151,21 @@ TEST_F(WindowInfosListenerInvokerTest, sendsForcedMessage) { int callCount = 0; const int expectedCallCount = 2; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, true); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, true); }}); { @@ -182,14 +183,14 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { int64_t lastUpdateId = -1; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate& update, - const sp&) { - std::scoped_lock lock{mutex}; - lastUpdateId = update.vsyncId; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + lastUpdateId = update.vsyncId; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({{}, {}, /* vsyncId= */ 1, 0}, {}, false); @@ -204,7 +205,7 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { EXPECT_EQ(lastUpdateId, 1); // Ack the first message. The third update should be sent. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(1, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -225,14 +226,17 @@ TEST_F(WindowInfosListenerInvokerTest, noListeners) { // delayed. BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({}, {}, false); - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); - mInvoker->windowInfosChanged({}, {}, false); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); }}); + BackgroundExecutor::getInstance().flushQueue(); + BackgroundExecutor::getInstance().sendCallbacks( + {[&]() { mInvoker->windowInfosChanged({}, {}, false); }}); { std::unique_lock lock{mutex}; -- cgit v1.2.3 From a8ce8fc1d892ba7dcb27961b7bf49cc6433da7bb Mon Sep 17 00:00:00 2001 From: Ken Chen Date: Thu, 31 Aug 2023 01:53:33 +0000 Subject: Revert "Improve updateInputFlinger performance" This reverts commit 82f3463d449eb13e28c5dbffeee16e10721c71d2. Reason for revert: Broken Build 10741338 on git_udc-mainline-prod on mainline_modules_arm-userdebug Bug: 298282482 Change-Id: I9a8eaaff637017cd84caaa5c0d570e24f257ed81 --- libs/gui/Android.bp | 2 - libs/gui/WindowInfosListenerReporter.cpp | 20 +- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 4 +- .../aidl/android/gui/WindowInfosListenerInfo.aidl | 25 -- libs/gui/android/gui/IWindowInfosListener.aidl | 4 +- libs/gui/android/gui/IWindowInfosPublisher.aidl | 23 -- libs/gui/fuzzer/libgui_fuzzer_utils.h | 4 +- libs/gui/include/gui/ISurfaceComposer.h | 1 - libs/gui/include/gui/WindowInfosListenerReporter.h | 8 +- libs/gui/tests/Surface_test.cpp | 3 +- services/surfaceflinger/BackgroundExecutor.cpp | 14 -- services/surfaceflinger/BackgroundExecutor.h | 1 - services/surfaceflinger/SurfaceFlinger.cpp | 14 +- services/surfaceflinger/SurfaceFlinger.h | 7 +- .../surfaceflinger/WindowInfosListenerInvoker.cpp | 253 ++++++++++----------- .../surfaceflinger/WindowInfosListenerInvoker.h | 35 ++- .../unittests/WindowInfosListenerInvokerTest.cpp | 156 +++++++------ 17 files changed, 242 insertions(+), 332 deletions(-) delete mode 100644 libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl delete mode 100644 libs/gui/android/gui/IWindowInfosPublisher.aidl diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 3c8df2bb29..bf34987b9e 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -73,7 +73,6 @@ filegroup { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", - "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfo.aidl", "android/gui/WindowInfosUpdate.aidl", @@ -91,7 +90,6 @@ cc_library_static { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", - "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfosUpdate.aidl", "android/gui/WindowInfo.aidl", diff --git a/libs/gui/WindowInfosListenerReporter.cpp b/libs/gui/WindowInfosListenerReporter.cpp index 0929b8e120..76e7b6e162 100644 --- a/libs/gui/WindowInfosListenerReporter.cpp +++ b/libs/gui/WindowInfosListenerReporter.cpp @@ -22,6 +22,7 @@ namespace android { using gui::DisplayInfo; +using gui::IWindowInfosReportedListener; using gui::WindowInfo; using gui::WindowInfosListener; using gui::aidl_utils::statusTFromBinderStatus; @@ -39,13 +40,8 @@ status_t WindowInfosListenerReporter::addWindowInfosListener( { std::scoped_lock lock(mListenersMutex); if (mWindowInfosListeners.empty()) { - gui::WindowInfosListenerInfo listenerInfo; - binder::Status s = surfaceComposer->addWindowInfosListener(this, &listenerInfo); + binder::Status s = surfaceComposer->addWindowInfosListener(this); status = statusTFromBinderStatus(s); - if (status == OK) { - mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); - mListenerId = listenerInfo.listenerId; - } } if (status == OK) { @@ -89,7 +85,8 @@ status_t WindowInfosListenerReporter::removeWindowInfosListener( } binder::Status WindowInfosListenerReporter::onWindowInfosChanged( - const gui::WindowInfosUpdate& update) { + const gui::WindowInfosUpdate& update, + const sp& windowInfosReportedListener) { std::unordered_set, gui::SpHash> windowInfosListeners; @@ -107,7 +104,9 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( listener->onWindowInfosChanged(update); } - mWindowInfosPublisher->ackWindowInfosReceived(update.vsyncId, mListenerId); + if (windowInfosReportedListener) { + windowInfosReportedListener->onWindowInfosReported(); + } return binder::Status::ok(); } @@ -115,10 +114,7 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( void WindowInfosListenerReporter::reconnect(const sp& composerService) { std::scoped_lock lock(mListenersMutex); if (!mWindowInfosListeners.empty()) { - gui::WindowInfosListenerInfo listenerInfo; - composerService->addWindowInfosListener(this, &listenerInfo); - mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); - mListenerId = listenerInfo.listenerId; + composerService->addWindowInfosListener(this); } } diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index 539a1c140e..ec3266ca83 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -40,14 +40,12 @@ import android.gui.IScreenCaptureListener; import android.gui.ISurfaceComposerClient; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; -import android.gui.IWindowInfosPublisher; import android.gui.LayerCaptureArgs; import android.gui.LayerDebugInfo; import android.gui.OverlayProperties; import android.gui.PullAtomData; import android.gui.ARect; import android.gui.StaticDisplayInfo; -import android.gui.WindowInfosListenerInfo; /** @hide */ interface ISurfaceComposer { @@ -502,7 +500,7 @@ interface ISurfaceComposer { */ int getMaxAcquiredBufferCount(); - WindowInfosListenerInfo addWindowInfosListener(IWindowInfosListener windowInfosListener); + void addWindowInfosListener(IWindowInfosListener windowInfosListener); void removeWindowInfosListener(IWindowInfosListener windowInfosListener); diff --git a/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl deleted file mode 100644 index 0ca13b768a..0000000000 --- a/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl +++ /dev/null @@ -1,25 +0,0 @@ -/** - * Copyright (c) 2023, The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.gui; - -import android.gui.IWindowInfosPublisher; - -/** @hide */ -parcelable WindowInfosListenerInfo { - long listenerId; - IWindowInfosPublisher windowInfosPublisher; -} \ No newline at end of file diff --git a/libs/gui/android/gui/IWindowInfosListener.aidl b/libs/gui/android/gui/IWindowInfosListener.aidl index 07cb5ed0e6..400229d99f 100644 --- a/libs/gui/android/gui/IWindowInfosListener.aidl +++ b/libs/gui/android/gui/IWindowInfosListener.aidl @@ -16,9 +16,11 @@ package android.gui; +import android.gui.IWindowInfosReportedListener; import android.gui.WindowInfosUpdate; /** @hide */ oneway interface IWindowInfosListener { - void onWindowInfosChanged(in WindowInfosUpdate update); + void onWindowInfosChanged( + in WindowInfosUpdate update, in @nullable IWindowInfosReportedListener windowInfosReportedListener); } diff --git a/libs/gui/android/gui/IWindowInfosPublisher.aidl b/libs/gui/android/gui/IWindowInfosPublisher.aidl deleted file mode 100644 index 5a9c32845e..0000000000 --- a/libs/gui/android/gui/IWindowInfosPublisher.aidl +++ /dev/null @@ -1,23 +0,0 @@ -/** - * Copyright (c) 2023, The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.gui; - -/** @hide */ -oneway interface IWindowInfosPublisher -{ - void ackWindowInfosReceived(long vsyncId, long listenerId); -} diff --git a/libs/gui/fuzzer/libgui_fuzzer_utils.h b/libs/gui/fuzzer/libgui_fuzzer_utils.h index 4c7d0562af..8c003d8ad4 100644 --- a/libs/gui/fuzzer/libgui_fuzzer_utils.h +++ b/libs/gui/fuzzer/libgui_fuzzer_utils.h @@ -153,8 +153,8 @@ public: MOCK_METHOD(binder::Status, setOverrideFrameRate, (int32_t, float), (override)); MOCK_METHOD(binder::Status, getGpuContextPriority, (int32_t*), (override)); MOCK_METHOD(binder::Status, getMaxAcquiredBufferCount, (int32_t*), (override)); - MOCK_METHOD(binder::Status, addWindowInfosListener, - (const sp&, gui::WindowInfosListenerInfo*), (override)); + MOCK_METHOD(binder::Status, addWindowInfosListener, (const sp&), + (override)); MOCK_METHOD(binder::Status, removeWindowInfosListener, (const sp&), (override)); MOCK_METHOD(binder::Status, getOverlaySupport, (gui::OverlayProperties*), (override)); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 3ff6735926..7c150d53d9 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include diff --git a/libs/gui/include/gui/WindowInfosListenerReporter.h b/libs/gui/include/gui/WindowInfosListenerReporter.h index 684e21ad96..38cb108912 100644 --- a/libs/gui/include/gui/WindowInfosListenerReporter.h +++ b/libs/gui/include/gui/WindowInfosListenerReporter.h @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -30,7 +30,8 @@ namespace android { class WindowInfosListenerReporter : public gui::BnWindowInfosListener { public: static sp getInstance(); - binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override; + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update, + const sp&) override; status_t addWindowInfosListener( const sp& windowInfosListener, const sp&, @@ -46,8 +47,5 @@ private: std::vector mLastWindowInfos GUARDED_BY(mListenersMutex); std::vector mLastDisplayInfos GUARDED_BY(mListenersMutex); - - sp mWindowInfosPublisher; - int64_t mListenerId; }; } // namespace android diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 8d7cf07b96..096a43cd95 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -998,8 +998,7 @@ public: } binder::Status addWindowInfosListener( - const sp& /*windowInfosListener*/, - gui::WindowInfosListenerInfo* /*outInfo*/) override { + const sp& /*windowInfosListener*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/BackgroundExecutor.cpp b/services/surfaceflinger/BackgroundExecutor.cpp index 5a1ec6f501..6ddf790d47 100644 --- a/services/surfaceflinger/BackgroundExecutor.cpp +++ b/services/surfaceflinger/BackgroundExecutor.cpp @@ -20,7 +20,6 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS #include -#include #include "BackgroundExecutor.h" @@ -61,17 +60,4 @@ void BackgroundExecutor::sendCallbacks(Callbacks&& tasks) { LOG_ALWAYS_FATAL_IF(sem_post(&mSemaphore), "sem_post failed"); } -void BackgroundExecutor::flushQueue() { - std::mutex mutex; - std::condition_variable cv; - bool flushComplete = false; - sendCallbacks({[&]() { - std::scoped_lock lock{mutex}; - flushComplete = true; - cv.notify_one(); - }}); - std::unique_lock lock{mutex}; - cv.wait(lock, [&]() { return flushComplete; }); -} - } // namespace android diff --git a/services/surfaceflinger/BackgroundExecutor.h b/services/surfaceflinger/BackgroundExecutor.h index 66b7d7a1fc..0fae5a5c93 100644 --- a/services/surfaceflinger/BackgroundExecutor.h +++ b/services/surfaceflinger/BackgroundExecutor.h @@ -34,7 +34,6 @@ public: // Queues callbacks onto a work queue to be executed by a background thread. // This is safe to call from multiple threads. void sendCallbacks(Callbacks&& tasks); - void flushQueue(); private: sem_t mSemaphore; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index db205b8a95..fe2db940f7 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -6158,7 +6158,8 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp windowInfosDebug.maxSendDelayVsyncId.value); StringAppendF(&result, " max send delay (ns): %" PRId64 " ns\n", windowInfosDebug.maxSendDelayDuration); - StringAppendF(&result, " unsent messages: %zu\n", windowInfosDebug.pendingMessageCount); + StringAppendF(&result, " unsent messages: %" PRIu32 "\n", + windowInfosDebug.pendingMessageCount); result.append("\n"); } @@ -7991,9 +7992,9 @@ void SurfaceFlinger::onActiveDisplayChangedLocked(const DisplayDevice* inactiveD forceApplyPolicy); } -status_t SurfaceFlinger::addWindowInfosListener(const sp& windowInfosListener, - gui::WindowInfosListenerInfo* outInfo) { - mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener, outInfo); +status_t SurfaceFlinger::addWindowInfosListener( + const sp& windowInfosListener) { + mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener); setTransactionFlags(eInputInfoUpdateNeeded); return NO_ERROR; } @@ -9075,8 +9076,7 @@ binder::Status SurfaceComposerAIDL::getMaxAcquiredBufferCount(int32_t* buffers) } binder::Status SurfaceComposerAIDL::addWindowInfosListener( - const sp& windowInfosListener, - gui::WindowInfosListenerInfo* outInfo) { + const sp& windowInfosListener) { status_t status; const int pid = IPCThreadState::self()->getCallingPid(); const int uid = IPCThreadState::self()->getCallingUid(); @@ -9084,7 +9084,7 @@ binder::Status SurfaceComposerAIDL::addWindowInfosListener( // WindowInfosListeners if (uid == AID_SYSTEM || uid == AID_GRAPHICS || checkPermission(sAccessSurfaceFlinger, pid, uid)) { - status = mFlinger->addWindowInfosListener(windowInfosListener, outInfo); + status = mFlinger->addWindowInfosListener(windowInfosListener); } else { status = PERMISSION_DENIED; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index d4700a4e25..0bc506f1fe 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -612,8 +612,7 @@ private: status_t getMaxAcquiredBufferCount(int* buffers) const; - status_t addWindowInfosListener(const sp& windowInfosListener, - gui::WindowInfosListenerInfo* outResult); + status_t addWindowInfosListener(const sp& windowInfosListener); status_t removeWindowInfosListener( const sp& windowInfosListener) const; @@ -1557,8 +1556,8 @@ public: binder::Status setOverrideFrameRate(int32_t uid, float frameRate) override; binder::Status getGpuContextPriority(int32_t* outPriority) override; binder::Status getMaxAcquiredBufferCount(int32_t* buffers) override; - binder::Status addWindowInfosListener(const sp& windowInfosListener, - gui::WindowInfosListenerInfo* outInfo) override; + binder::Status addWindowInfosListener( + const sp& windowInfosListener) override; binder::Status removeWindowInfosListener( const sp& windowInfosListener) override; diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp index 7062a4e3a7..20699ef123 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp +++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp @@ -14,9 +14,7 @@ * limitations under the License. */ -#include -#include -#include +#include #include #include #include @@ -25,130 +23,162 @@ #include "BackgroundExecutor.h" #include "WindowInfosListenerInvoker.h" -#undef ATRACE_TAG -#define ATRACE_TAG ATRACE_TAG_GRAPHICS - namespace android { using gui::DisplayInfo; using gui::IWindowInfosListener; using gui::WindowInfo; -void WindowInfosListenerInvoker::addWindowInfosListener(sp listener, - gui::WindowInfosListenerInfo* outInfo) { - int64_t listenerId = mNextListenerId++; - outInfo->listenerId = listenerId; - outInfo->windowInfosPublisher = sp::fromExisting(this); +using WindowInfosListenerVector = ftl::SmallVector, 3>; + +struct WindowInfosReportedListenerInvoker : gui::BnWindowInfosReportedListener, + IBinder::DeathRecipient { + WindowInfosReportedListenerInvoker(WindowInfosListenerVector windowInfosListeners, + WindowInfosReportedListenerSet windowInfosReportedListeners) + : mCallbacksPending(windowInfosListeners.size()), + mWindowInfosListeners(std::move(windowInfosListeners)), + mWindowInfosReportedListeners(std::move(windowInfosReportedListeners)) {} - BackgroundExecutor::getInstance().sendCallbacks( - {[this, listener = std::move(listener), listenerId]() { - ATRACE_NAME("WindowInfosListenerInvoker::addWindowInfosListener"); + binder::Status onWindowInfosReported() override { + if (--mCallbacksPending == 0) { + for (const auto& listener : mWindowInfosReportedListeners) { sp asBinder = IInterface::asBinder(listener); - asBinder->linkToDeath(sp::fromExisting(this)); - mWindowInfosListeners.try_emplace(asBinder, - std::make_pair(listenerId, std::move(listener))); - }}); + if (asBinder->isBinderAlive()) { + listener->onWindowInfosReported(); + } + } + + auto wpThis = wp::fromExisting(this); + for (const auto& listener : mWindowInfosListeners) { + sp binder = IInterface::asBinder(listener); + binder->unlinkToDeath(wpThis); + } + } + return binder::Status::ok(); + } + + void binderDied(const wp&) { onWindowInfosReported(); } + +private: + std::atomic mCallbacksPending; + static constexpr size_t kStaticCapacity = 3; + const WindowInfosListenerVector mWindowInfosListeners; + WindowInfosReportedListenerSet mWindowInfosReportedListeners; +}; + +void WindowInfosListenerInvoker::addWindowInfosListener(sp listener) { + sp asBinder = IInterface::asBinder(listener); + asBinder->linkToDeath(sp::fromExisting(this)); + + std::scoped_lock lock(mListenersMutex); + mWindowInfosListeners.try_emplace(asBinder, std::move(listener)); } void WindowInfosListenerInvoker::removeWindowInfosListener( const sp& listener) { - BackgroundExecutor::getInstance().sendCallbacks({[this, listener]() { - ATRACE_NAME("WindowInfosListenerInvoker::removeWindowInfosListener"); - sp asBinder = IInterface::asBinder(listener); - asBinder->unlinkToDeath(sp::fromExisting(this)); - mWindowInfosListeners.erase(asBinder); - }}); + sp asBinder = IInterface::asBinder(listener); + + std::scoped_lock lock(mListenersMutex); + asBinder->unlinkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.erase(asBinder); } void WindowInfosListenerInvoker::binderDied(const wp& who) { - BackgroundExecutor::getInstance().sendCallbacks({[this, who]() { - ATRACE_NAME("WindowInfosListenerInvoker::binderDied"); - auto it = mWindowInfosListeners.find(who); - int64_t listenerId = it->second.first; - mWindowInfosListeners.erase(who); - - std::vector vsyncIds; - for (auto& [vsyncId, state] : mUnackedState) { - if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(), - listenerId) != state.unackedListenerIds.end()) { - vsyncIds.push_back(vsyncId); - } - } - - for (int64_t vsyncId : vsyncIds) { - ackWindowInfosReceived(vsyncId, listenerId); - } - }}); + std::scoped_lock lock(mListenersMutex); + mWindowInfosListeners.erase(who); } void WindowInfosListenerInvoker::windowInfosChanged( gui::WindowInfosUpdate update, WindowInfosReportedListenerSet reportedListeners, bool forceImmediateCall) { - if (!mDelayInfo) { - mDelayInfo = DelayInfo{ - .vsyncId = update.vsyncId, - .frameTime = update.timestamp, - }; - } + WindowInfosListenerVector listeners; + { + std::scoped_lock lock{mMessagesMutex}; + + if (!mDelayInfo) { + mDelayInfo = DelayInfo{ + .vsyncId = update.vsyncId, + .frameTime = update.timestamp, + }; + } - // If there are unacked messages and this isn't a forced call, then return immediately. - // If a forced window infos change doesn't happen first, the update will be sent after - // the WindowInfosReportedListeners are called. If a forced window infos change happens or - // if there are subsequent delayed messages before this update is sent, then this message - // will be dropped and the listeners will only be called with the latest info. This is done - // to reduce the amount of binder memory used. - if (!mUnackedState.empty() && !forceImmediateCall) { - mDelayedUpdate = std::move(update); - mReportedListeners.merge(reportedListeners); - return; - } + // If there are unacked messages and this isn't a forced call, then return immediately. + // If a forced window infos change doesn't happen first, the update will be sent after + // the WindowInfosReportedListeners are called. If a forced window infos change happens or + // if there are subsequent delayed messages before this update is sent, then this message + // will be dropped and the listeners will only be called with the latest info. This is done + // to reduce the amount of binder memory used. + if (mActiveMessageCount > 0 && !forceImmediateCall) { + mDelayedUpdate = std::move(update); + mReportedListeners.merge(reportedListeners); + return; + } - if (mDelayedUpdate) { - mDelayedUpdate.reset(); - } + if (mDelayedUpdate) { + mDelayedUpdate.reset(); + } + + { + std::scoped_lock lock{mListenersMutex}; + for (const auto& [_, listener] : mWindowInfosListeners) { + listeners.push_back(listener); + } + } + if (CC_UNLIKELY(listeners.empty())) { + mReportedListeners.merge(reportedListeners); + mDelayInfo.reset(); + return; + } + + reportedListeners.insert(sp::fromExisting(this)); + reportedListeners.merge(mReportedListeners); + mReportedListeners.clear(); - if (CC_UNLIKELY(mWindowInfosListeners.empty())) { - mReportedListeners.merge(reportedListeners); + mActiveMessageCount++; + updateMaxSendDelay(); mDelayInfo.reset(); - return; } - reportedListeners.merge(mReportedListeners); - mReportedListeners.clear(); - - // Update mUnackedState to include the message we're about to send - auto [it, _] = mUnackedState.try_emplace(update.vsyncId, - UnackedState{.reportedListeners = - std::move(reportedListeners)}); - auto& unackedState = it->second; - for (auto& pair : mWindowInfosListeners) { - int64_t listenerId = pair.second.first; - unackedState.unackedListenerIds.push_back(listenerId); - } + auto reportedInvoker = + sp::make(listeners, std::move(reportedListeners)); - mDelayInfo.reset(); - updateMaxSendDelay(); + for (const auto& listener : listeners) { + sp asBinder = IInterface::asBinder(listener); - // Call the listeners - for (auto& pair : mWindowInfosListeners) { - auto& [listenerId, listener] = pair.second; - auto status = listener->onWindowInfosChanged(update); + // linkToDeath is used here to ensure that the windowInfosReportedListeners + // are called even if one of the windowInfosListeners dies before + // calling onWindowInfosReported. + asBinder->linkToDeath(reportedInvoker); + + auto status = listener->onWindowInfosChanged(update, reportedInvoker); if (!status.isOk()) { - ackWindowInfosReceived(update.vsyncId, listenerId); + reportedInvoker->onWindowInfosReported(); } } } -WindowInfosListenerInvoker::DebugInfo WindowInfosListenerInvoker::getDebugInfo() { - DebugInfo result; - BackgroundExecutor::getInstance().sendCallbacks({[&, this]() { - ATRACE_NAME("WindowInfosListenerInvoker::getDebugInfo"); - updateMaxSendDelay(); - result = mDebugInfo; - result.pendingMessageCount = mUnackedState.size(); +binder::Status WindowInfosListenerInvoker::onWindowInfosReported() { + BackgroundExecutor::getInstance().sendCallbacks({[this]() { + gui::WindowInfosUpdate update; + { + std::scoped_lock lock{mMessagesMutex}; + mActiveMessageCount--; + if (!mDelayedUpdate || mActiveMessageCount > 0) { + return; + } + update = std::move(*mDelayedUpdate); + mDelayedUpdate.reset(); + } + windowInfosChanged(std::move(update), {}, false); }}); - BackgroundExecutor::getInstance().flushQueue(); - return result; + return binder::Status::ok(); +} + +WindowInfosListenerInvoker::DebugInfo WindowInfosListenerInvoker::getDebugInfo() { + std::scoped_lock lock{mMessagesMutex}; + updateMaxSendDelay(); + mDebugInfo.pendingMessageCount = mActiveMessageCount; + return mDebugInfo; } void WindowInfosListenerInvoker::updateMaxSendDelay() { @@ -162,41 +192,4 @@ void WindowInfosListenerInvoker::updateMaxSendDelay() { } } -binder::Status WindowInfosListenerInvoker::ackWindowInfosReceived(int64_t vsyncId, - int64_t listenerId) { - BackgroundExecutor::getInstance().sendCallbacks({[this, vsyncId, listenerId]() { - ATRACE_NAME("WindowInfosListenerInvoker::ackWindowInfosReceived"); - auto it = mUnackedState.find(vsyncId); - if (it == mUnackedState.end()) { - return; - } - - auto& state = it->second; - state.unackedListenerIds.unstable_erase(std::find(state.unackedListenerIds.begin(), - state.unackedListenerIds.end(), - listenerId)); - if (!state.unackedListenerIds.empty()) { - return; - } - - WindowInfosReportedListenerSet reportedListeners{std::move(state.reportedListeners)}; - mUnackedState.erase(vsyncId); - - for (const auto& reportedListener : reportedListeners) { - sp asBinder = IInterface::asBinder(reportedListener); - if (asBinder->isBinderAlive()) { - reportedListener->onWindowInfosReported(); - } - } - - if (!mDelayedUpdate || !mUnackedState.empty()) { - return; - } - gui::WindowInfosUpdate update{std::move(*mDelayedUpdate)}; - mDelayedUpdate.reset(); - windowInfosChanged(std::move(update), {}, false); - }}); - return binder::Status::ok(); -} - } // namespace android diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h index f36b0edd7d..bc465a3a2b 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.h +++ b/services/surfaceflinger/WindowInfosListenerInvoker.h @@ -19,12 +19,11 @@ #include #include -#include +#include #include #include #include #include -#include #include #include @@ -36,22 +35,22 @@ using WindowInfosReportedListenerSet = std::unordered_set, gui::SpHash>; -class WindowInfosListenerInvoker : public gui::BnWindowInfosPublisher, +class WindowInfosListenerInvoker : public gui::BnWindowInfosReportedListener, public IBinder::DeathRecipient { public: - void addWindowInfosListener(sp, gui::WindowInfosListenerInfo*); + void addWindowInfosListener(sp); void removeWindowInfosListener(const sp& windowInfosListener); void windowInfosChanged(gui::WindowInfosUpdate update, WindowInfosReportedListenerSet windowInfosReportedListeners, bool forceImmediateCall); - binder::Status ackWindowInfosReceived(int64_t, int64_t) override; + binder::Status onWindowInfosReported() override; struct DebugInfo { VsyncId maxSendDelayVsyncId; nsecs_t maxSendDelayDuration; - size_t pendingMessageCount; + uint32_t pendingMessageCount; }; DebugInfo getDebugInfo(); @@ -59,28 +58,24 @@ protected: void binderDied(const wp& who) override; private: + std::mutex mListenersMutex; + static constexpr size_t kStaticCapacity = 3; - std::atomic mNextListenerId{0}; - ftl::SmallMap, const std::pair>, - kStaticCapacity> - mWindowInfosListeners; + ftl::SmallMap, const sp, kStaticCapacity> + mWindowInfosListeners GUARDED_BY(mListenersMutex); - std::optional mDelayedUpdate; + std::mutex mMessagesMutex; + uint32_t mActiveMessageCount GUARDED_BY(mMessagesMutex) = 0; + std::optional mDelayedUpdate GUARDED_BY(mMessagesMutex); WindowInfosReportedListenerSet mReportedListeners; - struct UnackedState { - ftl::SmallVector unackedListenerIds; - WindowInfosReportedListenerSet reportedListeners; - }; - ftl::SmallMap mUnackedState; - - DebugInfo mDebugInfo; + DebugInfo mDebugInfo GUARDED_BY(mMessagesMutex); struct DelayInfo { int64_t vsyncId; nsecs_t frameTime; }; - std::optional mDelayInfo; - void updateMaxSendDelay(); + std::optional mDelayInfo GUARDED_BY(mMessagesMutex); + void updateMaxSendDelay() REQUIRES(mMessagesMutex); }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp index c7b845e668..af4971b063 100644 --- a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp +++ b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp @@ -15,23 +15,35 @@ protected: WindowInfosListenerInvokerTest() : mInvoker(sp::make()) {} ~WindowInfosListenerInvokerTest() { + std::mutex mutex; + std::condition_variable cv; + bool flushComplete = false; // Flush the BackgroundExecutor thread to ensure any scheduled tasks are complete. // Otherwise, references those tasks hold may go out of scope before they are done // executing. - BackgroundExecutor::getInstance().flushQueue(); + BackgroundExecutor::getInstance().sendCallbacks({[&]() { + std::scoped_lock lock{mutex}; + flushComplete = true; + cv.notify_one(); + }}); + std::unique_lock lock{mutex}; + cv.wait(lock, [&]() { return flushComplete; }); } sp mInvoker; }; -using WindowInfosUpdateConsumer = std::function; +using WindowInfosUpdateConsumer = std::function&)>; class Listener : public gui::BnWindowInfosListener { public: Listener(WindowInfosUpdateConsumer consumer) : mConsumer(std::move(consumer)) {} - binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override { - mConsumer(update); + binder::Status onWindowInfosChanged( + const gui::WindowInfosUpdate& update, + const sp& reportedListener) override { + mConsumer(update, reportedListener); return binder::Status::ok(); } @@ -46,17 +58,15 @@ TEST_F(WindowInfosListenerInvokerTest, callsSingleListener) { int callCount = 0; - gui::WindowInfosListenerInfo listenerInfo; - mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); + mInvoker->addWindowInfosListener( + sp::make([&](const gui::WindowInfosUpdate&, + const sp& reportedListener) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); - listenerInfo.windowInfosPublisher - ->ackWindowInfosReceived(update.vsyncId, - listenerInfo.listenerId); - }), - &listenerInfo); + reportedListener->onWindowInfosReported(); + })); BackgroundExecutor::getInstance().sendCallbacks( {[this]() { mInvoker->windowInfosChanged({}, {}, false); }}); @@ -71,27 +81,21 @@ TEST_F(WindowInfosListenerInvokerTest, callsMultipleListeners) { std::mutex mutex; std::condition_variable cv; - size_t callCount = 0; - const size_t expectedCallCount = 3; - std::vector listenerInfos{expectedCallCount, - gui::WindowInfosListenerInfo{}}; - - for (size_t i = 0; i < expectedCallCount; i++) { - mInvoker->addWindowInfosListener(sp::make([&, &listenerInfo = listenerInfos[i]]( - const gui::WindowInfosUpdate& - update) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - - listenerInfo.windowInfosPublisher - ->ackWindowInfosReceived(update.vsyncId, - listenerInfo - .listenerId); - }), - &listenerInfos[i]); + int callCount = 0; + const int expectedCallCount = 3; + + for (int i = 0; i < expectedCallCount; i++) { + mInvoker->addWindowInfosListener(sp::make( + [&](const gui::WindowInfosUpdate&, + const sp& reportedListener) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + + reportedListener->onWindowInfosReported(); + })); } BackgroundExecutor::getInstance().sendCallbacks( @@ -110,20 +114,17 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { int callCount = 0; - // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived - gui::WindowInfosListenerInfo listenerInfo; - mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - }), - &listenerInfo); + // Simulate a slow ack by not calling the WindowInfosReportedListener. + mInvoker->addWindowInfosListener(sp::make( + [&](const gui::WindowInfosUpdate&, const sp&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + })); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, - false); - mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, - false); + mInvoker->windowInfosChanged({}, {}, false); + mInvoker->windowInfosChanged({}, {}, false); }}); { @@ -133,7 +134,7 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { EXPECT_EQ(callCount, 1); // Ack the first message. - listenerInfo.windowInfosPublisher->ackWindowInfosReceived(0, listenerInfo.listenerId); + mInvoker->onWindowInfosReported(); { std::unique_lock lock{mutex}; @@ -151,21 +152,19 @@ TEST_F(WindowInfosListenerInvokerTest, sendsForcedMessage) { int callCount = 0; const int expectedCallCount = 2; - // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived - gui::WindowInfosListenerInfo listenerInfo; - mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - }), - &listenerInfo); + // Simulate a slow ack by not calling the WindowInfosReportedListener. + mInvoker->addWindowInfosListener(sp::make( + [&](const gui::WindowInfosUpdate&, const sp&) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + })); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, - false); - mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, true); + mInvoker->windowInfosChanged({}, {}, false); + mInvoker->windowInfosChanged({}, {}, true); }}); { @@ -183,14 +182,14 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { int64_t lastUpdateId = -1; - // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived - gui::WindowInfosListenerInfo listenerInfo; - mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { - std::scoped_lock lock{mutex}; - lastUpdateId = update.vsyncId; - cv.notify_one(); - }), - &listenerInfo); + // Simulate a slow ack by not calling the WindowInfosReportedListener. + mInvoker->addWindowInfosListener( + sp::make([&](const gui::WindowInfosUpdate& update, + const sp&) { + std::scoped_lock lock{mutex}; + lastUpdateId = update.vsyncId; + cv.notify_one(); + })); BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({{}, {}, /* vsyncId= */ 1, 0}, {}, false); @@ -205,7 +204,7 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { EXPECT_EQ(lastUpdateId, 1); // Ack the first message. The third update should be sent. - listenerInfo.windowInfosPublisher->ackWindowInfosReceived(1, listenerInfo.listenerId); + mInvoker->onWindowInfosReported(); { std::unique_lock lock{mutex}; @@ -226,17 +225,14 @@ TEST_F(WindowInfosListenerInvokerTest, noListeners) { // delayed. BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({}, {}, false); - gui::WindowInfosListenerInfo listenerInfo; - mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - }), - &listenerInfo); + mInvoker->addWindowInfosListener(sp::make( + [&](const gui::WindowInfosUpdate&, const sp&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + })); + mInvoker->windowInfosChanged({}, {}, false); }}); - BackgroundExecutor::getInstance().flushQueue(); - BackgroundExecutor::getInstance().sendCallbacks( - {[&]() { mInvoker->windowInfosChanged({}, {}, false); }}); { std::unique_lock lock{mutex}; -- cgit v1.2.3 From d80fdc48244e1769a84c46448477754a4cd3b883 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Thu, 31 Aug 2023 02:57:50 +0000 Subject: Improve updateInputFlinger performance This change improves the performance of the WindowInfosListenerInvoker work done on SurfaceFlinger's background executor thread. The primary optimization made is not sending a WindowInfosReportedListener with every call to WindowInfosListener.onWindowInfosChanged. Instead, we send a new interface, WindowInfosPublisher, and a unique listener id to listeners when they're added. Listeners call WindowInfosPublisher.ackWindowInfosReceived with their id after processing each update. From traces taken during development, the new code is a major improvement, taking about 15% of the time spent previously on SurfaceFlinger's background thread for sending window infos. Performance with this change seems roughly in line with the performance in T. Bug: 290377931 Test: atest WindowInfosListenerTest Test: atest WindowInfosListenerInvokerTest Test: manually killing system server and checking valid state on restart Change-Id: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 (cherry picked from commit acd2258a5492a9e289fd7f4b8ea90543d6843a23) (cherry picked from commit e8a7ab25b2f2f17571279a2c2bf2ea0dff66c8e6) Merged-In: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 This reverts commit a8ce8fc1d892ba7dcb27961b7bf49cc6433da7bb. Change-Id: I3a9a5b5d397d6d37e9b81070a344a0202ce0f927 --- libs/gui/Android.bp | 2 + libs/gui/WindowInfosListenerReporter.cpp | 20 +- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 4 +- .../aidl/android/gui/WindowInfosListenerInfo.aidl | 25 ++ libs/gui/android/gui/IWindowInfosListener.aidl | 4 +- libs/gui/android/gui/IWindowInfosPublisher.aidl | 23 ++ libs/gui/fuzzer/libgui_fuzzer_utils.h | 4 +- libs/gui/include/gui/ISurfaceComposer.h | 1 + libs/gui/include/gui/WindowInfosListenerReporter.h | 8 +- libs/gui/tests/Surface_test.cpp | 3 +- services/surfaceflinger/BackgroundExecutor.cpp | 14 ++ services/surfaceflinger/BackgroundExecutor.h | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 14 +- services/surfaceflinger/SurfaceFlinger.h | 7 +- .../surfaceflinger/WindowInfosListenerInvoker.cpp | 253 +++++++++++---------- .../surfaceflinger/WindowInfosListenerInvoker.h | 35 +-- .../unittests/WindowInfosListenerInvokerTest.cpp | 156 ++++++------- 17 files changed, 332 insertions(+), 242 deletions(-) create mode 100644 libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl create mode 100644 libs/gui/android/gui/IWindowInfosPublisher.aidl diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index bf34987b9e..3c8df2bb29 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -73,6 +73,7 @@ filegroup { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfo.aidl", "android/gui/WindowInfosUpdate.aidl", @@ -90,6 +91,7 @@ cc_library_static { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfosUpdate.aidl", "android/gui/WindowInfo.aidl", diff --git a/libs/gui/WindowInfosListenerReporter.cpp b/libs/gui/WindowInfosListenerReporter.cpp index 76e7b6e162..0929b8e120 100644 --- a/libs/gui/WindowInfosListenerReporter.cpp +++ b/libs/gui/WindowInfosListenerReporter.cpp @@ -22,7 +22,6 @@ namespace android { using gui::DisplayInfo; -using gui::IWindowInfosReportedListener; using gui::WindowInfo; using gui::WindowInfosListener; using gui::aidl_utils::statusTFromBinderStatus; @@ -40,8 +39,13 @@ status_t WindowInfosListenerReporter::addWindowInfosListener( { std::scoped_lock lock(mListenersMutex); if (mWindowInfosListeners.empty()) { - binder::Status s = surfaceComposer->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + binder::Status s = surfaceComposer->addWindowInfosListener(this, &listenerInfo); status = statusTFromBinderStatus(s); + if (status == OK) { + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; + } } if (status == OK) { @@ -85,8 +89,7 @@ status_t WindowInfosListenerReporter::removeWindowInfosListener( } binder::Status WindowInfosListenerReporter::onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& windowInfosReportedListener) { + const gui::WindowInfosUpdate& update) { std::unordered_set, gui::SpHash> windowInfosListeners; @@ -104,9 +107,7 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( listener->onWindowInfosChanged(update); } - if (windowInfosReportedListener) { - windowInfosReportedListener->onWindowInfosReported(); - } + mWindowInfosPublisher->ackWindowInfosReceived(update.vsyncId, mListenerId); return binder::Status::ok(); } @@ -114,7 +115,10 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( void WindowInfosListenerReporter::reconnect(const sp& composerService) { std::scoped_lock lock(mListenersMutex); if (!mWindowInfosListeners.empty()) { - composerService->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + composerService->addWindowInfosListener(this, &listenerInfo); + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; } } diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index ec3266ca83..539a1c140e 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -40,12 +40,14 @@ import android.gui.IScreenCaptureListener; import android.gui.ISurfaceComposerClient; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; +import android.gui.IWindowInfosPublisher; import android.gui.LayerCaptureArgs; import android.gui.LayerDebugInfo; import android.gui.OverlayProperties; import android.gui.PullAtomData; import android.gui.ARect; import android.gui.StaticDisplayInfo; +import android.gui.WindowInfosListenerInfo; /** @hide */ interface ISurfaceComposer { @@ -500,7 +502,7 @@ interface ISurfaceComposer { */ int getMaxAcquiredBufferCount(); - void addWindowInfosListener(IWindowInfosListener windowInfosListener); + WindowInfosListenerInfo addWindowInfosListener(IWindowInfosListener windowInfosListener); void removeWindowInfosListener(IWindowInfosListener windowInfosListener); diff --git a/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl new file mode 100644 index 0000000000..0ca13b768a --- /dev/null +++ b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl @@ -0,0 +1,25 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +import android.gui.IWindowInfosPublisher; + +/** @hide */ +parcelable WindowInfosListenerInfo { + long listenerId; + IWindowInfosPublisher windowInfosPublisher; +} \ No newline at end of file diff --git a/libs/gui/android/gui/IWindowInfosListener.aidl b/libs/gui/android/gui/IWindowInfosListener.aidl index 400229d99f..07cb5ed0e6 100644 --- a/libs/gui/android/gui/IWindowInfosListener.aidl +++ b/libs/gui/android/gui/IWindowInfosListener.aidl @@ -16,11 +16,9 @@ package android.gui; -import android.gui.IWindowInfosReportedListener; import android.gui.WindowInfosUpdate; /** @hide */ oneway interface IWindowInfosListener { - void onWindowInfosChanged( - in WindowInfosUpdate update, in @nullable IWindowInfosReportedListener windowInfosReportedListener); + void onWindowInfosChanged(in WindowInfosUpdate update); } diff --git a/libs/gui/android/gui/IWindowInfosPublisher.aidl b/libs/gui/android/gui/IWindowInfosPublisher.aidl new file mode 100644 index 0000000000..5a9c32845e --- /dev/null +++ b/libs/gui/android/gui/IWindowInfosPublisher.aidl @@ -0,0 +1,23 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +/** @hide */ +oneway interface IWindowInfosPublisher +{ + void ackWindowInfosReceived(long vsyncId, long listenerId); +} diff --git a/libs/gui/fuzzer/libgui_fuzzer_utils.h b/libs/gui/fuzzer/libgui_fuzzer_utils.h index 8c003d8ad4..4c7d0562af 100644 --- a/libs/gui/fuzzer/libgui_fuzzer_utils.h +++ b/libs/gui/fuzzer/libgui_fuzzer_utils.h @@ -153,8 +153,8 @@ public: MOCK_METHOD(binder::Status, setOverrideFrameRate, (int32_t, float), (override)); MOCK_METHOD(binder::Status, getGpuContextPriority, (int32_t*), (override)); MOCK_METHOD(binder::Status, getMaxAcquiredBufferCount, (int32_t*), (override)); - MOCK_METHOD(binder::Status, addWindowInfosListener, (const sp&), - (override)); + MOCK_METHOD(binder::Status, addWindowInfosListener, + (const sp&, gui::WindowInfosListenerInfo*), (override)); MOCK_METHOD(binder::Status, removeWindowInfosListener, (const sp&), (override)); MOCK_METHOD(binder::Status, getOverlaySupport, (gui::OverlayProperties*), (override)); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 7c150d53d9..3ff6735926 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include diff --git a/libs/gui/include/gui/WindowInfosListenerReporter.h b/libs/gui/include/gui/WindowInfosListenerReporter.h index 38cb108912..684e21ad96 100644 --- a/libs/gui/include/gui/WindowInfosListenerReporter.h +++ b/libs/gui/include/gui/WindowInfosListenerReporter.h @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -30,8 +30,7 @@ namespace android { class WindowInfosListenerReporter : public gui::BnWindowInfosListener { public: static sp getInstance(); - binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update, - const sp&) override; + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override; status_t addWindowInfosListener( const sp& windowInfosListener, const sp&, @@ -47,5 +46,8 @@ private: std::vector mLastWindowInfos GUARDED_BY(mListenersMutex); std::vector mLastDisplayInfos GUARDED_BY(mListenersMutex); + + sp mWindowInfosPublisher; + int64_t mListenerId; }; } // namespace android diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 096a43cd95..8d7cf07b96 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -998,7 +998,8 @@ public: } binder::Status addWindowInfosListener( - const sp& /*windowInfosListener*/) override { + const sp& /*windowInfosListener*/, + gui::WindowInfosListenerInfo* /*outInfo*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/BackgroundExecutor.cpp b/services/surfaceflinger/BackgroundExecutor.cpp index 6ddf790d47..5a1ec6f501 100644 --- a/services/surfaceflinger/BackgroundExecutor.cpp +++ b/services/surfaceflinger/BackgroundExecutor.cpp @@ -20,6 +20,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS #include +#include #include "BackgroundExecutor.h" @@ -60,4 +61,17 @@ void BackgroundExecutor::sendCallbacks(Callbacks&& tasks) { LOG_ALWAYS_FATAL_IF(sem_post(&mSemaphore), "sem_post failed"); } +void BackgroundExecutor::flushQueue() { + std::mutex mutex; + std::condition_variable cv; + bool flushComplete = false; + sendCallbacks({[&]() { + std::scoped_lock lock{mutex}; + flushComplete = true; + cv.notify_one(); + }}); + std::unique_lock lock{mutex}; + cv.wait(lock, [&]() { return flushComplete; }); +} + } // namespace android diff --git a/services/surfaceflinger/BackgroundExecutor.h b/services/surfaceflinger/BackgroundExecutor.h index 0fae5a5c93..66b7d7a1fc 100644 --- a/services/surfaceflinger/BackgroundExecutor.h +++ b/services/surfaceflinger/BackgroundExecutor.h @@ -34,6 +34,7 @@ public: // Queues callbacks onto a work queue to be executed by a background thread. // This is safe to call from multiple threads. void sendCallbacks(Callbacks&& tasks); + void flushQueue(); private: sem_t mSemaphore; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fe2db940f7..db205b8a95 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -6158,8 +6158,7 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp windowInfosDebug.maxSendDelayVsyncId.value); StringAppendF(&result, " max send delay (ns): %" PRId64 " ns\n", windowInfosDebug.maxSendDelayDuration); - StringAppendF(&result, " unsent messages: %" PRIu32 "\n", - windowInfosDebug.pendingMessageCount); + StringAppendF(&result, " unsent messages: %zu\n", windowInfosDebug.pendingMessageCount); result.append("\n"); } @@ -7992,9 +7991,9 @@ void SurfaceFlinger::onActiveDisplayChangedLocked(const DisplayDevice* inactiveD forceApplyPolicy); } -status_t SurfaceFlinger::addWindowInfosListener( - const sp& windowInfosListener) { - mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener); +status_t SurfaceFlinger::addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { + mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener, outInfo); setTransactionFlags(eInputInfoUpdateNeeded); return NO_ERROR; } @@ -9076,7 +9075,8 @@ binder::Status SurfaceComposerAIDL::getMaxAcquiredBufferCount(int32_t* buffers) } binder::Status SurfaceComposerAIDL::addWindowInfosListener( - const sp& windowInfosListener) { + const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { status_t status; const int pid = IPCThreadState::self()->getCallingPid(); const int uid = IPCThreadState::self()->getCallingUid(); @@ -9084,7 +9084,7 @@ binder::Status SurfaceComposerAIDL::addWindowInfosListener( // WindowInfosListeners if (uid == AID_SYSTEM || uid == AID_GRAPHICS || checkPermission(sAccessSurfaceFlinger, pid, uid)) { - status = mFlinger->addWindowInfosListener(windowInfosListener); + status = mFlinger->addWindowInfosListener(windowInfosListener, outInfo); } else { status = PERMISSION_DENIED; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 0bc506f1fe..d4700a4e25 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -612,7 +612,8 @@ private: status_t getMaxAcquiredBufferCount(int* buffers) const; - status_t addWindowInfosListener(const sp& windowInfosListener); + status_t addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outResult); status_t removeWindowInfosListener( const sp& windowInfosListener) const; @@ -1556,8 +1557,8 @@ public: binder::Status setOverrideFrameRate(int32_t uid, float frameRate) override; binder::Status getGpuContextPriority(int32_t* outPriority) override; binder::Status getMaxAcquiredBufferCount(int32_t* buffers) override; - binder::Status addWindowInfosListener( - const sp& windowInfosListener) override; + binder::Status addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) override; binder::Status removeWindowInfosListener( const sp& windowInfosListener) override; diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp index 20699ef123..7062a4e3a7 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp +++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp @@ -14,7 +14,9 @@ * limitations under the License. */ -#include +#include +#include +#include #include #include #include @@ -23,162 +25,130 @@ #include "BackgroundExecutor.h" #include "WindowInfosListenerInvoker.h" +#undef ATRACE_TAG +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + namespace android { using gui::DisplayInfo; using gui::IWindowInfosListener; using gui::WindowInfo; -using WindowInfosListenerVector = ftl::SmallVector, 3>; - -struct WindowInfosReportedListenerInvoker : gui::BnWindowInfosReportedListener, - IBinder::DeathRecipient { - WindowInfosReportedListenerInvoker(WindowInfosListenerVector windowInfosListeners, - WindowInfosReportedListenerSet windowInfosReportedListeners) - : mCallbacksPending(windowInfosListeners.size()), - mWindowInfosListeners(std::move(windowInfosListeners)), - mWindowInfosReportedListeners(std::move(windowInfosReportedListeners)) {} +void WindowInfosListenerInvoker::addWindowInfosListener(sp listener, + gui::WindowInfosListenerInfo* outInfo) { + int64_t listenerId = mNextListenerId++; + outInfo->listenerId = listenerId; + outInfo->windowInfosPublisher = sp::fromExisting(this); - binder::Status onWindowInfosReported() override { - if (--mCallbacksPending == 0) { - for (const auto& listener : mWindowInfosReportedListeners) { + BackgroundExecutor::getInstance().sendCallbacks( + {[this, listener = std::move(listener), listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::addWindowInfosListener"); sp asBinder = IInterface::asBinder(listener); - if (asBinder->isBinderAlive()) { - listener->onWindowInfosReported(); - } - } - - auto wpThis = wp::fromExisting(this); - for (const auto& listener : mWindowInfosListeners) { - sp binder = IInterface::asBinder(listener); - binder->unlinkToDeath(wpThis); - } - } - return binder::Status::ok(); - } - - void binderDied(const wp&) { onWindowInfosReported(); } - -private: - std::atomic mCallbacksPending; - static constexpr size_t kStaticCapacity = 3; - const WindowInfosListenerVector mWindowInfosListeners; - WindowInfosReportedListenerSet mWindowInfosReportedListeners; -}; - -void WindowInfosListenerInvoker::addWindowInfosListener(sp listener) { - sp asBinder = IInterface::asBinder(listener); - asBinder->linkToDeath(sp::fromExisting(this)); - - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.try_emplace(asBinder, std::move(listener)); + asBinder->linkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.try_emplace(asBinder, + std::make_pair(listenerId, std::move(listener))); + }}); } void WindowInfosListenerInvoker::removeWindowInfosListener( const sp& listener) { - sp asBinder = IInterface::asBinder(listener); - - std::scoped_lock lock(mListenersMutex); - asBinder->unlinkToDeath(sp::fromExisting(this)); - mWindowInfosListeners.erase(asBinder); + BackgroundExecutor::getInstance().sendCallbacks({[this, listener]() { + ATRACE_NAME("WindowInfosListenerInvoker::removeWindowInfosListener"); + sp asBinder = IInterface::asBinder(listener); + asBinder->unlinkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.erase(asBinder); + }}); } void WindowInfosListenerInvoker::binderDied(const wp& who) { - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.erase(who); + BackgroundExecutor::getInstance().sendCallbacks({[this, who]() { + ATRACE_NAME("WindowInfosListenerInvoker::binderDied"); + auto it = mWindowInfosListeners.find(who); + int64_t listenerId = it->second.first; + mWindowInfosListeners.erase(who); + + std::vector vsyncIds; + for (auto& [vsyncId, state] : mUnackedState) { + if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(), + listenerId) != state.unackedListenerIds.end()) { + vsyncIds.push_back(vsyncId); + } + } + + for (int64_t vsyncId : vsyncIds) { + ackWindowInfosReceived(vsyncId, listenerId); + } + }}); } void WindowInfosListenerInvoker::windowInfosChanged( gui::WindowInfosUpdate update, WindowInfosReportedListenerSet reportedListeners, bool forceImmediateCall) { - WindowInfosListenerVector listeners; - { - std::scoped_lock lock{mMessagesMutex}; - - if (!mDelayInfo) { - mDelayInfo = DelayInfo{ - .vsyncId = update.vsyncId, - .frameTime = update.timestamp, - }; - } - - // If there are unacked messages and this isn't a forced call, then return immediately. - // If a forced window infos change doesn't happen first, the update will be sent after - // the WindowInfosReportedListeners are called. If a forced window infos change happens or - // if there are subsequent delayed messages before this update is sent, then this message - // will be dropped and the listeners will only be called with the latest info. This is done - // to reduce the amount of binder memory used. - if (mActiveMessageCount > 0 && !forceImmediateCall) { - mDelayedUpdate = std::move(update); - mReportedListeners.merge(reportedListeners); - return; - } - - if (mDelayedUpdate) { - mDelayedUpdate.reset(); - } + if (!mDelayInfo) { + mDelayInfo = DelayInfo{ + .vsyncId = update.vsyncId, + .frameTime = update.timestamp, + }; + } - { - std::scoped_lock lock{mListenersMutex}; - for (const auto& [_, listener] : mWindowInfosListeners) { - listeners.push_back(listener); - } - } - if (CC_UNLIKELY(listeners.empty())) { - mReportedListeners.merge(reportedListeners); - mDelayInfo.reset(); - return; - } + // If there are unacked messages and this isn't a forced call, then return immediately. + // If a forced window infos change doesn't happen first, the update will be sent after + // the WindowInfosReportedListeners are called. If a forced window infos change happens or + // if there are subsequent delayed messages before this update is sent, then this message + // will be dropped and the listeners will only be called with the latest info. This is done + // to reduce the amount of binder memory used. + if (!mUnackedState.empty() && !forceImmediateCall) { + mDelayedUpdate = std::move(update); + mReportedListeners.merge(reportedListeners); + return; + } - reportedListeners.insert(sp::fromExisting(this)); - reportedListeners.merge(mReportedListeners); - mReportedListeners.clear(); + if (mDelayedUpdate) { + mDelayedUpdate.reset(); + } - mActiveMessageCount++; - updateMaxSendDelay(); + if (CC_UNLIKELY(mWindowInfosListeners.empty())) { + mReportedListeners.merge(reportedListeners); mDelayInfo.reset(); + return; } - auto reportedInvoker = - sp::make(listeners, std::move(reportedListeners)); - - for (const auto& listener : listeners) { - sp asBinder = IInterface::asBinder(listener); + reportedListeners.merge(mReportedListeners); + mReportedListeners.clear(); + + // Update mUnackedState to include the message we're about to send + auto [it, _] = mUnackedState.try_emplace(update.vsyncId, + UnackedState{.reportedListeners = + std::move(reportedListeners)}); + auto& unackedState = it->second; + for (auto& pair : mWindowInfosListeners) { + int64_t listenerId = pair.second.first; + unackedState.unackedListenerIds.push_back(listenerId); + } - // linkToDeath is used here to ensure that the windowInfosReportedListeners - // are called even if one of the windowInfosListeners dies before - // calling onWindowInfosReported. - asBinder->linkToDeath(reportedInvoker); + mDelayInfo.reset(); + updateMaxSendDelay(); - auto status = listener->onWindowInfosChanged(update, reportedInvoker); + // Call the listeners + for (auto& pair : mWindowInfosListeners) { + auto& [listenerId, listener] = pair.second; + auto status = listener->onWindowInfosChanged(update); if (!status.isOk()) { - reportedInvoker->onWindowInfosReported(); + ackWindowInfosReceived(update.vsyncId, listenerId); } } } -binder::Status WindowInfosListenerInvoker::onWindowInfosReported() { - BackgroundExecutor::getInstance().sendCallbacks({[this]() { - gui::WindowInfosUpdate update; - { - std::scoped_lock lock{mMessagesMutex}; - mActiveMessageCount--; - if (!mDelayedUpdate || mActiveMessageCount > 0) { - return; - } - update = std::move(*mDelayedUpdate); - mDelayedUpdate.reset(); - } - windowInfosChanged(std::move(update), {}, false); - }}); - return binder::Status::ok(); -} - WindowInfosListenerInvoker::DebugInfo WindowInfosListenerInvoker::getDebugInfo() { - std::scoped_lock lock{mMessagesMutex}; - updateMaxSendDelay(); - mDebugInfo.pendingMessageCount = mActiveMessageCount; - return mDebugInfo; + DebugInfo result; + BackgroundExecutor::getInstance().sendCallbacks({[&, this]() { + ATRACE_NAME("WindowInfosListenerInvoker::getDebugInfo"); + updateMaxSendDelay(); + result = mDebugInfo; + result.pendingMessageCount = mUnackedState.size(); + }}); + BackgroundExecutor::getInstance().flushQueue(); + return result; } void WindowInfosListenerInvoker::updateMaxSendDelay() { @@ -192,4 +162,41 @@ void WindowInfosListenerInvoker::updateMaxSendDelay() { } } +binder::Status WindowInfosListenerInvoker::ackWindowInfosReceived(int64_t vsyncId, + int64_t listenerId) { + BackgroundExecutor::getInstance().sendCallbacks({[this, vsyncId, listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::ackWindowInfosReceived"); + auto it = mUnackedState.find(vsyncId); + if (it == mUnackedState.end()) { + return; + } + + auto& state = it->second; + state.unackedListenerIds.unstable_erase(std::find(state.unackedListenerIds.begin(), + state.unackedListenerIds.end(), + listenerId)); + if (!state.unackedListenerIds.empty()) { + return; + } + + WindowInfosReportedListenerSet reportedListeners{std::move(state.reportedListeners)}; + mUnackedState.erase(vsyncId); + + for (const auto& reportedListener : reportedListeners) { + sp asBinder = IInterface::asBinder(reportedListener); + if (asBinder->isBinderAlive()) { + reportedListener->onWindowInfosReported(); + } + } + + if (!mDelayedUpdate || !mUnackedState.empty()) { + return; + } + gui::WindowInfosUpdate update{std::move(*mDelayedUpdate)}; + mDelayedUpdate.reset(); + windowInfosChanged(std::move(update), {}, false); + }}); + return binder::Status::ok(); +} + } // namespace android diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h index bc465a3a2b..f36b0edd7d 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.h +++ b/services/surfaceflinger/WindowInfosListenerInvoker.h @@ -19,11 +19,12 @@ #include #include -#include +#include #include #include #include #include +#include #include #include @@ -35,22 +36,22 @@ using WindowInfosReportedListenerSet = std::unordered_set, gui::SpHash>; -class WindowInfosListenerInvoker : public gui::BnWindowInfosReportedListener, +class WindowInfosListenerInvoker : public gui::BnWindowInfosPublisher, public IBinder::DeathRecipient { public: - void addWindowInfosListener(sp); + void addWindowInfosListener(sp, gui::WindowInfosListenerInfo*); void removeWindowInfosListener(const sp& windowInfosListener); void windowInfosChanged(gui::WindowInfosUpdate update, WindowInfosReportedListenerSet windowInfosReportedListeners, bool forceImmediateCall); - binder::Status onWindowInfosReported() override; + binder::Status ackWindowInfosReceived(int64_t, int64_t) override; struct DebugInfo { VsyncId maxSendDelayVsyncId; nsecs_t maxSendDelayDuration; - uint32_t pendingMessageCount; + size_t pendingMessageCount; }; DebugInfo getDebugInfo(); @@ -58,24 +59,28 @@ protected: void binderDied(const wp& who) override; private: - std::mutex mListenersMutex; - static constexpr size_t kStaticCapacity = 3; - ftl::SmallMap, const sp, kStaticCapacity> - mWindowInfosListeners GUARDED_BY(mListenersMutex); + std::atomic mNextListenerId{0}; + ftl::SmallMap, const std::pair>, + kStaticCapacity> + mWindowInfosListeners; - std::mutex mMessagesMutex; - uint32_t mActiveMessageCount GUARDED_BY(mMessagesMutex) = 0; - std::optional mDelayedUpdate GUARDED_BY(mMessagesMutex); + std::optional mDelayedUpdate; WindowInfosReportedListenerSet mReportedListeners; - DebugInfo mDebugInfo GUARDED_BY(mMessagesMutex); + struct UnackedState { + ftl::SmallVector unackedListenerIds; + WindowInfosReportedListenerSet reportedListeners; + }; + ftl::SmallMap mUnackedState; + + DebugInfo mDebugInfo; struct DelayInfo { int64_t vsyncId; nsecs_t frameTime; }; - std::optional mDelayInfo GUARDED_BY(mMessagesMutex); - void updateMaxSendDelay() REQUIRES(mMessagesMutex); + std::optional mDelayInfo; + void updateMaxSendDelay(); }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp index af4971b063..c7b845e668 100644 --- a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp +++ b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp @@ -15,35 +15,23 @@ protected: WindowInfosListenerInvokerTest() : mInvoker(sp::make()) {} ~WindowInfosListenerInvokerTest() { - std::mutex mutex; - std::condition_variable cv; - bool flushComplete = false; // Flush the BackgroundExecutor thread to ensure any scheduled tasks are complete. // Otherwise, references those tasks hold may go out of scope before they are done // executing. - BackgroundExecutor::getInstance().sendCallbacks({[&]() { - std::scoped_lock lock{mutex}; - flushComplete = true; - cv.notify_one(); - }}); - std::unique_lock lock{mutex}; - cv.wait(lock, [&]() { return flushComplete; }); + BackgroundExecutor::getInstance().flushQueue(); } sp mInvoker; }; -using WindowInfosUpdateConsumer = std::function&)>; +using WindowInfosUpdateConsumer = std::function; class Listener : public gui::BnWindowInfosListener { public: Listener(WindowInfosUpdateConsumer consumer) : mConsumer(std::move(consumer)) {} - binder::Status onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& reportedListener) override { - mConsumer(update, reportedListener); + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override { + mConsumer(update); return binder::Status::ok(); } @@ -58,15 +46,17 @@ TEST_F(WindowInfosListenerInvokerTest, callsSingleListener) { int callCount = 0; - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); - reportedListener->onWindowInfosReported(); - })); + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo.listenerId); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks( {[this]() { mInvoker->windowInfosChanged({}, {}, false); }}); @@ -81,21 +71,27 @@ TEST_F(WindowInfosListenerInvokerTest, callsMultipleListeners) { std::mutex mutex; std::condition_variable cv; - int callCount = 0; - const int expectedCallCount = 3; - - for (int i = 0; i < expectedCallCount; i++) { - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - - reportedListener->onWindowInfosReported(); - })); + size_t callCount = 0; + const size_t expectedCallCount = 3; + std::vector listenerInfos{expectedCallCount, + gui::WindowInfosListenerInfo{}}; + + for (size_t i = 0; i < expectedCallCount; i++) { + mInvoker->addWindowInfosListener(sp::make([&, &listenerInfo = listenerInfos[i]]( + const gui::WindowInfosUpdate& + update) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo + .listenerId); + }), + &listenerInfos[i]); } BackgroundExecutor::getInstance().sendCallbacks( @@ -114,17 +110,20 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { int callCount = 0; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, + false); }}); { @@ -134,7 +133,7 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { EXPECT_EQ(callCount, 1); // Ack the first message. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(0, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -152,19 +151,21 @@ TEST_F(WindowInfosListenerInvokerTest, sendsForcedMessage) { int callCount = 0; const int expectedCallCount = 2; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, true); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, true); }}); { @@ -182,14 +183,14 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { int64_t lastUpdateId = -1; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate& update, - const sp&) { - std::scoped_lock lock{mutex}; - lastUpdateId = update.vsyncId; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + lastUpdateId = update.vsyncId; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({{}, {}, /* vsyncId= */ 1, 0}, {}, false); @@ -204,7 +205,7 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { EXPECT_EQ(lastUpdateId, 1); // Ack the first message. The third update should be sent. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(1, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -225,14 +226,17 @@ TEST_F(WindowInfosListenerInvokerTest, noListeners) { // delayed. BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({}, {}, false); - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); - mInvoker->windowInfosChanged({}, {}, false); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); }}); + BackgroundExecutor::getInstance().flushQueue(); + BackgroundExecutor::getInstance().sendCallbacks( + {[&]() { mInvoker->windowInfosChanged({}, {}, false); }}); { std::unique_lock lock{mutex}; -- cgit v1.2.3 From e11be022b16382382edfe6e24f85b831ed6a0cdf Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Wed, 12 Jul 2023 13:47:28 -0500 Subject: Improve updateInputFlinger performance This change improves the performance of the WindowInfosListenerInvoker work done on SurfaceFlinger's background executor thread. The primary optimization made is not sending a WindowInfosReportedListener with every call to WindowInfosListener.onWindowInfosChanged. Instead, we send a new interface, WindowInfosPublisher, and a unique listener id to listeners when they're added. Listeners call WindowInfosPublisher.ackWindowInfosReceived with their id after processing each update. From traces taken during development, the new code is a major improvement, taking about 15% of the time spent previously on SurfaceFlinger's background thread for sending window infos. Performance with this change seems roughly in line with the performance in T. Bug: 290377931 Test: atest WindowInfosListenerTest Test: atest WindowInfosListenerInvokerTest Test: manually killing system server and checking valid state on restart Change-Id: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 (cherry picked from commit acd2258a5492a9e289fd7f4b8ea90543d6843a23) Merged-In: Ib39ba935727df0bc1ab4030bcfe8301de7e64805 (cherry picked from commit 16cb723d942232e6b421a5001eb134736e688f7d) --- libs/gui/Android.bp | 3 + libs/gui/WindowInfosListenerReporter.cpp | 20 +- libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 4 +- .../aidl/android/gui/WindowInfosListenerInfo.aidl | 25 ++ libs/gui/android/gui/IWindowInfosListener.aidl | 4 +- libs/gui/android/gui/IWindowInfosPublisher.aidl | 23 ++ libs/gui/fuzzer/libgui_fuzzer_utils.h | 4 +- libs/gui/include/gui/ISurfaceComposer.h | 1 + libs/gui/include/gui/WindowInfosListenerReporter.h | 8 +- libs/gui/tests/Surface_test.cpp | 3 +- services/surfaceflinger/BackgroundExecutor.cpp | 14 ++ services/surfaceflinger/BackgroundExecutor.h | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 14 +- services/surfaceflinger/SurfaceFlinger.h | 7 +- .../surfaceflinger/WindowInfosListenerInvoker.cpp | 253 +++++++++++---------- .../surfaceflinger/WindowInfosListenerInvoker.h | 35 +-- .../unittests/WindowInfosListenerInvokerTest.cpp | 156 ++++++------- 17 files changed, 333 insertions(+), 242 deletions(-) create mode 100644 libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl create mode 100644 libs/gui/android/gui/IWindowInfosPublisher.aidl diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 72c6b1570a..a45e8c5c56 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -73,6 +73,7 @@ filegroup { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfo.aidl", "android/gui/WindowInfosUpdate.aidl", @@ -90,6 +91,7 @@ cc_library_static { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfosUpdate.aidl", "android/gui/WindowInfo.aidl", @@ -136,6 +138,7 @@ aidl_library { "android/gui/FocusRequest.aidl", "android/gui/InputApplicationInfo.aidl", "android/gui/IWindowInfosListener.aidl", + "android/gui/IWindowInfosPublisher.aidl", "android/gui/IWindowInfosReportedListener.aidl", "android/gui/WindowInfo.aidl", "android/gui/WindowInfosUpdate.aidl", diff --git a/libs/gui/WindowInfosListenerReporter.cpp b/libs/gui/WindowInfosListenerReporter.cpp index 76e7b6e162..0929b8e120 100644 --- a/libs/gui/WindowInfosListenerReporter.cpp +++ b/libs/gui/WindowInfosListenerReporter.cpp @@ -22,7 +22,6 @@ namespace android { using gui::DisplayInfo; -using gui::IWindowInfosReportedListener; using gui::WindowInfo; using gui::WindowInfosListener; using gui::aidl_utils::statusTFromBinderStatus; @@ -40,8 +39,13 @@ status_t WindowInfosListenerReporter::addWindowInfosListener( { std::scoped_lock lock(mListenersMutex); if (mWindowInfosListeners.empty()) { - binder::Status s = surfaceComposer->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + binder::Status s = surfaceComposer->addWindowInfosListener(this, &listenerInfo); status = statusTFromBinderStatus(s); + if (status == OK) { + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; + } } if (status == OK) { @@ -85,8 +89,7 @@ status_t WindowInfosListenerReporter::removeWindowInfosListener( } binder::Status WindowInfosListenerReporter::onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& windowInfosReportedListener) { + const gui::WindowInfosUpdate& update) { std::unordered_set, gui::SpHash> windowInfosListeners; @@ -104,9 +107,7 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( listener->onWindowInfosChanged(update); } - if (windowInfosReportedListener) { - windowInfosReportedListener->onWindowInfosReported(); - } + mWindowInfosPublisher->ackWindowInfosReceived(update.vsyncId, mListenerId); return binder::Status::ok(); } @@ -114,7 +115,10 @@ binder::Status WindowInfosListenerReporter::onWindowInfosChanged( void WindowInfosListenerReporter::reconnect(const sp& composerService) { std::scoped_lock lock(mListenersMutex); if (!mWindowInfosListeners.empty()) { - composerService->addWindowInfosListener(this); + gui::WindowInfosListenerInfo listenerInfo; + composerService->addWindowInfosListener(this, &listenerInfo); + mWindowInfosPublisher = std::move(listenerInfo.windowInfosPublisher); + mListenerId = listenerInfo.listenerId; } } diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index ec3266ca83..539a1c140e 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -40,12 +40,14 @@ import android.gui.IScreenCaptureListener; import android.gui.ISurfaceComposerClient; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; +import android.gui.IWindowInfosPublisher; import android.gui.LayerCaptureArgs; import android.gui.LayerDebugInfo; import android.gui.OverlayProperties; import android.gui.PullAtomData; import android.gui.ARect; import android.gui.StaticDisplayInfo; +import android.gui.WindowInfosListenerInfo; /** @hide */ interface ISurfaceComposer { @@ -500,7 +502,7 @@ interface ISurfaceComposer { */ int getMaxAcquiredBufferCount(); - void addWindowInfosListener(IWindowInfosListener windowInfosListener); + WindowInfosListenerInfo addWindowInfosListener(IWindowInfosListener windowInfosListener); void removeWindowInfosListener(IWindowInfosListener windowInfosListener); diff --git a/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl new file mode 100644 index 0000000000..0ca13b768a --- /dev/null +++ b/libs/gui/aidl/android/gui/WindowInfosListenerInfo.aidl @@ -0,0 +1,25 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +import android.gui.IWindowInfosPublisher; + +/** @hide */ +parcelable WindowInfosListenerInfo { + long listenerId; + IWindowInfosPublisher windowInfosPublisher; +} \ No newline at end of file diff --git a/libs/gui/android/gui/IWindowInfosListener.aidl b/libs/gui/android/gui/IWindowInfosListener.aidl index 400229d99f..07cb5ed0e6 100644 --- a/libs/gui/android/gui/IWindowInfosListener.aidl +++ b/libs/gui/android/gui/IWindowInfosListener.aidl @@ -16,11 +16,9 @@ package android.gui; -import android.gui.IWindowInfosReportedListener; import android.gui.WindowInfosUpdate; /** @hide */ oneway interface IWindowInfosListener { - void onWindowInfosChanged( - in WindowInfosUpdate update, in @nullable IWindowInfosReportedListener windowInfosReportedListener); + void onWindowInfosChanged(in WindowInfosUpdate update); } diff --git a/libs/gui/android/gui/IWindowInfosPublisher.aidl b/libs/gui/android/gui/IWindowInfosPublisher.aidl new file mode 100644 index 0000000000..5a9c32845e --- /dev/null +++ b/libs/gui/android/gui/IWindowInfosPublisher.aidl @@ -0,0 +1,23 @@ +/** + * Copyright (c) 2023, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.gui; + +/** @hide */ +oneway interface IWindowInfosPublisher +{ + void ackWindowInfosReceived(long vsyncId, long listenerId); +} diff --git a/libs/gui/fuzzer/libgui_fuzzer_utils.h b/libs/gui/fuzzer/libgui_fuzzer_utils.h index 8c003d8ad4..4c7d0562af 100644 --- a/libs/gui/fuzzer/libgui_fuzzer_utils.h +++ b/libs/gui/fuzzer/libgui_fuzzer_utils.h @@ -153,8 +153,8 @@ public: MOCK_METHOD(binder::Status, setOverrideFrameRate, (int32_t, float), (override)); MOCK_METHOD(binder::Status, getGpuContextPriority, (int32_t*), (override)); MOCK_METHOD(binder::Status, getMaxAcquiredBufferCount, (int32_t*), (override)); - MOCK_METHOD(binder::Status, addWindowInfosListener, (const sp&), - (override)); + MOCK_METHOD(binder::Status, addWindowInfosListener, + (const sp&, gui::WindowInfosListenerInfo*), (override)); MOCK_METHOD(binder::Status, removeWindowInfosListener, (const sp&), (override)); MOCK_METHOD(binder::Status, getOverlaySupport, (gui::OverlayProperties*), (override)); diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 7c150d53d9..3ff6735926 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include diff --git a/libs/gui/include/gui/WindowInfosListenerReporter.h b/libs/gui/include/gui/WindowInfosListenerReporter.h index 38cb108912..684e21ad96 100644 --- a/libs/gui/include/gui/WindowInfosListenerReporter.h +++ b/libs/gui/include/gui/WindowInfosListenerReporter.h @@ -18,7 +18,7 @@ #include #include -#include +#include #include #include #include @@ -30,8 +30,7 @@ namespace android { class WindowInfosListenerReporter : public gui::BnWindowInfosListener { public: static sp getInstance(); - binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update, - const sp&) override; + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override; status_t addWindowInfosListener( const sp& windowInfosListener, const sp&, @@ -47,5 +46,8 @@ private: std::vector mLastWindowInfos GUARDED_BY(mListenersMutex); std::vector mLastDisplayInfos GUARDED_BY(mListenersMutex); + + sp mWindowInfosPublisher; + int64_t mListenerId; }; } // namespace android diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 096a43cd95..8d7cf07b96 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -998,7 +998,8 @@ public: } binder::Status addWindowInfosListener( - const sp& /*windowInfosListener*/) override { + const sp& /*windowInfosListener*/, + gui::WindowInfosListenerInfo* /*outInfo*/) override { return binder::Status::ok(); } diff --git a/services/surfaceflinger/BackgroundExecutor.cpp b/services/surfaceflinger/BackgroundExecutor.cpp index 6ddf790d47..5a1ec6f501 100644 --- a/services/surfaceflinger/BackgroundExecutor.cpp +++ b/services/surfaceflinger/BackgroundExecutor.cpp @@ -20,6 +20,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS #include +#include #include "BackgroundExecutor.h" @@ -60,4 +61,17 @@ void BackgroundExecutor::sendCallbacks(Callbacks&& tasks) { LOG_ALWAYS_FATAL_IF(sem_post(&mSemaphore), "sem_post failed"); } +void BackgroundExecutor::flushQueue() { + std::mutex mutex; + std::condition_variable cv; + bool flushComplete = false; + sendCallbacks({[&]() { + std::scoped_lock lock{mutex}; + flushComplete = true; + cv.notify_one(); + }}); + std::unique_lock lock{mutex}; + cv.wait(lock, [&]() { return flushComplete; }); +} + } // namespace android diff --git a/services/surfaceflinger/BackgroundExecutor.h b/services/surfaceflinger/BackgroundExecutor.h index 0fae5a5c93..66b7d7a1fc 100644 --- a/services/surfaceflinger/BackgroundExecutor.h +++ b/services/surfaceflinger/BackgroundExecutor.h @@ -34,6 +34,7 @@ public: // Queues callbacks onto a work queue to be executed by a background thread. // This is safe to call from multiple threads. void sendCallbacks(Callbacks&& tasks); + void flushQueue(); private: sem_t mSemaphore; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fe2db940f7..db205b8a95 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -6158,8 +6158,7 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, const std::string& comp windowInfosDebug.maxSendDelayVsyncId.value); StringAppendF(&result, " max send delay (ns): %" PRId64 " ns\n", windowInfosDebug.maxSendDelayDuration); - StringAppendF(&result, " unsent messages: %" PRIu32 "\n", - windowInfosDebug.pendingMessageCount); + StringAppendF(&result, " unsent messages: %zu\n", windowInfosDebug.pendingMessageCount); result.append("\n"); } @@ -7992,9 +7991,9 @@ void SurfaceFlinger::onActiveDisplayChangedLocked(const DisplayDevice* inactiveD forceApplyPolicy); } -status_t SurfaceFlinger::addWindowInfosListener( - const sp& windowInfosListener) { - mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener); +status_t SurfaceFlinger::addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { + mWindowInfosListenerInvoker->addWindowInfosListener(windowInfosListener, outInfo); setTransactionFlags(eInputInfoUpdateNeeded); return NO_ERROR; } @@ -9076,7 +9075,8 @@ binder::Status SurfaceComposerAIDL::getMaxAcquiredBufferCount(int32_t* buffers) } binder::Status SurfaceComposerAIDL::addWindowInfosListener( - const sp& windowInfosListener) { + const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) { status_t status; const int pid = IPCThreadState::self()->getCallingPid(); const int uid = IPCThreadState::self()->getCallingUid(); @@ -9084,7 +9084,7 @@ binder::Status SurfaceComposerAIDL::addWindowInfosListener( // WindowInfosListeners if (uid == AID_SYSTEM || uid == AID_GRAPHICS || checkPermission(sAccessSurfaceFlinger, pid, uid)) { - status = mFlinger->addWindowInfosListener(windowInfosListener); + status = mFlinger->addWindowInfosListener(windowInfosListener, outInfo); } else { status = PERMISSION_DENIED; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 0bc506f1fe..d4700a4e25 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -612,7 +612,8 @@ private: status_t getMaxAcquiredBufferCount(int* buffers) const; - status_t addWindowInfosListener(const sp& windowInfosListener); + status_t addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outResult); status_t removeWindowInfosListener( const sp& windowInfosListener) const; @@ -1556,8 +1557,8 @@ public: binder::Status setOverrideFrameRate(int32_t uid, float frameRate) override; binder::Status getGpuContextPriority(int32_t* outPriority) override; binder::Status getMaxAcquiredBufferCount(int32_t* buffers) override; - binder::Status addWindowInfosListener( - const sp& windowInfosListener) override; + binder::Status addWindowInfosListener(const sp& windowInfosListener, + gui::WindowInfosListenerInfo* outInfo) override; binder::Status removeWindowInfosListener( const sp& windowInfosListener) override; diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp index 20699ef123..7062a4e3a7 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp +++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp @@ -14,7 +14,9 @@ * limitations under the License. */ -#include +#include +#include +#include #include #include #include @@ -23,162 +25,130 @@ #include "BackgroundExecutor.h" #include "WindowInfosListenerInvoker.h" +#undef ATRACE_TAG +#define ATRACE_TAG ATRACE_TAG_GRAPHICS + namespace android { using gui::DisplayInfo; using gui::IWindowInfosListener; using gui::WindowInfo; -using WindowInfosListenerVector = ftl::SmallVector, 3>; - -struct WindowInfosReportedListenerInvoker : gui::BnWindowInfosReportedListener, - IBinder::DeathRecipient { - WindowInfosReportedListenerInvoker(WindowInfosListenerVector windowInfosListeners, - WindowInfosReportedListenerSet windowInfosReportedListeners) - : mCallbacksPending(windowInfosListeners.size()), - mWindowInfosListeners(std::move(windowInfosListeners)), - mWindowInfosReportedListeners(std::move(windowInfosReportedListeners)) {} +void WindowInfosListenerInvoker::addWindowInfosListener(sp listener, + gui::WindowInfosListenerInfo* outInfo) { + int64_t listenerId = mNextListenerId++; + outInfo->listenerId = listenerId; + outInfo->windowInfosPublisher = sp::fromExisting(this); - binder::Status onWindowInfosReported() override { - if (--mCallbacksPending == 0) { - for (const auto& listener : mWindowInfosReportedListeners) { + BackgroundExecutor::getInstance().sendCallbacks( + {[this, listener = std::move(listener), listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::addWindowInfosListener"); sp asBinder = IInterface::asBinder(listener); - if (asBinder->isBinderAlive()) { - listener->onWindowInfosReported(); - } - } - - auto wpThis = wp::fromExisting(this); - for (const auto& listener : mWindowInfosListeners) { - sp binder = IInterface::asBinder(listener); - binder->unlinkToDeath(wpThis); - } - } - return binder::Status::ok(); - } - - void binderDied(const wp&) { onWindowInfosReported(); } - -private: - std::atomic mCallbacksPending; - static constexpr size_t kStaticCapacity = 3; - const WindowInfosListenerVector mWindowInfosListeners; - WindowInfosReportedListenerSet mWindowInfosReportedListeners; -}; - -void WindowInfosListenerInvoker::addWindowInfosListener(sp listener) { - sp asBinder = IInterface::asBinder(listener); - asBinder->linkToDeath(sp::fromExisting(this)); - - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.try_emplace(asBinder, std::move(listener)); + asBinder->linkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.try_emplace(asBinder, + std::make_pair(listenerId, std::move(listener))); + }}); } void WindowInfosListenerInvoker::removeWindowInfosListener( const sp& listener) { - sp asBinder = IInterface::asBinder(listener); - - std::scoped_lock lock(mListenersMutex); - asBinder->unlinkToDeath(sp::fromExisting(this)); - mWindowInfosListeners.erase(asBinder); + BackgroundExecutor::getInstance().sendCallbacks({[this, listener]() { + ATRACE_NAME("WindowInfosListenerInvoker::removeWindowInfosListener"); + sp asBinder = IInterface::asBinder(listener); + asBinder->unlinkToDeath(sp::fromExisting(this)); + mWindowInfosListeners.erase(asBinder); + }}); } void WindowInfosListenerInvoker::binderDied(const wp& who) { - std::scoped_lock lock(mListenersMutex); - mWindowInfosListeners.erase(who); + BackgroundExecutor::getInstance().sendCallbacks({[this, who]() { + ATRACE_NAME("WindowInfosListenerInvoker::binderDied"); + auto it = mWindowInfosListeners.find(who); + int64_t listenerId = it->second.first; + mWindowInfosListeners.erase(who); + + std::vector vsyncIds; + for (auto& [vsyncId, state] : mUnackedState) { + if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(), + listenerId) != state.unackedListenerIds.end()) { + vsyncIds.push_back(vsyncId); + } + } + + for (int64_t vsyncId : vsyncIds) { + ackWindowInfosReceived(vsyncId, listenerId); + } + }}); } void WindowInfosListenerInvoker::windowInfosChanged( gui::WindowInfosUpdate update, WindowInfosReportedListenerSet reportedListeners, bool forceImmediateCall) { - WindowInfosListenerVector listeners; - { - std::scoped_lock lock{mMessagesMutex}; - - if (!mDelayInfo) { - mDelayInfo = DelayInfo{ - .vsyncId = update.vsyncId, - .frameTime = update.timestamp, - }; - } - - // If there are unacked messages and this isn't a forced call, then return immediately. - // If a forced window infos change doesn't happen first, the update will be sent after - // the WindowInfosReportedListeners are called. If a forced window infos change happens or - // if there are subsequent delayed messages before this update is sent, then this message - // will be dropped and the listeners will only be called with the latest info. This is done - // to reduce the amount of binder memory used. - if (mActiveMessageCount > 0 && !forceImmediateCall) { - mDelayedUpdate = std::move(update); - mReportedListeners.merge(reportedListeners); - return; - } - - if (mDelayedUpdate) { - mDelayedUpdate.reset(); - } + if (!mDelayInfo) { + mDelayInfo = DelayInfo{ + .vsyncId = update.vsyncId, + .frameTime = update.timestamp, + }; + } - { - std::scoped_lock lock{mListenersMutex}; - for (const auto& [_, listener] : mWindowInfosListeners) { - listeners.push_back(listener); - } - } - if (CC_UNLIKELY(listeners.empty())) { - mReportedListeners.merge(reportedListeners); - mDelayInfo.reset(); - return; - } + // If there are unacked messages and this isn't a forced call, then return immediately. + // If a forced window infos change doesn't happen first, the update will be sent after + // the WindowInfosReportedListeners are called. If a forced window infos change happens or + // if there are subsequent delayed messages before this update is sent, then this message + // will be dropped and the listeners will only be called with the latest info. This is done + // to reduce the amount of binder memory used. + if (!mUnackedState.empty() && !forceImmediateCall) { + mDelayedUpdate = std::move(update); + mReportedListeners.merge(reportedListeners); + return; + } - reportedListeners.insert(sp::fromExisting(this)); - reportedListeners.merge(mReportedListeners); - mReportedListeners.clear(); + if (mDelayedUpdate) { + mDelayedUpdate.reset(); + } - mActiveMessageCount++; - updateMaxSendDelay(); + if (CC_UNLIKELY(mWindowInfosListeners.empty())) { + mReportedListeners.merge(reportedListeners); mDelayInfo.reset(); + return; } - auto reportedInvoker = - sp::make(listeners, std::move(reportedListeners)); - - for (const auto& listener : listeners) { - sp asBinder = IInterface::asBinder(listener); + reportedListeners.merge(mReportedListeners); + mReportedListeners.clear(); + + // Update mUnackedState to include the message we're about to send + auto [it, _] = mUnackedState.try_emplace(update.vsyncId, + UnackedState{.reportedListeners = + std::move(reportedListeners)}); + auto& unackedState = it->second; + for (auto& pair : mWindowInfosListeners) { + int64_t listenerId = pair.second.first; + unackedState.unackedListenerIds.push_back(listenerId); + } - // linkToDeath is used here to ensure that the windowInfosReportedListeners - // are called even if one of the windowInfosListeners dies before - // calling onWindowInfosReported. - asBinder->linkToDeath(reportedInvoker); + mDelayInfo.reset(); + updateMaxSendDelay(); - auto status = listener->onWindowInfosChanged(update, reportedInvoker); + // Call the listeners + for (auto& pair : mWindowInfosListeners) { + auto& [listenerId, listener] = pair.second; + auto status = listener->onWindowInfosChanged(update); if (!status.isOk()) { - reportedInvoker->onWindowInfosReported(); + ackWindowInfosReceived(update.vsyncId, listenerId); } } } -binder::Status WindowInfosListenerInvoker::onWindowInfosReported() { - BackgroundExecutor::getInstance().sendCallbacks({[this]() { - gui::WindowInfosUpdate update; - { - std::scoped_lock lock{mMessagesMutex}; - mActiveMessageCount--; - if (!mDelayedUpdate || mActiveMessageCount > 0) { - return; - } - update = std::move(*mDelayedUpdate); - mDelayedUpdate.reset(); - } - windowInfosChanged(std::move(update), {}, false); - }}); - return binder::Status::ok(); -} - WindowInfosListenerInvoker::DebugInfo WindowInfosListenerInvoker::getDebugInfo() { - std::scoped_lock lock{mMessagesMutex}; - updateMaxSendDelay(); - mDebugInfo.pendingMessageCount = mActiveMessageCount; - return mDebugInfo; + DebugInfo result; + BackgroundExecutor::getInstance().sendCallbacks({[&, this]() { + ATRACE_NAME("WindowInfosListenerInvoker::getDebugInfo"); + updateMaxSendDelay(); + result = mDebugInfo; + result.pendingMessageCount = mUnackedState.size(); + }}); + BackgroundExecutor::getInstance().flushQueue(); + return result; } void WindowInfosListenerInvoker::updateMaxSendDelay() { @@ -192,4 +162,41 @@ void WindowInfosListenerInvoker::updateMaxSendDelay() { } } +binder::Status WindowInfosListenerInvoker::ackWindowInfosReceived(int64_t vsyncId, + int64_t listenerId) { + BackgroundExecutor::getInstance().sendCallbacks({[this, vsyncId, listenerId]() { + ATRACE_NAME("WindowInfosListenerInvoker::ackWindowInfosReceived"); + auto it = mUnackedState.find(vsyncId); + if (it == mUnackedState.end()) { + return; + } + + auto& state = it->second; + state.unackedListenerIds.unstable_erase(std::find(state.unackedListenerIds.begin(), + state.unackedListenerIds.end(), + listenerId)); + if (!state.unackedListenerIds.empty()) { + return; + } + + WindowInfosReportedListenerSet reportedListeners{std::move(state.reportedListeners)}; + mUnackedState.erase(vsyncId); + + for (const auto& reportedListener : reportedListeners) { + sp asBinder = IInterface::asBinder(reportedListener); + if (asBinder->isBinderAlive()) { + reportedListener->onWindowInfosReported(); + } + } + + if (!mDelayedUpdate || !mUnackedState.empty()) { + return; + } + gui::WindowInfosUpdate update{std::move(*mDelayedUpdate)}; + mDelayedUpdate.reset(); + windowInfosChanged(std::move(update), {}, false); + }}); + return binder::Status::ok(); +} + } // namespace android diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h index bc465a3a2b..f36b0edd7d 100644 --- a/services/surfaceflinger/WindowInfosListenerInvoker.h +++ b/services/surfaceflinger/WindowInfosListenerInvoker.h @@ -19,11 +19,12 @@ #include #include -#include +#include #include #include #include #include +#include #include #include @@ -35,22 +36,22 @@ using WindowInfosReportedListenerSet = std::unordered_set, gui::SpHash>; -class WindowInfosListenerInvoker : public gui::BnWindowInfosReportedListener, +class WindowInfosListenerInvoker : public gui::BnWindowInfosPublisher, public IBinder::DeathRecipient { public: - void addWindowInfosListener(sp); + void addWindowInfosListener(sp, gui::WindowInfosListenerInfo*); void removeWindowInfosListener(const sp& windowInfosListener); void windowInfosChanged(gui::WindowInfosUpdate update, WindowInfosReportedListenerSet windowInfosReportedListeners, bool forceImmediateCall); - binder::Status onWindowInfosReported() override; + binder::Status ackWindowInfosReceived(int64_t, int64_t) override; struct DebugInfo { VsyncId maxSendDelayVsyncId; nsecs_t maxSendDelayDuration; - uint32_t pendingMessageCount; + size_t pendingMessageCount; }; DebugInfo getDebugInfo(); @@ -58,24 +59,28 @@ protected: void binderDied(const wp& who) override; private: - std::mutex mListenersMutex; - static constexpr size_t kStaticCapacity = 3; - ftl::SmallMap, const sp, kStaticCapacity> - mWindowInfosListeners GUARDED_BY(mListenersMutex); + std::atomic mNextListenerId{0}; + ftl::SmallMap, const std::pair>, + kStaticCapacity> + mWindowInfosListeners; - std::mutex mMessagesMutex; - uint32_t mActiveMessageCount GUARDED_BY(mMessagesMutex) = 0; - std::optional mDelayedUpdate GUARDED_BY(mMessagesMutex); + std::optional mDelayedUpdate; WindowInfosReportedListenerSet mReportedListeners; - DebugInfo mDebugInfo GUARDED_BY(mMessagesMutex); + struct UnackedState { + ftl::SmallVector unackedListenerIds; + WindowInfosReportedListenerSet reportedListeners; + }; + ftl::SmallMap mUnackedState; + + DebugInfo mDebugInfo; struct DelayInfo { int64_t vsyncId; nsecs_t frameTime; }; - std::optional mDelayInfo GUARDED_BY(mMessagesMutex); - void updateMaxSendDelay() REQUIRES(mMessagesMutex); + std::optional mDelayInfo; + void updateMaxSendDelay(); }; } // namespace android diff --git a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp index af4971b063..c7b845e668 100644 --- a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp +++ b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp @@ -15,35 +15,23 @@ protected: WindowInfosListenerInvokerTest() : mInvoker(sp::make()) {} ~WindowInfosListenerInvokerTest() { - std::mutex mutex; - std::condition_variable cv; - bool flushComplete = false; // Flush the BackgroundExecutor thread to ensure any scheduled tasks are complete. // Otherwise, references those tasks hold may go out of scope before they are done // executing. - BackgroundExecutor::getInstance().sendCallbacks({[&]() { - std::scoped_lock lock{mutex}; - flushComplete = true; - cv.notify_one(); - }}); - std::unique_lock lock{mutex}; - cv.wait(lock, [&]() { return flushComplete; }); + BackgroundExecutor::getInstance().flushQueue(); } sp mInvoker; }; -using WindowInfosUpdateConsumer = std::function&)>; +using WindowInfosUpdateConsumer = std::function; class Listener : public gui::BnWindowInfosListener { public: Listener(WindowInfosUpdateConsumer consumer) : mConsumer(std::move(consumer)) {} - binder::Status onWindowInfosChanged( - const gui::WindowInfosUpdate& update, - const sp& reportedListener) override { - mConsumer(update, reportedListener); + binder::Status onWindowInfosChanged(const gui::WindowInfosUpdate& update) override { + mConsumer(update); return binder::Status::ok(); } @@ -58,15 +46,17 @@ TEST_F(WindowInfosListenerInvokerTest, callsSingleListener) { int callCount = 0; - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); - reportedListener->onWindowInfosReported(); - })); + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo.listenerId); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks( {[this]() { mInvoker->windowInfosChanged({}, {}, false); }}); @@ -81,21 +71,27 @@ TEST_F(WindowInfosListenerInvokerTest, callsMultipleListeners) { std::mutex mutex; std::condition_variable cv; - int callCount = 0; - const int expectedCallCount = 3; - - for (int i = 0; i < expectedCallCount; i++) { - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, - const sp& reportedListener) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - - reportedListener->onWindowInfosReported(); - })); + size_t callCount = 0; + const size_t expectedCallCount = 3; + std::vector listenerInfos{expectedCallCount, + gui::WindowInfosListenerInfo{}}; + + for (size_t i = 0; i < expectedCallCount; i++) { + mInvoker->addWindowInfosListener(sp::make([&, &listenerInfo = listenerInfos[i]]( + const gui::WindowInfosUpdate& + update) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + + listenerInfo.windowInfosPublisher + ->ackWindowInfosReceived(update.vsyncId, + listenerInfo + .listenerId); + }), + &listenerInfos[i]); } BackgroundExecutor::getInstance().sendCallbacks( @@ -114,17 +110,20 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { int callCount = 0; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, + false); }}); { @@ -134,7 +133,7 @@ TEST_F(WindowInfosListenerInvokerTest, delaysUnackedCall) { EXPECT_EQ(callCount, 1); // Ack the first message. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(0, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -152,19 +151,21 @@ TEST_F(WindowInfosListenerInvokerTest, sendsForcedMessage) { int callCount = 0; const int expectedCallCount = 2; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - if (callCount == expectedCallCount) { - cv.notify_one(); - } - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + if (callCount == expectedCallCount) { + cv.notify_one(); + } + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { - mInvoker->windowInfosChanged({}, {}, false); - mInvoker->windowInfosChanged({}, {}, true); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 0, 0}, {}, + false); + mInvoker->windowInfosChanged(gui::WindowInfosUpdate{{}, {}, /* vsyncId= */ 1, 0}, {}, true); }}); { @@ -182,14 +183,14 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { int64_t lastUpdateId = -1; - // Simulate a slow ack by not calling the WindowInfosReportedListener. - mInvoker->addWindowInfosListener( - sp::make([&](const gui::WindowInfosUpdate& update, - const sp&) { - std::scoped_lock lock{mutex}; - lastUpdateId = update.vsyncId; - cv.notify_one(); - })); + // Simulate a slow ack by not calling IWindowInfosPublisher.ackWindowInfosReceived + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate& update) { + std::scoped_lock lock{mutex}; + lastUpdateId = update.vsyncId; + cv.notify_one(); + }), + &listenerInfo); BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({{}, {}, /* vsyncId= */ 1, 0}, {}, false); @@ -204,7 +205,7 @@ TEST_F(WindowInfosListenerInvokerTest, skipsDelayedMessage) { EXPECT_EQ(lastUpdateId, 1); // Ack the first message. The third update should be sent. - mInvoker->onWindowInfosReported(); + listenerInfo.windowInfosPublisher->ackWindowInfosReceived(1, listenerInfo.listenerId); { std::unique_lock lock{mutex}; @@ -225,14 +226,17 @@ TEST_F(WindowInfosListenerInvokerTest, noListeners) { // delayed. BackgroundExecutor::getInstance().sendCallbacks({[&]() { mInvoker->windowInfosChanged({}, {}, false); - mInvoker->addWindowInfosListener(sp::make( - [&](const gui::WindowInfosUpdate&, const sp&) { - std::scoped_lock lock{mutex}; - callCount++; - cv.notify_one(); - })); - mInvoker->windowInfosChanged({}, {}, false); + gui::WindowInfosListenerInfo listenerInfo; + mInvoker->addWindowInfosListener(sp::make([&](const gui::WindowInfosUpdate&) { + std::scoped_lock lock{mutex}; + callCount++; + cv.notify_one(); + }), + &listenerInfo); }}); + BackgroundExecutor::getInstance().flushQueue(); + BackgroundExecutor::getInstance().sendCallbacks( + {[&]() { mInvoker->windowInfosChanged({}, {}, false); }}); { std::unique_lock lock{mutex}; -- cgit v1.2.3 From a0c7cca5769008d02e0ce227feb3efd7471813a4 Mon Sep 17 00:00:00 2001 From: Robert Phillips Date: Wed, 23 Aug 2023 15:18:27 -0400 Subject: Don't look through the buffer cache in protected contexts Test: manual - protected content in Netflix apk, even without blurring Without this change getOrCreateBackendTexture can return an un-Protected backend texture when a Protected one is required. This causes the Protected content draw to be dropped. Note: This CL originally landed in main as part of a broader fix for blurring protected content, but it has been discovered to fix other issues with protected content that were otherwise still visible in this branch. Bug: 242266174 Bug: 294357792 Change-Id: I9d7bab890ae8d88656e77dede962b40e926580c8 (cherry picked from commit 59f71734258e3959d2d3d8937d689b1bb88941db) Merged-In: I9d7bab890ae8d88656e77dede962b40e926580c8 --- libs/renderengine/skia/SkiaRenderEngine.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/libs/renderengine/skia/SkiaRenderEngine.cpp b/libs/renderengine/skia/SkiaRenderEngine.cpp index 9e9df5216f..edf734271a 100644 --- a/libs/renderengine/skia/SkiaRenderEngine.cpp +++ b/libs/renderengine/skia/SkiaRenderEngine.cpp @@ -397,12 +397,10 @@ void SkiaRenderEngine::mapExternalTextureBuffer(const sp& buffer, } // We don't attempt to map a buffer if the buffer contains protected content. In GL this is // important because GPU resources for protected buffers are much more limited. (In Vk we - // simply match the existing behavior for protected buffers.) In Vk, we never cache any - // buffers while in a protected context, since Vk cannot share across contexts, and protected - // is less common. + // simply match the existing behavior for protected buffers.) We also never cache any + // buffers while in a protected context. const bool isProtectedBuffer = buffer->getUsage() & GRALLOC_USAGE_PROTECTED; - if (isProtectedBuffer || - (mRenderEngineType == RenderEngineType::SKIA_VK_THREADED && isProtected())) { + if (isProtectedBuffer || isProtected()) { return; } ATRACE_CALL(); @@ -467,9 +465,8 @@ void SkiaRenderEngine::unmapExternalTextureBuffer(sp&& buffer) { std::shared_ptr SkiaRenderEngine::getOrCreateBackendTexture( const sp& buffer, bool isOutputBuffer) { - // Do not lookup the buffer in the cache for protected contexts with the SkiaVk back-end - if (mRenderEngineType == RenderEngineType::SKIA_GL_THREADED || - (mRenderEngineType == RenderEngineType::SKIA_VK_THREADED && !isProtected())) { + // Do not lookup the buffer in the cache for protected contexts + if (!isProtected()) { if (const auto& it = mTextureCache.find(buffer->getId()); it != mTextureCache.end()) { return it->second; } -- cgit v1.2.3 From 77b758c59f58a05d1c0d45350796951bc778745f Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Fri, 4 Aug 2023 22:17:08 +0000 Subject: File size seal for memory mapped region When using memfd for cross process communication, we always need to seal the file size, otherwise remote process and shrink the size we memory mapped and thus crash the originate process causing a DoS Bug: 294609150 Test: Build Ignore-AOSP-First: security Change-Id: Ibc263c4f78df897e884378e3d984a188ca8772c7 Merged-In: Ibc263c4f78df897e884378e3d984a188ca8772c7 (cherry picked from commit 3d9f1e3b0a135b784b9ffa0e65d6a699c7ed1f8e) --- libs/binder/MemoryHeapBase.cpp | 4 ++-- libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/libs/binder/MemoryHeapBase.cpp b/libs/binder/MemoryHeapBase.cpp index 8fe1d2bb3d..34e747ef21 100644 --- a/libs/binder/MemoryHeapBase.cpp +++ b/libs/binder/MemoryHeapBase.cpp @@ -73,8 +73,8 @@ MemoryHeapBase::MemoryHeapBase(size_t size, uint32_t flags, char const * name) ALOGV("MemoryHeapBase: Attempting to force MemFD"); fd = memfd_create_region(name ? name : "MemoryHeapBase", size); if (fd < 0 || (mapfd(fd, true, size) != NO_ERROR)) return; - const int SEAL_FLAGS = ((mFlags & READ_ONLY) ? F_SEAL_FUTURE_WRITE : 0) | - ((mFlags & MEMFD_ALLOW_SEALING_FLAG) ? 0 : F_SEAL_SEAL); + const int SEAL_FLAGS = ((mFlags & READ_ONLY) ? F_SEAL_FUTURE_WRITE : 0) | F_SEAL_GROW | + F_SEAL_SHRINK | ((mFlags & MEMFD_ALLOW_SEALING_FLAG) ? 0 : F_SEAL_SEAL); if (SEAL_FLAGS && (fcntl(fd, F_ADD_SEALS, SEAL_FLAGS) == -1)) { ALOGE("MemoryHeapBase: MemFD %s sealing with flags %x failed with error %s", name, SEAL_FLAGS, strerror(errno)); diff --git a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp index 278dd2bf81..140270f5a1 100644 --- a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp +++ b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp @@ -37,7 +37,8 @@ TEST(MemoryHeapBase, MemfdSealed) { ASSERT_NE(mHeap.get(), nullptr); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdUnsealed) { @@ -48,7 +49,8 @@ TEST(MemoryHeapBase, MemfdUnsealed) { ASSERT_NE(mHeap.get(), nullptr); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), 0); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdSealedProtected) { @@ -59,7 +61,9 @@ TEST(MemoryHeapBase, MemfdSealedProtected) { ASSERT_NE(mHeap.get(), nullptr); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdUnsealedProtected) { @@ -71,7 +75,8 @@ TEST(MemoryHeapBase, MemfdUnsealedProtected) { ASSERT_NE(mHeap.get(), nullptr); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_FUTURE_WRITE); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(ftruncate(fd, 4096), -1); } #else -- cgit v1.2.3 From f2c1d9d28083fdcba53f346bba5289e72bc4be49 Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Fri, 4 Aug 2023 22:17:08 +0000 Subject: File size seal for memory mapped region When using memfd for cross process communication, we always need to seal the file size, otherwise remote process and shrink the size we memory mapped and thus crash the originate process causing a DoS Bug: 294609150 Test: Build Ignore-AOSP-First: security Change-Id: Ibc263c4f78df897e884378e3d984a188ca8772c7 Merged-In: Ibc263c4f78df897e884378e3d984a188ca8772c7 (cherry picked from commit 3d9f1e3b0a135b784b9ffa0e65d6a699c7ed1f8e) --- libs/binder/MemoryHeapBase.cpp | 4 ++-- libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/libs/binder/MemoryHeapBase.cpp b/libs/binder/MemoryHeapBase.cpp index 8132d46940..0968b89eae 100644 --- a/libs/binder/MemoryHeapBase.cpp +++ b/libs/binder/MemoryHeapBase.cpp @@ -73,8 +73,8 @@ MemoryHeapBase::MemoryHeapBase(size_t size, uint32_t flags, char const * name) ALOGV("MemoryHeapBase: Attempting to force MemFD"); fd = memfd_create_region(name ? name : "MemoryHeapBase", size); if (fd < 0 || (mapfd(fd, true, size) != NO_ERROR)) return; - const int SEAL_FLAGS = ((mFlags & READ_ONLY) ? F_SEAL_FUTURE_WRITE : 0) | - ((mFlags & MEMFD_ALLOW_SEALING) ? 0 : F_SEAL_SEAL); + const int SEAL_FLAGS = ((mFlags & READ_ONLY) ? F_SEAL_FUTURE_WRITE : 0) | F_SEAL_GROW | + F_SEAL_SHRINK | ((mFlags & MEMFD_ALLOW_SEALING) ? 0 : F_SEAL_SEAL); if (SEAL_FLAGS && (fcntl(fd, F_ADD_SEALS, SEAL_FLAGS) == -1)) { ALOGE("MemoryHeapBase: MemFD %s sealing with flags %x failed with error %s", name, SEAL_FLAGS, strerror(errno)); diff --git a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp index 21cb70be17..7d19fd4cff 100644 --- a/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp +++ b/libs/binder/tests/binderMemoryHeapBaseUnitTest.cpp @@ -35,7 +35,8 @@ TEST(MemoryHeapBase, MemfdSealed) { "Test mapping"); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdUnsealed) { @@ -45,7 +46,8 @@ TEST(MemoryHeapBase, MemfdUnsealed) { "Test mapping"); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), 0); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdSealedProtected) { @@ -55,7 +57,9 @@ TEST(MemoryHeapBase, MemfdSealedProtected) { "Test mapping"); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_SEAL | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), + F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(ftruncate(fd, 4096), -1); } TEST(MemoryHeapBase, MemfdUnsealedProtected) { @@ -66,7 +70,8 @@ TEST(MemoryHeapBase, MemfdUnsealedProtected) { "Test mapping"); int fd = mHeap->getHeapID(); EXPECT_NE(fd, -1); - EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_FUTURE_WRITE); + EXPECT_EQ(fcntl(fd, F_GET_SEALS), F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_FUTURE_WRITE); + EXPECT_EQ(ftruncate(fd, 4096), -1); } #else -- cgit v1.2.3 From 538c8f7276bad38cd9db35382a6c4cda09c4d050 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= Date: Thu, 28 Sep 2023 05:12:28 +0000 Subject: make it clear mGpuMemTotalMap is R/O MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test: TreeHugger Signed-off-by: Maciej Żenczykowski (cherry picked from https://android-review.googlesource.com/q/commit:af8d3a709b93cb2a857ff783840b3b3f1d83d16b) Merged-In: I0484c629a02b84b59ca19b35e24a6496fbfd0990 Change-Id: I0484c629a02b84b59ca19b35e24a6496fbfd0990 --- services/gpuservice/gpumem/GpuMem.cpp | 2 +- services/gpuservice/gpumem/include/gpumem/GpuMem.h | 4 ++-- services/gpuservice/tests/unittests/TestableGpuMem.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/gpuservice/gpumem/GpuMem.cpp b/services/gpuservice/gpumem/GpuMem.cpp index dd3cc3bd86..141fe021ee 100644 --- a/services/gpuservice/gpumem/GpuMem.cpp +++ b/services/gpuservice/gpumem/GpuMem.cpp @@ -77,7 +77,7 @@ void GpuMem::initialize() { mInitialized.store(true); } -void GpuMem::setGpuMemTotalMap(bpf::BpfMap& map) { +void GpuMem::setGpuMemTotalMap(bpf::BpfMapRO& map) { mGpuMemTotalMap = std::move(map); } diff --git a/services/gpuservice/gpumem/include/gpumem/GpuMem.h b/services/gpuservice/gpumem/include/gpumem/GpuMem.h index 7588b54818..9aa74d6863 100644 --- a/services/gpuservice/gpumem/include/gpumem/GpuMem.h +++ b/services/gpuservice/gpumem/include/gpumem/GpuMem.h @@ -44,12 +44,12 @@ private: friend class TestableGpuMem; // set gpu memory total map - void setGpuMemTotalMap(bpf::BpfMap& map); + void setGpuMemTotalMap(bpf::BpfMapRO& map); // indicate whether ebpf has been initialized std::atomic mInitialized = false; // bpf map for GPU memory total data - android::bpf::BpfMap mGpuMemTotalMap; + android::bpf::BpfMapRO mGpuMemTotalMap; // gpu memory tracepoint event category static constexpr char kGpuMemTraceGroup[] = "gpu_mem"; diff --git a/services/gpuservice/tests/unittests/TestableGpuMem.h b/services/gpuservice/tests/unittests/TestableGpuMem.h index 6c8becb075..f21843fe1a 100644 --- a/services/gpuservice/tests/unittests/TestableGpuMem.h +++ b/services/gpuservice/tests/unittests/TestableGpuMem.h @@ -28,7 +28,7 @@ public: void setInitialized() { mGpuMem->mInitialized.store(true); } - void setGpuMemTotalMap(bpf::BpfMap& map) { + void setGpuMemTotalMap(bpf::BpfMapRO& map) { mGpuMem->setGpuMemTotalMap(map); } -- cgit v1.2.3