android: camera_request: Don't embed full camera3_stream_buffer_t

The camera3_stream_buffer_t structure is meant to communicate between
the camera service and the HAL. They are short-live structures that
don't outlive the .process_capture_request() operation (when queuing
requests) or the .process_capture_result() callback.

We currently store copies of the camera3_stream_buffer_t passed to
.process_capture_request() in Camera3RequestDescriptor::StreamBuffer to
store the structure members that the HAL need, and reuse them when
calling the .process_capture_result() callback. This is conceptually not
right, as the camera3_stream_buffer_t pass to the callback are not the
same objects as the ones received in .process_capture_request().

Store individual fields of the camera3_stream_buffer_t in StreamBuffer
instead of copying the whole structure. This gives the HAL full control
of how data is stored, and properly decouples request queueing from
result reporting.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

src/android/camera_device.cpp

@@ -801,16 +801,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
 {
 	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
 
-	for (auto &buffer : descriptor->buffers_) {
+	for (auto &buffer : descriptor->buffers_)
-		/*
+		buffer.status = Camera3RequestDescriptor::Status::Error;
-		 * Signal to the framework it has to handle fences that have not
-		 * been waited on by setting the release fence to the acquire
-		 * fence value.
-		 */
-		buffer.buffer.release_fence = buffer.buffer.acquire_fence;
-		buffer.buffer.acquire_fence = -1;
-		buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
-	}
 
 	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 }
@@ -908,8 +900,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			<< " with " << descriptor->buffers_.size() << " streams";
 
 	for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {
-		camera3_stream *camera3Stream = buffer.buffer.stream;
+		CameraStream *cameraStream = buffer.stream;
-		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
+		camera3_stream_t *camera3Stream = cameraStream->camera3Stream();
 
 		std::stringstream ss;
 		ss << i << " - (" << camera3Stream->width << "x"
@@ -944,11 +936,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			 * lifetime management only.
 			 */
 			buffer.frameBuffer =
-				createFrameBuffer(*buffer.buffer.buffer,
+				createFrameBuffer(*buffer.camera3Buffer,
 						  cameraStream->configuration().pixelFormat,
 						  cameraStream->configuration().size);
 			frameBuffer = buffer.frameBuffer.get();
-			acquireFence = buffer.buffer.acquire_fence;
+			acquireFence = buffer.fence;
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
@@ -1055,12 +1047,11 @@ void CameraDevice::requestComplete(Request *request)
 	/*
 	 * Prepare the capture result for the Android camera stack.
 	 *
-	 * The buffer status is set to OK and later changed to ERROR if
+	 * The buffer status is set to Success and later changed to Error if
 	 * post-processing/compression fails.
 	 */
 	for (auto &buffer : descriptor->buffers_) {
-		CameraStream *cameraStream =
+		CameraStream *stream = buffer.stream;
-			static_cast<CameraStream *>(buffer.buffer.stream->priv);
 
 		/*
 		 * Streams of type Direct have been queued to the
@@ -1073,10 +1064,9 @@ void CameraDevice::requestComplete(Request *request)
 		 * \todo Instrument the CameraWorker to set the acquire
 		 * fence to -1 once it has handled it and remove this check.
 		 */
-		if (cameraStream->type() == CameraStream::Type::Direct)
+		if (stream->type() == CameraStream::Type::Direct)
-			buffer.buffer.acquire_fence = -1;
+			buffer.fence = -1;
-		buffer.buffer.release_fence = -1;
+		buffer.status = Camera3RequestDescriptor::Status::Success;
-		buffer.buffer.status = CAMERA3_BUFFER_STATUS_OK;
 	}
 
 	/*
@@ -1128,33 +1118,32 @@ void CameraDevice::requestComplete(Request *request)
 
 	/* Handle post-processing. */
 	for (auto &buffer : descriptor->buffers_) {
-		CameraStream *cameraStream =
+		CameraStream *stream = buffer.stream;
-			static_cast<CameraStream *>(buffer.buffer.stream->priv);
 
-		if (cameraStream->type() == CameraStream::Type::Direct)
+		if (stream->type() == CameraStream::Type::Direct)
 			continue;
 
-		FrameBuffer *src = request->findBuffer(cameraStream->stream());
+		FrameBuffer *src = request->findBuffer(stream->stream());
 		if (!src) {
 			LOG(HAL, Error) << "Failed to find a source stream buffer";
-			buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
+			buffer.status = Camera3RequestDescriptor::Status::Error;
-			notifyError(descriptor->frameNumber_, buffer.buffer.stream,
+			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
 				    CAMERA3_MSG_ERROR_BUFFER);
 			continue;
 		}
 
-		int ret = cameraStream->process(*src, buffer, descriptor);
+		int ret = stream->process(*src, buffer, descriptor);
 
 		/*
 		 * Return the FrameBuffer to the CameraStream now that we're
 		 * done processing it.
 		 */
-		if (cameraStream->type() == CameraStream::Type::Internal)
+		if (stream->type() == CameraStream::Type::Internal)
-			cameraStream->putBuffer(src);
+			stream->putBuffer(src);
 
 		if (ret) {
-			buffer.buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
+			buffer.status = Camera3RequestDescriptor::Status::Error;
-			notifyError(descriptor->frameNumber_, buffer.buffer.stream,
+			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
 				    CAMERA3_MSG_ERROR_BUFFER);
 		}
 	}
@@ -1185,8 +1174,24 @@ void CameraDevice::sendCaptureResults()
 			captureResult.result = descriptor->resultMetadata_->get();
 
 		std::vector<camera3_stream_buffer_t> resultBuffers;
-		for (const auto &buffer : descriptor->buffers_)
+		resultBuffers.reserve(descriptor->buffers_.size());
-			resultBuffers.emplace_back(buffer.buffer);
+
+		for (const auto &buffer : descriptor->buffers_) {
+			camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;
+
+			if (buffer.status == Camera3RequestDescriptor::Status::Success)
+				status = CAMERA3_BUFFER_STATUS_OK;
+
+			/*
+			 * Pass the buffer fence back to the camera framework as
+			 * a release fence. This instructs the framework to wait
+			 * on the acquire fence in case we haven't done so
+			 * ourselves for any reason.
+			 */
+			resultBuffers.push_back({ buffer.stream->camera3Stream(),
+						  buffer.camera3Buffer, status,
+						  -1, buffer.fence });
+		}
 
 		captureResult.num_output_buffers = resultBuffers.size();
 		captureResult.output_buffers = resultBuffers.data();

src/android/camera_request.cpp

@@ -7,6 +7,8 @@
 
 #include "camera_request.h"
 
+#include <libcamera/base/span.h>
+
 using namespace libcamera;
 
 /*
@@ -22,11 +24,20 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
 	frameNumber_ = camera3Request->frame_number;
 
 	/* Copy the camera3 request stream information for later access. */
-	const uint32_t numBuffers = camera3Request->num_output_buffers;
+	const Span<const camera3_stream_buffer_t> buffers{
+		camera3Request->output_buffers,
+		camera3Request->num_output_buffers
+	};
 
-	buffers_.resize(numBuffers);
+	buffers_.reserve(buffers.size());
-	for (uint32_t i = 0; i < numBuffers; i++)
+
-		buffers_[i].buffer = camera3Request->output_buffers[i];
+	for (const camera3_stream_buffer_t &buffer : buffers) {
+		CameraStream *stream =
+			static_cast<CameraStream *>(buffer.stream->priv);
+
+		buffers_.push_back({ stream, buffer.buffer, nullptr,
+				     buffer.acquire_fence, Status::Pending });
+	}
 
 	/* Clone the controls associated with the camera3 request. */
 	settings_ = CameraMetadata(camera3Request->settings);

src/android/camera_request.h

@@ -20,6 +20,8 @@
 #include "camera_metadata.h"
 #include "camera_worker.h"
 
+class CameraStream;
+
 class Camera3RequestDescriptor
 {
 public:
@@ -30,13 +32,11 @@ public:
 	};
 
 	struct StreamBuffer {
-		camera3_stream_buffer_t buffer;
+		CameraStream *stream;
-		/*
+		buffer_handle_t *camera3Buffer;
-		 * FrameBuffer instances created by wrapping a camera3 provided
-		 * dmabuf are emplaced in this vector of unique_ptr<> for
-		 * lifetime management.
-		 */
 		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
+		int fence;
+		Status status;
 	};
 
 	Camera3RequestDescriptor(libcamera::Camera *camera,

src/android/camera_stream.cpp

@@ -147,11 +147,11 @@ int CameraStream::process(const FrameBuffer &source,
 			  Camera3RequestDescriptor *request)
 {
 	/* Handle waiting on fences on the destination buffer. */
-	int fence = dest.buffer.acquire_fence;
+	int fence = dest.fence;
 	if (fence != -1) {
 		int ret = waitFence(fence);
 		::close(fence);
-		dest.buffer.acquire_fence = -1;
+		dest.fence = -1;
 		if (ret < 0) {
 			LOG(HAL, Error) << "Failed waiting for fence: "
 					<< fence << ": " << strerror(-ret);
@@ -167,7 +167,7 @@ int CameraStream::process(const FrameBuffer &source,
 	 * separate thread.
 	 */
 	const StreamConfiguration &output = configuration();
-	CameraBuffer destBuffer(*dest.buffer.buffer, output.pixelFormat,
+	CameraBuffer destBuffer(*dest.camera3Buffer, output.pixelFormat,
 				output.size, PROT_READ | PROT_WRITE);
 	if (!destBuffer.isValid()) {
 		LOG(HAL, Error) << "Failed to create destination buffer";