[PATCH v1 2/2] libcamera: controls: Expose string controls as `std::string_view`
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 1 12:16:46 CEST 2025
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 at 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 at 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
> >>
> @
More information about the libcamera-devel
mailing list