[libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use ControlIdMap in deserialization
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 9 15:58:38 CEST 2021
Hi Laurent,
On Tue, Aug 03, 2021 at 07:41:45PM +0300, Laurent Pinchart wrote:
> 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").
>
I don't think this series solves the issue mentioned in the bug, not
in full at least. If the idmap the ControlList refers to is one the
globally defined ones, then yes, they get re-used and the assertion
could be brought back in place. For ControlList that reference V4L2
controls the serializer still creates its own local copy of the idmap,
so the assertion would fail.
we can assert for controls::controls and properties::properties, but
for locally created idmaps there's no way around checking that all
elemenents in one list are present in the other list as well.
> > > ---
> > > 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