[PATCH v2 4/9] android: Migrate StreamBuffer::internalBuffer to Camera3RequestDescriptor

[Harvey Yang]

Previously there's a potential issue in StreamBuffer::internalBuffer's
lifetime, when more than one streams depend on the same internal buffer.

This patch fixes the issue by returning the buffer when the whole
Camera3RequestDescriptor is destructed.

Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
---
 src/android/camera_device.cpp  | 63 +++++++++++++++++++---------------
 src/android/camera_request.cpp | 13 ++++---
 src/android/camera_request.h   |  3 +-
 3 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 11613fac9..62f724041 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	 * to a libcamera stream. Streams of type Mapped will be handled later.
 	 *
 	 * Collect the CameraStream associated to each requested capture stream.
-	 * Since requestedStreams is an std:map<>, no duplications can happen.
+	 * Since requestedDirectBuffers is an std:map<>, no duplications can
+	 * happen.
 	 */
-	std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers;
+	std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers;
 	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
 		CameraStream *cameraStream = buffer.stream;
 		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
@@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			frameBuffer = buffer.frameBuffer.get();
 			acquireFence = std::move(buffer.fence);
 
-			requestedBuffers[cameraStream] = frameBuffer;
+			requestedDirectBuffers[cameraStream] = frameBuffer;
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
@@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			 * Get the frame buffer from the CameraStream internal
 			 * buffer pool.
 			 *
-			 * The buffer has to be returned to the CameraStream
-			 * once it has been processed.
+			 * The buffer will be returned to the CameraStream when
+			 * the request is destructed.
 			 */
 			frameBuffer = cameraStream->getBuffer();
-			buffer.internalBuffer = frameBuffer;
 			buffer.srcBuffer = frameBuffer;
 
-			requestedBuffers[cameraStream] = frameBuffer;
+			/*
+			 * Track the allocated internal buffers, which will be
+			 * recycled when the descriptor destroyed.
+			 * */
+			descriptor->internalBuffers_[cameraStream] = frameBuffer;
 			LOG(HAL, Debug) << ss.str() << " (internal)";
 
 			descriptor->pendingStreamsToProcess_.insert(
@@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		 * post-processing. No need to recycle the buffer since it's
 		 * owned by Android.
 		 */
-		auto iterDirectBuffer = requestedBuffers.find(sourceStream);
-		if (iterDirectBuffer != requestedBuffers.end()) {
+		auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream);
+		if (iterDirectBuffer != requestedDirectBuffers.end()) {
 			buffer.srcBuffer = iterDirectBuffer->second;
 			continue;
 		}
 
 		/*
-		 * If that's not the case, we need to add a buffer to the request
-		 * for this stream.
+		 * If that's not the case, we use an internal buffer allocated
+		 * from the source stream.
+		 *
+		 * If an internal buffer has been requested for the source
+		 * stream before, we should reuse it.
+		 */
+		auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream);
+		if (iterInternalBuffer != descriptor->internalBuffers_.end()) {
+			buffer.srcBuffer = iterInternalBuffer->second;
+			continue;
+		}
+
+		/*
+		 * Otherwise, we need to create an internal buffer to the
+		 * request for the source stream. Get the frame buffer from the
+		 * source stream's internal buffer pool.
+		 *
+		 * The buffer will be returned to the CameraStream when the
+		 * request is destructed.
 		 */
-		FrameBuffer *frameBuffer = cameraStream->getBuffer();
-		buffer.internalBuffer = frameBuffer;
+		FrameBuffer *frameBuffer = sourceStream->getBuffer();
 		buffer.srcBuffer = frameBuffer;
 
 		descriptor->request_->addBuffer(sourceStream->stream(),
 						frameBuffer, nullptr);
 
-		requestedBuffers[sourceStream] = frameBuffer;
+		/* Track the allocated internal buffer. */
+		descriptor->internalBuffers_[sourceStream] = frameBuffer;
 	}
 
 	/*
@@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request)
 		if (ret) {
 			setBufferStatus(*buffer, StreamBuffer::Status::Error);
 			descriptor->pendingStreamsToProcess_.erase(stream);
-
-			/*
-			 * If the framebuffer is internal to CameraStream return
-			 * it back now that we're done processing it.
-			 */
-			if (buffer->internalBuffer)
-				stream->putBuffer(buffer->internalBuffer);
 		}
 	}
 
@@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,
 {
 	setBufferStatus(*streamBuffer, status);
 
-	/*
-	 * If the framebuffer is internal to CameraStream return it back now
-	 * that we're done processing it.
-	 */
-	if (streamBuffer->internalBuffer)
-		streamBuffer->stream->putBuffer(streamBuffer->internalBuffer);
-
 	Camera3RequestDescriptor *request = streamBuffer->request;
 
 	{
diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
index 52a3ac1f7..a9240a83c 100644
--- a/src/android/camera_request.cpp
+++ b/src/android/camera_request.cpp
@@ -10,6 +10,7 @@
 #include <libcamera/base/span.h>
 
 #include "camera_buffer.h"
+#include "camera_stream.h"
 
 using namespace libcamera;
 
@@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
 	request_ = camera->createRequest(reinterpret_cast<uint64_t>(this));
 }
 
-Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
+Camera3RequestDescriptor::~Camera3RequestDescriptor()
+{
+	/*
+	 * Recycle the allocated internal buffer back to its source stream.
+	 */
+	for (auto &[sourceStream, frameBuffer] : internalBuffers_)
+		sourceStream->putBuffer(frameBuffer);
+}
 
 /**
  * \class StreamBuffer
@@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
  * \var StreamBuffer::status
  * \brief Track the status of the buffer
  *
- * \var StreamBuffer::internalBuffer
- * \brief Pointer to a buffer internally handled by CameraStream (if any)
- *
  * \var StreamBuffer::srcBuffer
  * \brief Pointer to the source frame buffer used for post-processing
  *
diff --git a/src/android/camera_request.h b/src/android/camera_request.h
index 335f1985d..6b2a00795 100644
--- a/src/android/camera_request.h
+++ b/src/android/camera_request.h
@@ -49,7 +49,6 @@ public:
 	std::unique_ptr<HALFrameBuffer> frameBuffer;
 	libcamera::UniqueFD fence;
 	Status status = Status::Success;
-	libcamera::FrameBuffer *internalBuffer = nullptr;
 	const libcamera::FrameBuffer *srcBuffer = nullptr;
 	std::unique_ptr<CameraBuffer> dstBuffer;
 	Camera3RequestDescriptor *request;
@@ -85,6 +84,8 @@ public:
 	std::unique_ptr<libcamera::Request> request_;
 	std::unique_ptr<CameraMetadata> resultMetadata_;
 
+	std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_;
+
 	bool complete_ = false;
 	Status status_ = Status::Success;
 
-- 
2.47.0.338.g60cca15819-goog