commit f34a813b3241efe96cfd1ac228ef29f406845062
author Ady Abraham <adyabr@google.com> Mon Feb 13 20:49:48 2023 -0800
committer Ady Abraham <adyabr@google.com> Wed Feb 15 19:36:43 2023 +0000
tree 7d26cbf6b9f9afe18e20b888c4937a1f2715d1df
parent c6c46b885a2071540f88dbcc363f67a4ae1dd02a

SF: vsync divisors should use the same reference point

Fix an issue where vsync divisors use different reference points
to classify whether a vsync is in phase or not. This caused an
issue where 30fps and 60fps might picked a difference reference point,
causing a 30fps vsync to be out of phase with a 60fps vsync.

Test: SF unit tests
Bug: 267780202
Change-Id: I95939670f2850e90978a816d319a59f7c6fa7ba5

services/surfaceflinger/Scheduler/VSyncPredictor.cpp

diff --git a/services/surfaceflinger/Scheduler/VSyncPredictor.cpp b/services/surfaceflinger/Scheduler/VSyncPredictor.cpp
index f8cb323..5a5afd8 100644
--- a/services/surfaceflinger/Scheduler/VSyncPredictor.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncPredictor.cpp
@@ -219,6 +219,17 @@
     return true;
 }
 
+auto VSyncPredictor::getVsyncSequenceLocked(nsecs_t timestamp) const -> VsyncSequence {
+    const auto vsync = nextAnticipatedVSyncTimeFromLocked(timestamp);
+    if (!mLastVsyncSequence) return {vsync, 0};
+
+    const auto [slope, _] = getVSyncPredictionModelLocked();
+    const auto [lastVsyncTime, lastVsyncSequence] = *mLastVsyncSequence;
+    const auto vsyncSequence = lastVsyncSequence +
+            static_cast<int64_t>(std::round((vsync - lastVsyncTime) / static_cast<float>(slope)));
+    return {vsync, vsyncSequence};
+}
+
 nsecs_t VSyncPredictor::nextAnticipatedVSyncTimeFromLocked(nsecs_t timePoint) const {
     auto const [slope, intercept] = getVSyncPredictionModelLocked();
 
@@ -258,12 +269,17 @@
 nsecs_t VSyncPredictor::nextAnticipatedVSyncTimeFrom(nsecs_t timePoint) const {
     std::lock_guard lock(mMutex);
 
-    // TODO(b/246164114): This implementation is not efficient at all. Refactor.
-    nsecs_t nextVsync = nextAnticipatedVSyncTimeFromLocked(timePoint);
-    while (!isVSyncInPhaseLocked(nextVsync, mDivisor)) {
-        nextVsync = nextAnticipatedVSyncTimeFromLocked(nextVsync + 1);
+    // update the mLastVsyncSequence for reference point
+    mLastVsyncSequence = getVsyncSequenceLocked(timePoint);
+
+    const auto mod = mLastVsyncSequence->seq % mDivisor;
+    if (mod == 0) {
+        return mLastVsyncSequence->vsyncTime;
     }
-    return nextVsync;
+
+    auto const [slope, intercept] = getVSyncPredictionModelLocked();
+    const auto approximateNextVsync = mLastVsyncSequence->vsyncTime + slope * (mDivisor - mod);
+    return nextAnticipatedVSyncTimeFromLocked(approximateNextVsync - slope / 2);
 }
 
 /*
@@ -289,51 +305,16 @@
     ATRACE_FORMAT("%s timePoint in: %.2f divisor: %zu", __func__, getTimePointIn(now, timePoint),
                   divisor);
 
-    struct VsyncError {
-        nsecs_t vsyncTimestamp;
-        float error;
-
-        bool operator<(const VsyncError& other) const { return error < other.error; }
-    };
-
     if (divisor <= 1 || timePoint == 0) {
         return true;
     }
 
     const nsecs_t period = mRateMap[mIdealPeriod].slope;
     const nsecs_t justBeforeTimePoint = timePoint - period / 2;
-    const nsecs_t dividedPeriod = mIdealPeriod / divisor;
-
-    // If this is the first time we have asked about this divisor with the
-    // current vsync period, it is considered in phase and we store the closest
-    // vsync timestamp
-    const auto knownTimestampIter = mRateDivisorKnownTimestampMap.find(dividedPeriod);
-    if (knownTimestampIter == mRateDivisorKnownTimestampMap.end()) {
-        const auto vsync = nextAnticipatedVSyncTimeFromLocked(justBeforeTimePoint);
-        mRateDivisorKnownTimestampMap[dividedPeriod] = vsync;
-        ATRACE_FORMAT_INSTANT("(first) knownVsync in: %.2f", getTimePointIn(now, vsync));
-        return true;
-    }
-
-    // Find the next N vsync timestamp where N is the divisor.
-    // One of these vsyncs will be in phase. We return the one which is
-    // the most aligned with the last known in phase vsync
-    std::vector<VsyncError> vsyncs(static_cast<size_t>(divisor));
-    const nsecs_t knownVsync = knownTimestampIter->second;
-    nsecs_t point = justBeforeTimePoint;
-    for (size_t i = 0; i < divisor; i++) {
-        const nsecs_t vsync = nextAnticipatedVSyncTimeFromLocked(point);
-        const auto numPeriods = static_cast<float>(vsync - knownVsync) / (period * divisor);
-        const auto error = std::abs(std::round(numPeriods) - numPeriods);
-        vsyncs[i] = {vsync, error};
-        point = vsync + 1;
-    }
-
-    const auto minVsyncError = std::min_element(vsyncs.begin(), vsyncs.end());
-    mRateDivisorKnownTimestampMap[dividedPeriod] = minVsyncError->vsyncTimestamp;
-    ATRACE_FORMAT_INSTANT("knownVsync in: %.2f",
-                          getTimePointIn(now, minVsyncError->vsyncTimestamp));
-    return std::abs(minVsyncError->vsyncTimestamp - timePoint) < period / 2;
+    const auto vsyncSequence = getVsyncSequenceLocked(justBeforeTimePoint);
+    ATRACE_FORMAT_INSTANT("vsync in: %.2f sequence: %" PRId64,
+                          getTimePointIn(now, vsyncSequence.vsyncTime), vsyncSequence.seq);
+    return vsyncSequence.seq % divisor == 0;
 }
 
 void VSyncPredictor::setDivisor(unsigned divisor) {
@@ -382,6 +363,7 @@
         mTimestamps.clear();
         mLastTimestampIndex = 0;
     }
+    mLastVsyncSequence.reset();
 }
 
 bool VSyncPredictor::needsMoreSamples() const {