[libcamera-devel] [PATCH v3 3/5] libcamera: control_serializer: Use the right idmap

[Jacopo Mondi]

Hi Laurent,

On Sun, Sep 26, 2021 at 10:56:52PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 24, 2021 at 07:25:23PM +0200, 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>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index b3590fdb93ec..af851b4661c8 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);
>
> A reference can't be null, that would be an undefined behaviour. The

Ah, right, we return a reference by dereferencing the idmap_ pointer
unconditionally in ControlInfoMap

	const ControlIdMap &idmap() const { return *idmap_; }

If idmap_ happens to be nullptr we'll have issues and the above ASSERT
clearly can't help. I'll drop it, thanks for spotting!



> compiler can skip the assert here. If you're concerned that
> ControlInfoMap::idmap_ may be null when ControlInfoMap::idmap() is
> called, the assert should go to ControlInfoMap::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 55eec71ffe35..d23eff9e2728 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -1040,6 +1040,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
>
> s/ControlsList/ControlList/
>
> > + * instances constructed with ControlList() have no associated idmap, nullptr is
>
> s/ControlList()/the default constructor/
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + * returned in that case.
> > + */
> > +
> >  const ControlValue *ControlList::find(unsigned int id) const
> >  {
> >  	const auto iter = controls_.find(id);
>
> --
> Regards,
>
> Laurent Pinchart