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

[Kieran Bingham]

Quoting Stefan Klug (2025-05-26 22:42:15)
> 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.
> 

Eeek.


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

Yikes, that was plain wrong wasn't it - it would cancel another cameras
queued requests if two cameras were running in parallel from the same
pipeline (which may not be common, but could be possible).

So this definitely seems like a worth while change.
Possibly even a fixes tag, but I won't dwell on that.


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

There's so much operating on the Camera::Private *data here I wonder if
there should be a Camera::Private::Stop() ... but I'm not sure that
would be helpful - we probably want to keep the Camera::Private as a
simple structure.


>  
>         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()) {
> +                       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);
>         }
> +

	/* Allow any waiting requests to be queued to the pipeline. */

> +       doQueueRequests();

I think I saw a later reply also suggest but perhaps:
	doQueueRequests(camera);

could simplify - so that PipelineHandler::doQueueRequests() doesn't
iterate over cameras which are not operating...

At least being explicit would help make sure we don't have bugs that
cameras only wake up and continue because of an event on a different
camera ... ... unless that's desired behaviour?


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