Camera: Handle consumer side detach in BufferManager
- BufferQueue consumer may detach buffers, and BufferManager isn't
aware. Hook up onBufferRemoved callback so that BufferManager's internal
handoutBuffer and attachedBuffer counts can be updated when a buffer
is detached.
- Deleted some code that's not exercised any more.
Test: Camera CTS and burst capture.
Bug: 38238747
Change-Id: I6861da6e013fe5609907f807639c5d691c0c3af9
diff --git a/services/camera/libcameraservice/device3/Camera3BufferManager.cpp b/services/camera/libcameraservice/device3/Camera3BufferManager.cpp
index d93b331..99b3ba8 100644
--- a/services/camera/libcameraservice/device3/Camera3BufferManager.cpp
+++ b/services/camera/libcameraservice/device3/Camera3BufferManager.cpp
@@ -128,11 +128,9 @@
// De-list all the buffers associated with this stream first.
StreamSet& currentSet = mStreamSetMap.editValueFor(streamSetId);
- BufferList& freeBufs = currentSet.freeBuffers;
BufferCountMap& handOutBufferCounts = currentSet.handoutBufferCountMap;
BufferCountMap& attachedBufferCounts = currentSet.attachedBufferCountMap;
InfoMap& infoMap = currentSet.streamInfoMap;
- removeBuffersFromBufferListLocked(freeBufs, streamId);
handOutBufferCounts.removeItem(streamId);
attachedBufferCounts.removeItem(streamId);
@@ -151,7 +149,7 @@
currentSet.allocatedBufferWaterMark = 0;
// Remove this stream set if all its streams have been removed.
- if (freeBufs.size() == 0 && handOutBufferCounts.size() == 0 && infoMap.size() == 0) {
+ if (handOutBufferCounts.size() == 0 && infoMap.size() == 0) {
mStreamSetMap.removeItem(streamSetId);
}
@@ -191,31 +189,25 @@
}
ALOGV("Stream %d set %d: Get buffer for stream: Allocate new", streamId, streamSetId);
- GraphicBufferEntry buffer =
- getFirstBufferFromBufferListLocked(streamSet.freeBuffers, streamId);
-
if (mGrallocVersion < HARDWARE_DEVICE_API_VERSION(1,0)) {
- // Allocate one if there is no free buffer available.
- if (buffer.graphicBuffer == nullptr) {
- const StreamInfo& info = streamSet.streamInfoMap.valueFor(streamId);
- buffer.fenceFd = -1;
+ const StreamInfo& info = streamSet.streamInfoMap.valueFor(streamId);
+ GraphicBufferEntry buffer;
+ buffer.fenceFd = -1;
+ buffer.graphicBuffer = new GraphicBuffer(
+ info.width, info.height, PixelFormat(info.format), info.combinedUsage,
+ std::string("Camera3BufferManager pid [") +
+ std::to_string(getpid()) + "]");
+ status_t res = buffer.graphicBuffer->initCheck();
- buffer.graphicBuffer = new GraphicBuffer(
- info.width, info.height, PixelFormat(info.format), info.combinedUsage,
- std::string("Camera3BufferManager pid [") +
- std::to_string(getpid()) + "]");
- status_t res = buffer.graphicBuffer->initCheck();
-
- ALOGV("%s: allocating a new graphic buffer (%dx%d, format 0x%x) %p with handle %p",
- __FUNCTION__, info.width, info.height, info.format,
- buffer.graphicBuffer.get(), buffer.graphicBuffer->handle);
- if (res < 0) {
- ALOGE("%s: graphic buffer allocation failed: (error %d %s) ",
- __FUNCTION__, res, strerror(-res));
- return res;
- }
- ALOGV("%s: allocation done", __FUNCTION__);
+ ALOGV("%s: allocating a new graphic buffer (%dx%d, format 0x%x) %p with handle %p",
+ __FUNCTION__, info.width, info.height, info.format,
+ buffer.graphicBuffer.get(), buffer.graphicBuffer->handle);
+ if (res < 0) {
+ ALOGE("%s: graphic buffer allocation failed: (error %d %s) ",
+ __FUNCTION__, res, strerror(-res));
+ return res;
}
+ ALOGV("%s: allocation done", __FUNCTION__);
// Increase the hand-out and attached buffer counts for tracking purposes.
bufferCount++;
@@ -242,7 +234,6 @@
for (size_t i = 0; i < streamSet.streamInfoMap.size(); i++) {
firstOtherStreamId = streamSet.streamInfoMap[i].streamId;
if (firstOtherStreamId != streamId) {
-
size_t otherBufferCount =
streamSet.handoutBufferCountMap.valueFor(firstOtherStreamId);
size_t otherAttachedBufferCount =
@@ -251,10 +242,6 @@
freeBufferIsAttached = true;
break;
}
- if (hasBufferForStreamLocked(streamSet.freeBuffers, firstOtherStreamId)) {
- freeBufferIsAttached = false;
- break;
- }
}
firstOtherStreamId = CAMERA3_STREAM_ID_INVALID;
}
@@ -263,8 +250,8 @@
}
// This will drop the reference to one free buffer, which will effectively free one
- // buffer (from the free buffer list) for the inactive streams.
- size_t totalAllocatedBufferCount = streamSet.freeBuffers.size();
+ // buffer for the inactive streams.
+ size_t totalAllocatedBufferCount = 0;
for (size_t i = 0; i < streamSet.attachedBufferCountMap.size(); i++) {
totalAllocatedBufferCount += streamSet.attachedBufferCountMap[i];
}
@@ -294,9 +281,6 @@
size_t& otherAttachedBufferCount =
streamSet.attachedBufferCountMap.editValueFor(firstOtherStreamId);
otherAttachedBufferCount--;
- } else {
- // Droppable buffer is in the free buffer list, grab and drop
- getFirstBufferFromBufferListLocked(streamSet.freeBuffers, firstOtherStreamId);
}
}
}
@@ -335,40 +319,42 @@
return OK;
}
-status_t Camera3BufferManager::returnBufferForStream(int streamId,
- int streamSetId, const sp<GraphicBuffer>& buffer, int fenceFd) {
+status_t Camera3BufferManager::onBuffersRemoved(int streamId, int streamSetId, size_t count) {
ATRACE_CALL();
Mutex::Autolock l(mLock);
- ALOGV_IF(buffer != 0, "%s: return buffer (%p) with handle (%p) for stream %d and stream set %d",
- __FUNCTION__, buffer.get(), buffer->handle, streamId, streamSetId);
+
+ ALOGV("Stream %d set %d: Buffer removed", streamId, streamSetId);
if (!checkIfStreamRegisteredLocked(streamId, streamSetId)){
- ALOGV("%s: returning buffer for an already unregistered stream (stream %d with set id %d),"
- "buffer will be dropped right away!", __FUNCTION__, streamId, streamSetId);
+ ALOGV("%s: signaling buffer removal for an already unregistered stream "
+ "(stream %d with set id %d)", __FUNCTION__, streamId, streamSetId);
return OK;
}
if (mGrallocVersion < HARDWARE_DEVICE_API_VERSION(1,0)) {
- // Add to the freeBuffer list.
StreamSet& streamSet = mStreamSetMap.editValueFor(streamSetId);
- if (buffer != 0) {
- BufferEntry entry;
- entry.add(streamId, GraphicBufferEntry(buffer, fenceFd));
- status_t res = addBufferToBufferListLocked(streamSet.freeBuffers, entry);
- if (res != OK) {
- ALOGE("%s: add buffer to free buffer list failed", __FUNCTION__);
- return res;
- }
+ BufferCountMap& handOutBufferCounts = streamSet.handoutBufferCountMap;
+ size_t& totalHandoutCount = handOutBufferCounts.editValueFor(streamId);
+ BufferCountMap& attachedBufferCounts = streamSet.attachedBufferCountMap;
+ size_t& totalAttachedCount = attachedBufferCounts.editValueFor(streamId);
+
+ if (count > totalHandoutCount) {
+ ALOGE("%s: Removed buffer count %zu greater than current handout count %zu",
+ __FUNCTION__, count, totalHandoutCount);
+ return BAD_VALUE;
+ }
+ if (count > totalAttachedCount) {
+ ALOGE("%s: Removed buffer count %zu greater than current attached count %zu",
+ __FUNCTION__, count, totalAttachedCount);
+ return BAD_VALUE;
}
- // Update the handed out and attached buffer count for this buffer.
- BufferCountMap& handOutBufferCounts = streamSet.handoutBufferCountMap;
- size_t& bufferCount = handOutBufferCounts.editValueFor(streamId);
- bufferCount--;
- size_t& attachedBufferCount = streamSet.attachedBufferCountMap.editValueFor(streamId);
- attachedBufferCount--;
+ totalHandoutCount -= count;
+ totalAttachedCount -= count;
+ ALOGV("%s: Stream %d set %d: Buffer count now %zu, attached buffer count now %zu",
+ __FUNCTION__, streamId, streamSetId, totalHandoutCount, totalAttachedCount);
} else {
- // TODO: implement this.
+ // TODO: implement gralloc V1 support
return BAD_VALUE;
}
@@ -404,17 +390,6 @@
lines.appendFormat(" stream id: %d, attached buffer count: %zu.\n",
streamId, bufferCount);
}
-
- lines.appendFormat(" Free buffer count: %zu\n",
- mStreamSetMap[i].freeBuffers.size());
- for (auto& bufEntry : mStreamSetMap[i].freeBuffers) {
- for (size_t m = 0; m < bufEntry.size(); m++) {
- const sp<GraphicBuffer>& buffer = bufEntry.valueAt(m).graphicBuffer;
- int streamId = bufEntry.keyAt(m);
- lines.appendFormat(" stream id: %d, buffer: %p, handle: %p.\n",
- streamId, buffer.get(), buffer->handle);
- }
- }
}
write(fd, lines.string(), lines.size());
}
@@ -444,67 +419,5 @@
return true;
}
-status_t Camera3BufferManager::addBufferToBufferListLocked(BufferList& bufList,
- const BufferEntry& buffer) {
- // TODO: need add some sanity check here.
- bufList.push_back(buffer);
-
- return OK;
-}
-
-status_t Camera3BufferManager::removeBuffersFromBufferListLocked(BufferList& bufferList,
- int streamId) {
- BufferList::iterator i = bufferList.begin();
- while (i != bufferList.end()) {
- ssize_t idx = i->indexOfKey(streamId);
- if (idx != NAME_NOT_FOUND) {
- ALOGV("%s: Remove a buffer for stream %d, free buffer total count: %zu",
- __FUNCTION__, streamId, bufferList.size());
- i->removeItem(streamId);
- if (i->isEmpty()) {
- i = bufferList.erase(i);
- }
- } else {
- i++;
- }
- }
-
- return OK;
-}
-
-bool Camera3BufferManager::hasBufferForStreamLocked(BufferList& buffers, int streamId) {
- BufferList::iterator i = buffers.begin();
- while (i != buffers.end()) {
- ssize_t idx = i->indexOfKey(streamId);
- if (idx != NAME_NOT_FOUND) {
- return true;
- }
- i++;
- }
-
- return false;
-}
-
-Camera3BufferManager::GraphicBufferEntry Camera3BufferManager::getFirstBufferFromBufferListLocked(
- BufferList& buffers, int streamId) {
- // Try to get the first buffer from the free buffer list if there is one.
- GraphicBufferEntry entry;
- BufferList::iterator i = buffers.begin();
- while (i != buffers.end()) {
- ssize_t idx = i->indexOfKey(streamId);
- if (idx != NAME_NOT_FOUND) {
- entry = GraphicBufferEntry(i->valueAt(idx));
- i = buffers.erase(i);
- break;
- } else {
- i++;
- }
- }
-
- ALOGV_IF(entry.graphicBuffer == 0, "%s: Unable to find free buffer for stream %d",
- __FUNCTION__, streamId);
- return entry;
-}
-
} // namespace camera3
} // namespace android
diff --git a/services/camera/libcameraservice/device3/Camera3BufferManager.h b/services/camera/libcameraservice/device3/Camera3BufferManager.h
index d1d7a6f..549bd99 100644
--- a/services/camera/libcameraservice/device3/Camera3BufferManager.h
+++ b/services/camera/libcameraservice/device3/Camera3BufferManager.h
@@ -147,31 +147,22 @@
status_t onBufferReleased(int streamId, int streamSetId);
/**
- * This method returns a buffer for a stream to this buffer manager.
+ * This method notifies the manager that certain buffers has been removed from the
+ * buffer queue by detachBuffer from the consumer.
*
- * When a buffer is returned, it is treated as a free buffer and may either be reused for future
- * getBufferForStream() calls, or freed if there total number of outstanding allocated buffers
- * is too large. The latter only applies to the case where the buffer are physically shared
- * between streams in the same stream set. A physically shared buffer is the buffer that has one
- * physical back store but multiple handles. Multiple stream can access the same physical memory
- * with their own handles. Physically shared buffer can only be supported by Gralloc HAL V1.
- * See hardware/libhardware/include/hardware/gralloc1.h for more details.
+ * The notification lets the manager update its internal handout buffer count and
+ * attached buffer counts accordingly. When buffers are detached from
+ * consumer, both handout and attached counts are decremented.
*
+ * Return values:
*
- * This call takes the ownership of the returned buffer if it was allocated by this buffer
- * manager; clients should not use this buffer after this call. Attempting to access this buffer
- * after this call will have undefined behavior. Holding a reference to this buffer after this
- * call may cause memory leakage. If a BufferQueue is used to track the buffers handed out by
- * this buffer queue, it is recommended to call detachNextBuffer() from the buffer queue after
- * BufferQueueProducer onBufferReleased callback is fired, and return it to this buffer manager.
- *
- * OK: Buffer return for this stream was successful.
- * BAD_VALUE: stream ID or streamSetId are invalid, or stream ID and stream set ID combination
- * doesn't match what was registered, or this stream wasn't registered to this
- * buffer manager before.
+ * OK: Buffer removal was processed succesfully
+ * BAD_VALUE: stream ID or streamSetId are invalid, or stream ID and stream set ID
+ * combination doesn't match what was registered, or this stream wasn't registered
+ * to this buffer manager before, or the removed buffer count is larger than
+ * current total handoutCount or attachedCount.
*/
- status_t returnBufferForStream(int streamId, int streamSetId, const sp<GraphicBuffer>& buffer,
- int fenceFd);
+ status_t onBuffersRemoved(int streamId, int streamSetId, size_t count);
/**
* Dump the buffer manager statistics.
@@ -256,11 +247,6 @@
*/
InfoMap streamInfoMap;
/**
- * The free buffer list for all the buffers belong to this set. The free buffers are
- * returned by the returnBufferForStream() call, and available for reuse.
- */
- BufferList freeBuffers;
- /**
* The count of the buffers that were handed out to the streams of this set.
*/
BufferCountMap handoutBufferCountMap;
@@ -293,38 +279,6 @@
*/
bool checkIfStreamRegisteredLocked(int streamId, int streamSetId) const;
- /**
- * Add a buffer entry to the BufferList. This method needs to be called with mLock held.
- */
- status_t addBufferToBufferListLocked(BufferList &bufList, const BufferEntry &buffer);
-
- /**
- * Remove all buffers from the BufferList.
- *
- * Note that this doesn't mean that the buffers are freed after this call. A buffer is freed
- * only if all other references to it are dropped.
- *
- * This method needs to be called with mLock held.
- */
- status_t removeBuffersFromBufferListLocked(BufferList &bufList, int streamId);
-
- /**
- * Get the first available buffer from the buffer list for this stream. The graphicBuffer inside
- * this entry will be NULL if there is no any GraphicBufferEntry found. After this call, the
- * GraphicBufferEntry will be removed from the BufferList if a GraphicBufferEntry is found.
- *
- * This method needs to be called with mLock held.
- *
- */
- GraphicBufferEntry getFirstBufferFromBufferListLocked(BufferList& buffers, int streamId);
-
- /**
- * Check if there is any buffer associated with this stream in the given buffer list.
- *
- * This method needs to be called with mLock held.
- *
- */
- bool inline hasBufferForStreamLocked(BufferList& buffers, int streamId);
};
} // namespace camera3
diff --git a/services/camera/libcameraservice/device3/Camera3OutputStream.cpp b/services/camera/libcameraservice/device3/Camera3OutputStream.cpp
index e46d55e..56174fa 100644
--- a/services/camera/libcameraservice/device3/Camera3OutputStream.cpp
+++ b/services/camera/libcameraservice/device3/Camera3OutputStream.cpp
@@ -550,6 +550,10 @@
res = mConsumer->getAndFlushRemovedBuffers(&removedBuffers);
if (res == OK) {
onBuffersRemovedLocked(removedBuffers);
+
+ if (mUseBufferManager && removedBuffers.size() > 0) {
+ mBufferManager->onBuffersRemoved(getId(), getStreamSetId(), removedBuffers.size());
+ }
}
}