Invoke commit callback after latch buffers
If commit callback is invoked before the buffers can be latched, they
won't be included in the commit callback and would only be invoked in
the complete callback. This defeats the purpose of the commit callback
as an early indicator that the buffer has been committed.
Move the commit callback invocation so it's after latchBuffers
This also fixes a bug where offscreen layers could be latched in
flushAndCommitTransactions, so only offscreen layers would get the
commit callback. This would cause issues if the offscreen layer
transaction was merged with a layer that was on screen, preventing the
on screen layer from getting a commit callback.
Test: LayerCallbackTest#CommitCallbackOffscreenLayer
Test: BLASTBufferQueueTest
Test: BLASTBufferQueueTransformTest
Test: BLASTFrameEventHistoryTest
Fixes: 205563588
Change-Id: I5692211dd7717258829be8eb8a68b2dcb4a5498d
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index 3881620..4a63544 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -88,10 +88,10 @@
void onFrameDequeued(const uint64_t) override;
void onFrameCancelled(const uint64_t) override;
- virtual void transactionCommittedCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
- const std::vector<SurfaceControlStats>& stats);
- void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
- const std::vector<SurfaceControlStats>& stats);
+ void transactionCommittedCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
+ const std::vector<SurfaceControlStats>& stats);
+ virtual void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
+ const std::vector<SurfaceControlStats>& stats);
void releaseBufferCallback(const ReleaseCallbackId& id, const sp<Fence>& releaseFence,
std::optional<uint32_t> currentMaxAcquiredBufferCount);
void setNextTransaction(SurfaceComposerClient::Transaction *t);
diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp
index b2d5048..8607e1d 100644
--- a/libs/gui/tests/BLASTBufferQueue_test.cpp
+++ b/libs/gui/tests/BLASTBufferQueue_test.cpp
@@ -72,31 +72,30 @@
int height, int32_t format)
: BLASTBufferQueue(name, surface, width, height, format) {}
- void transactionCommittedCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
- const std::vector<SurfaceControlStats>& stats) override {
- BLASTBufferQueue::transactionCommittedCallback(latchTime, presentFence, stats);
-
+ void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
+ const std::vector<SurfaceControlStats>& stats) override {
+ BLASTBufferQueue::transactionCallback(latchTime, presentFence, stats);
uint64_t frameNumber = stats[0].frameEventStats.frameNumber;
{
std::unique_lock lock{frameNumberMutex};
- mLastTransactionCommittedFrameNumber = frameNumber;
- mCommittedCV.notify_all();
+ mLastTransactionFrameNumber = frameNumber;
+ mWaitForCallbackCV.notify_all();
}
}
void waitForCallback(int64_t frameNumber) {
std::unique_lock lock{frameNumberMutex};
// Wait until all but one of the submitted buffers have been released.
- while (mLastTransactionCommittedFrameNumber < frameNumber) {
- mCommittedCV.wait(lock);
+ while (mLastTransactionFrameNumber < frameNumber) {
+ mWaitForCallbackCV.wait(lock);
}
}
private:
std::mutex frameNumberMutex;
- std::condition_variable mCommittedCV;
- int64_t mLastTransactionCommittedFrameNumber = -1;
+ std::condition_variable mWaitForCallbackCV;
+ int64_t mLastTransactionFrameNumber = -1;
};
class BLASTBufferQueueHelper {
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index d0f2174..98f2107 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1971,9 +1971,34 @@
mFrameTimeline->setSfWakeUp(vsyncId, frameTime, Fps::fromPeriodNsecs(stats.vsyncPeriod));
- mustComposite |= flushAndCommitTransactions();
+ bool needsTraversal = false;
+ if (clearTransactionFlags(eTransactionFlushNeeded)) {
+ needsTraversal = flushTransactionQueues();
+ }
+
+ const bool shouldCommit =
+ (getTransactionFlags() & ~eTransactionFlushNeeded) || needsTraversal;
+ if (shouldCommit) {
+ commitTransactions();
+ }
+
+ if (transactionFlushNeeded()) {
+ setTransactionFlags(eTransactionFlushNeeded);
+ }
+
+ mustComposite |= shouldCommit;
mustComposite |= latchBuffers();
+ // This has to be called after latchBuffers because we want to include the layers that have
+ // been latched in the commit callback
+ if (!needsTraversal) {
+ // Invoke empty transaction callbacks early.
+ mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);
+ } else {
+ // Invoke OnCommit callbacks.
+ mTransactionCallbackInvoker.sendCallbacks(true /* onCommitOnly */);
+ }
+
updateLayerGeometry();
if (tracePreComposition) {
@@ -2000,34 +2025,6 @@
return mustComposite && CC_LIKELY(mBootStage != BootStage::BOOTLOADER);
}
-bool SurfaceFlinger::flushAndCommitTransactions() {
- ATRACE_CALL();
-
- bool needsTraversal = false;
- if (clearTransactionFlags(eTransactionFlushNeeded)) {
- needsTraversal = flushTransactionQueues();
- }
-
- const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || needsTraversal;
- if (shouldCommit) {
- commitTransactions();
- }
-
- if (!needsTraversal) {
- // Invoke empty transaction callbacks early.
- mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);
- } else {
- // Invoke OnCommit callbacks.
- mTransactionCallbackInvoker.sendCallbacks(true /* onCommitOnly */);
- }
-
- if (transactionFlushNeeded()) {
- setTransactionFlags(eTransactionFlushNeeded);
- }
-
- return shouldCommit;
-}
-
void SurfaceFlinger::composite(nsecs_t frameTime) {
ATRACE_CALL();
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index bf628dc..5c85382 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -684,9 +684,6 @@
const std::optional<scheduler::RefreshRateConfigs::Policy>& policy, bool overridePolicy)
EXCLUDES(mStateLock);
- // Returns whether transactions were committed.
- bool flushAndCommitTransactions() EXCLUDES(mStateLock);
-
void commitTransactions() EXCLUDES(mStateLock);
void commitTransactionsLocked(uint32_t transactionFlags) REQUIRES(mStateLock);
void doCommitTransactions() REQUIRES(mStateLock);
diff --git a/services/surfaceflinger/tests/LayerCallback_test.cpp b/services/surfaceflinger/tests/LayerCallback_test.cpp
index 91a5b52..5c16fee 100644
--- a/services/surfaceflinger/tests/LayerCallback_test.cpp
+++ b/services/surfaceflinger/tests/LayerCallback_test.cpp
@@ -26,6 +26,7 @@
namespace android {
using android::hardware::graphics::common::V1_1::BufferUsage;
+using SCHash = SurfaceComposerClient::SCHash;
::testing::Environment* const binderEnv =
::testing::AddGlobalTestEnvironment(new BinderEnvironment());
@@ -102,6 +103,24 @@
}
}
+ static void waitForCommitCallback(
+ CallbackHelper& helper,
+ const std::unordered_set<sp<SurfaceControl>, SCHash>& committedSc) {
+ CallbackData callbackData;
+ ASSERT_NO_FATAL_FAILURE(helper.getCallbackData(&callbackData));
+
+ const auto& surfaceControlStats = callbackData.surfaceControlStats;
+
+ ASSERT_EQ(surfaceControlStats.size(), committedSc.size()) << "wrong number of surfaces";
+
+ for (const auto& stats : surfaceControlStats) {
+ ASSERT_NE(stats.surfaceControl, nullptr) << "returned null surface control";
+
+ const auto& expectedSc = committedSc.find(stats.surfaceControl);
+ ASSERT_NE(expectedSc, committedSc.end()) << "unexpected surface control";
+ }
+ }
+
DisplayEventReceiver mDisplayEventReceiver;
int mEpollFd;
@@ -1085,4 +1104,29 @@
EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true));
}
+TEST_F(LayerCallbackTest, CommitCallbackOffscreenLayer) {
+ sp<SurfaceControl> layer;
+ ASSERT_NO_FATAL_FAILURE(layer = createBufferStateLayer());
+ sp<SurfaceControl> offscreenLayer =
+ createSurface(mClient, "Offscreen Layer", 0, 0, PIXEL_FORMAT_RGBA_8888,
+ ISurfaceComposerClient::eFXSurfaceBufferState, layer.get());
+
+ Transaction transaction;
+ CallbackHelper callback;
+ int err = fillTransaction(transaction, &callback, layer, true);
+ err |= fillTransaction(transaction, &callback, offscreenLayer, true);
+ if (err) {
+ GTEST_SUCCEED() << "test not supported";
+ return;
+ }
+
+ transaction.reparent(offscreenLayer, nullptr)
+ .addTransactionCommittedCallback(callback.function, callback.getContext());
+ transaction.apply();
+
+ std::unordered_set<sp<SurfaceControl>, SCHash> committedSc;
+ committedSc.insert(layer);
+ committedSc.insert(offscreenLayer);
+ EXPECT_NO_FATAL_FAILURE(waitForCommitCallback(callback, committedSc));
+}
} // namespace android