| Message ID | 20250422124751.30409-3-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-04-22 13:47:51) > 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`. This breaks in CI at the build-history stage: https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 I wonder, are we missing a dependency declaration somewhere that should have otherwise caused something to get rebuilt? All of the other builds seemed to have worked on that pipeline ? > > 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 4bfe9615c..275f45200 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 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-04-22 13:47:51) >> 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`. > > This breaks in CI at the build-history stage: > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 > > I wonder, are we missing a dependency declaration somewhere that should > have otherwise caused something to get rebuilt? All of the other builds > seemed to have worked on that pipeline ? Yes indeed, `controls.py` is changed, but that is not considered by meson, so it does not regenerate the cpp files because neither the template nor `gen-controls.py`, etc. has changed. I am not sure how it could be best addressed, there is `depend_files` where `controls.py` could be specified, but I don't think it's too convenient. Or there is the `modulefinder` thing in the standard library: >>> os.getcwd() '/libcamera/utils/codegen' >>> import modulefinder >>> m = modulefinder.ModuleFinder() >>> m.run_script("./gen-controls.py") >>> m.modules["controls"] Module('controls', '/libcamera/utils/codegen/controls.py') I am wondering if this might be used to generate a dependency file that ninja can interpret. Although this approach is definitely more complex. Regards, Barnabás Pőcze > >> >> 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 4bfe9615c..275f45200 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 >>
Quoting Barnabás Pőcze (2025-04-25 09:18:28) > Hi > > > 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-04-22 13:47:51) > >> 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`. > > > > This breaks in CI at the build-history stage: > > > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 > > > > I wonder, are we missing a dependency declaration somewhere that should > > have otherwise caused something to get rebuilt? All of the other builds > > seemed to have worked on that pipeline ? > > Yes indeed, `controls.py` is changed, but that is not considered by > meson, so it does not regenerate the cpp files because neither the > template nor `gen-controls.py`, etc. has changed. Yikes. Probably something to bring up in the meson channels for support. I have no idea on a (generic/longer term) solution I'm afraid. > I am not sure how it could be best addressed, there is `depend_files` where > `controls.py` could be specified, but I don't think it's too convenient. > > Or there is the `modulefinder` thing in the standard library: > > >>> os.getcwd() > '/libcamera/utils/codegen' > >>> import modulefinder > >>> m = modulefinder.ModuleFinder() > >>> m.run_script("./gen-controls.py") > >>> m.modules["controls"] > Module('controls', '/libcamera/utils/codegen/controls.py') > > I am wondering if this might be used to generate a dependency file that ninja > can interpret. Although this approach is definitely more complex. Yes, I can see that could get ugly - and we're not supposed to have to parse the dependencies of every tool we use. Ok - so I guess this one is different because it's our own in built tooling ... I think we could : - Contact meson developers for support - Update this patch to 'also' (trivially) modify either the template or gen-controls.py - Ignore the build-history failure and just keep it as is.. I think if we want to land this series - at worst I'm ok with the last option - but I'd rather keep it as a last resort to make sure we maintain bisectability. And for that - I think even an arbitrary comment update to the gen-controls.py which could be later removed would be fine if it came to it. > > > Regards, > Barnabás Pőcze > > > > >> > >> 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 4bfe9615c..275f45200 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("' "); Aha, ok so this is the API breakage ... So we'll lose the ability to flexibly represent strings ? Does this append have to construct a string ? (i.e. are we going to end up just 'moving' the new construction? Any help/benefit from using include/libcamera/base/utils.h libcamera::join();? Ah - no that would only add separateors - what we would need as a string 'wrap' to allow wrapping the model with ' characters. There are other occasions where I've thought about adding a wrap .. wrapper function but it's probably overkill. > >> } > >> > >> 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. */ This looks like a new construction ... does this make a copy ? As far as I can tell - this is functionally correct/valid - and all the build tests (except the build-history) work - so I'd put : Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Curious to see if this is something that provides enough 'benefit' over changing how string controls are handled vs potential copies. Do we ever expect strings to be expensive? I could actually imagine some use cases where we could start making internal/debug controls that might report larger strings in fact ... so it might actaully be helpful there! > >> } > >> > >> 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 > >> > @
2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-04-25 09:18:28) >> Hi >> >> >> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-04-22 13:47:51) >>>> 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`. >>> >>> This breaks in CI at the build-history stage: >>> >>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 >>> >>> I wonder, are we missing a dependency declaration somewhere that should >>> have otherwise caused something to get rebuilt? All of the other builds >>> seemed to have worked on that pipeline ? >> >> Yes indeed, `controls.py` is changed, but that is not considered by >> meson, so it does not regenerate the cpp files because neither the >> template nor `gen-controls.py`, etc. has changed. > > Yikes. Probably something to bring up in the meson channels for support. > I have no idea on a (generic/longer term) solution I'm afraid. > >> I am not sure how it could be best addressed, there is `depend_files` where >> `controls.py` could be specified, but I don't think it's too convenient. >> >> Or there is the `modulefinder` thing in the standard library: >> >> >>> os.getcwd() >> '/libcamera/utils/codegen' >> >>> import modulefinder >> >>> m = modulefinder.ModuleFinder() >> >>> m.run_script("./gen-controls.py") >> >>> m.modules["controls"] >> Module('controls', '/libcamera/utils/codegen/controls.py') >> >> I am wondering if this might be used to generate a dependency file that ninja >> can interpret. Although this approach is definitely more complex. > > Yes, I can see that could get ugly - and we're not supposed to have to > parse the dependencies of every tool we use. Ok - so I guess this one is > different because it's our own in built tooling ... > > I think we could : > - Contact meson developers for support > - Update this patch to 'also' (trivially) modify either the template or > gen-controls.py > - Ignore the build-history failure and just keep it as is.. > > I think if we want to land this series - at worst I'm ok with the last > option - but I'd rather keep it as a last resort to make sure we > maintain bisectability. And for that - I think even an arbitrary comment > update to the gen-controls.py which could be later removed would be fine > if it came to it. v2 addresses this issue by adding `depend_files` to each `custom_target()`. Not the best solution, but I couldn't find anything better. > > > >> >> >> Regards, >> Barnabás Pőcze >> >>> >>>> >>>> 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 4bfe9615c..275f45200 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("' "); > > Aha, ok so this is the API breakage ... So we'll lose the ability to > flexibly represent strings ? > > Does this append have to construct a string ? (i.e. are we going to end > up just 'moving' the new construction? > > Any help/benefit from using include/libcamera/base/utils.h > libcamera::join();? Ah - no that would only add separateors - what we > would need as a string 'wrap' to allow wrapping the model with ' > characters. > > There are other occasions where I've thought about adding a wrap .. > wrapper function but it's probably overkill. > > > >>>> } >>>> >>>> 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. */ > > This looks like a new construction ... does this make a copy ? > > > As far as I can tell - this is functionally correct/valid - and all the > build tests (except the build-history) work - so I'd put : > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Curious to see if this is something that provides enough 'benefit' over > changing how string controls are handled vs potential copies. > > Do we ever expect strings to be expensive? > > I could actually imagine some use cases where we could start making > internal/debug controls that might report larger strings in fact ... so > it might actaully be helpful there! > > > >>>> } >>>> >>>> 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 >>>> >> @
Quoting Barnabás Pőcze (2025-05-01 11:25:12) > 2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-04-25 09:18:28) > >> Hi > >> > >> > >> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta: > >>> Quoting Barnabás Pőcze (2025-04-22 13:47:51) > >>>> 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`. > >>> > >>> This breaks in CI at the build-history stage: > >>> > >>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 > >>> > >>> I wonder, are we missing a dependency declaration somewhere that should > >>> have otherwise caused something to get rebuilt? All of the other builds > >>> seemed to have worked on that pipeline ? > >> > >> Yes indeed, `controls.py` is changed, but that is not considered by > >> meson, so it does not regenerate the cpp files because neither the > >> template nor `gen-controls.py`, etc. has changed. > > > > Yikes. Probably something to bring up in the meson channels for support. > > I have no idea on a (generic/longer term) solution I'm afraid. > > > >> I am not sure how it could be best addressed, there is `depend_files` where > >> `controls.py` could be specified, but I don't think it's too convenient. > >> > >> Or there is the `modulefinder` thing in the standard library: > >> > >> >>> os.getcwd() > >> '/libcamera/utils/codegen' > >> >>> import modulefinder > >> >>> m = modulefinder.ModuleFinder() > >> >>> m.run_script("./gen-controls.py") > >> >>> m.modules["controls"] > >> Module('controls', '/libcamera/utils/codegen/controls.py') > >> > >> I am wondering if this might be used to generate a dependency file that ninja > >> can interpret. Although this approach is definitely more complex. > > > > Yes, I can see that could get ugly - and we're not supposed to have to > > parse the dependencies of every tool we use. Ok - so I guess this one is > > different because it's our own in built tooling ... > > > > I think we could : > > - Contact meson developers for support > > - Update this patch to 'also' (trivially) modify either the template or > > gen-controls.py > > - Ignore the build-history failure and just keep it as is.. > > > > I think if we want to land this series - at worst I'm ok with the last > > option - but I'd rather keep it as a last resort to make sure we > > maintain bisectability. And for that - I think even an arbitrary comment > > update to the gen-controls.py which could be later removed would be fine > > if it came to it. > > v2 addresses this issue by adding `depend_files` to each `custom_target()`. > Not the best solution, but I couldn't find anything better. Excellent, that's fine with me. There were a few other statements/questions below here... any comment? - I won't bother asking them on the new version ... you can keep/add my tag to the new version. > > > > > > > > > >> > >> > >> Regards, > >> Barnabás Pőcze > >> > >>> > >>>> > >>>> 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 4bfe9615c..275f45200 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("' "); > > > > Aha, ok so this is the API breakage ... So we'll lose the ability to > > flexibly represent strings ? > > > > Does this append have to construct a string ? (i.e. are we going to end > > up just 'moving' the new construction? > > > > Any help/benefit from using include/libcamera/base/utils.h > > libcamera::join();? Ah - no that would only add separateors - what we > > would need as a string 'wrap' to allow wrapping the model with ' > > characters. > > > > There are other occasions where I've thought about adding a wrap .. > > wrapper function but it's probably overkill. > > > > > > > >>>> } > >>>> > >>>> 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. */ > > > > This looks like a new construction ... does this make a copy ? > > > > > > As far as I can tell - this is functionally correct/valid - and all the > > build tests (except the build-history) work - so I'd put : > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Curious to see if this is something that provides enough 'benefit' over > > changing how string controls are handled vs potential copies. > > > > Do we ever expect strings to be expensive? > > > > I could actually imagine some use cases where we could start making > > internal/debug controls that might report larger strings in fact ... so > > it might actaully be helpful there! > > > > > > > >>>> } > >>>> > >>>> 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 > >>>> > >> @ >
2025. 05. 01. 13:45 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-05-01 11:25:12) >> 2025. 05. 01. 12:16 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-04-25 09:18:28) >>>> Hi >>>> >>>> >>>> 2025. 04. 23. 10:33 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2025-04-22 13:47:51) >>>>>> 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`. >>>>> >>>>> This breaks in CI at the build-history stage: >>>>> >>>>> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/75119329#L1034 >>>>> >>>>> I wonder, are we missing a dependency declaration somewhere that should >>>>> have otherwise caused something to get rebuilt? All of the other builds >>>>> seemed to have worked on that pipeline ? >>>> >>>> Yes indeed, `controls.py` is changed, but that is not considered by >>>> meson, so it does not regenerate the cpp files because neither the >>>> template nor `gen-controls.py`, etc. has changed. >>> >>> Yikes. Probably something to bring up in the meson channels for support. >>> I have no idea on a (generic/longer term) solution I'm afraid. >>> >>>> I am not sure how it could be best addressed, there is `depend_files` where >>>> `controls.py` could be specified, but I don't think it's too convenient. >>>> >>>> Or there is the `modulefinder` thing in the standard library: >>>> >>>> >>> os.getcwd() >>>> '/libcamera/utils/codegen' >>>> >>> import modulefinder >>>> >>> m = modulefinder.ModuleFinder() >>>> >>> m.run_script("./gen-controls.py") >>>> >>> m.modules["controls"] >>>> Module('controls', '/libcamera/utils/codegen/controls.py') >>>> >>>> I am wondering if this might be used to generate a dependency file that ninja >>>> can interpret. Although this approach is definitely more complex. >>> >>> Yes, I can see that could get ugly - and we're not supposed to have to >>> parse the dependencies of every tool we use. Ok - so I guess this one is >>> different because it's our own in built tooling ... >>> >>> I think we could : >>> - Contact meson developers for support >>> - Update this patch to 'also' (trivially) modify either the template or >>> gen-controls.py >>> - Ignore the build-history failure and just keep it as is.. >>> >>> I think if we want to land this series - at worst I'm ok with the last >>> option - but I'd rather keep it as a last resort to make sure we >>> maintain bisectability. And for that - I think even an arbitrary comment >>> update to the gen-controls.py which could be later removed would be fine >>> if it came to it. >> >> v2 addresses this issue by adding `depend_files` to each `custom_target()`. >> Not the best solution, but I couldn't find anything better. > > Excellent, that's fine with me. > > There were a few other statements/questions below here... any comment? - > I won't bother asking them on the new version ... you can keep/add my > tag to the new version. Oops, I did miss those. > > >> >> >>> >>> >>> >>>> >>>> >>>> Regards, >>>> Barnabás Pőcze >>>> >>>>> >>>>>> >>>>>> 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 4bfe9615c..275f45200 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("' "); >>> >>> Aha, ok so this is the API breakage ... So we'll lose the ability to >>> flexibly represent strings ? One can always do e.g. `std::string(x.get(controls::Model))`. By returning an `std::string_view`, no `std::string` construction is forced. If the user wants an `std::string`, then they can construct one as seen above, and they get the previous behaviour back. >>> >>> Does this append have to construct a string ? (i.e. are we going to end >>> up just 'moving' the new construction? `std::string::append()` does not need to construct a separate string from `*model`. >>> >>> Any help/benefit from using include/libcamera/base/utils.h >>> libcamera::join();? Ah - no that would only add separateors - what we >>> would need as a string 'wrap' to allow wrapping the model with ' >>> characters. FYI, there is `std::quoted()` when working with streams: https://en.cppreference.com/w/cpp/io/manip/quoted Of course, that's not applicable here, but nonetheless I don't think this is a big issue. >>> >>> There are other occasions where I've thought about adding a wrap .. >>> wrapper function but it's probably overkill. >>> >>> >>> >>>>>> } >>>>>> >>>>>> 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. */ >>> >>> This looks like a new construction ... does this make a copy ? Yes, it does. Previously `cameraProperties.get(properties::Model)` would have made the equivalent copy, but since that now returns an `std::string_view`, the copy has to be explicitly made in the caller if an `std::string` is needed. So the number of copies does not change. (I did not find a way to pass a (ptr, len) pair to `TIFFSetField()` or similar, which could avoid the copy altogether.) >>> >>> >>> As far as I can tell - this is functionally correct/valid - and all the >>> build tests (except the build-history) work - so I'd put : >>> >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> Curious to see if this is something that provides enough 'benefit' over >>> changing how string controls are handled vs potential copies. >>> >>> Do we ever expect strings to be expensive? To be honest I don't really see it ever becoming a bottleneck. Regards, Barnabás Pőcze >>> >>> I could actually imagine some use cases where we could start making >>> internal/debug controls that might report larger strings in fact ... so >>> it might actaully be helpful there! >>> >>> >>> >>>>>> } >>>>>> >>>>>> 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 4bfe9615c..275f45200 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(-)