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

[Jacopo Mondi]

Hi Stefan

On Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:
> 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_;

So we keep the storage in Camera::Private but only access it from the
PipelineHandler base class ?

I understand operating on the waitingRequst_ queue in the Camera class
might not be desirable, as it's way easier to do that in the
PipelineHandler as it runs in the library thread so we don't have to
worry about concurrency.

However I'm not sure this is a good pattern, maybe we can create a map
in the PipelineHandler base class that associates a Camera to a queue
?

>  	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()

I've seen suggestions of passing a Camera * to this function, it might
remove one outer loop indeed. The function has two callers

void PipelineHandler::completeRequest(Request *request)

which indeed can get to Camera *

and void Request::Private::emitPrepareCompleted()

which can get to a Camera * as well

>  {
> -	while (!waitingRequests_.empty()) {
> -		Request *request = waitingRequests_.front();
> -		if (!request->_d()->prepared_)
> -			break;
> +	for (const std::weak_ptr<Camera> &ptr : cameras_) {

Why a weak_ptr ? Do we expect cameras_ to change while we're in this
thread ? Can't this be just

	for (auto &camera : cameras_) {


> +		std::shared_ptr<Camera> camera = ptr.lock();
> +		if (!camera)
> +			continue;

I don't think cameras_ can be modified by another thread ?

>
> -		doQueueRequest(request);
> -		waitingRequests_.pop();
> +		Camera::Private *data = camera->_d();
> +		while (!data->waitingRequests_.empty()) {
> +			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();
>  }
>
>  /**
> --
> 2.43.0
>