[libcamera-devel] [PATCH 4/6] libcamera: control_serializer: Use the right idmap
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 3 16:40:58 CEST 2021
Hi Jacopo,
On Fri, Sep 03, 2021 at 03:36:15PM +0200, Jacopo Mondi wrote:
> On Fri, Sep 03, 2021 at 03:58:30PM +0300, Laurent Pinchart wrote:
> > 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 :)
Fine with me. I think sending properties from the IPA to the pipeline
handler at init time is probably a valid use case.
> > > > > 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.
I'll consider that as a bug, so ASSERT() is good enough.
> > > > 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
>
> > > > 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