[libcamera-devel] [PATCH 4/6] libcamera: control_serializer: Use the right idmap

[Kieran Bingham]

On 01/09/2021 15:37, Jacopo Mondi wrote:
> 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 at jmondi.org>
> ---
>  include/libcamera/controls.h         |  1 +
>  src/libcamera/control_serializer.cpp | 49 +++++++++++++++++++++++-----
>  src/libcamera/controls.cpp           |  8 +++++
>  3 files changed, 49 insertions(+), 9 deletions(-)
> 
> 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 08cfecca3078..d4377c024079 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);
>  
> @@ -499,13 +509,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) {
> @@ -517,12 +529,31 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  			return {};
>  		}
>  
> -		infoMap = iter->first;
> +		const ControlInfoMap *infoMap = iter->first;
> +		idMap = &infoMap->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";

Can we have a return statement after the Fatal?

(Presuming that there is no safe path forwards if Fatal is lowered to
Error on Release builds).


> +		}
>  	}
>  
> -	ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);
> +	/*
> +	 * \todo If possible, 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.
> +	 */
> +	ASSERT(idMap);

I presume this assert can only be hit through the
IPA_CONTROL_ID_MAP_V4L2 path above?

Although actually - if there's ever a new IPA_CONTROL_ID_MAP type added,
it guards against that too - so the assert is good to have.

Only minors there:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +	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);
>