Support task ID for fps listener rather than SurfaceControl.

Bug: 174956756
Test: e2e test with dashboard cls
Change-Id: I841af53ac820a91d270a75c5cc0ca258df0a3945

10 files changed, 69 insertions, 47 deletions

diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index 05e19354da..989abd9a15 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -755,11 +755,10 @@ public:
         return error;
     }
 
-    virtual status_t addFpsListener(const sp<IBinder>& layerHandle,
-                                    const sp<gui::IFpsListener>& listener) {
+    virtual status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) {
         Parcel data, reply;
         SAFE_PARCEL(data.writeInterfaceToken, ISurfaceComposer::getInterfaceDescriptor());
-        SAFE_PARCEL(data.writeStrongBinder, layerHandle);
+        SAFE_PARCEL(data.writeInt32, taskId);
         SAFE_PARCEL(data.writeStrongBinder, IInterface::asBinder(listener));
         const status_t error =
                 remote()->transact(BnSurfaceComposer::ADD_FPS_LISTENER, data, &reply);
@@ -1669,8 +1668,8 @@ status_t BnSurfaceComposer::onTransact(
         }
         case ADD_FPS_LISTENER: {
             CHECK_INTERFACE(ISurfaceComposer, data, reply);
-            sp<IBinder> layerHandle;
-            status_t result = data.readNullableStrongBinder(&layerHandle);
+            int32_t taskId;
+            status_t result = data.readInt32(&taskId);
             if (result != NO_ERROR) {
                 ALOGE("addFpsListener: Failed to read layer handle");
                 return result;
@@ -1681,7 +1680,7 @@ status_t BnSurfaceComposer::onTransact(
                 ALOGE("addFpsListener: Failed to read listener");
                 return result;
             }
-            return addFpsListener(layerHandle, listener);
+            return addFpsListener(taskId, listener);
         }
         case REMOVE_FPS_LISTENER: {
             CHECK_INTERFACE(ISurfaceComposer, data, reply);
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index f5cde5932d..5e8ab929f5 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -1982,9 +1982,9 @@ status_t SurfaceComposerClient::removeRegionSamplingListener(
     return ComposerService::getComposerService()->removeRegionSamplingListener(listener);
 }
 
-status_t SurfaceComposerClient::addFpsListener(const sp<IBinder>& layerHandle,
+status_t SurfaceComposerClient::addFpsListener(int32_t taskId,
                                                const sp<gui::IFpsListener>& listener) {
-    return ComposerService::getComposerService()->addFpsListener(layerHandle, listener);
+    return ComposerService::getComposerService()->addFpsListener(taskId, listener);
 }
 
 status_t SurfaceComposerClient::removeFpsListener(const sp<gui::IFpsListener>& listener) {
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index 8b64bcffca..88cfe4b9ac 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -351,16 +351,15 @@ public:
 
     /* Registers a listener that streams fps updates from SurfaceFlinger.
      *
-     * The listener will stream fps updates for the layer tree rooted at layerHandle. Usually, this
-     * should be tied to a task. Layers that are not descendants of that task are out of scope for
-     * FPS computations.
+     * The listener will stream fps updates for the layer tree rooted at the layer denoted by the
+     * task ID, i.e., the layer must have the task ID as part of its layer metadata with key
+     * METADATA_TASK_ID. If there is no such layer, then no fps is expected to be reported.
      *
      * Multiple listeners may be supported.
      *
-     * Requires the ACCESS_SURFACE_FLINGER permission.
+     * Requires the READ_FRAME_BUFFER permission.
      */
-    virtual status_t addFpsListener(const sp<IBinder>& layerHandle,
-                                    const sp<gui::IFpsListener>& listener) = 0;
+    virtual status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) = 0;
     /*
      * Removes a listener that was streaming fps updates from SurfaceFlinger.
      */
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index ab0347c18f..de88943cde 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -589,8 +589,7 @@ public:
                                               const sp<IBinder>& stopLayerHandle,
                                               const sp<IRegionSamplingListener>& listener);
     static status_t removeRegionSamplingListener(const sp<IRegionSamplingListener>& listener);
-    static status_t addFpsListener(const sp<IBinder>& layerHandle,
-                                   const sp<gui::IFpsListener>& listener);
+    static status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener);
     static status_t removeFpsListener(const sp<gui::IFpsListener>& listener);
 
 private:
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index 91b2affa2f..e8fb71dc1d 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -824,8 +824,7 @@ public:
             const sp<IRegionSamplingListener>& /*listener*/) override {
         return NO_ERROR;
     }
-    status_t addFpsListener(const sp<IBinder>& /*layerHandle*/,
-                            const sp<gui::IFpsListener>& /*listener*/) {
+    status_t addFpsListener(int32_t /*taskId*/, const sp<gui::IFpsListener>& /*listener*/) {
         return NO_ERROR;
     }
     status_t removeFpsListener(const sp<gui::IFpsListener>& /*listener*/) { return NO_ERROR; }
diff --git a/services/surfaceflinger/FpsReporter.cpp b/services/surfaceflinger/FpsReporter.cpp
index c7dbf88d32..0bc2d3eb01 100644
--- a/services/surfaceflinger/FpsReporter.cpp
+++ b/services/surfaceflinger/FpsReporter.cpp
@@ -18,14 +18,16 @@
 #define LOG_TAG "FpsReporter"
 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
 
-#include "FpsReporter.h"
+#include <algorithm>
 
+#include "FpsReporter.h"
 #include "Layer.h"
+#include "SurfaceFlinger.h"
 
 namespace android {
 
-FpsReporter::FpsReporter(frametimeline::FrameTimeline& frameTimeline)
-      : mFrameTimeline(frameTimeline) {}
+FpsReporter::FpsReporter(frametimeline::FrameTimeline& frameTimeline, SurfaceFlinger& flinger)
+      : mFrameTimeline(frameTimeline), mFlinger(flinger) {}
 
 void FpsReporter::dispatchLayerFps() const {
     std::vector<TrackedListener> localListeners;
@@ -41,16 +43,33 @@ void FpsReporter::dispatchLayerFps() const {
                        });
     }
 
-    for (const auto& listener : localListeners) {
-        sp<Layer> promotedLayer = listener.layer.promote();
-        if (promotedLayer != nullptr) {
-            std::unordered_set<int32_t> layerIds;
-
-            promotedLayer->traverse(LayerVector::StateSet::Drawing,
-                                    [&](Layer* layer) { layerIds.insert(layer->getSequence()); });
+    std::unordered_set<int32_t> seenTasks;
+    std::vector<std::pair<TrackedListener, sp<Layer>>> listenersAndLayersToReport;
 
-            listener.listener->onFpsReported(mFrameTimeline.computeFps(layerIds));
+    mFlinger.mCurrentState.traverse([&](Layer* layer) {
+        auto& currentState = layer->getCurrentState();
+        if (currentState.metadata.has(METADATA_TASK_ID)) {
+            int32_t taskId = currentState.metadata.getInt32(METADATA_TASK_ID, 0);
+            if (seenTasks.count(taskId) == 0) {
+                // localListeners is expected to be tiny
+                for (TrackedListener& listener : localListeners) {
+                    if (listener.taskId == taskId) {
+                        seenTasks.insert(taskId);
+                        listenersAndLayersToReport.push_back({listener, sp<Layer>(layer)});
+                        break;
+                    }
+                }
+            }
         }
+    });
+
+    for (const auto& [listener, layer] : listenersAndLayersToReport) {
+        std::unordered_set<int32_t> layerIds;
+
+        layer->traverse(LayerVector::StateSet::Current,
+                        [&](Layer* layer) { layerIds.insert(layer->getSequence()); });
+
+        listener.listener->onFpsReported(mFrameTimeline.computeFps(layerIds));
     }
 }
 
@@ -59,11 +78,11 @@ void FpsReporter::binderDied(const wp<IBinder>& who) {
     mListeners.erase(who);
 }
 
-void FpsReporter::addListener(const sp<gui::IFpsListener>& listener, const wp<Layer>& layer) {
+void FpsReporter::addListener(const sp<gui::IFpsListener>& listener, int32_t taskId) {
     sp<IBinder> asBinder = IInterface::asBinder(listener);
     asBinder->linkToDeath(this);
     std::lock_guard lock(mMutex);
-    mListeners.emplace(wp<IBinder>(asBinder), TrackedListener{listener, layer});
+    mListeners.emplace(wp<IBinder>(asBinder), TrackedListener{listener, taskId});
 }
 
 void FpsReporter::removeListener(const sp<gui::IFpsListener>& listener) {
diff --git a/services/surfaceflinger/FpsReporter.h b/services/surfaceflinger/FpsReporter.h
index d64b3dc6cf..1cec295a31 100644
--- a/services/surfaceflinger/FpsReporter.h
+++ b/services/surfaceflinger/FpsReporter.h
@@ -27,10 +27,11 @@
 namespace android {
 
 class Layer;
+class SurfaceFlinger;
 
 class FpsReporter : public IBinder::DeathRecipient {
 public:
-    FpsReporter(frametimeline::FrameTimeline& frameTimeline);
+    FpsReporter(frametimeline::FrameTimeline& frameTimeline, SurfaceFlinger& flinger);
 
     // Dispatches updated layer fps values for the registered listeners
     // This method promotes Layer weak pointers and performs layer stack traversals, so mStateLock
@@ -41,7 +42,7 @@ public:
     void binderDied(const wp<IBinder>&) override;
 
     // Registers an Fps listener that listens to fps updates for the provided layer
-    void addListener(const sp<gui::IFpsListener>& listener, const wp<Layer>& layer);
+    void addListener(const sp<gui::IFpsListener>& listener, int32_t taskId);
     // Deregisters an Fps listener
     void removeListener(const sp<gui::IFpsListener>& listener);
 
@@ -55,10 +56,11 @@ private:
 
     struct TrackedListener {
         sp<gui::IFpsListener> listener;
-        wp<Layer> layer;
+        int32_t taskId;
     };
 
     frametimeline::FrameTimeline& mFrameTimeline;
+    SurfaceFlinger& mFlinger;
     std::unordered_map<wp<IBinder>, TrackedListener, WpHash> mListeners GUARDED_BY(mMutex);
 };
 
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index ad91183f18..727386c859 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1433,14 +1433,12 @@ status_t SurfaceFlinger::removeRegionSamplingListener(const sp<IRegionSamplingLi
     return NO_ERROR;
 }
 
-status_t SurfaceFlinger::addFpsListener(const sp<IBinder>& layerHandle,
-                                        const sp<gui::IFpsListener>& listener) {
+status_t SurfaceFlinger::addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) {
     if (!listener) {
         return BAD_VALUE;
     }
 
-    const wp<Layer> layer = fromHandle(layerHandle);
-    mFpsReporter->addListener(listener, layer);
+    mFpsReporter->addListener(listener, taskId);
     return NO_ERROR;
 }
 
@@ -3006,7 +3004,7 @@ void SurfaceFlinger::initScheduler(const DisplayDeviceState& displayState) {
     mRegionSamplingThread =
             new RegionSamplingThread(*this, *mScheduler,
                                      RegionSamplingThread::EnvironmentTimingTunables());
-    mFpsReporter = new FpsReporter(*mFrameTimeline);
+    mFpsReporter = new FpsReporter(*mFrameTimeline, *this);
     // Dispatch a mode change request for the primary display on scheduler
     // initialization, so that the EventThreads always contain a reference to a
     // prior configuration.
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 6434ca29ff..8e69eadb54 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -363,6 +363,7 @@ private:
     friend class BufferQueueLayer;
     friend class BufferStateLayer;
     friend class Client;
+    friend class FpsReporter;
     friend class Layer;
     friend class MonitoredProducer;
     friend class RefreshRateOverlay;
@@ -627,8 +628,7 @@ private:
     status_t addRegionSamplingListener(const Rect& samplingArea, const sp<IBinder>& stopLayerHandle,
                                        const sp<IRegionSamplingListener>& listener) override;
     status_t removeRegionSamplingListener(const sp<IRegionSamplingListener>& listener) override;
-    status_t addFpsListener(const sp<IBinder>& layerHandle,
-                            const sp<gui::IFpsListener>& listener) override;
+    status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) override;
     status_t removeFpsListener(const sp<gui::IFpsListener>& listener) override;
     status_t setDesiredDisplayModeSpecs(const sp<IBinder>& displayToken,
                                         ui::DisplayModeId displayModeId, bool allowGroupSwitching,
diff --git a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp
index a9e5df3e9c..a2291b2762 100644
--- a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp
+++ b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp
@@ -76,7 +76,7 @@ protected:
 
     void setupScheduler();
     void setupComposer(uint32_t virtualDisplayCount);
-    sp<BufferStateLayer> createBufferStateLayer();
+    sp<BufferStateLayer> createBufferStateLayer(LayerMetadata metadata);
 
     TestableSurfaceFlinger mFlinger;
     Hwc2::mock::Composer* mComposer = nullptr;
@@ -91,7 +91,7 @@ protected:
     sp<Layer> mUnrelated;
 
     sp<TestableFpsListener> mFpsListener;
-    sp<FpsReporter> mFpsReporter = new FpsReporter(mFrameTimeline);
+    sp<FpsReporter> mFpsReporter = new FpsReporter(mFrameTimeline, *(mFlinger.flinger()));
 };
 
 FpsReporterTest::FpsReporterTest() {
@@ -110,10 +110,10 @@ FpsReporterTest::~FpsReporterTest() {
     ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name());
 }
 
-sp<BufferStateLayer> FpsReporterTest::createBufferStateLayer() {
+sp<BufferStateLayer> FpsReporterTest::createBufferStateLayer(LayerMetadata metadata = {}) {
     sp<Client> client;
     LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", WIDTH, HEIGHT,
-                           LAYER_FLAGS, LayerMetadata());
+                           LAYER_FLAGS, metadata);
     return new BufferStateLayer(args);
 }
 
@@ -154,7 +154,10 @@ namespace {
 
 TEST_F(FpsReporterTest, callsListeners) {
     mParent = createBufferStateLayer();
-    mTarget = createBufferStateLayer();
+    const constexpr int32_t kTaskId = 12;
+    LayerMetadata targetMetadata;
+    targetMetadata.setInt32(METADATA_TASK_ID, kTaskId);
+    mTarget = createBufferStateLayer(targetMetadata);
     mChild = createBufferStateLayer();
     mGrandChild = createBufferStateLayer();
     mUnrelated = createBufferStateLayer();
@@ -162,6 +165,10 @@ TEST_F(FpsReporterTest, callsListeners) {
     mTarget->addChild(mChild);
     mChild->addChild(mGrandChild);
     mParent->commitChildList();
+    mFlinger.mutableCurrentState().layersSortedByZ.add(mParent);
+    mFlinger.mutableCurrentState().layersSortedByZ.add(mTarget);
+    mFlinger.mutableCurrentState().layersSortedByZ.add(mChild);
+    mFlinger.mutableCurrentState().layersSortedByZ.add(mGrandChild);
 
     float expectedFps = 44.0;
 
@@ -170,7 +177,7 @@ TEST_F(FpsReporterTest, callsListeners) {
                                                 mGrandChild->getSequence())))
             .WillOnce(Return(expectedFps));
 
-    mFpsReporter->addListener(mFpsListener, mTarget);
+    mFpsReporter->addListener(mFpsListener, kTaskId);
     mFpsReporter->dispatchLayerFps();
     EXPECT_EQ(expectedFps, mFpsListener->lastReportedFps);
     mFpsReporter->removeListener(mFpsListener);