SF: opportunistically try to present the next vsync on early offset
When SF changes from "late" vsync configuration to "early", it
might skip a frame if the work duration is longer than the next vsync.
This in turn causes jank on apps as SF skips a vsync, which will make
present the app buffer a vsync later.
Bug: 273702768
Test: manual
Test: presubmit
Change-Id: I3e3ab6a457beaecef9c2be529a53ad40f540e373
diff --git a/services/surfaceflinger/Android.bp b/services/surfaceflinger/Android.bp
index 71c75f9..9d0f285 100644
--- a/services/surfaceflinger/Android.bp
+++ b/services/surfaceflinger/Android.bp
@@ -31,9 +31,6 @@
"-Wconversion",
"-DANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION",
],
- static_libs: [
- "libsurfaceflingerflags",
- ],
}
cc_defaults {
diff --git a/services/surfaceflinger/CompositionEngine/Android.bp b/services/surfaceflinger/CompositionEngine/Android.bp
index 2dc7332..370e4b6 100644
--- a/services/surfaceflinger/CompositionEngine/Android.bp
+++ b/services/surfaceflinger/CompositionEngine/Android.bp
@@ -63,6 +63,7 @@
cc_library {
name: "libcompositionengine",
defaults: ["libcompositionengine_defaults"],
+ static_libs: ["libsurfaceflingerflags"],
srcs: [
"src/planner/CachedSet.cpp",
"src/planner/Flattener.cpp",
@@ -107,6 +108,7 @@
"libgtest",
"libgmock",
"libcompositionengine",
+ "libsurfaceflingerflags_test",
],
local_include_dirs: ["include"],
export_include_dirs: ["include"],
@@ -141,6 +143,7 @@
"librenderengine_mocks",
"libgmock",
"libgtest",
+ "libsurfaceflingerflags_test",
],
// For some reason, libvulkan isn't picked up from librenderengine
// Probably ASAN related?
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
index 1f922f1..c4c9fa5 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
@@ -28,10 +28,13 @@
#include "VSyncDispatchTimerQueue.h"
#include "VSyncTracker.h"
+#include <com_android_graphics_surfaceflinger_flags.h>
+
#undef LOG_TAG
#define LOG_TAG "VSyncDispatch"
namespace android::scheduler {
+using namespace com::android::graphics::surfaceflinger;
using base::StringAppendF;
@@ -100,8 +103,14 @@
mArmedInfo && (nextVsyncTime > (mArmedInfo->mActualVsyncTime + mMinVsyncDistance));
bool const wouldSkipAWakeup =
mArmedInfo && ((nextWakeupTime > (mArmedInfo->mActualWakeupTime + mMinVsyncDistance)));
- if (wouldSkipAVsyncTarget && wouldSkipAWakeup) {
- return getExpectedCallbackTime(nextVsyncTime, timing);
+ if (flags::dont_skip_on_early()) {
+ if (wouldSkipAVsyncTarget || wouldSkipAWakeup) {
+ return getExpectedCallbackTime(mArmedInfo->mActualVsyncTime, timing);
+ }
+ } else {
+ if (wouldSkipAVsyncTarget && wouldSkipAWakeup) {
+ return getExpectedCallbackTime(nextVsyncTime, timing);
+ }
}
nextVsyncTime = adjustVsyncIfNeeded(tracker, nextVsyncTime);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 2b70301..7e799bb 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -6446,6 +6446,8 @@
mMisc2FlagEarlyBootValue == mMisc2FlagLateBootValue ? "stable" : "modified");
StringAppendF(&result, "VrrConfigFlagValue: %s\n",
flagutils::vrrConfigEnabled() ? "true" : "false");
+ StringAppendF(&result, "DontSkipOnEarlyFlagValue: %s\n",
+ flags::dont_skip_on_early() ? "true" : "false");
getRenderEngine().dump(result);
diff --git a/services/surfaceflinger/surfaceflinger_flags.aconfig b/services/surfaceflinger/surfaceflinger_flags.aconfig
index fedf08b..01b8447 100644
--- a/services/surfaceflinger/surfaceflinger_flags.aconfig
+++ b/services/surfaceflinger/surfaceflinger_flags.aconfig
@@ -38,3 +38,10 @@
bug: "283055450"
is_fixed_read_only: true
}
+
+flag {
+ name: "dont_skip_on_early"
+ namespace: "core_graphics"
+ description: "This flag is guarding the behaviour where SurfaceFlinger is trying to opportunistically present a frame when the configuration change from late to early"
+ bug: "273702768"
+}
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index 36b1972..67f2dca 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -67,6 +67,7 @@
static_libs: [
"liblayers_proto",
"android.hardware.graphics.composer@2.1",
+ "libsurfaceflingerflags",
],
shared_libs: [
"android.hardware.graphics.common@1.2",
diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp
index c99809b..ec21eaf 100644
--- a/services/surfaceflinger/tests/unittests/Android.bp
+++ b/services/surfaceflinger/tests/unittests/Android.bp
@@ -42,6 +42,12 @@
],
}
+cc_aconfig_library {
+ name: "libsurfaceflingerflags_test",
+ aconfig_declarations: "surfaceflinger_flags",
+ test: true,
+}
+
cc_test {
name: "libsurfaceflinger_unittest",
defaults: [
@@ -168,6 +174,7 @@
"libtimestats_proto",
"libtonemap",
"perfetto_trace_protos",
+ "libsurfaceflingerflags_test",
],
shared_libs: [
"android.hardware.configstore-utils",
diff --git a/services/surfaceflinger/tests/unittests/FlagUtils.h b/services/surfaceflinger/tests/unittests/FlagUtils.h
new file mode 100644
index 0000000..7103684
--- /dev/null
+++ b/services/surfaceflinger/tests/unittests/FlagUtils.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2023 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.
+ */
+
+#pragma once
+
+#define SET_FLAG_FOR_TEST(name, value) TestFlagSetter _testflag_((name), (name), (value))
+
+namespace android {
+class TestFlagSetter {
+public:
+ TestFlagSetter(bool (*getter)(), void((*setter)(bool)), bool flagValue) {
+ const bool initialValue = getter();
+ setter(flagValue);
+ mResetFlagValue = [=] { setter(initialValue); };
+ }
+
+ ~TestFlagSetter() { mResetFlagValue(); }
+
+private:
+ std::function<void()> mResetFlagValue;
+};
+
+} // namespace android
diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
index 7af1da6..b8fdce1 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
@@ -30,13 +30,17 @@
#include <scheduler/TimeKeeper.h>
+#include "FlagUtils.h"
#include "Scheduler/VSyncDispatchTimerQueue.h"
#include "Scheduler/VSyncTracker.h"
+#include <com_android_graphics_surfaceflinger_flags.h>
+
using namespace testing;
using namespace std::literals;
namespace android::scheduler {
+using namespace com::android::graphics::surfaceflinger;
class MockVSyncTracker : public VSyncTracker {
public:
@@ -1061,6 +1065,48 @@
EXPECT_THAT(cb.mReadyTime[0], Eq(2000));
}
+TEST_F(VSyncDispatchTimerQueueTest, skipAVsyc) {
+ SET_FLAG_FOR_TEST(flags::dont_skip_on_early, false);
+
+ EXPECT_CALL(mMockClock, alarmAt(_, 500));
+ CountingCallback cb(mDispatch);
+ auto result =
+ mDispatch->schedule(cb,
+ {.workDuration = 500, .readyDuration = 0, .earliestVsync = 1000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(500, *result);
+ mMockClock.advanceBy(300);
+
+ result = mDispatch->schedule(cb,
+ {.workDuration = 800, .readyDuration = 0, .earliestVsync = 1000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(1200, *result);
+
+ advanceToNextCallback();
+ ASSERT_THAT(cb.mCalls.size(), Eq(1));
+}
+
+TEST_F(VSyncDispatchTimerQueueTest, dontskipAVsyc) {
+ SET_FLAG_FOR_TEST(flags::dont_skip_on_early, true);
+
+ EXPECT_CALL(mMockClock, alarmAt(_, 500));
+ CountingCallback cb(mDispatch);
+ auto result =
+ mDispatch->schedule(cb,
+ {.workDuration = 500, .readyDuration = 0, .earliestVsync = 1000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(500, *result);
+ mMockClock.advanceBy(300);
+
+ result = mDispatch->schedule(cb,
+ {.workDuration = 800, .readyDuration = 0, .earliestVsync = 1000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(200, *result);
+
+ advanceToNextCallback();
+ ASSERT_THAT(cb.mCalls.size(), Eq(1));
+}
+
class VSyncDispatchTimerQueueEntryTest : public testing::Test {
protected:
nsecs_t const mPeriod = 1000;
