Message ID | 20250501095818.3996419-4-barnabas.pocze@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > When retrieving the value from a `ControlValue` usually one of two > things happen: a small, trivially copyable object is returned by > value; or a view into the internal buffer is provided. This is true > for everything except strings, which are returned in `std::string`, > incurring the overhead of string construction. > > To guarantee no potentially "expensive" copies, use `std::string_view` > pointing to the internal buffer to return the value. This is similar > to how other array-like types are returned with a `Span<>`. > > This is an API break, but its scope is limited to just `properties::Model`. I've been a bit in two minds about this, until I realized we already require c++17 for applications. My understanding is that using string_view is no different than returning a Span<> when it comes to lifetime of the referenced data, so if we're fine with Span<> we're fine with string_view. And as pointed out by Barnabas, if a user wants a string, it can be constructed easily with the same cost as we have today. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 1 + > include/libcamera/controls.h | 15 ++++++++------- > src/apps/cam/capture_script.cpp | 2 +- > src/apps/cam/main.cpp | 2 +- > src/apps/common/dng_writer.cpp | 2 +- > src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > src/py/libcamera/py_helpers.cpp | 4 ++-- > test/controls/control_value.cpp | 4 ++-- > utils/codegen/controls.py | 2 +- > 9 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index 5d0594c68..41997f79f 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -13,6 +13,7 @@ > #include <map> > #include <stdint.h> > #include <string> > +#include <string_view> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 2ae4ec3d4..5a707a387 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -13,6 +13,7 @@ > #include <set> > #include <stdint.h> > #include <string> > +#include <string_view> > #include <unordered_map> > #include <vector> > > @@ -96,7 +97,7 @@ struct control_type<float> { > }; > > template<> > -struct control_type<std::string> { > +struct control_type<std::string_view> { > static constexpr ControlType value = ControlTypeString; > static constexpr std::size_t size = 0; > }; > @@ -138,7 +139,7 @@ public: > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > details::control_type<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > ControlValue(const T &value) > : type_(ControlTypeNone), numElements_(0) > @@ -148,7 +149,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -182,7 +183,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > T get() const > { > @@ -193,7 +194,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -210,7 +211,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > void set(const T &value) > { > @@ -219,7 +220,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > index fdf82efc0..bc4c7c3a0 100644 > --- a/src/apps/cam/capture_script.cpp > +++ b/src/apps/cam/capture_script.cpp > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > break; > } > case ControlTypeString: { > - value.set<std::string>(repr); > + value.set<std::string_view>(repr); > break; > } > default: > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > index fa266eca6..157d238d7 100644 > --- a/src/apps/cam/main.cpp > +++ b/src/apps/cam/main.cpp > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > */ > const auto &model = props.get(properties::Model); Does this need to be a reference now ? > if (model) > - name = "'" + *model + "' "; > + name.append("'").append(*model).append("' "); > } > > name += "(" + camera->id() + ")"; > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > index ac4619511..8d57023e1 100644 > --- a/src/apps/common/dng_writer.cpp > +++ b/src/apps/common/dng_writer.cpp > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > const auto &model = cameraProperties.get(properties::Model); likewise > if (model) { > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > } > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > index 6b6d0713c..9f25ba091 100644 > --- a/src/apps/qcam/cam_select_dialog.cpp > +++ b/src/apps/qcam/cam_select_dialog.cpp > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > cameraLocation_->setText("Unknown"); > } > > - const auto &model = properties.get(libcamera::properties::Model) > - .value_or("Unknown"); > + const auto model = properties.get(libcamera::properties::Model) > + .value_or("Unknown"); > > - cameraModel_->setText(QString::fromStdString(model)); > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > } > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > index 1ad1d4c1a..8c55ef845 100644 > --- a/src/py/libcamera/py_helpers.cpp > +++ b/src/py/libcamera/py_helpers.cpp > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > case ControlTypeFloat: > return valueOrTuple<float>(cv); > case ControlTypeString: > - return py::cast(cv.get<std::string>()); > + return py::cast(cv.get<std::string_view>()); > case ControlTypeSize: { > const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > return py::cast(v); > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > case ControlTypeFloat: > return controlValueMaybeArray<float>(ob); > case ControlTypeString: > - return ControlValue(ob.cast<std::string>()); > + return ControlValue(ob.cast<std::string_view>()); > case ControlTypeRectangle: > return controlValueMaybeArray<Rectangle>(ob); > case ControlTypeSize: > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index 5084fd0cf..032050a77 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -318,7 +318,7 @@ protected: > /* > * String type. > */ > - std::string string{ "libcamera" }; > + std::string_view string{ "libcamera" }; > value.set(string); Here's one thing: we now require users to use string_view when setting a control, and we anyway end up copying it's content into the ControlValue. Should we provide a set() overload for std::string ? Is it worth it ? > if (value.isNone() || !value.isArray() || > value.type() != ControlTypeString || > @@ -327,7 +327,7 @@ protected: > return TestFail; > } > > - if (value.get<std::string>() != string) { > + if (value.get<std::string_view>() != string) { For get() instead I think it's fine always returning a view > cerr << "Control value mismatch after setting to string" << endl; > return TestFail; > } > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > index e51610481..9399727bd 100644 > --- a/utils/codegen/controls.py > +++ b/utils/codegen/controls.py > @@ -111,7 +111,7 @@ class Control(object): > size = self.__data.get('size') > > if typ == 'string': > - return 'std::string' > + return 'std::string_view' > > if self.__size is None: > return typ > -- > 2.49.0 >
Hi 2025. 05. 02. 12:11 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: >> When retrieving the value from a `ControlValue` usually one of two >> things happen: a small, trivially copyable object is returned by >> value; or a view into the internal buffer is provided. This is true >> for everything except strings, which are returned in `std::string`, >> incurring the overhead of string construction. >> >> To guarantee no potentially "expensive" copies, use `std::string_view` >> pointing to the internal buffer to return the value. This is similar >> to how other array-like types are returned with a `Span<>`. >> >> This is an API break, but its scope is limited to just `properties::Model`. > > I've been a bit in two minds about this, until I realized we already > require c++17 for applications. > > My understanding is that using string_view is no different than > returning a Span<> when it comes to lifetime of the referenced data, > so if we're fine with Span<> we're fine with string_view. > > And as pointed out by Barnabas, if a user wants a string, it can be > constructed easily with the same cost as we have today. > >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/control_ids.h.in | 1 + >> include/libcamera/controls.h | 15 ++++++++------- >> src/apps/cam/capture_script.cpp | 2 +- >> src/apps/cam/main.cpp | 2 +- >> src/apps/common/dng_writer.cpp | 2 +- >> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- >> src/py/libcamera/py_helpers.cpp | 4 ++-- >> test/controls/control_value.cpp | 4 ++-- >> utils/codegen/controls.py | 2 +- >> 9 files changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in >> index 5d0594c68..41997f79f 100644 >> --- a/include/libcamera/control_ids.h.in >> +++ b/include/libcamera/control_ids.h.in >> @@ -13,6 +13,7 @@ >> #include <map> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> >> #include <libcamera/controls.h> >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 2ae4ec3d4..5a707a387 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -13,6 +13,7 @@ >> #include <set> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> #include <unordered_map> >> #include <vector> >> >> @@ -96,7 +97,7 @@ struct control_type<float> { >> }; >> >> template<> >> -struct control_type<std::string> { >> +struct control_type<std::string_view> { >> static constexpr ControlType value = ControlTypeString; >> static constexpr std::size_t size = 0; >> }; >> @@ -138,7 +139,7 @@ public: >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> details::control_type<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> ControlValue(const T &value) >> : type_(ControlTypeNone), numElements_(0) >> @@ -148,7 +149,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -182,7 +183,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> T get() const >> { >> @@ -193,7 +194,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -210,7 +211,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> void set(const T &value) >> { >> @@ -219,7 +220,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp >> index fdf82efc0..bc4c7c3a0 100644 >> --- a/src/apps/cam/capture_script.cpp >> +++ b/src/apps/cam/capture_script.cpp >> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, >> break; >> } >> case ControlTypeString: { >> - value.set<std::string>(repr); >> + value.set<std::string_view>(repr); >> break; >> } >> default: >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp >> index fa266eca6..157d238d7 100644 >> --- a/src/apps/cam/main.cpp >> +++ b/src/apps/cam/main.cpp >> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) >> */ >> const auto &model = props.get(properties::Model); > > Does this need to be a reference now ? > >> if (model) >> - name = "'" + *model + "' "; >> + name.append("'").append(*model).append("' "); >> } >> >> name += "(" + camera->id() + ")"; >> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp >> index ac4619511..8d57023e1 100644 >> --- a/src/apps/common/dng_writer.cpp >> +++ b/src/apps/common/dng_writer.cpp >> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, >> >> const auto &model = cameraProperties.get(properties::Model); > > likewise > >> if (model) { >> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); >> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); >> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ >> } >> >> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp >> index 6b6d0713c..9f25ba091 100644 >> --- a/src/apps/qcam/cam_select_dialog.cpp >> +++ b/src/apps/qcam/cam_select_dialog.cpp >> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) >> cameraLocation_->setText("Unknown"); >> } >> >> - const auto &model = properties.get(libcamera::properties::Model) >> - .value_or("Unknown"); >> + const auto model = properties.get(libcamera::properties::Model) >> + .value_or("Unknown"); >> >> - cameraModel_->setText(QString::fromStdString(model)); >> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); >> } >> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp >> index 1ad1d4c1a..8c55ef845 100644 >> --- a/src/py/libcamera/py_helpers.cpp >> +++ b/src/py/libcamera/py_helpers.cpp >> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) >> case ControlTypeFloat: >> return valueOrTuple<float>(cv); >> case ControlTypeString: >> - return py::cast(cv.get<std::string>()); >> + return py::cast(cv.get<std::string_view>()); >> case ControlTypeSize: { >> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); >> return py::cast(v); >> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) >> case ControlTypeFloat: >> return controlValueMaybeArray<float>(ob); >> case ControlTypeString: >> - return ControlValue(ob.cast<std::string>()); >> + return ControlValue(ob.cast<std::string_view>()); >> case ControlTypeRectangle: >> return controlValueMaybeArray<Rectangle>(ob); >> case ControlTypeSize: >> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp >> index 5084fd0cf..032050a77 100644 >> --- a/test/controls/control_value.cpp >> +++ b/test/controls/control_value.cpp >> @@ -318,7 +318,7 @@ protected: >> /* >> * String type. >> */ >> - std::string string{ "libcamera" }; >> + std::string_view string{ "libcamera" }; >> value.set(string); > > Here's one thing: we now require users to use string_view when setting > a control, and we anyway end up copying it's content into the > ControlValue. > > Should we provide a set() overload for std::string ? Is it worth it ? I think that could be useful, although I would defer it because there is only a single string-valued control (properties::Model). And if we want to support multiple "input" types for a single control types, then I think there are better ways but those need potentially more changes. Regards, Barnabás Pőcze > >> if (value.isNone() || !value.isArray() || >> value.type() != ControlTypeString || >> @@ -327,7 +327,7 @@ protected: >> return TestFail; >> } >> >> - if (value.get<std::string>() != string) { >> + if (value.get<std::string_view>() != string) { > > For get() instead I think it's fine always returning a view > >> cerr << "Control value mismatch after setting to string" << endl; >> return TestFail; >> } >> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py >> index e51610481..9399727bd 100644 >> --- a/utils/codegen/controls.py >> +++ b/utils/codegen/controls.py >> @@ -111,7 +111,7 @@ class Control(object): >> size = self.__data.get('size') >> >> if typ == 'string': >> - return 'std::string' >> + return 'std::string_view' >> >> if self.__size is None: >> return typ >> -- >> 2.49.0 >>
Hi Barnabás On Fri, May 02, 2025 at 12:22:13PM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 05. 02. 12:11 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > > > When retrieving the value from a `ControlValue` usually one of two > > > things happen: a small, trivially copyable object is returned by > > > value; or a view into the internal buffer is provided. This is true > > > for everything except strings, which are returned in `std::string`, > > > incurring the overhead of string construction. > > > > > > To guarantee no potentially "expensive" copies, use `std::string_view` > > > pointing to the internal buffer to return the value. This is similar > > > to how other array-like types are returned with a `Span<>`. > > > > > > This is an API break, but its scope is limited to just `properties::Model`. > > > > I've been a bit in two minds about this, until I realized we already > > require c++17 for applications. > > > > My understanding is that using string_view is no different than > > returning a Span<> when it comes to lifetime of the referenced data, > > so if we're fine with Span<> we're fine with string_view. > > > > And as pointed out by Barnabas, if a user wants a string, it can be > > constructed easily with the same cost as we have today. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > include/libcamera/control_ids.h.in | 1 + > > > include/libcamera/controls.h | 15 ++++++++------- > > > src/apps/cam/capture_script.cpp | 2 +- > > > src/apps/cam/main.cpp | 2 +- > > > src/apps/common/dng_writer.cpp | 2 +- > > > src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > > > src/py/libcamera/py_helpers.cpp | 4 ++-- > > > test/controls/control_value.cpp | 4 ++-- > > > utils/codegen/controls.py | 2 +- > > > 9 files changed, 20 insertions(+), 18 deletions(-) > > > > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > > > index 5d0594c68..41997f79f 100644 > > > --- a/include/libcamera/control_ids.h.in > > > +++ b/include/libcamera/control_ids.h.in > > > @@ -13,6 +13,7 @@ > > > #include <map> > > > #include <stdint.h> > > > #include <string> > > > +#include <string_view> > > > > > > #include <libcamera/controls.h> > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index 2ae4ec3d4..5a707a387 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -13,6 +13,7 @@ > > > #include <set> > > > #include <stdint.h> > > > #include <string> > > > +#include <string_view> > > > #include <unordered_map> > > > #include <vector> > > > > > > @@ -96,7 +97,7 @@ struct control_type<float> { > > > }; > > > > > > template<> > > > -struct control_type<std::string> { > > > +struct control_type<std::string_view> { > > > static constexpr ControlType value = ControlTypeString; > > > static constexpr std::size_t size = 0; > > > }; > > > @@ -138,7 +139,7 @@ public: > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > details::control_type<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > ControlValue(const T &value) > > > : type_(ControlTypeNone), numElements_(0) > > > @@ -148,7 +149,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > @@ -182,7 +183,7 @@ public: > > > > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > T get() const > > > { > > > @@ -193,7 +194,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > @@ -210,7 +211,7 @@ public: > > > > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > void set(const T &value) > > > { > > > @@ -219,7 +220,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > > > index fdf82efc0..bc4c7c3a0 100644 > > > --- a/src/apps/cam/capture_script.cpp > > > +++ b/src/apps/cam/capture_script.cpp > > > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > > > break; > > > } > > > case ControlTypeString: { > > > - value.set<std::string>(repr); > > > + value.set<std::string_view>(repr); > > > break; > > > } > > > default: > > > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > > > index fa266eca6..157d238d7 100644 > > > --- a/src/apps/cam/main.cpp > > > +++ b/src/apps/cam/main.cpp > > > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > > > */ > > > const auto &model = props.get(properties::Model); > > > > Does this need to be a reference now ? > > > > > if (model) > > > - name = "'" + *model + "' "; > > > + name.append("'").append(*model).append("' "); > > > } > > > > > > name += "(" + camera->id() + ")"; > > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > > > index ac4619511..8d57023e1 100644 > > > --- a/src/apps/common/dng_writer.cpp > > > +++ b/src/apps/common/dng_writer.cpp > > > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > > > const auto &model = cameraProperties.get(properties::Model); > > > > likewise > > > > > if (model) { > > > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > > > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > > > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > > > } > > > > > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > > > index 6b6d0713c..9f25ba091 100644 > > > --- a/src/apps/qcam/cam_select_dialog.cpp > > > +++ b/src/apps/qcam/cam_select_dialog.cpp > > > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > > > cameraLocation_->setText("Unknown"); > > > } > > > > > > - const auto &model = properties.get(libcamera::properties::Model) > > > - .value_or("Unknown"); > > > + const auto model = properties.get(libcamera::properties::Model) > > > + .value_or("Unknown"); > > > > > > - cameraModel_->setText(QString::fromStdString(model)); > > > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > > > } > > > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > > > index 1ad1d4c1a..8c55ef845 100644 > > > --- a/src/py/libcamera/py_helpers.cpp > > > +++ b/src/py/libcamera/py_helpers.cpp > > > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > > > case ControlTypeFloat: > > > return valueOrTuple<float>(cv); > > > case ControlTypeString: > > > - return py::cast(cv.get<std::string>()); > > > + return py::cast(cv.get<std::string_view>()); > > > case ControlTypeSize: { > > > const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > > > return py::cast(v); > > > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > > > case ControlTypeFloat: > > > return controlValueMaybeArray<float>(ob); > > > case ControlTypeString: > > > - return ControlValue(ob.cast<std::string>()); > > > + return ControlValue(ob.cast<std::string_view>()); > > > case ControlTypeRectangle: > > > return controlValueMaybeArray<Rectangle>(ob); > > > case ControlTypeSize: > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > index 5084fd0cf..032050a77 100644 > > > --- a/test/controls/control_value.cpp > > > +++ b/test/controls/control_value.cpp > > > @@ -318,7 +318,7 @@ protected: > > > /* > > > * String type. > > > */ > > > - std::string string{ "libcamera" }; > > > + std::string_view string{ "libcamera" }; > > > value.set(string); > > > > Here's one thing: we now require users to use string_view when setting > > a control, and we anyway end up copying it's content into the > > ControlValue. > > > > Should we provide a set() overload for std::string ? Is it worth it ? > > I think that could be useful, although I would defer it because there is only > a single string-valued control (properties::Model). And if we want to support > multiple "input" types for a single control types, then I think there are > better ways but those need potentially more changes. > Also, it's a property, so no way one can set it. Fine then, I would record a \todo so that we don't forget about this when we'll add a string control, but up to you. > > Regards, > Barnabás Pőcze > > > > > > if (value.isNone() || !value.isArray() || > > > value.type() != ControlTypeString || > > > @@ -327,7 +327,7 @@ protected: > > > return TestFail; > > > } > > > > > > - if (value.get<std::string>() != string) { > > > + if (value.get<std::string_view>() != string) { > > > > For get() instead I think it's fine always returning a view > > > > > cerr << "Control value mismatch after setting to string" << endl; > > > return TestFail; > > > } > > > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > > > index e51610481..9399727bd 100644 > > > --- a/utils/codegen/controls.py > > > +++ b/utils/codegen/controls.py > > > @@ -111,7 +111,7 @@ class Control(object): > > > size = self.__data.get('size') > > > > > > if typ == 'string': > > > - return 'std::string' > > > + return 'std::string_view' > > > > > > if self.__size is None: > > > return typ > > > -- > > > 2.49.0 > > > >
Hi 2025. 05. 02. 12:11 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: >> When retrieving the value from a `ControlValue` usually one of two >> things happen: a small, trivially copyable object is returned by >> value; or a view into the internal buffer is provided. This is true >> for everything except strings, which are returned in `std::string`, >> incurring the overhead of string construction. >> >> To guarantee no potentially "expensive" copies, use `std::string_view` >> pointing to the internal buffer to return the value. This is similar >> to how other array-like types are returned with a `Span<>`. >> >> This is an API break, but its scope is limited to just `properties::Model`. > > I've been a bit in two minds about this, until I realized we already > require c++17 for applications. > > My understanding is that using string_view is no different than > returning a Span<> when it comes to lifetime of the referenced data, > so if we're fine with Span<> we're fine with string_view. > > And as pointed out by Barnabas, if a user wants a string, it can be > constructed easily with the same cost as we have today. > >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/control_ids.h.in | 1 + >> include/libcamera/controls.h | 15 ++++++++------- >> src/apps/cam/capture_script.cpp | 2 +- >> src/apps/cam/main.cpp | 2 +- >> src/apps/common/dng_writer.cpp | 2 +- >> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- >> src/py/libcamera/py_helpers.cpp | 4 ++-- >> test/controls/control_value.cpp | 4 ++-- >> utils/codegen/controls.py | 2 +- >> 9 files changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in >> index 5d0594c68..41997f79f 100644 >> --- a/include/libcamera/control_ids.h.in >> +++ b/include/libcamera/control_ids.h.in >> @@ -13,6 +13,7 @@ >> #include <map> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> >> #include <libcamera/controls.h> >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 2ae4ec3d4..5a707a387 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -13,6 +13,7 @@ >> #include <set> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> #include <unordered_map> >> #include <vector> >> >> @@ -96,7 +97,7 @@ struct control_type<float> { >> }; >> >> template<> >> -struct control_type<std::string> { >> +struct control_type<std::string_view> { >> static constexpr ControlType value = ControlTypeString; >> static constexpr std::size_t size = 0; >> }; >> @@ -138,7 +139,7 @@ public: >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> details::control_type<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> ControlValue(const T &value) >> : type_(ControlTypeNone), numElements_(0) >> @@ -148,7 +149,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -182,7 +183,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> T get() const >> { >> @@ -193,7 +194,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -210,7 +211,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> void set(const T &value) >> { >> @@ -219,7 +220,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp >> index fdf82efc0..bc4c7c3a0 100644 >> --- a/src/apps/cam/capture_script.cpp >> +++ b/src/apps/cam/capture_script.cpp >> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, >> break; >> } >> case ControlTypeString: { >> - value.set<std::string>(repr); >> + value.set<std::string_view>(repr); >> break; >> } >> default: >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp >> index fa266eca6..157d238d7 100644 >> --- a/src/apps/cam/main.cpp >> +++ b/src/apps/cam/main.cpp >> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) >> */ >> const auto &model = props.get(properties::Model); > > Does this need to be a reference now ? There isn't any real difference because it's an `std::optional` in both cases (just with different value type), so I decided not to change it. > >> if (model) >> - name = "'" + *model + "' "; >> + name.append("'").append(*model).append("' "); >> } >> >> name += "(" + camera->id() + ")"; >> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp >> index ac4619511..8d57023e1 100644 >> --- a/src/apps/common/dng_writer.cpp >> +++ b/src/apps/common/dng_writer.cpp >> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, >> >> const auto &model = cameraProperties.get(properties::Model); > > likewise Same here. Regards, Barnabás Pőcze > >> if (model) { >> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); >> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); >> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ >> } >> >> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp >> index 6b6d0713c..9f25ba091 100644 >> --- a/src/apps/qcam/cam_select_dialog.cpp >> +++ b/src/apps/qcam/cam_select_dialog.cpp >> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) >> cameraLocation_->setText("Unknown"); >> } >> >> - const auto &model = properties.get(libcamera::properties::Model) >> - .value_or("Unknown"); >> + const auto model = properties.get(libcamera::properties::Model) >> + .value_or("Unknown"); >> >> - cameraModel_->setText(QString::fromStdString(model)); >> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); >> } >> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp >> index 1ad1d4c1a..8c55ef845 100644 >> --- a/src/py/libcamera/py_helpers.cpp >> +++ b/src/py/libcamera/py_helpers.cpp >> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) >> case ControlTypeFloat: >> return valueOrTuple<float>(cv); >> case ControlTypeString: >> - return py::cast(cv.get<std::string>()); >> + return py::cast(cv.get<std::string_view>()); >> case ControlTypeSize: { >> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); >> return py::cast(v); >> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) >> case ControlTypeFloat: >> return controlValueMaybeArray<float>(ob); >> case ControlTypeString: >> - return ControlValue(ob.cast<std::string>()); >> + return ControlValue(ob.cast<std::string_view>()); >> case ControlTypeRectangle: >> return controlValueMaybeArray<Rectangle>(ob); >> case ControlTypeSize: >> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp >> index 5084fd0cf..032050a77 100644 >> --- a/test/controls/control_value.cpp >> +++ b/test/controls/control_value.cpp >> @@ -318,7 +318,7 @@ protected: >> /* >> * String type. >> */ >> - std::string string{ "libcamera" }; >> + std::string_view string{ "libcamera" }; >> value.set(string); > > Here's one thing: we now require users to use string_view when setting > a control, and we anyway end up copying it's content into the > ControlValue. > > Should we provide a set() overload for std::string ? Is it worth it ? > >> if (value.isNone() || !value.isArray() || >> value.type() != ControlTypeString || >> @@ -327,7 +327,7 @@ protected: >> return TestFail; >> } >> >> - if (value.get<std::string>() != string) { >> + if (value.get<std::string_view>() != string) { > > For get() instead I think it's fine always returning a view > >> cerr << "Control value mismatch after setting to string" << endl; >> return TestFail; >> } >> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py >> index e51610481..9399727bd 100644 >> --- a/utils/codegen/controls.py >> +++ b/utils/codegen/controls.py >> @@ -111,7 +111,7 @@ class Control(object): >> size = self.__data.get('size') >> >> if typ == 'string': >> - return 'std::string' >> + return 'std::string_view' >> >> if self.__size is None: >> return typ >> -- >> 2.49.0 >>
Hi Barnabás On Fri, May 02, 2025 at 02:17:17PM +0200, Barnabás Pőcze wrote: > Hi > > > 2025. 05. 02. 12:11 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > > > When retrieving the value from a `ControlValue` usually one of two > > > things happen: a small, trivially copyable object is returned by > > > value; or a view into the internal buffer is provided. This is true > > > for everything except strings, which are returned in `std::string`, > > > incurring the overhead of string construction. > > > > > > To guarantee no potentially "expensive" copies, use `std::string_view` > > > pointing to the internal buffer to return the value. This is similar > > > to how other array-like types are returned with a `Span<>`. > > > > > > This is an API break, but its scope is limited to just `properties::Model`. > > > > I've been a bit in two minds about this, until I realized we already > > require c++17 for applications. > > > > My understanding is that using string_view is no different than > > returning a Span<> when it comes to lifetime of the referenced data, > > so if we're fine with Span<> we're fine with string_view. > > > > And as pointed out by Barnabas, if a user wants a string, it can be > > constructed easily with the same cost as we have today. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > include/libcamera/control_ids.h.in | 1 + > > > include/libcamera/controls.h | 15 ++++++++------- > > > src/apps/cam/capture_script.cpp | 2 +- > > > src/apps/cam/main.cpp | 2 +- > > > src/apps/common/dng_writer.cpp | 2 +- > > > src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > > > src/py/libcamera/py_helpers.cpp | 4 ++-- > > > test/controls/control_value.cpp | 4 ++-- > > > utils/codegen/controls.py | 2 +- > > > 9 files changed, 20 insertions(+), 18 deletions(-) > > > > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > > > index 5d0594c68..41997f79f 100644 > > > --- a/include/libcamera/control_ids.h.in > > > +++ b/include/libcamera/control_ids.h.in > > > @@ -13,6 +13,7 @@ > > > #include <map> > > > #include <stdint.h> > > > #include <string> > > > +#include <string_view> > > > > > > #include <libcamera/controls.h> > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index 2ae4ec3d4..5a707a387 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -13,6 +13,7 @@ > > > #include <set> > > > #include <stdint.h> > > > #include <string> > > > +#include <string_view> > > > #include <unordered_map> > > > #include <vector> > > > > > > @@ -96,7 +97,7 @@ struct control_type<float> { > > > }; > > > > > > template<> > > > -struct control_type<std::string> { > > > +struct control_type<std::string_view> { > > > static constexpr ControlType value = ControlTypeString; > > > static constexpr std::size_t size = 0; > > > }; > > > @@ -138,7 +139,7 @@ public: > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > details::control_type<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > ControlValue(const T &value) > > > : type_(ControlTypeNone), numElements_(0) > > > @@ -148,7 +149,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > @@ -182,7 +183,7 @@ public: > > > > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > T get() const > > > { > > > @@ -193,7 +194,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > @@ -210,7 +211,7 @@ public: > > > > > > #ifndef __DOXYGEN__ > > > template<typename T, std::enable_if_t<!details::is_span<T>::value && > > > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > void set(const T &value) > > > { > > > @@ -219,7 +220,7 @@ public: > > > } > > > > > > template<typename T, std::enable_if_t<details::is_span<T>::value || > > > - std::is_same<std::string, std::remove_cv_t<T>>::value, > > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > > > std::nullptr_t> = nullptr> > > > #else > > > template<typename T> > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > > > index fdf82efc0..bc4c7c3a0 100644 > > > --- a/src/apps/cam/capture_script.cpp > > > +++ b/src/apps/cam/capture_script.cpp > > > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > > > break; > > > } > > > case ControlTypeString: { > > > - value.set<std::string>(repr); > > > + value.set<std::string_view>(repr); > > > break; > > > } > > > default: > > > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > > > index fa266eca6..157d238d7 100644 > > > --- a/src/apps/cam/main.cpp > > > +++ b/src/apps/cam/main.cpp > > > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > > > */ > > > const auto &model = props.get(properties::Model); > > > > Does this need to be a reference now ? > > There isn't any real difference because it's an `std::optional` in both cases > (just with different value type), so I decided not to change it. > > > > > > > if (model) > > > - name = "'" + *model + "' "; > > > + name.append("'").append(*model).append("' "); > > > } > > > > > > name += "(" + camera->id() + ")"; > > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > > > index ac4619511..8d57023e1 100644 > > > --- a/src/apps/common/dng_writer.cpp > > > +++ b/src/apps/common/dng_writer.cpp > > > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > > > const auto &model = cameraProperties.get(properties::Model); > > > > likewise > > Same here. Ack, thanks for clarifying. Please add Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > > Regards, > Barnabás Pőcze > > > > > > if (model) { > > > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > > > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > > > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > > > } > > > > > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > > > index 6b6d0713c..9f25ba091 100644 > > > --- a/src/apps/qcam/cam_select_dialog.cpp > > > +++ b/src/apps/qcam/cam_select_dialog.cpp > > > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > > > cameraLocation_->setText("Unknown"); > > > } > > > > > > - const auto &model = properties.get(libcamera::properties::Model) > > > - .value_or("Unknown"); > > > + const auto model = properties.get(libcamera::properties::Model) > > > + .value_or("Unknown"); > > > > > > - cameraModel_->setText(QString::fromStdString(model)); > > > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > > > } > > > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > > > index 1ad1d4c1a..8c55ef845 100644 > > > --- a/src/py/libcamera/py_helpers.cpp > > > +++ b/src/py/libcamera/py_helpers.cpp > > > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > > > case ControlTypeFloat: > > > return valueOrTuple<float>(cv); > > > case ControlTypeString: > > > - return py::cast(cv.get<std::string>()); > > > + return py::cast(cv.get<std::string_view>()); > > > case ControlTypeSize: { > > > const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > > > return py::cast(v); > > > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > > > case ControlTypeFloat: > > > return controlValueMaybeArray<float>(ob); > > > case ControlTypeString: > > > - return ControlValue(ob.cast<std::string>()); > > > + return ControlValue(ob.cast<std::string_view>()); > > > case ControlTypeRectangle: > > > return controlValueMaybeArray<Rectangle>(ob); > > > case ControlTypeSize: > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > index 5084fd0cf..032050a77 100644 > > > --- a/test/controls/control_value.cpp > > > +++ b/test/controls/control_value.cpp > > > @@ -318,7 +318,7 @@ protected: > > > /* > > > * String type. > > > */ > > > - std::string string{ "libcamera" }; > > > + std::string_view string{ "libcamera" }; > > > value.set(string); > > > > Here's one thing: we now require users to use string_view when setting > > a control, and we anyway end up copying it's content into the > > ControlValue. > > > > Should we provide a set() overload for std::string ? Is it worth it ? > > > > > if (value.isNone() || !value.isArray() || > > > value.type() != ControlTypeString || > > > @@ -327,7 +327,7 @@ protected: > > > return TestFail; > > > } > > > > > > - if (value.get<std::string>() != string) { > > > + if (value.get<std::string_view>() != string) { > > > > For get() instead I think it's fine always returning a view > > > > > cerr << "Control value mismatch after setting to string" << endl; > > > return TestFail; > > > } > > > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > > > index e51610481..9399727bd 100644 > > > --- a/utils/codegen/controls.py > > > +++ b/utils/codegen/controls.py > > > @@ -111,7 +111,7 @@ class Control(object): > > > size = self.__data.get('size') > > > > > > if typ == 'string': > > > - return 'std::string' > > > + return 'std::string_view' > > > > > > if self.__size is None: > > > return typ > > > -- > > > 2.49.0 > > > >
Hi Barnabás, Thanks for the patch. Quoting Barnabás Pőcze (2025-05-01 11:58:18) > When retrieving the value from a `ControlValue` usually one of two > things happen: a small, trivially copyable object is returned by > value; or a view into the internal buffer is provided. This is true > for everything except strings, which are returned in `std::string`, > incurring the overhead of string construction. > > To guarantee no potentially "expensive" copies, use `std::string_view` > pointing to the internal buffer to return the value. This is similar > to how other array-like types are returned with a `Span<>`. > > This is an API break, but its scope is limited to just `properties::Model`. Creating compatibility report ... Binary compatibility: 100% Source compatibility: 99.8% Total binary compatibility problems: 0, warnings: 1 Total source compatibility problems: 2, warnings: 1 Not bad. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 1 + > include/libcamera/controls.h | 15 ++++++++------- > src/apps/cam/capture_script.cpp | 2 +- > src/apps/cam/main.cpp | 2 +- > src/apps/common/dng_writer.cpp | 2 +- > src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > src/py/libcamera/py_helpers.cpp | 4 ++-- > test/controls/control_value.cpp | 4 ++-- > utils/codegen/controls.py | 2 +- > 9 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index 5d0594c68..41997f79f 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -13,6 +13,7 @@ > #include <map> > #include <stdint.h> > #include <string> > +#include <string_view> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 2ae4ec3d4..5a707a387 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -13,6 +13,7 @@ > #include <set> > #include <stdint.h> > #include <string> > +#include <string_view> > #include <unordered_map> > #include <vector> > > @@ -96,7 +97,7 @@ struct control_type<float> { > }; > > template<> > -struct control_type<std::string> { > +struct control_type<std::string_view> { > static constexpr ControlType value = ControlTypeString; > static constexpr std::size_t size = 0; > }; > @@ -138,7 +139,7 @@ public: > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > details::control_type<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > ControlValue(const T &value) > : type_(ControlTypeNone), numElements_(0) > @@ -148,7 +149,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -182,7 +183,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > T get() const > { > @@ -193,7 +194,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -210,7 +211,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > void set(const T &value) > { > @@ -219,7 +220,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > index fdf82efc0..bc4c7c3a0 100644 > --- a/src/apps/cam/capture_script.cpp > +++ b/src/apps/cam/capture_script.cpp > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > break; > } > case ControlTypeString: { > - value.set<std::string>(repr); > + value.set<std::string_view>(repr); > break; > } > default: > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > index fa266eca6..157d238d7 100644 > --- a/src/apps/cam/main.cpp > +++ b/src/apps/cam/main.cpp > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > */ > const auto &model = props.get(properties::Model); > if (model) > - name = "'" + *model + "' "; > + name.append("'").append(*model).append("' "); > } > > name += "(" + camera->id() + ")"; > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > index ac4619511..8d57023e1 100644 > --- a/src/apps/common/dng_writer.cpp > +++ b/src/apps/common/dng_writer.cpp > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > const auto &model = cameraProperties.get(properties::Model); > if (model) { > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > } > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > index 6b6d0713c..9f25ba091 100644 > --- a/src/apps/qcam/cam_select_dialog.cpp > +++ b/src/apps/qcam/cam_select_dialog.cpp > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > cameraLocation_->setText("Unknown"); > } > > - const auto &model = properties.get(libcamera::properties::Model) > - .value_or("Unknown"); > + const auto model = properties.get(libcamera::properties::Model) > + .value_or("Unknown"); > > - cameraModel_->setText(QString::fromStdString(model)); > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > } > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > index 1ad1d4c1a..8c55ef845 100644 > --- a/src/py/libcamera/py_helpers.cpp > +++ b/src/py/libcamera/py_helpers.cpp > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > case ControlTypeFloat: > return valueOrTuple<float>(cv); > case ControlTypeString: > - return py::cast(cv.get<std::string>()); > + return py::cast(cv.get<std::string_view>()); > case ControlTypeSize: { > const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > return py::cast(v); > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > case ControlTypeFloat: > return controlValueMaybeArray<float>(ob); > case ControlTypeString: > - return ControlValue(ob.cast<std::string>()); > + return ControlValue(ob.cast<std::string_view>()); > case ControlTypeRectangle: > return controlValueMaybeArray<Rectangle>(ob); > case ControlTypeSize: > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index 5084fd0cf..032050a77 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -318,7 +318,7 @@ protected: > /* > * String type. > */ > - std::string string{ "libcamera" }; > + std::string_view string{ "libcamera" }; > value.set(string); > if (value.isNone() || !value.isArray() || > value.type() != ControlTypeString || > @@ -327,7 +327,7 @@ protected: > return TestFail; > } > > - if (value.get<std::string>() != string) { > + if (value.get<std::string_view>() != string) { > cerr << "Control value mismatch after setting to string" << endl; > return TestFail; > } > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > index e51610481..9399727bd 100644 > --- a/utils/codegen/controls.py > +++ b/utils/codegen/controls.py > @@ -111,7 +111,7 @@ class Control(object): > size = self.__data.get('size') > > if typ == 'string': > - return 'std::string' > + return 'std::string_view' > > if self.__size is None: > return typ > -- > 2.49.0 >
Hi Barnabás, Thank you for the patch. On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > When retrieving the value from a `ControlValue` usually one of two > things happen: a small, trivially copyable object is returned by > value; or a view into the internal buffer is provided. This is true > for everything except strings, which are returned in `std::string`, > incurring the overhead of string construction. > > To guarantee no potentially "expensive" copies, use `std::string_view` > pointing to the internal buffer to return the value. This is similar > to how other array-like types are returned with a `Span<>`. > > This is an API break, but its scope is limited to just `properties::Model`. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/control_ids.h.in | 1 + > include/libcamera/controls.h | 15 ++++++++------- > src/apps/cam/capture_script.cpp | 2 +- > src/apps/cam/main.cpp | 2 +- > src/apps/common/dng_writer.cpp | 2 +- > src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > src/py/libcamera/py_helpers.cpp | 4 ++-- > test/controls/control_value.cpp | 4 ++-- > utils/codegen/controls.py | 2 +- > 9 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index 5d0594c68..41997f79f 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -13,6 +13,7 @@ > #include <map> > #include <stdint.h> > #include <string> > +#include <string_view> > > #include <libcamera/controls.h> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 2ae4ec3d4..5a707a387 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -13,6 +13,7 @@ > #include <set> > #include <stdint.h> > #include <string> > +#include <string_view> > #include <unordered_map> > #include <vector> > > @@ -96,7 +97,7 @@ struct control_type<float> { > }; > > template<> > -struct control_type<std::string> { > +struct control_type<std::string_view> { > static constexpr ControlType value = ControlTypeString; > static constexpr std::size_t size = 0; > }; > @@ -138,7 +139,7 @@ public: > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > details::control_type<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > ControlValue(const T &value) > : type_(ControlTypeNone), numElements_(0) > @@ -148,7 +149,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -182,7 +183,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > T get() const > { > @@ -193,7 +194,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > @@ -210,7 +211,7 @@ public: > > #ifndef __DOXYGEN__ > template<typename T, std::enable_if_t<!details::is_span<T>::value && > - !std::is_same<std::string, std::remove_cv_t<T>>::value, > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > void set(const T &value) > { > @@ -219,7 +220,7 @@ public: > } > > template<typename T, std::enable_if_t<details::is_span<T>::value || > - std::is_same<std::string, std::remove_cv_t<T>>::value, > + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > std::nullptr_t> = nullptr> > #else > template<typename T> > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > index fdf82efc0..bc4c7c3a0 100644 > --- a/src/apps/cam/capture_script.cpp > +++ b/src/apps/cam/capture_script.cpp > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > break; > } > case ControlTypeString: { > - value.set<std::string>(repr); > + value.set<std::string_view>(repr); > break; > } > default: > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > index fa266eca6..157d238d7 100644 > --- a/src/apps/cam/main.cpp > +++ b/src/apps/cam/main.cpp > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > */ > const auto &model = props.get(properties::Model); > if (model) > - name = "'" + *model + "' "; > + name.append("'").append(*model).append("' "); > } > > name += "(" + camera->id() + ")"; > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > index ac4619511..8d57023e1 100644 > --- a/src/apps/common/dng_writer.cpp > +++ b/src/apps/common/dng_writer.cpp > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > const auto &model = cameraProperties.get(properties::Model); > if (model) { > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); Slightly annoying, but that's an issue with the TIFF library API, it's not our fault. > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > } > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > index 6b6d0713c..9f25ba091 100644 > --- a/src/apps/qcam/cam_select_dialog.cpp > +++ b/src/apps/qcam/cam_select_dialog.cpp > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > cameraLocation_->setText("Unknown"); > } > > - const auto &model = properties.get(libcamera::properties::Model) > - .value_or("Unknown"); > + const auto model = properties.get(libcamera::properties::Model) > + .value_or("Unknown"); I was concerned that this could result in a use-after-free if the argument to value_or() is a temporary std::string, but I think the lifetime of the temporary will be extended to cover the lifetime of model. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > - cameraModel_->setText(QString::fromStdString(model)); > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > } > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > index 1ad1d4c1a..8c55ef845 100644 > --- a/src/py/libcamera/py_helpers.cpp > +++ b/src/py/libcamera/py_helpers.cpp > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > case ControlTypeFloat: > return valueOrTuple<float>(cv); > case ControlTypeString: > - return py::cast(cv.get<std::string>()); > + return py::cast(cv.get<std::string_view>()); > case ControlTypeSize: { > const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > return py::cast(v); > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > case ControlTypeFloat: > return controlValueMaybeArray<float>(ob); > case ControlTypeString: > - return ControlValue(ob.cast<std::string>()); > + return ControlValue(ob.cast<std::string_view>()); > case ControlTypeRectangle: > return controlValueMaybeArray<Rectangle>(ob); > case ControlTypeSize: > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index 5084fd0cf..032050a77 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -318,7 +318,7 @@ protected: > /* > * String type. > */ > - std::string string{ "libcamera" }; > + std::string_view string{ "libcamera" }; > value.set(string); > if (value.isNone() || !value.isArray() || > value.type() != ControlTypeString || > @@ -327,7 +327,7 @@ protected: > return TestFail; > } > > - if (value.get<std::string>() != string) { > + if (value.get<std::string_view>() != string) { > cerr << "Control value mismatch after setting to string" << endl; > return TestFail; > } > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > index e51610481..9399727bd 100644 > --- a/utils/codegen/controls.py > +++ b/utils/codegen/controls.py > @@ -111,7 +111,7 @@ class Control(object): > size = self.__data.get('size') > > if typ == 'string': > - return 'std::string' > + return 'std::string_view' > > if self.__size is None: > return typ
2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: >> When retrieving the value from a `ControlValue` usually one of two >> things happen: a small, trivially copyable object is returned by >> value; or a view into the internal buffer is provided. This is true >> for everything except strings, which are returned in `std::string`, >> incurring the overhead of string construction. >> >> To guarantee no potentially "expensive" copies, use `std::string_view` >> pointing to the internal buffer to return the value. This is similar >> to how other array-like types are returned with a `Span<>`. >> >> This is an API break, but its scope is limited to just `properties::Model`. >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/control_ids.h.in | 1 + >> include/libcamera/controls.h | 15 ++++++++------- >> src/apps/cam/capture_script.cpp | 2 +- >> src/apps/cam/main.cpp | 2 +- >> src/apps/common/dng_writer.cpp | 2 +- >> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- >> src/py/libcamera/py_helpers.cpp | 4 ++-- >> test/controls/control_value.cpp | 4 ++-- >> utils/codegen/controls.py | 2 +- >> 9 files changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in >> index 5d0594c68..41997f79f 100644 >> --- a/include/libcamera/control_ids.h.in >> +++ b/include/libcamera/control_ids.h.in >> @@ -13,6 +13,7 @@ >> #include <map> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> >> #include <libcamera/controls.h> >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 2ae4ec3d4..5a707a387 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -13,6 +13,7 @@ >> #include <set> >> #include <stdint.h> >> #include <string> >> +#include <string_view> >> #include <unordered_map> >> #include <vector> >> >> @@ -96,7 +97,7 @@ struct control_type<float> { >> }; >> >> template<> >> -struct control_type<std::string> { >> +struct control_type<std::string_view> { >> static constexpr ControlType value = ControlTypeString; >> static constexpr std::size_t size = 0; >> }; >> @@ -138,7 +139,7 @@ public: >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> details::control_type<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> ControlValue(const T &value) >> : type_(ControlTypeNone), numElements_(0) >> @@ -148,7 +149,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -182,7 +183,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> T get() const >> { >> @@ -193,7 +194,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> @@ -210,7 +211,7 @@ public: >> >> #ifndef __DOXYGEN__ >> template<typename T, std::enable_if_t<!details::is_span<T>::value && >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> void set(const T &value) >> { >> @@ -219,7 +220,7 @@ public: >> } >> >> template<typename T, std::enable_if_t<details::is_span<T>::value || >> - std::is_same<std::string, std::remove_cv_t<T>>::value, >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >> std::nullptr_t> = nullptr> >> #else >> template<typename T> >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp >> index fdf82efc0..bc4c7c3a0 100644 >> --- a/src/apps/cam/capture_script.cpp >> +++ b/src/apps/cam/capture_script.cpp >> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, >> break; >> } >> case ControlTypeString: { >> - value.set<std::string>(repr); >> + value.set<std::string_view>(repr); >> break; >> } >> default: >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp >> index fa266eca6..157d238d7 100644 >> --- a/src/apps/cam/main.cpp >> +++ b/src/apps/cam/main.cpp >> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) >> */ >> const auto &model = props.get(properties::Model); >> if (model) >> - name = "'" + *model + "' "; >> + name.append("'").append(*model).append("' "); >> } >> >> name += "(" + camera->id() + ")"; >> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp >> index ac4619511..8d57023e1 100644 >> --- a/src/apps/common/dng_writer.cpp >> +++ b/src/apps/common/dng_writer.cpp >> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, >> >> const auto &model = cameraProperties.get(properties::Model); >> if (model) { >> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); >> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > > Slightly annoying, but that's an issue with the TIFF library API, it's > not our fault. > >> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ >> } >> >> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp >> index 6b6d0713c..9f25ba091 100644 >> --- a/src/apps/qcam/cam_select_dialog.cpp >> +++ b/src/apps/qcam/cam_select_dialog.cpp >> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) >> cameraLocation_->setText("Unknown"); >> } >> >> - const auto &model = properties.get(libcamera::properties::Model) >> - .value_or("Unknown"); >> + const auto model = properties.get(libcamera::properties::Model) >> + .value_or("Unknown"); > > I was concerned that this could result in a use-after-free if the > argument to value_or() is a temporary std::string, but I think the I am afraid that's exactly what would happen, an `std::string_view` is returned to the temporary `std::string`. So that would be a use-after-free in some cases. > lifetime of the temporary will be extended to cover the lifetime of > model. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> >> - cameraModel_->setText(QString::fromStdString(model)); >> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); >> } >> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp >> index 1ad1d4c1a..8c55ef845 100644 >> --- a/src/py/libcamera/py_helpers.cpp >> +++ b/src/py/libcamera/py_helpers.cpp >> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) >> case ControlTypeFloat: >> return valueOrTuple<float>(cv); >> case ControlTypeString: >> - return py::cast(cv.get<std::string>()); >> + return py::cast(cv.get<std::string_view>()); >> case ControlTypeSize: { >> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); >> return py::cast(v); >> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) >> case ControlTypeFloat: >> return controlValueMaybeArray<float>(ob); >> case ControlTypeString: >> - return ControlValue(ob.cast<std::string>()); >> + return ControlValue(ob.cast<std::string_view>()); >> case ControlTypeRectangle: >> return controlValueMaybeArray<Rectangle>(ob); >> case ControlTypeSize: >> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp >> index 5084fd0cf..032050a77 100644 >> --- a/test/controls/control_value.cpp >> +++ b/test/controls/control_value.cpp >> @@ -318,7 +318,7 @@ protected: >> /* >> * String type. >> */ >> - std::string string{ "libcamera" }; >> + std::string_view string{ "libcamera" }; >> value.set(string); >> if (value.isNone() || !value.isArray() || >> value.type() != ControlTypeString || >> @@ -327,7 +327,7 @@ protected: >> return TestFail; >> } >> >> - if (value.get<std::string>() != string) { >> + if (value.get<std::string_view>() != string) { >> cerr << "Control value mismatch after setting to string" << endl; >> return TestFail; >> } >> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py >> index e51610481..9399727bd 100644 >> --- a/utils/codegen/controls.py >> +++ b/utils/codegen/controls.py >> @@ -111,7 +111,7 @@ class Control(object): >> size = self.__data.get('size') >> >> if typ == 'string': >> - return 'std::string' >> + return 'std::string_view' >> >> if self.__size is None: >> return typ >
On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote: > 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta: > > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > >> When retrieving the value from a `ControlValue` usually one of two > >> things happen: a small, trivially copyable object is returned by > >> value; or a view into the internal buffer is provided. This is true > >> for everything except strings, which are returned in `std::string`, > >> incurring the overhead of string construction. > >> > >> To guarantee no potentially "expensive" copies, use `std::string_view` > >> pointing to the internal buffer to return the value. This is similar > >> to how other array-like types are returned with a `Span<>`. > >> > >> This is an API break, but its scope is limited to just `properties::Model`. > >> > >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> include/libcamera/control_ids.h.in | 1 + > >> include/libcamera/controls.h | 15 ++++++++------- > >> src/apps/cam/capture_script.cpp | 2 +- > >> src/apps/cam/main.cpp | 2 +- > >> src/apps/common/dng_writer.cpp | 2 +- > >> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > >> src/py/libcamera/py_helpers.cpp | 4 ++-- > >> test/controls/control_value.cpp | 4 ++-- > >> utils/codegen/controls.py | 2 +- > >> 9 files changed, 20 insertions(+), 18 deletions(-) > >> > >> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > >> index 5d0594c68..41997f79f 100644 > >> --- a/include/libcamera/control_ids.h.in > >> +++ b/include/libcamera/control_ids.h.in > >> @@ -13,6 +13,7 @@ > >> #include <map> > >> #include <stdint.h> > >> #include <string> > >> +#include <string_view> > >> > >> #include <libcamera/controls.h> > >> > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >> index 2ae4ec3d4..5a707a387 100644 > >> --- a/include/libcamera/controls.h > >> +++ b/include/libcamera/controls.h > >> @@ -13,6 +13,7 @@ > >> #include <set> > >> #include <stdint.h> > >> #include <string> > >> +#include <string_view> > >> #include <unordered_map> > >> #include <vector> > >> > >> @@ -96,7 +97,7 @@ struct control_type<float> { > >> }; > >> > >> template<> > >> -struct control_type<std::string> { > >> +struct control_type<std::string_view> { > >> static constexpr ControlType value = ControlTypeString; > >> static constexpr std::size_t size = 0; > >> }; > >> @@ -138,7 +139,7 @@ public: > >> #ifndef __DOXYGEN__ > >> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >> details::control_type<T>::value && > >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> ControlValue(const T &value) > >> : type_(ControlTypeNone), numElements_(0) > >> @@ -148,7 +149,7 @@ public: > >> } > >> > >> template<typename T, std::enable_if_t<details::is_span<T>::value || > >> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> #else > >> template<typename T> > >> @@ -182,7 +183,7 @@ public: > >> > >> #ifndef __DOXYGEN__ > >> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> T get() const > >> { > >> @@ -193,7 +194,7 @@ public: > >> } > >> > >> template<typename T, std::enable_if_t<details::is_span<T>::value || > >> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> #else > >> template<typename T> > >> @@ -210,7 +211,7 @@ public: > >> > >> #ifndef __DOXYGEN__ > >> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> void set(const T &value) > >> { > >> @@ -219,7 +220,7 @@ public: > >> } > >> > >> template<typename T, std::enable_if_t<details::is_span<T>::value || > >> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >> std::nullptr_t> = nullptr> > >> #else > >> template<typename T> > >> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > >> index fdf82efc0..bc4c7c3a0 100644 > >> --- a/src/apps/cam/capture_script.cpp > >> +++ b/src/apps/cam/capture_script.cpp > >> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > >> break; > >> } > >> case ControlTypeString: { > >> - value.set<std::string>(repr); > >> + value.set<std::string_view>(repr); > >> break; > >> } > >> default: > >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > >> index fa266eca6..157d238d7 100644 > >> --- a/src/apps/cam/main.cpp > >> +++ b/src/apps/cam/main.cpp > >> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > >> */ > >> const auto &model = props.get(properties::Model); > >> if (model) > >> - name = "'" + *model + "' "; > >> + name.append("'").append(*model).append("' "); > >> } > >> > >> name += "(" + camera->id() + ")"; > >> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > >> index ac4619511..8d57023e1 100644 > >> --- a/src/apps/common/dng_writer.cpp > >> +++ b/src/apps/common/dng_writer.cpp > >> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > >> > >> const auto &model = cameraProperties.get(properties::Model); > >> if (model) { > >> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > >> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > > > > Slightly annoying, but that's an issue with the TIFF library API, it's > > not our fault. > > > >> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > >> } > >> > >> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > >> index 6b6d0713c..9f25ba091 100644 > >> --- a/src/apps/qcam/cam_select_dialog.cpp > >> +++ b/src/apps/qcam/cam_select_dialog.cpp > >> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > >> cameraLocation_->setText("Unknown"); > >> } > >> > >> - const auto &model = properties.get(libcamera::properties::Model) > >> - .value_or("Unknown"); > >> + const auto model = properties.get(libcamera::properties::Model) > >> + .value_or("Unknown"); > > > > I was concerned that this could result in a use-after-free if the > > argument to value_or() is a temporary std::string, but I think the > > I am afraid that's exactly what would happen, an `std::string_view` is > returned to the temporary `std::string`. So that would be a use-after-free > in some cases. valgrind didn't catch any issue. Isn't the lifetime of temporary objects extended in this case ? > > lifetime of the temporary will be extended to cover the lifetime of > > model. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> > >> - cameraModel_->setText(QString::fromStdString(model)); > >> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > >> } > >> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > >> index 1ad1d4c1a..8c55ef845 100644 > >> --- a/src/py/libcamera/py_helpers.cpp > >> +++ b/src/py/libcamera/py_helpers.cpp > >> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > >> case ControlTypeFloat: > >> return valueOrTuple<float>(cv); > >> case ControlTypeString: > >> - return py::cast(cv.get<std::string>()); > >> + return py::cast(cv.get<std::string_view>()); > >> case ControlTypeSize: { > >> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > >> return py::cast(v); > >> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > >> case ControlTypeFloat: > >> return controlValueMaybeArray<float>(ob); > >> case ControlTypeString: > >> - return ControlValue(ob.cast<std::string>()); > >> + return ControlValue(ob.cast<std::string_view>()); > >> case ControlTypeRectangle: > >> return controlValueMaybeArray<Rectangle>(ob); > >> case ControlTypeSize: > >> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > >> index 5084fd0cf..032050a77 100644 > >> --- a/test/controls/control_value.cpp > >> +++ b/test/controls/control_value.cpp > >> @@ -318,7 +318,7 @@ protected: > >> /* > >> * String type. > >> */ > >> - std::string string{ "libcamera" }; > >> + std::string_view string{ "libcamera" }; > >> value.set(string); > >> if (value.isNone() || !value.isArray() || > >> value.type() != ControlTypeString || > >> @@ -327,7 +327,7 @@ protected: > >> return TestFail; > >> } > >> > >> - if (value.get<std::string>() != string) { > >> + if (value.get<std::string_view>() != string) { > >> cerr << "Control value mismatch after setting to string" << endl; > >> return TestFail; > >> } > >> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > >> index e51610481..9399727bd 100644 > >> --- a/utils/codegen/controls.py > >> +++ b/utils/codegen/controls.py > >> @@ -111,7 +111,7 @@ class Control(object): > >> size = self.__data.get('size') > >> > >> if typ == 'string': > >> - return 'std::string' > >> + return 'std::string_view' > >> > >> if self.__size is None: > >> return typ
2025. 10. 03. 18:58 keltezéssel, Laurent Pinchart írta: > On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote: >> 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta: >>> On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: >>>> When retrieving the value from a `ControlValue` usually one of two >>>> things happen: a small, trivially copyable object is returned by >>>> value; or a view into the internal buffer is provided. This is true >>>> for everything except strings, which are returned in `std::string`, >>>> incurring the overhead of string construction. >>>> >>>> To guarantee no potentially "expensive" copies, use `std::string_view` >>>> pointing to the internal buffer to return the value. This is similar >>>> to how other array-like types are returned with a `Span<>`. >>>> >>>> This is an API break, but its scope is limited to just `properties::Model`. >>>> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> include/libcamera/control_ids.h.in | 1 + >>>> include/libcamera/controls.h | 15 ++++++++------- >>>> src/apps/cam/capture_script.cpp | 2 +- >>>> src/apps/cam/main.cpp | 2 +- >>>> src/apps/common/dng_writer.cpp | 2 +- >>>> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- >>>> src/py/libcamera/py_helpers.cpp | 4 ++-- >>>> test/controls/control_value.cpp | 4 ++-- >>>> utils/codegen/controls.py | 2 +- >>>> 9 files changed, 20 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in >>>> index 5d0594c68..41997f79f 100644 >>>> --- a/include/libcamera/control_ids.h.in >>>> +++ b/include/libcamera/control_ids.h.in >>>> @@ -13,6 +13,7 @@ >>>> #include <map> >>>> #include <stdint.h> >>>> #include <string> >>>> +#include <string_view> >>>> >>>> #include <libcamera/controls.h> >>>> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>>> index 2ae4ec3d4..5a707a387 100644 >>>> --- a/include/libcamera/controls.h >>>> +++ b/include/libcamera/controls.h >>>> @@ -13,6 +13,7 @@ >>>> #include <set> >>>> #include <stdint.h> >>>> #include <string> >>>> +#include <string_view> >>>> #include <unordered_map> >>>> #include <vector> >>>> >>>> @@ -96,7 +97,7 @@ struct control_type<float> { >>>> }; >>>> >>>> template<> >>>> -struct control_type<std::string> { >>>> +struct control_type<std::string_view> { >>>> static constexpr ControlType value = ControlTypeString; >>>> static constexpr std::size_t size = 0; >>>> }; >>>> @@ -138,7 +139,7 @@ public: >>>> #ifndef __DOXYGEN__ >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && >>>> details::control_type<T>::value && >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> ControlValue(const T &value) >>>> : type_(ControlTypeNone), numElements_(0) >>>> @@ -148,7 +149,7 @@ public: >>>> } >>>> >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> #else >>>> template<typename T> >>>> @@ -182,7 +183,7 @@ public: >>>> >>>> #ifndef __DOXYGEN__ >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> T get() const >>>> { >>>> @@ -193,7 +194,7 @@ public: >>>> } >>>> >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> #else >>>> template<typename T> >>>> @@ -210,7 +211,7 @@ public: >>>> >>>> #ifndef __DOXYGEN__ >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> void set(const T &value) >>>> { >>>> @@ -219,7 +220,7 @@ public: >>>> } >>>> >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, >>>> std::nullptr_t> = nullptr> >>>> #else >>>> template<typename T> >>>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp >>>> index fdf82efc0..bc4c7c3a0 100644 >>>> --- a/src/apps/cam/capture_script.cpp >>>> +++ b/src/apps/cam/capture_script.cpp >>>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, >>>> break; >>>> } >>>> case ControlTypeString: { >>>> - value.set<std::string>(repr); >>>> + value.set<std::string_view>(repr); >>>> break; >>>> } >>>> default: >>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp >>>> index fa266eca6..157d238d7 100644 >>>> --- a/src/apps/cam/main.cpp >>>> +++ b/src/apps/cam/main.cpp >>>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) >>>> */ >>>> const auto &model = props.get(properties::Model); >>>> if (model) >>>> - name = "'" + *model + "' "; >>>> + name.append("'").append(*model).append("' "); >>>> } >>>> >>>> name += "(" + camera->id() + ")"; >>>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp >>>> index ac4619511..8d57023e1 100644 >>>> --- a/src/apps/common/dng_writer.cpp >>>> +++ b/src/apps/common/dng_writer.cpp >>>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, >>>> >>>> const auto &model = cameraProperties.get(properties::Model); >>>> if (model) { >>>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); >>>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); >>> >>> Slightly annoying, but that's an issue with the TIFF library API, it's >>> not our fault. >>> >>>> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ >>>> } >>>> >>>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp >>>> index 6b6d0713c..9f25ba091 100644 >>>> --- a/src/apps/qcam/cam_select_dialog.cpp >>>> +++ b/src/apps/qcam/cam_select_dialog.cpp >>>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) >>>> cameraLocation_->setText("Unknown"); >>>> } >>>> >>>> - const auto &model = properties.get(libcamera::properties::Model) >>>> - .value_or("Unknown"); >>>> + const auto model = properties.get(libcamera::properties::Model) >>>> + .value_or("Unknown"); >>> >>> I was concerned that this could result in a use-after-free if the >>> argument to value_or() is a temporary std::string, but I think the >> >> I am afraid that's exactly what would happen, an `std::string_view` is >> returned to the temporary `std::string`. So that would be a use-after-free >> in some cases. > > valgrind didn't catch any issue. Isn't the lifetime of temporary objects > extended in this case ? The return type of this `value_or()` call will be `std::string_view`, the underlying temporary object passed as argument will be destroyed. https://gcc.godbolt.org/z/Mb3qKT1jK This issue is generally present for array like controls: auto x = request->metadata().get(controls::ColourGains).value_or(std::vector<float> { 1.f, 1.f }); x[0] // UAF if `controls::ColourGains` is not in metadata or similar. > >>> lifetime of the temporary will be extended to cover the lifetime of >>> model. >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> >>>> - cameraModel_->setText(QString::fromStdString(model)); >>>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); >>>> } >>>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp >>>> index 1ad1d4c1a..8c55ef845 100644 >>>> --- a/src/py/libcamera/py_helpers.cpp >>>> +++ b/src/py/libcamera/py_helpers.cpp >>>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) >>>> case ControlTypeFloat: >>>> return valueOrTuple<float>(cv); >>>> case ControlTypeString: >>>> - return py::cast(cv.get<std::string>()); >>>> + return py::cast(cv.get<std::string_view>()); >>>> case ControlTypeSize: { >>>> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); >>>> return py::cast(v); >>>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) >>>> case ControlTypeFloat: >>>> return controlValueMaybeArray<float>(ob); >>>> case ControlTypeString: >>>> - return ControlValue(ob.cast<std::string>()); >>>> + return ControlValue(ob.cast<std::string_view>()); >>>> case ControlTypeRectangle: >>>> return controlValueMaybeArray<Rectangle>(ob); >>>> case ControlTypeSize: >>>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp >>>> index 5084fd0cf..032050a77 100644 >>>> --- a/test/controls/control_value.cpp >>>> +++ b/test/controls/control_value.cpp >>>> @@ -318,7 +318,7 @@ protected: >>>> /* >>>> * String type. >>>> */ >>>> - std::string string{ "libcamera" }; >>>> + std::string_view string{ "libcamera" }; >>>> value.set(string); >>>> if (value.isNone() || !value.isArray() || >>>> value.type() != ControlTypeString || >>>> @@ -327,7 +327,7 @@ protected: >>>> return TestFail; >>>> } >>>> >>>> - if (value.get<std::string>() != string) { >>>> + if (value.get<std::string_view>() != string) { >>>> cerr << "Control value mismatch after setting to string" << endl; >>>> return TestFail; >>>> } >>>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py >>>> index e51610481..9399727bd 100644 >>>> --- a/utils/codegen/controls.py >>>> +++ b/utils/codegen/controls.py >>>> @@ -111,7 +111,7 @@ class Control(object): >>>> size = self.__data.get('size') >>>> >>>> if typ == 'string': >>>> - return 'std::string' >>>> + return 'std::string_view' >>>> >>>> if self.__size is None: >>>> return typ >
On Fri, Oct 03, 2025 at 07:08:35PM +0200, Barnabás Pőcze wrote: > 2025. 10. 03. 18:58 keltezéssel, Laurent Pinchart írta: > > On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote: > >> 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta: > >>> On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote: > >>>> When retrieving the value from a `ControlValue` usually one of two > >>>> things happen: a small, trivially copyable object is returned by > >>>> value; or a view into the internal buffer is provided. This is true > >>>> for everything except strings, which are returned in `std::string`, > >>>> incurring the overhead of string construction. > >>>> > >>>> To guarantee no potentially "expensive" copies, use `std::string_view` > >>>> pointing to the internal buffer to return the value. This is similar > >>>> to how other array-like types are returned with a `Span<>`. > >>>> > >>>> This is an API break, but its scope is limited to just `properties::Model`. > >>>> > >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>> --- > >>>> include/libcamera/control_ids.h.in | 1 + > >>>> include/libcamera/controls.h | 15 ++++++++------- > >>>> src/apps/cam/capture_script.cpp | 2 +- > >>>> src/apps/cam/main.cpp | 2 +- > >>>> src/apps/common/dng_writer.cpp | 2 +- > >>>> src/apps/qcam/cam_select_dialog.cpp | 6 +++--- > >>>> src/py/libcamera/py_helpers.cpp | 4 ++-- > >>>> test/controls/control_value.cpp | 4 ++-- > >>>> utils/codegen/controls.py | 2 +- > >>>> 9 files changed, 20 insertions(+), 18 deletions(-) > >>>> > >>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > >>>> index 5d0594c68..41997f79f 100644 > >>>> --- a/include/libcamera/control_ids.h.in > >>>> +++ b/include/libcamera/control_ids.h.in > >>>> @@ -13,6 +13,7 @@ > >>>> #include <map> > >>>> #include <stdint.h> > >>>> #include <string> > >>>> +#include <string_view> > >>>> > >>>> #include <libcamera/controls.h> > >>>> > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>>> index 2ae4ec3d4..5a707a387 100644 > >>>> --- a/include/libcamera/controls.h > >>>> +++ b/include/libcamera/controls.h > >>>> @@ -13,6 +13,7 @@ > >>>> #include <set> > >>>> #include <stdint.h> > >>>> #include <string> > >>>> +#include <string_view> > >>>> #include <unordered_map> > >>>> #include <vector> > >>>> > >>>> @@ -96,7 +97,7 @@ struct control_type<float> { > >>>> }; > >>>> > >>>> template<> > >>>> -struct control_type<std::string> { > >>>> +struct control_type<std::string_view> { > >>>> static constexpr ControlType value = ControlTypeString; > >>>> static constexpr std::size_t size = 0; > >>>> }; > >>>> @@ -138,7 +139,7 @@ public: > >>>> #ifndef __DOXYGEN__ > >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >>>> details::control_type<T>::value && > >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> ControlValue(const T &value) > >>>> : type_(ControlTypeNone), numElements_(0) > >>>> @@ -148,7 +149,7 @@ public: > >>>> } > >>>> > >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || > >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> #else > >>>> template<typename T> > >>>> @@ -182,7 +183,7 @@ public: > >>>> > >>>> #ifndef __DOXYGEN__ > >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> T get() const > >>>> { > >>>> @@ -193,7 +194,7 @@ public: > >>>> } > >>>> > >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || > >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> #else > >>>> template<typename T> > >>>> @@ -210,7 +211,7 @@ public: > >>>> > >>>> #ifndef __DOXYGEN__ > >>>> template<typename T, std::enable_if_t<!details::is_span<T>::value && > >>>> - !std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> void set(const T &value) > >>>> { > >>>> @@ -219,7 +220,7 @@ public: > >>>> } > >>>> > >>>> template<typename T, std::enable_if_t<details::is_span<T>::value || > >>>> - std::is_same<std::string, std::remove_cv_t<T>>::value, > >>>> + std::is_same<std::string_view, std::remove_cv_t<T>>::value, > >>>> std::nullptr_t> = nullptr> > >>>> #else > >>>> template<typename T> > >>>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp > >>>> index fdf82efc0..bc4c7c3a0 100644 > >>>> --- a/src/apps/cam/capture_script.cpp > >>>> +++ b/src/apps/cam/capture_script.cpp > >>>> @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, > >>>> break; > >>>> } > >>>> case ControlTypeString: { > >>>> - value.set<std::string>(repr); > >>>> + value.set<std::string_view>(repr); > >>>> break; > >>>> } > >>>> default: > >>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp > >>>> index fa266eca6..157d238d7 100644 > >>>> --- a/src/apps/cam/main.cpp > >>>> +++ b/src/apps/cam/main.cpp > >>>> @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) > >>>> */ > >>>> const auto &model = props.get(properties::Model); > >>>> if (model) > >>>> - name = "'" + *model + "' "; > >>>> + name.append("'").append(*model).append("' "); > >>>> } > >>>> > >>>> name += "(" + camera->id() + ")"; > >>>> diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > >>>> index ac4619511..8d57023e1 100644 > >>>> --- a/src/apps/common/dng_writer.cpp > >>>> +++ b/src/apps/common/dng_writer.cpp > >>>> @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > >>>> > >>>> const auto &model = cameraProperties.get(properties::Model); > >>>> if (model) { > >>>> - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > >>>> + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); > >>> > >>> Slightly annoying, but that's an issue with the TIFF library API, it's > >>> not our fault. > >>> > >>>> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > >>>> } > >>>> > >>>> diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp > >>>> index 6b6d0713c..9f25ba091 100644 > >>>> --- a/src/apps/qcam/cam_select_dialog.cpp > >>>> +++ b/src/apps/qcam/cam_select_dialog.cpp > >>>> @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) > >>>> cameraLocation_->setText("Unknown"); > >>>> } > >>>> > >>>> - const auto &model = properties.get(libcamera::properties::Model) > >>>> - .value_or("Unknown"); > >>>> + const auto model = properties.get(libcamera::properties::Model) > >>>> + .value_or("Unknown"); > >>> > >>> I was concerned that this could result in a use-after-free if the > >>> argument to value_or() is a temporary std::string, but I think the > >> > >> I am afraid that's exactly what would happen, an `std::string_view` is > >> returned to the temporary `std::string`. So that would be a use-after-free > >> in some cases. > > > > valgrind didn't catch any issue. Isn't the lifetime of temporary objects > > extended in this case ? > > The return type of this `value_or()` call will be `std::string_view`, > the underlying temporary object passed as argument will be destroyed. > > https://gcc.godbolt.org/z/Mb3qKT1jK > > This issue is generally present for array like controls: > > auto x = request->metadata().get(controls::ColourGains).value_or(std::vector<float> { 1.f, 1.f }); > x[0] // UAF if `controls::ColourGains` is not in metadata > > or similar. You're right, I somehow extended the lifetime extension rules of range-based for loops to all expressions :-/ It's not a new issues so I don't think it needs to be solved right now, but it's something to consider for future revisions of this API. > >>> lifetime of the temporary will be extended to cover the lifetime of > >>> model. > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>>> > >>>> - cameraModel_->setText(QString::fromStdString(model)); > >>>> + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); > >>>> } > >>>> diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp > >>>> index 1ad1d4c1a..8c55ef845 100644 > >>>> --- a/src/py/libcamera/py_helpers.cpp > >>>> +++ b/src/py/libcamera/py_helpers.cpp > >>>> @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) > >>>> case ControlTypeFloat: > >>>> return valueOrTuple<float>(cv); > >>>> case ControlTypeString: > >>>> - return py::cast(cv.get<std::string>()); > >>>> + return py::cast(cv.get<std::string_view>()); > >>>> case ControlTypeSize: { > >>>> const Size *v = reinterpret_cast<const Size *>(cv.data().data()); > >>>> return py::cast(v); > >>>> @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) > >>>> case ControlTypeFloat: > >>>> return controlValueMaybeArray<float>(ob); > >>>> case ControlTypeString: > >>>> - return ControlValue(ob.cast<std::string>()); > >>>> + return ControlValue(ob.cast<std::string_view>()); > >>>> case ControlTypeRectangle: > >>>> return controlValueMaybeArray<Rectangle>(ob); > >>>> case ControlTypeSize: > >>>> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > >>>> index 5084fd0cf..032050a77 100644 > >>>> --- a/test/controls/control_value.cpp > >>>> +++ b/test/controls/control_value.cpp > >>>> @@ -318,7 +318,7 @@ protected: > >>>> /* > >>>> * String type. > >>>> */ > >>>> - std::string string{ "libcamera" }; > >>>> + std::string_view string{ "libcamera" }; > >>>> value.set(string); > >>>> if (value.isNone() || !value.isArray() || > >>>> value.type() != ControlTypeString || > >>>> @@ -327,7 +327,7 @@ protected: > >>>> return TestFail; > >>>> } > >>>> > >>>> - if (value.get<std::string>() != string) { > >>>> + if (value.get<std::string_view>() != string) { > >>>> cerr << "Control value mismatch after setting to string" << endl; > >>>> return TestFail; > >>>> } > >>>> diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py > >>>> index e51610481..9399727bd 100644 > >>>> --- a/utils/codegen/controls.py > >>>> +++ b/utils/codegen/controls.py > >>>> @@ -111,7 +111,7 @@ class Control(object): > >>>> size = self.__data.get('size') > >>>> > >>>> if typ == 'string': > >>>> - return 'std::string' > >>>> + return 'std::string_view' > >>>> > >>>> if self.__size is None: > >>>> return typ
diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in index 5d0594c68..41997f79f 100644 --- a/include/libcamera/control_ids.h.in +++ b/include/libcamera/control_ids.h.in @@ -13,6 +13,7 @@ #include <map> #include <stdint.h> #include <string> +#include <string_view> #include <libcamera/controls.h> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 2ae4ec3d4..5a707a387 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -13,6 +13,7 @@ #include <set> #include <stdint.h> #include <string> +#include <string_view> #include <unordered_map> #include <vector> @@ -96,7 +97,7 @@ struct control_type<float> { }; template<> -struct control_type<std::string> { +struct control_type<std::string_view> { static constexpr ControlType value = ControlTypeString; static constexpr std::size_t size = 0; }; @@ -138,7 +139,7 @@ public: #ifndef __DOXYGEN__ template<typename T, std::enable_if_t<!details::is_span<T>::value && details::control_type<T>::value && - !std::is_same<std::string, std::remove_cv_t<T>>::value, + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> ControlValue(const T &value) : type_(ControlTypeNone), numElements_(0) @@ -148,7 +149,7 @@ public: } template<typename T, std::enable_if_t<details::is_span<T>::value || - std::is_same<std::string, std::remove_cv_t<T>>::value, + std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> #else template<typename T> @@ -182,7 +183,7 @@ public: #ifndef __DOXYGEN__ template<typename T, std::enable_if_t<!details::is_span<T>::value && - !std::is_same<std::string, std::remove_cv_t<T>>::value, + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> T get() const { @@ -193,7 +194,7 @@ public: } template<typename T, std::enable_if_t<details::is_span<T>::value || - std::is_same<std::string, std::remove_cv_t<T>>::value, + std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> #else template<typename T> @@ -210,7 +211,7 @@ public: #ifndef __DOXYGEN__ template<typename T, std::enable_if_t<!details::is_span<T>::value && - !std::is_same<std::string, std::remove_cv_t<T>>::value, + !std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> void set(const T &value) { @@ -219,7 +220,7 @@ public: } template<typename T, std::enable_if_t<details::is_span<T>::value || - std::is_same<std::string, std::remove_cv_t<T>>::value, + std::is_same<std::string_view, std::remove_cv_t<T>>::value, std::nullptr_t> = nullptr> #else template<typename T> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp index fdf82efc0..bc4c7c3a0 100644 --- a/src/apps/cam/capture_script.cpp +++ b/src/apps/cam/capture_script.cpp @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id, break; } case ControlTypeString: { - value.set<std::string>(repr); + value.set<std::string_view>(repr); break; } default: diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp index fa266eca6..157d238d7 100644 --- a/src/apps/cam/main.cpp +++ b/src/apps/cam/main.cpp @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera) */ const auto &model = props.get(properties::Model); if (model) - name = "'" + *model + "' "; + name.append("'").append(*model).append("' "); } name += "(" + camera->id() + ")"; diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp index ac4619511..8d57023e1 100644 --- a/src/apps/common/dng_writer.cpp +++ b/src/apps/common/dng_writer.cpp @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, const auto &model = cameraProperties.get(properties::Model); if (model) { - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str()); /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ } diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp index 6b6d0713c..9f25ba091 100644 --- a/src/apps/qcam/cam_select_dialog.cpp +++ b/src/apps/qcam/cam_select_dialog.cpp @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId) cameraLocation_->setText("Unknown"); } - const auto &model = properties.get(libcamera::properties::Model) - .value_or("Unknown"); + const auto model = properties.get(libcamera::properties::Model) + .value_or("Unknown"); - cameraModel_->setText(QString::fromStdString(model)); + cameraModel_->setText(QString::fromUtf8(model.data(), model.length())); } diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp index 1ad1d4c1a..8c55ef845 100644 --- a/src/py/libcamera/py_helpers.cpp +++ b/src/py/libcamera/py_helpers.cpp @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv) case ControlTypeFloat: return valueOrTuple<float>(cv); case ControlTypeString: - return py::cast(cv.get<std::string>()); + return py::cast(cv.get<std::string_view>()); case ControlTypeSize: { const Size *v = reinterpret_cast<const Size *>(cv.data().data()); return py::cast(v); @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type) case ControlTypeFloat: return controlValueMaybeArray<float>(ob); case ControlTypeString: - return ControlValue(ob.cast<std::string>()); + return ControlValue(ob.cast<std::string_view>()); case ControlTypeRectangle: return controlValueMaybeArray<Rectangle>(ob); case ControlTypeSize: diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp index 5084fd0cf..032050a77 100644 --- a/test/controls/control_value.cpp +++ b/test/controls/control_value.cpp @@ -318,7 +318,7 @@ protected: /* * String type. */ - std::string string{ "libcamera" }; + std::string_view string{ "libcamera" }; value.set(string); if (value.isNone() || !value.isArray() || value.type() != ControlTypeString || @@ -327,7 +327,7 @@ protected: return TestFail; } - if (value.get<std::string>() != string) { + if (value.get<std::string_view>() != string) { cerr << "Control value mismatch after setting to string" << endl; return TestFail; } diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py index e51610481..9399727bd 100644 --- a/utils/codegen/controls.py +++ b/utils/codegen/controls.py @@ -111,7 +111,7 @@ class Control(object): size = self.__data.get('size') if typ == 'string': - return 'std::string' + return 'std::string_view' if self.__size is None: return typ
When retrieving the value from a `ControlValue` usually one of two things happen: a small, trivially copyable object is returned by value; or a view into the internal buffer is provided. This is true for everything except strings, which are returned in `std::string`, incurring the overhead of string construction. To guarantee no potentially "expensive" copies, use `std::string_view` pointing to the internal buffer to return the value. This is similar to how other array-like types are returned with a `Span<>`. This is an API break, but its scope is limited to just `properties::Model`. Bug: https://bugs.libcamera.org/show_bug.cgi?id=256 Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/control_ids.h.in | 1 + include/libcamera/controls.h | 15 ++++++++------- src/apps/cam/capture_script.cpp | 2 +- src/apps/cam/main.cpp | 2 +- src/apps/common/dng_writer.cpp | 2 +- src/apps/qcam/cam_select_dialog.cpp | 6 +++--- src/py/libcamera/py_helpers.cpp | 4 ++-- test/controls/control_value.cpp | 4 ++-- utils/codegen/controls.py | 2 +- 9 files changed, 20 insertions(+), 18 deletions(-)