From 88660d77daced91f8ecb80a2c295330d72210660 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Mon, 9 Aug 2021 14:37:56 +0000 Subject: [PATCH 1/7] Stop reporting frame stats from frames completed before observer was attached Test: Run app from bug report Fixes: 195699687 Change-Id: If80825dfb41467917b7b9b1e8c9ead1a0dcbffae --- libs/hwui/Android.bp | 1 + libs/hwui/FrameMetricsObserver.h | 23 ++++++++ libs/hwui/FrameMetricsReporter.cpp | 56 ++++++++++++++++++ libs/hwui/FrameMetricsReporter.h | 23 +++----- libs/hwui/JankTracker.cpp | 6 +- libs/hwui/JankTracker.h | 3 +- libs/hwui/renderthread/CanvasContext.cpp | 74 +++++++++++++++++------- libs/hwui/renderthread/CanvasContext.h | 41 ++++++------- native/android/surface_control.cpp | 2 +- 9 files changed, 165 insertions(+), 64 deletions(-) create mode 100644 libs/hwui/FrameMetricsReporter.cpp diff --git a/libs/hwui/Android.bp b/libs/hwui/Android.bp index 2c299fa323152..8d0fcd5c90362 100644 --- a/libs/hwui/Android.bp +++ b/libs/hwui/Android.bp @@ -587,6 +587,7 @@ cc_defaults { "HardwareBitmapUploader.cpp", "HWUIProperties.sysprop", "JankTracker.cpp", + "FrameMetricsReporter.cpp", "Layer.cpp", "LayerUpdateQueue.cpp", "ProfileData.cpp", diff --git a/libs/hwui/FrameMetricsObserver.h b/libs/hwui/FrameMetricsObserver.h index ef1f5aabcbd8b..2ae790106faee 100644 --- a/libs/hwui/FrameMetricsObserver.h +++ b/libs/hwui/FrameMetricsObserver.h @@ -26,6 +26,13 @@ public: virtual void notify(const int64_t* buffer) = 0; bool waitForPresentTime() const { return mWaitForPresentTime; }; + void reportMetricsFrom(int64_t frameNumber, int32_t surfaceControlId) { + mAttachedFrameNumber = frameNumber; + mSurfaceControlId = surfaceControlId; + }; + int64_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. + int64_t mAttachedFrameNumber; }; } // namespace uirenderer diff --git a/libs/hwui/FrameMetricsReporter.cpp b/libs/hwui/FrameMetricsReporter.cpp new file mode 100644 index 0000000000000..a5b1897e8cd1e --- /dev/null +++ b/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, + int64_t frameNumber, int32_t surfaceControlId) { + FatVector, 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 diff --git a/libs/hwui/FrameMetricsReporter.h b/libs/hwui/FrameMetricsReporter.h index 0ac025fb01db8..95ca77e487f37 100644 --- a/libs/hwui/FrameMetricsReporter.h +++ b/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, 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, int64_t frameNumber, + int32_t surfaceControlId); private: FatVector, 10> mObservers GUARDED_BY(mObserversLock); diff --git a/libs/hwui/JankTracker.cpp b/libs/hwui/JankTracker.cpp index 34e5577066f9f..3e5cbb5fd7582 100644 --- a/libs/hwui/JankTracker.cpp +++ b/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& reporter) { +void JankTracker::finishFrame(FrameInfo& frame, std::unique_ptr& 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_ptrreportFrameMetrics(frame.data(), false /* hasPresentTime */); + reporter->reportFrameMetrics(frame.data(), false /* hasPresentTime */, frameNumber, + surfaceControlId); } } diff --git a/libs/hwui/JankTracker.h b/libs/hwui/JankTracker.h index bdb784dc87470..bcd031efa78d7 100644 --- a/libs/hwui/JankTracker.h +++ b/libs/hwui/JankTracker.h @@ -57,7 +57,8 @@ public: } FrameInfo* startFrame() { return &mFrames.next(); } - void finishFrame(FrameInfo& frame, std::unique_ptr& reporter); + void finishFrame(FrameInfo& frame, std::unique_ptr& 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. diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index bb0b1352c360a..19a93f811011d 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/libs/hwui/renderthread/CanvasContext.cpp @@ -613,16 +613,18 @@ nsecs_t CanvasContext::draw() { if (requireSwap) { if (mExpectSurfaceStats) { reportMetricsWithPresentTime(); - std::lock_guard lock(mLast4FrameInfosMutex); - std::pair& next = mLast4FrameInfos.next(); - next.first = mCurrentFrameInfo; - next.second = frameCompleteNr; + std::lock_guard lock(mLast4FrameMetricsInfosMutex); + FrameMetricsInfo& next = mLast4FrameMetricsInfos.next(); + next.frameInfo = mCurrentFrameInfo; + next.frameNumber = frameCompleteNr; + next.surfaceId = mSurfaceControlGenerationId; } 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 +660,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 +686,50 @@ 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. + int64_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); } } - return nullptr; +} + +CanvasContext::FrameMetricsInfo CanvasContext::getFrameMetricsInfoFromLast4(uint64_t frameNumber) { + std::scoped_lock lock(mLast4FrameMetricsInfosMutex); + for (size_t i = 0; i < mLast4FrameMetricsInfos.size(); i++) { + if (mLast4FrameMetricsInfos[i].frameNumber == frameNumber) { + return mLast4FrameMetricsInfos[i]; + } + } + + return {}; } void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* control, ASurfaceControlStats* stats) { - - CanvasContext* instance = static_cast(context); + auto* instance = static_cast(context); const ASurfaceControlFunctions& functions = instance->mRenderThread.getASurfaceControlFunctions(); @@ -706,14 +737,17 @@ void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* cont nsecs_t gpuCompleteTime = functions.getAcquireTimeFunc(stats); uint64_t frameNumber = functions.getFrameNumberFunc(stats); - FrameInfo* frameInfo = instance->getFrameInfoFromLast4(frameNumber); + FrameMetricsInfo frameMetricsInfo = instance->getFrameMetricsInfoFromLast4(frameNumber); + FrameInfo* frameInfo = frameMetricsInfo.frameInfo; 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, + frameMetricsInfo.frameNumber, + frameMetricsInfo.surfaceId); } } diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index 2fed4686f16ec..7594cc01f9c2c 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -167,24 +167,8 @@ 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&& func); @@ -254,7 +238,13 @@ private: */ void reportMetricsWithPresentTime(); - FrameInfo* getFrameInfoFromLast4(uint64_t frameNumber); + struct FrameMetricsInfo { + FrameInfo* frameInfo; + int64_t frameNumber; + int32_t surfaceId; + }; + + CanvasContext::FrameMetricsInfo getFrameMetricsInfoFromLast4(uint64_t frameNumber); // The same type as Frame.mWidth and Frame.mHeight int32_t mLastFrameWidth = 0; @@ -266,7 +256,9 @@ 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. int32_t mSurfaceControlGenerationId = 0; // stopped indicates the CanvasContext will reject actual redraw operations, // and defer repaint until it is un-stopped @@ -308,10 +300,11 @@ private: FrameInfo* mCurrentFrameInfo = nullptr; - // List of frames that are awaiting GPU completion reporting - RingBuffer, 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 mLast4FrameMetricsInfos + GUARDED_BY(mLast4FrameMetricsInfosMutex); + std::mutex mLast4FrameMetricsInfosMutex; std::string mName; JankTracker mJankTracker; diff --git a/native/android/surface_control.cpp b/native/android/surface_control.cpp index 693a027bd0e2d..1f246e52d4599 100644 --- a/native/android/surface_control.cpp +++ b/native/android/surface_control.cpp @@ -662,4 +662,4 @@ void ASurfaceTransaction_setOnCommit(ASurfaceTransaction* aSurfaceTransaction, v Transaction* transaction = ASurfaceTransaction_to_Transaction(aSurfaceTransaction); transaction->addTransactionCommittedCallback(callback, context); -} \ No newline at end of file +} From bc9e5290313a24c7c77d0a2f18f82e29a84d05dc Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Mon, 23 Aug 2021 17:12:29 +0200 Subject: [PATCH 2/7] Pass surface control id to callback to accurately identify surface metrics belongs to Avoid getting the wrong frame info when duplicate frame numbers are found in the ring buffer. Will ensure there isn't a mismatch in the metrics data reported. Test: Existing tests Bug: 197515602 Change-Id: Iff9ba01f575f94e5a9872ee48c0dd1e5067880c3 --- libs/hwui/renderthread/CanvasContext.cpp | 36 +++++++++++++----------- libs/hwui/renderthread/CanvasContext.h | 5 ++-- libs/hwui/renderthread/RenderThread.h | 5 ++-- native/android/surface_control.cpp | 19 ++++++------- 4 files changed, 33 insertions(+), 32 deletions(-) diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index 19a93f811011d..e6d31d6cdf418 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/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); } } @@ -613,11 +614,13 @@ nsecs_t CanvasContext::draw() { if (requireSwap) { if (mExpectSurfaceStats) { reportMetricsWithPresentTime(); - std::lock_guard lock(mLast4FrameMetricsInfosMutex); - FrameMetricsInfo& next = mLast4FrameMetricsInfos.next(); - next.frameInfo = mCurrentFrameInfo; - next.frameNumber = frameCompleteNr; - next.surfaceId = mSurfaceControlGenerationId; + { // 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) @@ -716,19 +719,20 @@ void CanvasContext::removeFrameMetricsObserver(FrameMetricsObserver* observer) { } } -CanvasContext::FrameMetricsInfo CanvasContext::getFrameMetricsInfoFromLast4(uint64_t frameNumber) { +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) { - return mLast4FrameMetricsInfos[i]; + if (mLast4FrameMetricsInfos[i].frameNumber == frameNumber && + mLast4FrameMetricsInfos[i].surfaceId == surfaceControlId) { + return mLast4FrameMetricsInfos[i].frameInfo; } } - return {}; + return nullptr; } void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* control, - ASurfaceControlStats* stats) { + int32_t surfaceControlId, ASurfaceControlStats* stats) { auto* instance = static_cast(context); const ASurfaceControlFunctions& functions = @@ -737,17 +741,15 @@ void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* cont nsecs_t gpuCompleteTime = functions.getAcquireTimeFunc(stats); uint64_t frameNumber = functions.getFrameNumberFunc(stats); - FrameMetricsInfo frameMetricsInfo = instance->getFrameMetricsInfoFromLast4(frameNumber); + FrameInfo* frameInfo = instance->getFrameInfoFromLast4(frameNumber, surfaceControlId); - FrameInfo* frameInfo = frameMetricsInfo.frameInfo; 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, - frameMetricsInfo.frameNumber, - frameMetricsInfo.surfaceId); + instance->mJankTracker.finishFrame(*frameInfo, instance->mFrameMetricsReporter, frameNumber, + surfaceControlId); } } diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index 7594cc01f9c2c..b21dc75372e3a 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -197,7 +197,7 @@ public: // Called when SurfaceStats are available. static void onSurfaceStatsAvailable(void* context, ASurfaceControl* control, - ASurfaceControlStats* stats); + int32_t surfaceControlId, ASurfaceControlStats* stats); void setASurfaceTransactionCallback( const std::function& callback) { @@ -244,7 +244,7 @@ private: int32_t surfaceId; }; - CanvasContext::FrameMetricsInfo getFrameMetricsInfoFromLast4(uint64_t frameNumber); + FrameInfo* getFrameInfoFromLast4(uint64_t frameNumber, uint32_t surfaceControlId); // The same type as Frame.mWidth and Frame.mHeight int32_t mLastFrameWidth = 0; @@ -259,6 +259,7 @@ private: // 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 diff --git a/libs/hwui/renderthread/RenderThread.h b/libs/hwui/renderthread/RenderThread.h index 05d225b856dbf..0b81fc04edf56 100644 --- a/libs/hwui/renderthread/RenderThread.h +++ b/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); diff --git a/native/android/surface_control.cpp b/native/android/surface_control.cpp index 1f246e52d4599..31350ee9894fc 100644 --- a/native/android/surface_control.cpp +++ b/native/android/surface_control.cpp @@ -146,28 +146,25 @@ 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&, - const SurfaceStats& surfaceStats) { - +void ASurfaceControl_registerSurfaceStatsListener(ASurfaceControl* control, int32_t id, + void* context, + ASurfaceControl_SurfaceStatsListener func) { + SurfaceStatsCallback callback = [func, control, id](void* callback_context, nsecs_t, + const sp&, + const SurfaceStats& surfaceStats) { ASurfaceControlStats aSurfaceControlStats; - ASurfaceControl* aSurfaceControl = - reinterpret_cast(surfaceStats.surfaceControl.get()); aSurfaceControlStats.acquireTime = surfaceStats.acquireTime; aSurfaceControlStats.previousReleaseFence = surfaceStats.previousReleaseFence; aSurfaceControlStats.frameNumber = surfaceStats.eventStats.frameNumber; - (*func)(callback_context, aSurfaceControl, &aSurfaceControlStats); + (*func)(callback_context, control, id, &aSurfaceControlStats); }; + TransactionCompletedListener::getInstance()->addSurfaceStatsListener(context, reinterpret_cast(func), ASurfaceControl_to_SurfaceControl(control), callback); } - void ASurfaceControl_unregisterSurfaceStatsListener(void* context, ASurfaceControl_SurfaceStatsListener func) { TransactionCompletedListener::getInstance()->removeSurfaceStatsListener(context, From 0aa775e0858a148fe9c6706e3b990297097e54a6 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Thu, 26 Aug 2021 00:57:23 +0200 Subject: [PATCH 3/7] Update JankTrackerTests to reflect API changes Test: adb shell /data/nativetest/hwui_unit_tests/hwui_unit_tests --gtest_filter="JankTracker.*" Change-Id: I3de9e561540a19845a746932db6b67957886202d --- libs/hwui/tests/unit/JankTrackerTests.cpp | 35 ++++++++++++++++------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/libs/hwui/tests/unit/JankTrackerTests.cpp b/libs/hwui/tests/unit/JankTrackerTests.cpp index f467ebf5d8881..5b397de36a869 100644 --- a/libs/hwui/tests/unit/JankTrackerTests.cpp +++ b/libs/hwui/tests/unit/JankTrackerTests.cpp @@ -34,6 +34,9 @@ TEST(JankTracker, noJank) { JankTracker jankTracker(&container); std::unique_ptr reporter = std::make_unique(); + 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 reporter = std::make_unique(); + 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 reporter = std::make_unique(); + 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 reporter = std::make_unique(); + 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 reporter = std::make_unique(); + 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()); -} \ No newline at end of file +} From dfb85a2b828bd9c8a93c777113392de8890a43b8 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Thu, 26 Aug 2021 01:19:05 +0200 Subject: [PATCH 4/7] Add unit tests for FrameMetricsReporter Test: adb shell /data/nativetest/hwui_unit_tests/hwui_unit_tests --gtest_filter="FrameMetricsReporter.*" Change-Id: I97533bf71172cab29784d4177e53de50f285b991 --- libs/hwui/Android.bp | 1 + .../tests/unit/FrameMetricsReporterTests.cpp | 205 ++++++++++++++++++ 2 files changed, 206 insertions(+) create mode 100644 libs/hwui/tests/unit/FrameMetricsReporterTests.cpp diff --git a/libs/hwui/Android.bp b/libs/hwui/Android.bp index 8d0fcd5c90362..b931d030f2a02 100644 --- a/libs/hwui/Android.bp +++ b/libs/hwui/Android.bp @@ -692,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", diff --git a/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp b/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp new file mode 100644 index 0000000000000..6c1aee0cb6cbd --- /dev/null +++ b/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 +#include + +#include +#include +#include + +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(); + + auto observer = sp::make(false /*waitForPresentTime*/); + EXPECT_CALL(*observer, notify).Times(4); + + reporter->addObserver(observer.get()); + + const int64_t* stats; + bool hasPresentTime = false; + int64_t frameNumber = 0; + 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(); + + auto observer = sp::make(false /*waitForPresentTime*/); + reporter->addObserver(observer.get()); + + const int64_t* stats; + bool hasPresentTime = false; + int64_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(); + + auto observer = sp::make(true /*waitForPresentTime*/); + reporter->addObserver(observer.get()); + + const int64_t* stats; + bool hasPresentTime = false; + int64_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 frameNumbers{0, 1, 10}; + std::vector surfaceControlIds{0, 1, 10}; + for (uint64_t frameNumber : frameNumbers) { + for (int32_t surfaceControlId : surfaceControlIds) { + auto reporter = std::make_shared(); + + auto observer = + sp::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 frameNumbers{1, 10}; + std::vector surfaceControlIds{0, 1, 10}; + for (uint64_t frameNumber : frameNumbers) { + for (uint32_t surfaceControlId : surfaceControlIds) { + auto reporter = std::make_shared(); + + auto observer = + sp::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; + int64_t frameNumber = 3; + int32_t surfaceControlId = 0; + + auto reporter = std::make_shared(); + + auto observer = sp::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; + int64_t frameNumber = 3; + int32_t surfaceControlId = 0; + + auto reporter = std::make_shared(); + + auto observer1 = sp::make(hasPresentTime /*waitForPresentTime*/); + auto observer2 = sp::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); +} From 35b80cde9225952bc9ec6afccffd480e77d5ad2e Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Tue, 24 Aug 2021 11:03:51 +0200 Subject: [PATCH 5/7] Make frame number type consistent The frame numbers are ultimately stored as uint64_t so to avoid any implementation dependent casting we make sure all direct and indirect references to this value are stored as uint64_t Test: N/A Change-Id: I080c94b1007f2e884cf0fa3583bf51badc913a05 --- libs/hwui/FrameMetricsObserver.h | 6 +++--- libs/hwui/FrameMetricsReporter.cpp | 2 +- libs/hwui/FrameMetricsReporter.h | 2 +- libs/hwui/renderthread/CanvasContext.cpp | 6 +++--- libs/hwui/renderthread/CanvasContext.h | 4 ++-- libs/hwui/tests/unit/FrameMetricsReporterTests.cpp | 14 +++++++------- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/libs/hwui/FrameMetricsObserver.h b/libs/hwui/FrameMetricsObserver.h index 2ae790106faee..498ec5793caba 100644 --- a/libs/hwui/FrameMetricsObserver.h +++ b/libs/hwui/FrameMetricsObserver.h @@ -26,11 +26,11 @@ public: virtual void notify(const int64_t* buffer) = 0; bool waitForPresentTime() const { return mWaitForPresentTime; }; - void reportMetricsFrom(int64_t frameNumber, int32_t surfaceControlId) { + void reportMetricsFrom(uint64_t frameNumber, int32_t surfaceControlId) { mAttachedFrameNumber = frameNumber; mSurfaceControlId = surfaceControlId; }; - int64_t attachedFrameNumber() const { return mAttachedFrameNumber; }; + uint64_t attachedFrameNumber() const { return mAttachedFrameNumber; }; int32_t attachedSurfaceControlId() const { return mSurfaceControlId; }; /** @@ -64,7 +64,7 @@ private: // 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. - int64_t mAttachedFrameNumber; + uint64_t mAttachedFrameNumber; }; } // namespace uirenderer diff --git a/libs/hwui/FrameMetricsReporter.cpp b/libs/hwui/FrameMetricsReporter.cpp index a5b1897e8cd1e..ee32ea17bfaf2 100644 --- a/libs/hwui/FrameMetricsReporter.cpp +++ b/libs/hwui/FrameMetricsReporter.cpp @@ -20,7 +20,7 @@ namespace android { namespace uirenderer { void FrameMetricsReporter::reportFrameMetrics(const int64_t* stats, bool hasPresentTime, - int64_t frameNumber, int32_t surfaceControlId) { + uint64_t frameNumber, int32_t surfaceControlId) { FatVector, 10> copy; { std::lock_guard lock(mObserversLock); diff --git a/libs/hwui/FrameMetricsReporter.h b/libs/hwui/FrameMetricsReporter.h index 95ca77e487f37..7e51df7ce6fcc 100644 --- a/libs/hwui/FrameMetricsReporter.h +++ b/libs/hwui/FrameMetricsReporter.h @@ -69,7 +69,7 @@ public: * 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, int64_t frameNumber, + void reportFrameMetrics(const int64_t* stats, bool hasPresentTime, uint64_t frameNumber, int32_t surfaceControlId); private: diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index e6d31d6cdf418..c5ae0431e06bf 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/libs/hwui/renderthread/CanvasContext.cpp @@ -513,7 +513,7 @@ nsecs_t CanvasContext::draw() { mContentDrawBounds, mOpaque, mLightInfo, mRenderNodes, &(profiler())); - int64_t frameCompleteNr = getFrameNumber(); + uint64_t frameCompleteNr = getFrameNumber(); waitOnFences(); @@ -704,7 +704,7 @@ void CanvasContext::addFrameMetricsObserver(FrameMetricsObserver* observer) { // 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. - int64_t nextFrameNumber = getFrameNumber(); + uint64_t nextFrameNumber = getFrameNumber(); observer->reportMetricsFrom(nextFrameNumber, mSurfaceControlGenerationId); mFrameMetricsReporter->addObserver(observer); } @@ -890,7 +890,7 @@ void CanvasContext::enqueueFrameWork(std::function&& func) { mFrameFences.push_back(CommonPool::async(std::move(func))); } -int64_t CanvasContext::getFrameNumber() { +uint64_t CanvasContext::getFrameNumber() { // mFrameNumber is reset to -1 when the surface changes or we swap buffers if (mFrameNumber == -1 && mNativeSurface.get()) { mFrameNumber = ANativeWindow_getNextFrameId(mNativeSurface->getNativeWindow()); diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index b21dc75372e3a..852cbda6c3135 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -173,7 +173,7 @@ public: // Used to queue up work that needs to be completed before this frame completes void enqueueFrameWork(std::function&& func); - int64_t getFrameNumber(); + uint64_t getFrameNumber(); void waitOnFences(); @@ -281,7 +281,7 @@ private: // Need at least 4 because we do quad buffer. Add a 5th for good measure. RingBuffer mSwapHistory; - int64_t mFrameNumber = -1; + uint64_t mFrameNumber = -1; int64_t mDamageId = 0; // last vsync for a dropped frame due to stuffed queue diff --git a/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp b/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp index 6c1aee0cb6cbd..fb04700bbf703 100644 --- a/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp +++ b/libs/hwui/tests/unit/FrameMetricsReporterTests.cpp @@ -44,7 +44,7 @@ TEST(FrameMetricsReporter, reportsAllFramesIfNoFromFrameIsSpecified) { const int64_t* stats; bool hasPresentTime = false; - int64_t frameNumber = 0; + uint64_t frameNumber = 1; int32_t surfaceControlId = 0; reporter->reportFrameMetrics(stats, hasPresentTime, frameNumber, surfaceControlId); @@ -69,7 +69,7 @@ TEST(FrameMetricsReporter, respectsWaitForPresentTimeUnset) { const int64_t* stats; bool hasPresentTime = false; - int64_t frameNumber = 3; + uint64_t frameNumber = 3; int32_t surfaceControlId = 0; EXPECT_CALL(*observer, notify).Times(1); @@ -88,7 +88,7 @@ TEST(FrameMetricsReporter, respectsWaitForPresentTimeSet) { const int64_t* stats; bool hasPresentTime = false; - int64_t frameNumber = 3; + uint64_t frameNumber = 3; int32_t surfaceControlId = 0; EXPECT_CALL(*observer, notify).Times(0); @@ -136,9 +136,9 @@ TEST(FrameMetricsReporter, doesNotReportsFramesBeforeSpecifiedFromFrame) { bool hasPresentTime = false; std::vector frameNumbers{1, 10}; - std::vector surfaceControlIds{0, 1, 10}; + std::vector surfaceControlIds{0, 1, 10}; for (uint64_t frameNumber : frameNumbers) { - for (uint32_t surfaceControlId : surfaceControlIds) { + for (int32_t surfaceControlId : surfaceControlIds) { auto reporter = std::make_shared(); auto observer = @@ -161,7 +161,7 @@ TEST(FrameMetricsReporter, doesNotReportsFramesBeforeSpecifiedFromFrame) { TEST(FrameMetricsReporter, canRemoveObservers) { const int64_t* stats; bool hasPresentTime = false; - int64_t frameNumber = 3; + uint64_t frameNumber = 3; int32_t surfaceControlId = 0; auto reporter = std::make_shared(); @@ -183,7 +183,7 @@ TEST(FrameMetricsReporter, canRemoveObservers) { TEST(FrameMetricsReporter, canSupportMultipleObservers) { const int64_t* stats; bool hasPresentTime = false; - int64_t frameNumber = 3; + uint64_t frameNumber = 3; int32_t surfaceControlId = 0; auto reporter = std::make_shared(); From 7e6a5f29f59e732f3172439ae8417970fe229d42 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Tue, 31 Aug 2021 13:49:19 +0000 Subject: [PATCH 6/7] Set uninitialized frame number to 0 Now that the frame number is a uint instead of an int it's clearer if the uninitailized value is 0. Frame numbers start from 1 so 0 doesn't collide with any valid frame numbers. It also means comparing frame numbers to check if the frame is newer always works even when comparing to the uninitialized frame number. Test: N/A Change-Id: I0901ffeaefbdd6378e59d8858a06de92b0104cd9 --- libs/hwui/renderthread/CanvasContext.cpp | 8 ++++---- libs/hwui/renderthread/CanvasContext.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index c5ae0431e06bf..aa39d55adc36f 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/libs/hwui/renderthread/CanvasContext.cpp @@ -219,7 +219,7 @@ void CanvasContext::setupPipelineSurface() { } - mFrameNumber = -1; + mFrameNumber = 0; if (mNativeSurface != nullptr && hasSurface) { mHaveNewSurface = true; @@ -581,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; @@ -891,8 +891,8 @@ void CanvasContext::enqueueFrameWork(std::function&& func) { } uint64_t CanvasContext::getFrameNumber() { - // mFrameNumber is reset to -1 when the surface changes or we swap buffers - if (mFrameNumber == -1 && mNativeSurface.get()) { + // 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; diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index 852cbda6c3135..ec91e662fcd3c 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -281,7 +281,8 @@ private: // Need at least 4 because we do quad buffer. Add a 5th for good measure. RingBuffer mSwapHistory; - uint64_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 From 14b28ce9cec2c88fededf0f20aff0f8ed2c8a9e8 Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Mon, 6 Sep 2021 16:33:23 +0000 Subject: [PATCH 7/7] Get rid of unused ASurfaceControl in frame metrics listener callback Test: Existing tests Change-Id: Ia071814dabfc762d00d3324bfaa343ff94ab8338 --- libs/hwui/renderthread/CanvasContext.cpp | 4 ++-- libs/hwui/renderthread/CanvasContext.h | 4 ++-- native/android/surface_control.cpp | 7 +++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index aa39d55adc36f..b11e722ea2e38 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/libs/hwui/renderthread/CanvasContext.cpp @@ -731,8 +731,8 @@ FrameInfo* CanvasContext::getFrameInfoFromLast4(uint64_t frameNumber, uint32_t s return nullptr; } -void CanvasContext::onSurfaceStatsAvailable(void* context, ASurfaceControl* control, - int32_t surfaceControlId, ASurfaceControlStats* stats) { +void CanvasContext::onSurfaceStatsAvailable(void* context, int32_t surfaceControlId, + ASurfaceControlStats* stats) { auto* instance = static_cast(context); const ASurfaceControlFunctions& functions = diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index ec91e662fcd3c..8d9750dd2ca62 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -196,8 +196,8 @@ public: SkISize getNextFrameSize() const; // Called when SurfaceStats are available. - static void onSurfaceStatsAvailable(void* context, ASurfaceControl* control, - int32_t surfaceControlId, ASurfaceControlStats* stats); + static void onSurfaceStatsAvailable(void* context, int32_t surfaceControlId, + ASurfaceControlStats* stats); void setASurfaceTransactionCallback( const std::function& callback) { diff --git a/native/android/surface_control.cpp b/native/android/surface_control.cpp index 31350ee9894fc..7f74dd4c33c31 100644 --- a/native/android/surface_control.cpp +++ b/native/android/surface_control.cpp @@ -149,16 +149,15 @@ struct ASurfaceControlStats { void ASurfaceControl_registerSurfaceStatsListener(ASurfaceControl* control, int32_t id, void* context, ASurfaceControl_SurfaceStatsListener func) { - SurfaceStatsCallback callback = [func, control, id](void* callback_context, nsecs_t, - const sp&, - const SurfaceStats& surfaceStats) { + SurfaceStatsCallback callback = [func, id](void* callback_context, nsecs_t, const sp&, + const SurfaceStats& surfaceStats) { ASurfaceControlStats aSurfaceControlStats; aSurfaceControlStats.acquireTime = surfaceStats.acquireTime; aSurfaceControlStats.previousReleaseFence = surfaceStats.previousReleaseFence; aSurfaceControlStats.frameNumber = surfaceStats.eventStats.frameNumber; - (*func)(callback_context, control, id, &aSurfaceControlStats); + (*func)(callback_context, id, &aSurfaceControlStats); }; TransactionCompletedListener::getInstance()->addSurfaceStatsListener(context,