| Message ID | 20250924124713.3361707-7-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabas Thanks very much for sending this. I'm very excited to see the question of controls and how they relate to requests move forward! On Wed, 24 Sept 2025 at 13:47, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Add `Camera::applyControls()` whose purpose is to apply controls as soon > as possible, without going through `Request::controls()`. > > A new virtual function `PipelineHandler::applyControlsDevice()` is provided > for pipeline handler to implement fast-tracked application of controls. If > the pipeline handler does not implement that functionality, or it fails, > then a fallback mechanism is used. The controls will be saved for later, > and they will be merged into the control list of the next request sent to > the pipeline handler (`Camera::Private::waitingRequests_`). First of all, and I don't know if you're aware of this, at Raspberry Pi we did quite a lot of work on this question so perhaps it's worth covering that first. We implemented our scheme and have demonstrated it on and off over the last few years, most recently at the Nice workshop. Our implementation didn't present a separate mechanism to submit controls, instead we still relied on them being passed in the request. Though internally, we effectively ran a separate queue containing only the non-empty control lists, so that all the requests without a control list didn't get in the way. Having said that, I like the suggestion of the separate API that you presented, and I think it would work well for us so long as it's backed by a queue. Then we'd be able, for example, to queue up multiple different exposure values and have them applied on consecutive frames. The other feature of our scheme was that every control list was assigned an id number, starting at 1 and then incrementing by 1 with every new control list (not request). This id number would be reported back in a completed request, telling you exactly which the most recent control list was that had been applied. It was also valuable for detecting when things went wrong (via skipped or duplicated id numbers). Finally, those taking part in the Kamaros working group will appreciate that the above, with the queue mechanism, is very close to what's in the Kamaros spec. The principal difference is that Kamaros lets you have more than one such queue. For example, you might have some part of your system pushing AGC/AEC type controls into one queue, and possibly keeping it quite full, while another part of the system responds quickly to user input (perhaps wanting an auto-focus cycle). Best regards David > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/camera.h | 1 + > include/libcamera/internal/camera.h | 1 + > include/libcamera/internal/pipeline_handler.h | 7 ++ > src/libcamera/camera.cpp | 53 +++++++++ > src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- > 5 files changed, 171 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index b24a29740..94f940496 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -147,6 +147,7 @@ public: > > std::unique_ptr<Request> createRequest(uint64_t cookie = 0); > int queueRequest(Request *request); > + int applyControls(ControlList &&controls); > > int start(const ControlList *controls = nullptr); > int stop(); > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index 0f50c14a5..5a77298b3 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -38,6 +38,7 @@ public: > > std::list<Request *> queuedRequests_; > std::deque<Request *> waitingRequests_; > + ControlList pendingControls_; > ControlInfoMap controlInfo_; > ControlList properties_; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index e89d6a33e..16ff54bab 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -57,6 +57,7 @@ public: > > void registerRequest(Request *request); > void queueRequest(Request *request); > + int applyControls(Camera *camera, ControlList &&controls); > > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > @@ -75,6 +76,12 @@ protected: > void hotplugMediaDevice(MediaDevice *media); > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > + > + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) > + { > + return -EOPNOTSUPP; > + } > + > virtual void stopDevice(Camera *camera) = 0; > > virtual bool acquireDevice(Camera *camera); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2e1e146a2..778445e36 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -637,6 +637,14 @@ Camera::Private::~Private() > * queued requests was reached. > */ > > +/** > + * \var Camera::Private::pendingControls_ > + * \brief The list of pending controls > + * > + * This list tracks the controls that need to be applied to the device with the > + * next request going to PipelineHandler::queueRequestDevice(). > + */ > + > /** > * \var Camera::Private::controlInfo_ > * \brief The set of controls supported by the camera > @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) > return 0; > } > > +/** > + * \brief Apply controls immediately > + * \param[in] controls The list of controls to apply > + * > + * This function tries to ensure that the controls in \a controls are applied > + * to the camera as soon as possible. > + * > + * The exact guarantees are camera dependent, but it is guaranteed that the > + * controls will be applied no later than if they were part of the next \ref Request "request" > + * that the application \ref Camera::queueRequest() "queues". > + * > + * \context This function is \threadsafe. It may only be called when the camera > + * is in the Running state as defined in \ref camera_operation. > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not running > + */ > +int Camera::applyControls(ControlList &&controls) > +{ > + Private *const d = _d(); > + > + /* > + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? > + * If yes, should that list of pending controls be overwritten/updated when stopped? > + * And should it be cleared in `Camera::release()`? > + */ > + > + int ret = d->isAccessAllowed(Private::CameraRunning); > + if (ret < 0) > + return ret; > + > + if (controls.empty()) > + return 0; > + > + patchControlList(controls); > + > + /* > + * \todo Or `ConnectionTypeBlocking` to get the return value? > + */ > + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); > + > + return 0; > +} > + > /** > * \brief Start capture from camera > * \param[in] controls Controls to be applied before starting the Camera > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index d0d3fbc79..99bb2193f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) > ASSERT(data->queuedRequests_.empty()); > ASSERT(data->waitingRequests_.empty()); > > + /* > + * \todo Or not? This probably needs discussions regarding > + * the expected state of controls after a stop-start sequence. > + */ > + data->pendingControls_.clear(); > + > data->requestSequence_ = 0; > } > > @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) > request->_d()->prepare(300ms); > } > > +/** > + * \brief Apply controls immediately > + * \param[in] camera The camera > + * \param[in] controls The controls to apply > + * > + * This function tries to apply \a controls immediately to the device by > + * calling applyControlsDevice(). If that fails, then a fallback mechanism > + * is used to ensure that \a controls will be merged into the control list > + * of the next request submitted to the pipeline handler. > + * > + * This function also ensures that requests in Camera::Private::waitingRequests_ > + * will not override the fast-tracked controls. > + * > + * \context This function is called from the CameraManager thread. > + */ > +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) > +{ > + Camera::Private *data = camera->_d(); > + int res = applyControlsDevice(camera, controls); > + > + /* > + * Prevent later requests from overriding the fast-tracked controls. > + * \todo This is unfortunately slow. > + */ > + for (const auto &[k, v] : controls) { > + for (Request *r : data->waitingRequests_) > + r->controls().erase(k); > + } > + > + if (res < 0) { > + /* > + * \todo Always fall back or only if EOPNOTSUPP is returned? > + * Possibly there could be some way for the user to influence > + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). > + */ > + if (res != -EOPNOTSUPP) > + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; > + > + /* > + * Fall back to adding the controls to the next request that enters the > + * pipeline handler. See PipelineHandler::doQueueRequest(). > + */ > + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); > + } > + > + return 0; > +} > + > /** > * \brief Queue one requests to the device > */ > @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) > return; > } > > + if (!data->pendingControls_.empty()) { > + /* > + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is > + * needed to ensure that if `request` is newer than pendingControls_, > + * then its controls take precedence. > + * > + * For older requests, `PipelineHandler::applyControls()` has already removed > + * the controls that should be overridden. > + * > + * \todo How to handle (if at all) conflicting controls? > + * \todo How to handle failure of `queueRequestDevice()`? > + * After the merge it becomes impossible to retrieve the pending controls > + * from `request->controls()`. Making a copy in such a hot-path is not ideal. > + */ > + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); > + data->pendingControls_.clear(); > + } > + > int ret = queueRequestDevice(camera, request); > - if (ret) > + if (ret) { > + /* > + * \todo What to do now? > + * > + * The original `pendingControls_` is not easily recoverable. > + * > + * Initially > + * request->controls() = A u B > + * pendingControls_ = A' u C > + * > + * then after the above merge > + * request->controls() = A u B u C > + * pendingControls_ = A' > + * > + * so recovering `pendingControls_` without a copy of at least > + * `B` or `C` does not appear easy. > + * > + * But sometimes recovering is not even desirable: assume that the controls > + * in `pendingControls_` causes the failure. In that case keeping them around > + * will cause every single subsequent request to fail. > + */ > + > cancelRequest(request); > + } > } > > /** > @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) > * \return 0 on success or a negative error code otherwise > */ > > +/** > + * \fn PipelineHandler::applyControlsDevice() > + * \brief Apply controls immediately > + * \param[in] camera The camera > + * \param[in] controls The controls to apply > + * > + * This function applies \a controls to \a camera immediately. > + * > + * \context This function is called from the CameraManager thread. > + * > + * \return 0 on success or a negative error code otherwise > + * \return -EOPNOTSUPP if fast-tracking controls is not supported > + */ > + > /** > * \brief Complete a buffer for a request > * \param[in] request The request the buffer belongs to > -- > 2.51.0 >
Hi 2025. 09. 26. 10:46 keltezéssel, David Plowman írta: > Hi Barnabas > > Thanks very much for sending this. I'm very excited to see the > question of controls and how they relate to requests move forward! > > On Wed, 24 Sept 2025 at 13:47, Barnabás Pőcze > <barnabas.pocze@ideasonboard.com> wrote: >> >> Add `Camera::applyControls()` whose purpose is to apply controls as soon >> as possible, without going through `Request::controls()`. >> >> A new virtual function `PipelineHandler::applyControlsDevice()` is provided >> for pipeline handler to implement fast-tracked application of controls. If >> the pipeline handler does not implement that functionality, or it fails, >> then a fallback mechanism is used. The controls will be saved for later, >> and they will be merged into the control list of the next request sent to >> the pipeline handler (`Camera::Private::waitingRequests_`). > > First of all, and I don't know if you're aware of this, at Raspberry > Pi we did quite a lot of work on this question so perhaps it's worth > covering that first. We implemented our scheme and have demonstrated > it on and off over the last few years, most recently at the Nice > workshop. Yes, I think I have seen it. > > Our implementation didn't present a separate mechanism to submit > controls, instead we still relied on them being passed in the > request. Though internally, we effectively ran a separate queue > containing only the non-empty control lists, so that all the requests > without a control list didn't get in the way. Bypassing the request mechanism is kind of the intention here. Specifically, the idea is to support applications that keep many requests queued at any given time, but still want to be able to update controls "immediately". In that case setting the desired controls as part of the next request causes unnecessary latency. An alternative option is to let the application be responsible for keeping "just enough" requests queued at any given moment, and keep an application-side queue for the "rest" of the ready request. In that case it can set the controls on the first request in the application-side queue, reducing the latency. However, this queue would essentially be the same as `Camera::Private::waitingRequests_`, so the idea here is to try to provide a simpler mechanism. I have briefly looked at https://github.com/raspberrypi/libcamera/tree/pfc_2025 (I assume this is the one), and I could be misremembering, but I feel like the proposed mechanism here addresses a use-case somewhat different from what your scheme addresses? Or I could be missing a more fundamental connection somewhere? Regards, Barnabás Pőcze > > Having said that, I like the suggestion of the separate API that you > presented, and I think it would work well for us so long as it's > backed by a queue. Then we'd be able, for example, to queue up > multiple different exposure values and have them applied on > consecutive frames. > > The other feature of our scheme was that every control list was > assigned an id number, starting at 1 and then incrementing by 1 with > every new control list (not request). This id number would be reported > back in a completed request, telling you exactly which the most > recent control list was that had been applied. It was also valuable > for detecting when things went wrong (via skipped or duplicated id > numbers). > > Finally, those taking part in the Kamaros working group will > appreciate that the above, with the queue mechanism, is very close to > what's in the Kamaros spec. The principal difference is that Kamaros > lets you have more than one such queue. For example, you might have > some part of your system pushing AGC/AEC type controls into one > queue, and possibly keeping it quite full, while another part of the > system responds quickly to user input (perhaps wanting an auto-focus > cycle). > > Best regards > David > >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/camera.h | 1 + >> include/libcamera/internal/camera.h | 1 + >> include/libcamera/internal/pipeline_handler.h | 7 ++ >> src/libcamera/camera.cpp | 53 +++++++++ >> src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- >> 5 files changed, 171 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h >> index b24a29740..94f940496 100644 >> --- a/include/libcamera/camera.h >> +++ b/include/libcamera/camera.h >> @@ -147,6 +147,7 @@ public: >> >> std::unique_ptr<Request> createRequest(uint64_t cookie = 0); >> int queueRequest(Request *request); >> + int applyControls(ControlList &&controls); >> >> int start(const ControlList *controls = nullptr); >> int stop(); >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h >> index 0f50c14a5..5a77298b3 100644 >> --- a/include/libcamera/internal/camera.h >> +++ b/include/libcamera/internal/camera.h >> @@ -38,6 +38,7 @@ public: >> >> std::list<Request *> queuedRequests_; >> std::deque<Request *> waitingRequests_; >> + ControlList pendingControls_; >> ControlInfoMap controlInfo_; >> ControlList properties_; >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >> index e89d6a33e..16ff54bab 100644 >> --- a/include/libcamera/internal/pipeline_handler.h >> +++ b/include/libcamera/internal/pipeline_handler.h >> @@ -57,6 +57,7 @@ public: >> >> void registerRequest(Request *request); >> void queueRequest(Request *request); >> + int applyControls(Camera *camera, ControlList &&controls); >> >> bool completeBuffer(Request *request, FrameBuffer *buffer); >> void completeRequest(Request *request); >> @@ -75,6 +76,12 @@ protected: >> void hotplugMediaDevice(MediaDevice *media); >> >> virtual int queueRequestDevice(Camera *camera, Request *request) = 0; >> + >> + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) >> + { >> + return -EOPNOTSUPP; >> + } >> + >> virtual void stopDevice(Camera *camera) = 0; >> >> virtual bool acquireDevice(Camera *camera); >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 2e1e146a2..778445e36 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -637,6 +637,14 @@ Camera::Private::~Private() >> * queued requests was reached. >> */ >> >> +/** >> + * \var Camera::Private::pendingControls_ >> + * \brief The list of pending controls >> + * >> + * This list tracks the controls that need to be applied to the device with the >> + * next request going to PipelineHandler::queueRequestDevice(). >> + */ >> + >> /** >> * \var Camera::Private::controlInfo_ >> * \brief The set of controls supported by the camera >> @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) >> return 0; >> } >> >> +/** >> + * \brief Apply controls immediately >> + * \param[in] controls The list of controls to apply >> + * >> + * This function tries to ensure that the controls in \a controls are applied >> + * to the camera as soon as possible. >> + * >> + * The exact guarantees are camera dependent, but it is guaranteed that the >> + * controls will be applied no later than if they were part of the next \ref Request "request" >> + * that the application \ref Camera::queueRequest() "queues". >> + * >> + * \context This function is \threadsafe. It may only be called when the camera >> + * is in the Running state as defined in \ref camera_operation. >> + * >> + * \return 0 on success or a negative error code otherwise >> + * \retval -ENODEV The camera has been disconnected from the system >> + * \retval -EACCES The camera is not running >> + */ >> +int Camera::applyControls(ControlList &&controls) >> +{ >> + Private *const d = _d(); >> + >> + /* >> + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? >> + * If yes, should that list of pending controls be overwritten/updated when stopped? >> + * And should it be cleared in `Camera::release()`? >> + */ >> + >> + int ret = d->isAccessAllowed(Private::CameraRunning); >> + if (ret < 0) >> + return ret; >> + >> + if (controls.empty()) >> + return 0; >> + >> + patchControlList(controls); >> + >> + /* >> + * \todo Or `ConnectionTypeBlocking` to get the return value? >> + */ >> + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); >> + >> + return 0; >> +} >> + >> /** >> * \brief Start capture from camera >> * \param[in] controls Controls to be applied before starting the Camera >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index d0d3fbc79..99bb2193f 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) >> ASSERT(data->queuedRequests_.empty()); >> ASSERT(data->waitingRequests_.empty()); >> >> + /* >> + * \todo Or not? This probably needs discussions regarding >> + * the expected state of controls after a stop-start sequence. >> + */ >> + data->pendingControls_.clear(); >> + >> data->requestSequence_ = 0; >> } >> >> @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) >> request->_d()->prepare(300ms); >> } >> >> +/** >> + * \brief Apply controls immediately >> + * \param[in] camera The camera >> + * \param[in] controls The controls to apply >> + * >> + * This function tries to apply \a controls immediately to the device by >> + * calling applyControlsDevice(). If that fails, then a fallback mechanism >> + * is used to ensure that \a controls will be merged into the control list >> + * of the next request submitted to the pipeline handler. >> + * >> + * This function also ensures that requests in Camera::Private::waitingRequests_ >> + * will not override the fast-tracked controls. >> + * >> + * \context This function is called from the CameraManager thread. >> + */ >> +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) >> +{ >> + Camera::Private *data = camera->_d(); >> + int res = applyControlsDevice(camera, controls); >> + >> + /* >> + * Prevent later requests from overriding the fast-tracked controls. >> + * \todo This is unfortunately slow. >> + */ >> + for (const auto &[k, v] : controls) { >> + for (Request *r : data->waitingRequests_) >> + r->controls().erase(k); >> + } >> + >> + if (res < 0) { >> + /* >> + * \todo Always fall back or only if EOPNOTSUPP is returned? >> + * Possibly there could be some way for the user to influence >> + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). >> + */ >> + if (res != -EOPNOTSUPP) >> + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; >> + >> + /* >> + * Fall back to adding the controls to the next request that enters the >> + * pipeline handler. See PipelineHandler::doQueueRequest(). >> + */ >> + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); >> + } >> + >> + return 0; >> +} >> + >> /** >> * \brief Queue one requests to the device >> */ >> @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) >> return; >> } >> >> + if (!data->pendingControls_.empty()) { >> + /* >> + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is >> + * needed to ensure that if `request` is newer than pendingControls_, >> + * then its controls take precedence. >> + * >> + * For older requests, `PipelineHandler::applyControls()` has already removed >> + * the controls that should be overridden. >> + * >> + * \todo How to handle (if at all) conflicting controls? >> + * \todo How to handle failure of `queueRequestDevice()`? >> + * After the merge it becomes impossible to retrieve the pending controls >> + * from `request->controls()`. Making a copy in such a hot-path is not ideal. >> + */ >> + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); >> + data->pendingControls_.clear(); >> + } >> + >> int ret = queueRequestDevice(camera, request); >> - if (ret) >> + if (ret) { >> + /* >> + * \todo What to do now? >> + * >> + * The original `pendingControls_` is not easily recoverable. >> + * >> + * Initially >> + * request->controls() = A u B >> + * pendingControls_ = A' u C >> + * >> + * then after the above merge >> + * request->controls() = A u B u C >> + * pendingControls_ = A' >> + * >> + * so recovering `pendingControls_` without a copy of at least >> + * `B` or `C` does not appear easy. >> + * >> + * But sometimes recovering is not even desirable: assume that the controls >> + * in `pendingControls_` causes the failure. In that case keeping them around >> + * will cause every single subsequent request to fail. >> + */ >> + >> cancelRequest(request); >> + } >> } >> >> /** >> @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) >> * \return 0 on success or a negative error code otherwise >> */ >> >> +/** >> + * \fn PipelineHandler::applyControlsDevice() >> + * \brief Apply controls immediately >> + * \param[in] camera The camera >> + * \param[in] controls The controls to apply >> + * >> + * This function applies \a controls to \a camera immediately. >> + * >> + * \context This function is called from the CameraManager thread. >> + * >> + * \return 0 on success or a negative error code otherwise >> + * \return -EOPNOTSUPP if fast-tracking controls is not supported >> + */ >> + >> /** >> * \brief Complete a buffer for a request >> * \param[in] request The request the buffer belongs to >> -- >> 2.51.0 >>
Hi Barnabás, and thanks for the reply. On Fri, 26 Sept 2025 at 11:21, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Hi > > 2025. 09. 26. 10:46 keltezéssel, David Plowman írta: > > Hi Barnabas > > > > Thanks very much for sending this. I'm very excited to see the > > question of controls and how they relate to requests move forward! > > > > On Wed, 24 Sept 2025 at 13:47, Barnabás Pőcze > > <barnabas.pocze@ideasonboard.com> wrote: > >> > >> Add `Camera::applyControls()` whose purpose is to apply controls as soon > >> as possible, without going through `Request::controls()`. > >> > >> A new virtual function `PipelineHandler::applyControlsDevice()` is provided > >> for pipeline handler to implement fast-tracked application of controls. If > >> the pipeline handler does not implement that functionality, or it fails, > >> then a fallback mechanism is used. The controls will be saved for later, > >> and they will be merged into the control list of the next request sent to > >> the pipeline handler (`Camera::Private::waitingRequests_`). > > > > First of all, and I don't know if you're aware of this, at Raspberry > > Pi we did quite a lot of work on this question so perhaps it's worth > > covering that first. We implemented our scheme and have demonstrated > > it on and off over the last few years, most recently at the Nice > > workshop. > > Yes, I think I have seen it. > > > > > > Our implementation didn't present a separate mechanism to submit > > controls, instead we still relied on them being passed in the > > request. Though internally, we effectively ran a separate queue > > containing only the non-empty control lists, so that all the requests > > without a control list didn't get in the way. > > Bypassing the request mechanism is kind of the intention here. Specifically, > the idea is to support applications that keep many requests queued at any given > time, but still want to be able to update controls "immediately". In that case > setting the desired controls as part of the next request causes unnecessary latency. Yes, this is exactly the use case we were addressing, though we fixed it in a different way. However, I prefer the external mechanism that you have (so long as I can queue up control lists for several consecutive frames). > > An alternative option is to let the application be responsible for keeping "just enough" > requests queued at any given moment, and keep an application-side queue for the "rest" > of the ready request. In that case it can set the controls on the first request in the > application-side queue, reducing the latency. However, this queue would essentially be > the same as `Camera::Private::waitingRequests_`, so the idea here is to try to provide > a simpler mechanism. > > I have briefly looked at https://github.com/raspberrypi/libcamera/tree/pfc_2025 (I assume > this is the one), and I could be misremembering, but I feel like the proposed mechanism here > addresses a use-case somewhat different from what your scheme addresses? Or I could be missing > a more fundamental connection somewhere? The by-passing of the requests in the request queue is essentially for exactly the same purpose. But I agree that identifying which controls have been applied is largely orthogonal. However, there could be some interaction, so there may be some value in considering the two together. For example, perhaps sending a control list to the external queue gives you back an id number, which you would then look for in the request when it completes, so that's a (very small!) API change. (Obviously, figuring out when controls are applied is more difficult, and was probably what a lot of the code in that branch was dealing with...) Best regards David > > > Regards, > Barnabás Pőcze > > > > > Having said that, I like the suggestion of the separate API that you > > presented, and I think it would work well for us so long as it's > > backed by a queue. Then we'd be able, for example, to queue up > > multiple different exposure values and have them applied on > > consecutive frames. > > > > The other feature of our scheme was that every control list was > > assigned an id number, starting at 1 and then incrementing by 1 with > > every new control list (not request). This id number would be reported > > back in a completed request, telling you exactly which the most > > recent control list was that had been applied. It was also valuable > > for detecting when things went wrong (via skipped or duplicated id > > numbers). > > > > Finally, those taking part in the Kamaros working group will > > appreciate that the above, with the queue mechanism, is very close to > > what's in the Kamaros spec. The principal difference is that Kamaros > > lets you have more than one such queue. For example, you might have > > some part of your system pushing AGC/AEC type controls into one > > queue, and possibly keeping it quite full, while another part of the > > system responds quickly to user input (perhaps wanting an auto-focus > > cycle). > > > > Best regards > > David > > > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> include/libcamera/camera.h | 1 + > >> include/libcamera/internal/camera.h | 1 + > >> include/libcamera/internal/pipeline_handler.h | 7 ++ > >> src/libcamera/camera.cpp | 53 +++++++++ > >> src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- > >> 5 files changed, 171 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > >> index b24a29740..94f940496 100644 > >> --- a/include/libcamera/camera.h > >> +++ b/include/libcamera/camera.h > >> @@ -147,6 +147,7 @@ public: > >> > >> std::unique_ptr<Request> createRequest(uint64_t cookie = 0); > >> int queueRequest(Request *request); > >> + int applyControls(ControlList &&controls); > >> > >> int start(const ControlList *controls = nullptr); > >> int stop(); > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > >> index 0f50c14a5..5a77298b3 100644 > >> --- a/include/libcamera/internal/camera.h > >> +++ b/include/libcamera/internal/camera.h > >> @@ -38,6 +38,7 @@ public: > >> > >> std::list<Request *> queuedRequests_; > >> std::deque<Request *> waitingRequests_; > >> + ControlList pendingControls_; > >> ControlInfoMap controlInfo_; > >> ControlList properties_; > >> > >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > >> index e89d6a33e..16ff54bab 100644 > >> --- a/include/libcamera/internal/pipeline_handler.h > >> +++ b/include/libcamera/internal/pipeline_handler.h > >> @@ -57,6 +57,7 @@ public: > >> > >> void registerRequest(Request *request); > >> void queueRequest(Request *request); > >> + int applyControls(Camera *camera, ControlList &&controls); > >> > >> bool completeBuffer(Request *request, FrameBuffer *buffer); > >> void completeRequest(Request *request); > >> @@ -75,6 +76,12 @@ protected: > >> void hotplugMediaDevice(MediaDevice *media); > >> > >> virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > >> + > >> + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) > >> + { > >> + return -EOPNOTSUPP; > >> + } > >> + > >> virtual void stopDevice(Camera *camera) = 0; > >> > >> virtual bool acquireDevice(Camera *camera); > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index 2e1e146a2..778445e36 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -637,6 +637,14 @@ Camera::Private::~Private() > >> * queued requests was reached. > >> */ > >> > >> +/** > >> + * \var Camera::Private::pendingControls_ > >> + * \brief The list of pending controls > >> + * > >> + * This list tracks the controls that need to be applied to the device with the > >> + * next request going to PipelineHandler::queueRequestDevice(). > >> + */ > >> + > >> /** > >> * \var Camera::Private::controlInfo_ > >> * \brief The set of controls supported by the camera > >> @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) > >> return 0; > >> } > >> > >> +/** > >> + * \brief Apply controls immediately > >> + * \param[in] controls The list of controls to apply > >> + * > >> + * This function tries to ensure that the controls in \a controls are applied > >> + * to the camera as soon as possible. > >> + * > >> + * The exact guarantees are camera dependent, but it is guaranteed that the > >> + * controls will be applied no later than if they were part of the next \ref Request "request" > >> + * that the application \ref Camera::queueRequest() "queues". > >> + * > >> + * \context This function is \threadsafe. It may only be called when the camera > >> + * is in the Running state as defined in \ref camera_operation. > >> + * > >> + * \return 0 on success or a negative error code otherwise > >> + * \retval -ENODEV The camera has been disconnected from the system > >> + * \retval -EACCES The camera is not running > >> + */ > >> +int Camera::applyControls(ControlList &&controls) > >> +{ > >> + Private *const d = _d(); > >> + > >> + /* > >> + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? > >> + * If yes, should that list of pending controls be overwritten/updated when stopped? > >> + * And should it be cleared in `Camera::release()`? > >> + */ > >> + > >> + int ret = d->isAccessAllowed(Private::CameraRunning); > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (controls.empty()) > >> + return 0; > >> + > >> + patchControlList(controls); > >> + > >> + /* > >> + * \todo Or `ConnectionTypeBlocking` to get the return value? > >> + */ > >> + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * \brief Start capture from camera > >> * \param[in] controls Controls to be applied before starting the Camera > >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > >> index d0d3fbc79..99bb2193f 100644 > >> --- a/src/libcamera/pipeline_handler.cpp > >> +++ b/src/libcamera/pipeline_handler.cpp > >> @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) > >> ASSERT(data->queuedRequests_.empty()); > >> ASSERT(data->waitingRequests_.empty()); > >> > >> + /* > >> + * \todo Or not? This probably needs discussions regarding > >> + * the expected state of controls after a stop-start sequence. > >> + */ > >> + data->pendingControls_.clear(); > >> + > >> data->requestSequence_ = 0; > >> } > >> > >> @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) > >> request->_d()->prepare(300ms); > >> } > >> > >> +/** > >> + * \brief Apply controls immediately > >> + * \param[in] camera The camera > >> + * \param[in] controls The controls to apply > >> + * > >> + * This function tries to apply \a controls immediately to the device by > >> + * calling applyControlsDevice(). If that fails, then a fallback mechanism > >> + * is used to ensure that \a controls will be merged into the control list > >> + * of the next request submitted to the pipeline handler. > >> + * > >> + * This function also ensures that requests in Camera::Private::waitingRequests_ > >> + * will not override the fast-tracked controls. > >> + * > >> + * \context This function is called from the CameraManager thread. > >> + */ > >> +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) > >> +{ > >> + Camera::Private *data = camera->_d(); > >> + int res = applyControlsDevice(camera, controls); > >> + > >> + /* > >> + * Prevent later requests from overriding the fast-tracked controls. > >> + * \todo This is unfortunately slow. > >> + */ > >> + for (const auto &[k, v] : controls) { > >> + for (Request *r : data->waitingRequests_) > >> + r->controls().erase(k); > >> + } > >> + > >> + if (res < 0) { > >> + /* > >> + * \todo Always fall back or only if EOPNOTSUPP is returned? > >> + * Possibly there could be some way for the user to influence > >> + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). > >> + */ > >> + if (res != -EOPNOTSUPP) > >> + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; > >> + > >> + /* > >> + * Fall back to adding the controls to the next request that enters the > >> + * pipeline handler. See PipelineHandler::doQueueRequest(). > >> + */ > >> + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * \brief Queue one requests to the device > >> */ > >> @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) > >> return; > >> } > >> > >> + if (!data->pendingControls_.empty()) { > >> + /* > >> + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is > >> + * needed to ensure that if `request` is newer than pendingControls_, > >> + * then its controls take precedence. > >> + * > >> + * For older requests, `PipelineHandler::applyControls()` has already removed > >> + * the controls that should be overridden. > >> + * > >> + * \todo How to handle (if at all) conflicting controls? > >> + * \todo How to handle failure of `queueRequestDevice()`? > >> + * After the merge it becomes impossible to retrieve the pending controls > >> + * from `request->controls()`. Making a copy in such a hot-path is not ideal. > >> + */ > >> + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); > >> + data->pendingControls_.clear(); > >> + } > >> + > >> int ret = queueRequestDevice(camera, request); > >> - if (ret) > >> + if (ret) { > >> + /* > >> + * \todo What to do now? > >> + * > >> + * The original `pendingControls_` is not easily recoverable. > >> + * > >> + * Initially > >> + * request->controls() = A u B > >> + * pendingControls_ = A' u C > >> + * > >> + * then after the above merge > >> + * request->controls() = A u B u C > >> + * pendingControls_ = A' > >> + * > >> + * so recovering `pendingControls_` without a copy of at least > >> + * `B` or `C` does not appear easy. > >> + * > >> + * But sometimes recovering is not even desirable: assume that the controls > >> + * in `pendingControls_` causes the failure. In that case keeping them around > >> + * will cause every single subsequent request to fail. > >> + */ > >> + > >> cancelRequest(request); > >> + } > >> } > >> > >> /** > >> @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) > >> * \return 0 on success or a negative error code otherwise > >> */ > >> > >> +/** > >> + * \fn PipelineHandler::applyControlsDevice() > >> + * \brief Apply controls immediately > >> + * \param[in] camera The camera > >> + * \param[in] controls The controls to apply > >> + * > >> + * This function applies \a controls to \a camera immediately. > >> + * > >> + * \context This function is called from the CameraManager thread. > >> + * > >> + * \return 0 on success or a negative error code otherwise > >> + * \return -EOPNOTSUPP if fast-tracking controls is not supported > >> + */ > >> + > >> /** > >> * \brief Complete a buffer for a request > >> * \param[in] request The request the buffer belongs to > >> -- > >> 2.51.0 > >> >
Hi Barnabás, Thank you for the patch. Quoting Barnabás Pőcze (2025-09-24 14:47:11) > Add `Camera::applyControls()` whose purpose is to apply controls as soon > as possible, without going through `Request::controls()`. > > A new virtual function `PipelineHandler::applyControlsDevice()` is provided > for pipeline handler to implement fast-tracked application of controls. If > the pipeline handler does not implement that functionality, or it fails, > then a fallback mechanism is used. The controls will be saved for later, > and they will be merged into the control list of the next request sent to > the pipeline handler (`Camera::Private::waitingRequests_`). > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/camera.h | 1 + > include/libcamera/internal/camera.h | 1 + > include/libcamera/internal/pipeline_handler.h | 7 ++ > src/libcamera/camera.cpp | 53 +++++++++ > src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- > 5 files changed, 171 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index b24a29740..94f940496 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -147,6 +147,7 @@ public: > > std::unique_ptr<Request> createRequest(uint64_t cookie = 0); > int queueRequest(Request *request); > + int applyControls(ControlList &&controls); > > int start(const ControlList *controls = nullptr); > int stop(); > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index 0f50c14a5..5a77298b3 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -38,6 +38,7 @@ public: > > std::list<Request *> queuedRequests_; > std::deque<Request *> waitingRequests_; > + ControlList pendingControls_; > ControlInfoMap controlInfo_; > ControlList properties_; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index e89d6a33e..16ff54bab 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -57,6 +57,7 @@ public: > > void registerRequest(Request *request); > void queueRequest(Request *request); > + int applyControls(Camera *camera, ControlList &&controls); > > bool completeBuffer(Request *request, FrameBuffer *buffer); > void completeRequest(Request *request); > @@ -75,6 +76,12 @@ protected: > void hotplugMediaDevice(MediaDevice *media); > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > + > + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) > + { > + return -EOPNOTSUPP; > + } > + > virtual void stopDevice(Camera *camera) = 0; > > virtual bool acquireDevice(Camera *camera); > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2e1e146a2..778445e36 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -637,6 +637,14 @@ Camera::Private::~Private() > * queued requests was reached. > */ > > +/** > + * \var Camera::Private::pendingControls_ > + * \brief The list of pending controls > + * > + * This list tracks the controls that need to be applied to the device with the > + * next request going to PipelineHandler::queueRequestDevice(). > + */ > + > /** > * \var Camera::Private::controlInfo_ > * \brief The set of controls supported by the camera > @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) > return 0; > } > > +/** > + * \brief Apply controls immediately > + * \param[in] controls The list of controls to apply > + * > + * This function tries to ensure that the controls in \a controls are applied > + * to the camera as soon as possible. > + * > + * The exact guarantees are camera dependent, but it is guaranteed that the > + * controls will be applied no later than if they were part of the next \ref Request "request" > + * that the application \ref Camera::queueRequest() "queues". > + * > + * \context This function is \threadsafe. It may only be called when the camera > + * is in the Running state as defined in \ref camera_operation. > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not running > + */ > +int Camera::applyControls(ControlList &&controls) > +{ > + Private *const d = _d(); > + > + /* > + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? > + * If yes, should that list of pending controls be overwritten/updated when stopped? > + * And should it be cleared in `Camera::release()`? To keep things simple I would keep start() and applyControls() separate. Although it seems to be tempting to have applyControls as single entry point for simple usecases. Oh my :-) > + */ > + > + int ret = d->isAccessAllowed(Private::CameraRunning); > + if (ret < 0) > + return ret; > + > + if (controls.empty()) > + return 0; > + > + patchControlList(controls); > + > + /* > + * \todo Or `ConnectionTypeBlocking` to get the return value? > + */ > + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); > + > + return 0; > +} > + > /** > * \brief Start capture from camera > * \param[in] controls Controls to be applied before starting the Camera > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index d0d3fbc79..99bb2193f 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) > ASSERT(data->queuedRequests_.empty()); > ASSERT(data->waitingRequests_.empty()); > > + /* > + * \todo Or not? This probably needs discussions regarding > + * the expected state of controls after a stop-start sequence. > + */ > + data->pendingControls_.clear(); > + > data->requestSequence_ = 0; > } > > @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) > request->_d()->prepare(300ms); > } > > +/** > + * \brief Apply controls immediately > + * \param[in] camera The camera > + * \param[in] controls The controls to apply > + * > + * This function tries to apply \a controls immediately to the device by > + * calling applyControlsDevice(). If that fails, then a fallback mechanism > + * is used to ensure that \a controls will be merged into the control list > + * of the next request submitted to the pipeline handler. > + * > + * This function also ensures that requests in Camera::Private::waitingRequests_ > + * will not override the fast-tracked controls. > + * > + * \context This function is called from the CameraManager thread. > + */ > +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) > +{ > + Camera::Private *data = camera->_d(); > + int res = applyControlsDevice(camera, controls); > + > + /* > + * Prevent later requests from overriding the fast-tracked controls. > + * \todo This is unfortunately slow. I thought about that for a while. I think we will have enough issues defining the behavior for inter requests controls in cases with queue underruns. My take would be to simply document that as undefined/racy behavior and instruct the user to either use applyControls() or queueRequest() for the same control, but not both. This keeps the code and path simple without many downsides. > + */ > + for (const auto &[k, v] : controls) { > + for (Request *r : data->waitingRequests_) > + r->controls().erase(k); > + } > + > + if (res < 0) { > + /* > + * \todo Always fall back or only if EOPNOTSUPP is returned? > + * Possibly there could be some way for the user to influence > + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). > + */ > + if (res != -EOPNOTSUPP) > + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; > + > + /* > + * Fall back to adding the controls to the next request that enters the > + * pipeline handler. See PipelineHandler::doQueueRequest(). > + */ > + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); > + } You could also move this code into the default applyControlsDevice() implementation. Then a pipeline handler can either use the default or implement it's own. No need to handle a mixture of both. > + > + return 0; > +} > + > /** > * \brief Queue one requests to the device > */ > @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) > return; > } > > + if (!data->pendingControls_.empty()) { > + /* > + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is > + * needed to ensure that if `request` is newer than pendingControls_, > + * then its controls take precedence. > + * > + * For older requests, `PipelineHandler::applyControls()` has already removed > + * the controls that should be overridden. > + * > + * \todo How to handle (if at all) conflicting controls? As noted above I don't think we should bother too much :-) > + * \todo How to handle failure of `queueRequestDevice()`? > + * After the merge it becomes impossible to retrieve the pending controls > + * from `request->controls()`. Making a copy in such a hot-path is not ideal. That is correct. But I have a hard time to come up with a use case where this is problematic. And controls is reset internally by request->reuse() so I think this is acceptable. > + */ > + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); > + data->pendingControls_.clear(); > + } > + > int ret = queueRequestDevice(camera, request); > - if (ret) > + if (ret) { > + /* > + * \todo What to do now? > + * > + * The original `pendingControls_` is not easily recoverable. > + * > + * Initially > + * request->controls() = A u B > + * pendingControls_ = A' u C > + * > + * then after the above merge > + * request->controls() = A u B u C > + * pendingControls_ = A' Is that still the case? As you cleared pendingControls_ before. > + * > + * so recovering `pendingControls_` without a copy of at least > + * `B` or `C` does not appear easy. > + * > + * But sometimes recovering is not even desirable: assume that the controls > + * in `pendingControls_` causes the failure. In that case keeping them around > + * will cause every single subsequent request to fail. Do we expect cameras to regularly fail on queueRequest()? I think it is ok to have "lost" the pending controls in this case. Best regards, Stefan > + */ > + > cancelRequest(request); > + } > } > > /** > @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) > * \return 0 on success or a negative error code otherwise > */ > > +/** > + * \fn PipelineHandler::applyControlsDevice() > + * \brief Apply controls immediately > + * \param[in] camera The camera > + * \param[in] controls The controls to apply > + * > + * This function applies \a controls to \a camera immediately. > + * > + * \context This function is called from the CameraManager thread. > + * > + * \return 0 on success or a negative error code otherwise > + * \return -EOPNOTSUPP if fast-tracking controls is not supported > + */ > + > /** > * \brief Complete a buffer for a request > * \param[in] request The request the buffer belongs to > -- > 2.51.0 >
Hi again On Mon, 6 Oct 2025 at 14:56, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > Hi Barnabás, > > Thank you for the patch. > > Quoting Barnabás Pőcze (2025-09-24 14:47:11) > > Add `Camera::applyControls()` whose purpose is to apply controls as soon > > as possible, without going through `Request::controls()`. > > > > A new virtual function `PipelineHandler::applyControlsDevice()` is provided > > for pipeline handler to implement fast-tracked application of controls. If > > the pipeline handler does not implement that functionality, or it fails, > > then a fallback mechanism is used. The controls will be saved for later, > > and they will be merged into the control list of the next request sent to > > the pipeline handler (`Camera::Private::waitingRequests_`). > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > include/libcamera/camera.h | 1 + > > include/libcamera/internal/camera.h | 1 + > > include/libcamera/internal/pipeline_handler.h | 7 ++ > > src/libcamera/camera.cpp | 53 +++++++++ > > src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- > > 5 files changed, 171 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index b24a29740..94f940496 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -147,6 +147,7 @@ public: > > > > std::unique_ptr<Request> createRequest(uint64_t cookie = 0); > > int queueRequest(Request *request); > > + int applyControls(ControlList &&controls); > > > > int start(const ControlList *controls = nullptr); > > int stop(); > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > > index 0f50c14a5..5a77298b3 100644 > > --- a/include/libcamera/internal/camera.h > > +++ b/include/libcamera/internal/camera.h > > @@ -38,6 +38,7 @@ public: > > > > std::list<Request *> queuedRequests_; > > std::deque<Request *> waitingRequests_; > > + ControlList pendingControls_; > > ControlInfoMap controlInfo_; > > ControlList properties_; > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index e89d6a33e..16ff54bab 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -57,6 +57,7 @@ public: > > > > void registerRequest(Request *request); > > void queueRequest(Request *request); > > + int applyControls(Camera *camera, ControlList &&controls); > > > > bool completeBuffer(Request *request, FrameBuffer *buffer); > > void completeRequest(Request *request); > > @@ -75,6 +76,12 @@ protected: > > void hotplugMediaDevice(MediaDevice *media); > > > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0; > > + > > + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) > > + { > > + return -EOPNOTSUPP; > > + } > > + > > virtual void stopDevice(Camera *camera) = 0; > > > > virtual bool acquireDevice(Camera *camera); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 2e1e146a2..778445e36 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -637,6 +637,14 @@ Camera::Private::~Private() > > * queued requests was reached. > > */ > > > > +/** > > + * \var Camera::Private::pendingControls_ > > + * \brief The list of pending controls > > + * > > + * This list tracks the controls that need to be applied to the device with the > > + * next request going to PipelineHandler::queueRequestDevice(). > > + */ > > + > > /** > > * \var Camera::Private::controlInfo_ > > * \brief The set of controls supported by the camera > > @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) > > return 0; > > } > > > > +/** > > + * \brief Apply controls immediately > > + * \param[in] controls The list of controls to apply > > + * > > + * This function tries to ensure that the controls in \a controls are applied > > + * to the camera as soon as possible. > > + * > > + * The exact guarantees are camera dependent, but it is guaranteed that the > > + * controls will be applied no later than if they were part of the next \ref Request "request" > > + * that the application \ref Camera::queueRequest() "queues". > > + * > > + * \context This function is \threadsafe. It may only be called when the camera > > + * is in the Running state as defined in \ref camera_operation. > > + * > > + * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not running > > + */ > > +int Camera::applyControls(ControlList &&controls) > > +{ > > + Private *const d = _d(); > > + > > + /* > > + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? > > + * If yes, should that list of pending controls be overwritten/updated when stopped? > > + * And should it be cleared in `Camera::release()`? > > To keep things simple I would keep start() and applyControls() separate. > Although it seems to be tempting to have applyControls as single entry > point for simple usecases. Oh my :-) > > > + */ > > + > > + int ret = d->isAccessAllowed(Private::CameraRunning); > > + if (ret < 0) > > + return ret; > > + > > + if (controls.empty()) > > + return 0; > > + > > + patchControlList(controls); > > + > > + /* > > + * \todo Or `ConnectionTypeBlocking` to get the return value? > > + */ > > + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); > > + > > + return 0; > > +} > > + > > /** > > * \brief Start capture from camera > > * \param[in] controls Controls to be applied before starting the Camera > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index d0d3fbc79..99bb2193f 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) > > ASSERT(data->queuedRequests_.empty()); > > ASSERT(data->waitingRequests_.empty()); > > > > + /* > > + * \todo Or not? This probably needs discussions regarding > > + * the expected state of controls after a stop-start sequence. > > + */ > > + data->pendingControls_.clear(); > > + > > data->requestSequence_ = 0; > > } > > > > @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) > > request->_d()->prepare(300ms); > > } > > > > +/** > > + * \brief Apply controls immediately > > + * \param[in] camera The camera > > + * \param[in] controls The controls to apply > > + * > > + * This function tries to apply \a controls immediately to the device by > > + * calling applyControlsDevice(). If that fails, then a fallback mechanism > > + * is used to ensure that \a controls will be merged into the control list > > + * of the next request submitted to the pipeline handler. > > + * > > + * This function also ensures that requests in Camera::Private::waitingRequests_ > > + * will not override the fast-tracked controls. > > + * > > + * \context This function is called from the CameraManager thread. > > + */ > > +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) > > +{ > > + Camera::Private *data = camera->_d(); > > + int res = applyControlsDevice(camera, controls); > > + > > + /* > > + * Prevent later requests from overriding the fast-tracked controls. > > + * \todo This is unfortunately slow. > > I thought about that for a while. I think we will have enough issues > defining the behavior for inter requests controls in cases with queue > underruns. My take would be to simply document that as undefined/racy > behavior and instruct the user to either use applyControls() or > queueRequest() for the same control, but not both. This keeps the code > and path simple without many downsides. This brings to mind another related discussion we've had over the years as to whether the controls in a request are expected to be applied exactly in time for that request, or not. Personally I've never been a big fan of that because (a) we can't guarantee to achieve it, (b) it relies on having "enough" requests already in the queue so that those controls can be grabbed early enough. All of which means the controls take longer to apply, and you have to check what control list was applied on that request (when it completes) anyway. But as part of our per-frame-control work, we did implement this scheme too. However, it leads me to the thought that the controls in a request _could_ behave in this way, and then the controls in the external control list queue are applied "as soon as possible". We Raspberry Pi folks would only use the external queue, as that has the behaviour we want. Other folks could use the controls in the requests if they prefer. Or is this all over-complicated? I don't think we need to worry too much about merging control lists from the two sources. I can't imagine any sensible use case where an application might put the same control into both places - it feels a totally confused thing to be doing. It would be fine to flag it as an error and choose any arbitrary outcome for it. David > > > + */ > > + for (const auto &[k, v] : controls) { > > + for (Request *r : data->waitingRequests_) > > + r->controls().erase(k); > > + } > > + > > + if (res < 0) { > > + /* > > + * \todo Always fall back or only if EOPNOTSUPP is returned? > > + * Possibly there could be some way for the user to influence > > + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). > > + */ > > + if (res != -EOPNOTSUPP) > > + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; > > + > > + /* > > + * Fall back to adding the controls to the next request that enters the > > + * pipeline handler. See PipelineHandler::doQueueRequest(). > > + */ > > + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); > > + } > > You could also move this code into the default applyControlsDevice() > implementation. Then a pipeline handler can either use the default or > implement it's own. No need to handle a mixture of both. > > > + > > + return 0; > > +} > > + > > /** > > * \brief Queue one requests to the device > > */ > > @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) > > return; > > } > > > > + if (!data->pendingControls_.empty()) { > > + /* > > + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is > > + * needed to ensure that if `request` is newer than pendingControls_, > > + * then its controls take precedence. > > + * > > + * For older requests, `PipelineHandler::applyControls()` has already removed > > + * the controls that should be overridden. > > + * > > + * \todo How to handle (if at all) conflicting controls? > > As noted above I don't think we should bother too much :-) > > > + * \todo How to handle failure of `queueRequestDevice()`? > > + * After the merge it becomes impossible to retrieve the pending controls > > + * from `request->controls()`. Making a copy in such a hot-path is not ideal. > > That is correct. But I have a hard time to come up with a use case where > this is problematic. And controls is reset internally by > request->reuse() so I think this is acceptable. > > > + */ > > + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); > > + data->pendingControls_.clear(); > > + } > > + > > int ret = queueRequestDevice(camera, request); > > - if (ret) > > + if (ret) { > > + /* > > + * \todo What to do now? > > + * > > + * The original `pendingControls_` is not easily recoverable. > > + * > > + * Initially > > + * request->controls() = A u B > > + * pendingControls_ = A' u C > > + * > > + * then after the above merge > > + * request->controls() = A u B u C > > + * pendingControls_ = A' > > Is that still the case? As you cleared pendingControls_ before. > > > + * > > + * so recovering `pendingControls_` without a copy of at least > > + * `B` or `C` does not appear easy. > > + * > > + * But sometimes recovering is not even desirable: assume that the controls > > + * in `pendingControls_` causes the failure. In that case keeping them around > > + * will cause every single subsequent request to fail. > > Do we expect cameras to regularly fail on queueRequest()? I think it is > ok to have "lost" the pending controls in this case. > > Best regards, > Stefan > > > + */ > > + > > cancelRequest(request); > > + } > > } > > > > /** > > @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) > > * \return 0 on success or a negative error code otherwise > > */ > > > > +/** > > + * \fn PipelineHandler::applyControlsDevice() > > + * \brief Apply controls immediately > > + * \param[in] camera The camera > > + * \param[in] controls The controls to apply > > + * > > + * This function applies \a controls to \a camera immediately. > > + * > > + * \context This function is called from the CameraManager thread. > > + * > > + * \return 0 on success or a negative error code otherwise > > + * \return -EOPNOTSUPP if fast-tracking controls is not supported > > + */ > > + > > /** > > * \brief Complete a buffer for a request > > * \param[in] request The request the buffer belongs to > > -- > > 2.51.0 > >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index b24a29740..94f940496 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -147,6 +147,7 @@ public: std::unique_ptr<Request> createRequest(uint64_t cookie = 0); int queueRequest(Request *request); + int applyControls(ControlList &&controls); int start(const ControlList *controls = nullptr); int stop(); diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h index 0f50c14a5..5a77298b3 100644 --- a/include/libcamera/internal/camera.h +++ b/include/libcamera/internal/camera.h @@ -38,6 +38,7 @@ public: std::list<Request *> queuedRequests_; std::deque<Request *> waitingRequests_; + ControlList pendingControls_; ControlInfoMap controlInfo_; ControlList properties_; diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index e89d6a33e..16ff54bab 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -57,6 +57,7 @@ public: void registerRequest(Request *request); void queueRequest(Request *request); + int applyControls(Camera *camera, ControlList &&controls); bool completeBuffer(Request *request, FrameBuffer *buffer); void completeRequest(Request *request); @@ -75,6 +76,12 @@ protected: void hotplugMediaDevice(MediaDevice *media); virtual int queueRequestDevice(Camera *camera, Request *request) = 0; + + virtual int applyControlsDevice([[maybe_unused]] Camera *camera, [[maybe_unused]] const ControlList &controls) + { + return -EOPNOTSUPP; + } + virtual void stopDevice(Camera *camera) = 0; virtual bool acquireDevice(Camera *camera); diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2e1e146a2..778445e36 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -637,6 +637,14 @@ Camera::Private::~Private() * queued requests was reached. */ +/** + * \var Camera::Private::pendingControls_ + * \brief The list of pending controls + * + * This list tracks the controls that need to be applied to the device with the + * next request going to PipelineHandler::queueRequestDevice(). + */ + /** * \var Camera::Private::controlInfo_ * \brief The set of controls supported by the camera @@ -1374,6 +1382,51 @@ int Camera::queueRequest(Request *request) return 0; } +/** + * \brief Apply controls immediately + * \param[in] controls The list of controls to apply + * + * This function tries to ensure that the controls in \a controls are applied + * to the camera as soon as possible. + * + * The exact guarantees are camera dependent, but it is guaranteed that the + * controls will be applied no later than if they were part of the next \ref Request "request" + * that the application \ref Camera::queueRequest() "queues". + * + * \context This function is \threadsafe. It may only be called when the camera + * is in the Running state as defined in \ref camera_operation. + * + * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not running + */ +int Camera::applyControls(ControlList &&controls) +{ + Private *const d = _d(); + + /* + * \todo What to do if not running? Should it be stored and merged into the "start" `ControlList`? + * If yes, should that list of pending controls be overwritten/updated when stopped? + * And should it be cleared in `Camera::release()`? + */ + + int ret = d->isAccessAllowed(Private::CameraRunning); + if (ret < 0) + return ret; + + if (controls.empty()) + return 0; + + patchControlList(controls); + + /* + * \todo Or `ConnectionTypeBlocking` to get the return value? + */ + d->pipe_->invokeMethod(&PipelineHandler::applyControls, ConnectionTypeQueued, this, std::move(controls)); + + return 0; +} + /** * \brief Start capture from camera * \param[in] controls Controls to be applied before starting the Camera diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index d0d3fbc79..99bb2193f 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -387,6 +387,12 @@ void PipelineHandler::stop(Camera *camera) ASSERT(data->queuedRequests_.empty()); ASSERT(data->waitingRequests_.empty()); + /* + * \todo Or not? This probably needs discussions regarding + * the expected state of controls after a stop-start sequence. + */ + data->pendingControls_.clear(); + data->requestSequence_ = 0; } @@ -466,6 +472,54 @@ void PipelineHandler::queueRequest(Request *request) request->_d()->prepare(300ms); } +/** + * \brief Apply controls immediately + * \param[in] camera The camera + * \param[in] controls The controls to apply + * + * This function tries to apply \a controls immediately to the device by + * calling applyControlsDevice(). If that fails, then a fallback mechanism + * is used to ensure that \a controls will be merged into the control list + * of the next request submitted to the pipeline handler. + * + * This function also ensures that requests in Camera::Private::waitingRequests_ + * will not override the fast-tracked controls. + * + * \context This function is called from the CameraManager thread. + */ +int PipelineHandler::applyControls(Camera *camera, ControlList &&controls) +{ + Camera::Private *data = camera->_d(); + int res = applyControlsDevice(camera, controls); + + /* + * Prevent later requests from overriding the fast-tracked controls. + * \todo This is unfortunately slow. + */ + for (const auto &[k, v] : controls) { + for (Request *r : data->waitingRequests_) + r->controls().erase(k); + } + + if (res < 0) { + /* + * \todo Always fall back or only if EOPNOTSUPP is returned? + * Possibly there could be some way for the user to influence + * the fallback logic (e.g. `flags` argument for `Camera::applyControls()`). + */ + if (res != -EOPNOTSUPP) + LOG(Pipeline, Debug) << "Fast tracking controls failed: " << res; + + /* + * Fall back to adding the controls to the next request that enters the + * pipeline handler. See PipelineHandler::doQueueRequest(). + */ + data->pendingControls_.merge(std::move(controls), ControlList::MergePolicy::OverwriteExisting); + } + + return 0; +} + /** * \brief Queue one requests to the device */ @@ -484,9 +538,49 @@ void PipelineHandler::doQueueRequest(Request *request) return; } + if (!data->pendingControls_.empty()) { + /* + * Note that `ControlList::MergePolicy::KeepExisting` is used. This is + * needed to ensure that if `request` is newer than pendingControls_, + * then its controls take precedence. + * + * For older requests, `PipelineHandler::applyControls()` has already removed + * the controls that should be overridden. + * + * \todo How to handle (if at all) conflicting controls? + * \todo How to handle failure of `queueRequestDevice()`? + * After the merge it becomes impossible to retrieve the pending controls + * from `request->controls()`. Making a copy in such a hot-path is not ideal. + */ + request->controls().merge(std::move(data->pendingControls_), ControlList::MergePolicy::KeepExisting); + data->pendingControls_.clear(); + } + int ret = queueRequestDevice(camera, request); - if (ret) + if (ret) { + /* + * \todo What to do now? + * + * The original `pendingControls_` is not easily recoverable. + * + * Initially + * request->controls() = A u B + * pendingControls_ = A' u C + * + * then after the above merge + * request->controls() = A u B u C + * pendingControls_ = A' + * + * so recovering `pendingControls_` without a copy of at least + * `B` or `C` does not appear easy. + * + * But sometimes recovering is not even desirable: assume that the controls + * in `pendingControls_` causes the failure. In that case keeping them around + * will cause every single subsequent request to fail. + */ + cancelRequest(request); + } } /** @@ -532,6 +626,20 @@ void PipelineHandler::doQueueRequests(Camera *camera) * \return 0 on success or a negative error code otherwise */ +/** + * \fn PipelineHandler::applyControlsDevice() + * \brief Apply controls immediately + * \param[in] camera The camera + * \param[in] controls The controls to apply + * + * This function applies \a controls to \a camera immediately. + * + * \context This function is called from the CameraManager thread. + * + * \return 0 on success or a negative error code otherwise + * \return -EOPNOTSUPP if fast-tracking controls is not supported + */ + /** * \brief Complete a buffer for a request * \param[in] request The request the buffer belongs to
Add `Camera::applyControls()` whose purpose is to apply controls as soon as possible, without going through `Request::controls()`. A new virtual function `PipelineHandler::applyControlsDevice()` is provided for pipeline handler to implement fast-tracked application of controls. If the pipeline handler does not implement that functionality, or it fails, then a fallback mechanism is used. The controls will be saved for later, and they will be merged into the control list of the next request sent to the pipeline handler (`Camera::Private::waitingRequests_`). Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/camera.h | 1 + include/libcamera/internal/camera.h | 1 + include/libcamera/internal/pipeline_handler.h | 7 ++ src/libcamera/camera.cpp | 53 +++++++++ src/libcamera/pipeline_handler.cpp | 110 +++++++++++++++++- 5 files changed, 171 insertions(+), 1 deletion(-)