SF: VSyncReactor: correct distribute timestamp
Timestamp being distributed via the choreographer callback was the
anticipated vsync timestamp. This might make sense in the future, but
the correct legacy behavior was to distribute the event timestamp.
(code with problem was flagged off)
Test: correction of 2 unit tests
Test: boot to home
Test: uibench scroll, jitter not observed.
Fixes: 147487378
Change-Id: I142a6eaf849479dfe5b754db138f55d9e5870afd
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatch.h b/services/surfaceflinger/Scheduler/VSyncDispatch.h
index 56b3252..a6fb3a4 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatch.h
+++ b/services/surfaceflinger/Scheduler/VSyncDispatch.h
@@ -39,6 +39,14 @@
virtual ~VSyncDispatch();
/*
+ * A callback that can be registered to be awoken at a given time relative to a vsync event.
+ * \param [in] vsyncTime The timestamp of the vsync the callback is for.
+ * \param [in] targetWakeupTime The timestamp of intended wakeup time of the cb.
+ *
+ */
+ using Callback = std::function<void(nsecs_t vsyncTime, nsecs_t targetWakeupTime)>;
+
+ /*
* Registers a callback that will be called at designated points on the vsync timeline.
* The callback can be scheduled, rescheduled targeting vsync times, or cancelled.
* The token returned must be cleaned up via unregisterCallback.
@@ -51,7 +59,7 @@
* invocation of callbackFn.
*
*/
- virtual CallbackToken registerCallback(std::function<void(nsecs_t)> const& callbackFn,
+ virtual CallbackToken registerCallback(Callback const& callbackFn,
std::string callbackName) = 0;
/*
@@ -112,7 +120,7 @@
*/
class VSyncCallbackRegistration {
public:
- VSyncCallbackRegistration(VSyncDispatch&, std::function<void(nsecs_t)> const& callbackFn,
+ VSyncCallbackRegistration(VSyncDispatch&, VSyncDispatch::Callback const& callbackFn,
std::string const& callbackName);
VSyncCallbackRegistration(VSyncCallbackRegistration&&);
VSyncCallbackRegistration& operator=(VSyncCallbackRegistration&&);
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
index 48f2abb..2e5b6e9 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
@@ -29,7 +29,7 @@
TimeKeeper::~TimeKeeper() = default;
VSyncDispatchTimerQueueEntry::VSyncDispatchTimerQueueEntry(std::string const& name,
- std::function<void(nsecs_t)> const& cb,
+ VSyncDispatch::Callback const& cb,
nsecs_t minVsyncDistance)
: mName(name),
mCallback(cb),
@@ -97,13 +97,13 @@
return *mLastDispatchTime;
}
-void VSyncDispatchTimerQueueEntry::callback(nsecs_t t) {
+void VSyncDispatchTimerQueueEntry::callback(nsecs_t vsyncTimestamp, nsecs_t wakeupTimestamp) {
{
std::lock_guard<std::mutex> lk(mRunningMutex);
mRunning = true;
}
- mCallback(t);
+ mCallback(vsyncTimestamp, wakeupTimestamp);
std::lock_guard<std::mutex> lk(mRunningMutex);
mRunning = false;
@@ -171,7 +171,8 @@
void VSyncDispatchTimerQueue::timerCallback() {
struct Invocation {
std::shared_ptr<VSyncDispatchTimerQueueEntry> callback;
- nsecs_t timestamp;
+ nsecs_t vsyncTimestamp;
+ nsecs_t wakeupTimestamp;
};
std::vector<Invocation> invocations;
{
@@ -186,7 +187,7 @@
if (*wakeupTime < mIntendedWakeupTime + mTimerSlack) {
callback->executing();
invocations.emplace_back(
- Invocation{callback, *callback->lastExecutedVsyncTarget()});
+ Invocation{callback, *callback->lastExecutedVsyncTarget(), *wakeupTime});
}
}
@@ -195,12 +196,12 @@
}
for (auto const& invocation : invocations) {
- invocation.callback->callback(invocation.timestamp);
+ invocation.callback->callback(invocation.vsyncTimestamp, invocation.wakeupTimestamp);
}
}
VSyncDispatchTimerQueue::CallbackToken VSyncDispatchTimerQueue::registerCallback(
- std::function<void(nsecs_t)> const& callbackFn, std::string callbackName) {
+ Callback const& callbackFn, std::string callbackName) {
std::lock_guard<decltype(mMutex)> lk(mMutex);
return CallbackToken{
mCallbacks
@@ -271,7 +272,7 @@
}
VSyncCallbackRegistration::VSyncCallbackRegistration(VSyncDispatch& dispatch,
- std::function<void(nsecs_t)> const& callbackFn,
+ VSyncDispatch::Callback const& callbackFn,
std::string const& callbackName)
: mDispatch(dispatch),
mToken(dispatch.registerCallback(callbackFn, callbackName)),
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
index 0c9b4fe..087acc7 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
@@ -36,7 +36,7 @@
// Valid transition: disarmed -> armed ( when scheduled )
// Valid transition: armed -> running -> disarmed ( when timer is called)
// Valid transition: armed -> disarmed ( when cancelled )
- VSyncDispatchTimerQueueEntry(std::string const& name, std::function<void(nsecs_t)> const& fn,
+ VSyncDispatchTimerQueueEntry(std::string const& name, VSyncDispatch::Callback const& fn,
nsecs_t minVsyncDistance);
std::string_view name() const;
@@ -62,14 +62,14 @@
nsecs_t executing();
// End: functions that are not threadsafe.
- // Invoke the callback with the timestamp, moving the state from running->disarmed.
- void callback(nsecs_t timestamp);
+ // Invoke the callback with the two given timestamps, moving the state from running->disarmed.
+ void callback(nsecs_t vsyncTimestamp, nsecs_t wakeupTimestamp);
// Block calling thread while the callback is executing.
void ensureNotRunning();
private:
std::string const mName;
- std::function<void(nsecs_t)> const mCallback;
+ VSyncDispatch::Callback const mCallback;
nsecs_t mWorkDuration;
nsecs_t mEarliestVsync;
@@ -104,8 +104,7 @@
nsecs_t timerSlack, nsecs_t minVsyncDistance);
~VSyncDispatchTimerQueue();
- CallbackToken registerCallback(std::function<void(nsecs_t)> const& callbackFn,
- std::string callbackName) final;
+ CallbackToken registerCallback(Callback const& callbackFn, std::string callbackName) final;
void unregisterCallback(CallbackToken token) final;
ScheduleResult schedule(CallbackToken token, nsecs_t workDuration, nsecs_t earliestVsync) final;
CancelResult cancel(CallbackToken token) final;
diff --git a/services/surfaceflinger/Scheduler/VSyncReactor.cpp b/services/surfaceflinger/Scheduler/VSyncReactor.cpp
index a652053..6c0a64e 100644
--- a/services/surfaceflinger/Scheduler/VSyncReactor.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncReactor.cpp
@@ -48,7 +48,8 @@
nsecs_t period, nsecs_t offset, nsecs_t notBefore)
: mCallback(cb),
mRegistration(dispatch,
- std::bind(&CallbackRepeater::callback, this, std::placeholders::_1),
+ std::bind(&CallbackRepeater::callback, this, std::placeholders::_1,
+ std::placeholders::_2),
std::string(name)),
mPeriod(period),
mOffset(offset),
@@ -85,15 +86,13 @@
}
private:
- void callback(nsecs_t vsynctime) {
- nsecs_t period = 0;
+ void callback(nsecs_t vsynctime, nsecs_t wakeupTime) {
{
std::lock_guard<std::mutex> lk(mMutex);
- period = mPeriod;
mLastCallTime = vsynctime;
}
- mCallback->onDispSyncEvent(vsynctime - period);
+ mCallback->onDispSyncEvent(wakeupTime);
{
std::lock_guard<std::mutex> lk(mMutex);
diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp
index 5846c77..b51a025 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp
@@ -101,7 +101,7 @@
RepeatingCallbackReceiver(VSyncDispatch& dispatch, nsecs_t wl)
: mWorkload(wl),
mCallback(
- dispatch, [&](auto time) { callback_called(time); }, "repeat0") {}
+ dispatch, [&](auto time, auto) { callback_called(time); }, "repeat0") {}
void repeatedly_schedule(size_t iterations, std::function<void(nsecs_t)> const& onEachFrame) {
mCallbackTimes.reserve(iterations);
diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
index 5aff429..6fcb1af 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
@@ -97,13 +97,14 @@
CountingCallback(VSyncDispatch& dispatch)
: mDispatch(dispatch),
mToken(dispatch.registerCallback(std::bind(&CountingCallback::counter, this,
- std::placeholders::_1),
+ std::placeholders::_1,
+ std::placeholders::_2),
"test")) {}
~CountingCallback() { mDispatch.unregisterCallback(mToken); }
operator VSyncDispatch::CallbackToken() const { return mToken; }
- void counter(nsecs_t time) { mCalls.push_back(time); }
+ void counter(nsecs_t time, nsecs_t) { mCalls.push_back(time); }
VSyncDispatch& mDispatch;
VSyncDispatch::CallbackToken mToken;
@@ -115,7 +116,8 @@
PausingCallback(VSyncDispatch& dispatch, std::chrono::milliseconds pauseAmount)
: mDispatch(dispatch),
mToken(dispatch.registerCallback(std::bind(&PausingCallback::pause, this,
- std::placeholders::_1),
+ std::placeholders::_1,
+ std::placeholders::_2),
"test")),
mRegistered(true),
mPauseAmount(pauseAmount) {}
@@ -123,7 +125,7 @@
operator VSyncDispatch::CallbackToken() const { return mToken; }
- void pause(nsecs_t) {
+ void pause(nsecs_t, nsecs_t) {
std::unique_lock<std::mutex> lk(mMutex);
mPause = true;
mCv.notify_all();
@@ -462,7 +464,8 @@
EXPECT_CALL(mMockClock, alarmIn(_, 1000)).InSequence(seq);
VSyncDispatch::CallbackToken tmp;
- tmp = mDispatch.registerCallback([&](auto) { mDispatch.schedule(tmp, 100, 2000); }, "o.o");
+ tmp = mDispatch.registerCallback([&](auto, auto) { mDispatch.schedule(tmp, 100, 2000); },
+ "o.o");
mDispatch.schedule(tmp, 100, 1000);
advanceToNextCallback();
@@ -472,7 +475,7 @@
VSyncDispatch::CallbackToken tmp;
std::optional<nsecs_t> lastTarget;
tmp = mDispatch.registerCallback(
- [&](auto timestamp) {
+ [&](auto timestamp, auto) {
EXPECT_EQ(mDispatch.schedule(tmp, 400, timestamp - mVsyncMoveThreshold),
ScheduleResult::Scheduled);
EXPECT_EQ(mDispatch.schedule(tmp, 400, timestamp), ScheduleResult::Scheduled);
@@ -621,7 +624,7 @@
EXPECT_CALL(mMockClock, alarmCancel()).Times(1);
VSyncCallbackRegistration cb(
- mDispatch, [](auto) {}, "");
+ mDispatch, [](auto, auto) {}, "");
VSyncCallbackRegistration cb1(std::move(cb));
cb.schedule(100, 1000);
cb.cancel();
@@ -635,9 +638,9 @@
EXPECT_CALL(mMockClock, alarmCancel()).Times(1);
VSyncCallbackRegistration cb(
- mDispatch, [](auto) {}, "");
+ mDispatch, [](auto, auto) {}, "");
VSyncCallbackRegistration cb1(
- mDispatch, [](auto) {}, "");
+ mDispatch, [](auto, auto) {}, "");
cb1 = std::move(cb);
cb.schedule(100, 1000);
cb.cancel();
@@ -656,7 +659,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, stateAfterInitialization) {
std::string name("basicname");
VSyncDispatchTimerQueueEntry entry(
- name, [](auto) {}, mVsyncMoveThreshold);
+ name, [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_THAT(entry.name(), Eq(name));
EXPECT_FALSE(entry.lastExecutedVsyncTarget());
EXPECT_FALSE(entry.wakeupTime());
@@ -664,7 +667,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, stateScheduling) {
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_FALSE(entry.wakeupTime());
EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
@@ -684,7 +687,7 @@
.Times(1)
.WillOnce(Return(10000));
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_FALSE(entry.wakeupTime());
EXPECT_THAT(entry.schedule(500, 994, mStubTracker, now), Eq(ScheduleResult::Scheduled));
@@ -695,12 +698,14 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, runCallback) {
auto callCount = 0;
- auto calledTime = 0;
+ auto vsyncCalledTime = 0;
+ auto wakeupCalledTime = 0;
VSyncDispatchTimerQueueEntry entry(
"test",
- [&](auto time) {
+ [&](auto vsyncTime, auto wakeupTime) {
callCount++;
- calledTime = time;
+ vsyncCalledTime = vsyncTime;
+ wakeupCalledTime = wakeupTime;
},
mVsyncMoveThreshold);
@@ -709,10 +714,11 @@
ASSERT_TRUE(wakeup);
EXPECT_THAT(*wakeup, Eq(900));
- entry.callback(entry.executing());
+ entry.callback(entry.executing(), *wakeup);
EXPECT_THAT(callCount, Eq(1));
- EXPECT_THAT(calledTime, Eq(mPeriod));
+ EXPECT_THAT(vsyncCalledTime, Eq(mPeriod));
+ EXPECT_THAT(wakeupCalledTime, Eq(*wakeup));
EXPECT_FALSE(entry.wakeupTime());
auto lastCalledTarget = entry.lastExecutedVsyncTarget();
ASSERT_TRUE(lastCalledTarget);
@@ -726,7 +732,7 @@
.WillOnce(Return(1020));
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_FALSE(entry.wakeupTime());
entry.update(mStubTracker, 0);
@@ -745,7 +751,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, skipsUpdateIfJustScheduled) {
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
entry.update(mStubTracker, 0);
@@ -756,7 +762,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, willSnapToNextTargettableVSync) {
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
entry.executing(); // 1000 is executing
// had 1000 not been executing, this could have been scheduled for time 800.
@@ -773,7 +779,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest,
willRequestNextEstimateWhenSnappingToNextTargettableVSync) {
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
Sequence seq;
EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(500))
@@ -795,7 +801,7 @@
TEST_F(VSyncDispatchTimerQueueEntryTest, reportsScheduledIfStillTime) {
VSyncDispatchTimerQueueEntry entry(
- "test", [](auto) {}, mVsyncMoveThreshold);
+ "test", [](auto, auto) {}, mVsyncMoveThreshold);
EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
EXPECT_THAT(entry.schedule(50, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
diff --git a/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
index 72f8bb9..a9d8f9c 100644
--- a/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
@@ -73,7 +73,8 @@
class MockVSyncDispatch : public VSyncDispatch {
public:
- MOCK_METHOD2(registerCallback, CallbackToken(std::function<void(nsecs_t)> const&, std::string));
+ MOCK_METHOD2(registerCallback,
+ CallbackToken(std::function<void(nsecs_t, nsecs_t)> const&, std::string));
MOCK_METHOD1(unregisterCallback, void(CallbackToken));
MOCK_METHOD3(schedule, ScheduleResult(CallbackToken, nsecs_t, nsecs_t));
MOCK_METHOD1(cancel, CancelResult(CallbackToken token));
@@ -82,7 +83,7 @@
class VSyncDispatchWrapper : public VSyncDispatch {
public:
VSyncDispatchWrapper(std::shared_ptr<VSyncDispatch> const& dispatch) : mDispatch(dispatch) {}
- CallbackToken registerCallback(std::function<void(nsecs_t)> const& callbackFn,
+ CallbackToken registerCallback(std::function<void(nsecs_t, nsecs_t)> const& callbackFn,
std::string callbackName) final {
return mDispatch->registerCallback(callbackFn, callbackName);
}
@@ -159,14 +160,15 @@
static constexpr nsecs_t mPhase = 3000;
static constexpr nsecs_t mAnotherPhase = 5200;
static constexpr nsecs_t period = 10000;
- static constexpr nsecs_t mFakeCbTime = 2093;
+ static constexpr nsecs_t mFakeVSyncTime = 2093;
+ static constexpr nsecs_t mFakeWakeupTime = 1892;
static constexpr nsecs_t mFakeNow = 2214;
static constexpr const char mName[] = "callbacky";
VSyncDispatch::CallbackToken const mFakeToken{2398};
nsecs_t lastCallbackTime = 0;
StubCallback outerCb;
- std::function<void(nsecs_t)> innerCb;
+ std::function<void(nsecs_t, nsecs_t)> innerCb;
VSyncReactor mReactor;
};
@@ -399,7 +401,8 @@
.WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken)));
EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow))
.InSequence(seq);
- EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeCbTime))
+ EXPECT_CALL(*mMockDispatch,
+ schedule(mFakeToken, computeWorkload(period, mPhase), mFakeVSyncTime))
.Times(2)
.InSequence(seq);
EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).InSequence(seq);
@@ -407,24 +410,25 @@
mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime);
ASSERT_TRUE(innerCb);
- innerCb(mFakeCbTime);
- innerCb(mFakeCbTime);
+ innerCb(mFakeVSyncTime, mFakeWakeupTime);
+ innerCb(mFakeVSyncTime, mFakeWakeupTime);
}
-TEST_F(VSyncReactorTest, callbackTimestampReadapted) {
+TEST_F(VSyncReactorTest, callbackTimestampDistributedIsWakeupTime) {
Sequence seq;
EXPECT_CALL(*mMockDispatch, registerCallback(_, _))
.InSequence(seq)
.WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken)));
EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow))
.InSequence(seq);
- EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeCbTime))
+ EXPECT_CALL(*mMockDispatch,
+ schedule(mFakeToken, computeWorkload(period, mPhase), mFakeVSyncTime))
.InSequence(seq);
mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime);
ASSERT_TRUE(innerCb);
- innerCb(mFakeCbTime);
- EXPECT_THAT(outerCb.lastCallTime(), Optional(mFakeCbTime - period));
+ innerCb(mFakeVSyncTime, mFakeWakeupTime);
+ EXPECT_THAT(outerCb.lastCallTime(), Optional(mFakeWakeupTime));
}
TEST_F(VSyncReactorTest, eventListenersRemovedOnDestruction) {
@@ -500,7 +504,7 @@
mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime);
mReactor.changePhaseOffset(&outerCb, mAnotherPhase);
ASSERT_TRUE(innerCb);
- innerCb(mFakeCbTime);
+ innerCb(mFakeVSyncTime, mFakeWakeupTime);
}
TEST_F(VSyncReactorTest, negativeOffsetsApplied) {
@@ -550,7 +554,7 @@
ON_CALL(*mMockDispatch, schedule(_, _, _))
.WillByDefault(Return(ScheduleResult::CannotSchedule));
- EXPECT_DEATH(innerCb(mFakeCbTime), ".*");
+ EXPECT_DEATH(innerCb(mFakeVSyncTime, mFakeWakeupTime), ".*");
}
} // namespace android::scheduler