[RFC PATCH 1/4] libcamera: pipeline_handler: Move waitingRequests_ into camera class

[Barnabás Pőcze]

Hi

2025. 05. 26. 23:42 keltezéssel, Stefan Klug írta:
> The waiting requests of one camera should not be able to influence
> queuing to another camera. Therefore change the single waitingRequests_
> queue into one queue per camera.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>   include/libcamera/internal/camera.h           |  2 ++
>   include/libcamera/internal/pipeline_handler.h |  3 --
>   src/libcamera/pipeline_handler.cpp            | 34 +++++++++++++------
>   3 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 18f5c32a18e4..8a2e9ed5894d 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -10,6 +10,7 @@
>   #include <atomic>
>   #include <list>
>   #include <memory>
> +#include <queue>
>   #include <set>
>   #include <stdint.h>
>   #include <string>
> @@ -36,6 +37,7 @@ public:
>   	const PipelineHandler *pipe() const { return pipe_.get(); }
> 
>   	std::list<Request *> queuedRequests_;
> +	std::queue<Request *> waitingRequests_;
>   	ControlInfoMap controlInfo_;
>   	ControlList properties_;
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 972a2fa65310..dedc29e815fb 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -8,7 +8,6 @@
>   #pragma once
> 
>   #include <memory>
> -#include <queue>
>   #include <string>
>   #include <sys/types.h>
>   #include <vector>
> @@ -94,8 +93,6 @@ private:
>   	std::vector<std::shared_ptr<MediaDevice>> mediaDevices_;
>   	std::vector<std::weak_ptr<Camera>> cameras_;
> 
> -	std::queue<Request *> waitingRequests_;
> -
>   	const char *name_;
>   	unsigned int useCount_;
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index d84dff3c9f19..14d8602e67d8 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)
>   	/* Stop the pipeline handler and let the queued requests complete. */
>   	stopDevice(camera);
> 
> +	Camera::Private *data = camera->_d();
> +
>   	/* Cancel and signal as complete all waiting requests. */
> -	while (!waitingRequests_.empty()) {
> -		Request *request = waitingRequests_.front();
> -		waitingRequests_.pop();
> +	while (!data->waitingRequests_.empty()) {
> +		Request *request = data->waitingRequests_.front();
> +		data->waitingRequests_.pop();
>   		cancelRequest(request);
>   	}
> 
>   	/* Make sure no requests are pending. */
> -	Camera::Private *data = camera->_d();
>   	ASSERT(data->queuedRequests_.empty());
> 
>   	data->requestSequence_ = 0;
> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)
>   {
>   	LIBCAMERA_TRACEPOINT(request_queue, request);
> 
> -	waitingRequests_.push(request);
> +	Camera *camera = request->_d()->camera();
> +	Camera::Private *data = camera->_d();
> +	data->waitingRequests_.push(request);
> 
>   	request->_d()->prepare(300ms);
>   }
> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)
>    */
>   void PipelineHandler::doQueueRequests()
>   {
> -	while (!waitingRequests_.empty()) {
> -		Request *request = waitingRequests_.front();
> -		if (!request->_d()->prepared_)
> -			break;
> +	for (const std::weak_ptr<Camera> &ptr : cameras_) {
> +		std::shared_ptr<Camera> camera = ptr.lock();
> +		if (!camera)
> +			continue;
> 
> -		doQueueRequest(request);
> -		waitingRequests_.pop();
> +		Camera::Private *data = camera->_d();
> +		while (!data->waitingRequests_.empty()) {

As far as I understand `doQueueRequests()` runs when a request becomes "prepared".
So I would expect the above inner loop to be sufficient. Is there any reason why
the waiting request queues of all cameras are processed?


> +			Request *request = data->waitingRequests_.front();
> +			if (!request->_d()->prepared_)
> +				break;
> +
> +			doQueueRequest(request);
> +			data->waitingRequests_.pop();
> +		}
>   	}
>   }
> 
> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)
>   		data->queuedRequests_.pop_front();
>   		camera->requestComplete(req);
>   	}
> +
> +	doQueueRequests();

This only purpose of this is to handle early stopping due to `maxQueuedRequestsDevice()`, right?
If so, then I think this should be in the next patch.


Regards,
Barnabás Pőcze


>   }
> 
>   /**
> --
> 2.43.0
>