[{"id":34007,"web_url":"https://patchwork.libcamera.org/comment/34007/","msgid":"<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>","date":"2025-04-23T08:33:48","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n> When retrieving the value from a `ControlValue` usually one of two\n> things happen: a small, trivially copyable object is returned by\n> value; or a view into the internal buffer is provided. This is true\n> for everything except strings, which are returned in `std::string`,\n> incurring the overhead of string construction.\n> \n> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n> pointing to the internal buffer to return the value. This is similar\n> to how other array-like types are returned with a `Span<>`.\n> \n> This is an API break, but its scope is limited to just `properties::Model`.\n\nThis breaks in CI at the build-history stage:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n\nI wonder, are we missing a dependency declaration somewhere that should\nhave otherwise caused something to get rebuilt? All of the other builds\nseemed to have worked on that pipeline ?\n\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/control_ids.h.in | 1 +\n> include/libcamera/controls.h | 15 ++++++++-------\n> src/apps/cam/capture_script.cpp | 2 +-\n> src/apps/cam/main.cpp | 2 +-\n> src/apps/common/dng_writer.cpp | 2 +-\n> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n> src/py/libcamera/py_helpers.cpp | 4 ++--\n> test/controls/control_value.cpp | 4 ++--\n> utils/codegen/controls.py | 2 +-\n> 9 files changed, 20 insertions(+), 18 deletions(-)\n> \n> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> index 5d0594c68..41997f79f 100644\n> --- a/include/libcamera/control_ids.h.in\n> +++ b/include/libcamera/control_ids.h.in\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 4bfe9615c..275f45200 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -96,7 +97,7 @@ struct control_type {\n> };\n> \n> template<>\n> -struct control_type {\n> +struct control_type {\n> static constexpr ControlType value = ControlTypeString;\n> static constexpr std::size_t size = 0;\n> };\n> @@ -138,7 +139,7 @@ public:\n> #ifndef __DOXYGEN__\n> template::value &&\n> details::control_type::value &&\n> - !std::is_same>::value,\n> + !std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> ControlValue(const T &value)\n> : type_(ControlTypeNone), numElements_(0)\n> @@ -148,7 +149,7 @@ public:\n> }\n> \n> template::value ||\n> - std::is_same>::value,\n> + std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> #else\n> template\n> @@ -182,7 +183,7 @@ public:\n> \n> #ifndef __DOXYGEN__\n> template::value &&\n> - !std::is_same>::value,\n> + !std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> T get() const\n> {\n> @@ -193,7 +194,7 @@ public:\n> }\n> \n> template::value ||\n> - std::is_same>::value,\n> + std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> #else\n> template\n> @@ -210,7 +211,7 @@ public:\n> \n> #ifndef __DOXYGEN__\n> template::value &&\n> - !std::is_same>::value,\n> + !std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> void set(const T &value)\n> {\n> @@ -219,7 +220,7 @@ public:\n> }\n> \n> template::value ||\n> - std::is_same>::value,\n> + std::is_same>::value,\n> std::nullptr_t> = nullptr>\n> #else\n> template\n> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> index fdf82efc0..bc4c7c3a0 100644\n> --- a/src/apps/cam/capture_script.cpp\n> +++ b/src/apps/cam/capture_script.cpp\n> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n> break;\n> }\n> case ControlTypeString: {\n> - value.set(repr);\n> + value.set(repr);\n> break;\n> }\n> default:\n> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> index fa266eca6..157d238d7 100644\n> --- a/src/apps/cam/main.cpp\n> +++ b/src/apps/cam/main.cpp\n> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> */\n> const auto &model = props.get(properties::Model);\n> if (model)\n> - name = \"'\" + *model + \"' \";\n> + name.append(\"'\").append(*model).append(\"' \");\n> }\n> \n> name += \"(\" + camera->id() + \")\";\n> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n> index ac4619511..8d57023e1 100644\n> --- a/src/apps/common/dng_writer.cpp\n> +++ b/src/apps/common/dng_writer.cpp\n> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> \n> const auto &model = cameraProperties.get(properties::Model);\n> if (model) {\n> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> }\n> \n> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n> index 6b6d0713c..9f25ba091 100644\n> --- a/src/apps/qcam/cam_select_dialog.cpp\n> +++ b/src/apps/qcam/cam_select_dialog.cpp\n> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n> cameraLocation_->setText(\"Unknown\");\n> }\n> \n> - const auto &model = properties.get(libcamera::properties::Model)\n> - .value_or(\"Unknown\");\n> + const auto model = properties.get(libcamera::properties::Model)\n> + .value_or(\"Unknown\");\n> \n> - cameraModel_->setText(QString::fromStdString(model));\n> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n> }\n> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n> index 1ad1d4c1a..8c55ef845 100644\n> --- a/src/py/libcamera/py_helpers.cpp\n> +++ b/src/py/libcamera/py_helpers.cpp\n> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n> case ControlTypeFloat:\n> return valueOrTuple(cv);\n> case ControlTypeString:\n> - return py::cast(cv.get());\n> + return py::cast(cv.get());\n> case ControlTypeSize: {\n> const Size *v = reinterpret_cast(cv.data().data());\n> return py::cast(v);\n> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n> case ControlTypeFloat:\n> return controlValueMaybeArray(ob);\n> case ControlTypeString:\n> - return ControlValue(ob.cast());\n> + return ControlValue(ob.cast());\n> case ControlTypeRectangle:\n> return controlValueMaybeArray(ob);\n> case ControlTypeSize:\n> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n> index 5084fd0cf..032050a77 100644\n> --- a/test/controls/control_value.cpp\n> +++ b/test/controls/control_value.cpp\n> @@ -318,7 +318,7 @@ protected:\n> /*\n> * String type.\n> */\n> - std::string string{ \"libcamera\" };\n> + std::string_view string{ \"libcamera\" };\n> value.set(string);\n> if (value.isNone() || !value.isArray() ||\n> value.type() != ControlTypeString ||\n> @@ -327,7 +327,7 @@ protected:\n> return TestFail;\n> }\n> \n> - if (value.get() != string) {\n> + if (value.get() != string) {\n> cerr << \"Control value mismatch after setting to string\" << endl;\n> return TestFail;\n> }\n> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n> index e51610481..9399727bd 100644\n> --- a/utils/codegen/controls.py\n> +++ b/utils/codegen/controls.py\n> @@ -111,7 +111,7 @@ class Control(object):\n> size = self.__data.get('size')\n> \n> if typ == 'string':\n> - return 'std::string'\n> + return 'std::string_view'\n> \n> if self.__size is None:\n> return typ\n> -- \n> 2.49.0\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 21A06C327D\n\tfor ;\n\tWed, 23 Apr 2025 08:33:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FB4D68ACD;\n\tWed, 23 Apr 2025 10:33:52 +0200 (CEST)","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 BAE5E617E5\n\tfor ;\n\tWed, 23 Apr 2025 10:33:50 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7507116A;\n\tWed, 23 Apr 2025 10:33:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VOjKwJbw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745397229;\n\tbh=2N7wd7+gZ6OiJJt7MyTT9t6pZEWetNwOOGTCdM4hoLM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=VOjKwJbw6drW72F+QeydpZ95DfgZfRDCyYzN+GrZMh/vh4KLMhMY/M20IxZwbuQtT\n\trPfjbrjkBmGY7t/mTtmXUbQho6kjSl6l3LJhnvQup3mxOX8bPYW47UIr/qdDYVqa4u\n\t6/XayznZlcwS7I3OJ5V7w92dv1pnz1Rsu4a6vGoc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 23 Apr 2025 09:33:48 +0100","Message-ID":"<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":34033,"web_url":"https://patchwork.libcamera.org/comment/34033/","msgid":"","date":"2025-04-25T08:18:28","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n>> When retrieving the value from a `ControlValue` usually one of two\n>> things happen: a small, trivially copyable object is returned by\n>> value; or a view into the internal buffer is provided. This is true\n>> for everything except strings, which are returned in `std::string`,\n>> incurring the overhead of string construction.\n>>\n>> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n>> pointing to the internal buffer to return the value. This is similar\n>> to how other array-like types are returned with a `Span<>`.\n>>\n>> This is an API break, but its scope is limited to just `properties::Model`.\n> \n> This breaks in CI at the build-history stage:\n> \n> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n> \n> I wonder, are we missing a dependency declaration somewhere that should\n> have otherwise caused something to get rebuilt? All of the other builds\n> seemed to have worked on that pipeline ?\n\nYes indeed, `controls.py` is changed, but that is not considered by\nmeson, so it does not regenerate the cpp files because neither the\ntemplate nor `gen-controls.py`, etc. has changed.\n\nI am not sure how it could be best addressed, there is `depend_files` where\n`controls.py` could be specified, but I don't think it's too convenient.\n\nOr there is the `modulefinder` thing in the standard library:\n\n >>> os.getcwd()\n '/libcamera/utils/codegen'\n >>> import modulefinder\n >>> m = modulefinder.ModuleFinder()\n >>> m.run_script(\"./gen-controls.py\")\n >>> m.modules[\"controls\"]\n Module('controls', '/libcamera/utils/codegen/controls.py')\n\nI am wondering if this might be used to generate a dependency file that ninja\ncan interpret. Although this approach is definitely more complex.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/control_ids.h.in | 1 +\n>> include/libcamera/controls.h | 15 ++++++++-------\n>> src/apps/cam/capture_script.cpp | 2 +-\n>> src/apps/cam/main.cpp | 2 +-\n>> src/apps/common/dng_writer.cpp | 2 +-\n>> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n>> src/py/libcamera/py_helpers.cpp | 4 ++--\n>> test/controls/control_value.cpp | 4 ++--\n>> utils/codegen/controls.py | 2 +-\n>> 9 files changed, 20 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n>> index 5d0594c68..41997f79f 100644\n>> --- a/include/libcamera/control_ids.h.in\n>> +++ b/include/libcamera/control_ids.h.in\n>> @@ -13,6 +13,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 4bfe9615c..275f45200 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -13,6 +13,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -96,7 +97,7 @@ struct control_type {\n>> };\n>> \n>> template<>\n>> -struct control_type {\n>> +struct control_type {\n>> static constexpr ControlType value = ControlTypeString;\n>> static constexpr std::size_t size = 0;\n>> };\n>> @@ -138,7 +139,7 @@ public:\n>> #ifndef __DOXYGEN__\n>> template::value &&\n>> details::control_type::value &&\n>> - !std::is_same>::value,\n>> + !std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> ControlValue(const T &value)\n>> : type_(ControlTypeNone), numElements_(0)\n>> @@ -148,7 +149,7 @@ public:\n>> }\n>> \n>> template::value ||\n>> - std::is_same>::value,\n>> + std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> #else\n>> template\n>> @@ -182,7 +183,7 @@ public:\n>> \n>> #ifndef __DOXYGEN__\n>> template::value &&\n>> - !std::is_same>::value,\n>> + !std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> T get() const\n>> {\n>> @@ -193,7 +194,7 @@ public:\n>> }\n>> \n>> template::value ||\n>> - std::is_same>::value,\n>> + std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> #else\n>> template\n>> @@ -210,7 +211,7 @@ public:\n>> \n>> #ifndef __DOXYGEN__\n>> template::value &&\n>> - !std::is_same>::value,\n>> + !std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> void set(const T &value)\n>> {\n>> @@ -219,7 +220,7 @@ public:\n>> }\n>> \n>> template::value ||\n>> - std::is_same>::value,\n>> + std::is_same>::value,\n>> std::nullptr_t> = nullptr>\n>> #else\n>> template\n>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n>> index fdf82efc0..bc4c7c3a0 100644\n>> --- a/src/apps/cam/capture_script.cpp\n>> +++ b/src/apps/cam/capture_script.cpp\n>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n>> break;\n>> }\n>> case ControlTypeString: {\n>> - value.set(repr);\n>> + value.set(repr);\n>> break;\n>> }\n>> default:\n>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n>> index fa266eca6..157d238d7 100644\n>> --- a/src/apps/cam/main.cpp\n>> +++ b/src/apps/cam/main.cpp\n>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>> */\n>> const auto &model = props.get(properties::Model);\n>> if (model)\n>> - name = \"'\" + *model + \"' \";\n>> + name.append(\"'\").append(*model).append(\"' \");\n>> }\n>> \n>> name += \"(\" + camera->id() + \")\";\n>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n>> index ac4619511..8d57023e1 100644\n>> --- a/src/apps/common/dng_writer.cpp\n>> +++ b/src/apps/common/dng_writer.cpp\n>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>> \n>> const auto &model = cameraProperties.get(properties::Model);\n>> if (model) {\n>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>> }\n>> \n>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n>> index 6b6d0713c..9f25ba091 100644\n>> --- a/src/apps/qcam/cam_select_dialog.cpp\n>> +++ b/src/apps/qcam/cam_select_dialog.cpp\n>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n>> cameraLocation_->setText(\"Unknown\");\n>> }\n>> \n>> - const auto &model = properties.get(libcamera::properties::Model)\n>> - .value_or(\"Unknown\");\n>> + const auto model = properties.get(libcamera::properties::Model)\n>> + .value_or(\"Unknown\");\n>> \n>> - cameraModel_->setText(QString::fromStdString(model));\n>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n>> }\n>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n>> index 1ad1d4c1a..8c55ef845 100644\n>> --- a/src/py/libcamera/py_helpers.cpp\n>> +++ b/src/py/libcamera/py_helpers.cpp\n>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n>> case ControlTypeFloat:\n>> return valueOrTuple(cv);\n>> case ControlTypeString:\n>> - return py::cast(cv.get());\n>> + return py::cast(cv.get());\n>> case ControlTypeSize: {\n>> const Size *v = reinterpret_cast(cv.data().data());\n>> return py::cast(v);\n>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n>> case ControlTypeFloat:\n>> return controlValueMaybeArray(ob);\n>> case ControlTypeString:\n>> - return ControlValue(ob.cast());\n>> + return ControlValue(ob.cast());\n>> case ControlTypeRectangle:\n>> return controlValueMaybeArray(ob);\n>> case ControlTypeSize:\n>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n>> index 5084fd0cf..032050a77 100644\n>> --- a/test/controls/control_value.cpp\n>> +++ b/test/controls/control_value.cpp\n>> @@ -318,7 +318,7 @@ protected:\n>> /*\n>> * String type.\n>> */\n>> - std::string string{ \"libcamera\" };\n>> + std::string_view string{ \"libcamera\" };\n>> value.set(string);\n>> if (value.isNone() || !value.isArray() ||\n>> value.type() != ControlTypeString ||\n>> @@ -327,7 +327,7 @@ protected:\n>> return TestFail;\n>> }\n>> \n>> - if (value.get() != string) {\n>> + if (value.get() != string) {\n>> cerr << \"Control value mismatch after setting to string\" << endl;\n>> return TestFail;\n>> }\n>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n>> index e51610481..9399727bd 100644\n>> --- a/utils/codegen/controls.py\n>> +++ b/utils/codegen/controls.py\n>> @@ -111,7 +111,7 @@ class Control(object):\n>> size = self.__data.get('size')\n>> \n>> if typ == 'string':\n>> - return 'std::string'\n>> + return 'std::string_view'\n>> \n>> if self.__size is None:\n>> return typ\n>> -- \n>> 2.49.0\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 D8645BE08B\n\tfor ;\n\tFri, 25 Apr 2025 08:18:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2542468ACD;\n\tFri, 25 Apr 2025 10:18:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B743E60526\n\tfor ;\n\tFri, 25 Apr 2025 10:18:31 +0200 (CEST)","from [192.168.33.12] (185.221.143.16.nat.pool.zt.hu\n\t[185.221.143.16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA5AB982;\n\tFri, 25 Apr 2025 10:18:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vDcGgMY7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745569109;\n\tbh=15mVUwSeh0z8GCMe6/+8+xrwP3WzUf2hPOQ0L1AQDmI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=vDcGgMY7Zn1oUbVAmwzgRzNg6nlSwctDKVTLCFXG0yz6abw1M72bBxMzOccVJZoRh\n\tWbC+A1PSZkd0GqzfF5ldIX+bOAfIMqCnIXrQAFyB1oZ3eY4GOB0MRM6nvio2CyJjwK\n\tlrk8EHAQaLqfm8ryUCFE0IDGyMAPUPcBbAamLEp8=","Message-ID":"","Date":"Fri, 25 Apr 2025 10:18:28 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>\n\t<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34088,"web_url":"https://patchwork.libcamera.org/comment/34088/","msgid":"<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>","date":"2025-05-01T10:16:46","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-04-25 09:18:28)\n> Hi\n> \n> \n> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n> >> When retrieving the value from a `ControlValue` usually one of two\n> >> things happen: a small, trivially copyable object is returned by\n> >> value; or a view into the internal buffer is provided. This is true\n> >> for everything except strings, which are returned in `std::string`,\n> >> incurring the overhead of string construction.\n> >>\n> >> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n> >> pointing to the internal buffer to return the value. This is similar\n> >> to how other array-like types are returned with a `Span<>`.\n> >>\n> >> This is an API break, but its scope is limited to just `properties::Model`.\n> > \n> > This breaks in CI at the build-history stage:\n> > \n> > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n> > \n> > I wonder, are we missing a dependency declaration somewhere that should\n> > have otherwise caused something to get rebuilt? All of the other builds\n> > seemed to have worked on that pipeline ?\n> \n> Yes indeed, `controls.py` is changed, but that is not considered by\n> meson, so it does not regenerate the cpp files because neither the\n> template nor `gen-controls.py`, etc. has changed.\n\nYikes. Probably something to bring up in the meson channels for support.\nI have no idea on a (generic/longer term) solution I'm afraid.\n\n> I am not sure how it could be best addressed, there is `depend_files` where\n> `controls.py` could be specified, but I don't think it's too convenient.\n> \n> Or there is the `modulefinder` thing in the standard library:\n> \n> >>> os.getcwd()\n> '/libcamera/utils/codegen'\n> >>> import modulefinder\n> >>> m = modulefinder.ModuleFinder()\n> >>> m.run_script(\"./gen-controls.py\")\n> >>> m.modules[\"controls\"]\n> Module('controls', '/libcamera/utils/codegen/controls.py')\n> \n> I am wondering if this might be used to generate a dependency file that ninja\n> can interpret. Although this approach is definitely more complex.\n\nYes, I can see that could get ugly - and we're not supposed to have to\nparse the dependencies of every tool we use. Ok - so I guess this one is\ndifferent because it's our own in built tooling ... \n\nI think we could :\n - Contact meson developers for support\n - Update this patch to 'also' (trivially) modify either the template or\n gen-controls.py\n - Ignore the build-history failure and just keep it as is..\n\nI think if we want to land this series - at worst I'm ok with the last\noption - but I'd rather keep it as a last resort to make sure we\nmaintain bisectability. And for that - I think even an arbitrary comment\nupdate to the gen-controls.py which could be later removed would be fine\nif it came to it.\n\n\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> >>\n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> include/libcamera/control_ids.h.in | 1 +\n> >> include/libcamera/controls.h | 15 ++++++++-------\n> >> src/apps/cam/capture_script.cpp | 2 +-\n> >> src/apps/cam/main.cpp | 2 +-\n> >> src/apps/common/dng_writer.cpp | 2 +-\n> >> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n> >> src/py/libcamera/py_helpers.cpp | 4 ++--\n> >> test/controls/control_value.cpp | 4 ++--\n> >> utils/codegen/controls.py | 2 +-\n> >> 9 files changed, 20 insertions(+), 18 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> >> index 5d0594c68..41997f79f 100644\n> >> --- a/include/libcamera/control_ids.h.in\n> >> +++ b/include/libcamera/control_ids.h.in\n> >> @@ -13,6 +13,7 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> \n> >> #include \n> >> \n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> index 4bfe9615c..275f45200 100644\n> >> --- a/include/libcamera/controls.h\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -13,6 +13,7 @@\n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -96,7 +97,7 @@ struct control_type {\n> >> };\n> >> \n> >> template<>\n> >> -struct control_type {\n> >> +struct control_type {\n> >> static constexpr ControlType value = ControlTypeString;\n> >> static constexpr std::size_t size = 0;\n> >> };\n> >> @@ -138,7 +139,7 @@ public:\n> >> #ifndef __DOXYGEN__\n> >> template::value &&\n> >> details::control_type::value &&\n> >> - !std::is_same>::value,\n> >> + !std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> ControlValue(const T &value)\n> >> : type_(ControlTypeNone), numElements_(0)\n> >> @@ -148,7 +149,7 @@ public:\n> >> }\n> >> \n> >> template::value ||\n> >> - std::is_same>::value,\n> >> + std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> #else\n> >> template\n> >> @@ -182,7 +183,7 @@ public:\n> >> \n> >> #ifndef __DOXYGEN__\n> >> template::value &&\n> >> - !std::is_same>::value,\n> >> + !std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> T get() const\n> >> {\n> >> @@ -193,7 +194,7 @@ public:\n> >> }\n> >> \n> >> template::value ||\n> >> - std::is_same>::value,\n> >> + std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> #else\n> >> template\n> >> @@ -210,7 +211,7 @@ public:\n> >> \n> >> #ifndef __DOXYGEN__\n> >> template::value &&\n> >> - !std::is_same>::value,\n> >> + !std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> void set(const T &value)\n> >> {\n> >> @@ -219,7 +220,7 @@ public:\n> >> }\n> >> \n> >> template::value ||\n> >> - std::is_same>::value,\n> >> + std::is_same>::value,\n> >> std::nullptr_t> = nullptr>\n> >> #else\n> >> template\n> >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> >> index fdf82efc0..bc4c7c3a0 100644\n> >> --- a/src/apps/cam/capture_script.cpp\n> >> +++ b/src/apps/cam/capture_script.cpp\n> >> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n> >> break;\n> >> }\n> >> case ControlTypeString: {\n> >> - value.set(repr);\n> >> + value.set(repr);\n> >> break;\n> >> }\n> >> default:\n> >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> >> index fa266eca6..157d238d7 100644\n> >> --- a/src/apps/cam/main.cpp\n> >> +++ b/src/apps/cam/main.cpp\n> >> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >> */\n> >> const auto &model = props.get(properties::Model);\n> >> if (model)\n> >> - name = \"'\" + *model + \"' \";\n> >> + name.append(\"'\").append(*model).append(\"' \");\n\nAha, ok so this is the API breakage ... So we'll lose the ability to\nflexibly represent strings ?\n\nDoes this append have to construct a string ? (i.e. are we going to end\nup just 'moving' the new construction?\n\nAny help/benefit from using include/libcamera/base/utils.h\nlibcamera::join();? Ah - no that would only add separateors - what we\nwould need as a string 'wrap' to allow wrapping the model with '\ncharacters.\n\nThere are other occasions where I've thought about adding a wrap ..\nwrapper function but it's probably overkill.\n\n\n\n> >> }\n> >> \n> >> name += \"(\" + camera->id() + \")\";\n> >> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n> >> index ac4619511..8d57023e1 100644\n> >> --- a/src/apps/common/dng_writer.cpp\n> >> +++ b/src/apps/common/dng_writer.cpp\n> >> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >> \n> >> const auto &model = cameraProperties.get(properties::Model);\n> >> if (model) {\n> >> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n> >> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n> >> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n\nThis looks like a new construction ... does this make a copy ?\n\n\nAs far as I can tell - this is functionally correct/valid - and all the\nbuild tests (except the build-history) work - so I'd put :\n\n\nReviewed-by: Kieran Bingham \n\nCurious to see if this is something that provides enough 'benefit' over\nchanging how string controls are handled vs potential copies.\n\nDo we ever expect strings to be expensive?\n\nI could actually imagine some use cases where we could start making\ninternal/debug controls that might report larger strings in fact ... so\nit might actaully be helpful there!\n\n\n\n> >> }\n> >> \n> >> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n> >> index 6b6d0713c..9f25ba091 100644\n> >> --- a/src/apps/qcam/cam_select_dialog.cpp\n> >> +++ b/src/apps/qcam/cam_select_dialog.cpp\n> >> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n> >> cameraLocation_->setText(\"Unknown\");\n> >> }\n> >> \n> >> - const auto &model = properties.get(libcamera::properties::Model)\n> >> - .value_or(\"Unknown\");\n> >> + const auto model = properties.get(libcamera::properties::Model)\n> >> + .value_or(\"Unknown\");\n> >> \n> >> - cameraModel_->setText(QString::fromStdString(model));\n> >> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n> >> }\n> >> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n> >> index 1ad1d4c1a..8c55ef845 100644\n> >> --- a/src/py/libcamera/py_helpers.cpp\n> >> +++ b/src/py/libcamera/py_helpers.cpp\n> >> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n> >> case ControlTypeFloat:\n> >> return valueOrTuple(cv);\n> >> case ControlTypeString:\n> >> - return py::cast(cv.get());\n> >> + return py::cast(cv.get());\n> >> case ControlTypeSize: {\n> >> const Size *v = reinterpret_cast(cv.data().data());\n> >> return py::cast(v);\n> >> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n> >> case ControlTypeFloat:\n> >> return controlValueMaybeArray(ob);\n> >> case ControlTypeString:\n> >> - return ControlValue(ob.cast());\n> >> + return ControlValue(ob.cast());\n> >> case ControlTypeRectangle:\n> >> return controlValueMaybeArray(ob);\n> >> case ControlTypeSize:\n> >> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n> >> index 5084fd0cf..032050a77 100644\n> >> --- a/test/controls/control_value.cpp\n> >> +++ b/test/controls/control_value.cpp\n> >> @@ -318,7 +318,7 @@ protected:\n> >> /*\n> >> * String type.\n> >> */\n> >> - std::string string{ \"libcamera\" };\n> >> + std::string_view string{ \"libcamera\" };\n> >> value.set(string);\n> >> if (value.isNone() || !value.isArray() ||\n> >> value.type() != ControlTypeString ||\n> >> @@ -327,7 +327,7 @@ protected:\n> >> return TestFail;\n> >> }\n> >> \n> >> - if (value.get() != string) {\n> >> + if (value.get() != string) {\n> >> cerr << \"Control value mismatch after setting to string\" << endl;\n> >> return TestFail;\n> >> }\n> >> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n> >> index e51610481..9399727bd 100644\n> >> --- a/utils/codegen/controls.py\n> >> +++ b/utils/codegen/controls.py\n> >> @@ -111,7 +111,7 @@ class Control(object):\n> >> size = self.__data.get('size')\n> >> \n> >> if typ == 'string':\n> >> - return 'std::string'\n> >> + return 'std::string_view'\n> >> \n> >> if self.__size is None:\n> >> return typ\n> >> -- \n> >> 2.49.0\n> >>\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 DED0BC327D\n\tfor ;\n\tThu, 1 May 2025 10:16:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 061F468AD9;\n\tThu, 1 May 2025 12:16:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBF97617D7\n\tfor ;\n\tThu, 1 May 2025 12:16:48 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 94E8963D;\n\tThu, 1 May 2025 12:16:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HcPdqI3h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746094601;\n\tbh=AbnHOGyt+6C9V87wZ76FtppUfjh4Y/V2qbCATXpkkAM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=HcPdqI3hrR9w97QtMEmvy4GRNb6uPC1p5zemFwsf9O/c5iC8aGEKNgJozvsUZgilu\n\tgbFbe4K2oHOfXuYWdbusITJES2QQ6a4t6dw+c/41FW6Ft9ayJp4utOFEMHfho4DXQr\n\tY6Dxm9pQmpDUeYSKZX/0CcdE7Nmf3G2x1RntqhCM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>\n\t<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 01 May 2025 11:16:46 +0100","Message-ID":"<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":34089,"web_url":"https://patchwork.libcamera.org/comment/34089/","msgid":"<6b940091-30ff-49c9-ad67-b647cecd614f@ideasonboard.com>","date":"2025-05-01T10:25:12","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-04-25 09:18:28)\n>> Hi\n>>\n>>\n>> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n>>>> When retrieving the value from a `ControlValue` usually one of two\n>>>> things happen: a small, trivially copyable object is returned by\n>>>> value; or a view into the internal buffer is provided. This is true\n>>>> for everything except strings, which are returned in `std::string`,\n>>>> incurring the overhead of string construction.\n>>>>\n>>>> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n>>>> pointing to the internal buffer to return the value. This is similar\n>>>> to how other array-like types are returned with a `Span<>`.\n>>>>\n>>>> This is an API break, but its scope is limited to just `properties::Model`.\n>>>\n>>> This breaks in CI at the build-history stage:\n>>>\n>>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n>>>\n>>> I wonder, are we missing a dependency declaration somewhere that should\n>>> have otherwise caused something to get rebuilt? All of the other builds\n>>> seemed to have worked on that pipeline ?\n>>\n>> Yes indeed, `controls.py` is changed, but that is not considered by\n>> meson, so it does not regenerate the cpp files because neither the\n>> template nor `gen-controls.py`, etc. has changed.\n> \n> Yikes. Probably something to bring up in the meson channels for support.\n> I have no idea on a (generic/longer term) solution I'm afraid.\n> \n>> I am not sure how it could be best addressed, there is `depend_files` where\n>> `controls.py` could be specified, but I don't think it's too convenient.\n>>\n>> Or there is the `modulefinder` thing in the standard library:\n>>\n>> >>> os.getcwd()\n>> '/libcamera/utils/codegen'\n>> >>> import modulefinder\n>> >>> m = modulefinder.ModuleFinder()\n>> >>> m.run_script(\"./gen-controls.py\")\n>> >>> m.modules[\"controls\"]\n>> Module('controls', '/libcamera/utils/codegen/controls.py')\n>>\n>> I am wondering if this might be used to generate a dependency file that ninja\n>> can interpret. Although this approach is definitely more complex.\n> \n> Yes, I can see that could get ugly - and we're not supposed to have to\n> parse the dependencies of every tool we use. Ok - so I guess this one is\n> different because it's our own in built tooling ...\n> \n> I think we could :\n> - Contact meson developers for support\n> - Update this patch to 'also' (trivially) modify either the template or\n> gen-controls.py\n> - Ignore the build-history failure and just keep it as is..\n> \n> I think if we want to land this series - at worst I'm ok with the last\n> option - but I'd rather keep it as a last resort to make sure we\n> maintain bisectability. And for that - I think even an arbitrary comment\n> update to the gen-controls.py which could be later removed would be fine\n> if it came to it.\n\nv2 addresses this issue by adding `depend_files` to each `custom_target()`.\nNot the best solution, but I couldn't find anything better.\n\n\n> \n> \n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>>\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> include/libcamera/control_ids.h.in | 1 +\n>>>> include/libcamera/controls.h | 15 ++++++++-------\n>>>> src/apps/cam/capture_script.cpp | 2 +-\n>>>> src/apps/cam/main.cpp | 2 +-\n>>>> src/apps/common/dng_writer.cpp | 2 +-\n>>>> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n>>>> src/py/libcamera/py_helpers.cpp | 4 ++--\n>>>> test/controls/control_value.cpp | 4 ++--\n>>>> utils/codegen/controls.py | 2 +-\n>>>> 9 files changed, 20 insertions(+), 18 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n>>>> index 5d0594c68..41997f79f 100644\n>>>> --- a/include/libcamera/control_ids.h.in\n>>>> +++ b/include/libcamera/control_ids.h.in\n>>>> @@ -13,6 +13,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>>\n>>>> #include \n>>>>\n>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>> index 4bfe9615c..275f45200 100644\n>>>> --- a/include/libcamera/controls.h\n>>>> +++ b/include/libcamera/controls.h\n>>>> @@ -13,6 +13,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>>\n>>>> @@ -96,7 +97,7 @@ struct control_type {\n>>>> };\n>>>>\n>>>> template<>\n>>>> -struct control_type {\n>>>> +struct control_type {\n>>>> static constexpr ControlType value = ControlTypeString;\n>>>> static constexpr std::size_t size = 0;\n>>>> };\n>>>> @@ -138,7 +139,7 @@ public:\n>>>> #ifndef __DOXYGEN__\n>>>> template::value &&\n>>>> details::control_type::value &&\n>>>> - !std::is_same>::value,\n>>>> + !std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> ControlValue(const T &value)\n>>>> : type_(ControlTypeNone), numElements_(0)\n>>>> @@ -148,7 +149,7 @@ public:\n>>>> }\n>>>>\n>>>> template::value ||\n>>>> - std::is_same>::value,\n>>>> + std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> #else\n>>>> template\n>>>> @@ -182,7 +183,7 @@ public:\n>>>>\n>>>> #ifndef __DOXYGEN__\n>>>> template::value &&\n>>>> - !std::is_same>::value,\n>>>> + !std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> T get() const\n>>>> {\n>>>> @@ -193,7 +194,7 @@ public:\n>>>> }\n>>>>\n>>>> template::value ||\n>>>> - std::is_same>::value,\n>>>> + std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> #else\n>>>> template\n>>>> @@ -210,7 +211,7 @@ public:\n>>>>\n>>>> #ifndef __DOXYGEN__\n>>>> template::value &&\n>>>> - !std::is_same>::value,\n>>>> + !std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> void set(const T &value)\n>>>> {\n>>>> @@ -219,7 +220,7 @@ public:\n>>>> }\n>>>>\n>>>> template::value ||\n>>>> - std::is_same>::value,\n>>>> + std::is_same>::value,\n>>>> std::nullptr_t> = nullptr>\n>>>> #else\n>>>> template\n>>>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n>>>> index fdf82efc0..bc4c7c3a0 100644\n>>>> --- a/src/apps/cam/capture_script.cpp\n>>>> +++ b/src/apps/cam/capture_script.cpp\n>>>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n>>>> break;\n>>>> }\n>>>> case ControlTypeString: {\n>>>> - value.set(repr);\n>>>> + value.set(repr);\n>>>> break;\n>>>> }\n>>>> default:\n>>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n>>>> index fa266eca6..157d238d7 100644\n>>>> --- a/src/apps/cam/main.cpp\n>>>> +++ b/src/apps/cam/main.cpp\n>>>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>>> */\n>>>> const auto &model = props.get(properties::Model);\n>>>> if (model)\n>>>> - name = \"'\" + *model + \"' \";\n>>>> + name.append(\"'\").append(*model).append(\"' \");\n> \n> Aha, ok so this is the API breakage ... So we'll lose the ability to\n> flexibly represent strings ?\n> \n> Does this append have to construct a string ? (i.e. are we going to end\n> up just 'moving' the new construction?\n> \n> Any help/benefit from using include/libcamera/base/utils.h\n> libcamera::join();? Ah - no that would only add separateors - what we\n> would need as a string 'wrap' to allow wrapping the model with '\n> characters.\n> \n> There are other occasions where I've thought about adding a wrap ..\n> wrapper function but it's probably overkill.\n> \n> \n> \n>>>> }\n>>>>\n>>>> name += \"(\" + camera->id() + \")\";\n>>>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n>>>> index ac4619511..8d57023e1 100644\n>>>> --- a/src/apps/common/dng_writer.cpp\n>>>> +++ b/src/apps/common/dng_writer.cpp\n>>>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>>>\n>>>> const auto &model = cameraProperties.get(properties::Model);\n>>>> if (model) {\n>>>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n>>>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n>>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> \n> This looks like a new construction ... does this make a copy ?\n> \n> \n> As far as I can tell - this is functionally correct/valid - and all the\n> build tests (except the build-history) work - so I'd put :\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> Curious to see if this is something that provides enough 'benefit' over\n> changing how string controls are handled vs potential copies.\n> \n> Do we ever expect strings to be expensive?\n> \n> I could actually imagine some use cases where we could start making\n> internal/debug controls that might report larger strings in fact ... so\n> it might actaully be helpful there!\n> \n> \n> \n>>>> }\n>>>>\n>>>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n>>>> index 6b6d0713c..9f25ba091 100644\n>>>> --- a/src/apps/qcam/cam_select_dialog.cpp\n>>>> +++ b/src/apps/qcam/cam_select_dialog.cpp\n>>>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n>>>> cameraLocation_->setText(\"Unknown\");\n>>>> }\n>>>>\n>>>> - const auto &model = properties.get(libcamera::properties::Model)\n>>>> - .value_or(\"Unknown\");\n>>>> + const auto model = properties.get(libcamera::properties::Model)\n>>>> + .value_or(\"Unknown\");\n>>>>\n>>>> - cameraModel_->setText(QString::fromStdString(model));\n>>>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n>>>> }\n>>>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n>>>> index 1ad1d4c1a..8c55ef845 100644\n>>>> --- a/src/py/libcamera/py_helpers.cpp\n>>>> +++ b/src/py/libcamera/py_helpers.cpp\n>>>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n>>>> case ControlTypeFloat:\n>>>> return valueOrTuple(cv);\n>>>> case ControlTypeString:\n>>>> - return py::cast(cv.get());\n>>>> + return py::cast(cv.get());\n>>>> case ControlTypeSize: {\n>>>> const Size *v = reinterpret_cast(cv.data().data());\n>>>> return py::cast(v);\n>>>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n>>>> case ControlTypeFloat:\n>>>> return controlValueMaybeArray(ob);\n>>>> case ControlTypeString:\n>>>> - return ControlValue(ob.cast());\n>>>> + return ControlValue(ob.cast());\n>>>> case ControlTypeRectangle:\n>>>> return controlValueMaybeArray(ob);\n>>>> case ControlTypeSize:\n>>>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n>>>> index 5084fd0cf..032050a77 100644\n>>>> --- a/test/controls/control_value.cpp\n>>>> +++ b/test/controls/control_value.cpp\n>>>> @@ -318,7 +318,7 @@ protected:\n>>>> /*\n>>>> * String type.\n>>>> */\n>>>> - std::string string{ \"libcamera\" };\n>>>> + std::string_view string{ \"libcamera\" };\n>>>> value.set(string);\n>>>> if (value.isNone() || !value.isArray() ||\n>>>> value.type() != ControlTypeString ||\n>>>> @@ -327,7 +327,7 @@ protected:\n>>>> return TestFail;\n>>>> }\n>>>>\n>>>> - if (value.get() != string) {\n>>>> + if (value.get() != string) {\n>>>> cerr << \"Control value mismatch after setting to string\" << endl;\n>>>> return TestFail;\n>>>> }\n>>>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n>>>> index e51610481..9399727bd 100644\n>>>> --- a/utils/codegen/controls.py\n>>>> +++ b/utils/codegen/controls.py\n>>>> @@ -111,7 +111,7 @@ class Control(object):\n>>>> size = self.__data.get('size')\n>>>>\n>>>> if typ == 'string':\n>>>> - return 'std::string'\n>>>> + return 'std::string_view'\n>>>>\n>>>> if self.__size is None:\n>>>> return typ\n>>>> --\n>>>> 2.49.0\n>>>>\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 EFB56BE08B\n\tfor ;\n\tThu, 1 May 2025 10:25:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C84C68AD5;\n\tThu, 1 May 2025 12:25:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C84C4617D7\n\tfor ;\n\tThu, 1 May 2025 12:25:16 +0200 (CEST)","from [192.168.33.21] (185.221.143.50.nat.pool.zt.hu\n\t[185.221.143.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F15B82A\n\tfor ;\n\tThu, 1 May 2025 12:25:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LQkDIE0b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746095109;\n\tbh=cUW1+NUHJnNpnkndMEQX3jkKkC2euOm/GKZlmMVhV7I=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=LQkDIE0bRRe27YgQXqgnOoyDmrOvDyzL4vZFJd27uHHlHxzASEC7val0NCRkdd/8I\n\tWykXR1EnkySY9LnQ+xWuvFZCEdjRZDXzyr+vKTuiki8wd+L/G+w7BhVekBRbMtm8DW\n\t7Mfg6Od/A5k3MctLSxK4BBVEDzB7cmC6w4frEG4w=","Message-ID":"<6b940091-30ff-49c9-ad67-b647cecd614f@ideasonboard.com>","Date":"Thu, 1 May 2025 12:25:12 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","To":"libcamera-devel@lists.libcamera.org","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>\n\t<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>\n\t\n\t\n\t<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34092,"web_url":"https://patchwork.libcamera.org/comment/34092/","msgid":"<174609995358.2882969.4307860543156159477@ping.linuxembedded.co.uk>","date":"2025-05-01T11:45:53","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-05-01 11:25:12)\n> 2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-04-25 09:18:28)\n> >> Hi\n> >>\n> >>\n> >> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta:\n> >>> Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n> >>>> When retrieving the value from a `ControlValue` usually one of two\n> >>>> things happen: a small, trivially copyable object is returned by\n> >>>> value; or a view into the internal buffer is provided. This is true\n> >>>> for everything except strings, which are returned in `std::string`,\n> >>>> incurring the overhead of string construction.\n> >>>>\n> >>>> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n> >>>> pointing to the internal buffer to return the value. This is similar\n> >>>> to how other array-like types are returned with a `Span<>`.\n> >>>>\n> >>>> This is an API break, but its scope is limited to just `properties::Model`.\n> >>>\n> >>> This breaks in CI at the build-history stage:\n> >>>\n> >>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n> >>>\n> >>> I wonder, are we missing a dependency declaration somewhere that should\n> >>> have otherwise caused something to get rebuilt? All of the other builds\n> >>> seemed to have worked on that pipeline ?\n> >>\n> >> Yes indeed, `controls.py` is changed, but that is not considered by\n> >> meson, so it does not regenerate the cpp files because neither the\n> >> template nor `gen-controls.py`, etc. has changed.\n> > \n> > Yikes. Probably something to bring up in the meson channels for support.\n> > I have no idea on a (generic/longer term) solution I'm afraid.\n> > \n> >> I am not sure how it could be best addressed, there is `depend_files` where\n> >> `controls.py` could be specified, but I don't think it's too convenient.\n> >>\n> >> Or there is the `modulefinder` thing in the standard library:\n> >>\n> >> >>> os.getcwd()\n> >> '/libcamera/utils/codegen'\n> >> >>> import modulefinder\n> >> >>> m = modulefinder.ModuleFinder()\n> >> >>> m.run_script(\"./gen-controls.py\")\n> >> >>> m.modules[\"controls\"]\n> >> Module('controls', '/libcamera/utils/codegen/controls.py')\n> >>\n> >> I am wondering if this might be used to generate a dependency file that ninja\n> >> can interpret. Although this approach is definitely more complex.\n> > \n> > Yes, I can see that could get ugly - and we're not supposed to have to\n> > parse the dependencies of every tool we use. Ok - so I guess this one is\n> > different because it's our own in built tooling ...\n> > \n> > I think we could :\n> > - Contact meson developers for support\n> > - Update this patch to 'also' (trivially) modify either the template or\n> > gen-controls.py\n> > - Ignore the build-history failure and just keep it as is..\n> > \n> > I think if we want to land this series - at worst I'm ok with the last\n> > option - but I'd rather keep it as a last resort to make sure we\n> > maintain bisectability. And for that - I think even an arbitrary comment\n> > update to the gen-controls.py which could be later removed would be fine\n> > if it came to it.\n> \n> v2 addresses this issue by adding `depend_files` to each `custom_target()`.\n> Not the best solution, but I couldn't find anything better.\n\nExcellent, that's fine with me.\n\nThere were a few other statements/questions below here... any comment? -\nI won't bother asking them on the new version ... you can keep/add my\ntag to the new version.\n\n\n> \n> \n> > \n> > \n> > \n> >>\n> >>\n> >> Regards,\n> >> Barnabás Pőcze\n> >>\n> >>>\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> include/libcamera/control_ids.h.in | 1 +\n> >>>> include/libcamera/controls.h | 15 ++++++++-------\n> >>>> src/apps/cam/capture_script.cpp | 2 +-\n> >>>> src/apps/cam/main.cpp | 2 +-\n> >>>> src/apps/common/dng_writer.cpp | 2 +-\n> >>>> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n> >>>> src/py/libcamera/py_helpers.cpp | 4 ++--\n> >>>> test/controls/control_value.cpp | 4 ++--\n> >>>> utils/codegen/controls.py | 2 +-\n> >>>> 9 files changed, 20 insertions(+), 18 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> >>>> index 5d0594c68..41997f79f 100644\n> >>>> --- a/include/libcamera/control_ids.h.in\n> >>>> +++ b/include/libcamera/control_ids.h.in\n> >>>> @@ -13,6 +13,7 @@\n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> +#include \n> >>>>\n> >>>> #include \n> >>>>\n> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>> index 4bfe9615c..275f45200 100644\n> >>>> --- a/include/libcamera/controls.h\n> >>>> +++ b/include/libcamera/controls.h\n> >>>> @@ -13,6 +13,7 @@\n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> +#include \n> >>>> #include \n> >>>> #include \n> >>>>\n> >>>> @@ -96,7 +97,7 @@ struct control_type {\n> >>>> };\n> >>>>\n> >>>> template<>\n> >>>> -struct control_type {\n> >>>> +struct control_type {\n> >>>> static constexpr ControlType value = ControlTypeString;\n> >>>> static constexpr std::size_t size = 0;\n> >>>> };\n> >>>> @@ -138,7 +139,7 @@ public:\n> >>>> #ifndef __DOXYGEN__\n> >>>> template::value &&\n> >>>> details::control_type::value &&\n> >>>> - !std::is_same>::value,\n> >>>> + !std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> ControlValue(const T &value)\n> >>>> : type_(ControlTypeNone), numElements_(0)\n> >>>> @@ -148,7 +149,7 @@ public:\n> >>>> }\n> >>>>\n> >>>> template::value ||\n> >>>> - std::is_same>::value,\n> >>>> + std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> #else\n> >>>> template\n> >>>> @@ -182,7 +183,7 @@ public:\n> >>>>\n> >>>> #ifndef __DOXYGEN__\n> >>>> template::value &&\n> >>>> - !std::is_same>::value,\n> >>>> + !std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> T get() const\n> >>>> {\n> >>>> @@ -193,7 +194,7 @@ public:\n> >>>> }\n> >>>>\n> >>>> template::value ||\n> >>>> - std::is_same>::value,\n> >>>> + std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> #else\n> >>>> template\n> >>>> @@ -210,7 +211,7 @@ public:\n> >>>>\n> >>>> #ifndef __DOXYGEN__\n> >>>> template::value &&\n> >>>> - !std::is_same>::value,\n> >>>> + !std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> void set(const T &value)\n> >>>> {\n> >>>> @@ -219,7 +220,7 @@ public:\n> >>>> }\n> >>>>\n> >>>> template::value ||\n> >>>> - std::is_same>::value,\n> >>>> + std::is_same>::value,\n> >>>> std::nullptr_t> = nullptr>\n> >>>> #else\n> >>>> template\n> >>>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> >>>> index fdf82efc0..bc4c7c3a0 100644\n> >>>> --- a/src/apps/cam/capture_script.cpp\n> >>>> +++ b/src/apps/cam/capture_script.cpp\n> >>>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n> >>>> break;\n> >>>> }\n> >>>> case ControlTypeString: {\n> >>>> - value.set(repr);\n> >>>> + value.set(repr);\n> >>>> break;\n> >>>> }\n> >>>> default:\n> >>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> >>>> index fa266eca6..157d238d7 100644\n> >>>> --- a/src/apps/cam/main.cpp\n> >>>> +++ b/src/apps/cam/main.cpp\n> >>>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n> >>>> */\n> >>>> const auto &model = props.get(properties::Model);\n> >>>> if (model)\n> >>>> - name = \"'\" + *model + \"' \";\n> >>>> + name.append(\"'\").append(*model).append(\"' \");\n> > \n> > Aha, ok so this is the API breakage ... So we'll lose the ability to\n> > flexibly represent strings ?\n> > \n> > Does this append have to construct a string ? (i.e. are we going to end\n> > up just 'moving' the new construction?\n> > \n> > Any help/benefit from using include/libcamera/base/utils.h\n> > libcamera::join();? Ah - no that would only add separateors - what we\n> > would need as a string 'wrap' to allow wrapping the model with '\n> > characters.\n> > \n> > There are other occasions where I've thought about adding a wrap ..\n> > wrapper function but it's probably overkill.\n> > \n> > \n> > \n> >>>> }\n> >>>>\n> >>>> name += \"(\" + camera->id() + \")\";\n> >>>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n> >>>> index ac4619511..8d57023e1 100644\n> >>>> --- a/src/apps/common/dng_writer.cpp\n> >>>> +++ b/src/apps/common/dng_writer.cpp\n> >>>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n> >>>>\n> >>>> const auto &model = cameraProperties.get(properties::Model);\n> >>>> if (model) {\n> >>>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n> >>>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n> >>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n> > \n> > This looks like a new construction ... does this make a copy ?\n> > \n> > \n> > As far as I can tell - this is functionally correct/valid - and all the\n> > build tests (except the build-history) work - so I'd put :\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > Curious to see if this is something that provides enough 'benefit' over\n> > changing how string controls are handled vs potential copies.\n> > \n> > Do we ever expect strings to be expensive?\n> > \n> > I could actually imagine some use cases where we could start making\n> > internal/debug controls that might report larger strings in fact ... so\n> > it might actaully be helpful there!\n> > \n> > \n> > \n> >>>> }\n> >>>>\n> >>>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n> >>>> index 6b6d0713c..9f25ba091 100644\n> >>>> --- a/src/apps/qcam/cam_select_dialog.cpp\n> >>>> +++ b/src/apps/qcam/cam_select_dialog.cpp\n> >>>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n> >>>> cameraLocation_->setText(\"Unknown\");\n> >>>> }\n> >>>>\n> >>>> - const auto &model = properties.get(libcamera::properties::Model)\n> >>>> - .value_or(\"Unknown\");\n> >>>> + const auto model = properties.get(libcamera::properties::Model)\n> >>>> + .value_or(\"Unknown\");\n> >>>>\n> >>>> - cameraModel_->setText(QString::fromStdString(model));\n> >>>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n> >>>> }\n> >>>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n> >>>> index 1ad1d4c1a..8c55ef845 100644\n> >>>> --- a/src/py/libcamera/py_helpers.cpp\n> >>>> +++ b/src/py/libcamera/py_helpers.cpp\n> >>>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n> >>>> case ControlTypeFloat:\n> >>>> return valueOrTuple(cv);\n> >>>> case ControlTypeString:\n> >>>> - return py::cast(cv.get());\n> >>>> + return py::cast(cv.get());\n> >>>> case ControlTypeSize: {\n> >>>> const Size *v = reinterpret_cast(cv.data().data());\n> >>>> return py::cast(v);\n> >>>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n> >>>> case ControlTypeFloat:\n> >>>> return controlValueMaybeArray(ob);\n> >>>> case ControlTypeString:\n> >>>> - return ControlValue(ob.cast());\n> >>>> + return ControlValue(ob.cast());\n> >>>> case ControlTypeRectangle:\n> >>>> return controlValueMaybeArray(ob);\n> >>>> case ControlTypeSize:\n> >>>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n> >>>> index 5084fd0cf..032050a77 100644\n> >>>> --- a/test/controls/control_value.cpp\n> >>>> +++ b/test/controls/control_value.cpp\n> >>>> @@ -318,7 +318,7 @@ protected:\n> >>>> /*\n> >>>> * String type.\n> >>>> */\n> >>>> - std::string string{ \"libcamera\" };\n> >>>> + std::string_view string{ \"libcamera\" };\n> >>>> value.set(string);\n> >>>> if (value.isNone() || !value.isArray() ||\n> >>>> value.type() != ControlTypeString ||\n> >>>> @@ -327,7 +327,7 @@ protected:\n> >>>> return TestFail;\n> >>>> }\n> >>>>\n> >>>> - if (value.get() != string) {\n> >>>> + if (value.get() != string) {\n> >>>> cerr << \"Control value mismatch after setting to string\" << endl;\n> >>>> return TestFail;\n> >>>> }\n> >>>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n> >>>> index e51610481..9399727bd 100644\n> >>>> --- a/utils/codegen/controls.py\n> >>>> +++ b/utils/codegen/controls.py\n> >>>> @@ -111,7 +111,7 @@ class Control(object):\n> >>>> size = self.__data.get('size')\n> >>>>\n> >>>> if typ == 'string':\n> >>>> - return 'std::string'\n> >>>> + return 'std::string_view'\n> >>>>\n> >>>> if self.__size is None:\n> >>>> return typ\n> >>>> --\n> >>>> 2.49.0\n> >>>>\n> >> @\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 E4B48C327D\n\tfor ;\n\tThu, 1 May 2025 11:45:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0BEB617D7;\n\tThu, 1 May 2025 13:45:57 +0200 (CEST)","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 5A08A617D7\n\tfor ;\n\tThu, 1 May 2025 13:45:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B7E42E0;\n\tThu, 1 May 2025 13:45:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"izJJDjsq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746099949;\n\tbh=UKc/wGITXWizRGnpGTmAqZ4eBvfcpihGNt0HQw7K2bQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=izJJDjsqQ01HbRILWrc1oPugN65y5SZiYjlSQvCjue7G7hGXuYA0KfLJ7HxdcHXf6\n\tdLHeoWCdmSLathwr2MrNFE3V939pOmR31h/bidUbtqiXAFLLG4Hgbgfar/gHV/VbYH\n\to0ApG/9HA6YRDOLUIcfeyLBMOhwIodAzKEP+1nuI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<6b940091-30ff-49c9-ad67-b647cecd614f@ideasonboard.com>","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>\n\t<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>\n\t\n\t\n\t<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>\n\t<6b940091-30ff-49c9-ad67-b647cecd614f@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 01 May 2025 12:45:53 +0100","Message-ID":"<174609995358.2882969.4307860543156159477@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":34093,"web_url":"https://patchwork.libcamera.org/comment/34093/","msgid":"<4c843321-7cb5-47c2-bdd3-20e90a9d1a56@ideasonboard.com>","date":"2025-05-01T14:12:28","subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 05. 01. 13:45 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-05-01 11:25:12)\n>> 2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-04-25 09:18:28)\n>>>> Hi\n>>>>\n>>>>\n>>>> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta:\n>>>>> Quoting Barnabás Pőcze (2025-04-22 13:47:51)\n>>>>>> When retrieving the value from a `ControlValue` usually one of two\n>>>>>> things happen: a small, trivially copyable object is returned by\n>>>>>> value; or a view into the internal buffer is provided. This is true\n>>>>>> for everything except strings, which are returned in `std::string`,\n>>>>>> incurring the overhead of string construction.\n>>>>>>\n>>>>>> To guarantee no potentially \"expensive\" copies, use `std::string_view`\n>>>>>> pointing to the internal buffer to return the value. This is similar\n>>>>>> to how other array-like types are returned with a `Span<>`.\n>>>>>>\n>>>>>> This is an API break, but its scope is limited to just `properties::Model`.\n>>>>>\n>>>>> This breaks in CI at the build-history stage:\n>>>>>\n>>>>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034\n>>>>>\n>>>>> I wonder, are we missing a dependency declaration somewhere that should\n>>>>> have otherwise caused something to get rebuilt? All of the other builds\n>>>>> seemed to have worked on that pipeline ?\n>>>>\n>>>> Yes indeed, `controls.py` is changed, but that is not considered by\n>>>> meson, so it does not regenerate the cpp files because neither the\n>>>> template nor `gen-controls.py`, etc. has changed.\n>>>\n>>> Yikes. Probably something to bring up in the meson channels for support.\n>>> I have no idea on a (generic/longer term) solution I'm afraid.\n>>>\n>>>> I am not sure how it could be best addressed, there is `depend_files` where\n>>>> `controls.py` could be specified, but I don't think it's too convenient.\n>>>>\n>>>> Or there is the `modulefinder` thing in the standard library:\n>>>>\n>>>> >>> os.getcwd()\n>>>> '/libcamera/utils/codegen'\n>>>> >>> import modulefinder\n>>>> >>> m = modulefinder.ModuleFinder()\n>>>> >>> m.run_script(\"./gen-controls.py\")\n>>>> >>> m.modules[\"controls\"]\n>>>> Module('controls', '/libcamera/utils/codegen/controls.py')\n>>>>\n>>>> I am wondering if this might be used to generate a dependency file that ninja\n>>>> can interpret. Although this approach is definitely more complex.\n>>>\n>>> Yes, I can see that could get ugly - and we're not supposed to have to\n>>> parse the dependencies of every tool we use. Ok - so I guess this one is\n>>> different because it's our own in built tooling ...\n>>>\n>>> I think we could :\n>>> - Contact meson developers for support\n>>> - Update this patch to 'also' (trivially) modify either the template or\n>>> gen-controls.py\n>>> - Ignore the build-history failure and just keep it as is..\n>>>\n>>> I think if we want to land this series - at worst I'm ok with the last\n>>> option - but I'd rather keep it as a last resort to make sure we\n>>> maintain bisectability. And for that - I think even an arbitrary comment\n>>> update to the gen-controls.py which could be later removed would be fine\n>>> if it came to it.\n>>\n>> v2 addresses this issue by adding `depend_files` to each `custom_target()`.\n>> Not the best solution, but I couldn't find anything better.\n> \n> Excellent, that's fine with me.\n> \n> There were a few other statements/questions below here... any comment? -\n> I won't bother asking them on the new version ... you can keep/add my\n> tag to the new version.\n\nOops, I did miss those.\n\n\n> \n> \n>>\n>>\n>>>\n>>>\n>>>\n>>>>\n>>>>\n>>>> Regards,\n>>>> Barnabás Pőcze\n>>>>\n>>>>>\n>>>>>>\n>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256\n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> include/libcamera/control_ids.h.in | 1 +\n>>>>>> include/libcamera/controls.h | 15 ++++++++-------\n>>>>>> src/apps/cam/capture_script.cpp | 2 +-\n>>>>>> src/apps/cam/main.cpp | 2 +-\n>>>>>> src/apps/common/dng_writer.cpp | 2 +-\n>>>>>> src/apps/qcam/cam_select_dialog.cpp | 6 +++---\n>>>>>> src/py/libcamera/py_helpers.cpp | 4 ++--\n>>>>>> test/controls/control_value.cpp | 4 ++--\n>>>>>> utils/codegen/controls.py | 2 +-\n>>>>>> 9 files changed, 20 insertions(+), 18 deletions(-)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n>>>>>> index 5d0594c68..41997f79f 100644\n>>>>>> --- a/include/libcamera/control_ids.h.in\n>>>>>> +++ b/include/libcamera/control_ids.h.in\n>>>>>> @@ -13,6 +13,7 @@\n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> +#include \n>>>>>>\n>>>>>> #include \n>>>>>>\n>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>>> index 4bfe9615c..275f45200 100644\n>>>>>> --- a/include/libcamera/controls.h\n>>>>>> +++ b/include/libcamera/controls.h\n>>>>>> @@ -13,6 +13,7 @@\n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> +#include \n>>>>>> #include \n>>>>>> #include \n>>>>>>\n>>>>>> @@ -96,7 +97,7 @@ struct control_type {\n>>>>>> };\n>>>>>>\n>>>>>> template<>\n>>>>>> -struct control_type {\n>>>>>> +struct control_type {\n>>>>>> static constexpr ControlType value = ControlTypeString;\n>>>>>> static constexpr std::size_t size = 0;\n>>>>>> };\n>>>>>> @@ -138,7 +139,7 @@ public:\n>>>>>> #ifndef __DOXYGEN__\n>>>>>> template::value &&\n>>>>>> details::control_type::value &&\n>>>>>> - !std::is_same>::value,\n>>>>>> + !std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> ControlValue(const T &value)\n>>>>>> : type_(ControlTypeNone), numElements_(0)\n>>>>>> @@ -148,7 +149,7 @@ public:\n>>>>>> }\n>>>>>>\n>>>>>> template::value ||\n>>>>>> - std::is_same>::value,\n>>>>>> + std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> #else\n>>>>>> template\n>>>>>> @@ -182,7 +183,7 @@ public:\n>>>>>>\n>>>>>> #ifndef __DOXYGEN__\n>>>>>> template::value &&\n>>>>>> - !std::is_same>::value,\n>>>>>> + !std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> T get() const\n>>>>>> {\n>>>>>> @@ -193,7 +194,7 @@ public:\n>>>>>> }\n>>>>>>\n>>>>>> template::value ||\n>>>>>> - std::is_same>::value,\n>>>>>> + std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> #else\n>>>>>> template\n>>>>>> @@ -210,7 +211,7 @@ public:\n>>>>>>\n>>>>>> #ifndef __DOXYGEN__\n>>>>>> template::value &&\n>>>>>> - !std::is_same>::value,\n>>>>>> + !std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> void set(const T &value)\n>>>>>> {\n>>>>>> @@ -219,7 +220,7 @@ public:\n>>>>>> }\n>>>>>>\n>>>>>> template::value ||\n>>>>>> - std::is_same>::value,\n>>>>>> + std::is_same>::value,\n>>>>>> std::nullptr_t> = nullptr>\n>>>>>> #else\n>>>>>> template\n>>>>>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n>>>>>> index fdf82efc0..bc4c7c3a0 100644\n>>>>>> --- a/src/apps/cam/capture_script.cpp\n>>>>>> +++ b/src/apps/cam/capture_script.cpp\n>>>>>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,\n>>>>>> break;\n>>>>>> }\n>>>>>> case ControlTypeString: {\n>>>>>> - value.set(repr);\n>>>>>> + value.set(repr);\n>>>>>> break;\n>>>>>> }\n>>>>>> default:\n>>>>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n>>>>>> index fa266eca6..157d238d7 100644\n>>>>>> --- a/src/apps/cam/main.cpp\n>>>>>> +++ b/src/apps/cam/main.cpp\n>>>>>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)\n>>>>>> */\n>>>>>> const auto &model = props.get(properties::Model);\n>>>>>> if (model)\n>>>>>> - name = \"'\" + *model + \"' \";\n>>>>>> + name.append(\"'\").append(*model).append(\"' \");\n>>>\n>>> Aha, ok so this is the API breakage ... So we'll lose the ability to\n>>> flexibly represent strings ?\n\nOne can always do e.g. `std::string(x.get(controls::Model))`. By returning an\n`std::string_view`, no `std::string` construction is forced. If the user wants\nan `std::string`, then they can construct one as seen above, and they get the\nprevious behaviour back.\n\n\n>>>\n>>> Does this append have to construct a string ? (i.e. are we going to end\n>>> up just 'moving' the new construction?\n\n`std::string::append()` does not need to construct a separate string from `*model`.\n\n\n>>>\n>>> Any help/benefit from using include/libcamera/base/utils.h\n>>> libcamera::join();? Ah - no that would only add separateors - what we\n>>> would need as a string 'wrap' to allow wrapping the model with '\n>>> characters.\n\nFYI, there is `std::quoted()` when working with streams: https://en.cppreference.com/w/cpp/io/manip/quoted\nOf course, that's not applicable here, but nonetheless I don't think\nthis is a big issue.\n\n\n\n>>>\n>>> There are other occasions where I've thought about adding a wrap ..\n>>> wrapper function but it's probably overkill.\n>>>\n>>>\n>>>\n>>>>>> }\n>>>>>>\n>>>>>> name += \"(\" + camera->id() + \")\";\n>>>>>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp\n>>>>>> index ac4619511..8d57023e1 100644\n>>>>>> --- a/src/apps/common/dng_writer.cpp\n>>>>>> +++ b/src/apps/common/dng_writer.cpp\n>>>>>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,\n>>>>>>\n>>>>>> const auto &model = cameraProperties.get(properties::Model);\n>>>>>> if (model) {\n>>>>>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());\n>>>>>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());\n>>>>>> /* \\todo set TIFFTAG_UNIQUECAMERAMODEL. */\n>>>\n>>> This looks like a new construction ... does this make a copy ?\n\nYes, it does. Previously `cameraProperties.get(properties::Model)` would have\nmade the equivalent copy, but since that now returns an `std::string_view`,\nthe copy has to be explicitly made in the caller if an `std::string` is needed.\nSo the number of copies does not change. (I did not find a way to pass a (ptr, len)\npair to `TIFFSetField()` or similar, which could avoid the copy altogether.)\n\n\n>>>\n>>>\n>>> As far as I can tell - this is functionally correct/valid - and all the\n>>> build tests (except the build-history) work - so I'd put :\n>>>\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>> Curious to see if this is something that provides enough 'benefit' over\n>>> changing how string controls are handled vs potential copies.\n>>>\n>>> Do we ever expect strings to be expensive?\n\nTo be honest I don't really see it ever becoming a bottleneck.\n\n\nRegards,\nBarnabás Pőcze\n\n\n>>>\n>>> I could actually imagine some use cases where we could start making\n>>> internal/debug controls that might report larger strings in fact ... so\n>>> it might actaully be helpful there!\n>>>\n>>>\n>>>\n>>>>>> }\n>>>>>>\n>>>>>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp\n>>>>>> index 6b6d0713c..9f25ba091 100644\n>>>>>> --- a/src/apps/qcam/cam_select_dialog.cpp\n>>>>>> +++ b/src/apps/qcam/cam_select_dialog.cpp\n>>>>>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)\n>>>>>> cameraLocation_->setText(\"Unknown\");\n>>>>>> }\n>>>>>>\n>>>>>> - const auto &model = properties.get(libcamera::properties::Model)\n>>>>>> - .value_or(\"Unknown\");\n>>>>>> + const auto model = properties.get(libcamera::properties::Model)\n>>>>>> + .value_or(\"Unknown\");\n>>>>>>\n>>>>>> - cameraModel_->setText(QString::fromStdString(model));\n>>>>>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));\n>>>>>> }\n>>>>>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp\n>>>>>> index 1ad1d4c1a..8c55ef845 100644\n>>>>>> --- a/src/py/libcamera/py_helpers.cpp\n>>>>>> +++ b/src/py/libcamera/py_helpers.cpp\n>>>>>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)\n>>>>>> case ControlTypeFloat:\n>>>>>> return valueOrTuple(cv);\n>>>>>> case ControlTypeString:\n>>>>>> - return py::cast(cv.get());\n>>>>>> + return py::cast(cv.get());\n>>>>>> case ControlTypeSize: {\n>>>>>> const Size *v = reinterpret_cast(cv.data().data());\n>>>>>> return py::cast(v);\n>>>>>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)\n>>>>>> case ControlTypeFloat:\n>>>>>> return controlValueMaybeArray(ob);\n>>>>>> case ControlTypeString:\n>>>>>> - return ControlValue(ob.cast());\n>>>>>> + return ControlValue(ob.cast());\n>>>>>> case ControlTypeRectangle:\n>>>>>> return controlValueMaybeArray(ob);\n>>>>>> case ControlTypeSize:\n>>>>>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp\n>>>>>> index 5084fd0cf..032050a77 100644\n>>>>>> --- a/test/controls/control_value.cpp\n>>>>>> +++ b/test/controls/control_value.cpp\n>>>>>> @@ -318,7 +318,7 @@ protected:\n>>>>>> /*\n>>>>>> * String type.\n>>>>>> */\n>>>>>> - std::string string{ \"libcamera\" };\n>>>>>> + std::string_view string{ \"libcamera\" };\n>>>>>> value.set(string);\n>>>>>> if (value.isNone() || !value.isArray() ||\n>>>>>> value.type() != ControlTypeString ||\n>>>>>> @@ -327,7 +327,7 @@ protected:\n>>>>>> return TestFail;\n>>>>>> }\n>>>>>>\n>>>>>> - if (value.get() != string) {\n>>>>>> + if (value.get() != string) {\n>>>>>> cerr << \"Control value mismatch after setting to string\" << endl;\n>>>>>> return TestFail;\n>>>>>> }\n>>>>>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py\n>>>>>> index e51610481..9399727bd 100644\n>>>>>> --- a/utils/codegen/controls.py\n>>>>>> +++ b/utils/codegen/controls.py\n>>>>>> @@ -111,7 +111,7 @@ class Control(object):\n>>>>>> size = self.__data.get('size')\n>>>>>>\n>>>>>> if typ == 'string':\n>>>>>> - return 'std::string'\n>>>>>> + return 'std::string_view'\n>>>>>>\n>>>>>> if self.__size is None:\n>>>>>> return typ\n>>>>>> --\n>>>>>> 2.49.0\n>>>>>>\n>>>> @\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 D853DBE08B\n\tfor ;\n\tThu, 1 May 2025 14:12:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33A7C68AD9;\n\tThu, 1 May 2025 16:12:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 903B7617D7\n\tfor ;\n\tThu, 1 May 2025 16:12:32 +0200 (CEST)","from [192.168.33.21] (185.221.143.50.nat.pool.zt.hu\n\t[185.221.143.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E78F63D;\n\tThu, 1 May 2025 16:12:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sTHAePIT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746108745;\n\tbh=2JWQQ4Vn4ZgBWoRm02L3wZXQyU6CVIb8lHhDlSOWeto=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=sTHAePITgs6Hh0l4zGmZpLvSsR3WKIJPnd6BuncMsfHzGA+PdmWrW9txlQayug4mw\n\t4sEpEZTRndaeos0wisHSg2Tv86l383W+yfk9NJeK7A/DcGRX0wcEO5SokI8HMWf2Es\n\t6ikiGdg3eI/yUZjPJJ20i49xuxVpo24VD8sGK5U8=","Message-ID":"<4c843321-7cb5-47c2-bdd3-20e90a9d1a56@ideasonboard.com>","Date":"Thu, 1 May 2025 16:12:28 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] libcamera: controls: Expose string controls as\n\t`std::string_view`","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-3-barnabas.pocze@ideasonboard.com>\n\t<174539722838.891349.14908327028555187121@ping.linuxembedded.co.uk>\n\t\n\t\n\t<174609460609.299497.254800353994901122@ping.linuxembedded.co.uk>\n\t<6b940091-30ff-49c9-ad67-b647cecd614f@ideasonboard.com>\n\t<174609995358.2882969.4307860543156159477@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174609995358.2882969.4307860543156159477@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]