[PATCH v2 3/3] libcamera: controls: Expose string controls as `std::string_view`
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri May 2 15:07:41 CEST 2025
Hi Barnabás
On Fri, May 02, 2025 at 02:17:17PM +0200, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. 05. 02. 12:11 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Thu, May 01, 2025 at 11:58:18AM +0200, Barnabás Pőcze wrote:
> > > When retrieving the value from a `ControlValue` usually one of two
> > > things happen: a small, trivially copyable object is returned by
> > > value; or a view into the internal buffer is provided. This is true
> > > for everything except strings, which are returned in `std::string`,
> > > incurring the overhead of string construction.
> > >
> > > To guarantee no potentially "expensive" copies, use `std::string_view`
> > > pointing to the internal buffer to return the value. This is similar
> > > to how other array-like types are returned with a `Span<>`.
> > >
> > > This is an API break, but its scope is limited to just `properties::Model`.
> >
> > I've been a bit in two minds about this, until I realized we already
> > require c++17 for applications.
> >
> > My understanding is that using string_view is no different than
> > returning a Span<> when it comes to lifetime of the referenced data,
> > so if we're fine with Span<> we're fine with string_view.
> >
> > And as pointed out by Barnabas, if a user wants a string, it can be
> > constructed easily with the same cost as we have today.
> >
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=256
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze 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 2ae4ec3d4..5a707a387 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -13,6 +13,7 @@
> > > #include <set>
> > > #include <stdint.h>
> > > #include <string>
> > > +#include <string_view>
> > > #include <unordered_map>
> > > #include <vector>
> > >
> > > @@ -96,7 +97,7 @@ struct control_type<float> {
> > > };
> > >
> > > template<>
> > > -struct control_type<std::string> {
> > > +struct control_type<std::string_view> {
> > > static constexpr ControlType value = ControlTypeString;
> > > static constexpr std::size_t size = 0;
> > > };
> > > @@ -138,7 +139,7 @@ public:
> > > #ifndef __DOXYGEN__
> > > template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > > details::control_type<T>::value &&
> > > - !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > ControlValue(const T &value)
> > > : type_(ControlTypeNone), numElements_(0)
> > > @@ -148,7 +149,7 @@ public:
> > > }
> > >
> > > template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > > - std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > #else
> > > template<typename T>
> > > @@ -182,7 +183,7 @@ public:
> > >
> > > #ifndef __DOXYGEN__
> > > template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > > - !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > T get() const
> > > {
> > > @@ -193,7 +194,7 @@ public:
> > > }
> > >
> > > template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > > - std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > #else
> > > template<typename T>
> > > @@ -210,7 +211,7 @@ public:
> > >
> > > #ifndef __DOXYGEN__
> > > template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > > - !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + !std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > void set(const T &value)
> > > {
> > > @@ -219,7 +220,7 @@ public:
> > > }
> > >
> > > template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > > - std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > + std::is_same<std::string_view, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > #else
> > > template<typename T>
> > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
> > > index fdf82efc0..bc4c7c3a0 100644
> > > --- a/src/apps/cam/capture_script.cpp
> > > +++ b/src/apps/cam/capture_script.cpp
> > > @@ -502,7 +502,7 @@ ControlValue CaptureScript::parseScalarControl(const ControlId *id,
> > > break;
> > > }
> > > case ControlTypeString: {
> > > - value.set<std::string>(repr);
> > > + value.set<std::string_view>(repr);
> > > break;
> > > }
> > > default:
> > > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> > > index fa266eca6..157d238d7 100644
> > > --- a/src/apps/cam/main.cpp
> > > +++ b/src/apps/cam/main.cpp
> > > @@ -337,7 +337,7 @@ std::string CamApp::cameraName(const Camera *camera)
> > > */
> > > const auto &model = props.get(properties::Model);
> >
> > Does this need to be a reference now ?
>
> There isn't any real difference because it's an `std::optional` in both cases
> (just with different value type), so I decided not to change it.
>
>
> >
> > > if (model)
> > > - name = "'" + *model + "' ";
> > > + name.append("'").append(*model).append("' ");
> > > }
> > >
> > > name += "(" + camera->id() + ")";
> > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp
> > > index ac4619511..8d57023e1 100644
> > > --- a/src/apps/common/dng_writer.cpp
> > > +++ b/src/apps/common/dng_writer.cpp
> > > @@ -564,7 +564,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> > >
> > > const auto &model = cameraProperties.get(properties::Model);
> >
> > likewise
>
> Same here.
Ack, thanks for clarifying.
Please add
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
>
>
> Regards,
> Barnabás Pőcze
>
> >
> > > if (model) {
> > > - TIFFSetField(tif, TIFFTAG_MODEL, model->c_str());
> > > + TIFFSetField(tif, TIFFTAG_MODEL, std::string(*model).c_str());
> > > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */
> > > }
> > >
> > > diff --git a/src/apps/qcam/cam_select_dialog.cpp b/src/apps/qcam/cam_select_dialog.cpp
> > > index 6b6d0713c..9f25ba091 100644
> > > --- a/src/apps/qcam/cam_select_dialog.cpp
> > > +++ b/src/apps/qcam/cam_select_dialog.cpp
> > > @@ -114,8 +114,8 @@ void CameraSelectorDialog::updateCameraInfo(QString cameraId)
> > > cameraLocation_->setText("Unknown");
> > > }
> > >
> > > - const auto &model = properties.get(libcamera::properties::Model)
> > > - .value_or("Unknown");
> > > + const auto model = properties.get(libcamera::properties::Model)
> > > + .value_or("Unknown");
> > >
> > > - cameraModel_->setText(QString::fromStdString(model));
> > > + cameraModel_->setText(QString::fromUtf8(model.data(), model.length()));
> > > }
> > > diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp
> > > index 1ad1d4c1a..8c55ef845 100644
> > > --- a/src/py/libcamera/py_helpers.cpp
> > > +++ b/src/py/libcamera/py_helpers.cpp
> > > @@ -47,7 +47,7 @@ py::object controlValueToPy(const ControlValue &cv)
> > > case ControlTypeFloat:
> > > return valueOrTuple<float>(cv);
> > > case ControlTypeString:
> > > - return py::cast(cv.get<std::string>());
> > > + return py::cast(cv.get<std::string_view>());
> > > case ControlTypeSize: {
> > > const Size *v = reinterpret_cast<const Size *>(cv.data().data());
> > > return py::cast(v);
> > > @@ -88,7 +88,7 @@ ControlValue pyToControlValue(const py::object &ob, ControlType type)
> > > case ControlTypeFloat:
> > > return controlValueMaybeArray<float>(ob);
> > > case ControlTypeString:
> > > - return ControlValue(ob.cast<std::string>());
> > > + return ControlValue(ob.cast<std::string_view>());
> > > case ControlTypeRectangle:
> > > return controlValueMaybeArray<Rectangle>(ob);
> > > case ControlTypeSize:
> > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp
> > > index 5084fd0cf..032050a77 100644
> > > --- a/test/controls/control_value.cpp
> > > +++ b/test/controls/control_value.cpp
> > > @@ -318,7 +318,7 @@ protected:
> > > /*
> > > * String type.
> > > */
> > > - std::string string{ "libcamera" };
> > > + std::string_view string{ "libcamera" };
> > > value.set(string);
> >
> > Here's one thing: we now require users to use string_view when setting
> > a control, and we anyway end up copying it's content into the
> > ControlValue.
> >
> > Should we provide a set() overload for std::string ? Is it worth it ?
> >
> > > if (value.isNone() || !value.isArray() ||
> > > value.type() != ControlTypeString ||
> > > @@ -327,7 +327,7 @@ protected:
> > > return TestFail;
> > > }
> > >
> > > - if (value.get<std::string>() != string) {
> > > + if (value.get<std::string_view>() != string) {
> >
> > For get() instead I think it's fine always returning a view
> >
> > > cerr << "Control value mismatch after setting to string" << endl;
> > > return TestFail;
> > > }
> > > diff --git a/utils/codegen/controls.py b/utils/codegen/controls.py
> > > index e51610481..9399727bd 100644
> > > --- a/utils/codegen/controls.py
> > > +++ b/utils/codegen/controls.py
> > > @@ -111,7 +111,7 @@ class Control(object):
> > > size = self.__data.get('size')
> > >
> > > if typ == 'string':
> > > - return 'std::string'
> > > + return 'std::string_view'
> > >
> > > if self.__size is None:
> > > return typ
> > > --
> > > 2.49.0
> > >
>
More information about the libcamera-devel
mailing list