[v2,3/3] libcamera: controls: Expose string controls as `std::string_view`

23327 diff mbox series

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(-)

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@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 ?

>  		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

>  	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
>

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@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 ?
> 
>>   		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
> 
>>   	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 ?

I think that could be useful, although I would defer it because there is only
a single string-valued control (properties::Model). And if we want to support
multiple "input" types for a single control types, then I think there are
better ways but those need potentially more changes.


Regards,
Barnabás Pőcze

> 
>>   		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
>>

Hi Barnabás

On Fri, May 02, 2025 at 12:22:13PM +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@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 ?
> >
> > >   		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
> >
> > >   	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 ?
>
> I think that could be useful, although I would defer it because there is only
> a single string-valued control (properties::Model). And if we want to support
> multiple "input" types for a single control types, then I think there are
> better ways but those need potentially more changes.
>

Also, it's a property, so no way one can set it. Fine then, I would
record a \todo so that we don't forget about this when we'll add a
string control, but up to you.

>
> Regards,
> Barnabás Pőcze
>
> >
> > >   		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
> > >
>

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@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.


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
>>

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@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@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
> > >
>

Hi Barnabás,

Thanks for the patch.

Quoting Barnabás Pőcze (2025-05-01 11:58:18)
> 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`.

Creating compatibility report ...
Binary compatibility: 100%
Source compatibility: 99.8%
Total binary compatibility problems: 0, warnings: 1
Total source compatibility problems: 2, warnings: 1

Not bad.

> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=256
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@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);
>                 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 Barnabás,

Thank you for the patch.

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`.
> 
> 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 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);
>  		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());

Slightly annoying, but that's an issue with the TIFF library API, it's
not our fault.

>  		/* \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");

I was concerned that this could result in a use-after-free if the
argument to value_or() is a temporary std::string, but I think the
lifetime of the temporary will be extended to cover the lifetime of
model.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> -	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

2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> 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`.
>>
>> 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 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);
>>   		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());
> 
> Slightly annoying, but that's an issue with the TIFF library API, it's
> not our fault.
> 
>>   		/* \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");
> 
> I was concerned that this could result in a use-after-free if the
> argument to value_or() is a temporary std::string, but I think the

I am afraid that's exactly what would happen, an `std::string_view` is
returned to the temporary `std::string`. So that would be a use-after-free
in some cases.


> lifetime of the temporary will be extended to cover the lifetime of
> model.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>   
>> -	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
>

On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote:
> 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta:
> > 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`.
> >>
> >> 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 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);
> >>   		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());
> > 
> > Slightly annoying, but that's an issue with the TIFF library API, it's
> > not our fault.
> > 
> >>   		/* \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");
> > 
> > I was concerned that this could result in a use-after-free if the
> > argument to value_or() is a temporary std::string, but I think the
> 
> I am afraid that's exactly what would happen, an `std::string_view` is
> returned to the temporary `std::string`. So that would be a use-after-free
> in some cases.

valgrind didn't catch any issue. Isn't the lifetime of temporary objects
extended in this case ?

> > lifetime of the temporary will be extended to cover the lifetime of
> > model.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >>   
> >> -	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

2025. 10. 03. 18:58 keltezéssel, Laurent Pinchart írta:
> On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote:
>> 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta:
>>> 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`.
>>>>
>>>> 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 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);
>>>>    		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());
>>>
>>> Slightly annoying, but that's an issue with the TIFF library API, it's
>>> not our fault.
>>>
>>>>    		/* \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");
>>>
>>> I was concerned that this could result in a use-after-free if the
>>> argument to value_or() is a temporary std::string, but I think the
>>
>> I am afraid that's exactly what would happen, an `std::string_view` is
>> returned to the temporary `std::string`. So that would be a use-after-free
>> in some cases.
> 
> valgrind didn't catch any issue. Isn't the lifetime of temporary objects
> extended in this case ?

The return type of this `value_or()` call will be `std::string_view`,
the underlying temporary object passed as argument will be destroyed.

https://gcc.godbolt.org/z/Mb3qKT1jK

This issue is generally present for array like controls:

   auto x = request->metadata().get(controls::ColourGains).value_or(std::vector<float> { 1.f, 1.f });
   x[0] // UAF if `controls::ColourGains` is not in metadata

or similar.

> 
>>> lifetime of the temporary will be extended to cover the lifetime of
>>> model.
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>>    
>>>> -	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
>

On Fri, Oct 03, 2025 at 07:08:35PM +0200, Barnabás Pőcze wrote:
> 2025. 10. 03. 18:58 keltezéssel, Laurent Pinchart írta:
> > On Fri, Oct 03, 2025 at 03:30:21PM +0200, Barnabás Pőcze wrote:
> >> 2025. 10. 02. 20:27 keltezéssel, Laurent Pinchart írta:
> >>> 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`.
> >>>>
> >>>> 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 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);
> >>>>    		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());
> >>>
> >>> Slightly annoying, but that's an issue with the TIFF library API, it's
> >>> not our fault.
> >>>
> >>>>    		/* \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");
> >>>
> >>> I was concerned that this could result in a use-after-free if the
> >>> argument to value_or() is a temporary std::string, but I think the
> >>
> >> I am afraid that's exactly what would happen, an `std::string_view` is
> >> returned to the temporary `std::string`. So that would be a use-after-free
> >> in some cases.
> > 
> > valgrind didn't catch any issue. Isn't the lifetime of temporary objects
> > extended in this case ?
> 
> The return type of this `value_or()` call will be `std::string_view`,
> the underlying temporary object passed as argument will be destroyed.
> 
> https://gcc.godbolt.org/z/Mb3qKT1jK
> 
> This issue is generally present for array like controls:
> 
>    auto x = request->metadata().get(controls::ColourGains).value_or(std::vector<float> { 1.f, 1.f });
>    x[0] // UAF if `controls::ColourGains` is not in metadata
> 
> or similar.

You're right, I somehow extended the lifetime extension rules of
range-based for loops to all expressions :-/

It's not a new issues so I don't think it needs to be solved right now,
but it's something to consider for future revisions of this API.

> >>> lifetime of the temporary will be extended to cover the lifetime of
> >>> model.
> >>>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>
> >>>>    
> >>>> -	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