commit 8dd181f3e8274bf89ebde94c1c7d57c9c83b0a49
author chaviw <chaviw@google.com> Wed Jan 05 18:36:46 2022 -0600
committer chaviw <chaviw@google.com> Wed Jan 05 19:24:40 2022 -0600
tree 738f6dcff20f12f2a985e1811fe58515266e2627
parent 38e78ac1e8d5006248f209fa7796faeea8feca66

Replace releaseCallbackId with generateReleaseCallbackId in BufferData

releaseCallbackId was confusing data in BufferData because it wasn't
being parceled, even though the rest of the object was. This is because
the ReleaseCallbackId can be generated from info in BufferData.
Therefore, remove releaseCallbackId and instead replace with a function
that generates the ReleaseCallbackId

This fixes an issue when Transactions that contained buffers for the
same layer were being merged. The merge was expected to release the old
buffer. However, if the Transaction was parceled before it was merged,
the ReleaseCallbackId would be lost and the release callback would send
an invalid callback id, resulting in a lost buffer that never got
released. By using generateReleaseCallbackId, the callback id is
recreated when the release callback needs to be invoked since the
buffer and framenumber are already being parceled.

Test: ReleaseBufferCallbackTest
Test: AppConfigurationTests
Bug: 209920544
Change-Id: I2a24b8a9764959173c960048dc82e68f4c083898

libs/gui/SurfaceComposerClient.cpp

diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index cf04ec8..3ec347f 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -736,10 +736,10 @@
         // if the callback is in process, run on a different thread to avoid any lock contigency
         // issues in the client.
         SurfaceComposerClient::getDefault()
-                ->mReleaseCallbackThread.addReleaseCallback(state.bufferData.releaseCallbackId,
-                                                            fence);
+                ->mReleaseCallbackThread
+                .addReleaseCallback(state.bufferData.generateReleaseCallbackId(), fence);
     } else {
-        listener->onReleaseBuffer(state.bufferData.releaseCallbackId, fence, UINT_MAX);
+        listener->onReleaseBuffer(state.bufferData.generateReleaseCallbackId(), fence, UINT_MAX);
     }
 }
 
@@ -1335,7 +1335,7 @@
     BufferData bufferData = s->bufferData;
 
     TransactionCompletedListener::getInstance()->removeReleaseBufferCallback(
-            bufferData.releaseCallbackId);
+            bufferData.generateReleaseCallbackId());
     BufferData emptyBufferData;
     s->what &= ~layer_state_t::eBufferChanged;
     s->bufferData = emptyBufferData;
@@ -1347,7 +1347,7 @@
 SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer(
         const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,
         const std::optional<sp<Fence>>& fence, const std::optional<uint64_t>& frameNumber,
-        const ReleaseCallbackId& id, ReleaseBufferCallback callback) {
+        ReleaseBufferCallback callback) {
     layer_state_t* s = getLayerState(sc);
     if (!s) {
         mStatus = BAD_INDEX;
@@ -1371,7 +1371,7 @@
     if (mIsAutoTimestamp) {
         mDesiredPresentTime = systemTime();
     }
-    setReleaseBufferCallback(&bufferData, id, callback);
+    setReleaseBufferCallback(&bufferData, callback);
     s->what |= layer_state_t::eBufferChanged;
     s->bufferData = bufferData;
     registerSurfaceControlForCallback(sc);
@@ -1381,7 +1381,6 @@
 }
 
 void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bufferData,
-                                                                  const ReleaseCallbackId& id,
                                                                   ReleaseBufferCallback callback) {
     if (!callback) {
         return;
@@ -1394,9 +1393,8 @@
     }
 
     bufferData->releaseBufferListener = TransactionCompletedListener::getIInstance();
-    bufferData->releaseCallbackId = id;
     auto listener = TransactionCompletedListener::getInstance();
-    listener->setReleaseBufferCallback(id, callback);
+    listener->setReleaseBufferCallback(bufferData->generateReleaseCallbackId(), callback);
 }
 
 SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDataspace(