[{"id":18414,"web_url":"https://patchwork.libcamera.org/comment/18414/","msgid":"<20210729075210.GH2167@pyrite.rasen.tech>","date":"2021-07-29T07:52:10","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:\n> ControlInfoMap do not have a ControlId map associated, but rather\n> create one with the generateIdMap() function at creation time.\n> \n> As a consequence, when in the need to de-serialize a ControlInfoMap all\n> the ControlId it contains are created by the deserializer instance, not\n> being able to discern if the controls the ControlIdMap refers to are the\n> global libcamera controls (and proeprties) or instances local to the\n> V4L2 device that has first initialized the controls.\n> \n> As a consequence the ControlId stored in a de-serialized map will always\n> be newly created entities, preventing lookup by ControlId reference on a\n> de-serialized ControlInfoMap.\n> \n> In order to make it possible to use globally available ControlId\n> instances whenever possible, create ControlInfoMap with a reference to\n> an externally allocated ControlIdMap instead of generating one\n> internally.\n> \n> Has a consequence the class constructors take and additional argument,\n> which might be not pleaseant to type in, but enforces the concepts that\n> ControlInfoMap should be created with controls part of the same id map.\n> \n> As the ControlIdMap the ControlInfoMap refers to need to be allocated\n> externally:\n> - Use the globally available controls::controls (or\n>   properties::properties) id map when referring to libcamera controls\n> - The V4L2 device that creates ControlInfoMap by parsing the device's\n>   controls has to allocate a ControlIdMap and ties its lifetime\n>   management to the its own one\n> - The ControlSerializer that de-serializes a ControlInfoMap has to\n>   create and store the ControlIdMap the de-serialized info map refers to\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nI think it looks good. I see some issues deserializing on the pipeline\nhandler side when it'll create a new ControlIdMap when it was the one that\nsent the same one in the first place, but I think once the\nControlInfoMap caching is fixed then it shouldn't be an issue.\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  include/libcamera/controls.h                  | 13 ++-\n>  .../libcamera/internal/control_serializer.h   |  1 +\n>  include/libcamera/internal/v4l2_device.h      |  1 +\n>  include/libcamera/ipa/raspberrypi.h           | 40 ++++-----\n>  src/libcamera/control_serializer.cpp          | 11 ++-\n>  src/libcamera/controls.cpp                    | 82 ++++---------------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  3 +-\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  2 +-\n>  src/libcamera/pipeline/vimc/vimc.cpp          |  2 +-\n>  src/libcamera/v4l2_device.cpp                 |  3 +-\n>  .../ipa_data_serializer_test.cpp              |  4 +-\n>  12 files changed, 59 insertions(+), 106 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a43b22..4a2cdb335870 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -306,12 +306,11 @@ public:\n>  \n>  \tControlInfoMap() = default;\n>  \tControlInfoMap(const ControlInfoMap &other) = default;\n> -\tControlInfoMap(std::initializer_list<Map::value_type> init);\n> -\tControlInfoMap(Map &&info);\n> +\tControlInfoMap(std::initializer_list<Map::value_type> init,\n> +\t\t       const ControlIdMap &idmap);\n> +\tControlInfoMap(Map &&info, const ControlIdMap &idmap);\n>  \n>  \tControlInfoMap &operator=(const ControlInfoMap &other) = default;\n> -\tControlInfoMap &operator=(std::initializer_list<Map::value_type> init);\n> -\tControlInfoMap &operator=(Map &&info);\n>  \n>  \tusing Map::key_type;\n>  \tusing Map::mapped_type;\n> @@ -336,12 +335,10 @@ public:\n>  \titerator find(unsigned int key);\n>  \tconst_iterator find(unsigned int key) const;\n>  \n> -\tconst ControlIdMap &idmap() const { return idmap_; }\n> +\tconst ControlIdMap &idmap() const { return *idmap_; }\n>  \n>  private:\n> -\tvoid generateIdmap();\n> -\n> -\tControlIdMap idmap_;\n> +\tconst ControlIdMap *idmap_;\n>  };\n>  \n>  class ControlList\n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 7d4426c95d12..8a66be324138 100644\n> --- a/include/libcamera/internal/control_serializer.h\n> +++ b/include/libcamera/internal/control_serializer.h\n> @@ -48,6 +48,7 @@ private:\n>  \n>  \tunsigned int serial_;\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tstd::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;\n>  \tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n>  \tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n>  };\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 77b835b3cb80..423c8fb11845 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -69,6 +69,7 @@ private:\n>  \n>  \tstd::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tControlIdMap controlIdMap_;\n>  \tControlInfoMap controls_;\n>  \tstd::string deviceNode_;\n>  \tint fd_;\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index a8790000e2d9..521eaecd19b2 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -27,26 +27,26 @@ namespace RPi {\n>   * and the pipeline handler may be reverted so that it aborts when an\n>   * unsupported control is encountered.\n>   */\n> -static const ControlInfoMap Controls = {\n> -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> -\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> -\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> -};\n> +static const ControlInfoMap Controls({\n> +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> +\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> +\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> +\t}, controls::controls);\n>  \n>  } /* namespace RPi */\n>  \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 300466285a57..df6ed93f477e 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -93,6 +93,7 @@ void ControlSerializer::reset()\n>  \tinfoMapHandles_.clear();\n>  \tinfoMaps_.clear();\n>  \tcontrolIds_.clear();\n> +\tcontrolIdMaps_.clear();\n>  }\n>  \n>  size_t ControlSerializer::binarySize(const ControlValue &value)\n> @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t}\n>  \n>  \tControlInfoMap::Map ctrls;\n> +\tcontrolIdMaps_.emplace_back(std::make_unique<ControlIdMap>());\n> +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n>  \n>  \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n>  \t\tconst struct ipa_control_info_entry *entry =\n> @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t\t * purpose.\n>  \t\t */\n>  \t\tcontrolIds_.emplace_back(std::make_unique<ControlId>(entry->id, \"\", type));\n> +\t\tControlId *controlId = controlIds_.back().get();\n> +\t\t(*localIdMap)[entry->id] = controlId;\n>  \n>  \t\tif (entry->offset != values.offset()) {\n>  \t\t\tLOG(Serializer, Error)\n> @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t\t}\n>  \n>  \t\t/* Create and store the ControlInfo. */\n> -\t\tctrls.emplace(controlIds_.back().get(),\n> -\t\t\t      loadControlInfo(type, values));\n> +\t\tctrls.emplace(controlId, loadControlInfo(type, values));\n>  \t}\n>  \n>  \t/*\n>  \t * Create the ControlInfoMap in the cache, and store the map to handle\n>  \t * association.\n>  \t */\n> -\tControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);\n> +\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);\n> +\tControlInfoMap &map = infoMaps_[hdr->handle];\n>  \tinfoMapHandles_[&map] = hdr->handle;\n>  \n>  \treturn map;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 78109f4130a8..9409f0f9e782 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const\n>  /**\n>   * \\brief Construct a ControlInfoMap from an initializer list\n>   * \\param[in] init The initializer list\n> + * \\param[in] idmap The idmap used by the ControlInfoMap\n>   */\n> -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)\n> -\t: Map(init)\n> +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,\n> +\t\t\t       const ControlIdMap &idmap)\n> +\t: Map(init), idmap_(&idmap)\n>  {\n> -\tgenerateIdmap();\n>  }\n>  \n>  /**\n>   * \\brief Construct a ControlInfoMap from a plain map\n>   * \\param[in] info The control info plain map\n> + * \\param[in] idmap The idmap used by the ControlInfoMap\n>   *\n>   * Construct a new ControlInfoMap and populate its contents with those of\n>   * \\a info using move semantics. Upon return the \\a info map will be empty.\n>   */\n> -ControlInfoMap::ControlInfoMap(Map &&info)\n> -\t: Map(std::move(info))\n> +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)\n> +\t: Map(std::move(info)), idmap_(&idmap)\n>  {\n> -\tgenerateIdmap();\n>  }\n>  \n>  /**\n> @@ -643,34 +644,6 @@ ControlInfoMap::ControlInfoMap(Map &&info)\n>   * \\return A reference to the ControlInfoMap\n>   */\n>  \n> -/**\n> - * \\brief Replace the contents with those from the initializer list\n> - * \\param[in] init The initializer list\n> - * \\return A reference to the ControlInfoMap\n> - */\n> -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)\n> -{\n> -\tMap::operator=(init);\n> -\tgenerateIdmap();\n> -\treturn *this;\n> -}\n> -\n> -/**\n> - * \\brief Move assignment operator from a plain map\n> - * \\param[in] info The control info plain map\n> - *\n> - * Populate the map by replacing its contents with those of \\a info using move\n> - * semantics. Upon return the \\a info map will be empty.\n> - *\n> - * \\return A reference to the populated ControlInfoMap\n> - */\n> -ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> -{\n> -\tMap::operator=(std::move(info));\n> -\tgenerateIdmap();\n> -\treturn *this;\n> -}\n> -\n>  /**\n>   * \\brief Access specified element by numerical ID\n>   * \\param[in] id The numerical ID\n> @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n>   */\n>  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n>  {\n> -\treturn at(idmap_.at(id));\n> +\treturn at(idmap_->at(id));\n>  }\n>  \n>  /**\n> @@ -688,7 +661,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n>   */\n>  const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const\n>  {\n> -\treturn at(idmap_.at(id));\n> +\treturn at(idmap_->at(id));\n>  }\n>  \n>  /**\n> @@ -703,7 +676,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n>  \t * entries, we can thus just count the matching entries in idmap to\n>  \t * avoid an additional lookup.\n>  \t */\n> -\treturn idmap_.count(id);\n> +\treturn idmap_->count(id);\n>  }\n>  \n>  /**\n> @@ -714,8 +687,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n>   */\n>  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n>  {\n> -\tauto iter = idmap_.find(id);\n> -\tif (iter == idmap_.end())\n> +\tauto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n>  \t\treturn end();\n>  \n>  \treturn find(iter->second);\n> @@ -729,8 +702,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n>   */\n>  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n>  {\n> -\tauto iter = idmap_.find(id);\n> -\tif (iter == idmap_.end())\n> +\tauto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n>  \t\treturn end();\n>  \n>  \treturn find(iter->second);\n> @@ -747,33 +720,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n>   * \\return The ControlId map\n>   */\n>  \n> -void ControlInfoMap::generateIdmap()\n> -{\n> -\tidmap_.clear();\n> -\n> -\tfor (const auto &ctrl : *this) {\n> -\t\t/*\n> -\t\t * For string controls, min and max define the valid\n> -\t\t * range for the string size, not for the individual\n> -\t\t * values.\n> -\t\t */\n> -\t\tControlType rangeType = ctrl.first->type() == ControlTypeString\n> -\t\t\t\t      ? ControlTypeInteger32 : ctrl.first->type();\n> -\t\tconst ControlInfo &info = ctrl.second;\n> -\n> -\t\tif (info.min().type() != rangeType) {\n> -\t\t\tLOG(Controls, Error)\n> -\t\t\t\t<< \"Control \" << utils::hex(ctrl.first->id())\n> -\t\t\t\t<< \" type and info type mismatch\";\n> -\t\t\tidmap_.clear();\n> -\t\t\tclear();\n> -\t\t\treturn;\n> -\t\t}\n> -\n> -\t\tidmap_[ctrl.first->id()] = ctrl.first;\n> -\t}\n> -}\n> -\n>  /**\n>   * \\class ControlList\n>   * \\brief Associate a list of ControlId with their values for an object\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 76c3bb3d8aa9..048993365b44 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1057,7 +1057,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \n>  \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n>  \n> -\tdata->controlInfo_ = std::move(controls);\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> +\t\t\t\t\t    controls::controls);\n>  \n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42911a8fdfdb..710b9309448e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \t\t      std::forward_as_tuple(&controls::AeEnable),\n>  \t\t      std::forward_as_tuple(false, true));\n>  \n> -\tdata->controlInfo_ = std::move(ctrls);\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> +\t\t\t\t\t    controls::controls);\n>  \n>  \tdata->sensor_ = std::make_unique<CameraSensor>(sensor);\n>  \tret = data->sensor_->init();\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..573d8042c18c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)\n>  \t\taddControl(cid, info, &ctrls);\n>  \t}\n>  \n> -\tcontrolInfo_ = std::move(ctrls);\n> +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>  \n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 12f7517fd0ae..d4b041d33eb4 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -512,7 +512,7 @@ int VimcCameraData::init()\n>  \t\tctrls.emplace(id, info);\n>  \t}\n>  \n> -\tcontrolInfo_ = std::move(ctrls);\n> +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>  \n>  \t/* Initialize the camera properties. */\n>  \tproperties_ = sensor_->properties();\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 98d93a12a7be..6ccd8eb86016 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -611,12 +611,13 @@ void V4L2Device::listControls()\n>  \t\t\t\t << \" (\" << utils::hex(ctrl.id) << \")\";\n>  \n>  \t\tcontrolIds_.emplace_back(v4l2ControlId(ctrl));\n> +\t\tcontrolIdMap_[ctrl.id] = controlIds_.back().get();\n>  \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n>  \n>  \t\tctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n>  \t}\n>  \n> -\tcontrols_ = std::move(ctrls);\n> +\tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n>  }\n>  \n>  /**\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index 880bcd02c6be..3d845a129a5f 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -32,13 +32,13 @@\n>  using namespace std;\n>  using namespace libcamera;\n>  \n> -static const ControlInfoMap Controls = {\n> +static const ControlInfoMap Controls = ControlInfoMap({\n>  \t{ &controls::AeEnable, ControlInfo(false, true) },\n>  \t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>  \t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>  \t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>  \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -};\n> +\t}, controls::controls);\n>  \n>  namespace libcamera {\n>  \n> -- \n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 974BDC3230\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 29 Jul 2021 07:52:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 523F4687C5;\n\tThu, 29 Jul 2021 09:52:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B63F68536\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 29 Jul 2021 09:52:18 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F1944DD;\n\tThu, 29 Jul 2021 09:52:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u2dpLbXG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627545138;\n\tbh=mGBjNgihVfB1GWsaFLwcGKVqfY3RR22cJr1XGhF9aBU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u2dpLbXGc/aInkHiF/lgFx3S/jOy6iVCLBRCXNnMNlOkZbW+gFx5KF6sHNVW0/tDb\n\tCOuRXHUo6nZp7ChX357mcMHK1fVj6cVP+FS0ccuNHUCRYD1HL2bFv1HAHU/XVfkybH\n\tncNWd8x4KJ6iiEMXjr4DSZEoLuzgX4Riguvons7Y=","Date":"Thu, 29 Jul 2021 16:52:10 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20210729075210.GH2167@pyrite.rasen.tech>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210728161116.64489-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18520,"web_url":"https://patchwork.libcamera.org/comment/18520/","msgid":"<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","date":"2021-08-03T15:43:37","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:\n> ControlInfoMap do not have a ControlId map associated, but rather\n\ns/do not/does not/\n\n> create one with the generateIdMap() function at creation time.\n\ns/create/creates/\n\n> As a consequence, when in the need to de-serialize a ControlInfoMap all\n> the ControlId it contains are created by the deserializer instance, not\n> being able to discern if the controls the ControlIdMap refers to are the\n> global libcamera controls (and proeprties) or instances local to the\n> V4L2 device that has first initialized the controls.\n> \n> As a consequence the ControlId stored in a de-serialized map will always\n> be newly created entities, preventing lookup by ControlId reference on a\n> de-serialized ControlInfoMap.\n> \n> In order to make it possible to use globally available ControlId\n> instances whenever possible, create ControlInfoMap with a reference to\n> an externally allocated ControlIdMap instead of generating one\n> internally.\n> \n> Has a consequence the class constructors take and additional argument,\n\ns/Has/As/\n\n> which might be not pleaseant to type in, but enforces the concepts that\n> ControlInfoMap should be created with controls part of the same id map.\n\nIt's not that unpleasant, it seems worth it to me.\n\n> As the ControlIdMap the ControlInfoMap refers to need to be allocated\n\ns/need/needs/\n\n> externally:\n> - Use the globally available controls::controls (or\n>   properties::properties) id map when referring to libcamera controls\n\nIt's interesting to see that we actually don't use\nproperties::properties yet.\n\n> - The V4L2 device that creates ControlInfoMap by parsing the device's\n>   controls has to allocate a ControlIdMap and ties its lifetime\n>   management to the its own one\n\n\"to the its own one\" hurts a bit :-) You could possibly just drop \"and\nties ...\".\n\n> - The ControlSerializer that de-serializes a ControlInfoMap has to\n>   create and store the ControlIdMap the de-serialized info map refers to\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  include/libcamera/controls.h                  | 13 ++-\n>  .../libcamera/internal/control_serializer.h   |  1 +\n>  include/libcamera/internal/v4l2_device.h      |  1 +\n>  include/libcamera/ipa/raspberrypi.h           | 40 ++++-----\n>  src/libcamera/control_serializer.cpp          | 11 ++-\n>  src/libcamera/controls.cpp                    | 82 ++++---------------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  3 +-\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  2 +-\n>  src/libcamera/pipeline/vimc/vimc.cpp          |  2 +-\n>  src/libcamera/v4l2_device.cpp                 |  3 +-\n>  .../ipa_data_serializer_test.cpp              |  4 +-\n>  12 files changed, 59 insertions(+), 106 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a43b22..4a2cdb335870 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -306,12 +306,11 @@ public:\n>  \n>  \tControlInfoMap() = default;\n>  \tControlInfoMap(const ControlInfoMap &other) = default;\n> -\tControlInfoMap(std::initializer_list<Map::value_type> init);\n> -\tControlInfoMap(Map &&info);\n> +\tControlInfoMap(std::initializer_list<Map::value_type> init,\n> +\t\t       const ControlIdMap &idmap);\n> +\tControlInfoMap(Map &&info, const ControlIdMap &idmap);\n>  \n>  \tControlInfoMap &operator=(const ControlInfoMap &other) = default;\n> -\tControlInfoMap &operator=(std::initializer_list<Map::value_type> init);\n> -\tControlInfoMap &operator=(Map &&info);\n>  \n>  \tusing Map::key_type;\n>  \tusing Map::mapped_type;\n> @@ -336,12 +335,10 @@ public:\n>  \titerator find(unsigned int key);\n>  \tconst_iterator find(unsigned int key) const;\n>  \n> -\tconst ControlIdMap &idmap() const { return idmap_; }\n> +\tconst ControlIdMap &idmap() const { return *idmap_; }\n>  \n>  private:\n> -\tvoid generateIdmap();\n> -\n> -\tControlIdMap idmap_;\n> +\tconst ControlIdMap *idmap_;\n\nYou can also store a reference instead of a pointer.\n\n>  };\n>  \n>  class ControlList\n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 7d4426c95d12..8a66be324138 100644\n> --- a/include/libcamera/internal/control_serializer.h\n> +++ b/include/libcamera/internal/control_serializer.h\n> @@ -48,6 +48,7 @@ private:\n>  \n>  \tunsigned int serial_;\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tstd::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;\n>  \tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n>  \tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n>  };\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 77b835b3cb80..423c8fb11845 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -69,6 +69,7 @@ private:\n>  \n>  \tstd::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tControlIdMap controlIdMap_;\n>  \tControlInfoMap controls_;\n>  \tstd::string deviceNode_;\n>  \tint fd_;\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index a8790000e2d9..521eaecd19b2 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -27,26 +27,26 @@ namespace RPi {\n>   * and the pipeline handler may be reverted so that it aborts when an\n>   * unsupported control is encountered.\n>   */\n> -static const ControlInfoMap Controls = {\n> -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> -\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> -\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> -};\n> +static const ControlInfoMap Controls({\n> +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> +\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> +\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> +\t}, controls::controls);\n>  \n>  } /* namespace RPi */\n>  \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 300466285a57..df6ed93f477e 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -93,6 +93,7 @@ void ControlSerializer::reset()\n>  \tinfoMapHandles_.clear();\n>  \tinfoMaps_.clear();\n>  \tcontrolIds_.clear();\n> +\tcontrolIdMaps_.clear();\n>  }\n>  \n>  size_t ControlSerializer::binarySize(const ControlValue &value)\n> @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t}\n>  \n>  \tControlInfoMap::Map ctrls;\n> +\tcontrolIdMaps_.emplace_back(std::make_unique<ControlIdMap>());\n> +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n\nWith\n\n\tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n\nthe code below may look better. Or maybe you could use\nlocalIdMap->at(entry->id) instead of (*localIdMap)[entry->id].\n\n>  \n>  \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n>  \t\tconst struct ipa_control_info_entry *entry =\n> @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t\t * purpose.\n>  \t\t */\n>  \t\tcontrolIds_.emplace_back(std::make_unique<ControlId>(entry->id, \"\", type));\n> +\t\tControlId *controlId = controlIds_.back().get();\n> +\t\t(*localIdMap)[entry->id] = controlId;\n>  \n>  \t\tif (entry->offset != values.offset()) {\n>  \t\t\tLOG(Serializer, Error)\n> @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n>  \t\t}\n>  \n>  \t\t/* Create and store the ControlInfo. */\n> -\t\tctrls.emplace(controlIds_.back().get(),\n> -\t\t\t      loadControlInfo(type, values));\n> +\t\tctrls.emplace(controlId, loadControlInfo(type, values));\n>  \t}\n>  \n>  \t/*\n>  \t * Create the ControlInfoMap in the cache, and store the map to handle\n>  \t * association.\n>  \t */\n> -\tControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);\n> +\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);\n> +\tControlInfoMap &map = infoMaps_[hdr->handle];\n>  \tinfoMapHandles_[&map] = hdr->handle;\n>  \n>  \treturn map;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 78109f4130a8..9409f0f9e782 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const\n\nThe second paragraph of the ControlInfoMap class documentation needs\nto be updated, to explain how the class now needs to be constructed with\nan idmap (and how all elements must refer to the same idmap).\n\n>  /**\n>   * \\brief Construct a ControlInfoMap from an initializer list\n>   * \\param[in] init The initializer list\n> + * \\param[in] idmap The idmap used by the ControlInfoMap\n>   */\n> -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)\n> -\t: Map(init)\n> +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,\n> +\t\t\t       const ControlIdMap &idmap)\n> +\t: Map(init), idmap_(&idmap)\n>  {\n> -\tgenerateIdmap();\n>  }\n>  \n>  /**\n>   * \\brief Construct a ControlInfoMap from a plain map\n>   * \\param[in] info The control info plain map\n> + * \\param[in] idmap The idmap used by the ControlInfoMap\n>   *\n>   * Construct a new ControlInfoMap and populate its contents with those of\n>   * \\a info using move semantics. Upon return the \\a info map will be empty.\n>   */\n> -ControlInfoMap::ControlInfoMap(Map &&info)\n> -\t: Map(std::move(info))\n> +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)\n> +\t: Map(std::move(info)), idmap_(&idmap)\n>  {\n> -\tgenerateIdmap();\n>  }\n>  \n>  /**\n> @@ -643,34 +644,6 @@ ControlInfoMap::ControlInfoMap(Map &&info)\n>   * \\return A reference to the ControlInfoMap\n>   */\n>  \n> -/**\n> - * \\brief Replace the contents with those from the initializer list\n> - * \\param[in] init The initializer list\n> - * \\return A reference to the ControlInfoMap\n> - */\n> -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)\n> -{\n> -\tMap::operator=(init);\n> -\tgenerateIdmap();\n> -\treturn *this;\n> -}\n> -\n> -/**\n> - * \\brief Move assignment operator from a plain map\n> - * \\param[in] info The control info plain map\n> - *\n> - * Populate the map by replacing its contents with those of \\a info using move\n> - * semantics. Upon return the \\a info map will be empty.\n> - *\n> - * \\return A reference to the populated ControlInfoMap\n> - */\n> -ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> -{\n> -\tMap::operator=(std::move(info));\n> -\tgenerateIdmap();\n> -\treturn *this;\n> -}\n> -\n>  /**\n>   * \\brief Access specified element by numerical ID\n>   * \\param[in] id The numerical ID\n> @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n>   */\n>  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n>  {\n> -\treturn at(idmap_.at(id));\n> +\treturn at(idmap_->at(id));\n>  }\n>  \n>  /**\n> @@ -688,7 +661,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n>   */\n>  const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const\n>  {\n> -\treturn at(idmap_.at(id));\n> +\treturn at(idmap_->at(id));\n>  }\n>  \n>  /**\n> @@ -703,7 +676,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n>  \t * entries, we can thus just count the matching entries in idmap to\n>  \t * avoid an additional lookup.\n>  \t */\n> -\treturn idmap_.count(id);\n> +\treturn idmap_->count(id);\n>  }\n>  \n>  /**\n> @@ -714,8 +687,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n>   */\n>  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n>  {\n> -\tauto iter = idmap_.find(id);\n> -\tif (iter == idmap_.end())\n> +\tauto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n>  \t\treturn end();\n>  \n>  \treturn find(iter->second);\n> @@ -729,8 +702,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n>   */\n>  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n>  {\n> -\tauto iter = idmap_.find(id);\n> -\tif (iter == idmap_.end())\n> +\tauto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n>  \t\treturn end();\n>  \n>  \treturn find(iter->second);\n> @@ -747,33 +720,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n>   * \\return The ControlId map\n>   */\n>  \n> -void ControlInfoMap::generateIdmap()\n> -{\n> -\tidmap_.clear();\n> -\n> -\tfor (const auto &ctrl : *this) {\n> -\t\t/*\n> -\t\t * For string controls, min and max define the valid\n> -\t\t * range for the string size, not for the individual\n> -\t\t * values.\n> -\t\t */\n> -\t\tControlType rangeType = ctrl.first->type() == ControlTypeString\n> -\t\t\t\t      ? ControlTypeInteger32 : ctrl.first->type();\n> -\t\tconst ControlInfo &info = ctrl.second;\n> -\n> -\t\tif (info.min().type() != rangeType) {\n> -\t\t\tLOG(Controls, Error)\n> -\t\t\t\t<< \"Control \" << utils::hex(ctrl.first->id())\n> -\t\t\t\t<< \" type and info type mismatch\";\n> -\t\t\tidmap_.clear();\n> -\t\t\tclear();\n> -\t\t\treturn;\n> -\t\t}\n\nI think those sanity checks should be kept, You can rename the function\nto validate() (or something similar) and call it from the constructors.\nA useful additional check would be to verify that each entry in the\ncontrol info map is in the idmap.\n\n> -\n> -\t\tidmap_[ctrl.first->id()] = ctrl.first;\n> -\t}\n> -}\n> -\n>  /**\n>   * \\class ControlList\n>   * \\brief Associate a list of ControlId with their values for an object\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 76c3bb3d8aa9..048993365b44 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1057,7 +1057,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \n>  \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n>  \n> -\tdata->controlInfo_ = std::move(controls);\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> +\t\t\t\t\t    controls::controls);\n>  \n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42911a8fdfdb..710b9309448e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \t\t      std::forward_as_tuple(&controls::AeEnable),\n>  \t\t      std::forward_as_tuple(false, true));\n>  \n> -\tdata->controlInfo_ = std::move(ctrls);\n> +\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> +\t\t\t\t\t    controls::controls);\n>  \n>  \tdata->sensor_ = std::make_unique<CameraSensor>(sensor);\n>  \tret = data->sensor_->init();\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..573d8042c18c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)\n>  \t\taddControl(cid, info, &ctrls);\n>  \t}\n>  \n> -\tcontrolInfo_ = std::move(ctrls);\n> +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>  \n>  \treturn 0;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 12f7517fd0ae..d4b041d33eb4 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -512,7 +512,7 @@ int VimcCameraData::init()\n>  \t\tctrls.emplace(id, info);\n>  \t}\n>  \n> -\tcontrolInfo_ = std::move(ctrls);\n> +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>  \n>  \t/* Initialize the camera properties. */\n>  \tproperties_ = sensor_->properties();\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 98d93a12a7be..6ccd8eb86016 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -611,12 +611,13 @@ void V4L2Device::listControls()\n>  \t\t\t\t << \" (\" << utils::hex(ctrl.id) << \")\";\n>  \n>  \t\tcontrolIds_.emplace_back(v4l2ControlId(ctrl));\n> +\t\tcontrolIdMap_[ctrl.id] = controlIds_.back().get();\n>  \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n>  \n>  \t\tctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n>  \t}\n>  \n> -\tcontrols_ = std::move(ctrls);\n> +\tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n>  }\n>  \n>  /**\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index 880bcd02c6be..3d845a129a5f 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -32,13 +32,13 @@\n>  using namespace std;\n>  using namespace libcamera;\n>  \n> -static const ControlInfoMap Controls = {\n> +static const ControlInfoMap Controls = ControlInfoMap({\n>  \t{ &controls::AeEnable, ControlInfo(false, true) },\n>  \t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>  \t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>  \t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>  \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n\nShould this be reindented, like in include/libcamera/ipa/raspberrypi.h ?\n\nOverall this looks good to me.\n\n> -};\n> +\t}, controls::controls);\n>  \n>  namespace libcamera {\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5A5DC3232\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  3 Aug 2021 15:43:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34F4A687CE;\n\tTue,  3 Aug 2021 17:43:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2B6B6026A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  3 Aug 2021 17:43:49 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47C0124F;\n\tTue,  3 Aug 2021 17:43:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"clGGrXNm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628005429;\n\tbh=GBKxbMslkjrrUZRpHg/OuWyMh9AbheVSmcJfedfNM1s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=clGGrXNmnknCF5j3MknMTJAFBKECZkWrSZXBuvJct00b0Rm8YXesguYaZSAYWo6QI\n\tA7kVVXgoZX6S2KRu1z4p5cTBo1mZgL7HlfNduKAuFLsEinfZr3SWvFGgWwD0KvbYqh\n\tktDfbru7hsZ6ef19+IrNOXAwkxGIpp/8tmGXIInM=","Date":"Tue, 3 Aug 2021 18:43:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210728161116.64489-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18618,"web_url":"https://patchwork.libcamera.org/comment/18618/","msgid":"<20210809104822.wq2rfzzhfm7v53vm@uno.localdomain>","date":"2021-08-09T10:48:22","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:\n> > ControlInfoMap do not have a ControlId map associated, but rather\n>\n> s/do not/does not/\n>\n> > create one with the generateIdMap() function at creation time.\n>\n> s/create/creates/\n>\n> > As a consequence, when in the need to de-serialize a ControlInfoMap all\n> > the ControlId it contains are created by the deserializer instance, not\n> > being able to discern if the controls the ControlIdMap refers to are the\n> > global libcamera controls (and proeprties) or instances local to the\n> > V4L2 device that has first initialized the controls.\n> >\n> > As a consequence the ControlId stored in a de-serialized map will always\n> > be newly created entities, preventing lookup by ControlId reference on a\n> > de-serialized ControlInfoMap.\n> >\n> > In order to make it possible to use globally available ControlId\n> > instances whenever possible, create ControlInfoMap with a reference to\n> > an externally allocated ControlIdMap instead of generating one\n> > internally.\n> >\n> > Has a consequence the class constructors take and additional argument,\n>\n> s/Has/As/\n>\n> > which might be not pleaseant to type in, but enforces the concepts that\n> > ControlInfoMap should be created with controls part of the same id map.\n>\n> It's not that unpleasant, it seems worth it to me.\n>\n> > As the ControlIdMap the ControlInfoMap refers to need to be allocated\n>\n> s/need/needs/\n>\n> > externally:\n> > - Use the globally available controls::controls (or\n> >   properties::properties) id map when referring to libcamera controls\n>\n> It's interesting to see that we actually don't use\n> properties::properties yet.\n>\n> > - The V4L2 device that creates ControlInfoMap by parsing the device's\n> >   controls has to allocate a ControlIdMap and ties its lifetime\n> >   management to the its own one\n>\n> \"to the its own one\" hurts a bit :-) You could possibly just drop \"and\n> ties ...\".\n>\n> > - The ControlSerializer that de-serializes a ControlInfoMap has to\n> >   create and store the ControlIdMap the de-serialized info map refers to\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  include/libcamera/controls.h                  | 13 ++-\n> >  .../libcamera/internal/control_serializer.h   |  1 +\n> >  include/libcamera/internal/v4l2_device.h      |  1 +\n> >  include/libcamera/ipa/raspberrypi.h           | 40 ++++-----\n> >  src/libcamera/control_serializer.cpp          | 11 ++-\n> >  src/libcamera/controls.cpp                    | 82 ++++---------------\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  3 +-\n> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  2 +-\n> >  src/libcamera/pipeline/vimc/vimc.cpp          |  2 +-\n> >  src/libcamera/v4l2_device.cpp                 |  3 +-\n> >  .../ipa_data_serializer_test.cpp              |  4 +-\n> >  12 files changed, 59 insertions(+), 106 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1bc958a43b22..4a2cdb335870 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -306,12 +306,11 @@ public:\n> >\n> >  \tControlInfoMap() = default;\n> >  \tControlInfoMap(const ControlInfoMap &other) = default;\n> > -\tControlInfoMap(std::initializer_list<Map::value_type> init);\n> > -\tControlInfoMap(Map &&info);\n> > +\tControlInfoMap(std::initializer_list<Map::value_type> init,\n> > +\t\t       const ControlIdMap &idmap);\n> > +\tControlInfoMap(Map &&info, const ControlIdMap &idmap);\n> >\n> >  \tControlInfoMap &operator=(const ControlInfoMap &other) = default;\n> > -\tControlInfoMap &operator=(std::initializer_list<Map::value_type> init);\n> > -\tControlInfoMap &operator=(Map &&info);\n> >\n> >  \tusing Map::key_type;\n> >  \tusing Map::mapped_type;\n> > @@ -336,12 +335,10 @@ public:\n> >  \titerator find(unsigned int key);\n> >  \tconst_iterator find(unsigned int key) const;\n> >\n> > -\tconst ControlIdMap &idmap() const { return idmap_; }\n> > +\tconst ControlIdMap &idmap() const { return *idmap_; }\n> >\n> >  private:\n> > -\tvoid generateIdmap();\n> > -\n> > -\tControlIdMap idmap_;\n> > +\tconst ControlIdMap *idmap_;\n>\n> You can also store a reference instead of a pointer.\n\nNot really.. I mean, I could but then I will have to remove copy\nassignements and default constructor, breaking the construction of\nCameraData which has a default constructed ControlInfoMap member.\n\n>\n> >  };\n> >\n> >  class ControlList\n> > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> > index 7d4426c95d12..8a66be324138 100644\n> > --- a/include/libcamera/internal/control_serializer.h\n> > +++ b/include/libcamera/internal/control_serializer.h\n> > @@ -48,6 +48,7 @@ private:\n> >\n> >  \tunsigned int serial_;\n> >  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> > +\tstd::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;\n> >  \tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> >  \tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n> >  };\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index 77b835b3cb80..423c8fb11845 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -69,6 +69,7 @@ private:\n> >\n> >  \tstd::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;\n> >  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> > +\tControlIdMap controlIdMap_;\n> >  \tControlInfoMap controls_;\n> >  \tstd::string deviceNode_;\n> >  \tint fd_;\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index a8790000e2d9..521eaecd19b2 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -27,26 +27,26 @@ namespace RPi {\n> >   * and the pipeline handler may be reverted so that it aborts when an\n> >   * unsupported control is encountered.\n> >   */\n> > -static const ControlInfoMap Controls = {\n> > -\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > -\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > -\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > -\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > -\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > -\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > -\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > -\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > -\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > -\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > -\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > -\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > -\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> > -};\n> > +static const ControlInfoMap Controls({\n> > +\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > +\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > +\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > +\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > +\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > +\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > +\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> > +\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > +\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > +\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > +\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > +\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > +\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > +\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > +\t}, controls::controls);\n> >\n> >  } /* namespace RPi */\n> >\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 300466285a57..df6ed93f477e 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -93,6 +93,7 @@ void ControlSerializer::reset()\n> >  \tinfoMapHandles_.clear();\n> >  \tinfoMaps_.clear();\n> >  \tcontrolIds_.clear();\n> > +\tcontrolIdMaps_.clear();\n> >  }\n> >\n> >  size_t ControlSerializer::binarySize(const ControlValue &value)\n> > @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t}\n> >\n> >  \tControlInfoMap::Map ctrls;\n> > +\tcontrolIdMaps_.emplace_back(std::make_unique<ControlIdMap>());\n> > +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n>\n> With\n>\n> \tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n>\n> the code below may look better. Or maybe you could use\n> localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].\n>\n> >\n> >  \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> >  \t\tconst struct ipa_control_info_entry *entry =\n> > @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t\t * purpose.\n> >  \t\t */\n> >  \t\tcontrolIds_.emplace_back(std::make_unique<ControlId>(entry->id, \"\", type));\n> > +\t\tControlId *controlId = controlIds_.back().get();\n> > +\t\t(*localIdMap)[entry->id] = controlId;\n> >\n> >  \t\tif (entry->offset != values.offset()) {\n> >  \t\t\tLOG(Serializer, Error)\n> > @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t\t}\n> >\n> >  \t\t/* Create and store the ControlInfo. */\n> > -\t\tctrls.emplace(controlIds_.back().get(),\n> > -\t\t\t      loadControlInfo(type, values));\n> > +\t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> >  \t}\n> >\n> >  \t/*\n> >  \t * Create the ControlInfoMap in the cache, and store the map to handle\n> >  \t * association.\n> >  \t */\n> > -\tControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);\n> > +\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);\n> > +\tControlInfoMap &map = infoMaps_[hdr->handle];\n> >  \tinfoMapHandles_[&map] = hdr->handle;\n> >\n> >  \treturn map;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 78109f4130a8..9409f0f9e782 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const\n>\n> The second paragraph of the ControlInfoMap class documentation needs\n> to be updated, to explain how the class now needs to be constructed with\n> an idmap (and how all elements must refer to the same idmap).\n>\n> >  /**\n> >   * \\brief Construct a ControlInfoMap from an initializer list\n> >   * \\param[in] init The initializer list\n> > + * \\param[in] idmap The idmap used by the ControlInfoMap\n> >   */\n> > -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)\n> > -\t: Map(init)\n> > +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,\n> > +\t\t\t       const ControlIdMap &idmap)\n> > +\t: Map(init), idmap_(&idmap)\n> >  {\n> > -\tgenerateIdmap();\n> >  }\n> >\n> >  /**\n> >   * \\brief Construct a ControlInfoMap from a plain map\n> >   * \\param[in] info The control info plain map\n> > + * \\param[in] idmap The idmap used by the ControlInfoMap\n> >   *\n> >   * Construct a new ControlInfoMap and populate its contents with those of\n> >   * \\a info using move semantics. Upon return the \\a info map will be empty.\n> >   */\n> > -ControlInfoMap::ControlInfoMap(Map &&info)\n> > -\t: Map(std::move(info))\n> > +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)\n> > +\t: Map(std::move(info)), idmap_(&idmap)\n> >  {\n> > -\tgenerateIdmap();\n> >  }\n> >\n> >  /**\n> > @@ -643,34 +644,6 @@ ControlInfoMap::ControlInfoMap(Map &&info)\n> >   * \\return A reference to the ControlInfoMap\n> >   */\n> >\n> > -/**\n> > - * \\brief Replace the contents with those from the initializer list\n> > - * \\param[in] init The initializer list\n> > - * \\return A reference to the ControlInfoMap\n> > - */\n> > -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)\n> > -{\n> > -\tMap::operator=(init);\n> > -\tgenerateIdmap();\n> > -\treturn *this;\n> > -}\n> > -\n> > -/**\n> > - * \\brief Move assignment operator from a plain map\n> > - * \\param[in] info The control info plain map\n> > - *\n> > - * Populate the map by replacing its contents with those of \\a info using move\n> > - * semantics. Upon return the \\a info map will be empty.\n> > - *\n> > - * \\return A reference to the populated ControlInfoMap\n> > - */\n> > -ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> > -{\n> > -\tMap::operator=(std::move(info));\n> > -\tgenerateIdmap();\n> > -\treturn *this;\n> > -}\n> > -\n> >  /**\n> >   * \\brief Access specified element by numerical ID\n> >   * \\param[in] id The numerical ID\n> > @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> >   */\n> >  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n> >  {\n> > -\treturn at(idmap_.at(id));\n> > +\treturn at(idmap_->at(id));\n> >  }\n> >\n> >  /**\n> > @@ -688,7 +661,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n> >   */\n> >  const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const\n> >  {\n> > -\treturn at(idmap_.at(id));\n> > +\treturn at(idmap_->at(id));\n> >  }\n> >\n> >  /**\n> > @@ -703,7 +676,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n> >  \t * entries, we can thus just count the matching entries in idmap to\n> >  \t * avoid an additional lookup.\n> >  \t */\n> > -\treturn idmap_.count(id);\n> > +\treturn idmap_->count(id);\n> >  }\n> >\n> >  /**\n> > @@ -714,8 +687,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n> >   */\n> >  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n> >  {\n> > -\tauto iter = idmap_.find(id);\n> > -\tif (iter == idmap_.end())\n> > +\tauto iter = idmap_->find(id);\n> > +\tif (iter == idmap_->end())\n> >  \t\treturn end();\n> >\n> >  \treturn find(iter->second);\n> > @@ -729,8 +702,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n> >   */\n> >  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> >  {\n> > -\tauto iter = idmap_.find(id);\n> > -\tif (iter == idmap_.end())\n> > +\tauto iter = idmap_->find(id);\n> > +\tif (iter == idmap_->end())\n> >  \t\treturn end();\n> >\n> >  \treturn find(iter->second);\n> > @@ -747,33 +720,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> >   * \\return The ControlId map\n> >   */\n> >\n> > -void ControlInfoMap::generateIdmap()\n> > -{\n> > -\tidmap_.clear();\n> > -\n> > -\tfor (const auto &ctrl : *this) {\n> > -\t\t/*\n> > -\t\t * For string controls, min and max define the valid\n> > -\t\t * range for the string size, not for the individual\n> > -\t\t * values.\n> > -\t\t */\n> > -\t\tControlType rangeType = ctrl.first->type() == ControlTypeString\n> > -\t\t\t\t      ? ControlTypeInteger32 : ctrl.first->type();\n> > -\t\tconst ControlInfo &info = ctrl.second;\n> > -\n> > -\t\tif (info.min().type() != rangeType) {\n> > -\t\t\tLOG(Controls, Error)\n> > -\t\t\t\t<< \"Control \" << utils::hex(ctrl.first->id())\n> > -\t\t\t\t<< \" type and info type mismatch\";\n> > -\t\t\tidmap_.clear();\n> > -\t\t\tclear();\n> > -\t\t\treturn;\n> > -\t\t}\n>\n> I think those sanity checks should be kept, You can rename the function\n> to validate() (or something similar) and call it from the constructors.\n> A useful additional check would be to verify that each entry in the\n> control info map is in the idmap.\n\nI will ASSERT() that.\n\nThanks\n  j\n\n>\n> > -\n> > -\t\tidmap_[ctrl.first->id()] = ctrl.first;\n> > -\t}\n> > -}\n> > -\n> >  /**\n> >   * \\class ControlList\n> >   * \\brief Associate a list of ControlId with their values for an object\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 76c3bb3d8aa9..048993365b44 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1057,7 +1057,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >\n> >  \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> >\n> > -\tdata->controlInfo_ = std::move(controls);\n> > +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> > +\t\t\t\t\t    controls::controls);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 42911a8fdfdb..710b9309448e 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >  \t\t      std::forward_as_tuple(&controls::AeEnable),\n> >  \t\t      std::forward_as_tuple(false, true));\n> >\n> > -\tdata->controlInfo_ = std::move(ctrls);\n> > +\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> > +\t\t\t\t\t    controls::controls);\n> >\n> >  \tdata->sensor_ = std::make_unique<CameraSensor>(sensor);\n> >  \tret = data->sensor_->init();\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 0f634b8da609..573d8042c18c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)\n> >  \t\taddControl(cid, info, &ctrls);\n> >  \t}\n> >\n> > -\tcontrolInfo_ = std::move(ctrls);\n> > +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 12f7517fd0ae..d4b041d33eb4 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -512,7 +512,7 @@ int VimcCameraData::init()\n> >  \t\tctrls.emplace(id, info);\n> >  \t}\n> >\n> > -\tcontrolInfo_ = std::move(ctrls);\n> > +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >\n> >  \t/* Initialize the camera properties. */\n> >  \tproperties_ = sensor_->properties();\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 98d93a12a7be..6ccd8eb86016 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -611,12 +611,13 @@ void V4L2Device::listControls()\n> >  \t\t\t\t << \" (\" << utils::hex(ctrl.id) << \")\";\n> >\n> >  \t\tcontrolIds_.emplace_back(v4l2ControlId(ctrl));\n> > +\t\tcontrolIdMap_[ctrl.id] = controlIds_.back().get();\n> >  \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n> >\n> >  \t\tctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n> >  \t}\n> >\n> > -\tcontrols_ = std::move(ctrls);\n> > +\tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> >  }\n> >\n> >  /**\n> > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> > index 880bcd02c6be..3d845a129a5f 100644\n> > --- a/test/serialization/ipa_data_serializer_test.cpp\n> > +++ b/test/serialization/ipa_data_serializer_test.cpp\n> > @@ -32,13 +32,13 @@\n> >  using namespace std;\n> >  using namespace libcamera;\n> >\n> > -static const ControlInfoMap Controls = {\n> > +static const ControlInfoMap Controls = ControlInfoMap({\n> >  \t{ &controls::AeEnable, ControlInfo(false, true) },\n> >  \t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> >  \t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> >  \t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >  \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>\n> Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?\n>\n> Overall this looks good to me.\n>\n> > -};\n> > +\t}, controls::controls);\n> >\n> >  namespace libcamera {\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7246BC3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  9 Aug 2021 10:47:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C88D36884F;\n\tMon,  9 Aug 2021 12:47:34 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C92C60269\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Aug 2021 12:47:34 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id D711C240003;\n\tMon,  9 Aug 2021 10:47:33 +0000 (UTC)"],"Date":"Mon, 9 Aug 2021 12:48:22 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210809104822.wq2rfzzhfm7v53vm@uno.localdomain>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-2-jacopo@jmondi.org>\n\t<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18620,"web_url":"https://patchwork.libcamera.org/comment/18620/","msgid":"<20210809124321.h5ivfpi7bne2ivud@uno.localdomain>","date":"2021-08-09T12:43:21","subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n  sorry one more comment I missed in previous reply\n\nOn Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n\n[snip]\n\n> > @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t}\n> >\n> >  \tControlInfoMap::Map ctrls;\n> > +\tcontrolIdMaps_.emplace_back(std::make_unique<ControlIdMap>());\n> > +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n>\n> With\n>\n> \tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n>\n> the code below may look better. Or maybe you could use\n> localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].\n>\n\nNot really in this case, as I use operator[] to insert a new element,\nwhile std::unsorted_map::at() only allows to retrieve the existing\nones.\n\nI could use a reference, but I need a pointer in the next patch, so I\nguess we'll have to stick to the (*)[] syntax\n\n> >\n> >  \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> >  \t\tconst struct ipa_control_info_entry *entry =\n> > @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t\t * purpose.\n> >  \t\t */\n> >  \t\tcontrolIds_.emplace_back(std::make_unique<ControlId>(entry->id, \"\", type));\n> > +\t\tControlId *controlId = controlIds_.back().get();\n> > +\t\t(*localIdMap)[entry->id] = controlId;\n> >\n> >  \t\tif (entry->offset != values.offset()) {\n> >  \t\t\tLOG(Serializer, Error)\n> > @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &\n> >  \t\t}\n> >\n> >  \t\t/* Create and store the ControlInfo. */\n> > -\t\tctrls.emplace(controlIds_.back().get(),\n> > -\t\t\t      loadControlInfo(type, values));\n> > +\t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> >  \t}\n> >\n> >  \t/*\n> >  \t * Create the ControlInfoMap in the cache, and store the map to handle\n> >  \t * association.\n> >  \t */\n> > -\tControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);\n> > +\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);\n> > +\tControlInfoMap &map = infoMaps_[hdr->handle];\n> >  \tinfoMapHandles_[&map] = hdr->handle;\n> >\n> >  \treturn map;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 78109f4130a8..9409f0f9e782 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const\n>\n> The second paragraph of the ControlInfoMap class documentation needs\n> to be updated, to explain how the class now needs to be constructed with\n> an idmap (and how all elements must refer to the same idmap).\n>\n> >  /**\n> >   * \\brief Construct a ControlInfoMap from an initializer list\n> >   * \\param[in] init The initializer list\n> > + * \\param[in] idmap The idmap used by the ControlInfoMap\n> >   */\n> > -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)\n> > -\t: Map(init)\n> > +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,\n> > +\t\t\t       const ControlIdMap &idmap)\n> > +\t: Map(init), idmap_(&idmap)\n> >  {\n> > -\tgenerateIdmap();\n> >  }\n> >\n> >  /**\n> >   * \\brief Construct a ControlInfoMap from a plain map\n> >   * \\param[in] info The control info plain map\n> > + * \\param[in] idmap The idmap used by the ControlInfoMap\n> >   *\n> >   * Construct a new ControlInfoMap and populate its contents with those of\n> >   * \\a info using move semantics. Upon return the \\a info map will be empty.\n> >   */\n> > -ControlInfoMap::ControlInfoMap(Map &&info)\n> > -\t: Map(std::move(info))\n> > +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)\n> > +\t: Map(std::move(info)), idmap_(&idmap)\n> >  {\n> > -\tgenerateIdmap();\n> >  }\n> >\n> >  /**\n> > @@ -643,34 +644,6 @@ ControlInfoMap::ControlInfoMap(Map &&info)\n> >   * \\return A reference to the ControlInfoMap\n> >   */\n> >\n> > -/**\n> > - * \\brief Replace the contents with those from the initializer list\n> > - * \\param[in] init The initializer list\n> > - * \\return A reference to the ControlInfoMap\n> > - */\n> > -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)\n> > -{\n> > -\tMap::operator=(init);\n> > -\tgenerateIdmap();\n> > -\treturn *this;\n> > -}\n> > -\n> > -/**\n> > - * \\brief Move assignment operator from a plain map\n> > - * \\param[in] info The control info plain map\n> > - *\n> > - * Populate the map by replacing its contents with those of \\a info using move\n> > - * semantics. Upon return the \\a info map will be empty.\n> > - *\n> > - * \\return A reference to the populated ControlInfoMap\n> > - */\n> > -ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> > -{\n> > -\tMap::operator=(std::move(info));\n> > -\tgenerateIdmap();\n> > -\treturn *this;\n> > -}\n> > -\n> >  /**\n> >   * \\brief Access specified element by numerical ID\n> >   * \\param[in] id The numerical ID\n> > @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)\n> >   */\n> >  ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n> >  {\n> > -\treturn at(idmap_.at(id));\n> > +\treturn at(idmap_->at(id));\n> >  }\n> >\n> >  /**\n> > @@ -688,7 +661,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)\n> >   */\n> >  const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const\n> >  {\n> > -\treturn at(idmap_.at(id));\n> > +\treturn at(idmap_->at(id));\n> >  }\n> >\n> >  /**\n> > @@ -703,7 +676,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n> >  \t * entries, we can thus just count the matching entries in idmap to\n> >  \t * avoid an additional lookup.\n> >  \t */\n> > -\treturn idmap_.count(id);\n> > +\treturn idmap_->count(id);\n> >  }\n> >\n> >  /**\n> > @@ -714,8 +687,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const\n> >   */\n> >  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n> >  {\n> > -\tauto iter = idmap_.find(id);\n> > -\tif (iter == idmap_.end())\n> > +\tauto iter = idmap_->find(id);\n> > +\tif (iter == idmap_->end())\n> >  \t\treturn end();\n> >\n> >  \treturn find(iter->second);\n> > @@ -729,8 +702,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)\n> >   */\n> >  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> >  {\n> > -\tauto iter = idmap_.find(id);\n> > -\tif (iter == idmap_.end())\n> > +\tauto iter = idmap_->find(id);\n> > +\tif (iter == idmap_->end())\n> >  \t\treturn end();\n> >\n> >  \treturn find(iter->second);\n> > @@ -747,33 +720,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> >   * \\return The ControlId map\n> >   */\n> >\n> > -void ControlInfoMap::generateIdmap()\n> > -{\n> > -\tidmap_.clear();\n> > -\n> > -\tfor (const auto &ctrl : *this) {\n> > -\t\t/*\n> > -\t\t * For string controls, min and max define the valid\n> > -\t\t * range for the string size, not for the individual\n> > -\t\t * values.\n> > -\t\t */\n> > -\t\tControlType rangeType = ctrl.first->type() == ControlTypeString\n> > -\t\t\t\t      ? ControlTypeInteger32 : ctrl.first->type();\n> > -\t\tconst ControlInfo &info = ctrl.second;\n> > -\n> > -\t\tif (info.min().type() != rangeType) {\n> > -\t\t\tLOG(Controls, Error)\n> > -\t\t\t\t<< \"Control \" << utils::hex(ctrl.first->id())\n> > -\t\t\t\t<< \" type and info type mismatch\";\n> > -\t\t\tidmap_.clear();\n> > -\t\t\tclear();\n> > -\t\t\treturn;\n> > -\t\t}\n>\n> I think those sanity checks should be kept, You can rename the function\n> to validate() (or something similar) and call it from the constructors.\n> A useful additional check would be to verify that each entry in the\n> control info map is in the idmap.\n>\n> > -\n> > -\t\tidmap_[ctrl.first->id()] = ctrl.first;\n> > -\t}\n> > -}\n> > -\n> >  /**\n> >   * \\class ControlList\n> >   * \\brief Associate a list of ControlId with their values for an object\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 76c3bb3d8aa9..048993365b44 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1057,7 +1057,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >\n> >  \tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> >\n> > -\tdata->controlInfo_ = std::move(controls);\n> > +\tdata->controlInfo_ = ControlInfoMap(std::move(controls),\n> > +\t\t\t\t\t    controls::controls);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 42911a8fdfdb..710b9309448e 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >  \t\t      std::forward_as_tuple(&controls::AeEnable),\n> >  \t\t      std::forward_as_tuple(false, true));\n> >\n> > -\tdata->controlInfo_ = std::move(ctrls);\n> > +\tdata->controlInfo_ = ControlInfoMap(std::move(ctrls),\n> > +\t\t\t\t\t    controls::controls);\n> >\n> >  \tdata->sensor_ = std::make_unique<CameraSensor>(sensor);\n> >  \tret = data->sensor_->init();\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 0f634b8da609..573d8042c18c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)\n> >  \t\taddControl(cid, info, &ctrls);\n> >  \t}\n> >\n> > -\tcontrolInfo_ = std::move(ctrls);\n> > +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 12f7517fd0ae..d4b041d33eb4 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -512,7 +512,7 @@ int VimcCameraData::init()\n> >  \t\tctrls.emplace(id, info);\n> >  \t}\n> >\n> > -\tcontrolInfo_ = std::move(ctrls);\n> > +\tcontrolInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >\n> >  \t/* Initialize the camera properties. */\n> >  \tproperties_ = sensor_->properties();\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 98d93a12a7be..6ccd8eb86016 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -611,12 +611,13 @@ void V4L2Device::listControls()\n> >  \t\t\t\t << \" (\" << utils::hex(ctrl.id) << \")\";\n> >\n> >  \t\tcontrolIds_.emplace_back(v4l2ControlId(ctrl));\n> > +\t\tcontrolIdMap_[ctrl.id] = controlIds_.back().get();\n> >  \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n> >\n> >  \t\tctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n> >  \t}\n> >\n> > -\tcontrols_ = std::move(ctrls);\n> > +\tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> >  }\n> >\n> >  /**\n> > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> > index 880bcd02c6be..3d845a129a5f 100644\n> > --- a/test/serialization/ipa_data_serializer_test.cpp\n> > +++ b/test/serialization/ipa_data_serializer_test.cpp\n> > @@ -32,13 +32,13 @@\n> >  using namespace std;\n> >  using namespace libcamera;\n> >\n> > -static const ControlInfoMap Controls = {\n> > +static const ControlInfoMap Controls = ControlInfoMap({\n> >  \t{ &controls::AeEnable, ControlInfo(false, true) },\n> >  \t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> >  \t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> >  \t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >  \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>\n> Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?\n>\n> Overall this looks good to me.\n>\n> > -};\n> > +\t}, controls::controls);\n> >\n> >  namespace libcamera {\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C52F1C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  9 Aug 2021 12:42:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13E3168826;\n\tMon,  9 Aug 2021 14:42:35 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 097E960269\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Aug 2021 14:42:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6ED2F1BF20A;\n\tMon,  9 Aug 2021 12:42:32 +0000 (UTC)"],"Date":"Mon, 9 Aug 2021 14:43:21 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210809124321.h5ivfpi7bne2ivud@uno.localdomain>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-2-jacopo@jmondi.org>\n\t<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<YQlkKXe/KghMVbKZ@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create\n\tControlInfoMap with ControlIdMap","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]