[libcamera-devel] [RFC PATCH 2/2] libcamera: PipelineHandler: Retry queuing a capture request

[Sebastian Fricke]

Hey Hirokazu,

Thanks for the patch.

This patch currently doesn't apply on the master branch of libcamera:
```
error: sha1 information is lacking or useless (include/libcamera/internal/pipeline_handler.h).
error: could not build fake ancestor
Patch failed at 0002 libcamera: PipelineHandler: Retry queuing a capture request
```


On 29.03.2021 11:26, Hirokazu Honda wrote:
>PipelineHandler::queueRequestDevice() fails due to a buffer
>shortage. We should retry queuing a capture request later in the
>case.

s/in the case/in that case/

>
>Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>---
> include/libcamera/internal/pipeline_handler.h |  2 +
> src/libcamera/pipeline_handler.cpp            | 57 +++++++++++++++++--
> 2 files changed, 54 insertions(+), 5 deletions(-)
>
>diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>index 763da63e..e6f771a6 100644
>--- a/include/libcamera/internal/pipeline_handler.h
>+++ b/include/libcamera/internal/pipeline_handler.h
>@@ -44,6 +44,7 @@ public:
> 	virtual ~CameraData() = default;
>
> 	PipelineHandler *pipe_;
>+	std::queue<Request *> waitedRequests_;

s/waitedRequests_/waitingRequests_/ ?

Waited is the past tense of wait and therefore this variable sounds like
those are requests, that we waited for in the past, but we still wait
for them. Therefore we should use the present progressive, which in the
case of wait is waiting.
As an alternative, we might also call the variable: failedRequests_,
as it describes requests, that failed to be inserted into the queue
and need to be queued again.

The correction applies to all other cases below, where that variable is
used.

> 	std::deque<Request *> queuedRequests_;
> 	ControlInfoMap controlInfo_;
> 	ControlList properties_;
>@@ -99,6 +100,7 @@ protected:
> 	CameraManager *manager_;
>
> private:
>+	void tryQueueRequests(CameraData *data);
> 	void mediaDeviceDisconnected(MediaDevice *media);
> 	virtual void disconnect();
>
>diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>index 4cb58084..18952a1b 100644
>--- a/src/libcamera/pipeline_handler.cpp
>+++ b/src/libcamera/pipeline_handler.cpp
>@@ -70,6 +70,16 @@ LOG_DEFINE_CATEGORY(Pipeline)
>  * and remains valid until the instance is destroyed.
>  */
>
>+/**
>+ * \var CameraData::waitedRequests_
>+ * \brief The queue of not yet queued request
>+ *
>+ * The queue of not yet queued request is used to track requests that are not
>+ * queued in order to retry queueing them when a pipeline is ready to accept.

I find this sentence confusing to read, there is too much use of the
word queue.  How about:
This queue of failed requests is used to retry queuing as soon as the pipeline
is ready to accept them.


>+ *
>+ * \sa PipelineHandler::queueRequest(), PipelineHandler::tryQueueRequests().
>+ */
>+
> /**
>  * \var CameraData::queuedRequests_
>  * \brief The queue of queued and not yet completed request
>@@ -378,12 +388,44 @@ void PipelineHandler::queueRequest(Request *request)
>
> 	Camera *camera = request->camera_;
> 	CameraData *data = cameraData(camera);
>-	data->queuedRequests_.push_back(request);
>+	data->waitedRequests_.push(request);
>+
>+	tryQueueRequests(data);
>+}
>+
>+/**
>+ * \fn PipelineHandler::tryQueueRequests()
>+ * \brief Queue requests that are not yet queued.
>+ * \param[in] data The camera data whose waited requests are tried to queue.

s/The camera data whose waited requests are tried to queue./
   CameraData containing a queue of failed requests to retry queuing./
>+ *
>+ * This tries to queue as many requests as possible in order of the
>+ * waitedRequests_ in data. If queuing fails due to a buffer shortage, this
>+ * method stops and the next call of this method starts from the request that
>+ * fails in the previous call. On any other failures, the request is marked as

s/fails/failed/
s/any other failures/any other failure/

>+ * complete and proceed the successive requests.

s/proceed the/proceed with/

>+ *
>+ * \context This function shall be called from the CameraManager thread.
>+ */
>+void PipelineHandler::tryQueueRequests(CameraData *data)
>+{
>+	while (!data->waitedRequests_.empty()) {
>+		Request *request = data->waitedRequests_.front();
>+		Camera *camera = request->camera_;
>+		ASSERT(data == cameraData(camera));
>+
>+		data->queuedRequests_.push_back(request);
>+		int ret = queueRequestDevice(camera, request);
>+		if (ret == -ENOBUFS || ret == -ENOMEM) {
>+			data->queuedRequests_.pop_back();
>+			break;
>+		}
>
>-	int ret = queueRequestDevice(camera, request);
>-	if (ret) {
>-		request->result_ = ret;
>-		completeRequest(request);
>+		data->waitedRequests_.pop();
>+		if (ret) {
>+			request->result_ = ret;
>+			completeRequest(request);
>+			break;
>+		}
> 	}
> }
>
>@@ -440,6 +482,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  * submission order, the pipeline handler may call it on any complete request
>  * without any ordering constraint.
>  *
>+ * There might be requests that are waiting to be queued, this triggers
>+ * tryQueueRequests().
>+ *
>  * \context This function shall be called from the CameraManager thread.
>  */
> void PipelineHandler::completeRequest(Request *request)
>@@ -459,6 +504,8 @@ void PipelineHandler::completeRequest(Request *request)
> 		data->queuedRequests_.pop_front();
> 		camera->requestComplete(req);
> 	}
>+
>+	tryQueueRequests(data);
> }

It is difficult for me to follow the code here and the patch currently
doesn't apply could you rebase it on master and repost?

Greetings,
Sebastian
>
> /**
>-- 
>2.31.0.291.g576ba9dcdaf-goog
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel at lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel