BBQ: Recreate BBQ when SurfaceControl changes 1/2
Alternative approach to fab15e55446080bdcfc05ba315e8ef914b0a6f65 which
was racy because the disconnect callback is called without the
BQ lock and the client can continue to modify the BQ state after
disconnecting from the queue.
This approach resets the BQ and BBQ states when the SurfaceControl is
updated. This solves one concrete problem of not relying on the old
SurfaceControl to be destroyed in order to release the currently presented
buffer back to BQ.
In addition this change resets the sync state in BBQ with the rationale
the system does not want to sync on buffers presented on an older
SurfaceControl.
Bug: 197269223
Test: atest BLASTBufferQueueTest
Test: labtest ag/16407859 cf-foldable * 3 (b/197269223#comment40)
Change-Id: Id7049c3fcb7f68ed1bcaed8b82bd13b4397af000
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 85a4b67..34faf87 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -164,8 +164,7 @@
mCurrentMaxAcquiredBufferCount = mMaxAcquiredBuffers;
mNumAcquired = 0;
mNumFrameAvailable = 0;
- BQA_LOGV("BLASTBufferQueue created width=%d height=%d format=%d mTransformHint=%d", mSize.width,
- mSize.height, mFormat, mTransformHint);
+ BQA_LOGV("BLASTBufferQueue created");
}
BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp<SurfaceControl>& surface,
@@ -182,14 +181,14 @@
BQA_LOGE("Applying pending transactions on dtor %d",
static_cast<uint32_t>(mPendingTransactions.size()));
SurfaceComposerClient::Transaction t;
- for (auto& [targetFrameNumber, transaction] : mPendingTransactions) {
- t.merge(std::move(transaction));
- }
- t.apply();
+ mergePendingTransactions(&t, std::numeric_limits<uint64_t>::max() /* frameNumber */);
+ t.setApplyToken(mApplyToken).apply();
}
void BLASTBufferQueue::update(const sp<SurfaceControl>& surface, uint32_t width, uint32_t height,
int32_t format, SurfaceComposerClient::Transaction* outTransaction) {
+ LOG_ALWAYS_FATAL_IF(surface == nullptr, "BLASTBufferQueue: mSurfaceControl must not be NULL");
+
std::unique_lock _lock{mMutex};
if (mFormat != format) {
mFormat = format;
@@ -197,21 +196,20 @@
}
SurfaceComposerClient::Transaction t;
- const bool setBackpressureFlag = !SurfaceControl::isSameSurface(mSurfaceControl, surface);
+ const bool surfaceControlChanged = !SurfaceControl::isSameSurface(mSurfaceControl, surface);
bool applyTransaction = false;
// Always update the native object even though they might have the same layer handle, so we can
// get the updated transform hint from WM.
mSurfaceControl = surface;
- if (mSurfaceControl != nullptr) {
- if (setBackpressureFlag) {
- t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure,
- layer_state_t::eEnableBackpressure);
- applyTransaction = true;
- }
- mTransformHint = mSurfaceControl->getTransformHint();
- mBufferItemConsumer->setTransformHint(mTransformHint);
+ if (surfaceControlChanged) {
+ BQA_LOGD("Updating SurfaceControl without recreating BBQ");
+ t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure,
+ layer_state_t::eEnableBackpressure);
+ applyTransaction = true;
}
+ mTransformHint = mSurfaceControl->getTransformHint();
+ mBufferItemConsumer->setTransformHint(mTransformHint);
BQA_LOGV("update width=%d height=%d format=%d mTransformHint=%d", width, height, format,
mTransformHint);
@@ -225,11 +223,9 @@
mSize = mRequestedSize;
SurfaceComposerClient::Transaction* destFrameTransaction =
(outTransaction) ? outTransaction : &t;
- if (mSurfaceControl != nullptr) {
- destFrameTransaction->setDestinationFrame(mSurfaceControl,
- Rect(0, 0, newSize.getWidth(),
- newSize.getHeight()));
- }
+ destFrameTransaction->setDestinationFrame(mSurfaceControl,
+ Rect(0, 0, newSize.getWidth(),
+ newSize.getHeight()));
applyTransaction = true;
}
}
@@ -640,7 +636,7 @@
// add to shadow queue
mNumFrameAvailable++;
- if (mWaitForTransactionCallback && mNumFrameAvailable == 2) {
+ if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) {
acquireAndReleaseBuffer();
}
ATRACE_INT(mQueuedBufferTrace.c_str(),
@@ -717,7 +713,7 @@
// of the buffer, the next acquire may return with NO_BUFFER_AVAILABLE.
bool BLASTBufferQueue::maxBuffersAcquired(bool includeExtraAcquire) const {
int maxAcquiredBuffers = mMaxAcquiredBuffers + (includeExtraAcquire ? 2 : 1);
- return mNumAcquired == maxAcquiredBuffers;
+ return mNumAcquired >= maxAcquiredBuffers;
}
class BBQSurface : public Surface {
@@ -991,4 +987,54 @@
return mLastAcquiredFrameNumber;
}
+void BLASTBufferQueue::abandon() {
+ std::unique_lock _lock{mMutex};
+ // flush out the shadow queue
+ while (mNumFrameAvailable > 0) {
+ acquireAndReleaseBuffer();
+ }
+
+ // Clear submitted buffer states
+ mNumAcquired = 0;
+ mSubmitted.clear();
+ mPendingRelease.clear();
+
+ if (!mPendingTransactions.empty()) {
+ BQA_LOGD("Applying pending transactions on abandon %d",
+ static_cast<uint32_t>(mPendingTransactions.size()));
+ SurfaceComposerClient::Transaction t;
+ mergePendingTransactions(&t, std::numeric_limits<uint64_t>::max() /* frameNumber */);
+ t.setApplyToken(mApplyToken).apply();
+ }
+
+ // Clear sync states
+ if (mWaitForTransactionCallback) {
+ BQA_LOGD("mWaitForTransactionCallback cleared");
+ mWaitForTransactionCallback = false;
+ }
+
+ if (mSyncTransaction != nullptr) {
+ BQA_LOGD("mSyncTransaction cleared mAcquireSingleBuffer=%s",
+ mAcquireSingleBuffer ? "true" : "false");
+ mSyncTransaction = nullptr;
+ mAcquireSingleBuffer = false;
+ }
+
+ // abandon buffer queue
+ if (mBufferItemConsumer != nullptr) {
+ mBufferItemConsumer->abandon();
+ mBufferItemConsumer->setFrameAvailableListener(nullptr);
+ mBufferItemConsumer->setBufferFreedListener(nullptr);
+ mBufferItemConsumer->setBlastBufferQueue(nullptr);
+ }
+ mBufferItemConsumer = nullptr;
+ mConsumer = nullptr;
+ mProducer = nullptr;
+}
+
+bool BLASTBufferQueue::isSameSurfaceControl(const sp<SurfaceControl>& surfaceControl) const {
+ std::unique_lock _lock{mMutex};
+ return SurfaceControl::isSameSurface(mSurfaceControl, surfaceControl);
+}
+
} // namespace android
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index 8a2f392..8b2310d 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -82,6 +82,7 @@
return mProducer;
}
sp<Surface> getSurface(bool includeSurfaceControlHandle);
+ bool isSameSurfaceControl(const sp<SurfaceControl>& surfaceControl) const;
void onBufferFreed(const wp<GraphicBuffer>&/* graphicBuffer*/) override { /* TODO */ }
void onFrameReplaced(const BufferItem& item) override;
@@ -109,6 +110,7 @@
uint32_t getLastTransformHint() const;
uint64_t getLastAcquiredFrameNum();
+ void abandon();
virtual ~BLASTBufferQueue();
@@ -145,15 +147,15 @@
std::string mQueuedBufferTrace;
sp<SurfaceControl> mSurfaceControl;
- std::mutex mMutex;
+ mutable std::mutex mMutex;
std::condition_variable mCallbackCV;
// BufferQueue internally allows 1 more than
// the max to be acquired
int32_t mMaxAcquiredBuffers = 1;
- int32_t mNumFrameAvailable GUARDED_BY(mMutex);
- int32_t mNumAcquired GUARDED_BY(mMutex);
+ int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0;
+ int32_t mNumAcquired GUARDED_BY(mMutex) = 0;
// Keep a reference to the submitted buffers so we can release when surfaceflinger drops the
// buffer or the buffer has been presented and a new buffer is ready to be presented.
diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp
index 48b8621..42a32f3 100644
--- a/libs/gui/tests/BLASTBufferQueue_test.cpp
+++ b/libs/gui/tests/BLASTBufferQueue_test.cpp
@@ -1069,6 +1069,94 @@
checkScreenCapture(255, 0, 0, {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));
}
+// This test will currently fail because the old surfacecontrol will steal the last presented buffer
+// until the old surface control is destroyed. This is not necessarily a bug but to document a
+// limitation with the update API and to test any changes to make the api more robust. The current
+// approach for the client is to recreate the blastbufferqueue when the surfacecontrol updates.
+TEST_F(BLASTBufferQueueTest, DISABLED_DisconnectProducerTest) {
+ BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);
+ std::vector<sp<SurfaceControl>> surfaceControls;
+ sp<IGraphicBufferProducer> igbProducer;
+ for (int i = 0; i < 10; i++) {
+ sp<SurfaceControl> sc =
+ mClient->createSurface(String8("TestSurface"), mDisplayWidth, mDisplayHeight,
+ PIXEL_FORMAT_RGBA_8888,
+ ISurfaceComposerClient::eFXSurfaceBufferState,
+ /*parent*/ nullptr);
+ Transaction()
+ .setLayerStack(mSurfaceControl, ui::DEFAULT_LAYER_STACK)
+ .setLayer(mSurfaceControl, std::numeric_limits<int32_t>::max())
+ .show(mSurfaceControl)
+ .setDataspace(mSurfaceControl, ui::Dataspace::V0_SRGB)
+ .apply(true);
+ surfaceControls.push_back(sc);
+ adapter.update(sc, mDisplayWidth, mDisplayHeight);
+
+ setUpProducer(adapter, igbProducer);
+ Transaction next;
+ queueBuffer(igbProducer, 0, 255, 0, 0);
+ queueBuffer(igbProducer, 0, 0, 255, 0);
+ adapter.setSyncTransaction(&next, false);
+ queueBuffer(igbProducer, 255, 0, 0, 0);
+
+ CallbackHelper transactionCallback;
+ next.addTransactionCompletedCallback(transactionCallback.function,
+ transactionCallback.getContext())
+ .apply();
+
+ CallbackData callbackData;
+ transactionCallback.getCallbackData(&callbackData);
+ // capture screen and verify that it is red
+ ASSERT_EQ(NO_ERROR, captureDisplay(mCaptureArgs, mCaptureResults));
+ ASSERT_NO_FATAL_FAILURE(
+ checkScreenCapture(255, 0, 0,
+ {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));
+ igbProducer->disconnect(NATIVE_WINDOW_API_CPU);
+ }
+}
+
+// See DISABLED_DisconnectProducerTest
+TEST_F(BLASTBufferQueueTest, DISABLED_UpdateSurfaceControlTest) {
+ BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);
+ std::vector<sp<SurfaceControl>> surfaceControls;
+ sp<IGraphicBufferProducer> igbProducer;
+ for (int i = 0; i < 10; i++) {
+ sp<SurfaceControl> sc =
+ mClient->createSurface(String8("TestSurface"), mDisplayWidth, mDisplayHeight,
+ PIXEL_FORMAT_RGBA_8888,
+ ISurfaceComposerClient::eFXSurfaceBufferState,
+ /*parent*/ nullptr);
+ Transaction()
+ .setLayerStack(mSurfaceControl, ui::DEFAULT_LAYER_STACK)
+ .setLayer(mSurfaceControl, std::numeric_limits<int32_t>::max())
+ .show(mSurfaceControl)
+ .setDataspace(mSurfaceControl, ui::Dataspace::V0_SRGB)
+ .apply(true);
+ surfaceControls.push_back(sc);
+ adapter.update(sc, mDisplayWidth, mDisplayHeight);
+ setUpProducer(adapter, igbProducer);
+
+ Transaction next;
+ queueBuffer(igbProducer, 0, 255, 0, 0);
+ queueBuffer(igbProducer, 0, 0, 255, 0);
+ adapter.setSyncTransaction(&next, false);
+ queueBuffer(igbProducer, 255, 0, 0, 0);
+
+ CallbackHelper transactionCallback;
+ next.addTransactionCompletedCallback(transactionCallback.function,
+ transactionCallback.getContext())
+ .apply();
+
+ CallbackData callbackData;
+ transactionCallback.getCallbackData(&callbackData);
+ // capture screen and verify that it is red
+ ASSERT_EQ(NO_ERROR, captureDisplay(mCaptureArgs, mCaptureResults));
+ ASSERT_NO_FATAL_FAILURE(
+ checkScreenCapture(255, 0, 0,
+ {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));
+ }
+}
+
class TestProducerListener : public BnProducerListener {
public:
sp<IGraphicBufferProducer> mIgbp;