Merge "Prune logic from buffer stuffing recovery using UIDs" into main

Author: Melody Hsu

services/surfaceflinger/FrameTimeline/FrameTimeline.cpp

@@ -29,7 +29,6 @@
 #include <cinttypes>
 #include <numeric>
 #include <unordered_set>
-#include <vector>
 
 #include "../Jank/JankTracker.h"
 
@@ -1005,11 +1004,6 @@ void FrameTimeline::setSfPresent(nsecs_t sfPresentTime,
     finalizeCurrentDisplayFrame();
 }
 
-const std::vector<std::shared_ptr<frametimeline::SurfaceFrame>>& FrameTimeline::getPresentFrames()
-        const {
-    return mPresentFrames;
-}
-
 void FrameTimeline::onCommitNotComposited() {
     SFTRACE_CALL();
     std::scoped_lock lock(mMutex);
@@ -1530,7 +1524,6 @@ void FrameTimeline::flushPendingPresentFences() {
         mPendingPresentFences.erase(mPendingPresentFences.begin());
     }
 
-    mPresentFrames.clear();
     for (size_t i = 0; i < mPendingPresentFences.size(); i++) {
         const auto& pendingPresentFence = mPendingPresentFences[i];
         nsecs_t signalTime = Fence::SIGNAL_TIME_INVALID;
@@ -1544,12 +1537,6 @@ void FrameTimeline::flushPendingPresentFences() {
         auto& displayFrame = pendingPresentFence.second;
         displayFrame->onPresent(signalTime, mPreviousActualPresentTime);
 
-        // Surface frames have been jank classified and can be provided to caller
-        // to detect if buffer stuffing is occurring.
-        for (const auto& frame : displayFrame->getSurfaceFrames()) {
-            mPresentFrames.push_back(frame);
-        }
-
         mPreviousPredictionPresentTime =
                 displayFrame->trace(mSurfaceFlingerPid, monoBootOffset,
                                     mPreviousPredictionPresentTime, mFilterFramesBeforeTraceStarts);

services/surfaceflinger/FrameTimeline/FrameTimeline.h

@@ -331,11 +331,6 @@ class FrameTimeline {
     virtual void setSfPresent(nsecs_t sfPresentTime, const std::shared_ptr<FenceTime>& presentFence,
                               const std::shared_ptr<FenceTime>& gpuFence) = 0;
 
-    // Provides surface frames that have already been jank classified in the most recent
-    // flush of pending present fences. This allows buffer stuffing detection from SF.
-    virtual const std::vector<std::shared_ptr<frametimeline::SurfaceFrame>>& getPresentFrames()
-            const = 0;
-
     // Tells FrameTimeline that a frame was committed but not composited. This is used to flush
     // all the associated surface frames.
     virtual void onCommitNotComposited() = 0;
@@ -513,8 +508,6 @@ class FrameTimeline : public android::frametimeline::FrameTimeline {
     void setSfWakeUp(int64_t token, nsecs_t wakeupTime, Fps refreshRate, Fps renderRate) override;
     void setSfPresent(nsecs_t sfPresentTime, const std::shared_ptr<FenceTime>& presentFence,
                       const std::shared_ptr<FenceTime>& gpuFence = FenceTime::NO_FENCE) override;
-    const std::vector<std::shared_ptr<frametimeline::SurfaceFrame>>& getPresentFrames()
-            const override;
     void onCommitNotComposited() override;
     void parseArgs(const Vector<String16>& args, std::string& result) override;
     void setMaxDisplayFrames(uint32_t size) override;
@@ -562,9 +555,6 @@ class FrameTimeline : public android::frametimeline::FrameTimeline {
     // display frame, this is a good starting size for the vector so that we can avoid the
     // internal vector resizing that happens with push_back.
     static constexpr uint32_t kNumSurfaceFramesInitial = 10;
-    // Presented surface frames that have been jank classified and can
-    // indicate of potential buffer stuffing.
-    std::vector<std::shared_ptr<frametimeline::SurfaceFrame>> mPresentFrames;
 };
 
 } // namespace impl

services/surfaceflinger/Scheduler/EventThread.cpp

@@ -512,14 +512,6 @@ void EventThread::onModeRejected(PhysicalDisplayId displayId, DisplayModeId mode
     mCondition.notify_all();
 }
 
-// Merge lists of buffer stuffed Uids
-void EventThread::addBufferStuffedUids(BufferStuffingMap bufferStuffedUids) {
-    std::lock_guard<std::mutex> lock(mMutex);
-    for (auto& [uid, count] : bufferStuffedUids) {
-        mBufferStuffedUids.emplace_or_replace(uid, count);
-    }
-}
-
 void EventThread::threadMain(std::unique_lock<std::mutex>& lock) {
     DisplayEventConsumers consumers;
 
@@ -761,10 +753,6 @@ void EventThread::generateFrameTimeline(VsyncEventData& outVsyncEventData, nsecs
 
 void EventThread::dispatchEvent(const DisplayEventReceiver::Event& event,
                                 const DisplayEventConsumers& consumers) {
-    // List of Uids that have been sent vsync data with queued buffer count.
-    // Used to keep track of which Uids can be removed from the map of
-    // buffer stuffed clients.
-    ftl::SmallVector<uid_t, 10> uidsPostedQueuedBuffers;
     for (const auto& consumer : consumers) {
         DisplayEventReceiver::Event copy = event;
         if (event.header.type == DisplayEventType::DISPLAY_EVENT_VSYNC) {
@@ -774,13 +762,6 @@ void EventThread::dispatchEvent(const DisplayEventReceiver::Event& event,
                                   event.vsync.vsyncData.preferredExpectedPresentationTime(),
                                   event.vsync.vsyncData.preferredDeadlineTimestamp());
         }
-        auto it = mBufferStuffedUids.find(consumer->mOwnerUid);
-        if (it != mBufferStuffedUids.end()) {
-            copy.vsync.vsyncData.numberQueuedBuffers = it->second;
-            uidsPostedQueuedBuffers.emplace_back(consumer->mOwnerUid);
-        } else {
-            copy.vsync.vsyncData.numberQueuedBuffers = 0;
-        }
         switch (consumer->postEvent(copy)) {
             case NO_ERROR:
                 break;
@@ -796,12 +777,6 @@ void EventThread::dispatchEvent(const DisplayEventReceiver::Event& event,
                 removeDisplayEventConnectionLocked(consumer);
         }
     }
-    // The clients that have already received the queued buffer count
-    // can be removed from the buffer stuffed Uid list to avoid
-    // being sent duplicate messages.
-    for (auto uid : uidsPostedQueuedBuffers) {
-        mBufferStuffedUids.erase(uid);
-    }
     if (event.header.type == DisplayEventType::DISPLAY_EVENT_VSYNC &&
         FlagManager::getInstance().vrr_config()) {
         mLastCommittedVsyncTime =

services/surfaceflinger/Scheduler/EventThread.h

@@ -56,7 +56,6 @@ using gui::VsyncEventData;
 // ---------------------------------------------------------------------------
 
 using FrameRateOverride = DisplayEventReceiver::Event::FrameRateOverride;
-using BufferStuffingMap = ftl::SmallMap<uid_t, uint32_t, 10>;
 
 enum class VSyncRequest {
     None = -2,
@@ -141,10 +140,6 @@ class EventThread {
 
     virtual void onHdcpLevelsChanged(PhysicalDisplayId displayId, int32_t connectedLevel,
                                      int32_t maxLevel) = 0;
-
-    // An elevated number of queued buffers in the server is detected. This propagates a
-    // flag to Choreographer indicating that buffer stuffing recovery should begin.
-    virtual void addBufferStuffedUids(BufferStuffingMap bufferStuffedUids) = 0;
 };
 
 struct IEventThreadCallback {
@@ -199,8 +194,6 @@ class EventThread : public android::EventThread {
     void onHdcpLevelsChanged(PhysicalDisplayId displayId, int32_t connectedLevel,
                              int32_t maxLevel) override;
 
-    void addBufferStuffedUids(BufferStuffingMap bufferStuffedUids) override;
-
 private:
     friend EventThreadTest;
 
@@ -241,10 +234,6 @@ class EventThread : public android::EventThread {
     scheduler::VSyncCallbackRegistration mVsyncRegistration GUARDED_BY(mMutex);
     frametimeline::TokenManager* const mTokenManager;
 
-    // All consumers that need to recover from buffer stuffing and the number
-    // of their queued buffers.
-    BufferStuffingMap mBufferStuffedUids GUARDED_BY(mMutex);
-
     IEventThreadCallback& mCallback;
 
     std::thread mThread;

services/surfaceflinger/Scheduler/Scheduler.cpp

@@ -960,11 +960,6 @@ bool Scheduler::updateFrameRateOverridesLocked(GlobalSignals consideredSignals,
     return mFrameRateOverrideMappings.updateFrameRateOverridesByContent(frameRateOverrides);
 }
 
-void Scheduler::addBufferStuffedUids(BufferStuffingMap bufferStuffedUids) {
-    if (!mRenderEventThread) return;
-    mRenderEventThread->addBufferStuffedUids(std::move(bufferStuffedUids));
-}
-
 void Scheduler::promotePacesetterDisplay(PhysicalDisplayId pacesetterId, PromotionParams params) {
     std::shared_ptr<VsyncSchedule> pacesetterVsyncSchedule;
     {

services/surfaceflinger/Scheduler/Scheduler.h

@@ -337,10 +337,6 @@ class Scheduler : public IEventThreadCallback, android::impl::MessageQueue {
         mPacesetterFrameDurationFractionToSkip = frameDurationFraction;
     }
 
-    // Propagates a flag to the EventThread indicating that buffer stuffing
-    // recovery should begin.
-    void addBufferStuffedUids(BufferStuffingMap bufferStuffedUids);
-
     void setDebugPresentDelay(TimePoint delay) { mDebugPresentDelay = delay; }
 
 private:

services/surfaceflinger/SurfaceFlinger.cpp

@@ -66,13 +66,11 @@
 #include <ftl/concat.h>
 #include <ftl/fake_guard.h>
 #include <ftl/future.h>
-#include <ftl/small_map.h>
 #include <ftl/unit.h>
 #include <gui/AidlUtil.h>
 #include <gui/BufferQueue.h>
 #include <gui/DebugEGLImageTracker.h>
 #include <gui/IProducerListener.h>
-#include <gui/JankInfo.h>
 #include <gui/LayerMetadata.h>
 #include <gui/LayerState.h>
 #include <gui/Surface.h>
@@ -3315,40 +3313,12 @@ void SurfaceFlinger::onCompositionPresented(PhysicalDisplayId pacesetterId,
 
     const TimePoint presentTime = TimePoint::now();
 
-    // The Uids of layer owners that are in buffer stuffing mode, and their elevated
-    // buffer counts. Messages to start recovery are sent exclusively to these Uids.
-    BufferStuffingMap bufferStuffedUids;
-
     // Set presentation information before calling Layer::releasePendingBuffer, such that jank
     // information from previous' frame classification is already available when sending jank info
     // to clients, so they get jank classification as early as possible.
     mFrameTimeline->setSfPresent(presentTime.ns(), pacesetterPresentFenceTime,
                                  pacesetterGpuCompositionDoneFenceTime);
 
-    // Find and register any layers that are in buffer stuffing mode
-    const auto& presentFrames = mFrameTimeline->getPresentFrames();
-
-    for (const auto& frame : presentFrames) {
-        const auto& layer = mLayerLifecycleManager.getLayerFromId(frame->getLayerId());
-        if (!layer) continue;
-        uint32_t numberQueuedBuffers = layer->pendingBuffers ? layer->pendingBuffers->load() : 0;
-        int32_t jankType = frame->getJankType().value_or(JankType::None);
-        if (jankType & JankType::BufferStuffing &&
-            layer->flags & layer_state_t::eRecoverableFromBufferStuffing) {
-            auto [it, wasEmplaced] =
-                    bufferStuffedUids.try_emplace(layer->ownerUid.val(), numberQueuedBuffers);
-            // Update with maximum number of queued buffers, allows clients drawing
-            // multiple windows to account for the most severely stuffed window
-            if (!wasEmplaced && it->second < numberQueuedBuffers) {
-                it->second = numberQueuedBuffers;
-            }
-        }
-    }
-
-    if (!bufferStuffedUids.empty()) {
-        mScheduler->addBufferStuffedUids(std::move(bufferStuffedUids));
-    }
-
     // We use the CompositionEngine::getLastFrameRefreshTimestamp() which might
     // be sampled a little later than when we started doing work for this frame,
     // but that should be okay since CompositorTiming has snapping logic.

services/surfaceflinger/SurfaceFlinger.h

@@ -1459,8 +1459,6 @@ class SurfaceFlinger : public BnSurfaceComposer,
     // Flag used to set override desired display mode from backdoor
     bool mDebugDisplayModeSetByBackdoor = false;
 
-    // Tracks the number of maximum queued buffers by layer owner Uid.
-    using BufferStuffingMap = ftl::SmallMap<uid_t, uint32_t, 10>;
     BufferCountTracker mBufferCountTracker;
 
     std::unordered_map<DisplayId, sp<HdrLayerInfoReporter>> mHdrLayerInfoListeners

services/surfaceflinger/tests/unittests/EventThreadTest.cpp

@@ -23,7 +23,6 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
-#include <gui/DisplayEventReceiver.h>
 #include <log/log.h>
 #include <scheduler/VsyncConfig.h>
 #include <utils/Errors.h>
@@ -112,8 +111,6 @@ class EventThreadTest : public testing::Test, public IEventThreadCallback {
     void expectOnExpectedPresentTimePosted(nsecs_t expectedPresentTime);
     void expectUidFrameRateMappingEventReceivedByConnection(PhysicalDisplayId expectedDisplayId,
                                                             std::vector<FrameRateOverride>);
-    void expectQueuedBufferCountReceivedByConnection(
-            ConnectionEventRecorder& connectionEventRecorder, uint32_t expectedBufferCount);
 
     void onVSyncEvent(nsecs_t timestamp, nsecs_t expectedPresentationTime,
                       nsecs_t deadlineTimestamp) {
@@ -147,7 +144,6 @@ class EventThreadTest : public testing::Test, public IEventThreadCallback {
     sp<MockEventThreadConnection> mConnection;
     sp<MockEventThreadConnection> mThrottledConnection;
     std::unique_ptr<frametimeline::impl::TokenManager> mTokenManager;
-    std::vector<ConnectionEventRecorder*> mBufferStuffedConnectionRecorders;
 
     std::chrono::nanoseconds mVsyncPeriod;
 
@@ -380,14 +376,6 @@ void EventThreadTest::expectUidFrameRateMappingEventReceivedByConnection(
     EXPECT_EQ(expectedDisplayId, event.header.displayId);
 }
 
-void EventThreadTest::expectQueuedBufferCountReceivedByConnection(
-        ConnectionEventRecorder& connectionEventRecorder, uint32_t expectedBufferCount) {
-    auto args = connectionEventRecorder.waitForCall();
-    ASSERT_TRUE(args.has_value());
-    const auto& event = std::get<0>(args.value());
-    EXPECT_EQ(expectedBufferCount, event.vsync.vsyncData.numberQueuedBuffers);
-}
-
 namespace {
 
 using namespace testing;
@@ -880,63 +868,6 @@ TEST_F(EventThreadTest, postHcpLevelsChanged) {
     EXPECT_EQ(HDCP_V2, event.hdcpLevelsChange.maxLevel);
 }
 
-TEST_F(EventThreadTest, connectionReceivesBufferStuffing) {
-    setupEventThread();
-
-    // Create a connection that will experience buffer stuffing.
-    ConnectionEventRecorder stuffedConnectionEventRecorder{0};
-    sp<MockEventThreadConnection> stuffedConnection =
-            createConnection(stuffedConnectionEventRecorder,
-                             gui::ISurfaceComposer::EventRegistration::modeChanged |
-                                     gui::ISurfaceComposer::EventRegistration::frameRateOverride,
-                             111);
-
-    // Add a connection and buffer count to the list of stuffed Uids that will receive
-    // data in the next vsync event.
-    BufferStuffingMap bufferStuffedUids;
-    bufferStuffedUids.try_emplace(stuffedConnection->mOwnerUid, 3);
-    mThread->addBufferStuffedUids(bufferStuffedUids);
-    mBufferStuffedConnectionRecorders.emplace_back(&stuffedConnectionEventRecorder);
-
-    // Signal that we want the next vsync event to be posted to two connections.
-    mThread->requestNextVsync(mConnection);
-    mThread->requestNextVsync(stuffedConnection);
-    onVSyncEvent(123, 456, 789);
-
-    // Vsync event data contains number of queued buffers.
-    expectQueuedBufferCountReceivedByConnection(mConnectionEventCallRecorder, 0);
-    expectQueuedBufferCountReceivedByConnection(stuffedConnectionEventRecorder, 3);
-}
-
-TEST_F(EventThreadTest, connectionsWithSameUidReceiveBufferStuffing) {
-    setupEventThread();
-
-    // Create a connection with the same Uid as another connection.
-    ConnectionEventRecorder secondConnectionEventRecorder{0};
-    sp<MockEventThreadConnection> secondConnection =
-            createConnection(secondConnectionEventRecorder,
-                             gui::ISurfaceComposer::EventRegistration::modeChanged |
-                                     gui::ISurfaceComposer::EventRegistration::frameRateOverride,
-                             mConnectionUid);
-
-    // Add connection Uid and buffer count to the list of stuffed Uids that will receive
-    // data in the next vsync event.
-    BufferStuffingMap bufferStuffedUids;
-    bufferStuffedUids.try_emplace(mConnectionUid, 3);
-    mThread->addBufferStuffedUids(bufferStuffedUids);
-    mBufferStuffedConnectionRecorders.emplace_back(&mConnectionEventCallRecorder);
-    mBufferStuffedConnectionRecorders.emplace_back(&secondConnectionEventRecorder);
-
-    // Signal that we want the next vsync event to be posted to two connections.
-    mThread->requestNextVsync(mConnection);
-    mThread->requestNextVsync(secondConnection);
-    onVSyncEvent(123, 456, 789);
-
-    // Vsync event data contains number of queued buffers.
-    expectQueuedBufferCountReceivedByConnection(mConnectionEventCallRecorder, 3);
-    expectQueuedBufferCountReceivedByConnection(secondConnectionEventRecorder, 3);
-}
-
 } // namespace
 } // namespace android
 

services/surfaceflinger/tests/unittests/mock/MockEventThread.h

@@ -55,7 +55,6 @@ class EventThread : public android::EventThread {
     MOCK_METHOD(void, onHdcpLevelsChanged,
                 (PhysicalDisplayId displayId, int32_t connectedLevel, int32_t maxLevel),
                 (override));
-    MOCK_METHOD(void, addBufferStuffedUids, (BufferStuffingMap), (override));
 };
 
 } // namespace android::mock