libcamera: controls: Create ControlInfoMap with ControlIdMap

ControlInfoMap does not have a ControlId map associated, but rather
creates one with the generateIdMap() function at creation time.

As a consequence, when in the need to de-serialize a ControlInfoMap all
the ControlId it contains are created by the deserializer instance, not
being able to discern if the controls the ControlIdMap refers to are the
global libcamera controls (and properties) or instances local to the
V4L2 device that has first initialized the controls.

As a consequence the ControlId stored in a de-serialized map will always
be newly created entities, preventing lookup by ControlId reference on a
de-serialized ControlInfoMap.

In order to make it possible to use globally available ControlId
instances whenever possible, create ControlInfoMap with a reference to
an externally allocated ControlIdMap instead of generating one
internally.

As a consequence the class constructors take and additional argument,
which might be not pleasant to type in, but enforces the concepts that
ControlInfoMap should be created with controls part of the same id map.

As the ControlIdMap the ControlInfoMap refers to needs to be allocated
externally:
- Use the globally available controls::controls (or
  properties::properties) id map when referring to libcamera controls
- The V4L2 device that creates ControlInfoMap by parsing the device's
  controls has to allocate a ControlIdMap
- The ControlSerializer that de-serializes a ControlInfoMap has to
  create and store the ControlIdMap the de-serialized info map refers to

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

12 files changed, 104 insertions, 110 deletions

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index de733bd8..9b0d5a54 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -309,12 +309,11 @@ public:
 
 	ControlInfoMap() = default;
 	ControlInfoMap(const ControlInfoMap &other) = default;
-	ControlInfoMap(std::initializer_list<Map::value_type> init);
-	ControlInfoMap(Map &&info);
+	ControlInfoMap(std::initializer_list<Map::value_type> init,
+		       const ControlIdMap &idmap);
+	ControlInfoMap(Map &&info, const ControlIdMap &idmap);
 
 	ControlInfoMap &operator=(const ControlInfoMap &other) = default;
-	ControlInfoMap &operator=(std::initializer_list<Map::value_type> init);
-	ControlInfoMap &operator=(Map &&info);
 
 	using Map::key_type;
 	using Map::mapped_type;
@@ -339,12 +338,12 @@ public:
 	iterator find(unsigned int key);
 	const_iterator find(unsigned int key) const;
 
-	const ControlIdMap &idmap() const { return idmap_; }
+	const ControlIdMap &idmap() const { return *idmap_; }
 
 private:
-	void generateIdmap();
+	bool validate();
 
-	ControlIdMap idmap_;
+	const ControlIdMap *idmap_;
 };
 
 class ControlList
diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
index 7d4426c9..8a66be32 100644
--- a/include/libcamera/internal/control_serializer.h
+++ b/include/libcamera/internal/control_serializer.h
@@ -48,6 +48,7 @@ private:
 
 	unsigned int serial_;
 	std::vector<std::unique_ptr<ControlId>> controlIds_;
+	std::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;
 	std::map<unsigned int, ControlInfoMap> infoMaps_;
 	std::map<const ControlInfoMap *, unsigned int> infoMapHandles_;
 };
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index 77b835b3..423c8fb1 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -69,6 +69,7 @@ private:
 
 	std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
 	std::vector<std::unique_ptr<ControlId>> controlIds_;
+	ControlIdMap controlIdMap_;
 	ControlInfoMap controls_;
 	std::string deviceNode_;
 	int fd_;
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index a8790000..521eaecd 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -27,26 +27,26 @@ namespace RPi {
  * and the pipeline handler may be reverted so that it aborts when an
  * unsupported control is encountered.
  */
-static const ControlInfoMap Controls = {
-	{ &controls::AeEnable, ControlInfo(false, true) },
-	{ &controls::ExposureTime, ControlInfo(0, 999999) },
-	{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
-	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
-	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
-	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
-	{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
-	{ &controls::AwbEnable, ControlInfo(false, true) },
-	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
-	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
-	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
-	{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
-	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
-	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
-	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
-	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
-	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
-	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
-};
+static const ControlInfoMap Controls({
+		{ &controls::AeEnable, ControlInfo(false, true) },
+		{ &controls::ExposureTime, ControlInfo(0, 999999) },
+		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
+		{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
+		{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
+		{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
+		{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
+		{ &controls::AwbEnable, ControlInfo(false, true) },
+		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
+		{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
+		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
+		{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
+		{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
+		{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
+		{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
+		{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
+		{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
+		{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
+	}, controls::controls);
 
 } /* namespace RPi */
 
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index 6a65999f..ed456fd4 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -93,6 +93,7 @@ void ControlSerializer::reset()
 	infoMapHandles_.clear();
 	infoMaps_.clear();
 	controlIds_.clear();
+	controlIdMaps_.clear();
 }
 
 size_t ControlSerializer::binarySize(const ControlValue &value)
@@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 	}
 
 	ControlInfoMap::Map ctrls;
+	controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
+	ControlIdMap *localIdMap = controlIdMaps_.back().get();
 
 	for (unsigned int i = 0; i < hdr->entries; ++i) {
 		const struct ipa_control_info_entry *entry =
@@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 		 * purpose.
 		 */
 		controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
+		ControlId *controlId = controlIds_.back().get();
+		(*localIdMap)[entry->id] = controlId;
 
 		if (entry->offset != values.offset()) {
 			LOG(Serializer, Error)
@@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 		}
 
 		/* Create and store the ControlInfo. */
-		ctrls.emplace(controlIds_.back().get(),
-			      loadControlInfo(type, values));
+		ctrls.emplace(controlId, loadControlInfo(type, values));
 	}
 
 	/*
 	 * Create the ControlInfoMap in the cache, and store the map to handle
 	 * association.
 	 */
-	ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);
+	infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);
+	ControlInfoMap &map = infoMaps_[hdr->handle];
 	infoMapHandles_[&map] = hdr->handle;
 
 	return map;
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 64fd5c29..5c05ba4a 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -625,10 +625,10 @@ std::string ControlInfo::toString() const
  * constructed, and thus only exposes the read accessors of the
  * std::unsorted_map<> base class.
  *
- * In addition to the features of the standard unsorted map, this class also
- * provides access to the mapped elements using numerical ID keys. It maintains
- * an internal map of numerical ID to ControlId for this purpose, and exposes it
- * through the idmap() function to help construction of ControlList instances.
+ * The class is constructed with a reference to a ControlIdMap. This allows
+ * providing access to the mapped elements using numerical ID keys, in addition
+ * to the features of the standard unsorted map. All ControlId keys in the map
+ * must appear in the ControlIdMap.
  */
 
 /**
@@ -645,24 +645,27 @@ std::string ControlInfo::toString() const
 /**
  * \brief Construct a ControlInfoMap from an initializer list
  * \param[in] init The initializer list
+ * \param[in] idmap The idmap used by the ControlInfoMap
  */
-ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)
-	: Map(init)
+ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,
+			       const ControlIdMap &idmap)
+	: Map(init), idmap_(&idmap)
 {
-	generateIdmap();
+	ASSERT(validate());
 }
 
 /**
  * \brief Construct a ControlInfoMap from a plain map
  * \param[in] info The control info plain map
+ * \param[in] idmap The idmap used by the ControlInfoMap
  *
  * Construct a new ControlInfoMap and populate its contents with those of
  * \a info using move semantics. Upon return the \a info map will be empty.
  */
-ControlInfoMap::ControlInfoMap(Map &&info)
-	: Map(std::move(info))
+ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)
+	: Map(std::move(info)), idmap_(&idmap)
 {
-	generateIdmap();
+	ASSERT(validate());
 }
 
 /**
@@ -672,32 +675,41 @@ ControlInfoMap::ControlInfoMap(Map &&info)
  * \return A reference to the ControlInfoMap
  */
 
-/**
- * \brief Replace the contents with those from the initializer list
- * \param[in] init The initializer list
- * \return A reference to the ControlInfoMap
- */
-ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)
+bool ControlInfoMap::validate()
 {
-	Map::operator=(init);
-	generateIdmap();
-	return *this;
-}
+	for (const auto &ctrl : *this) {
+		const ControlId *id = ctrl.first;
+		auto it = idmap_->find(id->id());
 
-/**
- * \brief Move assignment operator from a plain map
- * \param[in] info The control info plain map
- *
- * Populate the map by replacing its contents with those of \a info using move
- * semantics. Upon return the \a info map will be empty.
- *
- * \return A reference to the populated ControlInfoMap
- */
-ControlInfoMap &ControlInfoMap::operator=(Map &&info)
-{
-	Map::operator=(std::move(info));
-	generateIdmap();
-	return *this;
+		/*
+		 * Make sure all control ids are part of the idmap and verify
+		 * the control info matches the expected type.
+		 */
+		if (it == idmap_->end() || it->second != id) {
+			LOG(Controls, Error)
+				<< "Control " << utils::hex(id->id())
+				<< " not in the idmap";
+			return false;
+		}
+
+		/*
+		 * For string controls, min and max define the valid
+		 * range for the string size, not for the individual
+		 * values.
+		 */
+		ControlType rangeType = id->type() == ControlTypeString
+				      ? ControlTypeInteger32 : id->type();
+		const ControlInfo &info = ctrl.second;
+
+		if (info.min().type() != rangeType) {
+			LOG(Controls, Error)
+				<< "Control " << utils::hex(id->id())
+				<< " type and info type mismatch";
+			return false;
+		}
+	}
+
+	return true;
 }
 
 /**
@@ -707,7 +719,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
  */
 ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
 {
-	return at(idmap_.at(id));
+	return at(idmap_->at(id));
 }
 
 /**
@@ -717,7 +729,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
  */
 const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
 {
-	return at(idmap_.at(id));
+	return at(idmap_->at(id));
 }
 
 /**
@@ -732,7 +744,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
 	 * entries, we can thus just count the matching entries in idmap to
 	 * avoid an additional lookup.
 	 */
-	return idmap_.count(id);
+	return idmap_->count(id);
 }
 
 /**
@@ -743,8 +755,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
  */
 ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
 {
-	auto iter = idmap_.find(id);
-	if (iter == idmap_.end())
+	auto iter = idmap_->find(id);
+	if (iter == idmap_->end())
 		return end();
 
 	return find(iter->second);
@@ -758,8 +770,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
  */
 ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
 {
-	auto iter = idmap_.find(id);
-	if (iter == idmap_.end())
+	auto iter = idmap_->find(id);
+	if (iter == idmap_->end())
 		return end();
 
 	return find(iter->second);
@@ -776,33 +788,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
  * \return The ControlId map
  */
 
-void ControlInfoMap::generateIdmap()
-{
-	idmap_.clear();
-
-	for (const auto &ctrl : *this) {
-		/*
-		 * For string controls, min and max define the valid
-		 * range for the string size, not for the individual
-		 * values.
-		 */
-		ControlType rangeType = ctrl.first->type() == ControlTypeString
-				      ? ControlTypeInteger32 : ctrl.first->type();
-		const ControlInfo &info = ctrl.second;
-
-		if (info.min().type() != rangeType) {
-			LOG(Controls, Error)
-				<< "Control " << utils::hex(ctrl.first->id())
-				<< " type and info type mismatch";
-			idmap_.clear();
-			clear();
-			return;
-		}
-
-		idmap_[ctrl.first->id()] = ctrl.first;
-	}
-}
-
 /**
  * \class ControlList
  * \brief Associate a list of ControlId with their values for an object
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 19cb5c4e..9c23788e 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1071,7 +1071,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 
 	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
 
-	data->controlInfo_ = std::move(controls);
+	data->controlInfo_ = ControlInfoMap(std::move(controls),
+					    controls::controls);
 
 	return 0;
 }
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 42911a8f..710b9309 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 		      std::forward_as_tuple(&controls::AeEnable),
 		      std::forward_as_tuple(false, true));
 
-	data->controlInfo_ = std::move(ctrls);
+	data->controlInfo_ = ControlInfoMap(std::move(ctrls),
+					    controls::controls);
 
 	data->sensor_ = std::make_unique<CameraSensor>(sensor);
 	ret = data->sensor_->init();
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 0f634b8d..573d8042 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)
 		addControl(cid, info, &ctrls);
 	}
 
-	controlInfo_ = std::move(ctrls);
+	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
 
 	return 0;
 }
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 12f7517f..d4b041d3 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -512,7 +512,7 @@ int VimcCameraData::init()
 		ctrls.emplace(id, info);
 	}
 
-	controlInfo_ = std::move(ctrls);
+	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
 
 	/* Initialize the camera properties. */
 	properties_ = sensor_->properties();
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index b0a70def..951592c6 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -611,12 +611,13 @@ void V4L2Device::listControls()
 				 << " (" << utils::hex(ctrl.id) << ")";
 
 		controlIds_.emplace_back(v4l2ControlId(ctrl));
+		controlIdMap_[ctrl.id] = controlIds_.back().get();
 		controlInfo_.emplace(ctrl.id, ctrl);
 
 		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
 	}
 
-	controls_ = std::move(ctrls);
+	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
 }
 
 /**
diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
index 880bcd02..bf7e34e2 100644
--- a/test/serialization/ipa_data_serializer_test.cpp
+++ b/test/serialization/ipa_data_serializer_test.cpp
@@ -32,13 +32,13 @@
 using namespace std;
 using namespace libcamera;
 
-static const ControlInfoMap Controls = {
-	{ &controls::AeEnable, ControlInfo(false, true) },
-	{ &controls::ExposureTime, ControlInfo(0, 999999) },
-	{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
-	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
-	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
-};
+static const ControlInfoMap Controls = ControlInfoMap({
+		{ &controls::AeEnable, ControlInfo(false, true) },
+		{ &controls::ExposureTime, ControlInfo(0, 999999) },
+		{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
+		{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
+		{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
+	}, controls::controls);
 
 namespace libcamera {