Merge changes from topics "consistent-frame-number-type", "metrics-callback-surface-id", "remove-asc-from-fmcallback"

* changes:
  Get rid of unused ASurfaceControl in frame metrics listener callback
  Set uninitialized frame number to 0
  Make frame number type consistent
  Add unit tests for FrameMetricsReporter
  Update JankTrackerTests to reflect API changes
  Pass surface control id to callback to accurately identify surface metrics belongs to
  Stop reporting frame stats from frames completed before observer was attached

libs/hwui/Android.bp

@@ -587,6 +587,7 @@ cc_defaults {
                 "HardwareBitmapUploader.cpp",
                 "HWUIProperties.sysprop",
                 "JankTracker.cpp",
+                "FrameMetricsReporter.cpp",
                 "Layer.cpp",
                 "LayerUpdateQueue.cpp",
                 "ProfileData.cpp",
@@ -691,6 +692,7 @@ cc_test {
         "tests/unit/FatVectorTests.cpp",
         "tests/unit/GraphicsStatsServiceTests.cpp",
         "tests/unit/JankTrackerTests.cpp",
+        "tests/unit/FrameMetricsReporterTests.cpp",
         "tests/unit/LayerUpdateQueueTests.cpp",
         "tests/unit/LinearAllocatorTests.cpp",
         "tests/unit/MatrixTests.cpp",

libs/hwui/FrameMetricsObserver.h

@@ -26,6 +26,13 @@ public:
     virtual void notify(const int64_t* buffer) = 0;
     bool waitForPresentTime() const { return mWaitForPresentTime; };
 
+    void reportMetricsFrom(uint64_t frameNumber, int32_t surfaceControlId) {
+        mAttachedFrameNumber = frameNumber;
+        mSurfaceControlId = surfaceControlId;
+    };
+    uint64_t attachedFrameNumber() const { return mAttachedFrameNumber; };
+    int32_t attachedSurfaceControlId() const { return mSurfaceControlId; };
+
     /**
      * Create a new metrics observer. An observer that watches present time gets notified at a
      * different time than the observer that doesn't.
@@ -42,6 +49,22 @@ public:
 
 private:
     const bool mWaitForPresentTime;
+
+    // The id of the surface control (mSurfaceControlGenerationId in CanvasContext)
+    // for which the mAttachedFrameNumber applies to. We rely on this value being
+    // an increasing counter. We will report metrics:
+    // - for all frames if the frame comes from a surface with a surfaceControlId
+    //   that is strictly greater than mSurfaceControlId.
+    // - for all frames with a frame number greater than or equal to mAttachedFrameNumber
+    //   if the frame comes from a surface with a surfaceControlId that is equal to the
+    //   mSurfaceControlId.
+    // We will never report metrics if the frame comes from a surface with a surfaceControlId
+    // that is strictly smaller than mSurfaceControlId.
+    int32_t mSurfaceControlId;
+
+    // The frame number the metrics observer was attached on. Metrics will be sent from this frame
+    // number (inclusive) onwards in the case that the surface id is equal to mSurfaceControlId.
+    uint64_t mAttachedFrameNumber;
 };
 
 }  // namespace uirenderer

libs/hwui/FrameMetricsReporter.cpp

@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "FrameMetricsReporter.h"
+
+namespace android {
+namespace uirenderer {
+
+void FrameMetricsReporter::reportFrameMetrics(const int64_t* stats, bool hasPresentTime,
+                                              uint64_t frameNumber, int32_t surfaceControlId) {
+    FatVector<sp<FrameMetricsObserver>, 10> copy;
+    {
+        std::lock_guard lock(mObserversLock);
+        copy.reserve(mObservers.size());
+        for (size_t i = 0; i < mObservers.size(); i++) {
+            auto observer = mObservers[i];
+
+            if (CC_UNLIKELY(surfaceControlId < observer->attachedSurfaceControlId())) {
+                // Don't notify if the metrics are from a frame that was run on an old
+                // surface (one from before the observer was attached).
+                ALOGV("skipped reporting metrics from old surface %d", surfaceControlId);
+                continue;
+            } else if (CC_UNLIKELY(surfaceControlId == observer->attachedSurfaceControlId() &&
+                                   frameNumber < observer->attachedFrameNumber())) {
+                // Don't notify if the metrics are from a frame that was queued by the
+                // BufferQueueProducer on the render thread before the observer was attached.
+                ALOGV("skipped reporting metrics from old frame %ld", (long)frameNumber);
+                continue;
+            }
+
+            const bool wantsPresentTime = observer->waitForPresentTime();
+            if (hasPresentTime == wantsPresentTime) {
+                copy.push_back(observer);
+            }
+        }
+    }
+    for (size_t i = 0; i < copy.size(); i++) {
+        copy[i]->notify(stats);
+    }
+}
+
+}  // namespace uirenderer
+}  // namespace android

libs/hwui/FrameMetricsReporter.h

@@ -63,23 +63,14 @@ public:
      * If an observer does not want present time, only notify when 'hasPresentTime' is false.
      * Never notify both types of observers from the same callback, because the callback with
      * 'hasPresentTime' is sent at a different time than the one without.
+     *
+     * The 'frameNumber' and 'surfaceControlId' associated to the frame whose's stats are being
+     * reported are used to determine whether or not the stats should be reported. We won't report
+     * stats of frames that are from "old" surfaces (i.e. with surfaceControlIds older than the one
+     * the observer was attached on) nor those that are from "old" frame numbers.
      */
-    void reportFrameMetrics(const int64_t* stats, bool hasPresentTime) {
-        FatVector<sp<FrameMetricsObserver>, 10> copy;
-        {
-            std::lock_guard lock(mObserversLock);
-            copy.reserve(mObservers.size());
-            for (size_t i = 0; i < mObservers.size(); i++) {
-                const bool wantsPresentTime = mObservers[i]->waitForPresentTime();
-                if (hasPresentTime == wantsPresentTime) {
-                    copy.push_back(mObservers[i]);
-                }
-            }
-        }
-        for (size_t i = 0; i < copy.size(); i++) {
-            copy[i]->notify(stats);
-        }
-    }
+    void reportFrameMetrics(const int64_t* stats, bool hasPresentTime, uint64_t frameNumber,
+                            int32_t surfaceControlId);
 
 private:
     FatVector<sp<FrameMetricsObserver>, 10> mObservers GUARDED_BY(mObserversLock);

libs/hwui/JankTracker.cpp

@@ -164,7 +164,8 @@ void JankTracker::calculateLegacyJank(FrameInfo& frame) REQUIRES(mDataMutex) {
             - lastFrameOffset + mFrameIntervalLegacy;
 }
 
-void JankTracker::finishFrame(FrameInfo& frame, std::unique_ptr<FrameMetricsReporter>& reporter) {
+void JankTracker::finishFrame(FrameInfo& frame, std::unique_ptr<FrameMetricsReporter>& reporter,
+                              int64_t frameNumber, int32_t surfaceControlId) {
     std::lock_guard lock(mDataMutex);
 
     calculateLegacyJank(frame);
@@ -253,7 +254,8 @@ void JankTracker::finishFrame(FrameInfo& frame, std::unique_ptr<FrameMetricsRepo
     }
 
     if (CC_UNLIKELY(reporter.get() != nullptr)) {
-        reporter->reportFrameMetrics(frame.data(), false /* hasPresentTime */);
+        reporter->reportFrameMetrics(frame.data(), false /* hasPresentTime */, frameNumber,
+                                     surfaceControlId);
     }
 }
 

libs/hwui/JankTracker.h

@@ -57,7 +57,8 @@ public:
     }
 
     FrameInfo* startFrame() { return &mFrames.next(); }
-    void finishFrame(FrameInfo& frame, std::unique_ptr<FrameMetricsReporter>& reporter);
+    void finishFrame(FrameInfo& frame, std::unique_ptr<FrameMetricsReporter>& reporter,
+                     int64_t frameNumber, int32_t surfaceId);
 
     // Calculates the 'legacy' jank information, i.e. with outdated refresh rate information and
     // without GPU completion or deadlined information.

libs/hwui/renderthread/CanvasContext.cpp

@@ -203,9 +203,10 @@ void CanvasContext::setSurfaceControl(ASurfaceControl* surfaceControl) {
     mSurfaceControl = surfaceControl;
     mSurfaceControlGenerationId++;
     mExpectSurfaceStats = surfaceControl != nullptr;
-    if (mSurfaceControl != nullptr) {
+    if (mExpectSurfaceStats) {
         funcs.acquireFunc(mSurfaceControl);
-        funcs.registerListenerFunc(surfaceControl, this, &onSurfaceStatsAvailable);
+        funcs.registerListenerFunc(surfaceControl, mSurfaceControlGenerationId, this,
+                                   &onSurfaceStatsAvailable);
     }
 }
 
@@ -218,7 +219,7 @@ void CanvasContext::setupPipelineSurface() {
 
     }
 
-    mFrameNumber = -1;
+    mFrameNumber = 0;
 
     if (mNativeSurface != nullptr && hasSurface) {
         mHaveNewSurface = true;
@@ -512,7 +513,7 @@ nsecs_t CanvasContext::draw() {
                                       mContentDrawBounds, mOpaque, mLightInfo, mRenderNodes,
                                       &(profiler()));
 
-    int64_t frameCompleteNr = getFrameNumber();
+    uint64_t frameCompleteNr = getFrameNumber();
 
     waitOnFences();
 
@@ -580,7 +581,7 @@ nsecs_t CanvasContext::draw() {
         mCurrentFrameInfo->set(FrameInfoIndex::DequeueBufferDuration) = swap.dequeueDuration;
         mCurrentFrameInfo->set(FrameInfoIndex::QueueBufferDuration) = swap.queueDuration;
         mHaveNewSurface = false;
-        mFrameNumber = -1;
+        mFrameNumber = 0;
     } else {
         mCurrentFrameInfo->set(FrameInfoIndex::DequeueBufferDuration) = 0;
         mCurrentFrameInfo->set(FrameInfoIndex::QueueBufferDuration) = 0;
@@ -613,16 +614,20 @@ nsecs_t CanvasContext::draw() {
     if (requireSwap) {
         if (mExpectSurfaceStats) {
             reportMetricsWithPresentTime();
-            std::lock_guard lock(mLast4FrameInfosMutex);
-            std::pair<FrameInfo*, int64_t>& next = mLast4FrameInfos.next();
-            next.first = mCurrentFrameInfo;
-            next.second = frameCompleteNr;
+            {  // acquire lock
+                std::lock_guard lock(mLast4FrameMetricsInfosMutex);
+                FrameMetricsInfo& next = mLast4FrameMetricsInfos.next();
+                next.frameInfo = mCurrentFrameInfo;
+                next.frameNumber = frameCompleteNr;
+                next.surfaceId = mSurfaceControlGenerationId;
+            }  // release lock
         } else {
             mCurrentFrameInfo->markFrameCompleted();
             mCurrentFrameInfo->set(FrameInfoIndex::GpuCompleted)
                     = mCurrentFrameInfo->get(FrameInfoIndex::FrameCompleted);
             std::scoped_lock lock(mFrameMetricsReporterMutex);
-            mJankTracker.finishFrame(*mCurrentFrameInfo, mFrameMetricsReporter);
+            mJankTracker.finishFrame(*mCurrentFrameInfo, mFrameMetricsReporter, frameCompleteNr,
+                                     mSurfaceControlGenerationId);
         }
     }
 
@@ -658,14 +663,18 @@ void CanvasContext::reportMetricsWithPresentTime() {
     ATRACE_CALL();
     FrameInfo* forthBehind;
     int64_t frameNumber;
+    int32_t surfaceControlId;
+
     {  // acquire lock
-        std::scoped_lock lock(mLast4FrameInfosMutex);
-        if (mLast4FrameInfos.size() != mLast4FrameInfos.capacity()) {
+        std::scoped_lock lock(mLast4FrameMetricsInfosMutex);
+        if (mLast4FrameMetricsInfos.size() != mLast4FrameMetricsInfos.capacity()) {
             // Not enough frames yet
             return;
         }
-        // Surface object keeps stats for the last 8 frames.
-        std::tie(forthBehind, frameNumber) = mLast4FrameInfos.front();
+        auto frameMetricsInfo = mLast4FrameMetricsInfos.front();
+        forthBehind = frameMetricsInfo.frameInfo;
+        frameNumber = frameMetricsInfo.frameNumber;
+        surfaceControlId = frameMetricsInfo.surfaceId;
     }  // release lock
 
     nsecs_t presentTime = 0;
@@ -680,25 +689,51 @@ void CanvasContext::reportMetricsWithPresentTime() {
     {  // acquire lock
         std::scoped_lock lock(mFrameMetricsReporterMutex);
         if (mFrameMetricsReporter != nullptr) {
-            mFrameMetricsReporter->reportFrameMetrics(forthBehind->data(), true /*hasPresentTime*/);
+            mFrameMetricsReporter->reportFrameMetrics(forthBehind->data(), true /*hasPresentTime*/,
+                                                      frameNumber, surfaceControlId);
         }
     }  // release lock
 }
 
-FrameInfo* CanvasContext::getFrameInfoFromLast4(uint64_t frameNumber) {
-    std::scoped_lock lock(mLast4FrameInfosMutex);
-    for (size_t i = 0; i < mLast4FrameInfos.size(); i++) {
-        if (mLast4FrameInfos[i].second == frameNumber) {
-            return mLast4FrameInfos[i].first;
+void CanvasContext::addFrameMetricsObserver(FrameMetricsObserver* observer) {
+    std::scoped_lock lock(mFrameMetricsReporterMutex);
+    if (mFrameMetricsReporter.get() == nullptr) {
+        mFrameMetricsReporter.reset(new FrameMetricsReporter());
+    }
+
+    // We want to make sure we aren't reporting frames that have already been queued by the
+    // BufferQueueProducer on the rendner thread but are still pending the callback to report their
+    // their frame metrics.
+    uint64_t nextFrameNumber = getFrameNumber();
+    observer->reportMetricsFrom(nextFrameNumber, mSurfaceControlGenerationId);
+    mFrameMetricsReporter->addObserver(observer);
+}
+
+void CanvasContext::removeFrameMetricsObserver(FrameMetricsObserver* observer) {
+    std::scoped_lock lock(mFrameMetricsReporterMutex);
+    if (mFrameMetricsReporter.get() != nullptr) {
+        mFrameMetricsReporter->removeObserver(observer);
+        if (!mFrameMetricsReporter->hasObservers()) {
+            mFrameMetricsReporter.reset(nullptr);
         }
     }
+}
+
+FrameInfo* CanvasContext::getFrameInfoFromLast4(uint64_t frameNumber, uint32_t surfaceControlId) {
+    std::scoped_lock lock(mLast4FrameMetricsInfosMutex);
+    for (size_t i = 0; i < mLast4FrameMetricsInfos.size(); i++) {
+        if (mLast4FrameMetricsInfos[i].frameNumber == frameNumber &&
+            mLast4FrameMetricsInfos[i].surfaceId == surfaceControlId) {
+            return mLast4FrameMetricsInfos[i].frameInfo;
+        }
+    }
+
     return nullptr;
 }
 
-void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* control,
-            ASurfaceControlStats* stats) {
-
-    CanvasContext* instance = static_cast<CanvasContext*>(context);
+void CanvasContext::onSurfaceStatsAvailable(void* context, int32_t surfaceControlId,
+                                            ASurfaceControlStats* stats) {
+    auto* instance = static_cast<CanvasContext*>(context);
 
     const ASurfaceControlFunctions& functions =
             instance->mRenderThread.getASurfaceControlFunctions();
@@ -706,14 +741,15 @@ void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* cont
     nsecs_t gpuCompleteTime = functions.getAcquireTimeFunc(stats);
     uint64_t frameNumber = functions.getFrameNumberFunc(stats);
 
-    FrameInfo* frameInfo = instance->getFrameInfoFromLast4(frameNumber);
+    FrameInfo* frameInfo = instance->getFrameInfoFromLast4(frameNumber, surfaceControlId);
 
     if (frameInfo != nullptr) {
         frameInfo->set(FrameInfoIndex::FrameCompleted) = std::max(gpuCompleteTime,
                 frameInfo->get(FrameInfoIndex::SwapBuffersCompleted));
         frameInfo->set(FrameInfoIndex::GpuCompleted) = gpuCompleteTime;
         std::scoped_lock lock(instance->mFrameMetricsReporterMutex);
-        instance->mJankTracker.finishFrame(*frameInfo, instance->mFrameMetricsReporter);
+        instance->mJankTracker.finishFrame(*frameInfo, instance->mFrameMetricsReporter, frameNumber,
+                                           surfaceControlId);
     }
 }
 
@@ -854,9 +890,9 @@ void CanvasContext::enqueueFrameWork(std::function<void()>&& func) {
     mFrameFences.push_back(CommonPool::async(std::move(func)));
 }
 
-int64_t CanvasContext::getFrameNumber() {
-    // mFrameNumber is reset to -1 when the surface changes or we swap buffers
-    if (mFrameNumber == -1 && mNativeSurface.get()) {
+uint64_t CanvasContext::getFrameNumber() {
+    // mFrameNumber is reset to 0 when the surface changes or we swap buffers
+    if (mFrameNumber == 0 && mNativeSurface.get()) {
         mFrameNumber = ANativeWindow_getNextFrameId(mNativeSurface->getNativeWindow());
     }
     return mFrameNumber;

libs/hwui/renderthread/CanvasContext.h

@@ -167,29 +167,13 @@ public:
 
     void setContentDrawBounds(const Rect& bounds) { mContentDrawBounds = bounds; }
 
-    void addFrameMetricsObserver(FrameMetricsObserver* observer) {
-        std::scoped_lock lock(mFrameMetricsReporterMutex);
-        if (mFrameMetricsReporter.get() == nullptr) {
-            mFrameMetricsReporter.reset(new FrameMetricsReporter());
-        }
-
-        mFrameMetricsReporter->addObserver(observer);
-    }
-
-    void removeFrameMetricsObserver(FrameMetricsObserver* observer) {
-        std::scoped_lock lock(mFrameMetricsReporterMutex);
-        if (mFrameMetricsReporter.get() != nullptr) {
-            mFrameMetricsReporter->removeObserver(observer);
-            if (!mFrameMetricsReporter->hasObservers()) {
-                mFrameMetricsReporter.reset(nullptr);
-            }
-        }
-    }
+    void addFrameMetricsObserver(FrameMetricsObserver* observer);
+    void removeFrameMetricsObserver(FrameMetricsObserver* observer);
 
     // Used to queue up work that needs to be completed before this frame completes
     void enqueueFrameWork(std::function<void()>&& func);
 
-    int64_t getFrameNumber();
+    uint64_t getFrameNumber();
 
     void waitOnFences();
 
@@ -212,8 +196,8 @@ public:
     SkISize getNextFrameSize() const;
 
     // Called when SurfaceStats are available.
-    static void onSurfaceStatsAvailable(void* context, ASurfaceControl* control,
-            ASurfaceControlStats* stats);
+    static void onSurfaceStatsAvailable(void* context, int32_t surfaceControlId,
+                                        ASurfaceControlStats* stats);
 
     void setASurfaceTransactionCallback(
             const std::function<bool(int64_t, int64_t, int64_t)>& callback) {
@@ -254,7 +238,13 @@ private:
      */
     void reportMetricsWithPresentTime();
 
-    FrameInfo* getFrameInfoFromLast4(uint64_t frameNumber);
+    struct FrameMetricsInfo {
+        FrameInfo* frameInfo;
+        int64_t frameNumber;
+        int32_t surfaceId;
+    };
+
+    FrameInfo* getFrameInfoFromLast4(uint64_t frameNumber, uint32_t surfaceControlId);
 
     // The same type as Frame.mWidth and Frame.mHeight
     int32_t mLastFrameWidth = 0;
@@ -266,7 +256,10 @@ private:
     // NULL to remove the reference
     ASurfaceControl* mSurfaceControl = nullptr;
     // id to track surface control changes and WebViewFunctor uses it to determine
-    // whether reparenting is needed
+    // whether reparenting is needed also used by FrameMetricsReporter to determine
+    // if a frame is from an "old" surface (i.e. one that existed before the
+    // observer was attched) and therefore shouldn't be reported.
+    // NOTE: It is important that this is an increasing counter.
     int32_t mSurfaceControlGenerationId = 0;
     // stopped indicates the CanvasContext will reject actual redraw operations,
     // and defer repaint until it is un-stopped
@@ -288,7 +281,8 @@ private:
 
     // Need at least 4 because we do quad buffer. Add a 5th for good measure.
     RingBuffer<SwapHistory, 5> mSwapHistory;
-    int64_t mFrameNumber = -1;
+    // Frame numbers start at 1, 0 means uninitialized
+    uint64_t mFrameNumber = 0;
     int64_t mDamageId = 0;
 
     // last vsync for a dropped frame due to stuffed queue
@@ -308,10 +302,11 @@ private:
 
     FrameInfo* mCurrentFrameInfo = nullptr;
 
-    // List of frames that are awaiting GPU completion reporting
-    RingBuffer<std::pair<FrameInfo*, int64_t>, 4> mLast4FrameInfos
-            GUARDED_BY(mLast4FrameInfosMutex);
-    std::mutex mLast4FrameInfosMutex;
+    // List of data of frames that are awaiting GPU completion reporting. Used to compute frame
+    // metrics and determine whether or not to report the metrics.
+    RingBuffer<FrameMetricsInfo, 4> mLast4FrameMetricsInfos
+            GUARDED_BY(mLast4FrameMetricsInfosMutex);
+    std::mutex mLast4FrameMetricsInfosMutex;
 
     std::string mName;
     JankTracker mJankTracker;

libs/hwui/renderthread/RenderThread.h

@@ -83,8 +83,9 @@ typedef ASurfaceControl* (*ASC_create)(ASurfaceControl* parent, const char* debu
 typedef void (*ASC_acquire)(ASurfaceControl* control);
 typedef void (*ASC_release)(ASurfaceControl* control);
 
-typedef void (*ASC_registerSurfaceStatsListener)(ASurfaceControl* control, void* context,
-        ASurfaceControl_SurfaceStatsListener func);
+typedef void (*ASC_registerSurfaceStatsListener)(ASurfaceControl* control, int32_t id,
+                                                 void* context,
+                                                 ASurfaceControl_SurfaceStatsListener func);
 typedef void (*ASC_unregisterSurfaceStatsListener)(void* context,
                                                    ASurfaceControl_SurfaceStatsListener func);
 

libs/hwui/tests/unit/FrameMetricsReporterTests.cpp

@@ -0,0 +1,205 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <FrameMetricsObserver.h>
+#include <FrameMetricsReporter.h>
+#include <utils/TimeUtils.h>
+
+using namespace android;
+using namespace android::uirenderer;
+
+using ::testing::NotNull;
+
+class TestFrameMetricsObserver : public FrameMetricsObserver {
+public:
+    explicit TestFrameMetricsObserver(bool waitForPresentTime)
+            : FrameMetricsObserver(waitForPresentTime){};
+
+    MOCK_METHOD(void, notify, (const int64_t* buffer), (override));
+};
+
+TEST(FrameMetricsReporter, reportsAllFramesIfNoFromFrameIsSpecified) {
+    auto reporter = std::make_shared<FrameMetricsReporter>();
+
+    auto observer = sp<TestFrameMetricsObserver>::make(false /*waitForPresentTime*/);
+    EXPECT_CALL(*observer, notify).Times(4);
+
+    reporter->addObserver(observer.get());
+
+    const int64_t* stats;
+    bool hasPresentTime = false;
+    uint64_t frameNumber = 1;
+    int32_t surfaceControlId = 0;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    frameNumber = 10;
+    surfaceControlId = 0;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    frameNumber = 0;
+    surfaceControlId = 2;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    frameNumber = 10;
+    surfaceControlId = 2;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+}
+
+TEST(FrameMetricsReporter, respectsWaitForPresentTimeUnset) {
+    auto reporter = std::make_shared<FrameMetricsReporter>();
+
+    auto observer = sp<TestFrameMetricsObserver>::make(false /*waitForPresentTime*/);
+    reporter->addObserver(observer.get());
+
+    const int64_t* stats;
+    bool hasPresentTime = false;
+    uint64_t frameNumber = 3;
+    int32_t surfaceControlId = 0;
+
+    EXPECT_CALL(*observer, notify).Times(1);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    EXPECT_CALL(*observer, notify).Times(0);
+    hasPresentTime = true;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+}
+
+TEST(FrameMetricsReporter, respectsWaitForPresentTimeSet) {
+    auto reporter = std::make_shared<FrameMetricsReporter>();
+
+    auto observer = sp<TestFrameMetricsObserver>::make(true /*waitForPresentTime*/);
+    reporter->addObserver(observer.get());
+
+    const int64_t* stats;
+    bool hasPresentTime = false;
+    uint64_t frameNumber = 3;
+    int32_t surfaceControlId = 0;
+
+    EXPECT_CALL(*observer, notify).Times(0);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    EXPECT_CALL(*observer, notify).Times(1);
+    hasPresentTime = true;
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+}
+
+TEST(FrameMetricsReporter, reportsAllFramesAfterSpecifiedFromFrame) {
+    const int64_t* stats;
+    bool hasPresentTime = false;
+
+    std::vector<uint64_t> frameNumbers{0, 1, 10};
+    std::vector<int32_t> surfaceControlIds{0, 1, 10};
+    for (uint64_t frameNumber : frameNumbers) {
+        for (int32_t surfaceControlId : surfaceControlIds) {
+            auto reporter = std::make_shared<FrameMetricsReporter>();
+
+            auto observer =
+                    sp<TestFrameMetricsObserver>::make(hasPresentTime /*waitForPresentTime*/);
+            observer->reportMetricsFrom(frameNumber, surfaceControlId);
+            reporter->addObserver(observer.get());
+
+            EXPECT_CALL(*observer, notify).Times(8);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 1, surfaceControlId);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 10, surfaceControlId);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId + 1);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber - 1,
+                                         surfaceControlId + 1);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 1,
+                                         surfaceControlId + 1);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 10,
+                                         surfaceControlId + 1);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 10,
+                                         surfaceControlId + 10);
+        }
+    }
+}
+
+TEST(FrameMetricsReporter, doesNotReportsFramesBeforeSpecifiedFromFrame) {
+    const int64_t* stats;
+    bool hasPresentTime = false;
+
+    std::vector<uint64_t> frameNumbers{1, 10};
+    std::vector<int32_t> surfaceControlIds{0, 1, 10};
+    for (uint64_t frameNumber : frameNumbers) {
+        for (int32_t surfaceControlId : surfaceControlIds) {
+            auto reporter = std::make_shared<FrameMetricsReporter>();
+
+            auto observer =
+                    sp<TestFrameMetricsObserver>::make(hasPresentTime /*waitForPresentTime*/);
+            observer->reportMetricsFrom(frameNumber, surfaceControlId);
+            reporter->addObserver(observer.get());
+
+            EXPECT_CALL(*observer, notify).Times(0);
+            reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber - 1, surfaceControlId);
+            if (surfaceControlId > 0) {
+                reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber,
+                                             surfaceControlId - 1);
+                reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber - 1,
+                                             surfaceControlId - 1);
+            }
+        }
+    }
+}
+
+TEST(FrameMetricsReporter, canRemoveObservers) {
+    const int64_t* stats;
+    bool hasPresentTime = false;
+    uint64_t frameNumber = 3;
+    int32_t surfaceControlId = 0;
+
+    auto reporter = std::make_shared<FrameMetricsReporter>();
+
+    auto observer = sp<TestFrameMetricsObserver>::make(hasPresentTime /*waitForPresentTime*/);
+
+    observer->reportMetricsFrom(frameNumber, surfaceControlId);
+    reporter->addObserver(observer.get());
+
+    EXPECT_CALL(*observer, notify).Times(1);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    ASSERT_TRUE(reporter->removeObserver(observer.get()));
+
+    EXPECT_CALL(*observer, notify).Times(0);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+}
+
+TEST(FrameMetricsReporter, canSupportMultipleObservers) {
+    const int64_t* stats;
+    bool hasPresentTime = false;
+    uint64_t frameNumber = 3;
+    int32_t surfaceControlId = 0;
+
+    auto reporter = std::make_shared<FrameMetricsReporter>();
+
+    auto observer1 = sp<TestFrameMetricsObserver>::make(hasPresentTime /*waitForPresentTime*/);
+    auto observer2 = sp<TestFrameMetricsObserver>::make(hasPresentTime /*waitForPresentTime*/);
+    observer1->reportMetricsFrom(frameNumber, surfaceControlId);
+    observer2->reportMetricsFrom(frameNumber + 10, surfaceControlId + 1);
+    reporter->addObserver(observer1.get());
+    reporter->addObserver(observer2.get());
+
+    EXPECT_CALL(*observer1, notify).Times(1);
+    EXPECT_CALL(*observer2, notify).Times(0);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId);
+
+    EXPECT_CALL(*observer1, notify).Times(1);
+    EXPECT_CALL(*observer2, notify).Times(1);
+    reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber + 10, surfaceControlId + 1);
+}

libs/hwui/tests/unit/JankTrackerTests.cpp

@@ -34,6 +34,9 @@ TEST(JankTracker, noJank) {
     JankTracker jankTracker(&container);
     std::unique_ptr<FrameMetricsReporter> reporter = std::make_unique<FrameMetricsReporter>();
 
+    uint64_t frameNumber = 0;
+    uint32_t surfaceId = 0;
+
     FrameInfo* info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 100_ms;
     info->set(FrameInfoIndex::Vsync) = 101_ms;
@@ -42,7 +45,7 @@ TEST(JankTracker, noJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 115_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 120_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 116_ms;
@@ -52,7 +55,7 @@ TEST(JankTracker, noJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 131_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 136_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(2, container.get()->totalFrameCount());
     ASSERT_EQ(0, container.get()->jankFrameCount());
@@ -65,6 +68,9 @@ TEST(JankTracker, jank) {
     JankTracker jankTracker(&container);
     std::unique_ptr<FrameMetricsReporter> reporter = std::make_unique<FrameMetricsReporter>();
 
+    uint64_t frameNumber = 0;
+    uint32_t surfaceId = 0;
+
     FrameInfo* info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 100_ms;
     info->set(FrameInfoIndex::Vsync) = 101_ms;
@@ -73,7 +79,7 @@ TEST(JankTracker, jank) {
     info->set(FrameInfoIndex::FrameCompleted) = 121_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 120_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(1, container.get()->totalFrameCount());
     ASSERT_EQ(1, container.get()->jankFrameCount());
@@ -85,6 +91,9 @@ TEST(JankTracker, legacyJankButNoRealJank) {
     JankTracker jankTracker(&container);
     std::unique_ptr<FrameMetricsReporter> reporter = std::make_unique<FrameMetricsReporter>();
 
+    uint64_t frameNumber = 0;
+    uint32_t surfaceId = 0;
+
     FrameInfo* info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 100_ms;
     info->set(FrameInfoIndex::Vsync) = 101_ms;
@@ -93,7 +102,7 @@ TEST(JankTracker, legacyJankButNoRealJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 118_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 120_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(1, container.get()->totalFrameCount());
     ASSERT_EQ(0, container.get()->jankFrameCount());
@@ -106,6 +115,9 @@ TEST(JankTracker, doubleStuffed) {
     JankTracker jankTracker(&container);
     std::unique_ptr<FrameMetricsReporter> reporter = std::make_unique<FrameMetricsReporter>();
 
+    uint64_t frameNumber = 0;
+    uint32_t surfaceId = 0;
+
     // First frame janks
     FrameInfo* info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 100_ms;
@@ -115,7 +127,7 @@ TEST(JankTracker, doubleStuffed) {
     info->set(FrameInfoIndex::FrameCompleted) = 121_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 120_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(1, container.get()->jankFrameCount());
 
@@ -128,7 +140,7 @@ TEST(JankTracker, doubleStuffed) {
     info->set(FrameInfoIndex::FrameCompleted) = 137_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 136_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(2, container.get()->totalFrameCount());
     ASSERT_EQ(1, container.get()->jankFrameCount());
@@ -140,6 +152,9 @@ TEST(JankTracker, doubleStuffedThenPauseThenJank) {
     JankTracker jankTracker(&container);
     std::unique_ptr<FrameMetricsReporter> reporter = std::make_unique<FrameMetricsReporter>();
 
+    uint64_t frameNumber = 0;
+    uint32_t surfaceId = 0;
+
     // First frame janks
     FrameInfo* info = jankTracker.startFrame();
     info->set(FrameInfoIndex::IntendedVsync) = 100_ms;
@@ -149,7 +164,7 @@ TEST(JankTracker, doubleStuffedThenPauseThenJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 121_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 120_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(1, container.get()->jankFrameCount());
 
@@ -162,7 +177,7 @@ TEST(JankTracker, doubleStuffedThenPauseThenJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 137_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 136_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(1, container.get()->jankFrameCount());
 
@@ -175,8 +190,8 @@ TEST(JankTracker, doubleStuffedThenPauseThenJank) {
     info->set(FrameInfoIndex::FrameCompleted) = 169_ms;
     info->set(FrameInfoIndex::FrameInterval) = 16_ms;
     info->set(FrameInfoIndex::FrameDeadline) = 168_ms;
-    jankTracker.finishFrame(*info, reporter);
+    jankTracker.finishFrame(*info, reporter, frameNumber, surfaceId);
 
     ASSERT_EQ(3, container.get()->totalFrameCount());
     ASSERT_EQ(2, container.get()->jankFrameCount());
-}
+}

native/android/surface_control.cpp

@@ -146,28 +146,24 @@ struct ASurfaceControlStats {
     uint64_t frameNumber;
 };
 
-void ASurfaceControl_registerSurfaceStatsListener(ASurfaceControl* control, void* context,
-        ASurfaceControl_SurfaceStatsListener func) {
-    SurfaceStatsCallback callback = [func](void* callback_context,
-                                                               nsecs_t,
-                                                               const sp<Fence>&,
-                                                               const SurfaceStats& surfaceStats) {
-
+void ASurfaceControl_registerSurfaceStatsListener(ASurfaceControl* control, int32_t id,
+                                                  void* context,
+                                                  ASurfaceControl_SurfaceStatsListener func) {
+    SurfaceStatsCallback callback = [func, id](void* callback_context, nsecs_t, const sp<Fence>&,
+                                               const SurfaceStats& surfaceStats) {
         ASurfaceControlStats aSurfaceControlStats;
 
-        ASurfaceControl* aSurfaceControl =
-                reinterpret_cast<ASurfaceControl*>(surfaceStats.surfaceControl.get());
         aSurfaceControlStats.acquireTime = surfaceStats.acquireTime;
         aSurfaceControlStats.previousReleaseFence = surfaceStats.previousReleaseFence;
         aSurfaceControlStats.frameNumber = surfaceStats.eventStats.frameNumber;
 
-        (*func)(callback_context, aSurfaceControl, &aSurfaceControlStats);
+        (*func)(callback_context, id, &aSurfaceControlStats);
     };
+
     TransactionCompletedListener::getInstance()->addSurfaceStatsListener(context,
             reinterpret_cast<void*>(func), ASurfaceControl_to_SurfaceControl(control), callback);
 }
 
-
 void ASurfaceControl_unregisterSurfaceStatsListener(void* context,
         ASurfaceControl_SurfaceStatsListener func) {
     TransactionCompletedListener::getInstance()->removeSurfaceStatsListener(context,
@@ -662,4 +658,4 @@ void ASurfaceTransaction_setOnCommit(ASurfaceTransaction* aSurfaceTransaction, v
     Transaction* transaction = ASurfaceTransaction_to_Transaction(aSurfaceTransaction);
 
     transaction->addTransactionCommittedCallback(callback, context);
-}
+}