[libcamera-devel,v2,1/5] libcamera: controls: Create ControlInfoMap with ControlIdMap
diff mbox series

Message ID 20210728161116.64489-2-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Initialize controls in the IPA
Related show

Commit Message

Jacopo Mondi July 28, 2021, 4:11 p.m. UTC 
ControlInfoMap do not have a ControlId map associated, but rather
create 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 proeprties) 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.

Has a consequence the class constructors take and additional argument,
which might be not pleaseant 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 need 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 and ties its lifetime
  management to the its own one
- 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@jmondi.org>
---
 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                    | 82 ++++---------------
 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              |  4 +-
 12 files changed, 59 insertions(+), 106 deletions(-)

Comments

Paul Elder July 29, 2021, 7:52 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:
> ControlInfoMap do not have a ControlId map associated, but rather
> create 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 proeprties) 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.
> 
> Has a consequence the class constructors take and additional argument,
> which might be not pleaseant 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 need 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 and ties its lifetime
>   management to the its own one
> - 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@jmondi.org>

I think it looks good. I see some issues deserializing on the pipeline
handler side when it'll create a new ControlIdMap when it was the one that
sent the same one in the first place, but I think once the
ControlInfoMap caching is fixed then it shouldn't be an issue.

Reviewed-by: Paul Elder <paul.elder@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                    | 82 ++++---------------
>  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              |  4 +-
>  12 files changed, 59 insertions(+), 106 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a43b22..4a2cdb335870 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -306,12 +306,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;
> @@ -336,12 +335,10 @@ 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();
> -
> -	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 300466285a57..df6ed93f477e 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 78109f4130a8..9409f0f9e782 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -616,24 +616,25 @@ 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();
>  }
>  
>  /**
>   * \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();
>  }
>  
>  /**
> @@ -643,34 +644,6 @@ 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)
> -{
> -	Map::operator=(init);
> -	generateIdmap();
> -	return *this;
> -}
> -
> -/**
> - * \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;
> -}
> -
>  /**
>   * \brief Access specified element by numerical ID
>   * \param[in] id The numerical ID
> @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
>   */
>  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
>  {
> -	return at(idmap_.at(id));
> +	return at(idmap_->at(id));
>  }
>  
>  /**
> @@ -688,7 +661,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));
>  }
>  
>  /**
> @@ -703,7 +676,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);
>  }
>  
>  /**
> @@ -714,8 +687,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);
> @@ -729,8 +702,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);
> @@ -747,33 +720,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 76c3bb3d8aa9..048993365b44 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1057,7 +1057,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 98d93a12a7be..6ccd8eb86016 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..3d845a129a5f 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 = {
> +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 {
>  
> -- 
> 2.32.0
>
Laurent Pinchart Aug. 3, 2021, 3:43 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:
> ControlInfoMap do not have a ControlId map associated, but rather

s/do not/does not/

> create one with the generateIdMap() function at creation time.

s/create/creates/

> 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 proeprties) 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.
> 
> Has a consequence the class constructors take and additional argument,

s/Has/As/

> which might be not pleaseant to type in, but enforces the concepts that
> ControlInfoMap should be created with controls part of the same id map.

It's not that unpleasant, it seems worth it to me.

> As the ControlIdMap the ControlInfoMap refers to need to be allocated

s/need/needs/

> externally:
> - Use the globally available controls::controls (or
>   properties::properties) id map when referring to libcamera controls

It's interesting to see that we actually don't use
properties::properties yet.

> - The V4L2 device that creates ControlInfoMap by parsing the device's
>   controls has to allocate a ControlIdMap and ties its lifetime
>   management to the its own one

"to the its own one" hurts a bit :-) You could possibly just drop "and
ties ...".

> - 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@jmondi.org>
> ---
>  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                    | 82 ++++---------------
>  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              |  4 +-
>  12 files changed, 59 insertions(+), 106 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a43b22..4a2cdb335870 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -306,12 +306,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;
> @@ -336,12 +335,10 @@ 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();
> -
> -	ControlIdMap idmap_;
> +	const ControlIdMap *idmap_;

You can also store a reference instead of a pointer.

>  };
>  
>  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 300466285a57..df6ed93f477e 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();

With

	ControlIdMap &localIdMap = *controlIdMaps_.back().get();

the code below may look better. Or maybe you could use
localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].

>  
>  	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 78109f4130a8..9409f0f9e782 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const

The second paragraph of the ControlInfoMap class documentation needs
to be updated, to explain how the class now needs to be constructed with
an idmap (and how all elements must refer to the same idmap).

>  /**
>   * \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();
>  }
>  
>  /**
>   * \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();
>  }
>  
>  /**
> @@ -643,34 +644,6 @@ 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)
> -{
> -	Map::operator=(init);
> -	generateIdmap();
> -	return *this;
> -}
> -
> -/**
> - * \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;
> -}
> -
>  /**
>   * \brief Access specified element by numerical ID
>   * \param[in] id The numerical ID
> @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
>   */
>  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
>  {
> -	return at(idmap_.at(id));
> +	return at(idmap_->at(id));
>  }
>  
>  /**
> @@ -688,7 +661,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));
>  }
>  
>  /**
> @@ -703,7 +676,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);
>  }
>  
>  /**
> @@ -714,8 +687,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);
> @@ -729,8 +702,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);
> @@ -747,33 +720,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;
> -		}

I think those sanity checks should be kept, You can rename the function
to validate() (or something similar) and call it from the constructors.
A useful additional check would be to verify that each entry in the
control info map is in the idmap.

> -
> -		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 76c3bb3d8aa9..048993365b44 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1057,7 +1057,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 98d93a12a7be..6ccd8eb86016 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..3d845a129a5f 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 = {
> +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) },

Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?

Overall this looks good to me.

> -};
> +	}, controls::controls);
>  
>  namespace libcamera {
>
Jacopo Mondi Aug. 9, 2021, 10:48 a.m. UTC | #3 
Hi Laurent,

On Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:
> > ControlInfoMap do not have a ControlId map associated, but rather
>
> s/do not/does not/
>
> > create one with the generateIdMap() function at creation time.
>
> s/create/creates/
>
> > 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 proeprties) 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.
> >
> > Has a consequence the class constructors take and additional argument,
>
> s/Has/As/
>
> > which might be not pleaseant to type in, but enforces the concepts that
> > ControlInfoMap should be created with controls part of the same id map.
>
> It's not that unpleasant, it seems worth it to me.
>
> > As the ControlIdMap the ControlInfoMap refers to need to be allocated
>
> s/need/needs/
>
> > externally:
> > - Use the globally available controls::controls (or
> >   properties::properties) id map when referring to libcamera controls
>
> It's interesting to see that we actually don't use
> properties::properties yet.
>
> > - The V4L2 device that creates ControlInfoMap by parsing the device's
> >   controls has to allocate a ControlIdMap and ties its lifetime
> >   management to the its own one
>
> "to the its own one" hurts a bit :-) You could possibly just drop "and
> ties ...".
>
> > - 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@jmondi.org>
> > ---
> >  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                    | 82 ++++---------------
> >  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              |  4 +-
> >  12 files changed, 59 insertions(+), 106 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 1bc958a43b22..4a2cdb335870 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -306,12 +306,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;
> > @@ -336,12 +335,10 @@ 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();
> > -
> > -	ControlIdMap idmap_;
> > +	const ControlIdMap *idmap_;
>
> You can also store a reference instead of a pointer.

Not really.. I mean, I could but then I will have to remove copy
assignements and default constructor, breaking the construction of
CameraData which has a default constructed ControlInfoMap member.

>
> >  };
> >
> >  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 300466285a57..df6ed93f477e 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();
>
> With
>
> 	ControlIdMap &localIdMap = *controlIdMaps_.back().get();
>
> the code below may look better. Or maybe you could use
> localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].
>
> >
> >  	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 78109f4130a8..9409f0f9e782 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const
>
> The second paragraph of the ControlInfoMap class documentation needs
> to be updated, to explain how the class now needs to be constructed with
> an idmap (and how all elements must refer to the same idmap).
>
> >  /**
> >   * \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();
> >  }
> >
> >  /**
> >   * \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();
> >  }
> >
> >  /**
> > @@ -643,34 +644,6 @@ 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)
> > -{
> > -	Map::operator=(init);
> > -	generateIdmap();
> > -	return *this;
> > -}
> > -
> > -/**
> > - * \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;
> > -}
> > -
> >  /**
> >   * \brief Access specified element by numerical ID
> >   * \param[in] id The numerical ID
> > @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> >   */
> >  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> >  {
> > -	return at(idmap_.at(id));
> > +	return at(idmap_->at(id));
> >  }
> >
> >  /**
> > @@ -688,7 +661,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));
> >  }
> >
> >  /**
> > @@ -703,7 +676,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);
> >  }
> >
> >  /**
> > @@ -714,8 +687,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);
> > @@ -729,8 +702,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);
> > @@ -747,33 +720,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;
> > -		}
>
> I think those sanity checks should be kept, You can rename the function
> to validate() (or something similar) and call it from the constructors.
> A useful additional check would be to verify that each entry in the
> control info map is in the idmap.

I will ASSERT() that.

Thanks
  j

>
> > -
> > -		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 76c3bb3d8aa9..048993365b44 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1057,7 +1057,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 98d93a12a7be..6ccd8eb86016 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..3d845a129a5f 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 = {
> > +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) },
>
> Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?
>
> Overall this looks good to me.
>
> > -};
> > +	}, controls::controls);
> >
> >  namespace libcamera {
> >
>
> --
> Regards,
>
> Laurent Pinchart
Jacopo Mondi Aug. 9, 2021, 12:43 p.m. UTC | #4 
Hi Laurent,
  sorry one more comment I missed in previous reply

On Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>

[snip]

> > @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> >  	}
> >
> >  	ControlInfoMap::Map ctrls;
> > +	controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> > +	ControlIdMap *localIdMap = controlIdMaps_.back().get();
>
> With
>
> 	ControlIdMap &localIdMap = *controlIdMaps_.back().get();
>
> the code below may look better. Or maybe you could use
> localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].
>

Not really in this case, as I use operator[] to insert a new element,
while std::unsorted_map::at() only allows to retrieve the existing
ones.

I could use a reference, but I need a pointer in the next patch, so I
guess we'll have to stick to the (*)[] syntax

> >
> >  	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 78109f4130a8..9409f0f9e782 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const
>
> The second paragraph of the ControlInfoMap class documentation needs
> to be updated, to explain how the class now needs to be constructed with
> an idmap (and how all elements must refer to the same idmap).
>
> >  /**
> >   * \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();
> >  }
> >
> >  /**
> >   * \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();
> >  }
> >
> >  /**
> > @@ -643,34 +644,6 @@ 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)
> > -{
> > -	Map::operator=(init);
> > -	generateIdmap();
> > -	return *this;
> > -}
> > -
> > -/**
> > - * \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;
> > -}
> > -
> >  /**
> >   * \brief Access specified element by numerical ID
> >   * \param[in] id The numerical ID
> > @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> >   */
> >  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> >  {
> > -	return at(idmap_.at(id));
> > +	return at(idmap_->at(id));
> >  }
> >
> >  /**
> > @@ -688,7 +661,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));
> >  }
> >
> >  /**
> > @@ -703,7 +676,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);
> >  }
> >
> >  /**
> > @@ -714,8 +687,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);
> > @@ -729,8 +702,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);
> > @@ -747,33 +720,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;
> > -		}
>
> I think those sanity checks should be kept, You can rename the function
> to validate() (or something similar) and call it from the constructors.
> A useful additional check would be to verify that each entry in the
> control info map is in the idmap.
>
> > -
> > -		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 76c3bb3d8aa9..048993365b44 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1057,7 +1057,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 98d93a12a7be..6ccd8eb86016 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..3d845a129a5f 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 = {
> > +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) },
>
> Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?
>
> Overall this looks good to me.
>
> > -};
> > +	}, controls::controls);
> >
> >  namespace libcamera {
> >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 1bc958a43b22..4a2cdb335870 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -306,12 +306,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;
@@ -336,12 +335,10 @@  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();
-
-	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 300466285a57..df6ed93f477e 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 78109f4130a8..9409f0f9e782 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -616,24 +616,25 @@  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();
 }
 
 /**
  * \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();
 }
 
 /**
@@ -643,34 +644,6 @@  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)
-{
-	Map::operator=(init);
-	generateIdmap();
-	return *this;
-}
-
-/**
- * \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;
-}
-
 /**
  * \brief Access specified element by numerical ID
  * \param[in] id The numerical ID
@@ -678,7 +651,7 @@  ControlInfoMap &ControlInfoMap::operator=(Map &&info)
  */
 ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
 {
-	return at(idmap_.at(id));
+	return at(idmap_->at(id));
 }
 
 /**
@@ -688,7 +661,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));
 }
 
 /**
@@ -703,7 +676,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);
 }
 
 /**
@@ -714,8 +687,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);
@@ -729,8 +702,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);
@@ -747,33 +720,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 76c3bb3d8aa9..048993365b44 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1057,7 +1057,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 98d93a12a7be..6ccd8eb86016 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..3d845a129a5f 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 = {
+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 {