| Message ID | 20251124-cam-control-override-v1-1-dfc3a2f3feee@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2025-11-24 16:43:44) > The ControlList associated with a Request are created with > the libcamera::controls::controls id map. > > The cam application, when parsing controls from a script > assigns to the Request's control list a newly created one, which is > initialized without a valid idmap or info map. > > This causes IPA that run in isolated mode to fail, as the IPC > serialization/deserialization code requires all ControlList to have > a valid idmap or info map associated. > > Instead of overwriting the Request::controls() list, merge it with > the one created by cam when parsing the capture script. > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/apps/cam/camera_session.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > --- a/src/apps/cam/camera_session.cpp > +++ b/src/apps/cam/camera_session.cpp > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > return 0; > > if (script_) > - request->controls() = script_->frameControls(queueCount_); > + request->controls().merge(script_->frameControls(queueCount_)); That looks accurate even after we find a way to fix the permissions to prevent the underlying list itself being replaced. Is that possible by making it const somehow? a const pointer, to non-const data ? Anyway, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > queueCount_++; > > > -- > 2.51.1 >
Hi Kieran On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-11-24 16:43:44) > > The ControlList associated with a Request are created with > > the libcamera::controls::controls id map. > > > > The cam application, when parsing controls from a script > > assigns to the Request's control list a newly created one, which is > > initialized without a valid idmap or info map. > > > > This causes IPA that run in isolated mode to fail, as the IPC > > serialization/deserialization code requires all ControlList to have > > a valid idmap or info map associated. > > > > Instead of overwriting the Request::controls() list, merge it with > > the one created by cam when parsing the capture script. > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/apps/cam/camera_session.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > > --- a/src/apps/cam/camera_session.cpp > > +++ b/src/apps/cam/camera_session.cpp > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > > return 0; > > > > if (script_) > > - request->controls() = script_->frameControls(queueCount_); > > + request->controls().merge(script_->frameControls(queueCount_)); > > That looks accurate even after we find a way to fix the permissions to > prevent the underlying list itself being replaced. > > Is that possible by making it const somehow? a const pointer, to > non-const data ? I'm looking into that Simply making "ControlList &Request::controls()" a "const ControlList &Request::controls() const" doesn't work 1) The Camera class modifies the control list of a request, and we can use the usual PIMPL pattern to expose to the library and interface which is different than the one exposed to applications 2) However a ControlList::merge()/set() can't be called on a const instance of ControlList, so application won't be able to add controls to a Request if we simply return "const ControlList" and we should add methods to the Request class to set/merge controls like "Request::addControls()" I'm not sure I like it though > > Anyway, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > queueCount_++; > > > > > > -- > > 2.51.1 > >
Hi 2025. 11. 24. 17:43 keltezéssel, Jacopo Mondi írta: > The ControlList associated with a Request are created with > the libcamera::controls::controls id map. > > The cam application, when parsing controls from a script > assigns to the Request's control list a newly created one, which is > initialized without a valid idmap or info map. > > This causes IPA that run in isolated mode to fail, as the IPC > serialization/deserialization code requires all ControlList to have > a valid idmap or info map associated. > > Instead of overwriting the Request::controls() list, merge it with > the one created by cam when parsing the capture script. > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 Fixes: 568865a6c14355 ("cam: Use script parser to set controls") > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/apps/cam/camera_session.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > --- a/src/apps/cam/camera_session.cpp > +++ b/src/apps/cam/camera_session.cpp > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > return 0; > > if (script_) > - request->controls() = script_->frameControls(queueCount_); > + request->controls().merge(script_->frameControls(queueCount_)); I feel like this should specify `MergePolicy::OverwriteExisting`. Not that it makes any difference at the moment. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > queueCount_++; > >
Hi 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: > Hi Kieran > > On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: >> Quoting Jacopo Mondi (2025-11-24 16:43:44) >>> The ControlList associated with a Request are created with >>> the libcamera::controls::controls id map. >>> >>> The cam application, when parsing controls from a script >>> assigns to the Request's control list a newly created one, which is >>> initialized without a valid idmap or info map. >>> >>> This causes IPA that run in isolated mode to fail, as the IPC >>> serialization/deserialization code requires all ControlList to have >>> a valid idmap or info map associated. >>> >>> Instead of overwriting the Request::controls() list, merge it with >>> the one created by cam when parsing the capture script. >>> >>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >>> src/apps/cam/camera_session.cpp | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 >>> --- a/src/apps/cam/camera_session.cpp >>> +++ b/src/apps/cam/camera_session.cpp >>> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) >>> return 0; >>> >>> if (script_) >>> - request->controls() = script_->frameControls(queueCount_); >>> + request->controls().merge(script_->frameControls(queueCount_)); >> >> That looks accurate even after we find a way to fix the permissions to >> prevent the underlying list itself being replaced. >> >> Is that possible by making it const somehow? a const pointer, to >> non-const data ? > > I'm looking into that > > Simply making "ControlList &Request::controls()" a "const ControlList > &Request::controls() const" doesn't work > > 1) The Camera class modifies the control list of a request, and we can > use the usual PIMPL pattern to expose to the library and interface > which is different than the one exposed to applications > 2) However a ControlList::merge()/set() can't be called on a const > instance of ControlList, so application won't be able to add controls > to a Request if we simply return "const ControlList" and we should add > methods to the Request class to set/merge controls like > "Request::addControls()" I'm not sure I like it though I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit of an orthogonal question here. Because what is important here is where the assignment operators are deleted. I feel like if we were to delete them on the "main" `ControlList` type, that could cause non-negligible inconvenience. One thing that could be done is to add a wrapper around the reference, this way no assignment can be done, but every other member function can be preserved. It unfortunately has quite a bit of duplication, but it only needs two changes in the current code base (in addition to the `.merge()` change). Or alternatively, maybe we could change the design of idmap/infomap/serialization. diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index b24a297400..595752d4b6 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -166,7 +166,7 @@ private: int exportFrameBuffers(Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers); - void patchControlList(ControlList &controls); + void patchControlList(Request::ControlListWrapper controls); }; } /* namespace libcamera */ diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 0c5939f7b3..1e85dcff1a 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -49,7 +49,65 @@ public: void reuse(ReuseFlag flags = Default); - ControlList &controls() { return *controls_; } +#ifndef __DOXYGEN__ + class ControlListWrapper { + public: + using iterator = ControlList::iterator; + using const_iterator = ControlList::const_iterator; + + ControlListWrapper(ControlList& list) + : list_(list) + { + } + + iterator begin() { return list_.begin(); } + iterator end() { return list_.end(); } + const_iterator begin() const { return list_.begin(); } + const_iterator end() const { return list_.end(); } + + bool empty() const { return list_.empty(); } + std::size_t size() const { return list_.size(); } + + void clear() { list_.clear(); } + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) + { + list_.merge(source, policy); + } + + bool contains(unsigned int id) const { return list_.contains(id); } + + template<typename T> + std::optional<T> get(const Control<T> &ctrl) const + { + return list_.get(ctrl); + } + + template<typename T, typename V> + void set(const Control<T> &ctrl, const V &value) + { + list_.set(ctrl, value); + } + + template<typename T, typename V, size_t Size> + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) + { + list_.set(ctrl, value); + } + + const ControlValue &get(unsigned int id) const { return list_.get(id); } + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } + + const ControlInfoMap *infoMap() const { return list_.infoMap(); } + const ControlIdMap *idMap() const { return list_.idMap(); } + + operator const ControlList&() const { return list_; } + + private: + ControlList &list_; + }; +#endif + + ControlListWrapper controls() { return *controls_; } ControlList &metadata() { return *metadata_; } const BufferMap &buffers() const { return bufferMap_; } int addBuffer(const Stream *stream, FrameBuffer *buffer, diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 80ff248c2a..266064f209 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) return 0; /* Translate the Android request settings to libcamera controls. */ - ControlList &controls = descriptor->request_->controls(); + auto controls = descriptor->request_->controls(); camera_metadata_ro_entry_t entry; if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { const int32_t *data = entry.data.i32; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2e1e146a25..23d8323963 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) * The control list is patched in place, turning the AeEnable control into * the equivalent ExposureTimeMode/AnalogueGainMode controls. */ -void Camera::patchControlList(ControlList &controls) +void Camera::patchControlList(Request::ControlListWrapper controls) { const auto &aeEnable = controls.get(controls::AeEnable); if (aeEnable) { > >> >> Anyway, >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> >>> queueCount_++; >>> >>> >>> -- >>> 2.51.1 >>>
Hi Barnabás On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: > Hi > > 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: > > Hi Kieran > > > > On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: > > > Quoting Jacopo Mondi (2025-11-24 16:43:44) > > > > The ControlList associated with a Request are created with > > > > the libcamera::controls::controls id map. > > > > > > > > The cam application, when parsing controls from a script > > > > assigns to the Request's control list a newly created one, which is > > > > initialized without a valid idmap or info map. > > > > > > > > This causes IPA that run in isolated mode to fail, as the IPC > > > > serialization/deserialization code requires all ControlList to have > > > > a valid idmap or info map associated. > > > > > > > > Instead of overwriting the Request::controls() list, merge it with > > > > the one created by cam when parsing the capture script. > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/apps/cam/camera_session.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > > > > --- a/src/apps/cam/camera_session.cpp > > > > +++ b/src/apps/cam/camera_session.cpp > > > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > > > > return 0; > > > > > > > > if (script_) > > > > - request->controls() = script_->frameControls(queueCount_); > > > > + request->controls().merge(script_->frameControls(queueCount_)); > > > > > > That looks accurate even after we find a way to fix the permissions to > > > prevent the underlying list itself being replaced. > > > > > > Is that possible by making it const somehow? a const pointer, to > > > non-const data ? > > > > I'm looking into that > > > > Simply making "ControlList &Request::controls()" a "const ControlList > > &Request::controls() const" doesn't work > > > > 1) The Camera class modifies the control list of a request, and we can > > use the usual PIMPL pattern to expose to the library and interface > > which is different than the one exposed to applications > > 2) However a ControlList::merge()/set() can't be called on a const > > instance of ControlList, so application won't be able to add controls > > to a Request if we simply return "const ControlList" and we should add > > methods to the Request class to set/merge controls like > > "Request::addControls()" I'm not sure I like it though > > I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit > of an orthogonal question here. Because what is important here is where the Could you explain a bit more what you mean here ? The problem at end, in my view, is that we allow to override the ControlLis which is part of a Request, and the Request class API should be changed to disallow that. > assignment operators are deleted. I feel like if we were to delete them on > the "main" `ControlList` type, that could cause non-negligible inconvenience. I don't this we should disallow assigning ControlList completely, no. > > One thing that could be done is to add a wrapper around the reference, this > way no assignment can be done, but every other member function can be preserved. > It unfortunately has quite a bit of duplication, but it only needs two changes > in the current code base (in addition to the `.merge()` change). > > Or alternatively, maybe we could change the design of idmap/infomap/serialization. > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index b24a297400..595752d4b6 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -166,7 +166,7 @@ private: > int exportFrameBuffers(Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > - void patchControlList(ControlList &controls); > + void patchControlList(Request::ControlListWrapper controls); > }; > } /* namespace libcamera */ > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 0c5939f7b3..1e85dcff1a 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -49,7 +49,65 @@ public: > void reuse(ReuseFlag flags = Default); > - ControlList &controls() { return *controls_; } > +#ifndef __DOXYGEN__ > + class ControlListWrapper { > + public: > + using iterator = ControlList::iterator; > + using const_iterator = ControlList::const_iterator; > + > + ControlListWrapper(ControlList& list) > + : list_(list) > + { > + } > + > + iterator begin() { return list_.begin(); } > + iterator end() { return list_.end(); } > + const_iterator begin() const { return list_.begin(); } > + const_iterator end() const { return list_.end(); } > + > + bool empty() const { return list_.empty(); } > + std::size_t size() const { return list_.size(); } > + > + void clear() { list_.clear(); } > + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) > + { > + list_.merge(source, policy); > + } > + > + bool contains(unsigned int id) const { return list_.contains(id); } > + > + template<typename T> > + std::optional<T> get(const Control<T> &ctrl) const > + { > + return list_.get(ctrl); > + } > + > + template<typename T, typename V> > + void set(const Control<T> &ctrl, const V &value) > + { > + list_.set(ctrl, value); > + } > + > + template<typename T, typename V, size_t Size> > + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) > + { > + list_.set(ctrl, value); > + } > + > + const ControlValue &get(unsigned int id) const { return list_.get(id); } > + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } > + > + const ControlInfoMap *infoMap() const { return list_.infoMap(); } > + const ControlIdMap *idMap() const { return list_.idMap(); } > + > + operator const ControlList&() const { return list_; } > + > + private: > + ControlList &list_; > + }; > +#endif > + > + ControlListWrapper controls() { return *controls_; } > ControlList &metadata() { return *metadata_; } > const BufferMap &buffers() const { return bufferMap_; } > int addBuffer(const Stream *stream, FrameBuffer *buffer, > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 80ff248c2a..266064f209 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > /* Translate the Android request settings to libcamera controls. */ > - ControlList &controls = descriptor->request_->controls(); > + auto controls = descriptor->request_->controls(); > camera_metadata_ro_entry_t entry; > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { > const int32_t *data = entry.data.i32; > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2e1e146a25..23d8323963 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > * The control list is patched in place, turning the AeEnable control into > * the equivalent ExposureTimeMode/AnalogueGainMode controls. > */ > -void Camera::patchControlList(ControlList &controls) > +void Camera::patchControlList(Request::ControlListWrapper controls) > { > const auto &aeEnable = controls.get(controls::AeEnable); > if (aeEnable) { > > > > > > > > > > > Anyway, > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > queueCount_++; > > > > > > > > > > > > -- > > > > 2.51.1 > > > > >
2025. 12. 01. 10:59 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: >> Hi >> >> 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: >>> Hi Kieran >>> >>> On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: >>>> Quoting Jacopo Mondi (2025-11-24 16:43:44) >>>>> The ControlList associated with a Request are created with >>>>> the libcamera::controls::controls id map. >>>>> >>>>> The cam application, when parsing controls from a script >>>>> assigns to the Request's control list a newly created one, which is >>>>> initialized without a valid idmap or info map. >>>>> >>>>> This causes IPA that run in isolated mode to fail, as the IPC >>>>> serialization/deserialization code requires all ControlList to have >>>>> a valid idmap or info map associated. >>>>> >>>>> Instead of overwriting the Request::controls() list, merge it with >>>>> the one created by cam when parsing the capture script. >>>>> >>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>> --- >>>>> src/apps/cam/camera_session.cpp | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>>>> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 >>>>> --- a/src/apps/cam/camera_session.cpp >>>>> +++ b/src/apps/cam/camera_session.cpp >>>>> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) >>>>> return 0; >>>>> >>>>> if (script_) >>>>> - request->controls() = script_->frameControls(queueCount_); >>>>> + request->controls().merge(script_->frameControls(queueCount_)); >>>> >>>> That looks accurate even after we find a way to fix the permissions to >>>> prevent the underlying list itself being replaced. >>>> >>>> Is that possible by making it const somehow? a const pointer, to >>>> non-const data ? >>> >>> I'm looking into that >>> >>> Simply making "ControlList &Request::controls()" a "const ControlList >>> &Request::controls() const" doesn't work >>> >>> 1) The Camera class modifies the control list of a request, and we can >>> use the usual PIMPL pattern to expose to the library and interface >>> which is different than the one exposed to applications >>> 2) However a ControlList::merge()/set() can't be called on a const >>> instance of ControlList, so application won't be able to add controls >>> to a Request if we simply return "const ControlList" and we should add >>> methods to the Request class to set/merge controls like >>> "Request::addControls()" I'm not sure I like it though >> >> I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit >> of an orthogonal question here. Because what is important here is where the > > Could you explain a bit more what you mean here ? If you have a method return `ControlList&`, then that can be assigned to unless the assignment operators have been deleted in the `ControlList` type. So whether `ControlList` is a "direct" implementation (like it is today), or a "pimpl" one does not have any direct effect on whether or not it can be assigned to. But disabling the assignment on the "main" `ControlList` type might not be desirable, hence my proposal below with the wrapper type. I hope I'm making sense. > > The problem at end, in my view, is that we allow to override the > ControlLis which is part of a Request, and the Request class API > should be changed to disallow that. > >> assignment operators are deleted. I feel like if we were to delete them on >> the "main" `ControlList` type, that could cause non-negligible inconvenience. > > I don't this we should disallow assigning ControlList completely, no. > >> >> One thing that could be done is to add a wrapper around the reference, this >> way no assignment can be done, but every other member function can be preserved. >> It unfortunately has quite a bit of duplication, but it only needs two changes >> in the current code base (in addition to the `.merge()` change). >> >> Or alternatively, maybe we could change the design of idmap/infomap/serialization. >> >> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h >> index b24a297400..595752d4b6 100644 >> --- a/include/libcamera/camera.h >> +++ b/include/libcamera/camera.h >> @@ -166,7 +166,7 @@ private: >> int exportFrameBuffers(Stream *stream, >> std::vector<std::unique_ptr<FrameBuffer>> *buffers); >> - void patchControlList(ControlList &controls); >> + void patchControlList(Request::ControlListWrapper controls); >> }; >> } /* namespace libcamera */ >> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >> index 0c5939f7b3..1e85dcff1a 100644 >> --- a/include/libcamera/request.h >> +++ b/include/libcamera/request.h >> @@ -49,7 +49,65 @@ public: >> void reuse(ReuseFlag flags = Default); >> - ControlList &controls() { return *controls_; } >> +#ifndef __DOXYGEN__ >> + class ControlListWrapper { >> + public: >> + using iterator = ControlList::iterator; >> + using const_iterator = ControlList::const_iterator; >> + >> + ControlListWrapper(ControlList& list) >> + : list_(list) >> + { >> + } >> + >> + iterator begin() { return list_.begin(); } >> + iterator end() { return list_.end(); } >> + const_iterator begin() const { return list_.begin(); } >> + const_iterator end() const { return list_.end(); } >> + >> + bool empty() const { return list_.empty(); } >> + std::size_t size() const { return list_.size(); } >> + >> + void clear() { list_.clear(); } >> + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) >> + { >> + list_.merge(source, policy); >> + } >> + >> + bool contains(unsigned int id) const { return list_.contains(id); } >> + >> + template<typename T> >> + std::optional<T> get(const Control<T> &ctrl) const >> + { >> + return list_.get(ctrl); >> + } >> + >> + template<typename T, typename V> >> + void set(const Control<T> &ctrl, const V &value) >> + { >> + list_.set(ctrl, value); >> + } >> + >> + template<typename T, typename V, size_t Size> >> + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) >> + { >> + list_.set(ctrl, value); >> + } >> + >> + const ControlValue &get(unsigned int id) const { return list_.get(id); } >> + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } >> + >> + const ControlInfoMap *infoMap() const { return list_.infoMap(); } >> + const ControlIdMap *idMap() const { return list_.idMap(); } >> + >> + operator const ControlList&() const { return list_; } >> + >> + private: >> + ControlList &list_; >> + }; >> +#endif >> + >> + ControlListWrapper controls() { return *controls_; } >> ControlList &metadata() { return *metadata_; } >> const BufferMap &buffers() const { return bufferMap_; } >> int addBuffer(const Stream *stream, FrameBuffer *buffer, >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 80ff248c2a..266064f209 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >> return 0; >> /* Translate the Android request settings to libcamera controls. */ >> - ControlList &controls = descriptor->request_->controls(); >> + auto controls = descriptor->request_->controls(); >> camera_metadata_ro_entry_t entry; >> if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { >> const int32_t *data = entry.data.i32; >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 2e1e146a25..23d8323963 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) >> * The control list is patched in place, turning the AeEnable control into >> * the equivalent ExposureTimeMode/AnalogueGainMode controls. >> */ >> -void Camera::patchControlList(ControlList &controls) >> +void Camera::patchControlList(Request::ControlListWrapper controls) >> { >> const auto &aeEnable = controls.get(controls::AeEnable); >> if (aeEnable) { >> >> >> >>> >>>> >>>> Anyway, >>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> >>>>> >>>>> queueCount_++; >>>>> >>>>> >>>>> -- >>>>> 2.51.1 >>>>> >>
Hi Barnabás On Mon, Dec 01, 2025 at 11:05:46AM +0100, Barnabás Pőcze wrote: > 2025. 12. 01. 10:59 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: > > > Hi > > > > > > 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: > > > > Hi Kieran > > > > > > > > On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: > > > > > Quoting Jacopo Mondi (2025-11-24 16:43:44) > > > > > > The ControlList associated with a Request are created with > > > > > > the libcamera::controls::controls id map. > > > > > > > > > > > > The cam application, when parsing controls from a script > > > > > > assigns to the Request's control list a newly created one, which is > > > > > > initialized without a valid idmap or info map. > > > > > > > > > > > > This causes IPA that run in isolated mode to fail, as the IPC > > > > > > serialization/deserialization code requires all ControlList to have > > > > > > a valid idmap or info map associated. > > > > > > > > > > > > Instead of overwriting the Request::controls() list, merge it with > > > > > > the one created by cam when parsing the capture script. > > > > > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > --- > > > > > > src/apps/cam/camera_session.cpp | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > > > > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > > > > > > --- a/src/apps/cam/camera_session.cpp > > > > > > +++ b/src/apps/cam/camera_session.cpp > > > > > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > > > > > > return 0; > > > > > > > > > > > > if (script_) > > > > > > - request->controls() = script_->frameControls(queueCount_); > > > > > > + request->controls().merge(script_->frameControls(queueCount_)); > > > > > > > > > > That looks accurate even after we find a way to fix the permissions to > > > > > prevent the underlying list itself being replaced. > > > > > > > > > > Is that possible by making it const somehow? a const pointer, to > > > > > non-const data ? > > > > > > > > I'm looking into that > > > > > > > > Simply making "ControlList &Request::controls()" a "const ControlList > > > > &Request::controls() const" doesn't work > > > > > > > > 1) The Camera class modifies the control list of a request, and we can > > > > use the usual PIMPL pattern to expose to the library and interface > > > > which is different than the one exposed to applications > > > > 2) However a ControlList::merge()/set() can't be called on a const > > > > instance of ControlList, so application won't be able to add controls > > > > to a Request if we simply return "const ControlList" and we should add > > > > methods to the Request class to set/merge controls like > > > > "Request::addControls()" I'm not sure I like it though > > > > > > I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit > > > of an orthogonal question here. Because what is important here is where the > > > > Could you explain a bit more what you mean here ? > > If you have a method return `ControlList&`, then that can be assigned to > unless the assignment operators have been deleted in the `ControlList` type. Or you can return a 'const Control&' and provide an interface in the Request class to set controls "on the request" (which make also sense semantically imho) https://patchwork.libcamera.org/patch/25212/ > So whether `ControlList` is a "direct" implementation (like it is today), > or a "pimpl" one does not have any direct effect on whether or not it can > be assigned to. But disabling the assignment on the "main" `ControlList` > type might not be desirable, hence my proposal below with the wrapper type. > I hope I'm making sense. Maybe I was clear in my idea of pimpl-ing the Request class and not the ControlList class ? > > > > > > The problem at end, in my view, is that we allow to override the > > ControlLis which is part of a Request, and the Request class API > > should be changed to disallow that. > > > > > assignment operators are deleted. I feel like if we were to delete them on > > > the "main" `ControlList` type, that could cause non-negligible inconvenience. > > > > I don't this we should disallow assigning ControlList completely, no. > > > > > > > > One thing that could be done is to add a wrapper around the reference, this > > > way no assignment can be done, but every other member function can be preserved. > > > It unfortunately has quite a bit of duplication, but it only needs two changes > > > in the current code base (in addition to the `.merge()` change). > > > > > > Or alternatively, maybe we could change the design of idmap/infomap/serialization. > > > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > index b24a297400..595752d4b6 100644 > > > --- a/include/libcamera/camera.h > > > +++ b/include/libcamera/camera.h > > > @@ -166,7 +166,7 @@ private: > > > int exportFrameBuffers(Stream *stream, > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > > - void patchControlList(ControlList &controls); > > > + void patchControlList(Request::ControlListWrapper controls); > > > }; > > > } /* namespace libcamera */ > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index 0c5939f7b3..1e85dcff1a 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -49,7 +49,65 @@ public: > > > void reuse(ReuseFlag flags = Default); > > > - ControlList &controls() { return *controls_; } > > > +#ifndef __DOXYGEN__ > > > + class ControlListWrapper { > > > + public: > > > + using iterator = ControlList::iterator; > > > + using const_iterator = ControlList::const_iterator; > > > + > > > + ControlListWrapper(ControlList& list) > > > + : list_(list) > > > + { > > > + } > > > + > > > + iterator begin() { return list_.begin(); } > > > + iterator end() { return list_.end(); } > > > + const_iterator begin() const { return list_.begin(); } > > > + const_iterator end() const { return list_.end(); } > > > + > > > + bool empty() const { return list_.empty(); } > > > + std::size_t size() const { return list_.size(); } > > > + > > > + void clear() { list_.clear(); } > > > + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) > > > + { > > > + list_.merge(source, policy); > > > + } > > > + > > > + bool contains(unsigned int id) const { return list_.contains(id); } > > > + > > > + template<typename T> > > > + std::optional<T> get(const Control<T> &ctrl) const > > > + { > > > + return list_.get(ctrl); > > > + } > > > + > > > + template<typename T, typename V> > > > + void set(const Control<T> &ctrl, const V &value) > > > + { > > > + list_.set(ctrl, value); > > > + } > > > + > > > + template<typename T, typename V, size_t Size> > > > + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) > > > + { > > > + list_.set(ctrl, value); > > > + } > > > + > > > + const ControlValue &get(unsigned int id) const { return list_.get(id); } > > > + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } > > > + > > > + const ControlInfoMap *infoMap() const { return list_.infoMap(); } > > > + const ControlIdMap *idMap() const { return list_.idMap(); } > > > + > > > + operator const ControlList&() const { return list_; } > > > + > > > + private: > > > + ControlList &list_; > > > + }; > > > +#endif > > > + > > > + ControlListWrapper controls() { return *controls_; } > > > ControlList &metadata() { return *metadata_; } > > > const BufferMap &buffers() const { return bufferMap_; } > > > int addBuffer(const Stream *stream, FrameBuffer *buffer, > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 80ff248c2a..266064f209 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > > return 0; > > > /* Translate the Android request settings to libcamera controls. */ > > > - ControlList &controls = descriptor->request_->controls(); > > > + auto controls = descriptor->request_->controls(); > > > camera_metadata_ro_entry_t entry; > > > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { > > > const int32_t *data = entry.data.i32; > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 2e1e146a25..23d8323963 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > > * The control list is patched in place, turning the AeEnable control into > > > * the equivalent ExposureTimeMode/AnalogueGainMode controls. > > > */ > > > -void Camera::patchControlList(ControlList &controls) > > > +void Camera::patchControlList(Request::ControlListWrapper controls) > > > { > > > const auto &aeEnable = controls.get(controls::AeEnable); > > > if (aeEnable) { > > > > > > > > > > > > > > > > > > > > > > > Anyway, > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > > > > > queueCount_++; > > > > > > > > > > > > > > > > > > -- > > > > > > 2.51.1 > > > > > > > > > >
2025. 12. 01. 11:10 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Dec 01, 2025 at 11:05:46AM +0100, Barnabás Pőcze wrote: >> 2025. 12. 01. 10:59 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: >>>> Hi >>>> >>>> 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: >>>>> Hi Kieran >>>>> >>>>> On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: >>>>>> Quoting Jacopo Mondi (2025-11-24 16:43:44) >>>>>>> The ControlList associated with a Request are created with >>>>>>> the libcamera::controls::controls id map. >>>>>>> >>>>>>> The cam application, when parsing controls from a script >>>>>>> assigns to the Request's control list a newly created one, which is >>>>>>> initialized without a valid idmap or info map. >>>>>>> >>>>>>> This causes IPA that run in isolated mode to fail, as the IPC >>>>>>> serialization/deserialization code requires all ControlList to have >>>>>>> a valid idmap or info map associated. >>>>>>> >>>>>>> Instead of overwriting the Request::controls() list, merge it with >>>>>>> the one created by cam when parsing the capture script. >>>>>>> >>>>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 >>>>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>>>> --- >>>>>>> src/apps/cam/camera_session.cpp | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>>>>>> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 >>>>>>> --- a/src/apps/cam/camera_session.cpp >>>>>>> +++ b/src/apps/cam/camera_session.cpp >>>>>>> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) >>>>>>> return 0; >>>>>>> >>>>>>> if (script_) >>>>>>> - request->controls() = script_->frameControls(queueCount_); >>>>>>> + request->controls().merge(script_->frameControls(queueCount_)); >>>>>> >>>>>> That looks accurate even after we find a way to fix the permissions to >>>>>> prevent the underlying list itself being replaced. >>>>>> >>>>>> Is that possible by making it const somehow? a const pointer, to >>>>>> non-const data ? >>>>> >>>>> I'm looking into that >>>>> >>>>> Simply making "ControlList &Request::controls()" a "const ControlList >>>>> &Request::controls() const" doesn't work >>>>> >>>>> 1) The Camera class modifies the control list of a request, and we can >>>>> use the usual PIMPL pattern to expose to the library and interface >>>>> which is different than the one exposed to applications >>>>> 2) However a ControlList::merge()/set() can't be called on a const >>>>> instance of ControlList, so application won't be able to add controls >>>>> to a Request if we simply return "const ControlList" and we should add >>>>> methods to the Request class to set/merge controls like >>>>> "Request::addControls()" I'm not sure I like it though >>>> >>>> I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit >>>> of an orthogonal question here. Because what is important here is where the >>> >>> Could you explain a bit more what you mean here ? >> >> If you have a method return `ControlList&`, then that can be assigned to >> unless the assignment operators have been deleted in the `ControlList` type. > > Or you can return a 'const Control&' and provide an interface in the > Request class to set controls "on the request" (which make also sense > semantically imho) > > https://patchwork.libcamera.org/patch/25212/ That's right, but it would be ideal if the interface could be kept the same. > >> So whether `ControlList` is a "direct" implementation (like it is today), >> or a "pimpl" one does not have any direct effect on whether or not it can >> be assigned to. But disabling the assignment on the "main" `ControlList` >> type might not be desirable, hence my proposal below with the wrapper type. >> I hope I'm making sense. > > Maybe I was clear in my idea of pimpl-ing the Request class and not > the ControlList class ? Ahh, I must have misunderstood. >> >> >>> >>> The problem at end, in my view, is that we allow to override the >>> ControlLis which is part of a Request, and the Request class API >>> should be changed to disallow that. >>> >>>> assignment operators are deleted. I feel like if we were to delete them on >>>> the "main" `ControlList` type, that could cause non-negligible inconvenience. >>> >>> I don't this we should disallow assigning ControlList completely, no. >>> >>>> >>>> One thing that could be done is to add a wrapper around the reference, this >>>> way no assignment can be done, but every other member function can be preserved. >>>> It unfortunately has quite a bit of duplication, but it only needs two changes >>>> in the current code base (in addition to the `.merge()` change). >>>> >>>> Or alternatively, maybe we could change the design of idmap/infomap/serialization. >>>> >>>> >>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h >>>> index b24a297400..595752d4b6 100644 >>>> --- a/include/libcamera/camera.h >>>> +++ b/include/libcamera/camera.h >>>> @@ -166,7 +166,7 @@ private: >>>> int exportFrameBuffers(Stream *stream, >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers); >>>> - void patchControlList(ControlList &controls); >>>> + void patchControlList(Request::ControlListWrapper controls); >>>> }; >>>> } /* namespace libcamera */ >>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>>> index 0c5939f7b3..1e85dcff1a 100644 >>>> --- a/include/libcamera/request.h >>>> +++ b/include/libcamera/request.h >>>> @@ -49,7 +49,65 @@ public: >>>> void reuse(ReuseFlag flags = Default); >>>> - ControlList &controls() { return *controls_; } >>>> +#ifndef __DOXYGEN__ >>>> + class ControlListWrapper { >>>> + public: >>>> + using iterator = ControlList::iterator; >>>> + using const_iterator = ControlList::const_iterator; >>>> + >>>> + ControlListWrapper(ControlList& list) >>>> + : list_(list) >>>> + { >>>> + } >>>> + >>>> + iterator begin() { return list_.begin(); } >>>> + iterator end() { return list_.end(); } >>>> + const_iterator begin() const { return list_.begin(); } >>>> + const_iterator end() const { return list_.end(); } >>>> + >>>> + bool empty() const { return list_.empty(); } >>>> + std::size_t size() const { return list_.size(); } >>>> + >>>> + void clear() { list_.clear(); } >>>> + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) >>>> + { >>>> + list_.merge(source, policy); >>>> + } >>>> + >>>> + bool contains(unsigned int id) const { return list_.contains(id); } >>>> + >>>> + template<typename T> >>>> + std::optional<T> get(const Control<T> &ctrl) const >>>> + { >>>> + return list_.get(ctrl); >>>> + } >>>> + >>>> + template<typename T, typename V> >>>> + void set(const Control<T> &ctrl, const V &value) >>>> + { >>>> + list_.set(ctrl, value); >>>> + } >>>> + >>>> + template<typename T, typename V, size_t Size> >>>> + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) >>>> + { >>>> + list_.set(ctrl, value); >>>> + } >>>> + >>>> + const ControlValue &get(unsigned int id) const { return list_.get(id); } >>>> + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } >>>> + >>>> + const ControlInfoMap *infoMap() const { return list_.infoMap(); } >>>> + const ControlIdMap *idMap() const { return list_.idMap(); } >>>> + >>>> + operator const ControlList&() const { return list_; } >>>> + >>>> + private: >>>> + ControlList &list_; >>>> + }; >>>> +#endif >>>> + >>>> + ControlListWrapper controls() { return *controls_; } >>>> ControlList &metadata() { return *metadata_; } >>>> const BufferMap &buffers() const { return bufferMap_; } >>>> int addBuffer(const Stream *stream, FrameBuffer *buffer, >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 80ff248c2a..266064f209 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >>>> return 0; >>>> /* Translate the Android request settings to libcamera controls. */ >>>> - ControlList &controls = descriptor->request_->controls(); >>>> + auto controls = descriptor->request_->controls(); >>>> camera_metadata_ro_entry_t entry; >>>> if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { >>>> const int32_t *data = entry.data.i32; >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>>> index 2e1e146a25..23d8323963 100644 >>>> --- a/src/libcamera/camera.cpp >>>> +++ b/src/libcamera/camera.cpp >>>> @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) >>>> * The control list is patched in place, turning the AeEnable control into >>>> * the equivalent ExposureTimeMode/AnalogueGainMode controls. >>>> */ >>>> -void Camera::patchControlList(ControlList &controls) >>>> +void Camera::patchControlList(Request::ControlListWrapper controls) >>>> { >>>> const auto &aeEnable = controls.get(controls::AeEnable); >>>> if (aeEnable) { >>>> >>>> >>>> >>>>> >>>>>> >>>>>> Anyway, >>>>>> >>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>> >>>>>>> >>>>>>> queueCount_++; >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> 2.51.1 >>>>>>> >>>> >>
Hi Barnabás On Mon, Dec 01, 2025 at 11:19:48AM +0100, Barnabás Pőcze wrote: > 2025. 12. 01. 11:10 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Dec 01, 2025 at 11:05:46AM +0100, Barnabás Pőcze wrote: > > > 2025. 12. 01. 10:59 keltezéssel, Jacopo Mondi írta: > > > > Hi Barnabás > > > > > > > > On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: > > > > > Hi > > > > > > > > > > 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: > > > > > > Hi Kieran > > > > > > > > > > > > On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: > > > > > > > Quoting Jacopo Mondi (2025-11-24 16:43:44) > > > > > > > > The ControlList associated with a Request are created with > > > > > > > > the libcamera::controls::controls id map. > > > > > > > > > > > > > > > > The cam application, when parsing controls from a script > > > > > > > > assigns to the Request's control list a newly created one, which is > > > > > > > > initialized without a valid idmap or info map. > > > > > > > > > > > > > > > > This causes IPA that run in isolated mode to fail, as the IPC > > > > > > > > serialization/deserialization code requires all ControlList to have > > > > > > > > a valid idmap or info map associated. > > > > > > > > > > > > > > > > Instead of overwriting the Request::controls() list, merge it with > > > > > > > > the one created by cam when parsing the capture script. > > > > > > > > > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > --- > > > > > > > > src/apps/cam/camera_session.cpp | 2 +- > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > > > > > > > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 > > > > > > > > --- a/src/apps/cam/camera_session.cpp > > > > > > > > +++ b/src/apps/cam/camera_session.cpp > > > > > > > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) > > > > > > > > return 0; > > > > > > > > > > > > > > > > if (script_) > > > > > > > > - request->controls() = script_->frameControls(queueCount_); > > > > > > > > + request->controls().merge(script_->frameControls(queueCount_)); > > > > > > > > > > > > > > That looks accurate even after we find a way to fix the permissions to > > > > > > > prevent the underlying list itself being replaced. > > > > > > > > > > > > > > Is that possible by making it const somehow? a const pointer, to > > > > > > > non-const data ? > > > > > > > > > > > > I'm looking into that > > > > > > > > > > > > Simply making "ControlList &Request::controls()" a "const ControlList > > > > > > &Request::controls() const" doesn't work > > > > > > > > > > > > 1) The Camera class modifies the control list of a request, and we can > > > > > > use the usual PIMPL pattern to expose to the library and interface > > > > > > which is different than the one exposed to applications > > > > > > 2) However a ControlList::merge()/set() can't be called on a const > > > > > > instance of ControlList, so application won't be able to add controls > > > > > > to a Request if we simply return "const ControlList" and we should add > > > > > > methods to the Request class to set/merge controls like > > > > > > "Request::addControls()" I'm not sure I like it though > > > > > > > > > > I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit > > > > > of an orthogonal question here. Because what is important here is where the > > > > > > > > Could you explain a bit more what you mean here ? > > > > > > If you have a method return `ControlList&`, then that can be assigned to > > > unless the assignment operators have been deleted in the `ControlList` type. > > > > Or you can return a 'const Control&' and provide an interface in the > > Request class to set controls "on the request" (which make also sense > > semantically imho) > > > > https://patchwork.libcamera.org/patch/25212/ > > That's right, but it would be ideal if the interface could be kept the same. Which "interface" ? To be honest, the thing I don't like about my above patch is that we're replicating the "set" ControlList interface on the Request. It's not a huge deal in my opinion, but it feels like we're now bound to keep the two in sync. > > > > > > > So whether `ControlList` is a "direct" implementation (like it is today), > > > or a "pimpl" one does not have any direct effect on whether or not it can > > > be assigned to. But disabling the assignment on the "main" `ControlList` > > > type might not be desirable, hence my proposal below with the wrapper type. > > > I hope I'm making sense. > > > > Maybe I was clear in my idea of pimpl-ing the Request class and not > > the ControlList class ? > > Ahh, I must have misunderstood. > > > > > > > > > > > > > > > > The problem at end, in my view, is that we allow to override the > > > > ControlLis which is part of a Request, and the Request class API > > > > should be changed to disallow that. > > > > > > > > > assignment operators are deleted. I feel like if we were to delete them on > > > > > the "main" `ControlList` type, that could cause non-negligible inconvenience. > > > > > > > > I don't this we should disallow assigning ControlList completely, no. > > > > > > > > > > > > > > One thing that could be done is to add a wrapper around the reference, this > > > > > way no assignment can be done, but every other member function can be preserved. > > > > > It unfortunately has quite a bit of duplication, but it only needs two changes > > > > > in the current code base (in addition to the `.merge()` change). > > > > > > > > > > Or alternatively, maybe we could change the design of idmap/infomap/serialization. > > > > > > > > > > > > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > > > > index b24a297400..595752d4b6 100644 > > > > > --- a/include/libcamera/camera.h > > > > > +++ b/include/libcamera/camera.h > > > > > @@ -166,7 +166,7 @@ private: > > > > > int exportFrameBuffers(Stream *stream, > > > > > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > > > > - void patchControlList(ControlList &controls); > > > > > + void patchControlList(Request::ControlListWrapper controls); > > > > > }; > > > > > } /* namespace libcamera */ > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > > index 0c5939f7b3..1e85dcff1a 100644 > > > > > --- a/include/libcamera/request.h > > > > > +++ b/include/libcamera/request.h > > > > > @@ -49,7 +49,65 @@ public: > > > > > void reuse(ReuseFlag flags = Default); > > > > > - ControlList &controls() { return *controls_; } > > > > > +#ifndef __DOXYGEN__ > > > > > + class ControlListWrapper { > > > > > + public: > > > > > + using iterator = ControlList::iterator; > > > > > + using const_iterator = ControlList::const_iterator; > > > > > + > > > > > + ControlListWrapper(ControlList& list) > > > > > + : list_(list) > > > > > + { > > > > > + } > > > > > + > > > > > + iterator begin() { return list_.begin(); } > > > > > + iterator end() { return list_.end(); } > > > > > + const_iterator begin() const { return list_.begin(); } > > > > > + const_iterator end() const { return list_.end(); } > > > > > + > > > > > + bool empty() const { return list_.empty(); } > > > > > + std::size_t size() const { return list_.size(); } > > > > > + > > > > > + void clear() { list_.clear(); } > > > > > + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) > > > > > + { > > > > > + list_.merge(source, policy); > > > > > + } > > > > > + > > > > > + bool contains(unsigned int id) const { return list_.contains(id); } > > > > > + > > > > > + template<typename T> > > > > > + std::optional<T> get(const Control<T> &ctrl) const > > > > > + { > > > > > + return list_.get(ctrl); > > > > > + } > > > > > + > > > > > + template<typename T, typename V> > > > > > + void set(const Control<T> &ctrl, const V &value) > > > > > + { > > > > > + list_.set(ctrl, value); > > > > > + } > > > > > + > > > > > + template<typename T, typename V, size_t Size> > > > > > + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) > > > > > + { > > > > > + list_.set(ctrl, value); > > > > > + } > > > > > + > > > > > + const ControlValue &get(unsigned int id) const { return list_.get(id); } > > > > > + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } > > > > > + > > > > > + const ControlInfoMap *infoMap() const { return list_.infoMap(); } > > > > > + const ControlIdMap *idMap() const { return list_.idMap(); } > > > > > + > > > > > + operator const ControlList&() const { return list_; } > > > > > + > > > > > + private: > > > > > + ControlList &list_; > > > > > + }; > > > > > +#endif > > > > > + > > > > > + ControlListWrapper controls() { return *controls_; } > > > > > ControlList &metadata() { return *metadata_; } > > > > > const BufferMap &buffers() const { return bufferMap_; } > > > > > int addBuffer(const Stream *stream, FrameBuffer *buffer, > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index 80ff248c2a..266064f209 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > > > > return 0; > > > > > /* Translate the Android request settings to libcamera controls. */ > > > > > - ControlList &controls = descriptor->request_->controls(); > > > > > + auto controls = descriptor->request_->controls(); > > > > > camera_metadata_ro_entry_t entry; > > > > > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { > > > > > const int32_t *data = entry.data.i32; > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > > index 2e1e146a25..23d8323963 100644 > > > > > --- a/src/libcamera/camera.cpp > > > > > +++ b/src/libcamera/camera.cpp > > > > > @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > > > > * The control list is patched in place, turning the AeEnable control into > > > > > * the equivalent ExposureTimeMode/AnalogueGainMode controls. > > > > > */ > > > > > -void Camera::patchControlList(ControlList &controls) > > > > > +void Camera::patchControlList(Request::ControlListWrapper controls) > > > > > { > > > > > const auto &aeEnable = controls.get(controls::AeEnable); > > > > > if (aeEnable) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Anyway, > > > > > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > > > > > > > > > > > queueCount_++; > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > 2.51.1 > > > > > > > > > > > > > > > > >
2025. 12. 01. 11:43 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Dec 01, 2025 at 11:19:48AM +0100, Barnabás Pőcze wrote: >> 2025. 12. 01. 11:10 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Mon, Dec 01, 2025 at 11:05:46AM +0100, Barnabás Pőcze wrote: >>>> 2025. 12. 01. 10:59 keltezéssel, Jacopo Mondi írta: >>>>> Hi Barnabás >>>>> >>>>> On Mon, Dec 01, 2025 at 10:19:20AM +0100, Barnabás Pőcze wrote: >>>>>> Hi >>>>>> >>>>>> 2025. 11. 25. 8:54 keltezéssel, Jacopo Mondi írta: >>>>>>> Hi Kieran >>>>>>> >>>>>>> On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote: >>>>>>>> Quoting Jacopo Mondi (2025-11-24 16:43:44) >>>>>>>>> The ControlList associated with a Request are created with >>>>>>>>> the libcamera::controls::controls id map. >>>>>>>>> >>>>>>>>> The cam application, when parsing controls from a script >>>>>>>>> assigns to the Request's control list a newly created one, which is >>>>>>>>> initialized without a valid idmap or info map. >>>>>>>>> >>>>>>>>> This causes IPA that run in isolated mode to fail, as the IPC >>>>>>>>> serialization/deserialization code requires all ControlList to have >>>>>>>>> a valid idmap or info map associated. >>>>>>>>> >>>>>>>>> Instead of overwriting the Request::controls() list, merge it with >>>>>>>>> the one created by cam when parsing the capture script. >>>>>>>>> >>>>>>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 >>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>>>>>> --- >>>>>>>>> src/apps/cam/camera_session.cpp | 2 +- >>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>>>>>>>> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 >>>>>>>>> --- a/src/apps/cam/camera_session.cpp >>>>>>>>> +++ b/src/apps/cam/camera_session.cpp >>>>>>>>> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) >>>>>>>>> return 0; >>>>>>>>> >>>>>>>>> if (script_) >>>>>>>>> - request->controls() = script_->frameControls(queueCount_); >>>>>>>>> + request->controls().merge(script_->frameControls(queueCount_)); >>>>>>>> >>>>>>>> That looks accurate even after we find a way to fix the permissions to >>>>>>>> prevent the underlying list itself being replaced. >>>>>>>> >>>>>>>> Is that possible by making it const somehow? a const pointer, to >>>>>>>> non-const data ? >>>>>>> >>>>>>> I'm looking into that >>>>>>> >>>>>>> Simply making "ControlList &Request::controls()" a "const ControlList >>>>>>> &Request::controls() const" doesn't work >>>>>>> >>>>>>> 1) The Camera class modifies the control list of a request, and we can >>>>>>> use the usual PIMPL pattern to expose to the library and interface >>>>>>> which is different than the one exposed to applications >>>>>>> 2) However a ControlList::merge()/set() can't be called on a const >>>>>>> instance of ControlList, so application won't be able to add controls >>>>>>> to a Request if we simply return "const ControlList" and we should add >>>>>>> methods to the Request class to set/merge controls like >>>>>>> "Request::addControls()" I'm not sure I like it though >>>>>> >>>>>> I'm not entirely sure that pimpl is sufficient here. I think pimpl is a bit >>>>>> of an orthogonal question here. Because what is important here is where the >>>>> >>>>> Could you explain a bit more what you mean here ? >>>> >>>> If you have a method return `ControlList&`, then that can be assigned to >>>> unless the assignment operators have been deleted in the `ControlList` type. >>> >>> Or you can return a 'const Control&' and provide an interface in the >>> Request class to set controls "on the request" (which make also sense >>> semantically imho) >>> >>> https://patchwork.libcamera.org/patch/25212/ >> >> That's right, but it would be ideal if the interface could be kept the same. > > Which "interface" ? By "interface" I meant that it might be nice to make `request->controls()` behave like a proper `ControlList`, i.e. have (mostly) the same methods, even for setting controls. > > To be honest, the thing I don't like about my above patch is that > we're replicating the "set" ControlList interface on the Request. > > It's not a huge deal in my opinion, but it feels like we're now bound > to keep the two in sync. > >> >> >>> >>>> So whether `ControlList` is a "direct" implementation (like it is today), >>>> or a "pimpl" one does not have any direct effect on whether or not it can >>>> be assigned to. But disabling the assignment on the "main" `ControlList` >>>> type might not be desirable, hence my proposal below with the wrapper type. >>>> I hope I'm making sense. >>> >>> Maybe I was clear in my idea of pimpl-ing the Request class and not >>> the ControlList class ? >> >> Ahh, I must have misunderstood. >> >> >>>> >>>> >>>>> >>>>> The problem at end, in my view, is that we allow to override the >>>>> ControlLis which is part of a Request, and the Request class API >>>>> should be changed to disallow that. >>>>> >>>>>> assignment operators are deleted. I feel like if we were to delete them on >>>>>> the "main" `ControlList` type, that could cause non-negligible inconvenience. >>>>> >>>>> I don't this we should disallow assigning ControlList completely, no. >>>>> >>>>>> >>>>>> One thing that could be done is to add a wrapper around the reference, this >>>>>> way no assignment can be done, but every other member function can be preserved. >>>>>> It unfortunately has quite a bit of duplication, but it only needs two changes >>>>>> in the current code base (in addition to the `.merge()` change). >>>>>> >>>>>> Or alternatively, maybe we could change the design of idmap/infomap/serialization. >>>>>> >>>>>> >>>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h >>>>>> index b24a297400..595752d4b6 100644 >>>>>> --- a/include/libcamera/camera.h >>>>>> +++ b/include/libcamera/camera.h >>>>>> @@ -166,7 +166,7 @@ private: >>>>>> int exportFrameBuffers(Stream *stream, >>>>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers); >>>>>> - void patchControlList(ControlList &controls); >>>>>> + void patchControlList(Request::ControlListWrapper controls); >>>>>> }; >>>>>> } /* namespace libcamera */ >>>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>>>>> index 0c5939f7b3..1e85dcff1a 100644 >>>>>> --- a/include/libcamera/request.h >>>>>> +++ b/include/libcamera/request.h >>>>>> @@ -49,7 +49,65 @@ public: >>>>>> void reuse(ReuseFlag flags = Default); >>>>>> - ControlList &controls() { return *controls_; } >>>>>> +#ifndef __DOXYGEN__ >>>>>> + class ControlListWrapper { >>>>>> + public: >>>>>> + using iterator = ControlList::iterator; >>>>>> + using const_iterator = ControlList::const_iterator; >>>>>> + >>>>>> + ControlListWrapper(ControlList& list) >>>>>> + : list_(list) >>>>>> + { >>>>>> + } >>>>>> + >>>>>> + iterator begin() { return list_.begin(); } >>>>>> + iterator end() { return list_.end(); } >>>>>> + const_iterator begin() const { return list_.begin(); } >>>>>> + const_iterator end() const { return list_.end(); } >>>>>> + >>>>>> + bool empty() const { return list_.empty(); } >>>>>> + std::size_t size() const { return list_.size(); } >>>>>> + >>>>>> + void clear() { list_.clear(); } >>>>>> + void merge(const ControlList &source, ControlList::MergePolicy policy = ControlList::MergePolicy::KeepExisting) >>>>>> + { >>>>>> + list_.merge(source, policy); >>>>>> + } >>>>>> + >>>>>> + bool contains(unsigned int id) const { return list_.contains(id); } >>>>>> + >>>>>> + template<typename T> >>>>>> + std::optional<T> get(const Control<T> &ctrl) const >>>>>> + { >>>>>> + return list_.get(ctrl); >>>>>> + } >>>>>> + >>>>>> + template<typename T, typename V> >>>>>> + void set(const Control<T> &ctrl, const V &value) >>>>>> + { >>>>>> + list_.set(ctrl, value); >>>>>> + } >>>>>> + >>>>>> + template<typename T, typename V, size_t Size> >>>>>> + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value) >>>>>> + { >>>>>> + list_.set(ctrl, value); >>>>>> + } >>>>>> + >>>>>> + const ControlValue &get(unsigned int id) const { return list_.get(id); } >>>>>> + void set(unsigned int id, const ControlValue &value) { return list_.set(id, value); } >>>>>> + >>>>>> + const ControlInfoMap *infoMap() const { return list_.infoMap(); } >>>>>> + const ControlIdMap *idMap() const { return list_.idMap(); } >>>>>> + >>>>>> + operator const ControlList&() const { return list_; } >>>>>> + >>>>>> + private: >>>>>> + ControlList &list_; >>>>>> + }; >>>>>> +#endif >>>>>> + >>>>>> + ControlListWrapper controls() { return *controls_; } >>>>>> ControlList &metadata() { return *metadata_; } >>>>>> const BufferMap &buffers() const { return bufferMap_; } >>>>>> int addBuffer(const Stream *stream, FrameBuffer *buffer, >>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>>>> index 80ff248c2a..266064f209 100644 >>>>>> --- a/src/android/camera_device.cpp >>>>>> +++ b/src/android/camera_device.cpp >>>>>> @@ -804,7 +804,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >>>>>> return 0; >>>>>> /* Translate the Android request settings to libcamera controls. */ >>>>>> - ControlList &controls = descriptor->request_->controls(); >>>>>> + auto controls = descriptor->request_->controls(); >>>>>> camera_metadata_ro_entry_t entry; >>>>>> if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) { >>>>>> const int32_t *data = entry.data.i32; >>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>>>>> index 2e1e146a25..23d8323963 100644 >>>>>> --- a/src/libcamera/camera.cpp >>>>>> +++ b/src/libcamera/camera.cpp >>>>>> @@ -1283,7 +1283,7 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) >>>>>> * The control list is patched in place, turning the AeEnable control into >>>>>> * the equivalent ExposureTimeMode/AnalogueGainMode controls. >>>>>> */ >>>>>> -void Camera::patchControlList(ControlList &controls) >>>>>> +void Camera::patchControlList(Request::ControlListWrapper controls) >>>>>> { >>>>>> const auto &aeEnable = controls.get(controls::AeEnable); >>>>>> if (aeEnable) { >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> Anyway, >>>>>>>> >>>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>>>>> >>>>>>>>> >>>>>>>>> queueCount_++; >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> 2.51.1 >>>>>>>>> >>>>>> >>>> >>
diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644 --- a/src/apps/cam/camera_session.cpp +++ b/src/apps/cam/camera_session.cpp @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request) return 0; if (script_) - request->controls() = script_->frameControls(queueCount_); + request->controls().merge(script_->frameControls(queueCount_)); queueCount_++;
The ControlList associated with a Request are created with the libcamera::controls::controls id map. The cam application, when parsing controls from a script assigns to the Request's control list a newly created one, which is initialized without a valid idmap or info map. This causes IPA that run in isolated mode to fail, as the IPC serialization/deserialization code requires all ControlList to have a valid idmap or info map associated. Instead of overwriting the Request::controls() list, merge it with the one created by cam when parsing the capture script. Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295 Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/apps/cam/camera_session.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)