[libcamera-devel] [PATCH 4/6] libcamera: control_serializer: Use the right idmap
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 3 14:58:30 CEST 2021
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.
> > > 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 ?
> > 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.
> >
> > 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
More information about the libcamera-devel
mailing list