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

[Sven Püschel]

Hi Barnabás,

On 5/27/25 09:27, Barnabás Pőcze wrote:
> 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?

This results from the fact that the patchset splits the waitingRequests_ 
from the PipelineHandler to the (multiple) individual Camera classes. 
Therefore an additional outer loop is necessary to iterate though all 
camera class instances holding potential waitingRequests_.

I would propose to add a Camera shared pointer as an argument to the 
doQueueRequests function (and adjust the signal) to avoid looping over 
all cameras.

>
>
>> +            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.

yeah, exactly. I agree to move it to the next patch.


Sincerely

     Sven

>
>
> Regards,
> Barnabás Pőcze
>
>
>>   }
>>
>>   /**
>> -- 
>> 2.43.0
>>
>
>