SF: Fix feedback loop with refresh rate overlay
RefreshRateOverlay was refactored to use transactions instead of APIs
internal to SF. A side effect is that the overlay layer feeds back into
the frame rate detection and idle heuristics, which causes oscillation
that trends to the high refresh rate.
The transaction to setFrameRate failed to apply, as the NoVote argument
was invalid. Make it valid, such that LayerHistory::summarize skips the
overlay layer.
Do not reset the idle timer for solely NO_VOTE transactions.
Bug: 221081400
Test: flame is not stuck at 90 Hz when overlay is enabled.
Test: Same with sf.debug.show_refresh_rate_overlay_spinner
Test: SetFrameRateTest
Change-Id: I6322c1c487672b602a0f974e8ecf445633dcc3a1
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index f7cd5c4..502031c 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -16,8 +16,8 @@
#define LOG_TAG "LayerState"
-#include <apex/window.h>
-#include <inttypes.h>
+#include <cinttypes>
+#include <cmath>
#include <android/native_window.h>
#include <binder/Parcel.h>
@@ -25,10 +25,9 @@
#include <gui/ISurfaceComposerClient.h>
#include <gui/LayerState.h>
#include <private/gui/ParcelUtils.h>
+#include <system/window.h>
#include <utils/Errors.h>
-#include <cmath>
-
namespace android {
using gui::FocusRequest;
@@ -679,7 +678,9 @@
if (compatibility != ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_DEFAULT &&
compatibility != ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_FIXED_SOURCE &&
- (!privileged || compatibility != ANATIVEWINDOW_FRAME_RATE_EXACT)) {
+ (!privileged ||
+ (compatibility != ANATIVEWINDOW_FRAME_RATE_EXACT &&
+ compatibility != ANATIVEWINDOW_FRAME_RATE_NO_VOTE))) {
ALOGE("%s failed - invalid compatibility value %d privileged: %s", functionName,
compatibility, privileged ? "yes" : "no");
return false;
diff --git a/libs/nativewindow/include/apex/window.h b/libs/nativewindow/include/apex/window.h
index 0923438..2d1354c 100644
--- a/libs/nativewindow/include/apex/window.h
+++ b/libs/nativewindow/include/apex/window.h
@@ -39,19 +39,6 @@
// clang-format on
};
-/*
- * Internal extension of compatibility value for ANativeWindow_setFrameRate. */
-enum ANativeWindow_FrameRateCompatibilityInternal {
- /**
- * This surface belongs to an app on the High Refresh Rate Deny list, and needs the display
- * to operate at the exact frame rate.
- *
- * This is used internally by the platform and should not be used by apps.
- * @hide
- */
- ANATIVEWINDOW_FRAME_RATE_EXACT = 100,
-};
-
/**
* Prototype of the function that an ANativeWindow implementation would call
* when ANativeWindow_cancelBuffer is called.
diff --git a/libs/nativewindow/include/system/window.h b/libs/nativewindow/include/system/window.h
index a319769..a54af1f 100644
--- a/libs/nativewindow/include/system/window.h
+++ b/libs/nativewindow/include/system/window.h
@@ -1018,6 +1018,24 @@
return window->perform(window, NATIVE_WINDOW_SET_AUTO_PREROTATION, autoPrerotation);
}
+/*
+ * Internal extension of ANativeWindow_FrameRateCompatibility.
+ */
+enum {
+ /**
+ * This surface belongs to an app on the High Refresh Rate Deny list, and needs the display
+ * to operate at the exact frame rate.
+ *
+ * Keep in sync with Surface.java constant.
+ */
+ ANATIVEWINDOW_FRAME_RATE_EXACT = 100,
+
+ /**
+ * This surface is ignored while choosing the refresh rate.
+ */
+ ANATIVEWINDOW_FRAME_RATE_NO_VOTE,
+};
+
static inline int native_window_set_frame_rate(struct ANativeWindow* window, float frameRate,
int8_t compatibility, int8_t changeFrameRateStrategy) {
return window->perform(window, NATIVE_WINDOW_SET_FRAME_RATE, (double)frameRate,
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 997b1a1..f7e1d1e 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2596,6 +2596,8 @@
return FrameRateCompatibility::ExactOrMultiple;
case ANATIVEWINDOW_FRAME_RATE_EXACT:
return FrameRateCompatibility::Exact;
+ case ANATIVEWINDOW_FRAME_RATE_NO_VOTE:
+ return FrameRateCompatibility::NoVote;
default:
LOG_ALWAYS_FATAL("Invalid frame rate compatibility value %d", compatibility);
return FrameRateCompatibility::Default;
diff --git a/services/surfaceflinger/RefreshRateOverlay.cpp b/services/surfaceflinger/RefreshRateOverlay.cpp
index 80aa072..d4435c2 100644
--- a/services/surfaceflinger/RefreshRateOverlay.cpp
+++ b/services/surfaceflinger/RefreshRateOverlay.cpp
@@ -45,6 +45,15 @@
constexpr int kBufferWidth = 4 * kDigitWidth + 3 * kDigitSpace;
constexpr int kBufferHeight = kDigitHeight;
+SurfaceComposerClient::Transaction createTransaction(const sp<SurfaceControl>& surface) {
+ constexpr float kFrameRate = 0.f;
+ constexpr int8_t kCompatibility = ANATIVEWINDOW_FRAME_RATE_NO_VOTE;
+ constexpr int8_t kSeamlessness = ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS;
+
+ return SurfaceComposerClient::Transaction().setFrameRate(surface, kFrameRate, kCompatibility,
+ kSeamlessness);
+}
+
} // namespace
void RefreshRateOverlay::SevenSegmentDrawer::drawSegment(Segment segment, int left, SkColor color,
@@ -213,12 +222,7 @@
return;
}
- constexpr float kFrameRate = 0.f;
- constexpr int8_t kCompatibility = static_cast<int8_t>(Layer::FrameRateCompatibility::NoVote);
- constexpr int8_t kSeamlessness = ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS;
-
- SurfaceComposerClient::Transaction()
- .setFrameRate(mSurfaceControl, kFrameRate, kCompatibility, kSeamlessness)
+ createTransaction(mSurfaceControl)
.setLayer(mSurfaceControl, INT32_MAX - 2)
.setTrustedOverlay(mSurfaceControl, true)
.apply();
@@ -243,9 +247,7 @@
}
}();
- SurfaceComposerClient::Transaction t;
- t.setTransform(mSurfaceControl, transform);
- t.apply();
+ createTransaction(mSurfaceControl).setTransform(mSurfaceControl, transform).apply();
BufferCache::const_iterator it = mBufferCache.find({fps.getIntValue(), transformHint});
if (it == mBufferCache.end()) {
@@ -287,25 +289,21 @@
Rect frame((3 * width) >> 4, height >> 5);
frame.offsetBy(width >> 5, height >> 4);
- SurfaceComposerClient::Transaction t;
- t.setMatrix(mSurfaceControl, frame.getWidth() / static_cast<float>(kBufferWidth), 0, 0,
- frame.getHeight() / static_cast<float>(kBufferHeight));
- t.setPosition(mSurfaceControl, frame.left, frame.top);
- t.apply();
+ createTransaction(mSurfaceControl)
+ .setMatrix(mSurfaceControl, frame.getWidth() / static_cast<float>(kBufferWidth), 0, 0,
+ frame.getHeight() / static_cast<float>(kBufferHeight))
+ .setPosition(mSurfaceControl, frame.left, frame.top)
+ .apply();
}
void RefreshRateOverlay::setLayerStack(ui::LayerStack stack) {
- SurfaceComposerClient::Transaction t;
- t.setLayerStack(mSurfaceControl, stack);
- t.apply();
+ createTransaction(mSurfaceControl).setLayerStack(mSurfaceControl, stack).apply();
}
void RefreshRateOverlay::changeRefreshRate(Fps fps) {
mCurrentFps = fps;
const auto buffer = getOrCreateBuffers(fps)[mFrame];
- SurfaceComposerClient::Transaction t;
- t.setBuffer(mSurfaceControl, buffer);
- t.apply();
+ createTransaction(mSurfaceControl).setBuffer(mSurfaceControl, buffer).apply();
}
void RefreshRateOverlay::animate() {
@@ -314,9 +312,7 @@
const auto& buffers = getOrCreateBuffers(*mCurrentFps);
mFrame = (mFrame + 1) % buffers.size();
const auto buffer = buffers[mFrame];
- SurfaceComposerClient::Transaction t;
- t.setBuffer(mSurfaceControl, buffer);
- t.apply();
+ createTransaction(mSurfaceControl).setBuffer(mSurfaceControl, buffer).apply();
}
} // namespace android
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 4c83030..64ba280 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3629,11 +3629,11 @@
}
void SurfaceFlinger::setTransactionFlags(uint32_t mask, TransactionSchedule schedule,
- const sp<IBinder>& applyToken) {
+ const sp<IBinder>& applyToken, FrameHint frameHint) {
modulateVsync(&VsyncModulator::setTransactionSchedule, schedule, applyToken);
if (const bool scheduled = mTransactionFlags.fetch_or(mask) & mask; !scheduled) {
- scheduleCommit(FrameHint::kActive);
+ scheduleCommit(frameHint);
}
}
@@ -4005,7 +4005,7 @@
}
void SurfaceFlinger::queueTransaction(TransactionState& state) {
- Mutex::Autolock _l(mQueueLock);
+ Mutex::Autolock lock(mQueueLock);
// Generate a CountDownLatch pending state if this is a synchronous transaction.
if ((state.flags & eSynchronous) || state.inputWindowCommands.syncInputWindows) {
@@ -4024,7 +4024,9 @@
return TransactionSchedule::Late;
}(state.flags);
- setTransactionFlags(eTransactionFlushNeeded, schedule, state.applyToken);
+ const auto frameHint = state.isFrameActive() ? FrameHint::kActive : FrameHint::kNone;
+
+ setTransactionFlags(eTransactionFlushNeeded, schedule, state.applyToken, frameHint);
}
void SurfaceFlinger::waitForSynchronousTransaction(
@@ -7172,15 +7174,6 @@
return calculateMaxAcquiredBufferCount(refreshRate, presentLatency);
}
-void TransactionState::traverseStatesWithBuffers(
- std::function<void(const layer_state_t&)> visitor) {
- for (const auto& state : states) {
- if (state.state.hasBufferChanges() && state.state.hasValidBuffer() && state.state.surface) {
- visitor(state.state);
- }
- }
-}
-
void SurfaceFlinger::handleLayerCreatedLocked(const LayerCreatedState& state) {
sp<Layer> layer = state.layer.promote();
if (!layer) {
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 97b0e8d..3c7facf 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -784,7 +784,8 @@
// Sets the masked bits, and schedules a commit if needed.
void setTransactionFlags(uint32_t mask, TransactionSchedule = TransactionSchedule::Late,
- const sp<IBinder>& applyToken = nullptr);
+ const sp<IBinder>& applyToken = nullptr,
+ FrameHint = FrameHint::kActive);
// Clears and returns the masked bits.
uint32_t clearTransactionFlags(uint32_t mask);
diff --git a/services/surfaceflinger/TransactionState.h b/services/surfaceflinger/TransactionState.h
index 04ca347..bab5326 100644
--- a/services/surfaceflinger/TransactionState.h
+++ b/services/surfaceflinger/TransactionState.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2021 The Android Open Source Project
+ * Copyright 2021 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.
@@ -16,12 +16,21 @@
#pragma once
+#include <condition_variable>
+#include <memory>
+#include <mutex>
+#include <vector>
+
#include <gui/LayerState.h>
+#include <system/window.h>
namespace android {
+
class CountDownLatch;
struct TransactionState {
+ TransactionState() = default;
+
TransactionState(const FrameTimelineInfo& frameTimelineInfo,
const Vector<ComposerState>& composerStates,
const Vector<DisplayState>& displayStates, uint32_t transactionFlags,
@@ -47,9 +56,30 @@
originUid(originUid),
id(transactionId) {}
- TransactionState() {}
+ // Invokes `void(const layer_state_t&)` visitor for matching layers.
+ template <typename Visitor>
+ void traverseStatesWithBuffers(Visitor&& visitor) const {
+ for (const auto& [state] : states) {
+ if (state.hasBufferChanges() && state.hasValidBuffer() && state.surface) {
+ visitor(state);
+ }
+ }
+ }
- void traverseStatesWithBuffers(std::function<void(const layer_state_t&)> visitor);
+ // TODO(b/185535769): Remove FrameHint. Instead, reset the idle timer (of the relevant physical
+ // display) on the main thread if commit leads to composite. Then, RefreshRateOverlay should be
+ // able to setFrameRate once, rather than for each transaction.
+ bool isFrameActive() const {
+ if (!displays.empty()) return true;
+
+ for (const auto& [state] : states) {
+ if (state.frameRateCompatibility != ANATIVEWINDOW_FRAME_RATE_NO_VOTE) {
+ return true;
+ }
+ }
+
+ return false;
+ }
FrameTimelineInfo frameTimelineInfo;
Vector<ComposerState> states;
diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
index 825f145..b9a5f36 100644
--- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
@@ -337,10 +337,22 @@
ANATIVEWINDOW_CHANGE_FRAME_RATE_ALWAYS, ""));
EXPECT_TRUE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_FIXED_SOURCE,
ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
+
+ // Privileged APIs.
+ EXPECT_FALSE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_EXACT,
+ ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
+ EXPECT_FALSE(ValidateFrameRate(0.0f, ANATIVEWINDOW_FRAME_RATE_NO_VOTE,
+ ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
+
+ constexpr bool kPrivileged = true;
EXPECT_TRUE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_EXACT,
ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, "",
- /*privileged=*/true));
+ kPrivileged));
+ EXPECT_TRUE(ValidateFrameRate(0.0f, ANATIVEWINDOW_FRAME_RATE_NO_VOTE,
+ ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, "",
+ kPrivileged));
+ // Invalid frame rate.
EXPECT_FALSE(ValidateFrameRate(-1, ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_DEFAULT,
ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
EXPECT_FALSE(ValidateFrameRate(1.0f / 0.0f, ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_DEFAULT,
@@ -348,15 +360,12 @@
EXPECT_FALSE(ValidateFrameRate(0.0f / 0.0f, ANATIVEWINDOW_FRAME_RATE_COMPATIBILITY_DEFAULT,
ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
- EXPECT_FALSE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_EXACT,
- ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
-
- // Invalid compatibility
+ // Invalid compatibility.
EXPECT_FALSE(
ValidateFrameRate(60.0f, -1, ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
EXPECT_FALSE(ValidateFrameRate(60.0f, 2, ANATIVEWINDOW_CHANGE_FRAME_RATE_ONLY_IF_SEAMLESS, ""));
- // Invalid change frame rate strategy
+ // Invalid change frame rate strategy.
EXPECT_FALSE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_EXACT, -1, ""));
EXPECT_FALSE(ValidateFrameRate(60.0f, ANATIVEWINDOW_FRAME_RATE_EXACT, 2, ""));
}