[{"id":18644,"web_url":"https://patchwork.libcamera.org/comment/18644/","msgid":"","date":"2021-08-10T00:45:46","subject":"Re: [libcamera-devel] [PATCH v3 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 Mon, Aug 09, 2021 at 05:23:04PM +0200, Jacopo Mondi wrote:\n> ControlInfoMap does not have a ControlId map associated, but rather\n> creates 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\ns/Has/As/\n\n> which might be not pleaseant to type in, but enforces the concepts that\n\ns/pleaseant/pleasant/\n\n> ControlInfoMap should be created with controls part of the same id map.\n> \n> As the ControlIdMap the ControlInfoMap refers to needs 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\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 \n> Reviewed-by: Paul Elder \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 | 94 +++++--------------\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 | 14 +--\n> 12 files changed, 77 insertions(+), 110 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index de733bd868a6..e9e09c200da2 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -309,12 +309,11 @@ public:\n> \n> \tControlInfoMap() = default;\n> \tControlInfoMap(const ControlInfoMap &other) = default;\n> -\tControlInfoMap(std::initializer_list init);\n> -\tControlInfoMap(Map &&info);\n> +\tControlInfoMap(std::initializer_list 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 init);\n> -\tControlInfoMap &operator=(Map &&info);\n> \n> \tusing Map::key_type;\n> \tusing Map::mapped_type;\n> @@ -339,12 +338,12 @@ 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> +\tconst ControlIdMap *idmap_;\n> \n> -\tControlIdMap idmap_;\n> +\tvoid validate();\n\nWe usually put functions before variables.\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> controlIds_;\n> +\tstd::vector> controlIdMaps_;\n> \tstd::map infoMaps_;\n> \tstd::map 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 controlInfo_;\n> \tstd::vector> 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(ByteStreamBuffer &\n> \t}\n> \n> \tControlInfoMap::Map ctrls;\n> +\tcontrolIdMaps_.emplace_back(std::make_unique());\n> +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n\nMy suggestion to use ->at() was wrong, but how about the other one,\nusing\n\n\tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n\nhere ?\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(ByteStreamBuffer &\n> \t\t * purpose.\n> \t\t */\n> \t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> +\t\tControlId *controlId = controlIds_.back().get();\n> +\t\t(*localIdMap)[entry->id] = controlId;\n\nYou will be able to write\n\n\t\tlocalIdMap[entry->id] = controlId;\n\n> \n> \t\tif (entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize(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 283472c5ab4d..f891692da669 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -625,10 +625,10 @@ std::string ControlInfo::toString() const\n> * constructed, and thus only exposes the read accessors of the\n> * std::unsorted_map<> base class.\n> *\n> - * In addition to the features of the standard unsorted map, this class also\n> - * provides access to the mapped elements using numerical ID keys. It maintains\n> - * an internal map of numerical ID to ControlId for this purpose, and exposes it\n> - * through the idmap() method to help construction of ControlList instances.\n> + * The class needs to be constructed with a reference to the ControlIdMap all\n> + * the ControlId belongs to. The ControlIdMap is used to provide, in addition to\n\ns/belongs/belong/\n\n> + * the features of the standard unsorted map, access to the mapped elements\n> + * using numerical ID keys.\n\nI'd like to put a stronger emphasis on the fact that all the entries\nhave to belong to the same map:\n\n * The class is constructed with a reference to a ControlIdMap. This allows\n * providing access to the mapped elements using numerical ID keys, in addition\n * to the features of the standard unsorted map. All ControlId keys in the map\n * must appear in the ControlIdMap.\n\n(Not sure I'm 100% happy with the result)\n\n> */\n> \n> /**\n> @@ -645,24 +645,27 @@ 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 init)\n> -\t: Map(init)\n> +ControlInfoMap::ControlInfoMap(std::initializer_list init,\n> +\t\t\t const ControlIdMap &idmap)\n> +\t: Map(init), idmap_(&idmap)\n> {\n> -\tgenerateIdmap();\n> +\tvalidate();\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> +\tvalidate();\n> }\n> \n> /**\n> @@ -672,32 +675,14 @@ 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 init)\n> +void ControlInfoMap::validate()\n> {\n> -\tMap::operator=(init);\n> -\tgenerateIdmap();\n> -\treturn *this;\n> -}\n> +\tfor (const auto &controlInfo : *this) {\n> +\t\tconst ControlId *id = controlInfo.first;\n> +\t\tauto it = idmap_->find(id->id());\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> +\t\tASSERT(it != idmap_->end());\n> +\t}\n\nYou also need to check that it->second == id, as the numerical IDs could\nbe the same, but the ControlId different.\n\nTo compile the code out in non-debug builds, we could move the assert\nout of the loop:\n\n\tASSERT(!std::count_if(begin(), end(),\n\t\t\t [&](const Map::value_type &item) {\n\t\t\t\t const ControlId *id = item.first;\n\t\t\t\t const auto it = idmap_->find(id->id());\n\t\t\t\t return it == idmap_->end() || it->second != id;\n\t\t\t }));\n\nOr maybe\n\n\tASSERT(validate());\n\nin the callers would be cleaner :-)\n\n> }\n> \n> /**\n> @@ -707,7 +692,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> @@ -717,7 +702,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> @@ -732,7 +717,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> @@ -743,8 +728,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> @@ -758,8 +743,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> @@ -776,33 +761,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\nThanks for adding the validate function, but I think you forgot to\ninclude this check in there :-)\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 19cb5c4ec9c3..9c23788e5231 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1071,7 +1071,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(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..bf7e34e28af3 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> -\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> +static const ControlInfoMap Controls = ControlInfoMap({\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::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> +\t}, controls::controls);\n> \n> namespace libcamera {\n>","headers":{"Return-Path":"","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 28631C3240\n\tfor ;\n\tTue, 10 Aug 2021 00:45:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75CCD68826;\n\tTue, 10 Aug 2021 02:45:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22F67687EB\n\tfor ;\n\tTue, 10 Aug 2021 02:45: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 8B2E8466;\n\tTue, 10 Aug 2021 02:45:48 +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=\"NxHioOB/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628556348;\n\tbh=DMYsP1L2xtRxHPljSKLtr93kMGc9tVbw9gobaRhLpuc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NxHioOB/1lgRZr0j0j3LvTD2axMep5j2hsHzm6AM6fXBDKxkrvKPH3KlDnd7PokJF\n\tKhMAtdTWMSiefoIfYnBLR7SACkjen0TVkK/mKYigDLmAWUWUK819CKHcvvsdBj5XVH\n\tWCk7/Vlc7aLc88+Eu7e3XQMUcW+gFBr+mt5ZN9FQ=","Date":"Tue, 10 Aug 2021 03:45:46 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210809152308.31947-1-jacopo@jmondi.org>\n\t<20210809152308.31947-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210809152308.31947-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18659,"web_url":"https://patchwork.libcamera.org/comment/18659/","msgid":"<20210810074457.zsj2s6oehr33r42f@uno.localdomain>","date":"2021-08-10T07:44:57","subject":"Re: [libcamera-devel] [PATCH v3 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 10, 2021 at 03:45:46AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 09, 2021 at 05:23:04PM +0200, Jacopo Mondi wrote:\n> > ControlInfoMap does not have a ControlId map associated, but rather\n> > creates 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>\n> s/Has/As/\n>\n> > which might be not pleaseant to type in, but enforces the concepts that\n>\n> s/pleaseant/pleasant/\n>\n> > ControlInfoMap should be created with controls part of the same id map.\n> >\n> > As the ControlIdMap the ControlInfoMap refers to needs 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\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 \n> > Reviewed-by: Paul Elder \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 | 94 +++++--------------\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 | 14 +--\n> > 12 files changed, 77 insertions(+), 110 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index de733bd868a6..e9e09c200da2 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -309,12 +309,11 @@ public:\n> >\n> > \tControlInfoMap() = default;\n> > \tControlInfoMap(const ControlInfoMap &other) = default;\n> > -\tControlInfoMap(std::initializer_list init);\n> > -\tControlInfoMap(Map &&info);\n> > +\tControlInfoMap(std::initializer_list 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 init);\n> > -\tControlInfoMap &operator=(Map &&info);\n> >\n> > \tusing Map::key_type;\n> > \tusing Map::mapped_type;\n> > @@ -339,12 +338,12 @@ 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> > +\tconst ControlIdMap *idmap_;\n> >\n> > -\tControlIdMap idmap_;\n> > +\tvoid validate();\n>\n> We usually put functions before variables.\n>\n\nUps, trivial\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> controlIds_;\n> > +\tstd::vector> controlIdMaps_;\n> > \tstd::map infoMaps_;\n> > \tstd::map 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 controlInfo_;\n> > \tstd::vector> 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(ByteStreamBuffer &\n> > \t}\n> >\n> > \tControlInfoMap::Map ctrls;\n> > +\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n>\n> My suggestion to use ->at() was wrong, but how about the other one,\n> using\n>\n> \tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n>\n> here ?\n>\n\nAs I replied to the previous version, I need a pointer in the next\npatch, so having a & here to turn it into a pointer immediately after\ndoesn't bring much value, doesn't it ?\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(ByteStreamBuffer &\n> > \t\t * purpose.\n> > \t\t */\n> > \t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> > +\t\tControlId *controlId = controlIds_.back().get();\n> > +\t\t(*localIdMap)[entry->id] = controlId;\n>\n> You will be able to write\n>\n> \t\tlocalIdMap[entry->id] = controlId;\n>\n> >\n> > \t\tif (entry->offset != values.offset()) {\n> > \t\t\tLOG(Serializer, Error)\n> > @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize(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 283472c5ab4d..f891692da669 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -625,10 +625,10 @@ std::string ControlInfo::toString() const\n> > * constructed, and thus only exposes the read accessors of the\n> > * std::unsorted_map<> base class.\n> > *\n> > - * In addition to the features of the standard unsorted map, this class also\n> > - * provides access to the mapped elements using numerical ID keys. It maintains\n> > - * an internal map of numerical ID to ControlId for this purpose, and exposes it\n> > - * through the idmap() method to help construction of ControlList instances.\n> > + * The class needs to be constructed with a reference to the ControlIdMap all\n> > + * the ControlId belongs to. The ControlIdMap is used to provide, in addition to\n>\n> s/belongs/belong/\n>\n> > + * the features of the standard unsorted map, access to the mapped elements\n> > + * using numerical ID keys.\n>\n> I'd like to put a stronger emphasis on the fact that all the entries\n> have to belong to the same map:\n>\n> * The class is constructed with a reference to a ControlIdMap. This allows\n> * providing access to the mapped elements using numerical ID keys, in addition\n> * to the features of the standard unsorted map. All ControlId keys in the map\n> * must appear in the ControlIdMap.\n>\n> (Not sure I'm 100% happy with the result)\n\nI would s/must appear/must belong/ but I got the point\n\n>\n> > */\n> >\n> > /**\n> > @@ -645,24 +645,27 @@ 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 init)\n> > -\t: Map(init)\n> > +ControlInfoMap::ControlInfoMap(std::initializer_list init,\n> > +\t\t\t const ControlIdMap &idmap)\n> > +\t: Map(init), idmap_(&idmap)\n> > {\n> > -\tgenerateIdmap();\n> > +\tvalidate();\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> > +\tvalidate();\n> > }\n> >\n> > /**\n> > @@ -672,32 +675,14 @@ 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 init)\n> > +void ControlInfoMap::validate()\n> > {\n> > -\tMap::operator=(init);\n> > -\tgenerateIdmap();\n> > -\treturn *this;\n> > -}\n> > +\tfor (const auto &controlInfo : *this) {\n> > +\t\tconst ControlId *id = controlInfo.first;\n> > +\t\tauto it = idmap_->find(id->id());\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> > +\t\tASSERT(it != idmap_->end());\n> > +\t}\n>\n> You also need to check that it->second == id, as the numerical IDs could\n> be the same, but the ControlId different.\n\nI'll test and see what happens when the IPA is isolated, as the\ndeserializer creates new ControlIds for v4l2 controls so maybe the\ncheck would fail...\n\n>\n> To compile the code out in non-debug builds, we could move the assert\n> out of the loop:\n>\n> \tASSERT(!std::count_if(begin(), end(),\n> \t\t\t [&](const Map::value_type &item) {\n> \t\t\t\t const ControlId *id = item.first;\n> \t\t\t\t const auto it = idmap_->find(id->id());\n> \t\t\t\t return it == idmap_->end() || it->second != id;\n> \t\t\t }));\n>\n> Or maybe\n>\n> \tASSERT(validate());\n>\n> in the callers would be cleaner :-)\n\nYes, that's probably better, I started with that but then changed it\nbut it was probably better to have a bool validate() and assert in the\ncaller.\n\n>\n> > }\n> >\n> > /**\n> > @@ -707,7 +692,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> > @@ -717,7 +702,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> > @@ -732,7 +717,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> > @@ -743,8 +728,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> > @@ -758,8 +743,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> > @@ -776,33 +761,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> Thanks for adding the validate function, but I think you forgot to\n> include this check in there :-)\n>\n\nDo we still need to validate the type in your opinion ? when using\nglobally defined control id maps (controls and properties) the control\ninfo are created by the pipeline handler, when using v4l2 controls the\ncontrol info type is retrieved from the v4l2 controls.. it's hard to\nget it wrong, but checking doesn't hurt I guess...\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 19cb5c4ec9c3..9c23788e5231 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1071,7 +1071,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(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..bf7e34e28af3 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> > -\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> > +static const ControlInfoMap Controls = ControlInfoMap({\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::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > +\t}, controls::controls);\n> >\n> > namespace libcamera {\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 6EA1FBD87D\n\tfor ;\n\tTue, 10 Aug 2021 07:44:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D14B468826;\n\tTue, 10 Aug 2021 09:44:11 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0196F687DE\n\tfor ;\n\tTue, 10 Aug 2021 09:44:09 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 6EB07C0004;\n\tTue, 10 Aug 2021 07:44:09 +0000 (UTC)"],"Date":"Tue, 10 Aug 2021 09:44:57 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210810074457.zsj2s6oehr33r42f@uno.localdomain>","References":"<20210809152308.31947-1-jacopo@jmondi.org>\n\t<20210809152308.31947-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18748,"web_url":"https://patchwork.libcamera.org/comment/18748/","msgid":"","date":"2021-08-13T00:30:37","subject":"Re: [libcamera-devel] [PATCH v3 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\nI know this has been merged already, but there's one coment below.\n\nOn Tue, Aug 10, 2021 at 09:44:57AM +0200, Jacopo Mondi wrote:\n> On Tue, Aug 10, 2021 at 03:45:46AM +0300, Laurent Pinchart wrote:\n> > On Mon, Aug 09, 2021 at 05:23:04PM +0200, Jacopo Mondi wrote:\n> > > ControlInfoMap does not have a ControlId map associated, but rather\n> > > creates 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> >\n> > s/Has/As/\n> >\n> > > which might be not pleaseant to type in, but enforces the concepts that\n> >\n> > s/pleaseant/pleasant/\n> >\n> > > ControlInfoMap should be created with controls part of the same id map.\n> > >\n> > > As the ControlIdMap the ControlInfoMap refers to needs 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\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 \n> > > Reviewed-by: Paul Elder \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 | 94 +++++--------------\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 | 14 +--\n> > > 12 files changed, 77 insertions(+), 110 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index de733bd868a6..e9e09c200da2 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -309,12 +309,11 @@ public:\n> > >\n> > > \tControlInfoMap() = default;\n> > > \tControlInfoMap(const ControlInfoMap &other) = default;\n> > > -\tControlInfoMap(std::initializer_list init);\n> > > -\tControlInfoMap(Map &&info);\n> > > +\tControlInfoMap(std::initializer_list 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 init);\n> > > -\tControlInfoMap &operator=(Map &&info);\n> > >\n> > > \tusing Map::key_type;\n> > > \tusing Map::mapped_type;\n> > > @@ -339,12 +338,12 @@ 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> > > +\tconst ControlIdMap *idmap_;\n> > >\n> > > -\tControlIdMap idmap_;\n> > > +\tvoid validate();\n> >\n> > We usually put functions before variables.\n> >\n> \n> Ups, trivial\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> controlIds_;\n> > > +\tstd::vector> controlIdMaps_;\n> > > \tstd::map infoMaps_;\n> > > \tstd::map 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 controlInfo_;\n> > > \tstd::vector> 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(ByteStreamBuffer &\n> > > \t}\n> > >\n> > > \tControlInfoMap::Map ctrls;\n> > > +\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > > +\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> >\n> > My suggestion to use ->at() was wrong, but how about the other one,\n> > using\n> >\n> > \tControlIdMap &localIdMap = *controlIdMaps_.back().get();\n> >\n> > here ?\n> >\n> \n> As I replied to the previous version, I need a pointer in the next\n> patch, so having a & here to turn it into a pointer immediately after\n> doesn't bring much value, doesn't it ?\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(ByteStreamBuffer &\n> > > \t\t * purpose.\n> > > \t\t */\n> > > \t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> > > +\t\tControlId *controlId = controlIds_.back().get();\n> > > +\t\t(*localIdMap)[entry->id] = controlId;\n> >\n> > You will be able to write\n> >\n> > \t\tlocalIdMap[entry->id] = controlId;\n> >\n> > >\n> > > \t\tif (entry->offset != values.offset()) {\n> > > \t\t\tLOG(Serializer, Error)\n> > > @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize(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 283472c5ab4d..f891692da669 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -625,10 +625,10 @@ std::string ControlInfo::toString() const\n> > > * constructed, and thus only exposes the read accessors of the\n> > > * std::unsorted_map<> base class.\n> > > *\n> > > - * In addition to the features of the standard unsorted map, this class also\n> > > - * provides access to the mapped elements using numerical ID keys. It maintains\n> > > - * an internal map of numerical ID to ControlId for this purpose, and exposes it\n> > > - * through the idmap() method to help construction of ControlList instances.\n> > > + * The class needs to be constructed with a reference to the ControlIdMap all\n> > > + * the ControlId belongs to. The ControlIdMap is used to provide, in addition to\n> >\n> > s/belongs/belong/\n> >\n> > > + * the features of the standard unsorted map, access to the mapped elements\n> > > + * using numerical ID keys.\n> >\n> > I'd like to put a stronger emphasis on the fact that all the entries\n> > have to belong to the same map:\n> >\n> > * The class is constructed with a reference to a ControlIdMap. This allows\n> > * providing access to the mapped elements using numerical ID keys, in addition\n> > * to the features of the standard unsorted map. All ControlId keys in the map\n> > * must appear in the ControlIdMap.\n> >\n> > (Not sure I'm 100% happy with the result)\n> \n> I would s/must appear/must belong/ but I got the point\n> \n> >\n> > > */\n> > >\n> > > /**\n> > > @@ -645,24 +645,27 @@ 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 init)\n> > > -\t: Map(init)\n> > > +ControlInfoMap::ControlInfoMap(std::initializer_list init,\n> > > +\t\t\t const ControlIdMap &idmap)\n> > > +\t: Map(init), idmap_(&idmap)\n> > > {\n> > > -\tgenerateIdmap();\n> > > +\tvalidate();\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> > > +\tvalidate();\n> > > }\n> > >\n> > > /**\n> > > @@ -672,32 +675,14 @@ 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 init)\n> > > +void ControlInfoMap::validate()\n> > > {\n> > > -\tMap::operator=(init);\n> > > -\tgenerateIdmap();\n> > > -\treturn *this;\n> > > -}\n> > > +\tfor (const auto &controlInfo : *this) {\n> > > +\t\tconst ControlId *id = controlInfo.first;\n> > > +\t\tauto it = idmap_->find(id->id());\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> > > +\t\tASSERT(it != idmap_->end());\n> > > +\t}\n> >\n> > You also need to check that it->second == id, as the numerical IDs could\n> > be the same, but the ControlId different.\n> \n> I'll test and see what happens when the IPA is isolated, as the\n> deserializer creates new ControlIds for v4l2 controls so maybe the\n> check would fail...\n> \n> >\n> > To compile the code out in non-debug builds, we could move the assert\n> > out of the loop:\n> >\n> > \tASSERT(!std::count_if(begin(), end(),\n> > \t\t\t [&](const Map::value_type &item) {\n> > \t\t\t\t const ControlId *id = item.first;\n> > \t\t\t\t const auto it = idmap_->find(id->id());\n> > \t\t\t\t return it == idmap_->end() || it->second != id;\n> > \t\t\t }));\n> >\n> > Or maybe\n> >\n> > \tASSERT(validate());\n> >\n> > in the callers would be cleaner :-)\n> \n> Yes, that's probably better, I started with that but then changed it\n> but it was probably better to have a bool validate() and assert in the\n> caller.\n> \n> >\n> > > }\n> > >\n> > > /**\n> > > @@ -707,7 +692,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> > > @@ -717,7 +702,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> > > @@ -732,7 +717,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> > > @@ -743,8 +728,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> > > @@ -758,8 +743,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> > > @@ -776,33 +761,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> > Thanks for adding the validate function, but I think you forgot to\n> > include this check in there :-)\n> \n> Do we still need to validate the type in your opinion ? when using\n> globally defined control id maps (controls and properties) the control\n> info are created by the pipeline handler, when using v4l2 controls the\n> control info type is retrieved from the v4l2 controls.. it's hard to\n> get it wrong, but checking doesn't hurt I guess...\n\nGood point. If you think it's pointless, we can drop it.\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 19cb5c4ec9c3..9c23788e5231 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1071,7 +1071,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(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..bf7e34e28af3 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> > > -\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> > > +static const ControlInfoMap Controls = ControlInfoMap({\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::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > +\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> > > +\t}, controls::controls);\n> > >\n> > > namespace libcamera {\n> > >","headers":{"Return-Path":"","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 F1C62C3240\n\tfor ;\n\tFri, 13 Aug 2021 00:30:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A269D6888A;\n\tFri, 13 Aug 2021 02:30:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F33D868826\n\tfor ;\n\tFri, 13 Aug 2021 02:30:42 +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 5F512EE;\n\tFri, 13 Aug 2021 02:30:42 +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=\"iLGCM9an\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628814642;\n\tbh=n/Q8pueefwRsECUSOqFdHr48ler8znZVNIezKGKW+zw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iLGCM9anHegMJp9NL1g5/+tGQt95TJOP8V59kM123MKkzcFIFw+U/PuTdn17l2oCt\n\tA9je1e4uRg3b5gFE5pA0ZbVqbB2VS+OuwU+PVzJHQAQF5gGEUGHbL+mOUHcxVxmdta\n\tyBrTyyH0A++xsAEq17q+weBvvKv85mlFKsOORDA4=","Date":"Fri, 13 Aug 2021 03:30:37 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210809152308.31947-1-jacopo@jmondi.org>\n\t<20210809152308.31947-2-jacopo@jmondi.org>\n\t\n\t<20210810074457.zsj2s6oehr33r42f@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210810074457.zsj2s6oehr33r42f@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]