[libcamera-devel,v2,3/5] libcamera: control_serializer: Use the right idmap
diff mbox series

Message ID 20210907111038.739104-4-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: control serializer fixes
Related show

Commit Message

Jacopo Mondi Sept. 7, 2021, 11:10 a.m. UTC 
When a ControlList is deserialized, the code searches for a valid
ControlInfoMap in the local cache and use its id map to initialize the
list. If no valid ControlInfoMap is found, as it's usually the case
for lists transporting libcamera controls and properties, the globally
defined controls::controls id map is used unconditionally.

This breaks the deserialization of libcamera properties, for which a
wrong idmap is used at construction time.

As the serialization header now transports an id_map_type field, store
the idmap type at serialization time, and re-use it at
deserialization time to identify the correct id map.

Also make the validation stricter by imposing to list of V4L2 controls to
have an associated ControlInfoMap available, as there is no globally
defined idmap for such controls.

To be able to retrieve the idmap associated with a ControlList, add an
accessor function to the ControlList class.

It might be worth in future using a ControlInfoMap to initialize the
deserialized ControlList to implement controls validation against their
limit. As such validation is not implemented at the moment, maintain the
current behaviour and initialize the control list with an idmap.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/controls.h         |  1 +
 src/libcamera/control_serializer.cpp | 51 +++++++++++++++++++++++-----
 src/libcamera/controls.cpp           |  8 +++++
 3 files changed, 51 insertions(+), 9 deletions(-)

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 6668e4bb1053..8e5bc8b70b94 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -407,6 +407,7 @@  public:
 	void set(unsigned int id, const ControlValue &value);
 
 	const ControlInfoMap *infoMap() const { return infoMap_; }
+	const ControlIdMap *idMap() const { return idmap_; }
 
 private:
 	const ControlValue *find(unsigned int id) const;
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index 27360526f9eb..d42840cddecb 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -275,6 +275,15 @@  int ControlSerializer::serialize(const ControlList &list,
 		infoMapHandle = 0;
 	}
 
+	const ControlIdMap *idmap = list.idMap();
+	enum ipa_controls_id_map_type idMapType;
+	if (idmap == &controls::controls)
+		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
+	else if (idmap == &properties::properties)
+		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
+	else
+		idMapType = IPA_CONTROL_ID_MAP_V4L2;
+
 	size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);
 	size_t valuesSize = 0;
 	for (const auto &ctrl : list)
@@ -287,6 +296,7 @@  int ControlSerializer::serialize(const ControlList &list,
 	hdr.entries = list.size();
 	hdr.size = sizeof(hdr) + entriesSize + valuesSize;
 	hdr.data_offset = sizeof(hdr) + entriesSize;
+	hdr.id_map_type = idMapType;
 
 	buffer.write(&hdr);
 
@@ -496,13 +506,15 @@  ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
 	}
 
 	/*
-	 * Retrieve the ControlInfoMap associated with the ControlList based on
-	 * its ID. The mapping between infoMap and ID is set up when serializing
-	 * or deserializing ControlInfoMap. If no mapping is found (which is
-	 * currently the case for ControlList related to libcamera controls),
-	 * use the global control::control idmap.
+	 * Retrieve the ControlIdMap associated with the ControlList.
+	 *
+	 * The idmap is either retrieved from the list's ControlInfoMap when
+	 * a valid handle has been initialized at serialization time, or by
+	 * using the header's id_map_type field for lists that refer to the
+	 * globally defined libcamera controls and properties, for which no
+	 * ControlInfoMap is available.
 	 */
-	const ControlInfoMap *infoMap;
+	const ControlIdMap *idMap;
 	if (hdr->handle) {
 		auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),
 					 [&](decltype(infoMapHandles_)::value_type &entry) {
@@ -514,12 +526,33 @@  ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
 			return {};
 		}
 
-		infoMap = iter->first;
+		const ControlInfoMap *infoMap = iter->first;
+		idMap = &infoMap->idmap();
+		ASSERT(idMap);
 	} else {
-		infoMap = nullptr;
+		switch (hdr->id_map_type) {
+		case IPA_CONTROL_ID_MAP_CONTROLS:
+			idMap = &controls::controls;
+			break;
+
+		case IPA_CONTROL_ID_MAP_PROPERTIES:
+			idMap = &properties::properties;
+			break;
+
+		case IPA_CONTROL_ID_MAP_V4L2:
+			LOG(Serializer, Fatal)
+				<< "A list of V4L2 controls requires an ControlInfoMap";
+			return {};
+		}
 	}
 
-	ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);
+	/*
+	 * \todo When available, initialize the list with the ControlInfoMap
+	 * so that controls can be validated against their limits.
+	 * Currently no validation is performed, so it's fine relying on the
+	 * idmap only.
+	 */
+	ControlList ctrls(*idMap);
 
 	for (unsigned int i = 0; i < hdr->entries; ++i) {
 		const struct ipa_control_value_entry *entry =
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 5c05ba4a7cd0..96625edb1380 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -1038,6 +1038,14 @@  void ControlList::set(unsigned int id, const ControlValue &value)
  * associated ControlInfoMap, nullptr is returned in that case.
  */
 
+/**
+ * \fn ControlList::idMap()
+ * \brief Retrieve the ControlId map used to construct the ControlList
+ * \return The ControlId map used to construct the ControlList. ControlsList
+ * instances constructed with ControlList() have no associated idmap, nullptr is
+ * returned in that case.
+ */
+
 const ControlValue *ControlList::find(unsigned int id) const
 {
 	const auto iter = controls_.find(id);