From 1c1d4c71c2178b888f07fb806c89fef2bcf3a41c Mon Sep 17 00:00:00 2001 From: Snild Dolkow Date: Fri, 22 Jan 2021 14:42:08 +0100 Subject: Ensure that expected present time is in the future onMessageInvalidate() sets mExpectedPresentTime to a value originating from the vsync event that scheduled it. If handling of the invalidate is delayed, this value will be in the past, which is clearly not a plausible present time. Because of this, queued frames may incorrectly be ignored as "too early". This has caused problems in screen-on animations: even though the first frame is drawn and ready, SurfaceFlinger shows an old frame. This looks particularly bad with a fade-from-black animation: the old frame flashes by before the first frame of the animation shows. Here's an example timeline of how this problem could manifest: 1000 ms INVALIDATE queued with expectedVSyncTimestamp = 1010ms 1013 ms SF calls setPowerMode() 1014 ms SystemUI queues new NotificationShade frame 1255 ms setPowerMode() returns; invalidate can now be handled 1256 ms invalidate runs; mExpectedPresentTime set to 1010ms 1257 ms handlePageFlip() skips NotificationShade frame; "too early" 1259 ms refresh runs, composits prehistoric NotificationShade frame It's a bit of a race: if SystemUI manages to queue its new frame before the INVALIDATE message's timestamp, there won't be a problem. To solve this, let's check that the expected present time is in the future, and pick the nearest future vsync point if it's not. In order to not break frame miss detection, mScheduledPresentTime is introduced and used instead of mExpectedPresentTime for jank calculations. Fixes: 178415552 Test: manual on Xperia device; flash of old frame is gone Test: compiles on aosp/redfin Change-Id: I095f1dd08374fd1d14552cd1af90d95e9718b4dd Merged-In: I095f1dd08374fd1d14552cd1af90d95e9718b4dd --- services/surfaceflinger/SurfaceFlinger.cpp | 12 +++++++++--- services/surfaceflinger/SurfaceFlinger.h | 1 + 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 261722da2b..b15663ed11 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1725,8 +1725,14 @@ void SurfaceFlinger::onMessageInvalidate(nsecs_t expectedVSyncTime) { // calculate the expected present time once and use the cached // value throughout this frame to make sure all layers are // seeing this same value. - const nsecs_t lastExpectedPresentTime = mExpectedPresentTime.load(); - mExpectedPresentTime = expectedVSyncTime; + if (expectedVSyncTime >= frameStart) { + mExpectedPresentTime = expectedVSyncTime; + } else { + mExpectedPresentTime = mScheduler->getDispSyncExpectedPresentTime(frameStart); + } + + const nsecs_t lastScheduledPresentTime = mScheduledPresentTime; + mScheduledPresentTime = expectedVSyncTime; // When Backpressure propagation is enabled we want to give a small grace period // for the present fence to fire instead of just giving up on this frame to handle cases @@ -1756,7 +1762,7 @@ void SurfaceFlinger::onMessageInvalidate(nsecs_t expectedVSyncTime) { const TracedOrdinal frameMissed = {"PrevFrameMissed", framePending || (previousPresentTime >= 0 && - (lastExpectedPresentTime < + (lastScheduledPresentTime < previousPresentTime - frameMissedSlop))}; const TracedOrdinal hwcFrameMissed = {"PrevHwcFrameMissed", mHadDeviceComposition && frameMissed}; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 61bd020703..90ac856277 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1199,6 +1199,7 @@ private: std::unique_ptr mRefreshRateStats; std::atomic mExpectedPresentTime = 0; + nsecs_t mScheduledPresentTime = 0; hal::Vsync mHWCVsyncPendingState = hal::Vsync::DISABLE; /* ------------------------------------------------------------------------ -- cgit v1.2.3-59-g8ed1b