[libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use ControlIdMap in deserialization
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 3 18:21:20 CEST 2021
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>
> ---
> 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