SF: change acquired buffers based on the current refresh rate
BLASTBufferQueue set the max acquired buffers based on SF
vsync configuration (sf.duration and app.duration). This is calculated
based on the max supported refresh rate on the device, and it turn
is propogated to apps via min_undequeued_buffers to the app could
allocate enough buffers for maintaining the pipeline SF expects.
This leads to a higher latency when the device is running in a lower
refresh rate as there are more buffers on the buffer queue then required.
In this change we are holding on the these "extra buffers" and not
releasing them back to the buffer queue so the app would use the number
of buffers it needs based on the current refresh rate, and to avoid having
unnecessary long latency.
Bug: 188553729
Test: Run backpressure based game on 60Hz and 120Hz and collect systraces
Change-Id: I9d4e6278f0ddd28008ac437ab0576aa79d05166a
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 8024482..52d3fa3 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -159,9 +159,7 @@
mBufferItemConsumer->setDefaultBufferFormat(convertBufferFormat(format));
mBufferItemConsumer->setBlastBufferQueue(this);
- int extraBufferCount = 0;
- ComposerService::getComposerService()->getExtraBufferCount(&extraBufferCount);
- mMaxAcquiredBuffers = 1 + extraBufferCount;
+ ComposerService::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers);
mBufferItemConsumer->setMaxAcquiredBufferCount(mMaxAcquiredBuffers);
mTransformHint = mSurfaceControl->getTransformHint();
@@ -308,18 +306,20 @@
// So we pass in a weak pointer to the BBQ and if it still alive, then we release the buffer.
// Otherwise, this is a no-op.
static void releaseBufferCallbackThunk(wp<BLASTBufferQueue> context, uint64_t graphicBufferId,
- const sp<Fence>& releaseFence, uint32_t transformHint) {
+ const sp<Fence>& releaseFence, uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) {
sp<BLASTBufferQueue> blastBufferQueue = context.promote();
ALOGV("releaseBufferCallbackThunk graphicBufferId=%" PRIu64 " blastBufferQueue=%s",
graphicBufferId, blastBufferQueue ? "alive" : "dead");
if (blastBufferQueue) {
- blastBufferQueue->releaseBufferCallback(graphicBufferId, releaseFence, transformHint);
+ blastBufferQueue->releaseBufferCallback(graphicBufferId, releaseFence, transformHint,
+ currentMaxAcquiredBufferCount);
}
}
void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId,
- const sp<Fence>& releaseFence,
- uint32_t transformHint) {
+ const sp<Fence>& releaseFence, uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) {
ATRACE_CALL();
std::unique_lock _lock{mMutex};
BQA_LOGV("releaseBufferCallback graphicBufferId=%" PRIu64, graphicBufferId);
@@ -330,15 +330,36 @@
mBufferItemConsumer->setTransformHint(mTransformHint);
}
- auto it = mSubmitted.find(graphicBufferId);
- if (it == mSubmitted.end()) {
- BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %" PRIu64,
- graphicBufferId);
- return;
+ // Calculate how many buffers we need to hold before we release them back
+ // to the buffer queue. This will prevent higher latency when we are running
+ // on a lower refresh rate than the max supported. We only do that for EGL
+ // clients as others don't care about latency
+ const bool isEGL = [&] {
+ const auto it = mSubmitted.find(graphicBufferId);
+ return it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL;
+ }();
+
+ const auto numPendingBuffersToHold =
+ isEGL ? std::max(0u, mMaxAcquiredBuffers - currentMaxAcquiredBufferCount) : 0;
+ mPendingRelease.emplace_back(ReleasedBuffer{graphicBufferId, releaseFence});
+
+ // Release all buffers that are beyond the ones that we need to hold
+ while (mPendingRelease.size() > numPendingBuffersToHold) {
+ const auto releaseBuffer = mPendingRelease.front();
+ mPendingRelease.pop_front();
+ auto it = mSubmitted.find(releaseBuffer.bufferId);
+ if (it == mSubmitted.end()) {
+ BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %" PRIu64,
+ graphicBufferId);
+ return;
+ }
+
+ mBufferItemConsumer->releaseBuffer(it->second, releaseBuffer.releaseFence);
+ mSubmitted.erase(it);
}
- mBufferItemConsumer->releaseBuffer(it->second, releaseFence);
- mSubmitted.erase(it);
+ ATRACE_INT("PendingRelease", mPendingRelease.size());
+
mNumAcquired--;
processNextBufferLocked(false /* useNextTransaction */);
mCallbackCV.notify_all();
@@ -420,7 +441,8 @@
auto releaseBufferCallback =
std::bind(releaseBufferCallbackThunk, wp<BLASTBufferQueue>(this) /* callbackContext */,
- std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);
+ std::placeholders::_1, std::placeholders::_2, std::placeholders::_3,
+ std::placeholders::_4);
t->setBuffer(mSurfaceControl, buffer, releaseBufferCallback);
t->setDataspace(mSurfaceControl, static_cast<ui::Dataspace>(bufferItem.mDataSpace));
t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata);
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index 71e18a9..0d7795e 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -1215,16 +1215,17 @@
return reply.readInt32();
}
- status_t getExtraBufferCount(int* extraBuffers) const override {
+ status_t getMaxAcquiredBufferCount(int* buffers) const override {
Parcel data, reply;
data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());
- status_t err = remote()->transact(BnSurfaceComposer::GET_EXTRA_BUFFER_COUNT, data, &reply);
+ status_t err =
+ remote()->transact(BnSurfaceComposer::GET_MAX_ACQUIRED_BUFFER_COUNT, data, &reply);
if (err != NO_ERROR) {
- ALOGE("getExtraBufferCount failed to read data: %s (%d)", strerror(-err), err);
+ ALOGE("getMaxAcquiredBufferCount failed to read data: %s (%d)", strerror(-err), err);
return err;
}
- return reply.readInt32(extraBuffers);
+ return reply.readInt32(buffers);
}
};
@@ -2069,14 +2070,14 @@
SAFE_PARCEL(reply->writeInt32, priority);
return NO_ERROR;
}
- case GET_EXTRA_BUFFER_COUNT: {
+ case GET_MAX_ACQUIRED_BUFFER_COUNT: {
CHECK_INTERFACE(ISurfaceComposer, data, reply);
- int extraBuffers = 0;
- int err = getExtraBufferCount(&extraBuffers);
+ int buffers = 0;
+ int err = getMaxAcquiredBufferCount(&buffers);
if (err != NO_ERROR) {
return err;
}
- SAFE_PARCEL(reply->writeInt32, extraBuffers);
+ SAFE_PARCEL(reply->writeInt32, buffers);
return NO_ERROR;
}
case OVERRIDE_HDR_TYPES: {
diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp
index 63d07ba..17499ec 100644
--- a/libs/gui/ITransactionCompletedListener.cpp
+++ b/libs/gui/ITransactionCompletedListener.cpp
@@ -119,6 +119,7 @@
SAFE_PARCEL(output->writeBool, false);
}
SAFE_PARCEL(output->writeUint32, transformHint);
+ SAFE_PARCEL(output->writeUint32, currentMaxAcquiredBufferCount);
SAFE_PARCEL(output->writeParcelable, eventStats);
SAFE_PARCEL(output->writeInt32, static_cast<int32_t>(jankData.size()));
for (const auto& data : jankData) {
@@ -138,6 +139,7 @@
SAFE_PARCEL(input->read, *previousReleaseFence);
}
SAFE_PARCEL(input->readUint32, &transformHint);
+ SAFE_PARCEL(input->readUint32, ¤tMaxAcquiredBufferCount);
SAFE_PARCEL(input->readParcelable, &eventStats);
int32_t jankData_size = 0;
@@ -251,11 +253,13 @@
stats);
}
- void onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence,
- uint32_t transformHint) override {
+ void onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence, uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) override {
callRemoteAsync<decltype(
&ITransactionCompletedListener::onReleaseBuffer)>(Tag::ON_RELEASE_BUFFER,
- graphicBufferId, releaseFence, transformHint);
+ graphicBufferId, releaseFence,
+ transformHint,
+ currentMaxAcquiredBufferCount);
}
};
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 660c5bd..c69435d 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -328,7 +328,8 @@
surfaceStats.previousReleaseFence
? surfaceStats.previousReleaseFence
: Fence::NO_FENCE,
- surfaceStats.transformHint);
+ surfaceStats.transformHint,
+ surfaceStats.currentMaxAcquiredBufferCount);
}
}
}
@@ -364,9 +365,9 @@
}
}
-void TransactionCompletedListener::onReleaseBuffer(uint64_t graphicBufferId,
- sp<Fence> releaseFence,
- uint32_t transformHint) {
+void TransactionCompletedListener::onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence,
+ uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) {
ReleaseBufferCallback callback;
{
std::scoped_lock<std::mutex> lock(mMutex);
@@ -376,7 +377,7 @@
ALOGE("Could not call release buffer callback, buffer not found %" PRIu64, graphicBufferId);
return;
}
- callback(graphicBufferId, releaseFence, transformHint);
+ callback(graphicBufferId, releaseFence, transformHint, currentMaxAcquiredBufferCount);
}
ReleaseBufferCallback TransactionCompletedListener::popReleaseBufferCallbackLocked(
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index d63425e..0981e76 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -90,7 +90,7 @@
void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
const std::vector<SurfaceControlStats>& stats);
void releaseBufferCallback(uint64_t graphicBufferId, const sp<Fence>& releaseFence,
- uint32_t transformHint);
+ uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount);
void setNextTransaction(SurfaceComposerClient::Transaction *t);
void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber);
void setTransactionCompleteCallback(uint64_t frameNumber,
@@ -141,6 +141,15 @@
// buffer or the buffer has been presented and a new buffer is ready to be presented.
std::unordered_map<uint64_t /* bufferId */, BufferItem> mSubmitted GUARDED_BY(mMutex);
+ // Keep a queue of the released buffers instead of immediately releasing
+ // the buffers back to the buffer queue. This would be controlled by SF
+ // setting the max acquired buffer count.
+ struct ReleasedBuffer {
+ uint64_t bufferId;
+ sp<Fence> releaseFence;
+ };
+ std::deque<ReleasedBuffer> mPendingRelease GUARDED_BY(mMutex);
+
ui::Size mSize GUARDED_BY(mMutex);
ui::Size mRequestedSize GUARDED_BY(mMutex);
int32_t mFormat GUARDED_BY(mMutex);
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index 439d90a..2a3f6a4 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -537,22 +537,21 @@
virtual int getGPUContextPriority() = 0;
/**
- * Gets the extra buffers a client would need to allocate if it passes
- * the Choreographer#getVsyncId with its buffers.
- *
- * When Choreographer#getVsyncId is passed to SurfaceFlinger, it is used
- * as an indication of when to latch the buffer. SurfaceFlinger will make
- * sure that it will give the app at least the time configured as the
- * 'appDuration' before trying to latch the buffer.
+ * Gets the number of buffers SurfaceFlinger would need acquire. This number
+ * would be propagated to the client via MIN_UNDEQUEUED_BUFFERS so that the
+ * client could allocate enough buffers to match SF expectations of the
+ * pipeline depth. SurfaceFlinger will make sure that it will give the app at
+ * least the time configured as the 'appDuration' before trying to latch
+ * the buffer.
*
* The total buffers needed for a given configuration is basically the
* numbers of vsyncs a single buffer is used across the stack. For the default
* configuration a buffer is held ~1 vsync by the app, ~1 vsync by SurfaceFlinger
* and 1 vsync by the display. The extra buffers are calculated as the
- * number of additional buffers on top of the 3 buffers already allocated
- * by the app.
+ * number of additional buffers on top of the 2 buffers already present
+ * in MIN_UNDEQUEUED_BUFFERS.
*/
- virtual status_t getExtraBufferCount(int* extraBuffers) const = 0;
+ virtual status_t getMaxAcquiredBufferCount(int* buffers) const = 0;
};
// ----------------------------------------------------------------------------
@@ -615,7 +614,7 @@
SET_FRAME_TIMELINE_INFO,
ADD_TRANSACTION_TRACE_LISTENER,
GET_GPU_CONTEXT_PRIORITY,
- GET_EXTRA_BUFFER_COUNT,
+ GET_MAX_ACQUIRED_BUFFER_COUNT,
GET_DYNAMIC_DISPLAY_INFO,
ADD_FPS_LISTENER,
REMOVE_FPS_LISTENER,
diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h
index 3bfeef1..d286c34 100644
--- a/libs/gui/include/gui/ITransactionCompletedListener.h
+++ b/libs/gui/include/gui/ITransactionCompletedListener.h
@@ -101,12 +101,14 @@
SurfaceStats() = default;
SurfaceStats(const sp<IBinder>& sc, nsecs_t time, const sp<Fence>& prevReleaseFence,
- uint32_t hint, FrameEventHistoryStats frameEventStats,
- std::vector<JankData> jankData, uint64_t previousBufferId)
+ uint32_t hint, uint32_t currentMaxAcquiredBuffersCount,
+ FrameEventHistoryStats frameEventStats, std::vector<JankData> jankData,
+ uint64_t previousBufferId)
: surfaceControl(sc),
acquireTime(time),
previousReleaseFence(prevReleaseFence),
transformHint(hint),
+ currentMaxAcquiredBufferCount(currentMaxAcquiredBuffersCount),
eventStats(frameEventStats),
jankData(std::move(jankData)),
previousBufferId(previousBufferId) {}
@@ -115,6 +117,7 @@
nsecs_t acquireTime = -1;
sp<Fence> previousReleaseFence;
uint32_t transformHint = 0;
+ uint32_t currentMaxAcquiredBufferCount = 0;
FrameEventHistoryStats eventStats;
std::vector<JankData> jankData;
uint64_t previousBufferId;
@@ -159,7 +162,8 @@
virtual void onTransactionCompleted(ListenerStats stats) = 0;
virtual void onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence,
- uint32_t transformHint) = 0;
+ uint32_t transformHint,
+ uint32_t currentMaxAcquiredBufferCount) = 0;
};
class BnTransactionCompletedListener : public SafeBnInterface<ITransactionCompletedListener> {
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 5aa132c..fa91bfa 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -83,7 +83,7 @@
const std::vector<SurfaceControlStats>& /*stats*/)>;
using ReleaseBufferCallback =
std::function<void(uint64_t /* graphicsBufferId */, const sp<Fence>& /*releaseFence*/,
- uint32_t transformHint)>;
+ uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount)>;
using SurfaceStatsCallback =
std::function<void(void* /*context*/, nsecs_t /*latchTime*/,
@@ -729,7 +729,7 @@
// BnTransactionCompletedListener overrides
void onTransactionCompleted(ListenerStats stats) override;
void onReleaseBuffer(uint64_t /* graphicsBufferId */, sp<Fence> releaseFence,
- uint32_t transformHint) override;
+ uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) override;
private:
ReleaseBufferCallback popReleaseBufferCallbackLocked(uint64_t /* graphicsBufferId */);
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index b8d34c3..59b0c04 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -902,7 +902,7 @@
int getGPUContextPriority() override { return 0; };
- status_t getExtraBufferCount(int* /*extraBuffers*/) const override { return NO_ERROR; }
+ status_t getMaxAcquiredBufferCount(int* /*buffers*/) const override { return NO_ERROR; }
protected:
IBinder* onAsBinder() override { return nullptr; }