| Message ID | 20250630081126.2384387-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, On Mon, Jun 30, 2025 at 10:11:16AM +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. I would just expand on the influence of this common waiting queue, for e.g. on the stop() cycle, which seems to be a bug now (and fixed with this patch). > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v1: > - Added camera param to doQueueRequests() which allows to remove a loop > --- > include/libcamera/internal/camera.h | 2 ++ > include/libcamera/internal/pipeline_handler.h | 5 +--- > src/libcamera/pipeline_handler.cpp | 26 ++++++++++++------- > 3 files changed, 19 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..be017ad47219 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> > @@ -89,13 +88,11 @@ private: > virtual void disconnect(); > > void doQueueRequest(Request *request); > - void doQueueRequests(); > + void doQueueRequests(Camera *camera); > > 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..dc4086aa9bb5 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; > @@ -414,7 +415,9 @@ void PipelineHandler::registerRequest(Request *request) > * Connect the request prepared signal to notify the pipeline handler > * when a request is ready to be processed. > */ > - request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > + request->_d()->prepared.connect(this, [&]() { > + doQueueRequests(request->_d()->camera()); > + }); > } > > /** > @@ -444,7 +447,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); > } > @@ -478,15 +483,16 @@ void PipelineHandler::doQueueRequest(Request *request) > * Iterate the list of waiting requests and queue them to the device one > * by one if they have been prepared. > */ > -void PipelineHandler::doQueueRequests() > +void PipelineHandler::doQueueRequests(Camera *camera) > { > - while (!waitingRequests_.empty()) { > - Request *request = waitingRequests_.front(); > + Camera::Private *data = camera->_d(); > + while (!data->waitingRequests_.empty()) { > + Request *request = data->waitingRequests_.front(); > if (!request->_d()->prepared_) > break; > > doQueueRequest(request); nit: I wonder if it would be beneficial (probably in a follow up patch) to expand the doQueueRequest() function here itself, as the loop is fetching Camera::Private and Camera pointers again, which is readily available above, along with request (meant to be queued). Other than that, this looks good to me: Reviewed-by: Umang Jain <uajain@igalia.com> > - waitingRequests_.pop(); > + data->waitingRequests_.pop(); > } > } > > -- > 2.48.1 >
Hi 2025. 06. 30. 10:11 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> > > --- > > Changes in v1: > - Added camera param to doQueueRequests() which allows to remove a loop > --- > include/libcamera/internal/camera.h | 2 ++ > include/libcamera/internal/pipeline_handler.h | 5 +--- > src/libcamera/pipeline_handler.cpp | 26 ++++++++++++------- > 3 files changed, 19 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..be017ad47219 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> > @@ -89,13 +88,11 @@ private: > virtual void disconnect(); > > void doQueueRequest(Request *request); > - void doQueueRequests(); > + void doQueueRequests(Camera *camera); > > 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..dc4086aa9bb5 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; > @@ -414,7 +415,9 @@ void PipelineHandler::registerRequest(Request *request) > * Connect the request prepared signal to notify the pipeline handler > * when a request is ready to be processed. > */ > - request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > + request->_d()->prepared.connect(this, [&]() { This captures `request` by reference, which is a local variable and will not exists when this callback runs. It should be `[request]` or similar. This likely explains the CI failure: https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1460735 Regards, Barnabás Pőcze > + doQueueRequests(request->_d()->camera()); > + }); > } > > /** > @@ -444,7 +447,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); > } > @@ -478,15 +483,16 @@ void PipelineHandler::doQueueRequest(Request *request) > * Iterate the list of waiting requests and queue them to the device one > * by one if they have been prepared. > */ > -void PipelineHandler::doQueueRequests() > +void PipelineHandler::doQueueRequests(Camera *camera) > { > - while (!waitingRequests_.empty()) { > - Request *request = waitingRequests_.front(); > + Camera::Private *data = camera->_d(); > + while (!data->waitingRequests_.empty()) { > + Request *request = data->waitingRequests_.front(); > if (!request->_d()->prepared_) > break; > > doQueueRequest(request); > - waitingRequests_.pop(); > + data->waitingRequests_.pop(); > } > } >
Hi Barnabás, Quoting Barnabás Pőcze (2025-06-30 13:00:58) > Hi > > 2025. 06. 30. 10:11 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> > > > > --- > > > > Changes in v1: > > - Added camera param to doQueueRequests() which allows to remove a loop > > --- > > include/libcamera/internal/camera.h | 2 ++ > > include/libcamera/internal/pipeline_handler.h | 5 +--- > > src/libcamera/pipeline_handler.cpp | 26 ++++++++++++------- > > 3 files changed, 19 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..be017ad47219 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> > > @@ -89,13 +88,11 @@ private: > > virtual void disconnect(); > > > > void doQueueRequest(Request *request); > > - void doQueueRequests(); > > + void doQueueRequests(Camera *camera); > > > > 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..dc4086aa9bb5 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; > > @@ -414,7 +415,9 @@ void PipelineHandler::registerRequest(Request *request) > > * Connect the request prepared signal to notify the pipeline handler > > * when a request is ready to be processed. > > */ > > - request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); > > + request->_d()->prepared.connect(this, [&]() { > > This captures `request` by reference, which is a local variable and will > not exists when this callback runs. It should be `[request]` or similar. > > This likely explains the CI failure: https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1460735 Ouch, that is a silly one. One of theses if it compiles it runs cases. I'll fix that. Thanks, Stefan > > > Regards, > Barnabás Pőcze > > > > + doQueueRequests(request->_d()->camera()); > > + }); > > } > > > > /** > > @@ -444,7 +447,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); > > } > > @@ -478,15 +483,16 @@ void PipelineHandler::doQueueRequest(Request *request) > > * Iterate the list of waiting requests and queue them to the device one > > * by one if they have been prepared. > > */ > > -void PipelineHandler::doQueueRequests() > > +void PipelineHandler::doQueueRequests(Camera *camera) > > { > > - while (!waitingRequests_.empty()) { > > - Request *request = waitingRequests_.front(); > > + Camera::Private *data = camera->_d(); > > + while (!data->waitingRequests_.empty()) { > > + Request *request = data->waitingRequests_.front(); > > if (!request->_d()->prepared_) > > break; > > > > doQueueRequest(request); > > - waitingRequests_.pop(); > > + data->waitingRequests_.pop(); > > } > > } > > >
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..be017ad47219 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> @@ -89,13 +88,11 @@ private: virtual void disconnect(); void doQueueRequest(Request *request); - void doQueueRequests(); + void doQueueRequests(Camera *camera); 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..dc4086aa9bb5 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; @@ -414,7 +415,9 @@ void PipelineHandler::registerRequest(Request *request) * Connect the request prepared signal to notify the pipeline handler * when a request is ready to be processed. */ - request->_d()->prepared.connect(this, &PipelineHandler::doQueueRequests); + request->_d()->prepared.connect(this, [&]() { + doQueueRequests(request->_d()->camera()); + }); } /** @@ -444,7 +447,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); } @@ -478,15 +483,16 @@ void PipelineHandler::doQueueRequest(Request *request) * Iterate the list of waiting requests and queue them to the device one * by one if they have been prepared. */ -void PipelineHandler::doQueueRequests() +void PipelineHandler::doQueueRequests(Camera *camera) { - while (!waitingRequests_.empty()) { - Request *request = waitingRequests_.front(); + Camera::Private *data = camera->_d(); + while (!data->waitingRequests_.empty()) { + Request *request = data->waitingRequests_.front(); if (!request->_d()->prepared_) break; doQueueRequest(request); - waitingRequests_.pop(); + data->waitingRequests_.pop(); } }
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> --- Changes in v1: - Added camera param to doQueueRequests() which allows to remove a loop --- include/libcamera/internal/camera.h | 2 ++ include/libcamera/internal/pipeline_handler.h | 5 +--- src/libcamera/pipeline_handler.cpp | 26 ++++++++++++------- 3 files changed, 19 insertions(+), 14 deletions(-)