[3/3] libcamera: Make ControlValue a view
diff mbox series

Message ID 20250806-control_storage-v1-3-2ec8424f6f7d@ideasonboard.com
State New
Headers show
Series
  • libcamera: Make ControlValue a view
Related show

Commit Message

Jacopo Mondi Aug. 6, 2025, 12:30 p.m. UTC
Rename ControlValueView to ControlValue in the whole codebase in
order to make the libcamera type ControlValue a read-only view over
control values stored somewhere else.

Usage of this view-like ControlValue class in public API allows to
provide the ControlList class with:

	const ControlValue get(unsigned int id) const;

that returns a reference to the value associated with a control id
without making a copy. Similarly, access to the min/max/def members of
a ControlInfo (as well as the list of supported values) now returns
a view without copying the stored values in a temporary ControlValue.

The actual control values can be copied in the caller by using the
already existing

	template<typename T> [[nodiscard]] auto get() const

function.

The rename on the whole code base has not required many manual
modifications in order to keep it compiling. The most notable ones are:

- ControlInfo::values() needs to re-create a vector in order to return
  it
- A new

	ControlList::set(unsigned int id, const ControlStorage &value)

  function had to be added to complement:

	void set(unsigned int id, const ControlValue &value);

  This new function serves to support direct initialization of
  controls in a control list with explicit values, in example:

  	lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));

  As a ControlValue cannot be constructed with a litterals or with
  values we need to go through a ControlStorage and then copy it to
  the control list backing storage.

- The ControlSerializer class functions binarySize() and store()
  have been moved to work with views

- The control_value.cpp test now actually tests ControlStorage

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/controls.h                    | 39 ++++++++++--------
 include/libcamera/internal/control_serializer.h |  4 +-
 src/gstreamer/gstlibcamera-controls.cpp.in      |  4 +-
 src/libcamera/control_serializer.cpp            |  4 +-
 src/libcamera/controls.cpp                      | 54 +++++++++++++++----------
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp    |  4 +-
 src/libcamera/sensor/camera_sensor_legacy.cpp   |  2 +-
 src/libcamera/sensor/camera_sensor_raw.cpp      |  2 +-
 src/libcamera/v4l2_device.cpp                   |  2 +-
 src/py/libcamera/py_helpers.cpp                 |  4 +-
 src/py/libcamera/py_helpers.h                   |  2 +-
 test/serialization/serialization_test.cpp       |  4 +-
 test/v4l2_videodevice/controls.cpp              |  4 +-
 13 files changed, 74 insertions(+), 55 deletions(-)

Patch
diff mbox series

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 8c531cca25538edadd08b8e1662227ba56065e40..a827b1bad9530f073acce112be3cce30f34e4404 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -26,7 +26,7 @@ 
 namespace libcamera {
 
 class ControlValidator;
-class ControlValueView;
+class ControlValue;
 
 enum ControlType {
 	ControlTypeNone,
@@ -162,7 +162,7 @@  public:
 		    value.data(), value.size(), sizeof(typename T::value_type));
 	}
 
-	explicit ControlStorage(const ControlValueView &cvv);
+	ControlStorage(const ControlValue &cvv);
 
 	~ControlStorage();
 
@@ -251,23 +251,23 @@  private:
 		 std::size_t numElements, std::size_t elementSize);
 };
 
-class ControlValueView
+class ControlValue
 {
 public:
-	constexpr ControlValueView() noexcept
+	constexpr ControlValue() noexcept
 		: type_(ControlTypeNone)
 	{
 	}
 
-	ControlValueView(const ControlStorage &cv) noexcept
-		: ControlValueView(cv.type(), cv.isArray(), cv.numElements(),
+	ControlValue(const ControlStorage &cv) noexcept
+		: ControlValue(cv.type(), cv.isArray(), cv.numElements(),
 				   reinterpret_cast<const std::byte *>(cv.data().data()))
 	{
 	}
 
 #ifndef __DOXYGEN__
 	// TODO: should have restricted access?
-	ControlValueView(ControlType type, bool isArray, std::size_t numElements,
+	ControlValue(ControlType type, bool isArray, std::size_t numElements,
 			 const std::byte *data) noexcept
 		: type_(type), isArray_(isArray), numElements_(numElements),
 		  data_(data)
@@ -283,9 +283,9 @@  public:
 	[[nodiscard]] std::size_t numElements() const { return numElements_; }
 	[[nodiscard]] Span<const std::byte> data() const;
 
-	[[nodiscard]] bool operator==(const ControlValueView &other) const;
+	[[nodiscard]] bool operator==(const ControlValue &other) const;
 
-	[[nodiscard]] bool operator!=(const ControlValueView &other) const
+	[[nodiscard]] bool operator!=(const ControlValue &other) const
 	{
 		return !(*this == other);
 	}
@@ -313,7 +313,7 @@  private:
 	const std::byte *data_ = nullptr;
 };
 
-std::ostream &operator<<(std::ostream &s, const ControlValueView &v);
+std::ostream &operator<<(std::ostream &s, const ControlValue &v);
 
 class ControlId
 {
@@ -405,10 +405,16 @@  public:
 	explicit ControlInfo(std::set<bool> values, bool def);
 	explicit ControlInfo(bool value);
 
-	const ControlStorage &min() const { return min_; }
-	const ControlStorage &max() const { return max_; }
-	const ControlStorage &def() const { return def_; }
-	const std::vector<ControlStorage> &values() const { return values_; }
+	const ControlValue min() const { return min_; }
+	const ControlValue max() const { return max_; }
+	const ControlValue def() const { return def_; }
+
+	const std::vector<ControlValue> values() const {
+		std::vector<ControlValue> values;
+		for (const auto &val : values_)
+			values.emplace_back(val);
+		return values;
+	}
 
 	std::string toString() const;
 
@@ -513,7 +519,7 @@  public:
 		if (entry == controls_.end())
 			return std::nullopt;
 
-		const ControlStorage &val = entry->second;
+		const ControlValue val = entry->second;
 		return val.get<T>();
 	}
 
@@ -537,7 +543,8 @@  public:
 		val->set(Span<const typename std::remove_cv_t<V>, Size>{ value.begin(), value.size() });
 	}
 
-	const ControlStorage &get(unsigned int id) const;
+	const ControlValue get(unsigned int id) const;
+	void set(unsigned int id, const ControlValue &value);
 	void set(unsigned int id, const ControlStorage &value);
 
 	const ControlInfoMap *infoMap() const { return infoMap_; }
diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
index a63f8675e3076eddfdda8c7b684499237451596b..9b22c30a6ff34dae3bcff4219745dc0bcf90ae75 100644
--- a/include/libcamera/internal/control_serializer.h
+++ b/include/libcamera/internal/control_serializer.h
@@ -41,10 +41,10 @@  public:
 	bool isCached(const ControlInfoMap &infoMap);
 
 private:
-	static size_t binarySize(const ControlStorage &value);
+	static size_t binarySize(const ControlValue &value);
 	static size_t binarySize(const ControlInfo &info);
 
-	static void store(const ControlStorage &value, ByteStreamBuffer &buffer);
+	static void store(const ControlValue &value, ByteStreamBuffer &buffer);
 	static void store(const ControlInfo &info, ByteStreamBuffer &buffer);
 
 	ControlStorage loadControlValue(ByteStreamBuffer &buffer,
diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in
index 346bf83fac076c8fa7c7d6f8a9b281626849aaa1..2a16b39a93d95938022b38c57aab74efa10678cf 100644
--- a/src/gstreamer/gstlibcamera-controls.cpp.in
+++ b/src/gstreamer/gstlibcamera-controls.cpp.in
@@ -159,7 +159,7 @@  bool GstCameraControls::getProperty(guint propId, GValue *value,
 			    controls::controls.at(propId)->name().c_str());
 		return true;
 	}
-	const ControlStorage &cv = controls_acc_.get(propId);
+	const ControlValue &cv = controls_acc_.get(propId);
 
 	switch (propId) {
 {%- for vendor, ctrls in controls %}
@@ -296,7 +296,7 @@  void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam)
 	     control != controls_acc_.end();
 	     ++control) {
 		unsigned int id = control->first;
-		ControlStorage value = control->second;
+		ControlValue value = control->second;
 
 		const ControlId *cid = capabilities_.idmap().at(id);
 		auto info = capabilities_.find(cid);
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index df8bcd8a938c6fd3db252ae56673483fa9923b7d..10f7c0cbacae5937eab53da60a405d0f88a6d57d 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -142,7 +142,7 @@  void ControlSerializer::reset()
 	controlIdMaps_.clear();
 }
 
-size_t ControlSerializer::binarySize(const ControlStorage &value)
+size_t ControlSerializer::binarySize(const ControlValue &value)
 {
 	return sizeof(ControlType) + value.data().size_bytes();
 }
@@ -192,7 +192,7 @@  size_t ControlSerializer::binarySize(const ControlList &list)
 	return size;
 }
 
-void ControlSerializer::store(const ControlStorage &value,
+void ControlSerializer::store(const ControlValue &value,
 			      ByteStreamBuffer &buffer)
 {
 	const ControlType type = value.type();
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index cdc4f12d1048d58c558feaa8a17b25814d2dae13..9043b3407d8ebd28a822c2089da44b7897c6f153 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -107,9 +107,9 @@  ControlStorage::ControlStorage()
 }
 
 /**
- * \brief Construct a ControlStorage from a ControlValueView
+ * \brief Construct a ControlStorage from a ControlValue
  */
-ControlStorage::ControlStorage(const ControlValueView &cvv)
+ControlStorage::ControlStorage(const ControlValue &cvv)
 	: ControlStorage()
 {
 	set(cvv.type(), cvv.isArray(), cvv.data().data(),
@@ -223,7 +223,7 @@  Span<uint8_t> ControlStorage::data()
  */
 std::string ControlStorage::toString() const
 {
-	return static_cast<std::ostringstream&&>(std::ostringstream{} << ControlValueView(*this)).str();
+	return static_cast<std::ostringstream&&>(std::ostringstream{} << ControlValue(*this)).str();
 }
 
 /**
@@ -329,18 +329,18 @@  void ControlStorage::reserve(ControlType type, bool isArray, std::size_t numElem
 }
 
 /**
- * \class ControlValueView
+ * \class ControlValue
  * \brief A non-owning view-like type to the value of a control
  */
 
 /**
- * \fn ControlValueView::ControlValueView()
+ * \fn ControlValue::ControlValue()
  * \brief Construct an empty view
  * \sa ControlStorage::ControlStorage()
  */
 
 /**
- * \fn ControlValueView::ControlValueView(const ControlStorage &v)
+ * \fn ControlValue::ControlValue(const ControlStorage &v)
  * \brief Construct a view referring to \a v
  *
  * The constructed view will refer to the value stored by \a v, and
@@ -350,31 +350,31 @@  void ControlStorage::reserve(ControlType type, bool isArray, std::size_t numElem
  */
 
 /**
- * \fn ControlValueView::operator bool() const
+ * \fn ControlValue::operator bool() const
  * \brief Determine if the referred value is valid
- * \sa ControlValueView::isNone()
+ * \sa ControlValue::isNone()
  */
 
 /**
- * \fn ControlType ControlValueView::type() const
+ * \fn ControlType ControlValue::type() const
  * \copydoc ControlStorage::type()
  * \sa ControlStorage::type()
  */
 
 /**
- * \fn ControlValueView::isNone() const
+ * \fn ControlValue::isNone() const
  * \copydoc ControlStorage::isNone()
  * \sa ControlStorage::isNone()
  */
 
 /**
- * \fn ControlValueView::isArray() const
+ * \fn ControlValue::isArray() const
  * \copydoc ControlStorage::isArray()
  * \sa ControlStorage::isArray()
  */
 
 /**
- * \fn ControlValueView::numElements() const
+ * \fn ControlValue::numElements() const
  * \copydoc ControlStorage::numElements()
  * \sa ControlStorage::numElements()
  */
@@ -383,7 +383,7 @@  void ControlStorage::reserve(ControlType type, bool isArray, std::size_t numElem
  * \copydoc ControlStorage::data()
  * \sa ControlStorage::data()
  */
-Span<const std::byte> ControlValueView::data() const
+Span<const std::byte> ControlValue::data() const
 {
 	return { data_, numElements_ * ControlValueSize[type_] };
 }
@@ -391,9 +391,9 @@  Span<const std::byte> ControlValueView::data() const
 /**
  * \copydoc ControlStorage::operator==()
  * \sa ControlStorage::operator==()
- * \sa ControlValueView::operator!=()
+ * \sa ControlValue::operator!=()
  */
-bool ControlValueView::operator==(const ControlValueView &other) const
+bool ControlValue::operator==(const ControlValue &other) const
 {
 	if (type_ != other.type_)
 		return false;
@@ -410,14 +410,14 @@  bool ControlValueView::operator==(const ControlValueView &other) const
 }
 
 /**
- * \fn ControlValueView::operator!=() const
+ * \fn ControlValue::operator!=() const
  * \copydoc ControlStorage::operator!=()
  * \sa ControlStorage::operator!=()
- * \sa ControlValueView::operator==()
+ * \sa ControlValue::operator==()
  */
 
 /**
- * \fn template<typename T> T ControlValueView::get() const
+ * \fn template<typename T> T ControlValue::get() const
  * \copydoc ControlStorage::get()
  * \sa ControlStorage::get()
  */
@@ -426,7 +426,7 @@  bool ControlValueView::operator==(const ControlValueView &other) const
  * \brief Insert a text representation of a value into an output stream
  * \sa ControlStorage::toString()
  */
-std::ostream &operator<<(std::ostream &s, const ControlValueView &v)
+std::ostream &operator<<(std::ostream &s, const ControlValue &v)
 {
 	const auto type = v.type();
 	if (type == ControlTypeNone)
@@ -1234,7 +1234,7 @@  bool ControlList::contains(unsigned int id) const
  *
  * \return The control value
  */
-const ControlStorage &ControlList::get(unsigned int id) const
+const ControlValue ControlList::get(unsigned int id) const
 {
 	static const ControlStorage zero;
 
@@ -1257,7 +1257,7 @@  const ControlStorage &ControlList::get(unsigned int id) const
  * The behaviour is undefined if the control \a id is not supported by the
  * object that the list refers to.
  */
-void ControlList::set(unsigned int id, const ControlStorage &value)
+void ControlList::set(unsigned int id, const ControlValue &value)
 {
 	ControlStorage *val = find(id);
 	if (!val)
@@ -1266,6 +1266,18 @@  void ControlList::set(unsigned int id, const ControlStorage &value)
 	*val = value;
 }
 
+/**
+ * \brief Set the value of control \a id to \a value
+ * \copydoc ControlList::set(unsigned int id, const ControlValue &value)
+ */
+void ControlList::set(unsigned int id, const ControlStorage &value)
+{
+	ControlStorage *val = find(id);
+	if (!val)
+		return;
+
+	*val = value;
+}
 /**
  * \fn ControlList::infoMap()
  * \brief Retrieve the ControlInfoMap used to construct the ControlList
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 627aa69be294d89f796cfac148c315b29d6de633..a166b0bc4a51f6b81ccb7f0dd98dfe6a30b235e6 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -721,7 +721,7 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 	}
 
 	/* Map the control info. */
-	const std::vector<ControlStorage> &v4l2Values = v4l2Info.values();
+	const std::vector<ControlValue> &v4l2Values = v4l2Info.values();
 	int32_t min = v4l2Info.min().get<int32_t>();
 	int32_t max = v4l2Info.max().get<int32_t>();
 	int32_t def = v4l2Info.def().get<int32_t>();
@@ -793,7 +793,7 @@  void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
 		> exposureModes;
 		std::optional<controls::ExposureTimeModeEnum> lcDef;
 
-		for (const ControlStorage &value : v4l2Values) {
+		for (const ControlValue value : v4l2Values) {
 			const auto x = value.get<int32_t>();
 
 			if (0 <= x && static_cast<std::size_t>(x) < exposureModes.size()) {
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
index 366a66880ff0525b8198acccdaca946da538faad..f269b07eff51c48c2ac64b7a080eff3e7575c122 100644
--- a/src/libcamera/sensor/camera_sensor_legacy.cpp
+++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
@@ -538,7 +538,7 @@  void CameraSensorLegacy::initTestPatternModes()
 	for (const auto &it : testPatternModes)
 		indexToTestPatternMode[it.second] = it.first;
 
-	for (const ControlStorage &value : v4l2TestPattern->second.values()) {
+	for (const ControlValue value : v4l2TestPattern->second.values()) {
 		const int32_t index = value.get<int32_t>();
 
 		const auto it = indexToTestPatternMode.find(index);
diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
index fc41ee1b71c360c7e1b2ac1aed15a44c277ad7a1..d7aefd0b198401c73a7b8bd5b5dc690acf725758 100644
--- a/src/libcamera/sensor/camera_sensor_raw.cpp
+++ b/src/libcamera/sensor/camera_sensor_raw.cpp
@@ -720,7 +720,7 @@  void CameraSensorRaw::initTestPatternModes()
 	for (const auto &it : testPatternModes)
 		indexToTestPatternMode[it.second] = it.first;
 
-	for (const ControlStorage &value : v4l2TestPattern->second.values()) {
+	for (const ControlValue value : v4l2TestPattern->second.values()) {
 		const int32_t index = value.get<int32_t>();
 
 		const auto it = indexToTestPatternMode.find(index);
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 9be9df78416d8a5a4c6891b2926fcea41f808233..3d187ba5eed293e43b0d532310f00c81acf5a952 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -189,7 +189,7 @@  ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
 			return {};
 		}
 
-		ctrls.set(id, {});
+		ctrls.set(id, ControlStorage{});
 	}
 
 	std::vector<v4l2_ext_control> v4l2Ctrls(ids.size());
diff --git a/src/py/libcamera/py_helpers.cpp b/src/py/libcamera/py_helpers.cpp
index 75283396b1dca7b5e16695e11630496cf1104e3a..bd269745a6dc41cc22d4ac0a8f79f1017e130bb2 100644
--- a/src/py/libcamera/py_helpers.cpp
+++ b/src/py/libcamera/py_helpers.cpp
@@ -16,7 +16,7 @@  namespace py = pybind11;
 using namespace libcamera;
 
 template<typename T>
-static py::object valueOrTuple(const ControlStorage &cv)
+static py::object valueOrTuple(const ControlValue &cv)
 {
 	if (cv.isArray()) {
 		const T *v = reinterpret_cast<const T *>(cv.data().data());
@@ -31,7 +31,7 @@  static py::object valueOrTuple(const ControlStorage &cv)
 	return py::cast(cv.get<T>());
 }
 
-py::object controlValueToPy(const ControlStorage &cv)
+py::object controlValueToPy(const ControlValue &cv)
 {
 	switch (cv.type()) {
 	case ControlTypeNone:
diff --git a/src/py/libcamera/py_helpers.h b/src/py/libcamera/py_helpers.h
index 47a0ee462c3c3396504c22a99173d83b0d549366..a7dd565e9e64ca3a1fd7d008da96619fac8374fe 100644
--- a/src/py/libcamera/py_helpers.h
+++ b/src/py/libcamera/py_helpers.h
@@ -9,5 +9,5 @@ 
 
 #include <pybind11/pybind11.h>
 
-pybind11::object controlValueToPy(const libcamera::ControlStorage &cv);
+pybind11::object controlValueToPy(const libcamera::ControlValue &cv);
 libcamera::ControlStorage pyToControlValue(const pybind11::object &ob, libcamera::ControlType type);
diff --git a/test/serialization/serialization_test.cpp b/test/serialization/serialization_test.cpp
index 4b5188972deed3f473f75527fb279148a1d52c40..9d2e282281db69d5188c4521d9127d951a45f3b6 100644
--- a/test/serialization/serialization_test.cpp
+++ b/test/serialization/serialization_test.cpp
@@ -78,12 +78,12 @@  bool SerializationTest::equals(const ControlList &lhs, const ControlList &rhs)
 	cerr << "lhs:" << endl;
 	for (const auto &value : rlhs)
 		cerr << "- " << value.first << ": "
-		     << value.second.toString() << endl;
+		     << value.second << endl;
 
 	cerr << "rhs:" << endl;
 	for (const auto &value : rrhs)
 		cerr << "- " << value.first << ": "
-		     << value.second.toString() << endl;
+		     << value.second << endl;
 
 	return false;
 }
diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
index b0130295e47caddc4d9b85f61a89e1405694b89b..1f3d672b91cad2d14c04114cc6f971c8dc72ce34 100644
--- a/test/v4l2_videodevice/controls.cpp
+++ b/test/v4l2_videodevice/controls.cpp
@@ -86,7 +86,7 @@  protected:
 		 * The VIVID_CID_INTEGER64 control can take any value, just test
 		 * that its value can be retrieved and has the right type.
 		 */
-		ctrls.get(VIVID_CID_INTEGER64).get<int64_t>();
+		(void)ctrls.get(VIVID_CID_INTEGER64).get<int64_t>();
 
 		uint8_t u8Min = u8.min().get<uint8_t>();
 		uint8_t u8Max = u8.max().get<uint8_t>();
@@ -129,7 +129,7 @@  protected:
 
 		if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() ||
 		    ctrls.get(V4L2_CID_CONTRAST) != contrast.max() ||
-		    ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) {
+		    ctrls.get(V4L2_CID_SATURATION).get<int32_t>() != saturation.min().get<int32_t>() + 1) {
 			cerr << "Controls not updated when set" << endl;
 			return TestFail;
 		}