[libcamera-devel] [PATCH v4 1/5] libcamera: controls: Create ControlInfoMap with ControlIdMap
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 11 14:13:37 CEST 2021
Hi Jacopo,
Thank you for the patch.
On Tue, Aug 10, 2021 at 05:12:07PM +0200, Jacopo Mondi wrote:
> ControlInfoMap does not have a ControlId map associated, but rather
> creates one with the generateIdMap() function at creation time.
>
> As a consequence, when in the need to de-serialize a ControlInfoMap all
> the ControlId it contains are created by the deserializer instance, not
> being able to discern if the controls the ControlIdMap refers to are the
> global libcamera controls (and properties) or instances local to the
> V4L2 device that has first initialized the controls.
>
> As a consequence the ControlId stored in a de-serialized map will always
> be newly created entities, preventing lookup by ControlId reference on a
> de-serialized ControlInfoMap.
>
> In order to make it possible to use globally available ControlId
> instances whenever possible, create ControlInfoMap with a reference to
> an externally allocated ControlIdMap instead of generating one
> internally.
>
> As a consequence the class constructors take and additional argument,
> which might be not pleasant to type in, but enforces the concepts that
> ControlInfoMap should be created with controls part of the same id map.
>
> As the ControlIdMap the ControlInfoMap refers to needs to be allocated
> externally:
> - Use the globally available controls::controls (or
> properties::properties) id map when referring to libcamera controls
> - The V4L2 device that creates ControlInfoMap by parsing the device's
> controls has to allocate a ControlIdMap
> - The ControlSerializer that de-serializes a ControlInfoMap has to
> create and store the ControlIdMap the de-serialized info map refers to
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/controls.h | 13 +-
> .../libcamera/internal/control_serializer.h | 1 +
> include/libcamera/internal/v4l2_device.h | 1 +
> include/libcamera/ipa/raspberrypi.h | 40 +++---
> src/libcamera/control_serializer.cpp | 11 +-
> src/libcamera/controls.cpp | 121 ++++++++----------
> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +-
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-
> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> src/libcamera/v4l2_device.cpp | 3 +-
> .../ipa_data_serializer_test.cpp | 14 +-
> 12 files changed, 104 insertions(+), 110 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index de733bd868a6..9b0d5a545301 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -309,12 +309,11 @@ public:
>
> ControlInfoMap() = default;
> ControlInfoMap(const ControlInfoMap &other) = default;
> - ControlInfoMap(std::initializer_list<Map::value_type> init);
> - ControlInfoMap(Map &&info);
> + ControlInfoMap(std::initializer_list<Map::value_type> init,
> + const ControlIdMap &idmap);
> + ControlInfoMap(Map &&info, const ControlIdMap &idmap);
>
> ControlInfoMap &operator=(const ControlInfoMap &other) = default;
> - ControlInfoMap &operator=(std::initializer_list<Map::value_type> init);
> - ControlInfoMap &operator=(Map &&info);
>
> using Map::key_type;
> using Map::mapped_type;
> @@ -339,12 +338,12 @@ public:
> iterator find(unsigned int key);
> const_iterator find(unsigned int key) const;
>
> - const ControlIdMap &idmap() const { return idmap_; }
> + const ControlIdMap &idmap() const { return *idmap_; }
>
> private:
> - void generateIdmap();
> + bool validate();
>
> - ControlIdMap idmap_;
> + const ControlIdMap *idmap_;
> };
>
> class ControlList
> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> index 7d4426c95d12..8a66be324138 100644
> --- a/include/libcamera/internal/control_serializer.h
> +++ b/include/libcamera/internal/control_serializer.h
> @@ -48,6 +48,7 @@ private:
>
> unsigned int serial_;
> std::vector<std::unique_ptr<ControlId>> controlIds_;
> + std::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;
> std::map<unsigned int, ControlInfoMap> infoMaps_;
> std::map<const ControlInfoMap *, unsigned int> infoMapHandles_;
> };
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 77b835b3cb80..423c8fb11845 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -69,6 +69,7 @@ private:
>
> std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
> std::vector<std::unique_ptr<ControlId>> controlIds_;
> + ControlIdMap controlIdMap_;
> ControlInfoMap controls_;
> std::string deviceNode_;
> int fd_;
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a8790000e2d9..521eaecd19b2 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -27,26 +27,26 @@ namespace RPi {
> * and the pipeline handler may be reverted so that it aborts when an
> * unsupported control is encountered.
> */
> -static const ControlInfoMap Controls = {
> - { &controls::AeEnable, ControlInfo(false, true) },
> - { &controls::ExposureTime, ControlInfo(0, 999999) },
> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> - { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> - { &controls::AwbEnable, ControlInfo(false, true) },
> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> - { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> - { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> -};
> +static const ControlInfoMap Controls({
> + { &controls::AeEnable, ControlInfo(false, true) },
> + { &controls::ExposureTime, ControlInfo(0, 999999) },
> + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> + { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> + { &controls::AwbEnable, ControlInfo(false, true) },
> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> + { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> + { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> + }, controls::controls);
>
> } /* namespace RPi */
>
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index 6a65999f8161..ed456fd445a0 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -93,6 +93,7 @@ void ControlSerializer::reset()
> infoMapHandles_.clear();
> infoMaps_.clear();
> controlIds_.clear();
> + controlIdMaps_.clear();
> }
>
> size_t ControlSerializer::binarySize(const ControlValue &value)
> @@ -376,6 +377,8 @@ 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 =
> @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> * purpose.
> */
> controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
> + ControlId *controlId = controlIds_.back().get();
> + (*localIdMap)[entry->id] = controlId;
>
> if (entry->offset != values.offset()) {
> LOG(Serializer, Error)
> @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> }
>
> /* Create and store the ControlInfo. */
> - ctrls.emplace(controlIds_.back().get(),
> - loadControlInfo(type, values));
> + ctrls.emplace(controlId, loadControlInfo(type, values));
> }
>
> /*
> * Create the ControlInfoMap in the cache, and store the map to handle
> * association.
> */
> - ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);
> + infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);
> + ControlInfoMap &map = infoMaps_[hdr->handle];
> infoMapHandles_[&map] = hdr->handle;
>
> return map;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 64fd5c296226..5c05ba4a7cd0 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -625,10 +625,10 @@ std::string ControlInfo::toString() const
> * constructed, and thus only exposes the read accessors of the
> * std::unsorted_map<> base class.
> *
> - * In addition to the features of the standard unsorted map, this class also
> - * provides access to the mapped elements using numerical ID keys. It maintains
> - * an internal map of numerical ID to ControlId for this purpose, and exposes it
> - * through the idmap() function to help construction of ControlList instances.
> + * The class is constructed with a reference to a ControlIdMap. This allows
> + * providing access to the mapped elements using numerical ID keys, in addition
> + * to the features of the standard unsorted map. All ControlId keys in the map
> + * must appear in the ControlIdMap.
> */
>
> /**
> @@ -645,24 +645,27 @@ std::string ControlInfo::toString() const
> /**
> * \brief Construct a ControlInfoMap from an initializer list
> * \param[in] init The initializer list
> + * \param[in] idmap The idmap used by the ControlInfoMap
> */
> -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)
> - : Map(init)
> +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,
> + const ControlIdMap &idmap)
> + : Map(init), idmap_(&idmap)
> {
> - generateIdmap();
> + ASSERT(validate());
> }
>
> /**
> * \brief Construct a ControlInfoMap from a plain map
> * \param[in] info The control info plain map
> + * \param[in] idmap The idmap used by the ControlInfoMap
> *
> * Construct a new ControlInfoMap and populate its contents with those of
> * \a info using move semantics. Upon return the \a info map will be empty.
> */
> -ControlInfoMap::ControlInfoMap(Map &&info)
> - : Map(std::move(info))
> +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)
> + : Map(std::move(info)), idmap_(&idmap)
> {
> - generateIdmap();
> + ASSERT(validate());
> }
>
> /**
> @@ -672,32 +675,41 @@ ControlInfoMap::ControlInfoMap(Map &&info)
> * \return A reference to the ControlInfoMap
> */
>
> -/**
> - * \brief Replace the contents with those from the initializer list
> - * \param[in] init The initializer list
> - * \return A reference to the ControlInfoMap
> - */
> -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)
> +bool ControlInfoMap::validate()
> {
> - Map::operator=(init);
> - generateIdmap();
> - return *this;
> -}
> + for (const auto &ctrl : *this) {
> + const ControlId *id = ctrl.first;
> + auto it = idmap_->find(id->id());
>
> -/**
> - * \brief Move assignment operator from a plain map
> - * \param[in] info The control info plain map
> - *
> - * Populate the map by replacing its contents with those of \a info using move
> - * semantics. Upon return the \a info map will be empty.
> - *
> - * \return A reference to the populated ControlInfoMap
> - */
> -ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> -{
> - Map::operator=(std::move(info));
> - generateIdmap();
> - return *this;
> + /*
> + * Make sure all control ids are part of the idmap and verify
> + * the control info matches the expected type.
> + */
> + if (it == idmap_->end() || it->second != id) {
> + LOG(Controls, Error)
> + << "Control " << utils::hex(id->id())
> + << " not in the idmap";
> + return false;
> + }
> +
> + /*
> + * For string controls, min and max define the valid
> + * range for the string size, not for the individual
> + * values.
> + */
> + ControlType rangeType = id->type() == ControlTypeString
> + ? ControlTypeInteger32 : id->type();
> + const ControlInfo &info = ctrl.second;
> +
> + if (info.min().type() != rangeType) {
> + LOG(Controls, Error)
> + << "Control " << utils::hex(id->id())
> + << " type and info type mismatch";
> + return false;
> + }
> + }
> +
> + return true;
> }
>
> /**
> @@ -707,7 +719,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> */
> ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> {
> - return at(idmap_.at(id));
> + return at(idmap_->at(id));
> }
>
> /**
> @@ -717,7 +729,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> */
> const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
> {
> - return at(idmap_.at(id));
> + return at(idmap_->at(id));
> }
>
> /**
> @@ -732,7 +744,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> * entries, we can thus just count the matching entries in idmap to
> * avoid an additional lookup.
> */
> - return idmap_.count(id);
> + return idmap_->count(id);
> }
>
> /**
> @@ -743,8 +755,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> */
> ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> {
> - auto iter = idmap_.find(id);
> - if (iter == idmap_.end())
> + auto iter = idmap_->find(id);
> + if (iter == idmap_->end())
> return end();
>
> return find(iter->second);
> @@ -758,8 +770,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> */
> ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> {
> - auto iter = idmap_.find(id);
> - if (iter == idmap_.end())
> + auto iter = idmap_->find(id);
> + if (iter == idmap_->end())
> return end();
>
> return find(iter->second);
> @@ -776,33 +788,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> * \return The ControlId map
> */
>
> -void ControlInfoMap::generateIdmap()
> -{
> - idmap_.clear();
> -
> - for (const auto &ctrl : *this) {
> - /*
> - * For string controls, min and max define the valid
> - * range for the string size, not for the individual
> - * values.
> - */
> - ControlType rangeType = ctrl.first->type() == ControlTypeString
> - ? ControlTypeInteger32 : ctrl.first->type();
> - const ControlInfo &info = ctrl.second;
> -
> - if (info.min().type() != rangeType) {
> - LOG(Controls, Error)
> - << "Control " << utils::hex(ctrl.first->id())
> - << " type and info type mismatch";
> - idmap_.clear();
> - clear();
> - return;
> - }
> -
> - idmap_[ctrl.first->id()] = ctrl.first;
> - }
> -}
> -
> /**
> * \class ControlList
> * \brief Associate a list of ControlId with their values for an object
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 19cb5c4ec9c3..9c23788e5231 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1071,7 +1071,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>
> controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
>
> - data->controlInfo_ = std::move(controls);
> + data->controlInfo_ = ControlInfoMap(std::move(controls),
> + controls::controls);
>
> return 0;
> }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 42911a8fdfdb..710b9309448e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> std::forward_as_tuple(&controls::AeEnable),
> std::forward_as_tuple(false, true));
>
> - data->controlInfo_ = std::move(ctrls);
> + data->controlInfo_ = ControlInfoMap(std::move(ctrls),
> + controls::controls);
>
> data->sensor_ = std::make_unique<CameraSensor>(sensor);
> ret = data->sensor_->init();
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 0f634b8da609..573d8042c18c 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)
> addControl(cid, info, &ctrls);
> }
>
> - controlInfo_ = std::move(ctrls);
> + controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>
> return 0;
> }
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 12f7517fd0ae..d4b041d33eb4 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -512,7 +512,7 @@ int VimcCameraData::init()
> ctrls.emplace(id, info);
> }
>
> - controlInfo_ = std::move(ctrls);
> + controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>
> /* Initialize the camera properties. */
> properties_ = sensor_->properties();
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index b0a70defc3d0..951592c698f7 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -611,12 +611,13 @@ void V4L2Device::listControls()
> << " (" << utils::hex(ctrl.id) << ")";
>
> controlIds_.emplace_back(v4l2ControlId(ctrl));
> + controlIdMap_[ctrl.id] = controlIds_.back().get();
> controlInfo_.emplace(ctrl.id, ctrl);
>
> ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> }
>
> - controls_ = std::move(ctrls);
> + controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> }
>
> /**
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index 880bcd02c6be..bf7e34e28af3 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -32,13 +32,13 @@
> using namespace std;
> using namespace libcamera;
>
> -static const ControlInfoMap Controls = {
> - { &controls::AeEnable, ControlInfo(false, true) },
> - { &controls::ExposureTime, ControlInfo(0, 999999) },
> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> -};
> +static const ControlInfoMap Controls = ControlInfoMap({
> + { &controls::AeEnable, ControlInfo(false, true) },
> + { &controls::ExposureTime, ControlInfo(0, 999999) },
> + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> + }, controls::controls);
>
> namespace libcamera {
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list