[{"id":37130,"web_url":"https://patchwork.libcamera.org/comment/37130/","msgid":"<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>","date":"2025-12-01T10:23:32","subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2025-11-26 18:38:53)\n> The ControlList inside a Request is initialized with the Camera\n> ControlInfoMap, which lists the limits of the controls supported\n> by the Camera.\n> \n> The Request class currently exposes the control list it contains through\n> the Request::controls() function as a raw pointer, potentially allowing\n> applications to replace the Request's control list with a different one.\n> \n> The following pattern:\n> \n> request->controls() = {};\n> \n> is then currently valid but opens the door to potential issues, as the\n> new list might be initialized with a different info map or without any\n> info map at all.\n> \n> To avoid applications to override the control list contained in a\n> Request, make the Request::controls() function return a const reference\n> and move the non-const version to the Request::Private class for\n> internal usage.\n> \n> Add to the Request class 4 overloads of the setControl() function that\n> allows applications to set controls on a Request control list without\n> accessing it.\n\nI think good idea.\n\n> \n> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/request.h | 1 +\n> include/libcamera/request.h | 27 +++++++++++++++++-\n> src/android/camera_device.cpp | 8 +++---\n> src/apps/cam/camera_session.cpp | 2 +-\n> src/gstreamer/gstlibcamera-controls.cpp.in | 2 +-\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/request.cpp | 46 +++++++++++++++++++++---------\n> src/py/libcamera/py_main.cpp | 2 +-\n> src/v4l2/v4l2_camera.cpp | 2 +-\n> 9 files changed, 69 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 643f67cabf5778f7515c0117d06d4e0a3fdec4f8..4807036b7ae22c0e90886889ff5741fd9c5c0e9f 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -36,6 +36,7 @@ public:\n> Camera *camera() const { return camera_; }\n> bool hasPendingBuffers() const;\n> \n> + ControlList &controls();\n> ControlList &metadata() { return *metadata_; }\n> \n> bool completeBuffer(FrameBuffer *buffer);\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index c9aeddb62680923dc3490a23dc6c3f70f70cc075..31860e1e1e7680ded00148b7d96ca03f820d1ebc 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -49,7 +49,7 @@ public:\n> \n> void reuse(ReuseFlag flags = Default);\n> \n> - ControlList &controls() { return *controls_; }\n> + const ControlList &controls() const { return *controls_; }\n> const ControlList &metadata() const;\n> const BufferMap &buffers() const { return bufferMap_; }\n> int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> @@ -64,6 +64,31 @@ public:\n> \n> std::string toString() const;\n> \n> + template\n> + void setControl(const Control &ctrl, const V &value)\n> + {\n> + controls_->set(ctrl, value);\n> + }\n> +\n> +#ifndef __DOXYGEN__\n> + template\n> + void setControl(const Control> &ctrl,\n> + const Span &values)\n> + {\n> + controls_->set(ctrl, values);\n> + }\n> +#endif\n> +\n> + void setControls(const ControlList &other)\n\nimo s/setControls/merge/ because the meaning of \"set\" feels too different from\n\"merge\". I also wonder that it might be useful for applications to be able to\nset the merge policy.\n\nUnless you want to actually set controls_ = other, in which case I don't think\nmerge() is the right way? (although since you used the default merge policy of\nKeepExisting maybe that wasn't the intention?)\n\nJudging from how you use it, afaict cam probably wants full overwrite while gst\nand v4l2_camera probably wants an actual merge, so maybe we need to add a\nseparate function for controls_ = other.\n\nOtherwise, looks good.\n\n\nPaul\n\n> + {\n> + controls_->merge(other);\n> + }\n> +\n> + void setControl(unsigned int id, const ControlValue &value)\n> + {\n> + controls_->set(id, value);\n> + }\n> +\n> private:\n> LIBCAMERA_DISABLE_COPY(Request)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 80ff248c2aa316d2f7ccc59d4621a7c86770bf5f..80145f6d103546b4214316f58ecf283accd108ff 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -804,19 +804,19 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> return 0;\n> \n> /* Translate the Android request settings to libcamera controls. */\n> - ControlList &controls = descriptor->request_->controls();\n> + Request *req = descriptor->request_.get();\n> camera_metadata_ro_entry_t entry;\n> if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) {\n> const int32_t *data = entry.data.i32;\n> Rectangle cropRegion{ data[0], data[1],\n> static_cast(data[2]),\n> static_cast(data[3]) };\n> - controls.set(controls::ScalerCrop, cropRegion);\n> + req->setControl(controls::ScalerCrop, cropRegion);\n> }\n> \n> if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> const uint8_t *data = entry.data.u8;\n> - controls.set(controls::draft::FaceDetectMode, data[0]);\n> + req->setControl(controls::draft::FaceDetectMode, data[0]);\n> }\n> \n> if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> @@ -854,7 +854,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> return -EINVAL;\n> }\n> \n> - controls.set(controls::draft::TestPatternMode, testPatternMode);\n> + req->setControl(controls::draft::TestPatternMode, testPatternMode);\n> }\n> \n> return 0;\n> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..246806a0502cec465ac85989f7c93d5885427767 100644\n> --- a/src/apps/cam/camera_session.cpp\n> +++ b/src/apps/cam/camera_session.cpp\n> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)\n> return 0;\n> \n> if (script_)\n> - request->controls() = script_->frameControls(queueCount_);\n> + request->setControls(script_->frameControls(queueCount_));\n> \n> queueCount_++;\n> \n> diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> index 6faf3ee7a6c3401bb42c3720505ea9d5d6f4186a..40a543e755e29f29b797417a58537fc2d05fa9ea 100644\n> --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> @@ -276,7 +276,7 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> \n> void GstCameraControls::applyControls(std::unique_ptr &request)\n> {\n> - request->controls().merge(controls_);\n> + request->setControls(controls_);\n> controls_.clear();\n> }\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..1918c6f4340d790e93ac23baa906f038cd30f0bc 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1366,7 +1366,7 @@ int Camera::queueRequest(Request *request)\n> }\n> \n> /* Pre-process AeEnable. */\n> - patchControlList(request->controls());\n> + patchControlList(request->_d()->controls());\n> \n> d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> ConnectionTypeQueued, request);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index a661b2f5c8ae9ae2bcbab2dcdceeef7dcb8d0930..c8c47f4c75cad116efcdd95032b5def19c900d95 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -87,6 +87,16 @@ bool Request::Private::hasPendingBuffers() const\n> return !pending_.empty();\n> }\n> \n> +/**\n> + * \\fn Request::Private::controls()\n> + * \\brief Retrieve the request's ControlList\n> + * \\return A reference to the ControlList in this request\n> + */\n> +ControlList &Request::Private::controls()\n> +{\n> + return *_o()->controls_;\n> +}\n> +\n> /**\n> * \\fn Request::Private::metadata()\n> * \\brief Retrieve the request's metadata\n> @@ -414,19 +424,9 @@ void Request::reuse(ReuseFlag flags)\n> }\n> \n> /**\n> - * \\fn Request::controls()\n> - * \\brief Retrieve the request's ControlList\n> - *\n> - * Requests store a list of controls to be applied to all frames captured for\n> - * the request. They are created with an empty list of controls that can be\n> - * accessed through this function. Control values can be retrieved using\n> - * ControlList::get() and updated using ControlList::set().\n> - *\n> - * Only controls supported by the camera to which this request will be\n> - * submitted shall be included in the controls list. Attempting to add an\n> - * unsupported control causes undefined behaviour.\n> - *\n> - * \\return A reference to the ControlList in this request\n> + * \\fn Request::controls() const\n> + * \\brief Retrieve a const reference to the request's ControlList\n> + * \\return A const reference to the ControlList in this request\n> */\n> \n> /**\n> @@ -623,4 +623,24 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n> return out;\n> }\n> \n> +/**\n> + * \\fn Request::setControl(const Control &ctrl, const V &value)\n> + * \\brief Set control \\a ctrl in the Request\n> + * \\param[in] ctrl The control\n> + * \\param[in] value The control value\n> + */\n> +\n> +/**\n> + * \\fn Request::setControls(const ControlList &other)\n> + * \\brief Merge the control list \\a other in the Request\n> + * \\param[in] other The control list to add to the Request\n> + */\n> +\n> +/**\n> + * \\fn Request::setControl(unsigned int id, const ControlValue &value)\n> + * \\brief Set control \\a id in the Request to \\a value\n> + * \\param[in] id The control numerical id\n> + * \\param[in] value The control value\n> + */\n> +\n> } /* namespace libcamera */\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index a983ea75c35669cc5d1ff433eddfdd99e9388e7c..97cc6cfbe7fbc4ece5c7a779a9ae6c8c7d23cf7b 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -463,7 +463,7 @@ PYBIND11_MODULE(_libcamera, m)\n> .def_property_readonly(\"sequence\", &Request::sequence)\n> .def_property_readonly(\"has_pending_buffers\", &Request::hasPendingBuffers)\n> .def(\"set_control\", [](Request &self, const ControlId &id, py::object value) {\n> - self.controls().set(id.id(), pyToControlValue(value, id.type()));\n> + self.setControl(id.id(), pyToControlValue(value, id.type()));\n> })\n> .def_property_readonly(\"metadata\", [](Request &self) {\n> /* Convert ControlList to std container */\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 94d138cd5710b9bd5c83cacd3cbd963f71aeb3c7..7e918cc3e803cdad67043f1dbdc212d5b57e37f3 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -269,7 +269,7 @@ int V4L2Camera::qbuf(unsigned int index)\n> return 0;\n> }\n> \n> - request->controls().merge(std::move(controls_));\n> + request->setControls(controls_);\n> \n> ret = camera_->queueRequest(request);\n> if (ret < 0) {\n> \n> -- \n> 2.51.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2A16CC0F1B\n\tfor ;\n\tMon, 1 Dec 2025 10:23:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCCF160B0B;\n\tMon, 1 Dec 2025 11:23:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88567609D8\n\tfor ;\n\tMon, 1 Dec 2025 11:23:39 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:67cc:a360:f133:b401])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 029AA6AC;\n\tMon, 1 Dec 2025 11:21:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rF1G1Vc5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764584486;\n\tbh=w1WcEX+4nZapQBgovOomYUN7oymzPBYBuUNGoNfgh6A=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rF1G1Vc5AD+Zn6s4+bsET377eWSvTT2hAu9/o40/y4LZqvtZVJfuD358hoc8opX4b\n\thAZHAHav8rlA3SPrB+NaD16goaV1y0Xn8nD6iyeRQgXp4Vnq+LYLY66XwJjrHPXt5a\n\tCFoBI9Qr7lP6WqcHi5z3f+fmYtlC1oImkaBRtJIU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>","References":"<20251126-cam-control-override-v2-0-305024a1f190@ideasonboard.com>\n\t<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>","Subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","From":"Paul Elder ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 01 Dec 2025 19:23:32 +0900","Message-ID":"<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37146,"web_url":"https://patchwork.libcamera.org/comment/37146/","msgid":"<20251201150154.GA29230@pendragon.ideasonboard.com>","date":"2025-12-01T15:01:54","subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Dec 01, 2025 at 07:23:32PM +0900, Paul Elder wrote:\n> Quoting Jacopo Mondi (2025-11-26 18:38:53)\n> > The ControlList inside a Request is initialized with the Camera\n> > ControlInfoMap, which lists the limits of the controls supported\n> > by the Camera.\n> > \n> > The Request class currently exposes the control list it contains through\n> > the Request::controls() function as a raw pointer, potentially allowing\n> > applications to replace the Request's control list with a different one.\n> > \n> > The following pattern:\n> > \n> > request->controls() = {};\n> > \n> > is then currently valid but opens the door to potential issues, as the\n> > new list might be initialized with a different info map or without any\n> > info map at all.\n> > \n> > To avoid applications to override the control list contained in a\n> > Request, make the Request::controls() function return a const reference\n> > and move the non-const version to the Request::Private class for\n> > internal usage.\n> > \n> > Add to the Request class 4 overloads of the setControl() function that\n> > allows applications to set controls on a Request control list without\n> > accessing it.\n> \n> I think good idea.\n\nI'm afraid I don't agree :-( The whole point of having a ControlList is\nto use it as a container for controls. Setting a control shouldn't be\ndone on the Request class itself, otherwise we could as well stop\nexposing the ControlList class completely.\n\nIf you want to avoid\n\n\trequest->controls() = {};\n\nyou could stop exposing the ControlList assignment operator. Other\ndesign options are likely possible too.\n\n> > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/request.h | 1 +\n> > include/libcamera/request.h | 27 +++++++++++++++++-\n> > src/android/camera_device.cpp | 8 +++---\n> > src/apps/cam/camera_session.cpp | 2 +-\n> > src/gstreamer/gstlibcamera-controls.cpp.in | 2 +-\n> > src/libcamera/camera.cpp | 2 +-\n> > src/libcamera/request.cpp | 46 +++++++++++++++++++++---------\n> > src/py/libcamera/py_main.cpp | 2 +-\n> > src/v4l2/v4l2_camera.cpp | 2 +-\n> > 9 files changed, 69 insertions(+), 23 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > index 643f67cabf5778f7515c0117d06d4e0a3fdec4f8..4807036b7ae22c0e90886889ff5741fd9c5c0e9f 100644\n> > --- a/include/libcamera/internal/request.h\n> > +++ b/include/libcamera/internal/request.h\n> > @@ -36,6 +36,7 @@ public:\n> > Camera *camera() const { return camera_; }\n> > bool hasPendingBuffers() const;\n> > \n> > + ControlList &controls();\n> > ControlList &metadata() { return *metadata_; }\n> > \n> > bool completeBuffer(FrameBuffer *buffer);\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index c9aeddb62680923dc3490a23dc6c3f70f70cc075..31860e1e1e7680ded00148b7d96ca03f820d1ebc 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -49,7 +49,7 @@ public:\n> > \n> > void reuse(ReuseFlag flags = Default);\n> > \n> > - ControlList &controls() { return *controls_; }\n> > + const ControlList &controls() const { return *controls_; }\n> > const ControlList &metadata() const;\n> > const BufferMap &buffers() const { return bufferMap_; }\n> > int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> > @@ -64,6 +64,31 @@ public:\n> > \n> > std::string toString() const;\n> > \n> > + template\n> > + void setControl(const Control &ctrl, const V &value)\n> > + {\n> > + controls_->set(ctrl, value);\n> > + }\n> > +\n> > +#ifndef __DOXYGEN__\n> > + template\n> > + void setControl(const Control> &ctrl,\n> > + const Span &values)\n> > + {\n> > + controls_->set(ctrl, values);\n> > + }\n> > +#endif\n> > +\n> > + void setControls(const ControlList &other)\n> \n> imo s/setControls/merge/ because the meaning of \"set\" feels too different from\n> \"merge\". I also wonder that it might be useful for applications to be able to\n> set the merge policy.\n> \n> Unless you want to actually set controls_ = other, in which case I don't think\n> merge() is the right way? (although since you used the default merge policy of\n> KeepExisting maybe that wasn't the intention?)\n> \n> Judging from how you use it, afaict cam probably wants full overwrite while gst\n> and v4l2_camera probably wants an actual merge, so maybe we need to add a\n> separate function for controls_ = other.\n> \n> Otherwise, looks good.\n> \n> \n> Paul\n> \n> > + {\n> > + controls_->merge(other);\n> > + }\n> > +\n> > + void setControl(unsigned int id, const ControlValue &value)\n> > + {\n> > + controls_->set(id, value);\n> > + }\n> > +\n> > private:\n> > LIBCAMERA_DISABLE_COPY(Request)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 80ff248c2aa316d2f7ccc59d4621a7c86770bf5f..80145f6d103546b4214316f58ecf283accd108ff 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -804,19 +804,19 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > return 0;\n> > \n> > /* Translate the Android request settings to libcamera controls. */\n> > - ControlList &controls = descriptor->request_->controls();\n> > + Request *req = descriptor->request_.get();\n> > camera_metadata_ro_entry_t entry;\n> > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) {\n> > const int32_t *data = entry.data.i32;\n> > Rectangle cropRegion{ data[0], data[1],\n> > static_cast(data[2]),\n> > static_cast(data[3]) };\n> > - controls.set(controls::ScalerCrop, cropRegion);\n> > + req->setControl(controls::ScalerCrop, cropRegion);\n> > }\n> > \n> > if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> > const uint8_t *data = entry.data.u8;\n> > - controls.set(controls::draft::FaceDetectMode, data[0]);\n> > + req->setControl(controls::draft::FaceDetectMode, data[0]);\n> > }\n> > \n> > if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> > @@ -854,7 +854,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > return -EINVAL;\n> > }\n> > \n> > - controls.set(controls::draft::TestPatternMode, testPatternMode);\n> > + req->setControl(controls::draft::TestPatternMode, testPatternMode);\n> > }\n> > \n> > return 0;\n> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..246806a0502cec465ac85989f7c93d5885427767 100644\n> > --- a/src/apps/cam/camera_session.cpp\n> > +++ b/src/apps/cam/camera_session.cpp\n> > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)\n> > return 0;\n> > \n> > if (script_)\n> > - request->controls() = script_->frameControls(queueCount_);\n> > + request->setControls(script_->frameControls(queueCount_));\n> > \n> > queueCount_++;\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > index 6faf3ee7a6c3401bb42c3720505ea9d5d6f4186a..40a543e755e29f29b797417a58537fc2d05fa9ea 100644\n> > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > @@ -276,7 +276,7 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > \n> > void GstCameraControls::applyControls(std::unique_ptr &request)\n> > {\n> > - request->controls().merge(controls_);\n> > + request->setControls(controls_);\n> > controls_.clear();\n> > }\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..1918c6f4340d790e93ac23baa906f038cd30f0bc 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1366,7 +1366,7 @@ int Camera::queueRequest(Request *request)\n> > }\n> > \n> > /* Pre-process AeEnable. */\n> > - patchControlList(request->controls());\n> > + patchControlList(request->_d()->controls());\n> > \n> > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > ConnectionTypeQueued, request);\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index a661b2f5c8ae9ae2bcbab2dcdceeef7dcb8d0930..c8c47f4c75cad116efcdd95032b5def19c900d95 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -87,6 +87,16 @@ bool Request::Private::hasPendingBuffers() const\n> > return !pending_.empty();\n> > }\n> > \n> > +/**\n> > + * \\fn Request::Private::controls()\n> > + * \\brief Retrieve the request's ControlList\n> > + * \\return A reference to the ControlList in this request\n> > + */\n> > +ControlList &Request::Private::controls()\n> > +{\n> > + return *_o()->controls_;\n> > +}\n> > +\n> > /**\n> > * \\fn Request::Private::metadata()\n> > * \\brief Retrieve the request's metadata\n> > @@ -414,19 +424,9 @@ void Request::reuse(ReuseFlag flags)\n> > }\n> > \n> > /**\n> > - * \\fn Request::controls()\n> > - * \\brief Retrieve the request's ControlList\n> > - *\n> > - * Requests store a list of controls to be applied to all frames captured for\n> > - * the request. They are created with an empty list of controls that can be\n> > - * accessed through this function. Control values can be retrieved using\n> > - * ControlList::get() and updated using ControlList::set().\n> > - *\n> > - * Only controls supported by the camera to which this request will be\n> > - * submitted shall be included in the controls list. Attempting to add an\n> > - * unsupported control causes undefined behaviour.\n> > - *\n> > - * \\return A reference to the ControlList in this request\n> > + * \\fn Request::controls() const\n> > + * \\brief Retrieve a const reference to the request's ControlList\n> > + * \\return A const reference to the ControlList in this request\n> > */\n> > \n> > /**\n> > @@ -623,4 +623,24 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n> > return out;\n> > }\n> > \n> > +/**\n> > + * \\fn Request::setControl(const Control &ctrl, const V &value)\n> > + * \\brief Set control \\a ctrl in the Request\n> > + * \\param[in] ctrl The control\n> > + * \\param[in] value The control value\n> > + */\n> > +\n> > +/**\n> > + * \\fn Request::setControls(const ControlList &other)\n> > + * \\brief Merge the control list \\a other in the Request\n> > + * \\param[in] other The control list to add to the Request\n> > + */\n> > +\n> > +/**\n> > + * \\fn Request::setControl(unsigned int id, const ControlValue &value)\n> > + * \\brief Set control \\a id in the Request to \\a value\n> > + * \\param[in] id The control numerical id\n> > + * \\param[in] value The control value\n> > + */\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index a983ea75c35669cc5d1ff433eddfdd99e9388e7c..97cc6cfbe7fbc4ece5c7a779a9ae6c8c7d23cf7b 100644\n> > --- a/src/py/libcamera/py_main.cpp\n> > +++ b/src/py/libcamera/py_main.cpp\n> > @@ -463,7 +463,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > .def_property_readonly(\"sequence\", &Request::sequence)\n> > .def_property_readonly(\"has_pending_buffers\", &Request::hasPendingBuffers)\n> > .def(\"set_control\", [](Request &self, const ControlId &id, py::object value) {\n> > - self.controls().set(id.id(), pyToControlValue(value, id.type()));\n> > + self.setControl(id.id(), pyToControlValue(value, id.type()));\n> > })\n> > .def_property_readonly(\"metadata\", [](Request &self) {\n> > /* Convert ControlList to std container */\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 94d138cd5710b9bd5c83cacd3cbd963f71aeb3c7..7e918cc3e803cdad67043f1dbdc212d5b57e37f3 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -269,7 +269,7 @@ int V4L2Camera::qbuf(unsigned int index)\n> > return 0;\n> > }\n> > \n> > - request->controls().merge(std::move(controls_));\n> > + request->setControls(controls_);\n> > \n> > ret = camera_->queueRequest(request);\n> > if (ret < 0) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 230B2C0F1B\n\tfor ;\n\tMon, 1 Dec 2025 15:02:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACA8760C23;\n\tMon, 1 Dec 2025 16:02:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B85F260A7B\n\tfor ;\n\tMon, 1 Dec 2025 16:02:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p9411226-ipngn12302marunouchi.tokyo.ocn.ne.jp [153.160.235.226])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 24EA84F1;\n\tMon, 1 Dec 2025 15:59:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ve89gayC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764601200;\n\tbh=mlPRAyuDW/JLHijL2o2EDTuQ8OiTN9EHCnTT0yHB8E4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ve89gayCty62ZPE3wu2/693CYGw2Fm0kXOi+xJojG6Q3I56ftHQZDlXR7YqLnXOJC\n\tQDyQzJSaJJVZBdY37tYFXrO3c4/Jc46fMXqvB1meGadCTh9Nu5TB74hQnClxHwpzvG\n\tX7Ik6N88Lwp8G6cxEiMmCqH+UU36WZb0OlMN0VTc=","Date":"Tue, 2 Dec 2025 00:01:54 +0900","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","Message-ID":"<20251201150154.GA29230@pendragon.ideasonboard.com>","References":"<20251126-cam-control-override-v2-0-305024a1f190@ideasonboard.com>\n\t<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>\n\t<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37147,"web_url":"https://patchwork.libcamera.org/comment/37147/","msgid":"<20251201152556.GB29230@pendragon.ideasonboard.com>","date":"2025-12-01T15:25:56","subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 02, 2025 at 12:01:54AM +0900, Laurent Pinchart wrote:\n> On Mon, Dec 01, 2025 at 07:23:32PM +0900, Paul Elder wrote:\n> > Quoting Jacopo Mondi (2025-11-26 18:38:53)\n> > > The ControlList inside a Request is initialized with the Camera\n> > > ControlInfoMap, which lists the limits of the controls supported\n> > > by the Camera.\n> > > \n> > > The Request class currently exposes the control list it contains through\n> > > the Request::controls() function as a raw pointer, potentially allowing\n> > > applications to replace the Request's control list with a different one.\n> > > \n> > > The following pattern:\n> > > \n> > > request->controls() = {};\n> > > \n> > > is then currently valid but opens the door to potential issues, as the\n> > > new list might be initialized with a different info map or without any\n> > > info map at all.\n> > > \n> > > To avoid applications to override the control list contained in a\n> > > Request, make the Request::controls() function return a const reference\n> > > and move the non-const version to the Request::Private class for\n> > > internal usage.\n> > > \n> > > Add to the Request class 4 overloads of the setControl() function that\n> > > allows applications to set controls on a Request control list without\n> > > accessing it.\n> > \n> > I think good idea.\n> \n> I'm afraid I don't agree :-( The whole point of having a ControlList is\n> to use it as a container for controls. Setting a control shouldn't be\n> done on the Request class itself, otherwise we could as well stop\n> exposing the ControlList class completely.\n> \n> If you want to avoid\n> \n> \trequest->controls() = {};\n> \n> you could stop exposing the ControlList assignment operator.\n\nProbably not a workable option as I think we need that operator inside\nlibcamera. It may still be worth a try though. I'd recommend thinking of\nthe C API and how it will end up storing controls in a buffer allocated\nfor the request.\n\nOther things to consider could be to prevent applications from\nconstructing a ControlList in the first place. That may face the same\ndifficulties as not exposing the assignment operator though, but maybe\nit would be possible not to expose the ControlIdMap and ControlInfoMap\nto applications to prevent them from creating incorrect ControlList\ninstances. The default constructor would still be available, but we\ncould quite easily catch \n\n\trequest->controls() = {};\n\nby checking for a null idmap when queuing the request and returning an\nerror.\n\nI don't know what the right solution is, but I think this patch isn't\nit.\n\n> Other design options are likely possible too.\n> \n> > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/request.h | 1 +\n> > > include/libcamera/request.h | 27 +++++++++++++++++-\n> > > src/android/camera_device.cpp | 8 +++---\n> > > src/apps/cam/camera_session.cpp | 2 +-\n> > > src/gstreamer/gstlibcamera-controls.cpp.in | 2 +-\n> > > src/libcamera/camera.cpp | 2 +-\n> > > src/libcamera/request.cpp | 46 +++++++++++++++++++++---------\n> > > src/py/libcamera/py_main.cpp | 2 +-\n> > > src/v4l2/v4l2_camera.cpp | 2 +-\n> > > 9 files changed, 69 insertions(+), 23 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > index 643f67cabf5778f7515c0117d06d4e0a3fdec4f8..4807036b7ae22c0e90886889ff5741fd9c5c0e9f 100644\n> > > --- a/include/libcamera/internal/request.h\n> > > +++ b/include/libcamera/internal/request.h\n> > > @@ -36,6 +36,7 @@ public:\n> > > Camera *camera() const { return camera_; }\n> > > bool hasPendingBuffers() const;\n> > > \n> > > + ControlList &controls();\n> > > ControlList &metadata() { return *metadata_; }\n> > > \n> > > bool completeBuffer(FrameBuffer *buffer);\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index c9aeddb62680923dc3490a23dc6c3f70f70cc075..31860e1e1e7680ded00148b7d96ca03f820d1ebc 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -49,7 +49,7 @@ public:\n> > > \n> > > void reuse(ReuseFlag flags = Default);\n> > > \n> > > - ControlList &controls() { return *controls_; }\n> > > + const ControlList &controls() const { return *controls_; }\n> > > const ControlList &metadata() const;\n> > > const BufferMap &buffers() const { return bufferMap_; }\n> > > int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> > > @@ -64,6 +64,31 @@ public:\n> > > \n> > > std::string toString() const;\n> > > \n> > > + template\n> > > + void setControl(const Control &ctrl, const V &value)\n> > > + {\n> > > + controls_->set(ctrl, value);\n> > > + }\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > + template\n> > > + void setControl(const Control> &ctrl,\n> > > + const Span &values)\n> > > + {\n> > > + controls_->set(ctrl, values);\n> > > + }\n> > > +#endif\n> > > +\n> > > + void setControls(const ControlList &other)\n> > \n> > imo s/setControls/merge/ because the meaning of \"set\" feels too different from\n> > \"merge\". I also wonder that it might be useful for applications to be able to\n> > set the merge policy.\n> > \n> > Unless you want to actually set controls_ = other, in which case I don't think\n> > merge() is the right way? (although since you used the default merge policy of\n> > KeepExisting maybe that wasn't the intention?)\n> > \n> > Judging from how you use it, afaict cam probably wants full overwrite while gst\n> > and v4l2_camera probably wants an actual merge, so maybe we need to add a\n> > separate function for controls_ = other.\n> > \n> > Otherwise, looks good.\n> > \n> > \n> > Paul\n> > \n> > > + {\n> > > + controls_->merge(other);\n> > > + }\n> > > +\n> > > + void setControl(unsigned int id, const ControlValue &value)\n> > > + {\n> > > + controls_->set(id, value);\n> > > + }\n> > > +\n> > > private:\n> > > LIBCAMERA_DISABLE_COPY(Request)\n> > > \n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 80ff248c2aa316d2f7ccc59d4621a7c86770bf5f..80145f6d103546b4214316f58ecf283accd108ff 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -804,19 +804,19 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > return 0;\n> > > \n> > > /* Translate the Android request settings to libcamera controls. */\n> > > - ControlList &controls = descriptor->request_->controls();\n> > > + Request *req = descriptor->request_.get();\n> > > camera_metadata_ro_entry_t entry;\n> > > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) {\n> > > const int32_t *data = entry.data.i32;\n> > > Rectangle cropRegion{ data[0], data[1],\n> > > static_cast(data[2]),\n> > > static_cast(data[3]) };\n> > > - controls.set(controls::ScalerCrop, cropRegion);\n> > > + req->setControl(controls::ScalerCrop, cropRegion);\n> > > }\n> > > \n> > > if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> > > const uint8_t *data = entry.data.u8;\n> > > - controls.set(controls::draft::FaceDetectMode, data[0]);\n> > > + req->setControl(controls::draft::FaceDetectMode, data[0]);\n> > > }\n> > > \n> > > if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> > > @@ -854,7 +854,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > return -EINVAL;\n> > > }\n> > > \n> > > - controls.set(controls::draft::TestPatternMode, testPatternMode);\n> > > + req->setControl(controls::draft::TestPatternMode, testPatternMode);\n> > > }\n> > > \n> > > return 0;\n> > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..246806a0502cec465ac85989f7c93d5885427767 100644\n> > > --- a/src/apps/cam/camera_session.cpp\n> > > +++ b/src/apps/cam/camera_session.cpp\n> > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)\n> > > return 0;\n> > > \n> > > if (script_)\n> > > - request->controls() = script_->frameControls(queueCount_);\n> > > + request->setControls(script_->frameControls(queueCount_));\n> > > \n> > > queueCount_++;\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > index 6faf3ee7a6c3401bb42c3720505ea9d5d6f4186a..40a543e755e29f29b797417a58537fc2d05fa9ea 100644\n> > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > @@ -276,7 +276,7 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > > \n> > > void GstCameraControls::applyControls(std::unique_ptr &request)\n> > > {\n> > > - request->controls().merge(controls_);\n> > > + request->setControls(controls_);\n> > > controls_.clear();\n> > > }\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..1918c6f4340d790e93ac23baa906f038cd30f0bc 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1366,7 +1366,7 @@ int Camera::queueRequest(Request *request)\n> > > }\n> > > \n> > > /* Pre-process AeEnable. */\n> > > - patchControlList(request->controls());\n> > > + patchControlList(request->_d()->controls());\n> > > \n> > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > > ConnectionTypeQueued, request);\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index a661b2f5c8ae9ae2bcbab2dcdceeef7dcb8d0930..c8c47f4c75cad116efcdd95032b5def19c900d95 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -87,6 +87,16 @@ bool Request::Private::hasPendingBuffers() const\n> > > return !pending_.empty();\n> > > }\n> > > \n> > > +/**\n> > > + * \\fn Request::Private::controls()\n> > > + * \\brief Retrieve the request's ControlList\n> > > + * \\return A reference to the ControlList in this request\n> > > + */\n> > > +ControlList &Request::Private::controls()\n> > > +{\n> > > + return *_o()->controls_;\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn Request::Private::metadata()\n> > > * \\brief Retrieve the request's metadata\n> > > @@ -414,19 +424,9 @@ void Request::reuse(ReuseFlag flags)\n> > > }\n> > > \n> > > /**\n> > > - * \\fn Request::controls()\n> > > - * \\brief Retrieve the request's ControlList\n> > > - *\n> > > - * Requests store a list of controls to be applied to all frames captured for\n> > > - * the request. They are created with an empty list of controls that can be\n> > > - * accessed through this function. Control values can be retrieved using\n> > > - * ControlList::get() and updated using ControlList::set().\n> > > - *\n> > > - * Only controls supported by the camera to which this request will be\n> > > - * submitted shall be included in the controls list. Attempting to add an\n> > > - * unsupported control causes undefined behaviour.\n> > > - *\n> > > - * \\return A reference to the ControlList in this request\n> > > + * \\fn Request::controls() const\n> > > + * \\brief Retrieve a const reference to the request's ControlList\n> > > + * \\return A const reference to the ControlList in this request\n> > > */\n> > > \n> > > /**\n> > > @@ -623,4 +623,24 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n> > > return out;\n> > > }\n> > > \n> > > +/**\n> > > + * \\fn Request::setControl(const Control &ctrl, const V &value)\n> > > + * \\brief Set control \\a ctrl in the Request\n> > > + * \\param[in] ctrl The control\n> > > + * \\param[in] value The control value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Request::setControls(const ControlList &other)\n> > > + * \\brief Merge the control list \\a other in the Request\n> > > + * \\param[in] other The control list to add to the Request\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Request::setControl(unsigned int id, const ControlValue &value)\n> > > + * \\brief Set control \\a id in the Request to \\a value\n> > > + * \\param[in] id The control numerical id\n> > > + * \\param[in] value The control value\n> > > + */\n> > > +\n> > > } /* namespace libcamera */\n> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > > index a983ea75c35669cc5d1ff433eddfdd99e9388e7c..97cc6cfbe7fbc4ece5c7a779a9ae6c8c7d23cf7b 100644\n> > > --- a/src/py/libcamera/py_main.cpp\n> > > +++ b/src/py/libcamera/py_main.cpp\n> > > @@ -463,7 +463,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > .def_property_readonly(\"sequence\", &Request::sequence)\n> > > .def_property_readonly(\"has_pending_buffers\", &Request::hasPendingBuffers)\n> > > .def(\"set_control\", [](Request &self, const ControlId &id, py::object value) {\n> > > - self.controls().set(id.id(), pyToControlValue(value, id.type()));\n> > > + self.setControl(id.id(), pyToControlValue(value, id.type()));\n> > > })\n> > > .def_property_readonly(\"metadata\", [](Request &self) {\n> > > /* Convert ControlList to std container */\n> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > index 94d138cd5710b9bd5c83cacd3cbd963f71aeb3c7..7e918cc3e803cdad67043f1dbdc212d5b57e37f3 100644\n> > > --- a/src/v4l2/v4l2_camera.cpp\n> > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > @@ -269,7 +269,7 @@ int V4L2Camera::qbuf(unsigned int index)\n> > > return 0;\n> > > }\n> > > \n> > > - request->controls().merge(std::move(controls_));\n> > > + request->setControls(controls_);\n> > > \n> > > ret = camera_->queueRequest(request);\n> > > if (ret < 0) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D9BEBD80A\n\tfor ;\n\tMon, 1 Dec 2025 15:26:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C63E60BE2;\n\tMon, 1 Dec 2025 16:26:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35AD3606D5\n\tfor ;\n\tMon, 1 Dec 2025 16:26:17 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p9411226-ipngn12302marunouchi.tokyo.ocn.ne.jp [153.160.235.226])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id BF2EA6DF;\n\tMon, 1 Dec 2025 16:24:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"O+/5DEoq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764602643;\n\tbh=umADePVZzcn2x2SLNCTVRUUI4iLQ5zVVyv4b9AI2GOs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O+/5DEoqZKfuSwewIZ9e75XOz8JClG9pUm2H9Q+99sURRSei7brkRjT4HngPIG1ej\n\tpiqgbyvsBRKAdIKrAoe8CDOTE0UeSAJSOb39EDMveE7HHYvfCcM8bmHY9ARWOG1YIR\n\t2If32V34g3+9jD/6RnqF7HyzMLIh2T1O47k9H4sc=","Date":"Tue, 2 Dec 2025 00:25:56 +0900","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","Message-ID":"<20251201152556.GB29230@pendragon.ideasonboard.com>","References":"<20251126-cam-control-override-v2-0-305024a1f190@ideasonboard.com>\n\t<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>\n\t<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>\n\t<20251201150154.GA29230@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251201150154.GA29230@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37152,"web_url":"https://patchwork.libcamera.org/comment/37152/","msgid":"<20251202021927.GE32430@pendragon.ideasonboard.com>","date":"2025-12-02T02:19:27","subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 02, 2025 at 12:25:56AM +0900, Laurent Pinchart wrote:\n> On Tue, Dec 02, 2025 at 12:01:54AM +0900, Laurent Pinchart wrote:\n> > On Mon, Dec 01, 2025 at 07:23:32PM +0900, Paul Elder wrote:\n> > > Quoting Jacopo Mondi (2025-11-26 18:38:53)\n> > > > The ControlList inside a Request is initialized with the Camera\n> > > > ControlInfoMap, which lists the limits of the controls supported\n> > > > by the Camera.\n> > > > \n> > > > The Request class currently exposes the control list it contains through\n> > > > the Request::controls() function as a raw pointer, potentially allowing\n> > > > applications to replace the Request's control list with a different one.\n> > > > \n> > > > The following pattern:\n> > > > \n> > > > request->controls() = {};\n> > > > \n> > > > is then currently valid but opens the door to potential issues, as the\n> > > > new list might be initialized with a different info map or without any\n> > > > info map at all.\n> > > > \n> > > > To avoid applications to override the control list contained in a\n> > > > Request, make the Request::controls() function return a const reference\n> > > > and move the non-const version to the Request::Private class for\n> > > > internal usage.\n> > > > \n> > > > Add to the Request class 4 overloads of the setControl() function that\n> > > > allows applications to set controls on a Request control list without\n> > > > accessing it.\n> > > \n> > > I think good idea.\n> > \n> > I'm afraid I don't agree :-( The whole point of having a ControlList is\n> > to use it as a container for controls. Setting a control shouldn't be\n> > done on the Request class itself, otherwise we could as well stop\n> > exposing the ControlList class completely.\n> > \n> > If you want to avoid\n> > \n> > \trequest->controls() = {};\n> > \n> > you could stop exposing the ControlList assignment operator.\n> \n> Probably not a workable option as I think we need that operator inside\n> libcamera. It may still be worth a try though. I'd recommend thinking of\n> the C API and how it will end up storing controls in a buffer allocated\n> for the request.\n> \n> Other things to consider could be to prevent applications from\n> constructing a ControlList in the first place. That may face the same\n> difficulties as not exposing the assignment operator though, but maybe\n> it would be possible not to expose the ControlIdMap and ControlInfoMap\n> to applications to prevent them from creating incorrect ControlList\n> instances. The default constructor would still be available, but we\n> could quite easily catch \n> \n> \trequest->controls() = {};\n> \n> by checking for a null idmap when queuing the request and returning an\n> error.\n> \n> I don't know what the right solution is, but I think this patch isn't\n> it.\n\nSleeping over this made me think that we probably shouldn't create a\nfool-proof API here. Moving setControl() to the Request class would have\na big impact on all applications, and I don't want the force that pain\nonto users when we know that we need to rewrite the controls API for the\nstable C ABI.\n\nWhat I think we can however do is improve the situation with extra\nchecks at Camera::queueRequest() time. If we return an error there when\nthe control list has no idMap/infoMap, we'll catch constructs like\n\n\trequest->controls() = {};\n\nWe could take additional steps by trying to make the two non-default\nconstructors impossible to use for applications.\n\n\tControlList(const ControlIdMap &idmap, const ControlValidator *validator = nullptr);\n\tControlList(const ControlInfoMap &infoMap, const ControlValidator *validator = nullptr);\n\nMy first idea was to stop exposing ControlIdMap and/or ControlInfoMap,\nas ControList::infoMap() and ControlList::idMap() don't seem to be used\nby applications. Unfortunately, ControlInfoMap is the return type of\nCamera::controls(), so we need to keep it. It may be possible to add an\nargument to those constructors with a type not exposed through the\npublic API, but that's quite a hack. I don't think it's worth guarding\nagainst that for the time being.\n\n> > Other design options are likely possible too.\n> > \n> > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/request.h | 1 +\n> > > > include/libcamera/request.h | 27 +++++++++++++++++-\n> > > > src/android/camera_device.cpp | 8 +++---\n> > > > src/apps/cam/camera_session.cpp | 2 +-\n> > > > src/gstreamer/gstlibcamera-controls.cpp.in | 2 +-\n> > > > src/libcamera/camera.cpp | 2 +-\n> > > > src/libcamera/request.cpp | 46 +++++++++++++++++++++---------\n> > > > src/py/libcamera/py_main.cpp | 2 +-\n> > > > src/v4l2/v4l2_camera.cpp | 2 +-\n> > > > 9 files changed, 69 insertions(+), 23 deletions(-)\n> > > > \n> > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > index 643f67cabf5778f7515c0117d06d4e0a3fdec4f8..4807036b7ae22c0e90886889ff5741fd9c5c0e9f 100644\n> > > > --- a/include/libcamera/internal/request.h\n> > > > +++ b/include/libcamera/internal/request.h\n> > > > @@ -36,6 +36,7 @@ public:\n> > > > Camera *camera() const { return camera_; }\n> > > > bool hasPendingBuffers() const;\n> > > > \n> > > > + ControlList &controls();\n> > > > ControlList &metadata() { return *metadata_; }\n> > > > \n> > > > bool completeBuffer(FrameBuffer *buffer);\n> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > index c9aeddb62680923dc3490a23dc6c3f70f70cc075..31860e1e1e7680ded00148b7d96ca03f820d1ebc 100644\n> > > > --- a/include/libcamera/request.h\n> > > > +++ b/include/libcamera/request.h\n> > > > @@ -49,7 +49,7 @@ public:\n> > > > \n> > > > void reuse(ReuseFlag flags = Default);\n> > > > \n> > > > - ControlList &controls() { return *controls_; }\n> > > > + const ControlList &controls() const { return *controls_; }\n> > > > const ControlList &metadata() const;\n> > > > const BufferMap &buffers() const { return bufferMap_; }\n> > > > int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> > > > @@ -64,6 +64,31 @@ public:\n> > > > \n> > > > std::string toString() const;\n> > > > \n> > > > + template\n> > > > + void setControl(const Control &ctrl, const V &value)\n> > > > + {\n> > > > + controls_->set(ctrl, value);\n> > > > + }\n> > > > +\n> > > > +#ifndef __DOXYGEN__\n> > > > + template\n> > > > + void setControl(const Control> &ctrl,\n> > > > + const Span &values)\n> > > > + {\n> > > > + controls_->set(ctrl, values);\n> > > > + }\n> > > > +#endif\n> > > > +\n> > > > + void setControls(const ControlList &other)\n> > > \n> > > imo s/setControls/merge/ because the meaning of \"set\" feels too different from\n> > > \"merge\". I also wonder that it might be useful for applications to be able to\n> > > set the merge policy.\n> > > \n> > > Unless you want to actually set controls_ = other, in which case I don't think\n> > > merge() is the right way? (although since you used the default merge policy of\n> > > KeepExisting maybe that wasn't the intention?)\n> > > \n> > > Judging from how you use it, afaict cam probably wants full overwrite while gst\n> > > and v4l2_camera probably wants an actual merge, so maybe we need to add a\n> > > separate function for controls_ = other.\n> > > \n> > > Otherwise, looks good.\n> > > \n> > > \n> > > Paul\n> > > \n> > > > + {\n> > > > + controls_->merge(other);\n> > > > + }\n> > > > +\n> > > > + void setControl(unsigned int id, const ControlValue &value)\n> > > > + {\n> > > > + controls_->set(id, value);\n> > > > + }\n> > > > +\n> > > > private:\n> > > > LIBCAMERA_DISABLE_COPY(Request)\n> > > > \n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 80ff248c2aa316d2f7ccc59d4621a7c86770bf5f..80145f6d103546b4214316f58ecf283accd108ff 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -804,19 +804,19 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > return 0;\n> > > > \n> > > > /* Translate the Android request settings to libcamera controls. */\n> > > > - ControlList &controls = descriptor->request_->controls();\n> > > > + Request *req = descriptor->request_.get();\n> > > > camera_metadata_ro_entry_t entry;\n> > > > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) {\n> > > > const int32_t *data = entry.data.i32;\n> > > > Rectangle cropRegion{ data[0], data[1],\n> > > > static_cast(data[2]),\n> > > > static_cast(data[3]) };\n> > > > - controls.set(controls::ScalerCrop, cropRegion);\n> > > > + req->setControl(controls::ScalerCrop, cropRegion);\n> > > > }\n> > > > \n> > > > if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> > > > const uint8_t *data = entry.data.u8;\n> > > > - controls.set(controls::draft::FaceDetectMode, data[0]);\n> > > > + req->setControl(controls::draft::FaceDetectMode, data[0]);\n> > > > }\n> > > > \n> > > > if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> > > > @@ -854,7 +854,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > return -EINVAL;\n> > > > }\n> > > > \n> > > > - controls.set(controls::draft::TestPatternMode, testPatternMode);\n> > > > + req->setControl(controls::draft::TestPatternMode, testPatternMode);\n> > > > }\n> > > > \n> > > > return 0;\n> > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..246806a0502cec465ac85989f7c93d5885427767 100644\n> > > > --- a/src/apps/cam/camera_session.cpp\n> > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)\n> > > > return 0;\n> > > > \n> > > > if (script_)\n> > > > - request->controls() = script_->frameControls(queueCount_);\n> > > > + request->setControls(script_->frameControls(queueCount_));\n> > > > \n> > > > queueCount_++;\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > index 6faf3ee7a6c3401bb42c3720505ea9d5d6f4186a..40a543e755e29f29b797417a58537fc2d05fa9ea 100644\n> > > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > @@ -276,7 +276,7 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > > > \n> > > > void GstCameraControls::applyControls(std::unique_ptr &request)\n> > > > {\n> > > > - request->controls().merge(controls_);\n> > > > + request->setControls(controls_);\n> > > > controls_.clear();\n> > > > }\n> > > > \n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..1918c6f4340d790e93ac23baa906f038cd30f0bc 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -1366,7 +1366,7 @@ int Camera::queueRequest(Request *request)\n> > > > }\n> > > > \n> > > > /* Pre-process AeEnable. */\n> > > > - patchControlList(request->controls());\n> > > > + patchControlList(request->_d()->controls());\n> > > > \n> > > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > > > ConnectionTypeQueued, request);\n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index a661b2f5c8ae9ae2bcbab2dcdceeef7dcb8d0930..c8c47f4c75cad116efcdd95032b5def19c900d95 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -87,6 +87,16 @@ bool Request::Private::hasPendingBuffers() const\n> > > > return !pending_.empty();\n> > > > }\n> > > > \n> > > > +/**\n> > > > + * \\fn Request::Private::controls()\n> > > > + * \\brief Retrieve the request's ControlList\n> > > > + * \\return A reference to the ControlList in this request\n> > > > + */\n> > > > +ControlList &Request::Private::controls()\n> > > > +{\n> > > > + return *_o()->controls_;\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\fn Request::Private::metadata()\n> > > > * \\brief Retrieve the request's metadata\n> > > > @@ -414,19 +424,9 @@ void Request::reuse(ReuseFlag flags)\n> > > > }\n> > > > \n> > > > /**\n> > > > - * \\fn Request::controls()\n> > > > - * \\brief Retrieve the request's ControlList\n> > > > - *\n> > > > - * Requests store a list of controls to be applied to all frames captured for\n> > > > - * the request. They are created with an empty list of controls that can be\n> > > > - * accessed through this function. Control values can be retrieved using\n> > > > - * ControlList::get() and updated using ControlList::set().\n> > > > - *\n> > > > - * Only controls supported by the camera to which this request will be\n> > > > - * submitted shall be included in the controls list. Attempting to add an\n> > > > - * unsupported control causes undefined behaviour.\n> > > > - *\n> > > > - * \\return A reference to the ControlList in this request\n> > > > + * \\fn Request::controls() const\n> > > > + * \\brief Retrieve a const reference to the request's ControlList\n> > > > + * \\return A const reference to the ControlList in this request\n> > > > */\n> > > > \n> > > > /**\n> > > > @@ -623,4 +623,24 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n> > > > return out;\n> > > > }\n> > > > \n> > > > +/**\n> > > > + * \\fn Request::setControl(const Control &ctrl, const V &value)\n> > > > + * \\brief Set control \\a ctrl in the Request\n> > > > + * \\param[in] ctrl The control\n> > > > + * \\param[in] value The control value\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Request::setControls(const ControlList &other)\n> > > > + * \\brief Merge the control list \\a other in the Request\n> > > > + * \\param[in] other The control list to add to the Request\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Request::setControl(unsigned int id, const ControlValue &value)\n> > > > + * \\brief Set control \\a id in the Request to \\a value\n> > > > + * \\param[in] id The control numerical id\n> > > > + * \\param[in] value The control value\n> > > > + */\n> > > > +\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > > > index a983ea75c35669cc5d1ff433eddfdd99e9388e7c..97cc6cfbe7fbc4ece5c7a779a9ae6c8c7d23cf7b 100644\n> > > > --- a/src/py/libcamera/py_main.cpp\n> > > > +++ b/src/py/libcamera/py_main.cpp\n> > > > @@ -463,7 +463,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > > .def_property_readonly(\"sequence\", &Request::sequence)\n> > > > .def_property_readonly(\"has_pending_buffers\", &Request::hasPendingBuffers)\n> > > > .def(\"set_control\", [](Request &self, const ControlId &id, py::object value) {\n> > > > - self.controls().set(id.id(), pyToControlValue(value, id.type()));\n> > > > + self.setControl(id.id(), pyToControlValue(value, id.type()));\n> > > > })\n> > > > .def_property_readonly(\"metadata\", [](Request &self) {\n> > > > /* Convert ControlList to std container */\n> > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > > index 94d138cd5710b9bd5c83cacd3cbd963f71aeb3c7..7e918cc3e803cdad67043f1dbdc212d5b57e37f3 100644\n> > > > --- a/src/v4l2/v4l2_camera.cpp\n> > > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > > @@ -269,7 +269,7 @@ int V4L2Camera::qbuf(unsigned int index)\n> > > > return 0;\n> > > > }\n> > > > \n> > > > - request->controls().merge(std::move(controls_));\n> > > > + request->setControls(controls_);\n> > > > \n> > > > ret = camera_->queueRequest(request);\n> > > > if (ret < 0) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 99982BD80A\n\tfor ;\n\tTue, 2 Dec 2025 02:19:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B87E960C6F;\n\tTue, 2 Dec 2025 03:19:51 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47D7960C23\n\tfor ;\n\tTue, 2 Dec 2025 03:19:49 +0100 (CET)","from pendragon.ideasonboard.com\n\t(113x43x203x98.ap113.ftth.arteria-hikari.net [113.43.203.98])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 77EDA55;\n\tTue, 2 Dec 2025 03:17:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Gmjs9inY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764641855;\n\tbh=379wFAekCE0j7cDp4eErf+ahV/9wvi+ienCcB9r53VM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gmjs9inYhKxR47+d0uNGuRqfdVvZC/akhVzGHEu3BU1S22ToKl+aAcCmAwPCO6k7X\n\tucTXpfE28XaTdD7iNZ7PdFsbtztwVNkzWgw/NNMNObceiYE36kfd+bPnNzukeeAj8v\n\t7ys8nXN+1IRGVVNXeOpq4pAIkSeiU23zJ/SH7oms=","Date":"Tue, 2 Dec 2025 11:19:27 +0900","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","Message-ID":"<20251202021927.GE32430@pendragon.ideasonboard.com>","References":"<20251126-cam-control-override-v2-0-305024a1f190@ideasonboard.com>\n\t<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>\n\t<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>\n\t<20251201150154.GA29230@pendragon.ideasonboard.com>\n\t<20251201152556.GB29230@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251201152556.GB29230@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37160,"web_url":"https://patchwork.libcamera.org/comment/37160/","msgid":"","date":"2025-12-02T13:50:05","subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\nOn Tue, Dec 02, 2025 at 11:19:27AM +0900, Laurent Pinchart wrote:\n> On Tue, Dec 02, 2025 at 12:25:56AM +0900, Laurent Pinchart wrote:\n> > On Tue, Dec 02, 2025 at 12:01:54AM +0900, Laurent Pinchart wrote:\n> > > On Mon, Dec 01, 2025 at 07:23:32PM +0900, Paul Elder wrote:\n> > > > Quoting Jacopo Mondi (2025-11-26 18:38:53)\n> > > > > The ControlList inside a Request is initialized with the Camera\n> > > > > ControlInfoMap, which lists the limits of the controls supported\n> > > > > by the Camera.\n> > > > >\n> > > > > The Request class currently exposes the control list it contains through\n> > > > > the Request::controls() function as a raw pointer, potentially allowing\n> > > > > applications to replace the Request's control list with a different one.\n> > > > >\n> > > > > The following pattern:\n> > > > >\n> > > > > request->controls() = {};\n> > > > >\n> > > > > is then currently valid but opens the door to potential issues, as the\n> > > > > new list might be initialized with a different info map or without any\n> > > > > info map at all.\n> > > > >\n> > > > > To avoid applications to override the control list contained in a\n> > > > > Request, make the Request::controls() function return a const reference\n> > > > > and move the non-const version to the Request::Private class for\n> > > > > internal usage.\n> > > > >\n> > > > > Add to the Request class 4 overloads of the setControl() function that\n> > > > > allows applications to set controls on a Request control list without\n> > > > > accessing it.\n> > > >\n> > > > I think good idea.\n> > >\n> > > I'm afraid I don't agree :-( The whole point of having a ControlList is\n> > > to use it as a container for controls. Setting a control shouldn't be\n> > > done on the Request class itself, otherwise we could as well stop\n> > > exposing the ControlList class completely.\n> > >\n> > > If you want to avoid\n> > >\n> > > \trequest->controls() = {};\n> > >\n> > > you could stop exposing the ControlList assignment operator.\n> >\n> > Probably not a workable option as I think we need that operator inside\n> > libcamera. It may still be worth a try though. I'd recommend thinking of\n> > the C API and how it will end up storing controls in a buffer allocated\n> > for the request.\n> >\n> > Other things to consider could be to prevent applications from\n> > constructing a ControlList in the first place. That may face the same\n> > difficulties as not exposing the assignment operator though, but maybe\n> > it would be possible not to expose the ControlIdMap and ControlInfoMap\n> > to applications to prevent them from creating incorrect ControlList\n> > instances. The default constructor would still be available, but we\n> > could quite easily catch\n> >\n> > \trequest->controls() = {};\n> >\n> > by checking for a null idmap when queuing the request and returning an\n> > error.\n> >\n> > I don't know what the right solution is, but I think this patch isn't\n> > it.\n>\n> Sleeping over this made me think that we probably shouldn't create a\n> fool-proof API here. Moving setControl() to the Request class would have\n> a big impact on all applications, and I don't want the force that pain\n> onto users when we know that we need to rewrite the controls API for the\n> stable C ABI.\n\nAh that for sure\n\n>\n> What I think we can however do is improve the situation with extra\n> checks at Camera::queueRequest() time. If we return an error there when\n> the control list has no idMap/infoMap, we'll catch constructs like\n>\n> \trequest->controls() = {};\n>\n\nThat would work, I can do that on top of the first two patches in this\nseries\n\n> We could take additional steps by trying to make the two non-default\n> constructors impossible to use for applications.\n>\n> \tControlList(const ControlIdMap &idmap, const ControlValidator *validator = nullptr);\n> \tControlList(const ControlInfoMap &infoMap, const ControlValidator *validator = nullptr);\n>\n> My first idea was to stop exposing ControlIdMap and/or ControlInfoMap,\n> as ControList::infoMap() and ControlList::idMap() don't seem to be used\n> by applications. Unfortunately, ControlInfoMap is the return type of\n> Camera::controls(), so we need to keep it. It may be possible to add an\n> argument to those constructors with a type not exposed through the\n> public API, but that's quite a hack. I don't think it's worth guarding\n> against that for the time being.\n>\n\nAgred, feels like calling for troubles indeed\n\nI'll give the queueRequest() method a go\n\nThanks\n j\n\n> > > Other design options are likely possible too.\n> > >\n> > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/internal/request.h | 1 +\n> > > > > include/libcamera/request.h | 27 +++++++++++++++++-\n> > > > > src/android/camera_device.cpp | 8 +++---\n> > > > > src/apps/cam/camera_session.cpp | 2 +-\n> > > > > src/gstreamer/gstlibcamera-controls.cpp.in | 2 +-\n> > > > > src/libcamera/camera.cpp | 2 +-\n> > > > > src/libcamera/request.cpp | 46 +++++++++++++++++++++---------\n> > > > > src/py/libcamera/py_main.cpp | 2 +-\n> > > > > src/v4l2/v4l2_camera.cpp | 2 +-\n> > > > > 9 files changed, 69 insertions(+), 23 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> > > > > index 643f67cabf5778f7515c0117d06d4e0a3fdec4f8..4807036b7ae22c0e90886889ff5741fd9c5c0e9f 100644\n> > > > > --- a/include/libcamera/internal/request.h\n> > > > > +++ b/include/libcamera/internal/request.h\n> > > > > @@ -36,6 +36,7 @@ public:\n> > > > > Camera *camera() const { return camera_; }\n> > > > > bool hasPendingBuffers() const;\n> > > > >\n> > > > > + ControlList &controls();\n> > > > > ControlList &metadata() { return *metadata_; }\n> > > > >\n> > > > > bool completeBuffer(FrameBuffer *buffer);\n> > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > > > index c9aeddb62680923dc3490a23dc6c3f70f70cc075..31860e1e1e7680ded00148b7d96ca03f820d1ebc 100644\n> > > > > --- a/include/libcamera/request.h\n> > > > > +++ b/include/libcamera/request.h\n> > > > > @@ -49,7 +49,7 @@ public:\n> > > > >\n> > > > > void reuse(ReuseFlag flags = Default);\n> > > > >\n> > > > > - ControlList &controls() { return *controls_; }\n> > > > > + const ControlList &controls() const { return *controls_; }\n> > > > > const ControlList &metadata() const;\n> > > > > const BufferMap &buffers() const { return bufferMap_; }\n> > > > > int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> > > > > @@ -64,6 +64,31 @@ public:\n> > > > >\n> > > > > std::string toString() const;\n> > > > >\n> > > > > + template\n> > > > > + void setControl(const Control &ctrl, const V &value)\n> > > > > + {\n> > > > > + controls_->set(ctrl, value);\n> > > > > + }\n> > > > > +\n> > > > > +#ifndef __DOXYGEN__\n> > > > > + template\n> > > > > + void setControl(const Control> &ctrl,\n> > > > > + const Span &values)\n> > > > > + {\n> > > > > + controls_->set(ctrl, values);\n> > > > > + }\n> > > > > +#endif\n> > > > > +\n> > > > > + void setControls(const ControlList &other)\n> > > >\n> > > > imo s/setControls/merge/ because the meaning of \"set\" feels too different from\n> > > > \"merge\". I also wonder that it might be useful for applications to be able to\n> > > > set the merge policy.\n> > > >\n> > > > Unless you want to actually set controls_ = other, in which case I don't think\n> > > > merge() is the right way? (although since you used the default merge policy of\n> > > > KeepExisting maybe that wasn't the intention?)\n> > > >\n> > > > Judging from how you use it, afaict cam probably wants full overwrite while gst\n> > > > and v4l2_camera probably wants an actual merge, so maybe we need to add a\n> > > > separate function for controls_ = other.\n> > > >\n> > > > Otherwise, looks good.\n> > > >\n> > > >\n> > > > Paul\n> > > >\n> > > > > + {\n> > > > > + controls_->merge(other);\n> > > > > + }\n> > > > > +\n> > > > > + void setControl(unsigned int id, const ControlValue &value)\n> > > > > + {\n> > > > > + controls_->set(id, value);\n> > > > > + }\n> > > > > +\n> > > > > private:\n> > > > > LIBCAMERA_DISABLE_COPY(Request)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 80ff248c2aa316d2f7ccc59d4621a7c86770bf5f..80145f6d103546b4214316f58ecf283accd108ff 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -804,19 +804,19 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > > return 0;\n> > > > >\n> > > > > /* Translate the Android request settings to libcamera controls. */\n> > > > > - ControlList &controls = descriptor->request_->controls();\n> > > > > + Request *req = descriptor->request_.get();\n> > > > > camera_metadata_ro_entry_t entry;\n> > > > > if (settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry)) {\n> > > > > const int32_t *data = entry.data.i32;\n> > > > > Rectangle cropRegion{ data[0], data[1],\n> > > > > static_cast(data[2]),\n> > > > > static_cast(data[3]) };\n> > > > > - controls.set(controls::ScalerCrop, cropRegion);\n> > > > > + req->setControl(controls::ScalerCrop, cropRegion);\n> > > > > }\n> > > > >\n> > > > > if (settings.getEntry(ANDROID_STATISTICS_FACE_DETECT_MODE, &entry)) {\n> > > > > const uint8_t *data = entry.data.u8;\n> > > > > - controls.set(controls::draft::FaceDetectMode, data[0]);\n> > > > > + req->setControl(controls::draft::FaceDetectMode, data[0]);\n> > > > > }\n> > > > >\n> > > > > if (settings.getEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, &entry)) {\n> > > > > @@ -854,7 +854,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > > return -EINVAL;\n> > > > > }\n> > > > >\n> > > > > - controls.set(controls::draft::TestPatternMode, testPatternMode);\n> > > > > + req->setControl(controls::draft::TestPatternMode, testPatternMode);\n> > > > > }\n> > > > >\n> > > > > return 0;\n> > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..246806a0502cec465ac85989f7c93d5885427767 100644\n> > > > > --- a/src/apps/cam/camera_session.cpp\n> > > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)\n> > > > > return 0;\n> > > > >\n> > > > > if (script_)\n> > > > > - request->controls() = script_->frameControls(queueCount_);\n> > > > > + request->setControls(script_->frameControls(queueCount_));\n> > > > >\n> > > > > queueCount_++;\n> > > > >\n> > > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > > index 6faf3ee7a6c3401bb42c3720505ea9d5d6f4186a..40a543e755e29f29b797417a58537fc2d05fa9ea 100644\n> > > > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > > @@ -276,7 +276,7 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > > > >\n> > > > > void GstCameraControls::applyControls(std::unique_ptr &request)\n> > > > > {\n> > > > > - request->controls().merge(controls_);\n> > > > > + request->setControls(controls_);\n> > > > > controls_.clear();\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..1918c6f4340d790e93ac23baa906f038cd30f0bc 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -1366,7 +1366,7 @@ int Camera::queueRequest(Request *request)\n> > > > > }\n> > > > >\n> > > > > /* Pre-process AeEnable. */\n> > > > > - patchControlList(request->controls());\n> > > > > + patchControlList(request->_d()->controls());\n> > > > >\n> > > > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > > > > ConnectionTypeQueued, request);\n> > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > index a661b2f5c8ae9ae2bcbab2dcdceeef7dcb8d0930..c8c47f4c75cad116efcdd95032b5def19c900d95 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -87,6 +87,16 @@ bool Request::Private::hasPendingBuffers() const\n> > > > > return !pending_.empty();\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn Request::Private::controls()\n> > > > > + * \\brief Retrieve the request's ControlList\n> > > > > + * \\return A reference to the ControlList in this request\n> > > > > + */\n> > > > > +ControlList &Request::Private::controls()\n> > > > > +{\n> > > > > + return *_o()->controls_;\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\fn Request::Private::metadata()\n> > > > > * \\brief Retrieve the request's metadata\n> > > > > @@ -414,19 +424,9 @@ void Request::reuse(ReuseFlag flags)\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > - * \\fn Request::controls()\n> > > > > - * \\brief Retrieve the request's ControlList\n> > > > > - *\n> > > > > - * Requests store a list of controls to be applied to all frames captured for\n> > > > > - * the request. They are created with an empty list of controls that can be\n> > > > > - * accessed through this function. Control values can be retrieved using\n> > > > > - * ControlList::get() and updated using ControlList::set().\n> > > > > - *\n> > > > > - * Only controls supported by the camera to which this request will be\n> > > > > - * submitted shall be included in the controls list. Attempting to add an\n> > > > > - * unsupported control causes undefined behaviour.\n> > > > > - *\n> > > > > - * \\return A reference to the ControlList in this request\n> > > > > + * \\fn Request::controls() const\n> > > > > + * \\brief Retrieve a const reference to the request's ControlList\n> > > > > + * \\return A const reference to the ControlList in this request\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -623,4 +623,24 @@ std::ostream &operator<<(std::ostream &out, const Request &r)\n> > > > > return out;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn Request::setControl(const Control &ctrl, const V &value)\n> > > > > + * \\brief Set control \\a ctrl in the Request\n> > > > > + * \\param[in] ctrl The control\n> > > > > + * \\param[in] value The control value\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Request::setControls(const ControlList &other)\n> > > > > + * \\brief Merge the control list \\a other in the Request\n> > > > > + * \\param[in] other The control list to add to the Request\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Request::setControl(unsigned int id, const ControlValue &value)\n> > > > > + * \\brief Set control \\a id in the Request to \\a value\n> > > > > + * \\param[in] id The control numerical id\n> > > > > + * \\param[in] value The control value\n> > > > > + */\n> > > > > +\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > > > > index a983ea75c35669cc5d1ff433eddfdd99e9388e7c..97cc6cfbe7fbc4ece5c7a779a9ae6c8c7d23cf7b 100644\n> > > > > --- a/src/py/libcamera/py_main.cpp\n> > > > > +++ b/src/py/libcamera/py_main.cpp\n> > > > > @@ -463,7 +463,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > > > .def_property_readonly(\"sequence\", &Request::sequence)\n> > > > > .def_property_readonly(\"has_pending_buffers\", &Request::hasPendingBuffers)\n> > > > > .def(\"set_control\", [](Request &self, const ControlId &id, py::object value) {\n> > > > > - self.controls().set(id.id(), pyToControlValue(value, id.type()));\n> > > > > + self.setControl(id.id(), pyToControlValue(value, id.type()));\n> > > > > })\n> > > > > .def_property_readonly(\"metadata\", [](Request &self) {\n> > > > > /* Convert ControlList to std container */\n> > > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > > > index 94d138cd5710b9bd5c83cacd3cbd963f71aeb3c7..7e918cc3e803cdad67043f1dbdc212d5b57e37f3 100644\n> > > > > --- a/src/v4l2/v4l2_camera.cpp\n> > > > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > > > @@ -269,7 +269,7 @@ int V4L2Camera::qbuf(unsigned int index)\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > - request->controls().merge(std::move(controls_));\n> > > > > + request->setControls(controls_);\n> > > > >\n> > > > > ret = camera_->queueRequest(request);\n> > > > > if (ret < 0) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EC55BD80A\n\tfor ;\n\tTue, 2 Dec 2025 13:50:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF54760D0C;\n\tTue, 2 Dec 2025 14:50:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B3F16096B\n\tfor ;\n\tTue, 2 Dec 2025 14:50:10 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90F67110;\n\tTue, 2 Dec 2025 14:47:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Kq+iV3u2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764683275;\n\tbh=EsR5tPihxiKAPOiiRAgIl7nj8gjLBfHvK7cTte3GqRA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Kq+iV3u2vvrc0QRwuojrY37NzSpWXDDv4tZM1VYoGGfnWFXUaxQsXgg2S27EPu9B5\n\t6cc6uv5q9JmEDaPTRCOqJd9P8hgyJAQ+eeL4gSzBriCnZFz48vEG31opCHIhszIoTM\n\tfvUHXzemqk1CO02TXCVym7Qqrtoh6piqA/Ncde7Y=","Date":"Tue, 2 Dec 2025 14:50:05 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Paul Elder , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/3] libcamera: request: Remove write-access to\n\tcontrols_","Message-ID":"","References":"<20251126-cam-control-override-v2-0-305024a1f190@ideasonboard.com>\n\t<20251126-cam-control-override-v2-3-305024a1f190@ideasonboard.com>\n\t<176458461249.3882822.6100690386193369973@neptunite.rasen.tech>\n\t<20251201150154.GA29230@pendragon.ideasonboard.com>\n\t<20251201152556.GB29230@pendragon.ideasonboard.com>\n\t<20251202021927.GE32430@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251202021927.GE32430@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]