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

[Jacopo Mondi]

Hi Laurent,

On Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 03, 2021 at 11:21:46AM +0200, Jacopo Mondi wrote:
> > On Thu, Sep 02, 2021 at 03:34:01PM +0100, Kieran Bingham wrote:
> > > 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.
>
> When does this happen ? Shouldn't we ensure that we first serialize a
> ControInfoMap for the properties ? Or do we not have that ? I'm OK with
> this patch as a short-term fix, but I'd like to understand the big
> picture.
>

Properties are intialized using the properties::properties idmap, as
having a ControlInfoMap for unmutable controls like properties are
doesn't make much sense. Hence they won't even have an handle
associated, and the current implementation uses controls::controls as
idmap if not handle is available.

We don't triggere the issue as we don't serialize properties, but as
soon as it happens, we're screwed.

And yes, we can catch it during review but we'll forget or we carry
around the issue untile someone gets fed up and addresses it (see
ControlInfoMap::def() which is a small change that was due since a
year or so).

Given that the patch is not super complex I would rather fix it now :)

> > > > 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;
> >
> > Ups, missing a line break here!
> >
> > > > +		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).
> >
> > Right! In production builds this won't exit. I'll add a return
> >
> > > > +		}
> > > >  	}
> > > >
> > > > -	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?
> >
> > It could actually be hit through the if (hdr->handle) branch
> > where the idMap is retrieved from the infoMap
>
> Can this happen in practice, or would it be a bug somewhere ?

Only if someone tries to serialize a ControlList constructed through
the default constructor, which is unlikely, but possible.

>
> > > 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.
> >
> > If we add a new IPA_CONTROL_ID_MAP type the compiler will warn that
> > not all the cases are handled in the switch statment, as I didn't add
> > a default: case precisely for that reason.
> >
> > So yes, I could move the ASSERT() in the if() {} branch actually.
>
> Sounds good to me.
>

Thanks
   j

> > >
> > > 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);
>
> --
> Regards,
>
> Laurent Pinchart