Camera: Don't hold locks during shared output dequeue
Blocking dequeue calls for async output surfaces seem to be
possible. Refactor the code so that stream splitter doesn't
hold any locks during output surface dequeue calls triggered
by buffer released callbacks.
Bug: 73953239
Test: Camera CTS
Change-Id: Ia2166de73d2b8de7ef4157e81c87f53c8a264c0c
diff --git a/services/camera/libcameraservice/device3/Camera3StreamSplitter.cpp b/services/camera/libcameraservice/device3/Camera3StreamSplitter.cpp
index bc12f39..e3bb5dc 100644
--- a/services/camera/libcameraservice/device3/Camera3StreamSplitter.cpp
+++ b/services/camera/libcameraservice/device3/Camera3StreamSplitter.cpp
@@ -364,7 +364,7 @@
// queue, no onBufferReleased is called by the buffer queue.
// Proactively trigger the callback to avoid buffer loss.
if (queueOutput.bufferReplaced) {
- onBufferReleasedByOutputLocked(output, surfaceId);
+ onBufferReplacedLocked(output, surfaceId);
}
return res;
@@ -582,7 +582,16 @@
void Camera3StreamSplitter::onBufferReleasedByOutput(
const sp<IGraphicBufferProducer>& from) {
ATRACE_CALL();
+ sp<Fence> fence;
+
+ int slot = BufferItem::INVALID_BUFFER_SLOT;
+ auto res = from->dequeueBuffer(&slot, &fence, mWidth, mHeight, mFormat, mProducerUsage,
+ nullptr, nullptr);
Mutex::Autolock lock(mMutex);
+ handleOutputDequeueStatusLocked(res, slot);
+ if (res != OK) {
+ return;
+ }
size_t surfaceId = 0;
bool found = false;
@@ -598,61 +607,47 @@
return;
}
- onBufferReleasedByOutputLocked(from, surfaceId);
+ returnOutputBufferLocked(fence, from, surfaceId, slot);
}
-void Camera3StreamSplitter::onBufferReleasedByOutputLocked(
+void Camera3StreamSplitter::onBufferReplacedLocked(
const sp<IGraphicBufferProducer>& from, size_t surfaceId) {
ATRACE_CALL();
- sp<GraphicBuffer> buffer;
sp<Fence> fence;
- if (mOutputSlots[from] == nullptr) {
- //Output surface got likely removed by client.
- return;
- }
- auto outputSlots = *mOutputSlots[from];
int slot = BufferItem::INVALID_BUFFER_SLOT;
auto res = from->dequeueBuffer(&slot, &fence, mWidth, mHeight, mFormat, mProducerUsage,
nullptr, nullptr);
- if (res == NO_INIT) {
- // If we just discovered that this output has been abandoned, note that,
- // but we can't do anything else, since buffer is invalid
- onAbandonedLocked();
- return;
- } else if (res == IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) {
- SP_LOGE("%s: Producer needs to re-allocate buffer!", __FUNCTION__);
- SP_LOGE("%s: This should not happen with buffer allocation disabled!", __FUNCTION__);
- return;
- } else if (res == IGraphicBufferProducer::RELEASE_ALL_BUFFERS) {
- SP_LOGE("%s: All slot->buffer mapping should be released!", __FUNCTION__);
- SP_LOGE("%s: This should not happen with buffer allocation disabled!", __FUNCTION__);
- return;
- } else if (res == NO_MEMORY) {
- SP_LOGE("%s: No free buffers", __FUNCTION__);
- return;
- } else if (res == WOULD_BLOCK) {
- SP_LOGE("%s: Dequeue call will block", __FUNCTION__);
- return;
- } else if (res != OK || (slot == BufferItem::INVALID_BUFFER_SLOT)) {
- SP_LOGE("%s: dequeue buffer from output failed (%d)", __FUNCTION__, res);
+ handleOutputDequeueStatusLocked(res, slot);
+ if (res != OK) {
return;
}
- buffer = outputSlots[slot];
+ returnOutputBufferLocked(fence, from, surfaceId, slot);
+}
+
+void Camera3StreamSplitter::returnOutputBufferLocked(const sp<Fence>& fence,
+ const sp<IGraphicBufferProducer>& from, size_t surfaceId, int slot) {
+ sp<GraphicBuffer> buffer;
+
+ if (mOutputSlots[from] == nullptr) {
+ //Output surface got likely removed by client.
+ return;
+ }
+
+ auto outputSlots = *mOutputSlots[from];
+ buffer = outputSlots[slot];
BufferTracker& tracker = *(mBuffers[buffer->getId()]);
// Merge the release fence of the incoming buffer so that the fence we send
// back to the input includes all of the outputs' fences
if (fence != nullptr && fence->isValid()) {
tracker.mergeFence(fence);
}
- SP_LOGV("%s: dequeued buffer %" PRId64 " %p from output %p", __FUNCTION__,
- buffer->getId(), buffer.get(), from.get());
auto detachBuffer = mDetachedBuffers.find(buffer->getId());
bool detach = (detachBuffer != mDetachedBuffers.end());
if (detach) {
- res = from->detachBuffer(slot);
+ auto res = from->detachBuffer(slot);
if (res == NO_ERROR) {
outputSlots[slot] = nullptr;
} else {
@@ -664,6 +659,26 @@
decrementBufRefCountLocked(buffer->getId(), surfaceId);
}
+void Camera3StreamSplitter::handleOutputDequeueStatusLocked(status_t res, int slot) {
+ if (res == NO_INIT) {
+ // If we just discovered that this output has been abandoned, note that,
+ // but we can't do anything else, since buffer is invalid
+ onAbandonedLocked();
+ } else if (res == IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) {
+ SP_LOGE("%s: Producer needs to re-allocate buffer!", __FUNCTION__);
+ SP_LOGE("%s: This should not happen with buffer allocation disabled!", __FUNCTION__);
+ } else if (res == IGraphicBufferProducer::RELEASE_ALL_BUFFERS) {
+ SP_LOGE("%s: All slot->buffer mapping should be released!", __FUNCTION__);
+ SP_LOGE("%s: This should not happen with buffer allocation disabled!", __FUNCTION__);
+ } else if (res == NO_MEMORY) {
+ SP_LOGE("%s: No free buffers", __FUNCTION__);
+ } else if (res == WOULD_BLOCK) {
+ SP_LOGE("%s: Dequeue call will block", __FUNCTION__);
+ } else if (res != OK || (slot == BufferItem::INVALID_BUFFER_SLOT)) {
+ SP_LOGE("%s: dequeue buffer from output failed (%d)", __FUNCTION__, res);
+ }
+}
+
void Camera3StreamSplitter::onAbandonedLocked() {
// If this is called from binderDied callback, it means the app process
// holding the binder has died. CameraService will be notified of the binder
diff --git a/services/camera/libcameraservice/device3/Camera3StreamSplitter.h b/services/camera/libcameraservice/device3/Camera3StreamSplitter.h
index 76a5b7d..fea1bdb 100644
--- a/services/camera/libcameraservice/device3/Camera3StreamSplitter.h
+++ b/services/camera/libcameraservice/device3/Camera3StreamSplitter.h
@@ -122,10 +122,8 @@
// onFrameAvailable call to proceed.
void onBufferReleasedByOutput(const sp<IGraphicBufferProducer>& from);
- // This is the implementation of onBufferReleasedByOutput without the mutex locked.
- // It could either be called from onBufferReleasedByOutput or from
- // onFrameAvailable when a buffer in the async buffer queue is overwritten.
- void onBufferReleasedByOutputLocked(const sp<IGraphicBufferProducer>& from, size_t surfaceId);
+ // Called by outputBufferLocked when a buffer in the async buffer queue got replaced.
+ void onBufferReplacedLocked(const sp<IGraphicBufferProducer>& from, size_t surfaceId);
// When this is called, the splitter disconnects from (i.e., abandons) its
// input queue and signals any waiting onFrameAvailable calls to wake up.
@@ -138,6 +136,13 @@
// 0, return the buffer back to the input BufferQueue.
void decrementBufRefCountLocked(uint64_t id, size_t surfaceId);
+ // Check for and handle any output surface dequeue errors.
+ void handleOutputDequeueStatusLocked(status_t res, int slot);
+
+ // Handles released output surface buffers.
+ void returnOutputBufferLocked(const sp<Fence>& fence, const sp<IGraphicBufferProducer>& from,
+ size_t surfaceId, int slot);
+
// This is a thin wrapper class that lets us determine which BufferQueue
// the IProducerListener::onBufferReleased callback is associated with. We
// create one of these per output BufferQueue, and then pass the producer