[libcamera-devel] [PATCH v3 1/1] android: Wait on fences in CameraStream::process()

[Jacopo Mondi]

Acquire fences for streams of type Mapped generated by
post-processing are not correctly handled and are currently
ignored by the camera HAL.

Fix this by adding CameraStream::waitFence(), executed before
starting the post-processing in CameraStream::process().

The change applies to all streams generated by post-processing (Mapped
and Internal) but currently acquire fences of Internal streams are
handled by the camera worker. Post-pone that to post-processing time by
passing -1 to the Worker for Internal streams.

Also correct the release_fence handling for failed captures, as the
framework requires the release fences to be set to the acquire fence
value if the acquire fence has not been waited on.

Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
---
 src/android/camera_device.cpp | 39 +++++++++++++++++++++++----
 src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--
 src/android/camera_stream.h   |  4 ++-
 3 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ab38116800cd..0fe11fe833b0 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 		/*
 		 * Inspect the camera stream type, create buffers opportunely
-		 * and add them to the Request if required.
+		 * and add them to the Request if required. Only acquire fences
+		 * for streams of type Direct are handled by the CameraWorker,
+		 * while fences for streams of type Internal and Mapped are
+		 * handled at post-processing time.
 		 */
 		FrameBuffer *buffer = nullptr;
+		int acquireFence = -1;
 		switch (cameraStream->type()) {
 		case CameraStream::Type::Mapped:
 			/*
@@ -1008,6 +1012,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 						  cameraStream->configuration().size));
 
 			buffer = descriptor.frameBuffers_.back().get();
+			acquireFence = camera3Buffer.acquire_fence;
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
@@ -1030,7 +1035,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		}
 
 		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
-						camera3Buffer.acquire_fence);
+					       acquireFence);
 	}
 
 	/*
@@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)
 	captureResult.frame_number = descriptor.frameNumber_;
 	captureResult.num_output_buffers = descriptor.buffers_.size();
 	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
-		buffer.acquire_fence = -1;
+		CameraStream *cameraStream =
+			static_cast<CameraStream *>(buffer.stream->priv);
+
+		/*
+		 * Streams of type Direct have been queued to the
+		 * libcamera::Camera and their acquisition fences have
+		 * already been waited on by the CameraWorker.
+		 *
+		 * Acquire fences of streams of type Internal and Mapped
+		 * will be handled during post-processing.
+		 *
+		 * \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)
+			buffer.acquire_fence = -1;
 		buffer.release_fence = -1;
 		buffer.status = CAMERA3_BUFFER_STATUS_OK;
 	}
@@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)
 			    CAMERA3_MSG_ERROR_REQUEST);
 
 		captureResult.partial_result = 0;
-		for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
+		for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
+			/*
+			 * 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.release_fence = buffer.acquire_fence;
+			buffer.acquire_fence = -1;
 			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
+		}
+
 		callbacks_->process_capture_result(callbacks_, &captureResult);
 
 		return;
@@ -1181,7 +1210,7 @@ void CameraDevice::requestComplete(Request *request)
 			continue;
 		}
 
-		int ret = cameraStream->process(*src, *buffer.buffer,
+		int ret = cameraStream->process(*src, buffer,
 						descriptor.settings_,
 						resultMetadata.get());
 		/*
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index e30c7ee4fcb3..2363985398fb 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -7,7 +7,11 @@
 
 #include "camera_stream.h"
 
+#include <errno.h>
+#include <string.h>
 #include <sys/mman.h>
+#include <sys/poll.h>
+#include <unistd.h>
 
 #include <libcamera/formats.h>
 
@@ -109,11 +113,53 @@ int CameraStream::configure()
 	return 0;
 }
 
+int CameraStream::waitFence(int fence)
+{
+	/*
+	 * \todo The implementation here is copied from camera_worker.cpp
+	 * and both should be removed once libcamera is instrumented to handle
+	 * fences waiting in the core.
+	 *
+	 * \todo Better characterize the timeout. Currently equal to the one
+	 * used by the Rockchip Camera HAL on ChromeOS.
+	 */
+	constexpr unsigned int timeoutMs = 300;
+	struct pollfd fds = { fence, POLLIN, 0 };
+
+	do {
+		int ret = poll(&fds, 1, timeoutMs);
+		if (ret == 0)
+			return -ETIME;
+
+		if (ret > 0) {
+			if (fds.revents & (POLLERR | POLLNVAL))
+				return -EINVAL;
+
+			return 0;
+		}
+	} while (errno == EINTR || errno == EAGAIN);
+
+	return -errno;
+}
+
 int CameraStream::process(const FrameBuffer &source,
-			  buffer_handle_t camera3Dest,
+			  camera3_stream_buffer_t &camera3Buffer,
 			  const CameraMetadata &requestMetadata,
 			  CameraMetadata *resultMetadata)
 {
+	/* Handle waiting on fences on the destination buffer. */
+	int fence = camera3Buffer.acquire_fence;
+	if (fence != -1) {
+		int ret = waitFence(fence);
+		::close(fence);
+		camera3Buffer.acquire_fence = -1;
+		if (ret < 0) {
+			LOG(HAL, Error) << "Failed waiting for fence: "
+					<< fence << ": " << strerror(-ret);
+			return ret;
+		}
+	}
+
 	if (!postProcessor_)
 		return 0;
 
@@ -122,7 +168,7 @@ int CameraStream::process(const FrameBuffer &source,
 	 * separate thread.
 	 */
 	const StreamConfiguration &output = configuration();
-	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
+	CameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,
 			  PROT_READ | PROT_WRITE);
 	if (!dest.isValid()) {
 		LOG(HAL, Error) << "Failed to map android blob buffer";
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 2dab6c3a37d1..03ecfa94fc8c 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -119,13 +119,15 @@ public:
 
 	int configure();
 	int process(const libcamera::FrameBuffer &source,
-		    buffer_handle_t camera3Dest,
+		    camera3_stream_buffer_t &camera3Buffer,
 		    const CameraMetadata &requestMetadata,
 		    CameraMetadata *resultMetadata);
 	libcamera::FrameBuffer *getBuffer();
 	void putBuffer(libcamera::FrameBuffer *buffer);
 
 private:
+	int waitFence(int fence);
+
 	CameraDevice *const cameraDevice_;
 	const libcamera::CameraConfiguration *config_;
 	const Type type_;
-- 
2.32.0