[PATCH v1 2/2] libcamera: controls: Expose string controls as `std::string_view`
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Fri Apr 25 10:18:28 CEST 2025
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 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("' ");
>> }
>>
>> 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
>>
More information about the libcamera-devel
mailing list