diff --git a/include/gui/BufferQueueCore.h b/include/gui/BufferQueueCore.h index a6065a9e9..40026bda3 100644 --- a/include/gui/BufferQueueCore.h +++ b/include/gui/BufferQueueCore.h @@ -30,9 +30,6 @@ #include #include -#include -#include - #define BQ_LOGV(x, ...) ALOGV("[%s] " x, mConsumerName.string(), ##__VA_ARGS__) #define BQ_LOGD(x, ...) ALOGD("[%s] " x, mConsumerName.string(), ##__VA_ARGS__) #define BQ_LOGI(x, ...) ALOGI("[%s] " x, mConsumerName.string(), ##__VA_ARGS__) @@ -126,10 +123,6 @@ private: // waitWhileAllocatingLocked blocks until mIsAllocating is false. void waitWhileAllocatingLocked() const; - // validateConsistencyLocked ensures that the free lists are in sync with - // the information stored in mSlots - void validateConsistencyLocked() const; - // mAllocator is the connection to SurfaceFlinger that is used to allocate // new GraphicBuffer objects. sp mAllocator; @@ -184,14 +177,6 @@ private: // mQueue is a FIFO of queued buffers used in synchronous mode. Fifo mQueue; - // mFreeSlots contains all of the slots which are FREE and do not currently - // have a buffer attached - std::set mFreeSlots; - - // mFreeBuffers contains all of the slots which are FREE and currently have - // a buffer attached - std::list mFreeBuffers; - // mOverrideMaxBufferCount is the limit on the number of buffers that will // be allocated at one time. This value is set by the producer by calling // setBufferCount. The default is 0, which means that the producer doesn't diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp index c7d5e0032..526c3b729 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -120,7 +120,6 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, if (mCore->stillTracking(front)) { // Front buffer is still in mSlots, so mark the slot as free mSlots[front->mSlot].mBufferState = BufferSlot::FREE; - mCore->mFreeBuffers.push_back(front->mSlot); } mCore->mQueue.erase(front); front = mCore->mQueue.begin(); @@ -174,8 +173,6 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, ATRACE_INT(mCore->mConsumerName.string(), mCore->mQueue.size()); - mCore->validateConsistencyLocked(); - return NO_ERROR; } @@ -202,7 +199,6 @@ status_t BufferQueueConsumer::detachBuffer(int slot) { mCore->freeBufferLocked(slot); mCore->mDequeueCondition.broadcast(); - mCore->validateConsistencyLocked(); return NO_ERROR; } @@ -221,11 +217,18 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot, Mutex::Autolock lock(mCore->mMutex); - // Make sure we don't have too many acquired buffers + // Make sure we don't have too many acquired buffers and find a free slot + // to put the buffer into (the oldest if there are multiple). int numAcquiredBuffers = 0; + int found = BufferQueueCore::INVALID_BUFFER_SLOT; for (int s = 0; s < BufferQueueDefs::NUM_BUFFER_SLOTS; ++s) { if (mSlots[s].mBufferState == BufferSlot::ACQUIRED) { ++numAcquiredBuffers; + } else if (mSlots[s].mBufferState == BufferSlot::FREE) { + if (found == BufferQueueCore::INVALID_BUFFER_SLOT || + mSlots[s].mFrameNumber < mSlots[found].mFrameNumber) { + found = s; + } } } @@ -235,17 +238,6 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot, mCore->mMaxAcquiredBufferCount); return INVALID_OPERATION; } - - // Find a free slot to put the buffer into - int found = BufferQueueCore::INVALID_BUFFER_SLOT; - if (!mCore->mFreeSlots.empty()) { - auto slot = mCore->mFreeSlots.begin(); - found = *slot; - mCore->mFreeSlots.erase(slot); - } else if (!mCore->mFreeBuffers.empty()) { - found = mCore->mFreeBuffers.front(); - mCore->mFreeBuffers.remove(found); - } if (found == BufferQueueCore::INVALID_BUFFER_SLOT) { BQ_LOGE("attachBuffer(P): could not find free buffer slot"); return NO_MEMORY; @@ -279,8 +271,6 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot, // for attached buffers. mSlots[*outSlot].mAcquireCalled = false; - mCore->validateConsistencyLocked(); - return NO_ERROR; } @@ -321,7 +311,6 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber, mSlots[slot].mEglFence = eglFence; mSlots[slot].mFence = releaseFence; mSlots[slot].mBufferState = BufferSlot::FREE; - mCore->mFreeBuffers.push_back(slot); listener = mCore->mConnectedProducerListener; BQ_LOGV("releaseBuffer: releasing slot %d", slot); } else if (mSlots[slot].mNeedsCleanupOnRelease) { @@ -336,7 +325,6 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber, } mCore->mDequeueCondition.broadcast(); - mCore->validateConsistencyLocked(); } // Autolock scope // Call back without lock held diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp index 29415c94c..edebc4553 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -53,8 +53,6 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : mConnectedProducerListener(), mSlots(), mQueue(), - mFreeSlots(), - mFreeBuffers(), mOverrideMaxBufferCount(0), mDequeueCondition(), mUseAsyncBuffer(true), @@ -78,9 +76,6 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : BQ_LOGE("createGraphicBufferAlloc failed"); } } - for (int slot = 0; slot < BufferQueueDefs::NUM_BUFFER_SLOTS; ++slot) { - mFreeSlots.insert(slot); - } } BufferQueueCore::~BufferQueueCore() {} @@ -195,20 +190,12 @@ status_t BufferQueueCore::setDefaultMaxBufferCountLocked(int count) { void BufferQueueCore::freeBufferLocked(int slot) { BQ_LOGV("freeBufferLocked: slot %d", slot); - bool hadBuffer = mSlots[slot].mGraphicBuffer != NULL; mSlots[slot].mGraphicBuffer.clear(); if (mSlots[slot].mBufferState == BufferSlot::ACQUIRED) { mSlots[slot].mNeedsCleanupOnRelease = true; } - if (mSlots[slot].mBufferState != BufferSlot::FREE) { - mFreeSlots.insert(slot); - } else if (hadBuffer) { - // If the slot was FREE, but we had a buffer, we need to move this slot - // from the free buffers list to the the free slots list - mFreeBuffers.remove(slot); - mFreeSlots.insert(slot); - } mSlots[slot].mBufferState = BufferSlot::FREE; + mSlots[slot].mFrameNumber = UINT32_MAX; mSlots[slot].mAcquireCalled = false; // Destroy fence as BufferQueue now takes ownership @@ -217,7 +204,6 @@ void BufferQueueCore::freeBufferLocked(int slot) { mSlots[slot].mEglFence = EGL_NO_SYNC_KHR; } mSlots[slot].mFence = Fence::NO_FENCE; - validateConsistencyLocked(); } void BufferQueueCore::freeAllBuffersLocked() { @@ -250,48 +236,4 @@ void BufferQueueCore::waitWhileAllocatingLocked() const { } } -void BufferQueueCore::validateConsistencyLocked() const { - static const useconds_t PAUSE_TIME = 0; - for (int slot = 0; slot < BufferQueueDefs::NUM_BUFFER_SLOTS; ++slot) { - bool isInFreeSlots = mFreeSlots.count(slot) != 0; - bool isInFreeBuffers = - std::find(mFreeBuffers.cbegin(), mFreeBuffers.cend(), slot) != - mFreeBuffers.cend(); - if (mSlots[slot].mBufferState == BufferSlot::FREE) { - if (mSlots[slot].mGraphicBuffer == NULL) { - if (!isInFreeSlots) { - BQ_LOGE("Slot %d is FREE but is not in mFreeSlots", slot); - usleep(PAUSE_TIME); - } - if (isInFreeBuffers) { - BQ_LOGE("Slot %d is in mFreeSlots " - "but is also in mFreeBuffers", slot); - usleep(PAUSE_TIME); - } - } else { - if (!isInFreeBuffers) { - BQ_LOGE("Slot %d is FREE but is not in mFreeBuffers", slot); - usleep(PAUSE_TIME); - } - if (isInFreeSlots) { - BQ_LOGE("Slot %d is in mFreeBuffers " - "but is also in mFreeSlots", slot); - usleep(PAUSE_TIME); - } - } - } else { - if (isInFreeSlots) { - BQ_LOGE("Slot %d is in mFreeSlots but is not FREE (%d)", - slot, mSlots[slot].mBufferState); - usleep(PAUSE_TIME); - } - if (isInFreeBuffers) { - BQ_LOGE("Slot %d is in mFreeBuffers but is not FREE (%d)", - slot, mSlots[slot].mBufferState); - usleep(PAUSE_TIME); - } - } - } -} - } // namespace android diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index b54641e98..6452cddcf 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -161,6 +161,8 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller, } } + // Look for a free buffer to give to the client + *found = BufferQueueCore::INVALID_BUFFER_SLOT; int dequeuedCount = 0; int acquiredCount = 0; for (int s = 0; s < maxBufferCount; ++s) { @@ -171,6 +173,15 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller, case BufferSlot::ACQUIRED: ++acquiredCount; break; + case BufferSlot::FREE: + // We return the oldest of the free buffers to avoid + // stalling the producer if possible, since the consumer + // may still have pending reads of in-flight buffers + if (*found == BufferQueueCore::INVALID_BUFFER_SLOT || + mSlots[s].mFrameNumber < mSlots[*found].mFrameNumber) { + *found = s; + } + break; default: break; } @@ -203,8 +214,6 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller, } } - *found = BufferQueueCore::INVALID_BUFFER_SLOT; - // If we disconnect and reconnect quickly, we can be in a state where // our slots are empty but we have many buffers in the queue. This can // cause us to run out of memory if we outrun the consumer. Wait here if @@ -214,16 +223,6 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller, if (tooManyBuffers) { BQ_LOGV("%s: queue size is %zu, waiting", caller, mCore->mQueue.size()); - } else { - if (!mCore->mFreeBuffers.empty()) { - auto slot = mCore->mFreeBuffers.begin(); - *found = *slot; - mCore->mFreeBuffers.erase(slot); - } else if (!mCore->mFreeSlots.empty()) { - auto slot = mCore->mFreeSlots.begin(); - *found = *slot; - mCore->mFreeSlots.erase(slot); - } } // If no buffer is found, or if the queue has too many buffers @@ -336,8 +335,6 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot, *outFence = mSlots[found].mFence; mSlots[found].mEglFence = EGL_NO_SYNC_KHR; mSlots[found].mFence = Fence::NO_FENCE; - - mCore->validateConsistencyLocked(); } // Autolock scope if (returnFlags & BUFFER_NEEDS_REALLOCATION) { @@ -358,6 +355,7 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot, return NO_INIT; } + mSlots[*outSlot].mFrameNumber = UINT32_MAX; mSlots[*outSlot].mGraphicBuffer = graphicBuffer; } // Autolock scope } @@ -416,7 +414,6 @@ status_t BufferQueueProducer::detachBuffer(int slot) { mCore->freeBufferLocked(slot); mCore->mDequeueCondition.broadcast(); - mCore->validateConsistencyLocked(); return NO_ERROR; } @@ -441,19 +438,27 @@ status_t BufferQueueProducer::detachNextBuffer(sp* outBuffer, return NO_INIT; } - if (mCore->mFreeBuffers.empty()) { - return NO_MEMORY; + // Find the oldest valid slot + int found = BufferQueueCore::INVALID_BUFFER_SLOT; + for (int s = 0; s < BufferQueueDefs::NUM_BUFFER_SLOTS; ++s) { + if (mSlots[s].mBufferState == BufferSlot::FREE && + mSlots[s].mGraphicBuffer != NULL) { + if (found == BufferQueueCore::INVALID_BUFFER_SLOT || + mSlots[s].mFrameNumber < mSlots[found].mFrameNumber) { + found = s; + } + } } - int found = mCore->mFreeBuffers.front(); - mCore->mFreeBuffers.remove(found); + if (found == BufferQueueCore::INVALID_BUFFER_SLOT) { + return NO_MEMORY; + } BQ_LOGV("detachNextBuffer detached slot %d", found); *outBuffer = mSlots[found].mGraphicBuffer; *outFence = mSlots[found].mFence; mCore->freeBufferLocked(found); - mCore->validateConsistencyLocked(); return NO_ERROR; } @@ -501,8 +506,6 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot, mSlots[*outSlot].mFence = Fence::NO_FENCE; mSlots[*outSlot].mRequestBufferCalled = true; - mCore->validateConsistencyLocked(); - return returnFlags; } @@ -637,7 +640,9 @@ status_t BufferQueueProducer::queueBuffer(int slot, // mark it as freed if (mCore->stillTracking(front)) { mSlots[front->mSlot].mBufferState = BufferSlot::FREE; - mCore->mFreeBuffers.push_front(front->mSlot); + // Reset the frame number of the freed buffer so that it is + // the first in line to be dequeued again + mSlots[front->mSlot].mFrameNumber = 0; } // Overwrite the droppable buffer with the incoming one *front = item; @@ -659,8 +664,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, // Take a ticket for the callback functions callbackTicket = mNextCallbackTicket++; - - mCore->validateConsistencyLocked(); } // Autolock scope // Wait without lock held @@ -721,11 +724,10 @@ void BufferQueueProducer::cancelBuffer(int slot, const sp& fence) { return; } - mCore->mFreeBuffers.push_front(slot); mSlots[slot].mBufferState = BufferSlot::FREE; + mSlots[slot].mFrameNumber = 0; mSlots[slot].mFence = fence; mCore->mDequeueCondition.broadcast(); - mCore->validateConsistencyLocked(); } int BufferQueueProducer::query(int what, int *outValue) { @@ -1007,19 +1009,13 @@ void BufferQueueProducer::allocateBuffers(bool async, uint32_t width, } mCore->freeBufferLocked(slot); // Clean up the slot first mSlots[slot].mGraphicBuffer = buffers[i]; + mSlots[slot].mFrameNumber = 0; mSlots[slot].mFence = Fence::NO_FENCE; - - // freeBufferLocked puts this slot on the free slots list. Since - // we then attached a buffer, move the slot to free buffer list. - mCore->mFreeSlots.erase(slot); - mCore->mFreeBuffers.push_front(slot); - BQ_LOGV("allocateBuffers: allocated a new buffer in slot %d", slot); } mCore->mIsAllocating = false; mCore->mIsAllocatingCondition.broadcast(); - mCore->validateConsistencyLocked(); } // Autolock scope } }