[PATCH v1 2/2] libcamera: controls: Expose string controls as `std::string_view`
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Thu May 1 16:12:28 CEST 2025
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 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 ?
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 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?
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
>>>>>>
>>>> @
>>
More information about the libcamera-devel
mailing list