| Message ID | 20210924172525.160482-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@jmondi.org> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@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 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@ideasonboard.com> > + * returned in that case. > + */ > + > const ControlValue *ControlList::find(unsigned int id) const > { > const auto iter = controls_.find(id);
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@jmondi.org> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@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@ideasonboard.com> > > > + * returned in that case. > > + */ > > + > > const ControlValue *ControlList::find(unsigned int id) const > > { > > const auto iter = controls_.find(id); > > -- > Regards, > > Laurent Pinchart
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); } 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 + * 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);