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

9 files changed, 101 insertions, 64 deletions

diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index f7cd5c4f71..502031c8d8 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 @@ bool ValidateFrameRate(float frameRate, int8_t compatibility, int8_t changeFrame
 
     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 0923438eec..2d1354cdf1 100644
--- a/libs/nativewindow/include/apex/window.h
+++ b/libs/nativewindow/include/apex/window.h
@@ -39,19 +39,6 @@ enum ANativeWindowPerform {
     // 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 a319769148..a54af1fa62 100644
--- a/libs/nativewindow/include/system/window.h
+++ b/libs/nativewindow/include/system/window.h
@@ -1018,6 +1018,24 @@ static inline int native_window_set_auto_prerotation(struct ANativeWindow* windo
     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 997b1a1b1a..f7e1d1ee95 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2596,6 +2596,8 @@ Layer::FrameRateCompatibility Layer::FrameRate::convertCompatibility(int8_t comp
             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 80aa07231f..d4435c2818 100644
--- a/services/surfaceflinger/RefreshRateOverlay.cpp
+++ b/services/surfaceflinger/RefreshRateOverlay.cpp
@@ -45,6 +45,15 @@ constexpr int kDigitSpace = 16;
 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 @@ RefreshRateOverlay::RefreshRateOverlay(FpsRange fpsRange, bool showSpinner)
         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 @@ auto RefreshRateOverlay::getOrCreateBuffers(Fps fps) -> const Buffers& {
         }
     }();
 
-    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 @@ void RefreshRateOverlay::setViewport(ui::Size viewport) {
     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 @@ void RefreshRateOverlay::animate() {
     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 4c830307e8..64ba280f9c 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3629,11 +3629,11 @@ uint32_t SurfaceFlinger::clearTransactionFlags(uint32_t mask) {
 }
 
 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 @@ auto SurfaceFlinger::transactionIsReadyToBeApplied(
 }
 
 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 @@ void SurfaceFlinger::queueTransaction(TransactionState& state) {
         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 @@ int SurfaceFlinger::getMaxAcquiredBufferCountForRefreshRate(Fps refreshRate) con
     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 97b0e8db5c..3c7facfc12 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -784,7 +784,8 @@ private:
 
     // 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 04ca347b2f..bab5326093 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 @@ struct TransactionState {
             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);
+            }
+        }
+    }
+
+    // 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;
+            }
+        }
 
-    void traverseStatesWithBuffers(std::function<void(const layer_state_t&)> visitor);
+        return false;
+    }
 
     FrameTimelineInfo frameTimelineInfo;
     Vector<ComposerState> states;
diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
index 825f145ecd..b9a5f36794 100644
--- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
@@ -337,10 +337,22 @@ TEST_F(SetFrameRateTest, ValidateFrameRate) {
                                   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 @@ TEST_F(SetFrameRateTest, ValidateFrameRate) {
     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, ""));
 }