From bc9e5290313a24c7c77d0a2f18f82e29a84d05dc Mon Sep 17 00:00:00 2001 From: Pablo Gamito Date: Mon, 23 Aug 2021 17:12:29 +0200 Subject: [PATCH] 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,