[libcamera-devel] [PATCH v3 3/5] libcamera: controls: Use ControlIdMap in deserialization
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 10 02:53:07 CEST 2021
Hi Jacopo,
Thank you for the patch.
On Mon, Aug 09, 2021 at 05:23:06PM +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 us 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>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> include/libcamera/ipa/ipa_controls.h | 9 +++-
> src/libcamera/control_serializer.cpp | 65 +++++++++++++++++++++++-----
> src/libcamera/ipa_controls.cpp | 29 +++++++++++++
> 3 files changed, 90 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> index 6d3bf279c22d..f4bb7b77a974 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 ipa_controls_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];
> + ipa_controls_id_map_type id_map_type;
In C you need to include the enum keyword.
> + 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..9127678f25f6 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,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> for (const auto &ctrl : infoMap)
> valuesSize += binarySize(ctrl.second);
>
> + const ControlIdMap *idmap = &infoMap.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;
> +
> /* Prepare the packet header, assign a handle to the ControlInfoMap. */
> struct ipa_controls_header hdr;
> hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> @@ -195,6 +205,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> hdr.entries = infoMap.size();
> hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> hdr.data_offset = sizeof(hdr) + entriesSize;
> + hdr.id_map_type = idMapType;
>
> buffer.write(&hdr);
>
> @@ -368,6 +379,33 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> return {};
> }
>
> + /*
> + * 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.
Copied and pasted from e-mail, with spaces instead of tabs ? :-)
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> + const ControlIdMap *idMap = nullptr;
> + ControlIdMap *localIdMap = 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:
> + controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> + localIdMap = controlIdMaps_.back().get();
> + idMap = localIdMap;
> + break;
> + default:
> + LOG(Serializer, Error)
> + << "Unknown id map type: " << hdr->id_map_type;
> + return {};
> + }
> +
> ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));
> ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);
>
> @@ -377,9 +415,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 +423,21 @@ 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.
> - */
> - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
> - ControlId *controlId = controlIds_.back().get();
> - (*localIdMap)[entry->id] = controlId;
> +
> + /* 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)
> @@ -413,7 +454,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> * Create the ControlInfoMap in the cache, and store the map to handle
> * association.
> */
> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);
> + infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);
> ControlInfoMap &map = infoMaps_[hdr->handle];
> infoMapHandles_[&map] = hdr->handle;
>
> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> index 8fd726513182..fb98cda30ede 100644
> --- a/src/libcamera/ipa_controls.cpp
> +++ b/src/libcamera/ipa_controls.cpp
> @@ -134,6 +134,33 @@
> * \brief The current control serialization format version
> */
>
> +/**
> + * \var ipa_controls_id_map_type
> + * \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 ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_CONTROLS
> + * \brief The numerical control identifier are resolved to a ControlId * using
> + * the global controls::controls id map
> + * \var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_PROPERTIES
> + * \brief The numerical control identifier are resolved to a ControlId * using
> + * the global properties::properties id map
> + * \var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_V4L2
> + * \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 +176,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::id_map_type
> + * The id map type as defined by the ipa_controls_id_map_type enumeration
> * \var ipa_controls_header::reserved
> * Reserved for future extensions
> */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list