[libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use ControlIdMap in deserialization
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 3 18:41:45 CEST 2021
On Tue, Aug 03, 2021 at 07:21:21PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jul 28, 2021 at 06:11:13PM +0200, Jacopo Mondi wrote:
> > Introduce a new field in the controls serialization protocol to
> > allow discerning which ControlIdMap a ControlInfoMap refers to.
> >
> > The newly introduced IdMapType enumeration describes the possible
> > info maps:
> > - Either the globally available controls::controls and
> > properties::properties maps, which are valid across IPC boundaries
> > - A ControlIdMap created locally by the V4L2 device, which is not valid
> > across the IPC boundaries
> >
> > At de-serialization time the idMapType filed is inspected and
> > - If the idmap is a globally available one, there's no need to create
> > new ControlId instances when populating the de-serialized
> > ControlInfoMap. Use the globally available map to retrieve the
> > ControlId reference and use it
> > - If the idmap is a map only available locally, create a new ControlId
> > as it used to happen before this patch.
> >
> > As a direct consequence, this change allows to perform lookup by
> > ControlId reference on de-serialized ControlIdMap that refers to the
> > libcamera defined controls::controls and properties::properties.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
This is related to https://bugs.libcamera.org/show_bug.cgi?id=31, right
? If so, should the series include a patch that addresses the todo
comment in ControlList::merge() ? Maybe it could be as simple as a
revert of 414babb60b54 ("libcamera: controls: Remove merge assertion").
> > ---
> > include/libcamera/ipa/ipa_controls.h | 9 +++-
> > src/libcamera/control_serializer.cpp | 73 +++++++++++++++++++++++-----
> > src/libcamera/ipa_controls.cpp | 31 ++++++++++++
> > 3 files changed, 101 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> > index 6d3bf279c22d..5b1690066314 100644
> > --- a/include/libcamera/ipa/ipa_controls.h
> > +++ b/include/libcamera/ipa/ipa_controls.h
> > @@ -15,13 +15,20 @@ extern "C" {
> >
> > #define IPA_CONTROLS_FORMAT_VERSION 1
> >
> > +enum class IdMapType {
> > + LIBCAMERA_CONTROLS,
> > + LIBCAMERA_PROPERTIES,
> > + V4L2_CONTROLS,
> > +};
>
> This is in an extern "C" block, as the structures defined herein were
> meant to be useable from C code. "enum class" is thus not valid. Let's
> name this enum ipa_control_id_map_type to match the rest of the file:
>
> enum ipa_control_id_map_type {
> IPA_CONTROL_ID_MAP_CONTROLS,
> IPA_CONTROL_ID_MAP_PROPERTIES,
> IPA_CONTROL_ID_MAP_V4L2,
> };
>
> > +
> > struct ipa_controls_header {
> > uint32_t version;
> > uint32_t handle;
> > uint32_t entries;
> > uint32_t size;
> > uint32_t data_offset;
> > - uint32_t reserved[3];
> > + IdMapType idMapType;
>
> And this field should then be named id_map_type.
>
> > + uint32_t reserved[2];
> > };
> >
> > struct ipa_control_value_entry {
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index df6ed93f477e..255f9d30fb85 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -17,6 +17,7 @@
> > #include <libcamera/control_ids.h>
> > #include <libcamera/controls.h>
> > #include <libcamera/ipa/ipa_controls.h>
> > +#include <libcamera/property_ids.h>
> >
> > #include "libcamera/internal/byte_stream_buffer.h"
> >
> > @@ -188,6 +189,16 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > for (const auto &ctrl : infoMap)
> > valuesSize += binarySize(ctrl.second);
> >
> > + /* Serialize the id map type to be used when de-serializing. */
>
> It's kind of implied that the data we serialize is used for
> deserialization :-)
>
> > + const ControlIdMap *idmap = &infoMap.idmap();
> > + enum IdMapType idMapType;
> > + if (idmap == &controls::controls)
> > + idMapType = IdMapType::LIBCAMERA_CONTROLS;
> > + else if (idmap == &properties::properties)
> > + idMapType = IdMapType::LIBCAMERA_PROPERTIES;
> > + else
> > + idMapType = IdMapType::V4L2_CONTROLS;
> > +
> > /* Prepare the packet header, assign a handle to the ControlInfoMap. */
> > struct ipa_controls_header hdr;
> > hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> > @@ -195,6 +206,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > hdr.entries = infoMap.size();
> > hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> > hdr.data_offset = sizeof(hdr) + entriesSize;
> > + hdr.idMapType = idMapType;
> >
> > buffer.write(&hdr);
> >
> > @@ -368,6 +380,34 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > return {};
> > }
> >
> > + /*
> > + * Use the id map type to retrieve the ControlId instances associated
> > + * to the serialized numerical identifier.
>
> s/to/with/
>
> > + *
> > + * If a globally available id map such as controls::controls or
>
> s/available/defined/
>
> > + * properties::properties is used, the ControlId instances can be
> > + * retrieved from there.
> > + *
> > + * If, otherwise, the idmap is valid only on one side of the IPC
> > + * boundary (as in case it has been created by the V4L2 devices)
> > + * the deserializer shall create new ControlId and store them in an
> > + * locally created id map.
>
> Could this be simplified to
>
> * Use the ControlIdMap corresponding to the id map type. If the type
> * references a globally defined id map (such as controls::controls
> * or properties::properties), Use it. Otherwise, create a local id map
> * that will be populated with dynamically created ControlId instances
> * when deserializing individual ControlInfoMap entries.
>
> > + */
> > + const ControlIdMap *idMap = nullptr;
> > + ControlIdMap *localIdMap;
>
> I'd initialize this to nullptr as well to ensure it will never be used
> uninitialized.
>
> > + switch (hdr->idMapType) {
> > + case IdMapType::LIBCAMERA_CONTROLS:
> > + idMap = &controls::controls;
> > + break;
> > + case IdMapType::LIBCAMERA_PROPERTIES:
> > + idMap = &properties::properties;
> > + break;
> > + case IdMapType::V4L2_CONTROLS:
> > + controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> > + localIdMap = controlIdMaps_.back().get();
>
> I'd add
>
> idMap = localIdMap;
>
> here (see below).
>
> > + break;
>
> Never trust a binary stream:
>
> default:
> return {};
>
> > + }
> > +
> > ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));
> > ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);
> >
> > @@ -377,9 +417,6 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > }
> >
> > ControlInfoMap::Map ctrls;
> > - controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> > - ControlIdMap *localIdMap = controlIdMaps_.back().get();
> > -
> > for (unsigned int i = 0; i < hdr->entries; ++i) {
> > const struct ipa_control_info_entry *entry =
> > entries.read<decltype(*entry)>();
> > @@ -388,15 +425,27 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > return {};
> > }
> >
> > - /* Create and cache the individual ControlId. */
> > ControlType type = static_cast<ControlType>(entry->type);
> > - /**
> > - * \todo Find a way to preserve the control name for debugging
> > - * purpose.
> > +
> > + /*
> > + * If we have a valid id map, reuse the control id there stored,
> > + * Otherwise populate the local idmap with a new ControlId
> > + * reference.
> > */
> > - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
> > - ControlId *controlId = controlIds_.back().get();
> > - (*localIdMap)[entry->id] = controlId;
> > + const ControlId *controlId;
> > + if (idMap) {
> > + controlId = idMap->at(entry->id);
> > + ASSERT(controlId);
> > + } else {
> > + /**
> > + * \todo Find a way to preserve the control name for
> > + * debugging purpose.
> > + */
> > + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
> > + "", type));
> > + controlId = controlIds_.back().get();
> > + (*localIdMap)[entry->id] = controlId;
> > + }
>
> Would this be more readable (related to the "see below" comment above) ?
>
> /* If we're using a local id map, populate it. */
> if (localIdMap) {
> /**
> * \todo Find a way to preserve the control name for
> * debugging purpose.
> */
> controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
> "", type));
> (*localIdMap)[entry->id] = controlIds_.back().get();
> }
>
> const ControlId *controlId = idMap->at(entry->id);
> ASSERT(controlId);
>
> >
> > if (entry->offset != values.offset()) {
> > LOG(Serializer, Error)
> > @@ -405,10 +454,12 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > return {};
> > }
> >
> > - /* Create and store the ControlInfo. */
> > ctrls.emplace(controlId, loadControlInfo(type, values));
> > }
> >
> > + if (!idMap)
> > + idMap = localIdMap;
> > +
>
> And this could be dropped.
>
> > /*
> > * Create the ControlInfoMap in the cache, and store the map to handle
> > * association.
> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> > index 8fd726513182..1323fa31a28f 100644
> > --- a/src/libcamera/ipa_controls.cpp
> > +++ b/src/libcamera/ipa_controls.cpp
> > @@ -134,6 +134,35 @@
> > * \brief The current control serialization format version
> > */
> >
> > +/**
> > + * \enum IdMapType
> > + * \brief Enumerates the different control id map types
> > + *
> > + * Each ControlInfoMap and ControlList refers to a control id map that
> > + * associates the ControlId references to a numerical identifier.
> > + * During the serialization procedure the raw pointers to the ControlId
> > + * instances cannot be transported on the wire, hence their numerical id is
> > + * used to identify them in the serialized data buffer. At deserialization time
> > + * it is required to associate back to the numerical id the ControlId instance
> > + * it represents. This enumeration describes which ControlIdMap should be
> > + * used to perform such operation.
> > + *
> > + * \var IdMapType::LIBCAMERA_CONTROLS
> > + * \brief The numerical control identifier are resolved to a ControlId * using
> > + * the global controls::controls id map
> > + *
> > + * \var IdMapType::LIBCAMERA_PROPERTIES
> > + * \brief The numerical control identifier are resolved to a ControlId * using
> > + * the global properties::properties id map
> > + *
> > + * \var IdMapType::V4L2_CONTROLS
> > + * \brief ControlId for V4L2 defined controls are created by the video device
> > + * that enumerates them, and are not available across the IPC boundaries. The
> > + * deserializer shall create new ControlId instances for them as well as store
> > + * them in a dedicated ControlIdMap. Only lookup by numerical id can be
> > + * performed on de-serialized ControlInfoMap that represents V4L2 controls.
> > + */
> > +
> > /**
> > * \struct ipa_controls_header
> > * \brief Serialized control packet header
> > @@ -149,6 +178,8 @@
> > * The total packet size in bytes
> > * \var ipa_controls_header::data_offset
> > * Offset in bytes from the beginning of the packet of the data section start
> > + * \var ipa_controls_header::idMapType
> > + * The id map type as defined by the IdMapType enumeration
> > * \var ipa_controls_header::reserved
> > * Reserved for future extensions
> > */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list