Merge "Avoid copying Transaction objects unneccessarily." into main

Author: Anton Ivanov

libs/gui/BLASTBufferQueue.cpp

@@ -1032,7 +1032,7 @@ void BLASTBufferQueue::mergeWithNextTransaction(SurfaceComposerClient::Transacti
         // Apply the transaction since we have already acquired the desired frame.
         t->setApplyToken(mApplyToken).apply();
     } else {
-        mPendingTransactions.emplace_back(frameNumber, *t);
+        mPendingTransactions.emplace_back(frameNumber, std::move(*t));
         // Clear the transaction so it can't be applied elsewhere.
         t->clear();
     }
@@ -1050,8 +1050,8 @@ void BLASTBufferQueue::applyPendingTransactions(uint64_t frameNumber) {
 void BLASTBufferQueue::mergePendingTransactions(SurfaceComposerClient::Transaction* t,
                                                 uint64_t frameNumber) {
     auto mergeTransaction =
-            [&t, currentFrameNumber = frameNumber](
-                    std::tuple<uint64_t, SurfaceComposerClient::Transaction> pendingTransaction) {
+            [t, currentFrameNumber = frameNumber](
+                    std::pair<uint64_t, SurfaceComposerClient::Transaction>& pendingTransaction) {
                 auto& [targetFrameNumber, transaction] = pendingTransaction;
                 if (currentFrameNumber < targetFrameNumber) {
                     return false;

libs/gui/SurfaceComposerClient.cpp

@@ -828,11 +828,10 @@ SurfaceComposerClient::Transaction::Transaction() {
     mTransactionCompletedListener = TransactionCompletedListener::getInstance();
 }
 
-SurfaceComposerClient::Transaction::Transaction(const Transaction& other) {
-    mState = other.mState;
-    mListenerCallbacks = other.mListenerCallbacks;
-    mTransactionCompletedListener = TransactionCompletedListener::getInstance();
-}
+SurfaceComposerClient::Transaction::Transaction(Transaction&& other)
+      : mTransactionCompletedListener(TransactionCompletedListener::getInstance()),
+        mState(std::move(other.mState)),
+        mListenerCallbacks(std::move(other.mListenerCallbacks)) {}
 
 void SurfaceComposerClient::Transaction::sanitize(int pid, int uid) {
     uint32_t permissions = LayerStatePermissions::getTransactionPermissions(pid, uid);

libs/gui/include/gui/BLASTBufferQueue.h

@@ -284,7 +284,7 @@ class BLASTBufferQueue : public ConsumerBase::FrameAvailableListener {
     std::function<void(SurfaceComposerClient::Transaction*)> mTransactionReadyCallback
             GUARDED_BY(mMutex);
     SurfaceComposerClient::Transaction* mSyncTransaction GUARDED_BY(mMutex);
-    std::vector<std::tuple<uint64_t /* framenumber */, SurfaceComposerClient::Transaction>>
+    std::vector<std::pair<uint64_t /* framenumber */, SurfaceComposerClient::Transaction>>
             mPendingTransactions GUARDED_BY(mMutex);
 
     std::queue<std::pair<uint64_t, FrameTimelineInfo>> mPendingFrameTimelines GUARDED_BY(mMutex);

libs/gui/include/gui/SurfaceComposerClient.h

@@ -443,7 +443,7 @@ class SurfaceComposerClient : public RefBase
         virtual ~PresentationCallbackRAII();
     };
 
-    class Transaction : public Parcelable {
+    class Transaction {
     private:
         static sp<IBinder> sApplyToken;
         static std::mutex sApplyTokenMutex;
@@ -464,19 +464,20 @@ class SurfaceComposerClient : public RefBase
 
     protected:
         // Accessed in tests.
+        explicit Transaction(Transaction const& other) = default;
         std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
                 mListenerCallbacks;
 
     public:
         Transaction();
-        virtual ~Transaction() = default;
-        Transaction(Transaction const& other);
+        Transaction(Transaction&& other);
+        Transaction& operator=(Transaction&& other) = default;
 
         // Factory method that creates a new Transaction instance from the parcel.
         static std::unique_ptr<Transaction> createFromParcel(const Parcel* parcel);
 
-        status_t writeToParcel(Parcel* parcel) const override;
-        status_t readFromParcel(const Parcel* parcel) override;
+        status_t writeToParcel(Parcel* parcel) const;
+        status_t readFromParcel(const Parcel* parcel);
 
         // Clears the contents of the transaction without applying it.
         void clear();

libs/gui/include/gui/TransactionState.h

@@ -26,7 +26,8 @@ namespace android {
 class TransactionState {
 public:
     explicit TransactionState() = default;
-    TransactionState(TransactionState const& other) = default;
+    TransactionState(TransactionState&& other) = default;
+    TransactionState& operator=(TransactionState&& other) = default;
     status_t writeToParcel(Parcel* parcel) const;
     status_t readFromParcel(const Parcel* parcel);
     layer_state_t* getLayerState(const sp<SurfaceControl>& sc);
@@ -86,6 +87,9 @@ class TransactionState {
     std::vector<ListenerCallbacks> mListenerCallbacks;
 
 private:
+    explicit TransactionState(TransactionState const& other) = default;
+    friend class TransactionApplicationTest;
+    friend class SurfaceComposerClient;
     // We keep track of the last MAX_MERGE_HISTORY_LENGTH merged transaction ids.
     // Ordered most recently merged to least recently merged.
     static constexpr size_t MAX_MERGE_HISTORY_LENGTH = 10u;

services/surfaceflinger/tests/DereferenceSurfaceControl_test.cpp

@@ -52,6 +52,8 @@ class DereferenceSurfaceControlTest : public LayerTransactionTest {
 };
 
 TEST_F(DereferenceSurfaceControlTest, LayerNotInTransaction) {
+    // Last strong pointer is removed, the layer is destroyed and is removed
+    // from compostion.
     fgLayer = nullptr;
     {
         SCOPED_TRACE("after setting null");
@@ -61,7 +63,9 @@ TEST_F(DereferenceSurfaceControlTest, LayerNotInTransaction) {
 }
 
 TEST_F(DereferenceSurfaceControlTest, LayerInTransaction) {
-    auto transaction = Transaction().show(fgLayer);
+    Transaction transaction;
+    transaction.show(fgLayer);
+    // |transaction| retains a strong pointer, so layer is retained.
     fgLayer = nullptr;
     {
         SCOPED_TRACE("after setting null");

services/surfaceflinger/tests/IPC_test.cpp

@@ -42,14 +42,22 @@ using CallbackInfo = SurfaceComposerClient::CallbackInfo;
 using TCLHash = SurfaceComposerClient::TCLHash;
 using android::hardware::graphics::common::V1_1::BufferUsage;
 
-class TransactionHelper : public Transaction {
+class TransactionHelper : public Transaction, public Parcelable {
 public:
+    TransactionHelper() : Transaction() {}
     size_t getNumListeners() { return mListenerCallbacks.size(); }
 
     std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
     getListenerCallbacks() {
         return mListenerCallbacks;
     }
+    status_t writeToParcel(Parcel* parcel) const override {
+        return Transaction::writeToParcel(parcel);
+    }
+
+    status_t readFromParcel(const Parcel* parcel) override {
+        return Transaction::readFromParcel(parcel);
+    }
 };
 
 class IPCTestUtils {

services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp

@@ -78,7 +78,7 @@ class TransactionApplicationTest : public testing::Test {
         }
     };
 
-    void checkEqual(TransactionInfo info, QueuedTransactionState state) {
+    void checkEqual(const TransactionInfo& info, const QueuedTransactionState& state) {
         EXPECT_EQ(0u, info.mComposerStates.size());
         EXPECT_EQ(0u, state.states.size());
 
@@ -383,7 +383,7 @@ class LatchUnsignaledTest : public TransactionApplicationTest {
         EXPECT_TRUE(mFlinger.getTransactionQueue().isEmpty());
         EXPECT_EQ(0u, mFlinger.getPendingTransactionQueue().size());
         std::unordered_set<uint32_t> createdLayers;
-        for (auto transaction : transactions) {
+        for (auto& transaction : transactions) {
             for (auto& state : transaction.mComposerStates) {
                 auto layerId = static_cast<uint32_t>(state.state.layerId);
                 if (createdLayers.find(layerId) == createdLayers.end()) {