[libcamera-devel] [PATCH v4 8/9] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic

[Naushir Patuck]

Add further queueing into the RPiStream object to ensure that we always
follow the buffer ordering (be it internal or external) given by
incoming Requests.

This is essential, otherwise we risk dropping frames that are meant to
be part of a Request, and can cause the pipeline to stall indefinitely.
This also prevents any possibility of mismatched frame buffers going
through the pipeline and out to the application.

Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
---
 .../pipeline/raspberrypi/rpi_stream.cpp       | 70 ++++++++++++++++---
 .../pipeline/raspberrypi/rpi_stream.h         | 12 +++-
 2 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
index 02f8d3e0..6635a751 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
@@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)
 	 */
 	if (!buffer) {
 		if (availableBuffers_.empty()) {
-			LOG(RPISTREAM, Warning) << "No buffers available for "
+			LOG(RPISTREAM, Info) << "No buffers available for "
 						<< name_;
-			return -EINVAL;
+			/*
+			 * Note that we need to requeue an internal buffer as soon
+			 * as one becomes available.
+			 */
+			requeueBuffers_.push(nullptr);
+			return 0;
 		}
 
 		buffer = availableBuffers_.front();
 		availableBuffers_.pop();
 	}
 
-	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
-			      << " for " << name_;
+	/*
+	 * If no earlier requests are pending to be queued we can go ahead and
+	 * queue the buffer into the device.
+	 */
+	if (requeueBuffers_.empty())
+		return queueToDevice(buffer);
 
-	int ret = dev_->queueBuffer(buffer);
-	if (ret) {
-		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
-				      << name_;
-	}
+	/*
+	 * There are earlier buffers to be queued, so this bufferm ust go on
+	 * the waiting list.
+	 */
+	requeueBuffers_.push(buffer);
 
-	return ret;
+	return 0;
 }
 
 void RPiStream::returnBuffer(FrameBuffer *buffer)
@@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)
 	/* This can only be called for external streams. */
 	assert(external_);
 
+	/* Push this buffer back into the queue to be used again. */
 	availableBuffers_.push(buffer);
+
+	/*
+	 * Do we have any buffers that are waiting to be queued?
+	 * If so, do it now as availableBuffers_ will not be empty.
+	 */
+	while (!requeueBuffers_.empty()) {
+		FrameBuffer *buffer = requeueBuffers_.front();
+
+		if (!buffer) {
+			/*
+			 * We want to queue an internal buffer, but none
+			 * are available. Can't do anything, quit the loop.
+			 */
+			if (availableBuffers_.empty())
+				break;
+
+			/*
+			 * We want to queue an internal buffer, and at least one
+			 * is available.
+			 */
+			buffer = availableBuffers_.front();
+			availableBuffers_.pop();
+		}
+
+		requeueBuffers_.pop();
+		queueToDevice(buffer);
+	}
 }
 
 bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
@@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
 void RPiStream::clearBuffers()
 {
 	availableBuffers_ = std::queue<FrameBuffer *>{};
+	requeueBuffers_ = std::queue<FrameBuffer *>{};
 	internalBuffers_.clear();
 	bufferList_.clear();
 }
 
+int RPiStream::queueToDevice(FrameBuffer *buffer)
+{
+	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
+			      << " for " << name_;
+
+	int ret = dev_->queueBuffer(buffer);
+	if (ret)
+		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
+				      << name_;
+	return ret;
+}
+
 } /* namespace RPi */
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
index af9c2ad2..6222c867 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
@@ -52,6 +52,7 @@ public:
 
 private:
 	void clearBuffers();
+	int queueToDevice(FrameBuffer *buffer);
 	/*
 	 * Indicates that this stream is active externally, i.e. the buffers
 	 * might be provided by (and returned to) the application.
@@ -63,7 +64,7 @@ private:
 	std::string name_;
 	/* The actual device stream. */
 	std::unique_ptr<V4L2VideoDevice> dev_;
-	/* All framebuffers associated with this device stream. */
+	/* All frame buffers associated with this device stream. */
 	std::vector<FrameBuffer *> bufferList_;
 	/*
 	 * List of frame buffer that we can use if none have been provided by
@@ -71,6 +72,15 @@ private:
 	 * buffers exported internally.
 	 */
 	std::queue<FrameBuffer *> availableBuffers_;
+	/*
+	 * List of frame buffer that are to be re-queued into the device.
+	 * A nullptr indicates any internal buffer can be used (from availableBuffers_),
+	 * whereas a valid pointer indicates an external buffer to be queued.
+	 *
+	 * Ordering buffers to be queued is important here as it must match the
+	 * requests coming from the application.
+	 */
+	std::queue<FrameBuffer *> requeueBuffers_;
 	/*
 	 * This is a list of buffers exported internally. Need to keep this around
 	 * as the stream needs to maintain ownership of these buffers.
-- 
2.25.1