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
diff --git a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
index f6b0def..027a15e 100644
--- a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
+++ b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
@@ -85,7 +85,7 @@
sp<Fence> fence, CallbackHelper& callback, const ReleaseCallbackId& id,
ReleaseBufferCallbackHelper& releaseCallback) {
Transaction t;
- t.setBuffer(layer, buffer, fence, id.framenumber, id, releaseCallback.getCallback());
+ t.setBuffer(layer, buffer, fence, id.framenumber, releaseCallback.getCallback());
t.addTransactionCompletedCallback(callback.function, callback.getContext());
t.apply();
}
@@ -300,7 +300,7 @@
nsecs_t time = systemTime() + std::chrono::nanoseconds(100ms).count();
Transaction t;
- t.setBuffer(layer, firstBuffer, std::nullopt, std::nullopt, firstBufferCallbackId,
+ t.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
releaseCallback->getCallback());
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
@@ -316,7 +316,7 @@
// Dropping frames in transaction queue emits a callback
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
- t.setBuffer(layer, secondBuffer, std::nullopt, std::nullopt, secondBufferCallbackId,
+ t.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
releaseCallback->getCallback());
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
@@ -360,7 +360,7 @@
Transaction transaction1;
transaction1.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- secondBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
transaction1.addTransactionCompletedCallback(callback1.function, callback1.getContext());
// Set a different TransactionCompletedListener to mimic a second process
@@ -395,14 +395,14 @@
// Create transaction with a buffer.
Transaction transaction;
transaction.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- firstBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
// Call setBuffer on the same transaction with a different buffer.
transaction.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- secondBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
@@ -417,7 +417,7 @@
// Create transaction with a buffer.
Transaction transaction1;
transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- firstBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
@@ -425,7 +425,7 @@
// Create a second transaction with a new buffer for the same layer.
Transaction transaction2;
transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- secondBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
// merge transaction1 into transaction2 so ensure we get a proper buffer release callback.
transaction1.merge(std::move(transaction2));
@@ -446,7 +446,7 @@
Transaction transaction1;
transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- firstBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
// Sent a second buffer to allow the first buffer to get released.
sp<GraphicBuffer> secondBuffer = getBuffer();
@@ -454,7 +454,7 @@
Transaction transaction2;
transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- secondBufferCallbackId, releaseCallback->getCallback());
+ releaseCallback->getCallback());
// Set a different TransactionCompletedListener to mimic a second process
TransactionCompletedListener::setInstance(secondCompletedListener);