BufferQueue: track buffer-queue by instance vs. by reference
Instead of representing the buffer-queue as a vector of buffer
indices, represent them as a vector of BufferItems (copies).
This allows modifying the buffer slots independent of the queued
buffers.
As part of this change, BufferSlot properties that are only
been relevant in the buffer-queue have been removed.
Also, invalid scalingMode in queueBuffer now returns an error.
ConsumerBase has also changed to allow reuse of the same
buffer slots by different buffers.
Change-Id: If2a698fa142b67c69ad41b8eaca6e127eb3ef75b
Signed-off-by: Lajos Molnar <lajos@google.com>
Related-to-bug: 7093648
diff --git a/libs/gui/BufferItemConsumer.cpp b/libs/gui/BufferItemConsumer.cpp
index 7db1b84..ba04bdf 100644
--- a/libs/gui/BufferItemConsumer.cpp
+++ b/libs/gui/BufferItemConsumer.cpp
@@ -82,9 +82,9 @@
Mutex::Autolock _l(mMutex);
- err = addReleaseFenceLocked(item.mBuf, releaseFence);
+ err = addReleaseFenceLocked(item.mBuf, item.mGraphicBuffer, releaseFence);
- err = releaseBufferLocked(item.mBuf, EGL_NO_DISPLAY,
+ err = releaseBufferLocked(item.mBuf, item.mGraphicBuffer, EGL_NO_DISPLAY,
EGL_NO_SYNC_KHR);
if (err != OK) {
BI_LOGE("Failed to release buffer: %s (%d)",
diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp
index 942151f..34dbd71 100644
--- a/libs/gui/BufferQueue.cpp
+++ b/libs/gui/BufferQueue.cpp
@@ -418,6 +418,7 @@
return NO_INIT;
}
+ mSlots[*outBuf].mFrameNumber = ~0;
mSlots[*outBuf].mGraphicBuffer = graphicBuffer;
}
}
@@ -435,7 +436,8 @@
eglDestroySyncKHR(dpy, eglFence);
}
- ST_LOGV("dequeueBuffer: returning slot=%d buf=%p flags=%#x", *outBuf,
+ ST_LOGV("dequeueBuffer: returning slot=%d/%llu buf=%p flags=%#x", *outBuf,
+ mSlots[*outBuf].mFrameNumber,
mSlots[*outBuf].mGraphicBuffer->handle, returnFlags);
return returnFlags;
@@ -491,15 +493,22 @@
return BAD_VALUE;
}
- ST_LOGV("queueBuffer: slot=%d time=%#llx crop=[%d,%d,%d,%d] tr=%#x "
- "scale=%s",
- buf, timestamp, crop.left, crop.top, crop.right, crop.bottom,
- transform, scalingModeName(scalingMode));
+ switch (scalingMode) {
+ case NATIVE_WINDOW_SCALING_MODE_FREEZE:
+ case NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW:
+ case NATIVE_WINDOW_SCALING_MODE_SCALE_CROP:
+ case NATIVE_WINDOW_SCALING_MODE_NO_SCALE_CROP:
+ break;
+ default:
+ ST_LOGE("unknown scaling mode: %d", scalingMode);
+ return -EINVAL;
+ }
sp<ConsumerListener> listener;
{ // scope for the lock
Mutex::Autolock lock(mMutex);
+
if (mAbandoned) {
ST_LOGE("queueBuffer: BufferQueue has been abandoned!");
return NO_INIT;
@@ -519,6 +528,12 @@
return -EINVAL;
}
+ ST_LOGV("queueBuffer: slot=%d/%llu time=%#llx crop=[%d,%d,%d,%d] "
+ "tr=%#x scale=%s",
+ buf, mFrameCounter + 1, timestamp,
+ crop.left, crop.top, crop.right, crop.bottom,
+ transform, scalingModeName(scalingMode));
+
const sp<GraphicBuffer>& graphicBuffer(mSlots[buf].mGraphicBuffer);
Rect bufferRect(graphicBuffer->getWidth(), graphicBuffer->getHeight());
Rect croppedCrop;
@@ -529,9 +544,25 @@
return -EINVAL;
}
+ mSlots[buf].mFence = fence;
+ mSlots[buf].mBufferState = BufferSlot::QUEUED;
+ mFrameCounter++;
+ mSlots[buf].mFrameNumber = mFrameCounter;
+
+ BufferItem item;
+ item.mAcquireCalled = mSlots[buf].mAcquireCalled;
+ item.mGraphicBuffer = mSlots[buf].mGraphicBuffer;
+ item.mCrop = crop;
+ item.mTransform = transform;
+ item.mScalingMode = scalingMode;
+ item.mTimestamp = timestamp;
+ item.mFrameNumber = mFrameCounter;
+ item.mBuf = buf;
+ item.mFence = fence;
+
if (mSynchronousMode) {
// In synchronous mode we queue all buffers in a FIFO.
- mQueue.push_back(buf);
+ mQueue.push_back(item);
// Synchronous mode always signals that an additional frame should
// be consumed.
@@ -539,7 +570,7 @@
} else {
// In asynchronous mode we only keep the most recent buffer.
if (mQueue.empty()) {
- mQueue.push_back(buf);
+ mQueue.push_back(item);
// Asynchronous mode only signals that a frame should be
// consumed if no previous frame was pending. If a frame were
@@ -547,34 +578,15 @@
listener = mConsumerListener;
} else {
Fifo::iterator front(mQueue.begin());
- // buffer currently queued is freed
- mSlots[*front].mBufferState = BufferSlot::FREE;
+ // buffer slot currently queued is marked free if still tracked
+ if (stillTracking(front)) {
+ mSlots[front->mBuf].mBufferState = BufferSlot::FREE;
+ }
// and we record the new buffer index in the queued list
- *front = buf;
+ *front = item;
}
}
- mSlots[buf].mTimestamp = timestamp;
- mSlots[buf].mCrop = crop;
- mSlots[buf].mTransform = transform;
- mSlots[buf].mFence = fence;
-
- switch (scalingMode) {
- case NATIVE_WINDOW_SCALING_MODE_FREEZE:
- case NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW:
- case NATIVE_WINDOW_SCALING_MODE_SCALE_CROP:
- break;
- default:
- ST_LOGE("unknown scaling mode: %d (ignoring)", scalingMode);
- scalingMode = mSlots[buf].mScalingMode;
- break;
- }
-
- mSlots[buf].mBufferState = BufferSlot::QUEUED;
- mSlots[buf].mScalingMode = scalingMode;
- mFrameCounter++;
- mSlots[buf].mFrameNumber = mFrameCounter;
-
mBufferHasBeenQueued = true;
mDequeueCondition.broadcast();
@@ -718,7 +730,14 @@
int fifoSize = 0;
Fifo::const_iterator i(mQueue.begin());
while (i != mQueue.end()) {
- fifo.appendFormat("%02d ", *i++);
+ fifo.appendFormat("%02d:%p crop=[%d,%d,%d,%d], "
+ "xform=0x%02x, time=%#llx, scale=%s\n",
+ i->mBuf, i->mGraphicBuffer.get(),
+ i->mCrop.left, i->mCrop.top, i->mCrop.right,
+ i->mCrop.bottom, i->mTransform, i->mTimestamp,
+ scalingModeName(i->mScalingMode)
+ );
+ i++;
fifoSize++;
}
@@ -746,14 +765,10 @@
for (int i=0 ; i<maxBufferCount ; i++) {
const BufferSlot& slot(mSlots[i]);
result.appendFormat(
- "%s%s[%02d] "
- "state=%-8s, crop=[%d,%d,%d,%d], "
- "xform=0x%02x, time=%#llx, scale=%s",
+ "%s%s[%02d:%p] state=%-8s",
prefix, (slot.mBufferState == BufferSlot::ACQUIRED)?">":" ", i,
- stateName(slot.mBufferState),
- slot.mCrop.left, slot.mCrop.top, slot.mCrop.right,
- slot.mCrop.bottom, slot.mTransform, slot.mTimestamp,
- scalingModeName(slot.mScalingMode)
+ slot.mGraphicBuffer.get(),
+ stateName(slot.mBufferState)
);
const sp<GraphicBuffer>& buf(slot.mGraphicBuffer);
@@ -820,27 +835,27 @@
// deep, while in synchronous mode we use the oldest buffer.
if (!mQueue.empty()) {
Fifo::iterator front(mQueue.begin());
- int buf = *front;
-
+ int buf = front->mBuf;
+ *buffer = *front;
ATRACE_BUFFER_INDEX(buf);
- if (mSlots[buf].mAcquireCalled) {
- buffer->mGraphicBuffer = NULL;
- } else {
- buffer->mGraphicBuffer = mSlots[buf].mGraphicBuffer;
+ ST_LOGV("acquireBuffer: acquiring { slot=%d/%llu, buffer=%p }",
+ front->mBuf, front->mFrameNumber,
+ front->mGraphicBuffer->handle);
+ // if front buffer still being tracked update slot state
+ if (stillTracking(front)) {
+ mSlots[buf].mAcquireCalled = true;
+ mSlots[buf].mNeedsCleanupOnRelease = false;
+ mSlots[buf].mBufferState = BufferSlot::ACQUIRED;
+ mSlots[buf].mFence = Fence::NO_FENCE;
}
- buffer->mCrop = mSlots[buf].mCrop;
- buffer->mTransform = mSlots[buf].mTransform;
- buffer->mScalingMode = mSlots[buf].mScalingMode;
- buffer->mFrameNumber = mSlots[buf].mFrameNumber;
- buffer->mTimestamp = mSlots[buf].mTimestamp;
- buffer->mBuf = buf;
- buffer->mFence = mSlots[buf].mFence;
- mSlots[buf].mAcquireCalled = true;
- mSlots[buf].mNeedsCleanupOnRelease = false;
- mSlots[buf].mBufferState = BufferSlot::ACQUIRED;
- mSlots[buf].mFence = Fence::NO_FENCE;
+ // If the buffer has previously been acquired by the consumer, set
+ // mGraphicBuffer to NULL to avoid unnecessarily remapping this
+ // buffer on the consumer side.
+ if (buffer->mAcquireCalled) {
+ buffer->mGraphicBuffer = NULL;
+ }
mQueue.erase(front);
mDequeueCondition.broadcast();
@@ -853,7 +868,8 @@
return NO_ERROR;
}
-status_t BufferQueue::releaseBuffer(int buf, EGLDisplay display,
+status_t BufferQueue::releaseBuffer(
+ int buf, uint64_t frameNumber, EGLDisplay display,
EGLSyncKHR eglFence, const sp<Fence>& fence) {
ATRACE_CALL();
ATRACE_BUFFER_INDEX(buf);
@@ -864,12 +880,35 @@
return BAD_VALUE;
}
- mSlots[buf].mEglDisplay = display;
- mSlots[buf].mEglFence = eglFence;
- mSlots[buf].mFence = fence;
+ // Check if this buffer slot is on the queue
+ bool slotQueued = false;
+ Fifo::iterator front(mQueue.begin());
+ while (front != mQueue.end() && !slotQueued) {
+ if (front->mBuf == buf)
+ slotQueued = true;
+ front++;
+ }
+
+ // If the frame number has changed because buffer has been reallocated,
+ // we can ignore this releaseBuffer for the old buffer.
+ if (frameNumber != mSlots[buf].mFrameNumber) {
+ // This should only occur if new buffer is still in the queue
+ ALOGE_IF(!slotQueued,
+ "received old buffer(#%lld) after new buffer(#%lld) on same "
+ "slot #%d already acquired", frameNumber,
+ mSlots[buf].mFrameNumber, buf);
+ return STALE_BUFFER_SLOT;
+ }
+ // this should never happen
+ ALOGE_IF(slotQueued,
+ "received new buffer(#%lld) on slot #%d that has not yet been "
+ "acquired", frameNumber, buf);
// The buffer can now only be released if its in the acquired state
if (mSlots[buf].mBufferState == BufferSlot::ACQUIRED) {
+ mSlots[buf].mEglDisplay = display;
+ mSlots[buf].mEglFence = eglFence;
+ mSlots[buf].mFence = fence;
mSlots[buf].mBufferState = BufferSlot::FREE;
} else if (mSlots[buf].mNeedsCleanupOnRelease) {
ST_LOGV("releasing a stale buf %d its state was %d", buf, mSlots[buf].mBufferState);
@@ -934,6 +973,17 @@
mask |= 1 << i;
}
}
+
+ // Remove buffers in flight (on the queue) from the mask where acquire has
+ // been called, as the consumer will not receive the buffer address, so
+ // it should not free these slots.
+ Fifo::iterator front(mQueue.begin());
+ while (front != mQueue.end()) {
+ if (front->mAcquireCalled)
+ mask &= ~(1 << front->mBuf);
+ front++;
+ }
+
*slotMask = mask;
ST_LOGV("getReleasedBuffers: returning mask %#x", mask);
@@ -977,16 +1027,14 @@
}
void BufferQueue::freeAllBuffersExceptHeadLocked() {
- int head = -1;
- if (!mQueue.empty()) {
- Fifo::iterator front(mQueue.begin());
- head = *front;
- }
+ // only called if mQueue is not empty
+ Fifo::iterator front(mQueue.begin());
mBufferHasBeenQueued = false;
for (int i = 0; i < NUM_BUFFER_SLOTS; i++) {
- if (i != head) {
+ const BufferSlot &slot = mSlots[i];
+ if (slot.mGraphicBuffer == NULL ||
+ slot.mGraphicBuffer->handle != front->mGraphicBuffer->handle)
freeBufferLocked(i);
- }
}
}
@@ -1052,6 +1100,22 @@
return maxBufferCount;
}
+bool BufferQueue::stillTracking(const BufferItem *item) const {
+ const BufferSlot &slot = mSlots[item->mBuf];
+
+ ST_LOGV("stillTracking?: item: { slot=%d/%llu, buffer=%p }, "
+ "slot: { slot=%d/%llu, buffer=%p }",
+ item->mBuf, item->mFrameNumber,
+ (item->mGraphicBuffer.get() ? item->mGraphicBuffer->handle : 0),
+ item->mBuf, slot.mFrameNumber,
+ (slot.mGraphicBuffer.get() ? slot.mGraphicBuffer->handle : 0));
+
+ // Compare item with its original buffer slot. We can check the slot
+ // as the buffer would not be moved to a different slot by the producer.
+ return (slot.mGraphicBuffer != NULL &&
+ item->mGraphicBuffer->handle == slot.mGraphicBuffer->handle);
+}
+
BufferQueue::ProxyConsumerListener::ProxyConsumerListener(
const wp<BufferQueue::ConsumerListener>& consumerListener):
mConsumerListener(consumerListener) {}
diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index 8d911c9..fd9d153 100644
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -95,6 +95,7 @@
CB_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
mSlots[slotIndex].mGraphicBuffer = 0;
mSlots[slotIndex].mFence = Fence::NO_FENCE;
+ mSlots[slotIndex].mFrameNumber = 0;
}
// Used for refactoring, should not be in final interface
@@ -191,21 +192,31 @@
mSlots[item->mBuf].mGraphicBuffer = item->mGraphicBuffer;
}
+ mSlots[item->mBuf].mFrameNumber = item->mFrameNumber;
mSlots[item->mBuf].mFence = item->mFence;
- CB_LOGV("acquireBufferLocked: -> slot=%d", item->mBuf);
+ CB_LOGV("acquireBufferLocked: -> slot=%d/%llu",
+ item->mBuf, item->mFrameNumber);
return OK;
}
-status_t ConsumerBase::addReleaseFence(int slot, const sp<Fence>& fence) {
+status_t ConsumerBase::addReleaseFence(int slot,
+ const sp<GraphicBuffer> graphicBuffer, const sp<Fence>& fence) {
Mutex::Autolock lock(mMutex);
- return addReleaseFenceLocked(slot, fence);
+ return addReleaseFenceLocked(slot, graphicBuffer, fence);
}
-status_t ConsumerBase::addReleaseFenceLocked(int slot, const sp<Fence>& fence) {
+status_t ConsumerBase::addReleaseFenceLocked(int slot,
+ const sp<GraphicBuffer> graphicBuffer, const sp<Fence>& fence) {
CB_LOGV("addReleaseFenceLocked: slot=%d", slot);
+ // If consumer no longer tracks this graphicBuffer, we can safely
+ // drop this fence, as it will never be received by the producer.
+ if (!stillTracking(slot, graphicBuffer)) {
+ return OK;
+ }
+
if (!mSlots[slot].mFence.get()) {
mSlots[slot].mFence = fence;
} else {
@@ -225,11 +236,20 @@
return OK;
}
-status_t ConsumerBase::releaseBufferLocked(int slot, EGLDisplay display,
- EGLSyncKHR eglFence) {
- CB_LOGV("releaseBufferLocked: slot=%d", slot);
- status_t err = mBufferQueue->releaseBuffer(slot, display, eglFence,
- mSlots[slot].mFence);
+status_t ConsumerBase::releaseBufferLocked(
+ int slot, const sp<GraphicBuffer> graphicBuffer,
+ EGLDisplay display, EGLSyncKHR eglFence) {
+ // If consumer no longer tracks this graphicBuffer (we received a new
+ // buffer on the same slot), the buffer producer is definitely no longer
+ // tracking it.
+ if (!stillTracking(slot, graphicBuffer)) {
+ return OK;
+ }
+
+ CB_LOGV("releaseBufferLocked: slot=%d/%llu",
+ slot, mSlots[slot].mFrameNumber);
+ status_t err = mBufferQueue->releaseBuffer(slot, mSlots[slot].mFrameNumber,
+ display, eglFence, mSlots[slot].mFence);
if (err == BufferQueue::STALE_BUFFER_SLOT) {
freeBufferLocked(slot);
}
@@ -239,4 +259,13 @@
return err;
}
+bool ConsumerBase::stillTracking(int slot,
+ const sp<GraphicBuffer> graphicBuffer) {
+ if (slot < 0 || slot >= BufferQueue::NUM_BUFFER_SLOTS) {
+ return false;
+ }
+ return (mSlots[slot].mGraphicBuffer != NULL &&
+ mSlots[slot].mGraphicBuffer->handle == graphicBuffer->handle);
+}
+
} // namespace android
diff --git a/libs/gui/CpuConsumer.cpp b/libs/gui/CpuConsumer.cpp
index 0543649..0834361 100644
--- a/libs/gui/CpuConsumer.cpp
+++ b/libs/gui/CpuConsumer.cpp
@@ -189,7 +189,9 @@
// disconnected after this buffer was acquired.
if (CC_LIKELY(mAcquiredBuffers[lockedIdx].mGraphicBuffer ==
mSlots[buf].mGraphicBuffer)) {
- releaseBufferLocked(buf, EGL_NO_DISPLAY, EGL_NO_SYNC_KHR);
+ releaseBufferLocked(
+ buf, mAcquiredBuffers[lockedIdx].mGraphicBuffer,
+ EGL_NO_DISPLAY, EGL_NO_SYNC_KHR);
}
AcquiredBuffer &ab = mAcquiredBuffers.editItemAt(lockedIdx);
diff --git a/libs/gui/GLConsumer.cpp b/libs/gui/GLConsumer.cpp
index 344a93a..6d29edc 100644
--- a/libs/gui/GLConsumer.cpp
+++ b/libs/gui/GLConsumer.cpp
@@ -188,12 +188,16 @@
return NO_ERROR;
}
-status_t GLConsumer::releaseBufferLocked(int buf, EGLDisplay display,
- EGLSyncKHR eglFence) {
- status_t err = ConsumerBase::releaseBufferLocked(buf, display, eglFence);
-
+status_t GLConsumer::releaseBufferLocked(int buf,
+ sp<GraphicBuffer> graphicBuffer,
+ EGLDisplay display, EGLSyncKHR eglFence) {
+ // release the buffer if it hasn't already been discarded by the
+ // BufferQueue. This can happen, for example, when the producer of this
+ // buffer has reallocated the original buffer slot after this buffer
+ // was acquired.
+ status_t err = ConsumerBase::releaseBufferLocked(
+ buf, graphicBuffer, display, eglFence);
mEglSlots[buf].mEglFence = EGL_NO_SYNC_KHR;
-
return err;
}
@@ -237,7 +241,10 @@
if (err != NO_ERROR) {
// Release the buffer we just acquired. It's not safe to
// release the old buffer, so instead we just drop the new frame.
- releaseBufferLocked(buf, mEglDisplay, EGL_NO_SYNC_KHR);
+ // As we are still under lock since acquireBuffer, it is safe to
+ // release by slot.
+ releaseBufferLocked(buf, mSlots[buf].mGraphicBuffer,
+ mEglDisplay, EGL_NO_SYNC_KHR);
return err;
}
@@ -248,7 +255,8 @@
// release old buffer
if (mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) {
- status_t status = releaseBufferLocked(mCurrentTexture, mEglDisplay,
+ status_t status = releaseBufferLocked(
+ mCurrentTexture, mCurrentTextureBuf, mEglDisplay,
mEglSlots[mCurrentTexture].mEglFence);
if (status != NO_ERROR && status != BufferQueue::STALE_BUFFER_SLOT) {
ST_LOGE("releaseAndUpdate: failed to release buffer: %s (%d)",
@@ -334,7 +342,8 @@
void GLConsumer::setReleaseFence(const sp<Fence>& fence) {
if (fence->isValid() &&
mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) {
- status_t err = addReleaseFence(mCurrentTexture, fence);
+ status_t err = addReleaseFence(mCurrentTexture,
+ mCurrentTextureBuf, fence);
if (err != OK) {
ST_LOGE("setReleaseFence: failed to add the fence: %s (%d)",
strerror(-err), err);
@@ -503,7 +512,8 @@
return UNKNOWN_ERROR;
}
sp<Fence> fence(new Fence(fenceFd));
- status_t err = addReleaseFenceLocked(mCurrentTexture, fence);
+ status_t err = addReleaseFenceLocked(mCurrentTexture,
+ mCurrentTextureBuf, fence);
if (err != OK) {
ST_LOGE("syncForReleaseLocked: error adding release fence: "
"%s (%d)", strerror(-err), err);