Prune logic from buffer stuffing recovery using UIDs

Buffer stuffing recovery was initially implemented using
UIDs and sending events to subscribed clients. A different,
simpler approach using callbacks when blocked on dequeueBuffer
has since been favored (change-id I24e34591e809aebd7626657c6340faed323227ad)
and leaves the initial UID event subscription logic obsolete.

Leaving this code causes issues since the logic attempts to
update an event that is part of union, causing issues in other
events that do not hold onto the current object.

This is a partial revert of change-id I38f0eb3d6ef1331e07d6022fa3a0e16c556ba06f

Bug: b/294922229
Test: presubmit
Flag: EXEMPT, code cleanup
Change-Id: I53f653550c58376c8d82a491ecf0aebdeeafbc0d

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

@@ -1458,8 +1458,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