libcamera: Handle request completion explicitly in pipeline handlers

Request complete by themselves when all the buffers they contain have
completed, connecting the buffer's completed signal to be notified of
buffer completion. While this works for now, it prevents pipelines from
delaying request completion until all metadata is available, and makes
it impossible to ensure that requests complete in the order they are
queued.

To fix this, make request completion handling explicit in pipeline
handlers. The base PipelineHandler class is extended with
implementations of the queueRequest() and stop() methods and gets new
completeBuffer() and completeRequest() methods to help pipeline handlers
tracking requests and buffers.

The three existing pipeline handlers connect the bufferReady signal of
their capture video node to a slot of their respective camera data
instance, where they use the PipelineHandler helpers to notify buffer
and request completion. Request completion is handled synchronously with
buffer completion as the pipeline handlers don't need to support more
advanced use cases, but this paves the road for future work.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

9 files changed, 187 insertions, 33 deletions

diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
index 524eb47d..e2d1cf04 100644
--- a/src/libcamera/buffer.cpp
+++ b/src/libcamera/buffer.cpp
@@ -213,11 +213,6 @@ Buffer::Buffer()
  */
 
 /**
- * \var Buffer::completed
- * \brief A Signal to provide notifications that the specific Buffer is ready
- */
-
-/**
  * \fn Buffer::bytesused()
  * \brief Retrieve the number of bytes occupied by the data in the buffer
  * \return Number of bytes occupied in the buffer
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 3090874b..8e17085b 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -175,6 +175,12 @@ const std::string &Camera::name() const
 }
 
 /**
+ * \var Camera::bufferCompleted
+ * \brief Signal emitted when a buffer for a request queued to the camera has
+ * completed
+ */
+
+/**
  * \var Camera::requestCompleted
  * \brief Signal emitted when a request queued to the camera has completed
  */
@@ -606,4 +612,19 @@ int Camera::stop()
 	return 0;
 }
 
+/**
+ * \brief Handle request completion and notify application
+ * \param[in] request The request that has completed
+ *
+ * This function is called by the pipeline handler to notify the camera that
+ * the request has completed. It emits the requestCompleted signal and deletes
+ * the request.
+ */
+void Camera::requestComplete(Request *request)
+{
+	std::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));
+	requestCompleted.emit(request, buffers);
+	delete request;
+}
+
 } /* namespace libcamera */
diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
index 2a426039..acb376e0 100644
--- a/src/libcamera/include/pipeline_handler.h
+++ b/src/libcamera/include/pipeline_handler.h
@@ -7,6 +7,7 @@
 #ifndef __LIBCAMERA_PIPELINE_HANDLER_H__
 #define __LIBCAMERA_PIPELINE_HANDLER_H__
 
+#include <list>
 #include <map>
 #include <memory>
 #include <string>
@@ -14,6 +15,7 @@
 
 namespace libcamera {
 
+class Buffer;
 class BufferPool;
 class Camera;
 class CameraManager;
@@ -35,6 +37,7 @@ public:
 
 	Camera *camera_;
 	PipelineHandler *pipe_;
+	std::list<Request *> queuedRequests_;
 
 private:
 	CameraData(const CameraData &) = delete;
@@ -58,9 +61,12 @@ public:
 	virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
 
 	virtual int start(Camera *camera) = 0;
-	virtual void stop(Camera *camera) = 0;
+	virtual void stop(Camera *camera);
 
-	virtual int queueRequest(Camera *camera, Request *request) = 0;
+	virtual int queueRequest(Camera *camera, Request *request);
+
+	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
+	void completeRequest(Camera *camera, Request *request);
 
 protected:
 	void registerCamera(std::shared_ptr<Camera> camera,
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 79dbfa13..55489c31 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -63,6 +63,8 @@ private:
 			delete sensor_;
 		}
 
+		void bufferReady(Buffer *buffer);
+
 		V4L2Device *cio2_;
 		V4L2Subdevice *csi2_;
 		V4L2Subdevice *sensor_;
@@ -236,6 +238,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
 
 	if (data->cio2_->streamOff())
 		LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
+
+	PipelineHandler::stop(camera);
 }
 
 int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
@@ -250,7 +254,11 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
-	data->cio2_->queueBuffer(buffer);
+	int ret = data->cio2_->queueBuffer(buffer);
+	if (ret < 0)
+		return ret;
+
+	PipelineHandler::queueRequest(camera, request);
 
 	return 0;
 }
@@ -396,6 +404,8 @@ void PipelineHandlerIPU3::registerCameras()
 		if (ret)
 			continue;
 
+		data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady);
+
 		data->sensor_ = new V4L2Subdevice(sensor);
 		ret = data->sensor_->open();
 		if (ret)
@@ -417,6 +427,14 @@ void PipelineHandlerIPU3::registerCameras()
 	}
 }
 
+void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
+{
+	Request *request = queuedRequests_.front();
+
+	pipe_->completeBuffer(camera_, request, buffer);
+	pipe_->completeRequest(camera_, request);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
index 9e8c7c05..cc3e0cd9 100644
--- a/src/libcamera/pipeline/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo.cpp
@@ -56,6 +56,8 @@ private:
 			delete video_;
 		}
 
+		void bufferReady(Buffer *buffer);
+
 		V4L2Device *video_;
 		Stream stream_;
 	};
@@ -153,6 +155,7 @@ void PipelineHandlerUVC::stop(Camera *camera)
 {
 	UVCCameraData *data = cameraData(camera);
 	data->video_->streamOff();
+	PipelineHandler::stop(camera);
 }
 
 int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
@@ -166,7 +169,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
-	data->video_->queueBuffer(buffer);
+	int ret = data->video_->queueBuffer(buffer);
+	if (ret < 0)
+		return ret;
+
+	PipelineHandler::queueRequest(camera, request);
 
 	return 0;
 }
@@ -199,6 +206,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
 	if (data->video_->open())
 		return false;
 
+	data->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);
+
 	/* Create and register the camera. */
 	std::set<Stream *> streams{ &data->stream_ };
 	std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
@@ -210,6 +219,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
 	return true;
 }
 
+void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)
+{
+	Request *request = queuedRequests_.front();
+
+	pipe_->completeBuffer(camera_, request, buffer);
+	pipe_->completeRequest(camera_, request);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
index 2051c14c..2e8c26fb 100644
--- a/src/libcamera/pipeline/vimc.cpp
+++ b/src/libcamera/pipeline/vimc.cpp
@@ -56,6 +56,8 @@ private:
 			delete video_;
 		}
 
+		void bufferReady(Buffer *buffer);
+
 		V4L2Device *video_;
 		Stream stream_;
 	};
@@ -153,6 +155,7 @@ void PipelineHandlerVimc::stop(Camera *camera)
 {
 	VimcCameraData *data = cameraData(camera);
 	data->video_->streamOff();
+	PipelineHandler::stop(camera);
 }
 
 int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
@@ -166,7 +169,11 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
-	data->video_->queueBuffer(buffer);
+	int ret = data->video_->queueBuffer(buffer);
+	if (ret < 0)
+		return ret;
+
+	PipelineHandler::queueRequest(camera, request);
 
 	return 0;
 }
@@ -198,6 +205,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 	if (data->video_->open())
 		return false;
 
+	data->video_->bufferReady.connect(data.get(), &VimcCameraData::bufferReady);
+
 	/* Create and register the camera. */
 	std::set<Stream *> streams{ &data->stream_ };
 	std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
@@ -207,6 +216,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 	return true;
 }
 
+void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)
+{
+	Request *request = queuedRequests_.front();
+
+	pipe_->completeBuffer(camera_, request, buffer);
+	pipe_->completeRequest(camera_, request);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 54f23794..1a858f26 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -5,6 +5,7 @@
  * pipeline_handler.cpp - Pipeline handler infrastructure
  */
 
+#include <libcamera/buffer.h>
 #include <libcamera/camera.h>
 #include <libcamera/camera_manager.h>
 
@@ -75,6 +76,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
  */
 
 /**
+ * \var CameraData::queuedRequests_
+ * \brief The list of queued and not yet completed request
+ *
+ * The list of queued request is used to track requests queued in order to
+ * ensure completion of all requests when the pipeline handler is stopped.
+ *
+ * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
+ * PipelineHandler::completeRequest()
+ */
+
+/**
  * \class PipelineHandler
  * \brief Create and manage cameras based on a set of media devices
  *
@@ -226,8 +238,30 @@ PipelineHandler::~PipelineHandler()
  * This method stops capturing and processing requests immediately. All pending
  * requests are cancelled and complete immediately in an error state.
  *
- * \todo Complete the pending requests immediately
+ * Pipeline handlers shall override this method to stop the pipeline, ensure
+ * that all pending request completion signaled through completeRequest() have
+ * returned, and call the base implementation of the stop() method as the last
+ * step of their implementation. The base implementation cancels all requests
+ * queued but not yet complete.
  */
+void PipelineHandler::stop(Camera *camera)
+{
+	CameraData *data = cameraData(camera);
+
+	while (!data->queuedRequests_.empty()) {
+		Request *request = data->queuedRequests_.front();
+		data->queuedRequests_.pop_front();
+
+		while (!request->pending_.empty()) {
+			Buffer *buffer = *request->pending_.begin();
+			buffer->cancel();
+			completeBuffer(camera, request, buffer);
+		}
+
+		request->complete(Request::RequestCancelled);
+		camera->requestComplete(request);
+	}
+}
 
 /**
  * \fn PipelineHandler::queueRequest()
@@ -241,8 +275,65 @@ PipelineHandler::~PipelineHandler()
  * parameters will be applied to the frames captured in the buffers provided in
  * the request.
  *
+ * Pipeline handlers shall override this method. The base implementation in the
+ * PipelineHandler class keeps track of queued requests in order to ensure
+ * completion of all requests when the pipeline handler is stopped with stop().
+ * Requests completion shall be signaled by the pipeline handler using the
+ * completeRequest() method.
+ *
  * \return 0 on success or a negative error code otherwise
  */
+int PipelineHandler::queueRequest(Camera *camera, Request *request)
+{
+	CameraData *data = cameraData(camera);
+	data->queuedRequests_.push_back(request);
+
+	return 0;
+}
+
+/**
+ * \brief Complete a buffer for a request
+ * \param[in] camera The camera the request belongs to
+ * \param[in] request The request the buffer belongs to
+ * \param[in] buffer The buffer that has completed
+ *
+ * This method shall be called by pipeline handlers to signal completion of the
+ * \a buffer part of the \a request. It notifies applications of buffer
+ * completion and updates the request's internal buffer tracking. The request
+ * is not completed automatically when the last buffer completes, pipeline
+ * handlers shall complete requests explicitly with completeRequest().
+ *
+ * \return True if all buffers contained in the request have completed, false
+ * otherwise
+ */
+bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
+				     Buffer *buffer)
+{
+	camera->bufferCompleted.emit(request, buffer);
+	return request->completeBuffer(buffer);
+}
+
+/**
+ * \brief Signal request completion
+ * \param[in] camera The camera that the request belongs to
+ * \param[in] request The request that has completed
+ *
+ * The pipeline handler shall call this method to notify the \a camera that the
+ * request request has complete. The request is deleted and shall not be
+ * accessed once this method returns.
+ *
+ * The pipeline handler shall ensure that requests complete in the same order
+ * they are submitted.
+ */
+void PipelineHandler::completeRequest(Camera *camera, Request *request)
+{
+	CameraData *data = cameraData(camera);
+	ASSERT(request == data->queuedRequests_.front());
+	data->queuedRequests_.pop_front();
+
+	request->complete(Request::RequestComplete);
+	camera->requestComplete(request);
+}
 
 /**
  * \brief Register a camera to the camera manager and pipeline handler
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index cb170930..e0e77e97 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -113,7 +113,6 @@ int Request::prepare()
 {
 	for (auto const &pair : bufferMap_) {
 		Buffer *buffer = pair.second;
-		buffer->completed.connect(this, &Request::bufferCompleted);
 		pending_.insert(buffer);
 	}
 
@@ -133,31 +132,24 @@ void Request::complete(Status status)
 }
 
 /**
- * \brief Slot for the buffer completed signal
+ * \brief Complete a buffer for the request
+ * \param[in] buffer The buffer that has completed
  *
- * The bufferCompleted method serves as slot where to connect the
- * Buffer::completed signal that is emitted when a buffer has available
- * data.
+ * A request tracks the status of all buffers it contains through a set of
+ * pending buffers. This function removes the \a buffer from the set to mark it
+ * as complete. All buffers associate with the request shall be marked as
+ * complete by calling this function once and once only before reporting the
+ * request as complete with the complete() method.
  *
- * The request completes when all the buffers it contains are ready to be
- * presented to the application. It then emits the Camera::requestCompleted
- * signal and is automatically deleted.
+ * \return True if all buffers contained in the request have completed, false
+ * otherwise
  */
-void Request::bufferCompleted(Buffer *buffer)
+bool Request::completeBuffer(Buffer *buffer)
 {
-	buffer->completed.disconnect(this, &Request::bufferCompleted);
-
 	int ret = pending_.erase(buffer);
 	ASSERT(ret == 1);
 
-	if (!pending_.empty())
-		return;
-
-	complete(RequestComplete);
-
-	std::map<Stream *, Buffer *> buffers(std::move(bufferMap_));
-	camera_->requestCompleted.emit(this, buffers);
-	delete this;
+	return pending_.empty();
 }
 
 } /* namespace libcamera */
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 03e4abab..e2036aac 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -825,9 +825,6 @@ void V4L2Device::bufferAvailable(EventNotifier *notifier)
 
 	/* Notify anyone listening to the device. */
 	bufferReady.emit(buffer);
-
-	/* Notify anyone listening to the buffer specifically. */
-	buffer->completed.emit(buffer);
 }
 
 /**