| Message ID | 20250501095818.3996419-4-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| 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 >
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(-)