Blast: Use a unique id to track buffers
When buffer queue is configured in shared buffer mode, the client
can queue the same buffer over and over again and the consumer
can acquire the same buffer repeatedly. BBQ tracks acquired buffers
by graphic buffer id and SCC tracks release buffer callbacks by
graphic buffer ids. This breaks if a buffer is reused before release.
Fix this by using graphic buffer id and framenumber to track acquired
buffers and buffer release callbacks.
Test: atest CtsDeqpTestCases:dEQP-VK.wsi.android.shared_presentable_image.scale_none.identity.inherit.demand CtsDeqpTestCases:dEQP-VK.wsi.android.colorspace#basic
Bug: 191220669
Change-Id: Ibd54df9fa6780c26cd8de769bf9e43163abbed5a
diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h
index cd3d80e..760c8b9 100644
--- a/services/surfaceflinger/BufferLayer.h
+++ b/services/surfaceflinger/BufferLayer.h
@@ -133,6 +133,7 @@
bool mTransformToDisplayInverse{false};
std::shared_ptr<renderengine::ExternalTexture> mBuffer;
+ uint64_t mFrameNumber;
int mBufferSlot{BufferQueue::INVALID_BUFFER_SLOT};
bool mFrameLatencyNeeded{false};
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 6b5cf04..8cf0d66 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -44,25 +44,18 @@
using PresentState = frametimeline::SurfaceFrame::PresentState;
namespace {
void callReleaseBufferCallback(const sp<ITransactionCompletedListener>& listener,
- const sp<GraphicBuffer>& buffer, const sp<Fence>& releaseFence,
- uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) {
+ const sp<GraphicBuffer>& buffer, uint64_t framenumber,
+ const sp<Fence>& releaseFence, uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) {
if (!listener) {
return;
}
- listener->onReleaseBuffer(buffer->getId(), releaseFence ? releaseFence : Fence::NO_FENCE,
- transformHint, currentMaxAcquiredBufferCount);
+ listener->onReleaseBuffer({buffer->getId(), framenumber},
+ releaseFence ? releaseFence : Fence::NO_FENCE, transformHint,
+ currentMaxAcquiredBufferCount);
}
} // namespace
-// clang-format off
-const std::array<float, 16> BufferStateLayer::IDENTITY_MATRIX{
- 1, 0, 0, 0,
- 0, 1, 0, 0,
- 0, 0, 1, 0,
- 0, 0, 0, 1
-};
-// clang-format on
-
BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args)
: BufferLayer(args), mHwcSlotGenerator(new HwcSlotGenerator()) {
mDrawingState.dataspace = ui::Dataspace::V0_SRGB;
@@ -75,8 +68,8 @@
// issue with the clone layer trying to use the texture.
if (mBufferInfo.mBuffer != nullptr && !isClone()) {
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
- mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFence,
- mTransformHint,
+ mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFrameNumber,
+ mBufferInfo.mFence, mTransformHint,
mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
mOwnerUid));
}
@@ -87,7 +80,7 @@
if (ch == nullptr) {
return OK;
}
- ch->previousBufferId = mPreviousBufferId;
+ ch->previousReleaseCallbackId = mPreviousReleaseCallbackId;
if (!ch->previousReleaseFence.get()) {
ch->previousReleaseFence = fence;
return OK;
@@ -214,7 +207,7 @@
// see BufferStateLayer::onLayerDisplayed.
for (auto& handle : mDrawingState.callbackHandles) {
if (handle->releasePreviousBuffer) {
- handle->previousBufferId = mPreviousBufferId;
+ handle->previousReleaseCallbackId = mPreviousReleaseCallbackId;
break;
}
}
@@ -438,8 +431,8 @@
// dropped and we should decrement the pending buffer count and
// call any release buffer callbacks if set.
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
- mDrawingState.buffer->getBuffer(), mDrawingState.acquireFence,
- mTransformHint,
+ mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber,
+ mDrawingState.acquireFence, mTransformHint,
mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
mOwnerUid));
decrementPendingBufferCount();
@@ -684,7 +677,7 @@
* DeferTransactionUntil -> frameNumber = 2
* Random other stuff
* }
- * Now imagine getHeadFrameNumber returned mDrawingState.mFrameNumber (or mCurrentFrameNumber).
+ * Now imagine mFrameNumber returned mDrawingState.frameNumber (or mCurrentFrameNumber).
* Prior to doTransaction SurfaceFlinger will call notifyAvailableFrames, but because we
* haven't swapped mDrawingState to mDrawingState yet we will think the sync point
* is not ready. So we will return false from applyPendingState and not swap
@@ -792,9 +785,10 @@
decrementPendingBufferCount();
}
- mPreviousBufferId = getCurrentBufferId();
+ mPreviousReleaseCallbackId = {getCurrentBufferId(), mBufferInfo.mFrameNumber};
mBufferInfo.mBuffer = s.buffer;
mBufferInfo.mFence = s.acquireFence;
+ mBufferInfo.mFrameNumber = s.frameNumber;
return NO_ERROR;
}
@@ -969,8 +963,8 @@
// then we will drop a buffer and should decrement the pending buffer count and
// call any release buffer callbacks if set.
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
- mDrawingState.buffer->getBuffer(), mDrawingState.acquireFence,
- mTransformHint,
+ mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber,
+ mDrawingState.acquireFence, mTransformHint,
mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
mOwnerUid));
decrementPendingBufferCount();
diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h
index 2747018..e567478 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -143,15 +143,8 @@
bool bufferNeedsFiltering() const override;
- static const std::array<float, 16> IDENTITY_MATRIX;
-
- std::unique_ptr<renderengine::Image> mTextureImage;
-
- mutable uint64_t mFrameNumber{0};
- uint64_t mFrameCounter{0};
-
sp<Fence> mPreviousReleaseFence;
- uint64_t mPreviousBufferId = 0;
+ ReleaseCallbackId mPreviousReleaseCallbackId = ReleaseCallbackId::INVALID_ID;
uint64_t mPreviousReleasedFrameNumber = 0;
bool mReleasePreviousBuffer = false;
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp
index fdf16a7..6af69f0 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -237,7 +237,8 @@
handle->previousReleaseFence,
handle->transformHint,
handle->currentMaxAcquiredBufferCount,
- eventStats, jankData, handle->previousBufferId);
+ eventStats, jankData,
+ handle->previousReleaseCallbackId);
}
return NO_ERROR;
}
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h
index 444bec6..6f4d812 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -51,7 +51,7 @@
nsecs_t refreshStartTime = 0;
nsecs_t dequeueReadyTime = 0;
uint64_t frameNumber = 0;
- uint64_t previousBufferId = 0;
+ ReleaseCallbackId previousReleaseCallbackId = ReleaseCallbackId::INVALID_ID;
};
class TransactionCallbackInvoker {
diff --git a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
index 5aa809d..579a26e 100644
--- a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
+++ b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
@@ -29,7 +29,7 @@
// b/181132765 - disabled until cuttlefish failures are investigated
class ReleaseBufferCallbackHelper {
public:
- static void function(void* callbackContext, uint64_t graphicsBufferId,
+ static void function(void* callbackContext, ReleaseCallbackId callbackId,
const sp<Fence>& releaseFence,
uint32_t /*currentMaxAcquiredBufferCount*/) {
if (!callbackContext) {
@@ -38,11 +38,11 @@
ReleaseBufferCallbackHelper* helper =
static_cast<ReleaseBufferCallbackHelper*>(callbackContext);
std::lock_guard lock(helper->mMutex);
- helper->mCallbackDataQueue.emplace(graphicsBufferId, releaseFence);
+ helper->mCallbackDataQueue.emplace(callbackId, releaseFence);
helper->mConditionVariable.notify_all();
}
- void getCallbackData(uint64_t* bufferId) {
+ void getCallbackData(ReleaseCallbackId* callbackId) {
std::unique_lock lock(mMutex);
if (mCallbackDataQueue.empty()) {
if (!mConditionVariable.wait_for(lock, std::chrono::seconds(3),
@@ -53,7 +53,7 @@
auto callbackData = mCallbackDataQueue.front();
mCallbackDataQueue.pop();
- *bufferId = callbackData.first;
+ *callbackId = callbackData.first;
}
void verifyNoCallbacks() {
@@ -72,7 +72,7 @@
std::mutex mMutex;
std::condition_variable mConditionVariable;
- std::queue<std::pair<uint64_t, sp<Fence>>> mCallbackDataQueue;
+ std::queue<std::pair<ReleaseCallbackId, sp<Fence>>> mCallbackDataQueue;
};
class ReleaseBufferCallbackTest : public LayerTransactionTest {
@@ -82,10 +82,11 @@
}
static void submitBuffer(const sp<SurfaceControl>& layer, sp<GraphicBuffer> buffer,
- sp<Fence> fence, CallbackHelper& callback,
+ sp<Fence> fence, CallbackHelper& callback, const ReleaseCallbackId& id,
ReleaseBufferCallbackHelper& releaseCallback) {
Transaction t;
- t.setBuffer(layer, buffer, releaseCallback.getCallback());
+ t.setFrameNumber(layer, id.framenumber);
+ t.setBuffer(layer, buffer, id, releaseCallback.getCallback());
t.setAcquireFence(layer, fence);
t.addTransactionCompletedCallback(callback.function, callback.getContext());
t.apply();
@@ -98,10 +99,10 @@
}
static void waitForReleaseBufferCallback(ReleaseBufferCallbackHelper& releaseCallback,
- uint64_t expectedReleaseBufferId) {
- uint64_t actualReleaseBufferId;
+ const ReleaseCallbackId& expectedCallbackId) {
+ ReleaseCallbackId actualReleaseBufferId;
releaseCallback.getCallbackData(&actualReleaseBufferId);
- EXPECT_EQ(expectedReleaseBufferId, actualReleaseBufferId);
+ EXPECT_EQ(expectedCallbackId, actualReleaseBufferId);
releaseCallback.verifyNoCallbacks();
}
static ReleaseBufferCallbackHelper* getReleaseBufferCallbackHelper() {
@@ -116,6 +117,10 @@
BufferUsage::COMPOSER_OVERLAY,
"test");
}
+ static uint64_t generateFrameNumber() {
+ static uint64_t sFrameNumber = 0;
+ return ++sFrameNumber;
+ }
};
TEST_F(ReleaseBufferCallbackTest, DISABLED_PresentBuffer) {
@@ -125,7 +130,9 @@
// If a buffer is being presented, we should not emit a release callback.
sp<GraphicBuffer> firstBuffer = getBuffer();
- submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback);
+ ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, firstBufferCallbackId,
+ *releaseCallback);
ExpectedResult expected;
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED);
@@ -143,13 +150,15 @@
// If a presented buffer is replaced, we should emit a release callback for the
// previously presented buffer.
sp<GraphicBuffer> secondBuffer = getBuffer();
- submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback);
+ ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, secondBufferCallbackId,
+ *releaseCallback);
expected = ExpectedResult();
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED,
ExpectedResult::PreviousBuffer::RELEASED);
ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected));
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
TEST_F(ReleaseBufferCallbackTest, DISABLED_OffScreenLayer) {
@@ -160,7 +169,9 @@
// If a buffer is being presented, we should not emit a release callback.
sp<GraphicBuffer> firstBuffer = getBuffer();
- submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback);
+ ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, firstBuffer, Fence::NO_FENCE, transactionCallback, firstBufferCallbackId,
+ *releaseCallback);
ExpectedResult expected;
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED);
@@ -184,23 +195,27 @@
// If a presented buffer is replaced, we should emit a release callback for the
// previously presented buffer.
sp<GraphicBuffer> secondBuffer = getBuffer();
- submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback);
+ ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, secondBuffer, Fence::NO_FENCE, transactionCallback, secondBufferCallbackId,
+ *releaseCallback);
expected = ExpectedResult();
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED,
ExpectedResult::PreviousBuffer::NOT_RELEASED);
ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected));
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
// If continue to submit buffer we continue to get release callbacks
sp<GraphicBuffer> thirdBuffer = getBuffer();
- submitBuffer(layer, thirdBuffer, Fence::NO_FENCE, transactionCallback, *releaseCallback);
+ ReleaseCallbackId thirdBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, thirdBuffer, Fence::NO_FENCE, transactionCallback, thirdBufferCallbackId,
+ *releaseCallback);
expected = ExpectedResult();
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED,
ExpectedResult::PreviousBuffer::NOT_RELEASED);
ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected));
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBufferCallbackId));
}
TEST_F(ReleaseBufferCallbackTest, DISABLED_LayerLifecycle_layerdestroy) {
@@ -210,7 +225,9 @@
// If a buffer is being presented, we should not emit a release callback.
sp<GraphicBuffer> firstBuffer = getBuffer();
- submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback);
+ ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, firstBufferCallbackId,
+ *releaseCallback);
{
ExpectedResult expected;
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
@@ -225,7 +242,7 @@
t.apply();
layer = nullptr;
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
// Destroying a never presented layer emits a callback.
@@ -242,7 +259,9 @@
// Submitting a buffer does not emit a callback.
sp<GraphicBuffer> firstBuffer = getBuffer();
- submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback);
+ ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, firstBuffer, Fence::NO_FENCE, *transactionCallback, firstBufferCallbackId,
+ *releaseCallback);
{
ExpectedResult expected;
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
@@ -253,19 +272,21 @@
// Submitting a second buffer will replace the drawing state buffer and emit a callback.
sp<GraphicBuffer> secondBuffer = getBuffer();
- submitBuffer(layer, secondBuffer, Fence::NO_FENCE, *transactionCallback, *releaseCallback);
+ ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
+ submitBuffer(layer, secondBuffer, Fence::NO_FENCE, *transactionCallback, secondBufferCallbackId,
+ *releaseCallback);
{
ExpectedResult expected;
expected.addSurface(ExpectedResult::Transaction::PRESENTED, layer,
ExpectedResult::Buffer::NOT_ACQUIRED);
ASSERT_NO_FATAL_FAILURE(waitForCallback(*transactionCallback, expected));
ASSERT_NO_FATAL_FAILURE(
- waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId()));
+ waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
// Destroying the offscreen layer emits a callback.
layer = nullptr;
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, secondBufferCallbackId));
}
TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) {
@@ -275,12 +296,13 @@
// If a buffer is being presented, we should not emit a release callback.
sp<GraphicBuffer> firstBuffer = getBuffer();
+ ReleaseCallbackId firstBufferCallbackId(firstBuffer->getId(), generateFrameNumber());
// Try to present 100ms in the future
nsecs_t time = systemTime() + std::chrono::nanoseconds(100ms).count();
Transaction t;
- t.setBuffer(layer, firstBuffer, releaseCallback->getCallback());
+ t.setBuffer(layer, firstBuffer, firstBufferCallbackId, releaseCallback->getCallback());
t.setAcquireFence(layer, Fence::NO_FENCE);
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
@@ -295,7 +317,8 @@
// Dropping frames in transaction queue emits a callback
sp<GraphicBuffer> secondBuffer = getBuffer();
- t.setBuffer(layer, secondBuffer, releaseCallback->getCallback());
+ ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
+ t.setBuffer(layer, secondBuffer, secondBufferCallbackId, releaseCallback->getCallback());
t.setAcquireFence(layer, Fence::NO_FENCE);
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
@@ -307,7 +330,7 @@
ExpectedResult::Buffer::NOT_ACQUIRED,
ExpectedResult::PreviousBuffer::RELEASED);
ASSERT_NO_FATAL_FAILURE(waitForCallback(transactionCallback, expected));
- ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBuffer->getId()));
+ ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
} // namespace android