| Message ID | 20250526214224.13631-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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 >> > >
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@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 >
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@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 >
Hi Jacopo, Thank you for the review and sorry for the late feedback. Quoting Jacopo Mondi (2025-05-30 10:59:55) > 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@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 > ? I was unsure about this, too. But having something like a std::map<Camera*, std::queue<Request *>> mapOfWaitingRequests_ also felt a bit cumbersome, when there is already a internal per camera storage. I don't think the current solution is a big breach of architecture, but I can move it into the Pipeline handler if you want. Up to you. > > > 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 If I remember correctly, I tried with auto& but then realized cameras_ is a vector of std::weak_ptr<Camera> so I removed the auto to make that fact clear. And afaik there is no easier way to iterate over such a vector than to lock every entry. Or do I miss some c++ magic here? But anyways, as suggested by the former reviewer, the loop will vanish in the next version. > > 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 ? I don't think so either. But I have to properly handle the weak_ptr. Or do I miss something there? Best regards, Stefan > > > > > - 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 > >
Quoting Stefan Klug (2025-06-24 15:35:35) > Hi Jacopo, > > Thank you for the review and sorry for the late feedback. > > Quoting Jacopo Mondi (2025-05-30 10:59:55) > > 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@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 > > ? > > I was unsure about this, too. But having something like a > std::map<Camera*, std::queue<Request *>> mapOfWaitingRequests_ also felt > a bit cumbersome, when there is already a internal per camera storage. > > I don't think the current solution is a big breach of architecture, but > I can move it into the Pipeline handler if you want. Up to you. > Stiring in here - personally - I think this wait queue is a property of the camera - and it's reasonable to keep it directly associated with the camera. I wouldn't want to have to keep some arbitrary map of extra private camera information in (the base) pipeline handler when we have a private camera data which maps to this use case already. -- Kieran > > > > > 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 > > If I remember correctly, I tried with auto& but then realized cameras_ > is a vector of std::weak_ptr<Camera> so I removed the auto to make that > fact clear. And afaik there is no easier way to iterate over such a > vector than to lock every entry. Or do I miss some c++ magic here? > > But anyways, as suggested by the former reviewer, the loop will vanish > in the next version. > > > > > 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 ? > > I don't think so either. But I have to properly handle the weak_ptr. Or > do I miss something there? > > Best regards, > Stefan > > > > > > > > > - 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 > > >
Hi Kieran, Stefan On Tue, Jun 24, 2025 at 03:46:29PM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2025-06-24 15:35:35) > > Hi Jacopo, > > > > Thank you for the review and sorry for the late feedback. > > > > Quoting Jacopo Mondi (2025-05-30 10:59:55) > > > 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@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 > > > ? > > > > I was unsure about this, too. But having something like a > > std::map<Camera*, std::queue<Request *>> mapOfWaitingRequests_ also felt > > a bit cumbersome, when there is already a internal per camera storage. > > > > I don't think the current solution is a big breach of architecture, but > > I can move it into the Pipeline handler if you want. Up to you. > > > > Stiring in here - personally - I think this wait queue is a property of > the camera - and it's reasonable to keep it directly associated with the > camera. I agree with the association, but the queue of requests is only accessed from the pipeline handler base class > > I wouldn't want to have to keep some arbitrary map of extra private > camera information in (the base) pipeline handler when we have a private > camera data which maps to this use case already. I don't agree this is "extra private camera information" Requests are queued to the pipeline handler base class (which already has a queue of waiting requests btw). The Camera class mostly bridges the queueRequest call between the application and the library thread. But that's not a big deal, whatever you two like the most. > > > > > > > > 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 > > > > If I remember correctly, I tried with auto& but then realized cameras_ > > is a vector of std::weak_ptr<Camera> so I removed the auto to make that > > fact clear. And afaik there is no easier way to iterate over such a > > vector than to lock every entry. Or do I miss some c++ magic here? > > > > But anyways, as suggested by the former reviewer, the loop will vanish > > in the next version. > > sure > > > > > > 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 ? > > > > I don't think so either. But I have to properly handle the weak_ptr. Or > > do I miss something there? Yeah my point was that the check might not be necessary (however doesn't hurt ?) > > > > Best regards, > > Stefan > > > > > > > > > > > > > - 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 > > > >
On Tue, Jun 24, 2025 at 05:49:50PM +0200, Jacopo Mondi wrote: > On Tue, Jun 24, 2025 at 03:46:29PM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2025-06-24 15:35:35) > > > Quoting Jacopo Mondi (2025-05-30 10:59:55) > > > > 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@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 > > > > ? > > > > > > I was unsure about this, too. But having something like a > > > std::map<Camera*, std::queue<Request *>> mapOfWaitingRequests_ also felt > > > a bit cumbersome, when there is already a internal per camera storage. > > > > > > I don't think the current solution is a big breach of architecture, but > > > I can move it into the Pipeline handler if you want. Up to you. > > > > Stiring in here - personally - I think this wait queue is a property of > > the camera - and it's reasonable to keep it directly associated with the > > camera. > > I agree with the association, but the queue of requests is only > accessed from the pipeline handler base class True, but it's internal to the core classes of libcamera, I don't think it's a big deal. I also think it's best to keep the queue in Camera::Private. Camera::Private and PipelineHandler are designed to work together, they're not independently developed components. > > I wouldn't want to have to keep some arbitrary map of extra private > > camera information in (the base) pipeline handler when we have a private > > camera data which maps to this use case already. > > I don't agree this is "extra private camera information" > > Requests are queued to the pipeline handler base class (which already > has a queue of waiting requests btw). The Camera class mostly bridges the > queueRequest call between the application and the library thread. > > But that's not a big deal, whatever you two like the most. > > > > > > 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 > > > > > > If I remember correctly, I tried with auto& but then realized cameras_ > > > is a vector of std::weak_ptr<Camera> so I removed the auto to make that > > > fact clear. And afaik there is no easier way to iterate over such a > > > vector than to lock every entry. Or do I miss some c++ magic here? > > > > > > But anyways, as suggested by the former reviewer, the loop will vanish > > > in the next version. > > sure > > > > > > > > > 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 ? > > > > > > I don't think so either. But I have to properly handle the weak_ptr. Or > > > do I miss something there? > > Yeah my point was that the check might not be necessary (however > doesn't hurt ?) > > > > > > - 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(); > > > > > } > > > > > > > > > > /**
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()) { + 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(); } /**
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@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(-)