[{"id":19297,"web_url":"https://patchwork.libcamera.org/comment/19297/","msgid":"<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>","date":"2021-09-02T15:00:13","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 01/09/2021 15:37, Jacopo Mondi wrote:\n> The ControlList class has a pair of accessor functions with a similar\n> signature:\n> \n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> \tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> As ControlList::infoMap_ can be nullptr, the two functions returns the\n> class members as pointers and not references.\n> \n> The ControlInfoMap class has instead:\n> \n> \tconst ControlIdMap &idmap() const { return *idmap_; }\n> \n> Which is disturbingly different in the signature and return type.\n> \n> Rationalize the accessor function names by stabilizing on:\n> \n> \tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 2 +-\n> src/libcamera/camera_sensor.cpp | 2 +-\n> src/libcamera/control_serializer.cpp | 4 ++--\n> src/libcamera/controls.cpp | 4 ++--\n> src/libcamera/delayed_controls.cpp | 4 ++--\n> 5 files changed, 8 insertions(+), 8 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 8e5bc8b70b94..de6faf600e19 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -338,7 +338,7 @@ 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> \tbool validate();\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 876685097f76..48af3a617ee1 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()\n> \t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> \t};\n> \n> -\tconst ControlIdMap &controls = subdev_->controls().idmap();\n> +\tconst ControlIdMap &controls = *subdev_->controls().idMap();\n\nAye? Is this legal?\n\nIs that declaring a reference against a dereferenced pointer?\n\nI can't tell if that's making a copy - or ... time for a compile-explorer:\n\n\thttps://godbolt.org/z/j75669Mrf\n\nOk - so it's not making a copy, and it generates identical code to:\n\n\tconst ControlIdMap *controls = subdev_->controls().idMap()\n\nBut without you having to change all the uses of controls to controls->\ninstead of controls. ?\n\nBut given there are only 3 ... ?\n\n\nhttps://isocpp.org/wiki/faq/references#refs-vs-ptrs\n states\n> \"Use references when you can, and pointers when you have to.\"\n\nBut https://isocpp.org/wiki/faq/references#refs-not-null\nmakes it clear:\n\n> It means this is illegal:\n> \n> T* p = nullptr;\n> T& r = *p; // illegal\n\nSo we have to be /really/ sure that idMap() would not return a nullptr,\nor we get into undefined behaviour, and the cpu turns into a black hole.\n\nI suspect a reference use here is dangerous ...\n\n\n\n\n\n> \tfor (uint32_t ctrl : optionalControls) {\n> \t\tif (!controls.count(ctrl))\n> \t\t\tLOG(CameraSensor, Debug)\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index d4377c024079..7eef041a16ee 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \tfor (const auto &ctrl : infoMap)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> \n> -\tconst ControlIdMap *idmap = &infoMap.idmap();\n> +\tconst ControlIdMap *idmap = infoMap.idMap();\n> \tenum ipa_controls_id_map_type idMapType;\n> \tif (idmap == &controls::controls)\n> \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t}\n> \n> \t\tconst ControlInfoMap *infoMap = iter->first;\n> -\t\tidMap = &infoMap->idmap();\n> +\t\tidMap = infoMap->idMap();\n> \t} else {\n> \t\tswitch (hdr->id_map_type) {\n> \t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 96625edb1380..9b9bad212db3 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> }\n> \n> /**\n> - * \\fn const ControlIdMap &ControlInfoMap::idmap() const\n> + * \\fn const ControlIdMap *ControlInfoMap::idMap() const\n> * \\brief Retrieve the ControlId map\n> *\n> * Constructing ControlList instances for V4L2 controls requires a ControlIdMap\n> @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> * \\param[in] validator The validator (may be null)\n> */\n> ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)\n> -\t: validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)\n> +\t: validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)\n> {\n> }\n> \n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 90ce7e0b5b52..763021ef09bb 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -130,7 +130,7 @@ void DelayedControls::reset()\n> \t/* Seed the control queue with the controls reported by the device. */\n> \tvalues_.clear();\n> \tfor (const auto &ctrl : controls) {\n> -\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> +\t\tconst ControlId *id = device_->controls().idMap()->at(ctrl.first);\n> \t\t/*\n> \t\t * Do not mark this control value as updated, it does not need\n> \t\t * to be written to to device on startup.\n> @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)\n> \t}\n> \n> \t/* Update with new controls. */\n> -\tconst ControlIdMap &idmap = device_->controls().idmap();\n> +\tconst ControlIdMap &idmap = *device_->controls().idMap();\n\nAnother location of taking a reference from a pointer.\nI think this is likely to flag up the UBSAN, even if it compiles cleanly.\n\n\n> \tfor (const auto &control : controls) {\n> \t\tconst auto &it = idmap.find(control.first);\n> \t\tif (it == idmap.end()) {\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 9CE55BDC71\n\tfor ;\n\tThu, 2 Sep 2021 15:00:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 071EE6916A;\n\tThu, 2 Sep 2021 17:00:18 +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 009ED60255\n\tfor ;\n\tThu, 2 Sep 2021 17:00:15 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7180B18F9;\n\tThu, 2 Sep 2021 17:00:15 +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=\"mhqYgVWB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630594815;\n\tbh=+NHoSBB7rcbK22gbsedMuq53BzL4St0tiDg2dXPpEuk=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=mhqYgVWBvBFmkg0CyDVQVf5Fjn+pqXzgTofRJE/yHyw5/LL886hJmUJCUMFl+ZPdS\n\tVyM25/0TVaBGJueETfhcgZL3dUd0EJJI8aVlqIWKnrD2vqaJV1Tucb5/+3E1icUu7d\n\tFF+V90QIVjVx13IEu6rRDKsDDta2ywA7gFNwak4k=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-6-jacopo@jmondi.org>","From":"Kieran Bingham ","Message-ID":"<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>","Date":"Thu, 2 Sep 2021 16:00:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210901143800.119118-6-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19343,"web_url":"https://patchwork.libcamera.org/comment/19343/","msgid":"<20210903102429.wsnkulnb23k6h7kz@uno.localdomain>","date":"2021-09-03T10:24:29","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 04:00:13PM +0100, Kieran Bingham wrote:\n> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> > The ControlList class has a pair of accessor functions with a similar\n> > signature:\n> >\n> > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > \tconst ControlIdMap *idMap() const { return idmap_; }\n> >\n> > As ControlList::infoMap_ can be nullptr, the two functions returns the\n> > class members as pointers and not references.\n> >\n> > The ControlInfoMap class has instead:\n> >\n> > \tconst ControlIdMap &idmap() const { return *idmap_; }\n> >\n> > Which is disturbingly different in the signature and return type.\n> >\n> > Rationalize the accessor function names by stabilizing on:\n> >\n> > \tconst ControlIdMap *idMap() const { return idmap_; }\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 2 +-\n> > src/libcamera/camera_sensor.cpp | 2 +-\n> > src/libcamera/control_serializer.cpp | 4 ++--\n> > src/libcamera/controls.cpp | 4 ++--\n> > src/libcamera/delayed_controls.cpp | 4 ++--\n> > 5 files changed, 8 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 8e5bc8b70b94..de6faf600e19 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -338,7 +338,7 @@ 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> > \tbool validate();\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 876685097f76..48af3a617ee1 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()\n> > \t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> > \t};\n> >\n> > -\tconst ControlIdMap &controls = subdev_->controls().idmap();\n> > +\tconst ControlIdMap &controls = *subdev_->controls().idMap();\n>\n> Aye? Is this legal?\n>\n> Is that declaring a reference against a dereferenced pointer?\n>\n> I can't tell if that's making a copy - or ... time for a compile-explorer:\n>\n> \thttps://godbolt.org/z/j75669Mrf\n>\n> Ok - so it's not making a copy, and it generates identical code to:\n>\n> \tconst ControlIdMap *controls = subdev_->controls().idMap()\n\nThanks for checking, this was my expectation\n\n>\n> But without you having to change all the uses of controls to controls->\n> instead of controls. ?\n\nYes, which I wouldn't mind to be honest\n\n>\n> But given there are only 3 ... ?\n>\n>\n> https://isocpp.org/wiki/faq/references#refs-vs-ptrs\n> states\n> > \"Use references when you can, and pointers when you have to.\"\n>\n> But https://isocpp.org/wiki/faq/references#refs-not-null\n> makes it clear:\n>\n> > It means this is illegal:\n> >\n> > T* p = nullptr;\n> > T& r = *p; // illegal\n>\n> So we have to be /really/ sure that idMap() would not return a nullptr,\n> or we get into undefined behaviour, and the cpu turns into a black hole.\n>\n> I suspect a reference use here is dangerous ...\n\nAlso using an unchecked pointer is, as we would immediately dereference\nit causing a segfault.\n\nUnfortunately we need to construct empty ControlInfoMap with idmap_ ==\nnullptr, as there are instances of that class as class members of\nothers, so a default constructor is required.\n\nWe could:\n1) Default construct ControlInfoMap with idmap_ pointing to a known\n valid idmap like controls::controls. We would avoid segfaults or\n undefined behaviour but accessing a default constructed\n ControlInfoMap's idmap would return a reference to a possibly ids\n\n2) Make\n ControlInfoMap::idmap() const\n {\n ASSERT(idmap_);\n return idmap_;\n }\n\nNote that the same problem exists with ControlList as we have the\n\nControlList::ControlList()\n\t: validator_(nullptr), idmap_(nullptr), infoMap_(nullptr)\n{\n}\n\nconstructor and un-guarded accessors\n\n\tconst ControlInfoMap *infoMap() const { return infoMap_; }\n\tconst ControlIdMap *idMap() const { return idmap_; }\n\nso I guess deferencing a nullptr there is theoretically possible.\n\nAlso note that it is possible to construct a ControlList with a\nnullptr infoMap\n\nControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n\t: validator_(validator), idmap_(&idmap), infoMap_(nullptr)\n{\n}\n\nso it's legal to have it as nullptr, and ASSERT might be quite harsh.\n\nActually some parts of our code (serializers in particular) have to\nconstantly deal with the if (!list->infoMap()) case.\n\nI think the fragile part comes from the fact a ControlList might be\nonly optionally associated with a ControlInfoMap and that's not been made\na stricter requirement.\n\nI think we should rather try to impose that, as that would allow to\nimplement controls auto-validation against their limits and if any\npiece of code cretes a ControlList, it should have access to the\ncontrols limits.\n\nThis is what it might look like (on top of this series) making it a\nrequirement to contruct a ControlList with a ControlInfoMap and\nremoving the possibility to contruct it with a ControlIdMap:\n\nhttps://paste.debian.net/1210212/\n\nIn brief:\n- Request now creates controls_ and metadata_ with the\n controls::controls id map\n - Use camera_->controls() as the request has access to it.\n I see no drawbacks, and the validator_ does exactly this to validate\n controls (we can make a ControlList auto-validate if we impose to\n be constructed with a ControlInfoMap)\n\n- IPAs creates control lists with controls::controls id map\n IPA should be in charge of initializaing/updating some of the camera\n controls. They have access to the ControlInfoMap of controls they\n handle, and they can use it to construct ControlList. It might be\n challanging to keep the map in sync between the PH and the IPA\n\n - IPU3 is ok-ish: it initializes ControlsInfoMap of camera controls\n in init(), it does not yet update limits if configure() but has\n all it needs to do so\n\n - RaspberryPi is as usual top-class: it already receives the camera controls\n in configure(). We might want to move the initialization of some\n controls to the IPA in future, but that shouldn't make a\n difference\n\n - RkISP1 needs a lot of love. It does not handle camera controls at\n all atm, but it's trivial to provide them at configure() time as\n RPi does.\n\n- Properties:\n ../src/libcamera/camera_sensor.cpp:58:11: error: no matching function for call to ‘libcamera::ControlList::ControlList(const ControlIdMap&)’\n 58 | properties_(properties::properties)\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\n Properties are expressed as ControlList but there is a non-enforced\n rule that they are not mutable, or better it is only enforced\n towards application by returning properties as const & from the\n Camera class).\n\n For properties it is fair to contruct them with an id map as\n they are read only so creating a ControlInfoMap to associate it to\n a non-modifiable control list doesn't make much sense.\n\n I wonder if properties should not be made a read-only sub-class of\n ControlList and only allow them to be initialized by ControlIdMap.\n\n A possible challange would be that serializing them would require\n more effort, but for now, there doesn't seem a need to serialize\n properties lists between IPA and PH.\n\nAll in all, I think it's worth a try, but I might have missed some\nreasons why I shouldn't go there!\n\nThanks\n j\n\n>\n>\n>\n>\n>\n> > \tfor (uint32_t ctrl : optionalControls) {\n> > \t\tif (!controls.count(ctrl))\n> > \t\t\tLOG(CameraSensor, Debug)\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index d4377c024079..7eef041a16ee 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > \tfor (const auto &ctrl : infoMap)\n> > \t\tvaluesSize += binarySize(ctrl.second);\n> >\n> > -\tconst ControlIdMap *idmap = &infoMap.idmap();\n> > +\tconst ControlIdMap *idmap = infoMap.idMap();\n> > \tenum ipa_controls_id_map_type idMapType;\n> > \tif (idmap == &controls::controls)\n> > \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > \t\t}\n> >\n> > \t\tconst ControlInfoMap *infoMap = iter->first;\n> > -\t\tidMap = &infoMap->idmap();\n> > +\t\tidMap = infoMap->idMap();\n> > \t} else {\n> > \t\tswitch (hdr->id_map_type) {\n> > \t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 96625edb1380..9b9bad212db3 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> > }\n> >\n> > /**\n> > - * \\fn const ControlIdMap &ControlInfoMap::idmap() const\n> > + * \\fn const ControlIdMap *ControlInfoMap::idMap() const\n> > * \\brief Retrieve the ControlId map\n> > *\n> > * Constructing ControlList instances for V4L2 controls requires a ControlIdMap\n> > @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> > * \\param[in] validator The validator (may be null)\n> > */\n> > ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)\n> > -\t: validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)\n> > +\t: validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)\n> > {\n> > }\n> >\n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > index 90ce7e0b5b52..763021ef09bb 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -130,7 +130,7 @@ void DelayedControls::reset()\n> > \t/* Seed the control queue with the controls reported by the device. */\n> > \tvalues_.clear();\n> > \tfor (const auto &ctrl : controls) {\n> > -\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> > +\t\tconst ControlId *id = device_->controls().idMap()->at(ctrl.first);\n> > \t\t/*\n> > \t\t * Do not mark this control value as updated, it does not need\n> > \t\t * to be written to to device on startup.\n> > @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)\n> > \t}\n> >\n> > \t/* Update with new controls. */\n> > -\tconst ControlIdMap &idmap = device_->controls().idmap();\n> > +\tconst ControlIdMap &idmap = *device_->controls().idMap();\n>\n> Another location of taking a reference from a pointer.\n> I think this is likely to flag up the UBSAN, even if it compiles cleanly.\n>\n>\n> > \tfor (const auto &control : controls) {\n> > \t\tconst auto &it = idmap.find(control.first);\n> > \t\tif (it == idmap.end()) {\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 D9CBBBD87D\n\tfor ;\n\tFri, 3 Sep 2021 10:23:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D3136916A;\n\tFri, 3 Sep 2021 12:23:43 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 834F169167\n\tfor ;\n\tFri, 3 Sep 2021 12:23:41 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id EACE2240015;\n\tFri, 3 Sep 2021 10:23:40 +0000 (UTC)"],"Date":"Fri, 3 Sep 2021 12:24:29 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210903102429.wsnkulnb23k6h7kz@uno.localdomain>","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-6-jacopo@jmondi.org>\n\t<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","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":19355,"web_url":"https://patchwork.libcamera.org/comment/19355/","msgid":"<4ff40a94-a22e-c313-c9aa-959c55476d8e@ideasonboard.com>","date":"2021-09-03T11:14:40","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 03/09/2021 11:24, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Thu, Sep 02, 2021 at 04:00:13PM +0100, Kieran Bingham wrote:\n>> On 01/09/2021 15:37, Jacopo Mondi wrote:\n>>> The ControlList class has a pair of accessor functions with a similar\n>>> signature:\n>>>\n>>> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n>>> \tconst ControlIdMap *idMap() const { return idmap_; }\n>>>\n>>> As ControlList::infoMap_ can be nullptr, the two functions returns the\n>>> class members as pointers and not references.\n>>>\n>>> The ControlInfoMap class has instead:\n>>>\n>>> \tconst ControlIdMap &idmap() const { return *idmap_; }\n>>>\n>>> Which is disturbingly different in the signature and return type.\n>>>\n>>> Rationalize the accessor function names by stabilizing on:\n>>>\n>>> \tconst ControlIdMap *idMap() const { return idmap_; }\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> include/libcamera/controls.h | 2 +-\n>>> src/libcamera/camera_sensor.cpp | 2 +-\n>>> src/libcamera/control_serializer.cpp | 4 ++--\n>>> src/libcamera/controls.cpp | 4 ++--\n>>> src/libcamera/delayed_controls.cpp | 4 ++--\n>>> 5 files changed, 8 insertions(+), 8 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> index 8e5bc8b70b94..de6faf600e19 100644\n>>> --- a/include/libcamera/controls.h\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -338,7 +338,7 @@ 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>>> \tbool validate();\n>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>>> index 876685097f76..48af3a617ee1 100644\n>>> --- a/src/libcamera/camera_sensor.cpp\n>>> +++ b/src/libcamera/camera_sensor.cpp\n>>> @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()\n>>> \t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n>>> \t};\n>>>\n>>> -\tconst ControlIdMap &controls = subdev_->controls().idmap();\n>>> +\tconst ControlIdMap &controls = *subdev_->controls().idMap();\n>>\n>> Aye? Is this legal?\n>>\n>> Is that declaring a reference against a dereferenced pointer?\n>>\n>> I can't tell if that's making a copy - or ... time for a compile-explorer:\n>>\n>> \thttps://godbolt.org/z/j75669Mrf\n>>\n>> Ok - so it's not making a copy, and it generates identical code to:\n>>\n>> \tconst ControlIdMap *controls = subdev_->controls().idMap()\n> \n> Thanks for checking, this was my expectation\n> \n>>\n>> But without you having to change all the uses of controls to controls->\n>> instead of controls. ?\n> \n> Yes, which I wouldn't mind to be honest\n> \n>>\n>> But given there are only 3 ... ?\n>>\n>>\n>> https://isocpp.org/wiki/faq/references#refs-vs-ptrs\n>> states\n>>> \"Use references when you can, and pointers when you have to.\"\n>>\n>> But https://isocpp.org/wiki/faq/references#refs-not-null\n>> makes it clear:\n>>\n>>> It means this is illegal:\n>>>\n>>> T* p = nullptr;\n>>> T& r = *p; // illegal\n>>\n>> So we have to be /really/ sure that idMap() would not return a nullptr,\n>> or we get into undefined behaviour, and the cpu turns into a black hole.\n>>\n>> I suspect a reference use here is dangerous ...\n> \n> Also using an unchecked pointer is, as we would immediately dereference\n> it causing a segfault.\n\n\nYes, but at least that is defined behaviour, vs undefined with a route\nto dereferencing a null-reference.\n\nIf the compiler spots that could happen it might simply decide to\noptimise out the whole function to a return 0; (or ...something worse\nperhaps).\n\n\n> Unfortunately we need to construct empty ControlInfoMap with idmap_ ==\n> nullptr, as there are instances of that class as class members of\n> others, so a default constructor is required.\n> \n> We could:\n> 1) Default construct ControlInfoMap with idmap_ pointing to a known\n> valid idmap like controls::controls. We would avoid segfaults or\n> undefined behaviour but accessing a default constructed\n> ControlInfoMap's idmap would return a reference to a possibly ids\n\nI think this might just hide bugs, or cause them indeed.\n\n\n> \n> 2) Make\n> ControlInfoMap::idmap() const\n> {\n> ASSERT(idmap_);\n\nI think that might be a good idea anyway...\n\n> return idmap_;\n> }\n> \n> Note that the same problem exists with ControlList as we have the\n> \n> ControlList::ControlList()\n> \t: validator_(nullptr), idmap_(nullptr), infoMap_(nullptr)\n> {\n> }\n> \n> constructor and un-guarded accessors\n> \n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> \tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> so I guess deferencing a nullptr there is theoretically possible.\n\n\nThat's ok, The distinction is ...\n\n - Dereferencing a null pointer is defined behaviour.\n We know it will segfault.\n\n - Making a reference from a pointer which could potentially be null\n (not that /is/ null) could be optimised out by the compiler.\n\n\n> Also note that it is possible to construct a ControlList with a\n> nullptr infoMap\n> \n> ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> \t: validator_(validator), idmap_(&idmap), infoMap_(nullptr)\n\nOk, that's very bad.\n\nIf we can call\n\n ControlList(nullptr, nullptr);\n\non that constructor (ControlIdMap &idmap) - Then, I believe the compiler\ncan do anything it likes. Including not construct (or allocate memory?)\nat all.\n\n\n> {\n> }\n> \n> so it's legal to have it as nullptr, and ASSERT might be quite harsh.\n\n\nIf we can choose to pass in a nullptr here, then it's important that\nit's not taken as a reference.\n\n\n\n> Actually some parts of our code (serializers in particular) have to\n> constantly deal with the if (!list->infoMap()) case.\n> \n> I think the fragile part comes from the fact a ControlList might be\n> only optionally associated with a ControlInfoMap and that's not been made\n> a stricter requirement.\n> \n> I think we should rather try to impose that, as that would allow to\n> implement controls auto-validation against their limits and if any\n> piece of code cretes a ControlList, it should have access to the\n> controls limits.\n\n\nOk - this is a slightly separate thing, so just to make sure I'm clear,\nI am opposed to anything where we pass a pointer and store it as a\nreference.\n\nBut\n - I'm ok updating to only use pointers as pointers or\n - I'm ok updating the code to only pass references into references ;-)\n\n\n> This is what it might look like (on top of this series) making it a\n> requirement to contruct a ControlList with a ControlInfoMap and\n> removing the possibility to contruct it with a ControlIdMap:\n> \n> https://paste.debian.net/1210212/\n> \n> In brief:\n> - Request now creates controls_ and metadata_ with the\n> controls::controls id map\n> - Use camera_->controls() as the request has access to it.\n> I see no drawbacks, and the validator_ does exactly this to validate\n> controls (we can make a ControlList auto-validate if we impose to\n> be constructed with a ControlInfoMap)\n> \n> - IPAs creates control lists with controls::controls id map\n> IPA should be in charge of initializaing/updating some of the camera\n> controls. They have access to the ControlInfoMap of controls they\n> handle, and they can use it to construct ControlList. It might be\n> challanging to keep the map in sync between the PH and the IPA\n> \n> - IPU3 is ok-ish: it initializes ControlsInfoMap of camera controls\n> in init(), it does not yet update limits if configure() but has\n> all it needs to do so\n> \n> - RaspberryPi is as usual top-class: it already receives the camera controls\n> in configure(). We might want to move the initialization of some\n> controls to the IPA in future, but that shouldn't make a\n> difference\n> \n> - RkISP1 needs a lot of love. It does not handle camera controls at\n> all atm, but it's trivial to provide them at configure() time as\n> RPi does.\n> \n> - Properties:\n> ../src/libcamera/camera_sensor.cpp:58:11: error: no matching function for call to ‘libcamera::ControlList::ControlList(const ControlIdMap&)’\n> 58 | properties_(properties::properties)\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> Properties are expressed as ControlList but there is a non-enforced\n> rule that they are not mutable, or better it is only enforced\n> towards application by returning properties as const & from the\n> Camera class).\n> \n> For properties it is fair to contruct them with an id map as\n> they are read only so creating a ControlInfoMap to associate it to\n> a non-modifiable control list doesn't make much sense.\n> \n> I wonder if properties should not be made a read-only sub-class of\n> ControlList and only allow them to be initialized by ControlIdMap.\n> \n> A possible challange would be that serializing them would require\n> more effort, but for now, there doesn't seem a need to serialize\n> properties lists between IPA and PH.\n> \n> All in all, I think it's worth a try, but I might have missed some\n> reasons why I shouldn't go there!\n> \n> Thanks\n> j\n> \n>>\n>>\n>>\n>>\n>>\n>>> \tfor (uint32_t ctrl : optionalControls) {\n>>> \t\tif (!controls.count(ctrl))\n>>> \t\t\tLOG(CameraSensor, Debug)\n>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>>> index d4377c024079..7eef041a16ee 100644\n>>> --- a/src/libcamera/control_serializer.cpp\n>>> +++ b/src/libcamera/control_serializer.cpp\n>>> @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n>>> \tfor (const auto &ctrl : infoMap)\n>>> \t\tvaluesSize += binarySize(ctrl.second);\n>>>\n>>> -\tconst ControlIdMap *idmap = &infoMap.idmap();\n>>> +\tconst ControlIdMap *idmap = infoMap.idMap();\n>>> \tenum ipa_controls_id_map_type idMapType;\n>>> \tif (idmap == &controls::controls)\n>>> \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n>>> @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>> \t\t}\n>>>\n>>> \t\tconst ControlInfoMap *infoMap = iter->first;\n>>> -\t\tidMap = &infoMap->idmap();\n>>> +\t\tidMap = infoMap->idMap();\n>>> \t} else {\n>>> \t\tswitch (hdr->id_map_type) {\n>>> \t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>> index 96625edb1380..9b9bad212db3 100644\n>>> --- a/src/libcamera/controls.cpp\n>>> +++ b/src/libcamera/controls.cpp\n>>> @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n>>> }\n>>>\n>>> /**\n>>> - * \\fn const ControlIdMap &ControlInfoMap::idmap() const\n>>> + * \\fn const ControlIdMap *ControlInfoMap::idMap() const\n>>> * \\brief Retrieve the ControlId map\n>>> *\n>>> * Constructing ControlList instances for V4L2 controls requires a ControlIdMap\n>>> @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n>>> * \\param[in] validator The validator (may be null)\n>>> */\n>>> ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)\n>>> -\t: validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)\n>>> +\t: validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)\n>>> {\n>>> }\n>>>\n>>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n>>> index 90ce7e0b5b52..763021ef09bb 100644\n>>> --- a/src/libcamera/delayed_controls.cpp\n>>> +++ b/src/libcamera/delayed_controls.cpp\n>>> @@ -130,7 +130,7 @@ void DelayedControls::reset()\n>>> \t/* Seed the control queue with the controls reported by the device. */\n>>> \tvalues_.clear();\n>>> \tfor (const auto &ctrl : controls) {\n>>> -\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n>>> +\t\tconst ControlId *id = device_->controls().idMap()->at(ctrl.first);\n>>> \t\t/*\n>>> \t\t * Do not mark this control value as updated, it does not need\n>>> \t\t * to be written to to device on startup.\n>>> @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)\n>>> \t}\n>>>\n>>> \t/* Update with new controls. */\n>>> -\tconst ControlIdMap &idmap = device_->controls().idmap();\n>>> +\tconst ControlIdMap &idmap = *device_->controls().idMap();\n>>\n>> Another location of taking a reference from a pointer.\n>> I think this is likely to flag up the UBSAN, even if it compiles cleanly.\n>>\n>>\n>>> \tfor (const auto &control : controls) {\n>>> \t\tconst auto &it = idmap.find(control.first);\n>>> \t\tif (it == idmap.end()) {\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 8BA4BBD87D\n\tfor ;\n\tFri, 3 Sep 2021 11:14:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE34A6916A;\n\tFri, 3 Sep 2021 13:14:44 +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 411C169167\n\tfor ;\n\tFri, 3 Sep 2021 13:14:43 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4777BBE;\n\tFri, 3 Sep 2021 13:14: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=\"HMAC/4I7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630667682;\n\tbh=E/xzFkXHWvYBAwVDbz/rL+HX45b5vLx38LvD4CjrCuM=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=HMAC/4I7pxAIvp1ayQY3+N4fVQJIrJmgMhkzr55/BpOnTQFh+I6/fJALbHeuAzpxB\n\tSZe3eqMCmCQw07xd55PzottfgMC6KYiWxpuLvJ+psAOa4tPSCkIGpGJt1977x13n1C\n\tujQf7i2dqy4CG+B/dBAKQa0Dz5CTR+XaDzJu/GzY=","From":"Kieran Bingham ","To":"Jacopo Mondi ","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-6-jacopo@jmondi.org>\n\t<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>\n\t<20210903102429.wsnkulnb23k6h7kz@uno.localdomain>","Message-ID":"<4ff40a94-a22e-c313-c9aa-959c55476d8e@ideasonboard.com>","Date":"Fri, 3 Sep 2021 12:14:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210903102429.wsnkulnb23k6h7kz@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","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":19371,"web_url":"https://patchwork.libcamera.org/comment/19371/","msgid":"","date":"2021-09-03T12:38:28","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Sep 03, 2021 at 12:14:40PM +0100, Kieran Bingham wrote:\n> On 03/09/2021 11:24, Jacopo Mondi wrote:\n> > On Thu, Sep 02, 2021 at 04:00:13PM +0100, Kieran Bingham wrote:\n> >> On 01/09/2021 15:37, Jacopo Mondi wrote:\n> >>> The ControlList class has a pair of accessor functions with a similar\n> >>> signature:\n> >>>\n> >>> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> >>> \tconst ControlIdMap *idMap() const { return idmap_; }\n> >>>\n> >>> As ControlList::infoMap_ can be nullptr, the two functions returns the\n> >>> class members as pointers and not references.\n> >>>\n> >>> The ControlInfoMap class has instead:\n> >>>\n> >>> \tconst ControlIdMap &idmap() const { return *idmap_; }\n> >>>\n> >>> Which is disturbingly different in the signature and return type.\n> >>>\n> >>> Rationalize the accessor function names by stabilizing on:\n> >>>\n> >>> \tconst ControlIdMap *idMap() const { return idmap_; }\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> include/libcamera/controls.h | 2 +-\n> >>> src/libcamera/camera_sensor.cpp | 2 +-\n> >>> src/libcamera/control_serializer.cpp | 4 ++--\n> >>> src/libcamera/controls.cpp | 4 ++--\n> >>> src/libcamera/delayed_controls.cpp | 4 ++--\n> >>> 5 files changed, 8 insertions(+), 8 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>> index 8e5bc8b70b94..de6faf600e19 100644\n> >>> --- a/include/libcamera/controls.h\n> >>> +++ b/include/libcamera/controls.h\n> >>> @@ -338,7 +338,7 @@ 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> >>> \tbool validate();\n> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>> index 876685097f76..48af3a617ee1 100644\n> >>> --- a/src/libcamera/camera_sensor.cpp\n> >>> +++ b/src/libcamera/camera_sensor.cpp\n> >>> @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()\n> >>> \t\tV4L2_CID_CAMERA_SENSOR_ROTATION,\n> >>> \t};\n> >>>\n> >>> -\tconst ControlIdMap &controls = subdev_->controls().idmap();\n> >>> +\tconst ControlIdMap &controls = *subdev_->controls().idMap();\n> >>\n> >> Aye? Is this legal?\n> >>\n> >> Is that declaring a reference against a dereferenced pointer?\n> >>\n> >> I can't tell if that's making a copy - or ... time for a compile-explorer:\n> >>\n> >> \thttps://godbolt.org/z/j75669Mrf\n> >>\n> >> Ok - so it's not making a copy, and it generates identical code to:\n> >>\n> >> \tconst ControlIdMap *controls = subdev_->controls().idMap()\n> > \n> > Thanks for checking, this was my expectation\n> >\n> >> But without you having to change all the uses of controls to controls->\n> >> instead of controls. ?\n> > \n> > Yes, which I wouldn't mind to be honest\n> > \n> >> But given there are only 3 ... ?\n> >>\n> >> https://isocpp.org/wiki/faq/references#refs-vs-ptrs\n> >> states\n> >>> \"Use references when you can, and pointers when you have to.\"\n> >>\n> >> But https://isocpp.org/wiki/faq/references#refs-not-null\n> >> makes it clear:\n> >>\n> >>> It means this is illegal:\n> >>>\n> >>> T* p = nullptr;\n> >>> T& r = *p; // illegal\n> >>\n> >> So we have to be /really/ sure that idMap() would not return a nullptr,\n> >> or we get into undefined behaviour, and the cpu turns into a black hole.\n> >>\n> >> I suspect a reference use here is dangerous ...\n> > \n> > Also using an unchecked pointer is, as we would immediately dereference\n> > it causing a segfault.\n> \n> Yes, but at least that is defined behaviour, vs undefined with a route\n> to dereferencing a null-reference.\n\nDereferencing a null pointer is also undefined behaviour. To be\npedantic, dereferencing a null pointer is allowed, but using the\nresulting value is undefined behaviour:\n\n\tchar *p = nullptr;\n\tsizeof(*p);\t\t// valid\n\t*p;\t\t\t// valid\n\t*p = 0;\t\t\t// invalid, undefined behaviour\n\n> If the compiler spots that could happen it might simply decide to\n> optimise out the whole function to a return 0; (or ...something worse\n> perhaps).\n\nIf we can guarantee that the return value of idMap() will never be null\nhere, I think it's fine. What would definitely not be fine is\n\n\n\tControlIdMap &controls = *subdev_->controls().idMap();\n\n\t... code that doesn't use controls ...\n\n\tif (&controls == nullptr)\n\t\t...\n\nThe compiler could drop the check, or do something even worse.\n\nCreating a reference from a pointer essentially binds it to the object\nobtained by dereferencing the pointer, which is considered as using the\nvalue, and is thus undefined behaviour if the pointer is null. In the\nexample above, it's the first line than is considered undefined\nbehaviour if the pointer can be null.\n\n> > Unfortunately we need to construct empty ControlInfoMap with idmap_ ==\n> > nullptr, as there are instances of that class as class members of\n> > others, so a default constructor is required.\n\nIf we can guarantee that nobody call ControlInfoMap::idmap() on such an\ninstance that has a null idmap_, then there's no undefined behaviour. It\ndoesn't meant it's a good API though.\n\nMaybe we could modify the classes that embed an instance of\nControlInfoMap to allocate it dynamically instead ?\n\n> > We could:\n> > 1) Default construct ControlInfoMap with idmap_ pointing to a known\n> > valid idmap like controls::controls. We would avoid segfaults or\n> > undefined behaviour but accessing a default constructed\n> > ControlInfoMap's idmap would return a reference to a possibly ids\n> \n> I think this might just hide bugs, or cause them indeed.\n\nAgreed, I don't like this much.\n\n> > 2) Make\n> > ControlInfoMap::idmap() const\n> > {\n> > ASSERT(idmap_);\n> \n> I think that might be a good idea anyway...\n> \n> > return idmap_;\n> > }\n> > \n> > Note that the same problem exists with ControlList as we have the\n> > \n> > ControlList::ControlList()\n> > \t: validator_(nullptr), idmap_(nullptr), infoMap_(nullptr)\n> > {\n> > }\n> > \n> > constructor and un-guarded accessors\n> > \n> > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > \tconst ControlIdMap *idMap() const { return idmap_; }\n> > \n> > so I guess deferencing a nullptr there is theoretically possible.\n> \n> That's ok, The distinction is ...\n> \n> - Dereferencing a null pointer is defined behaviour.\n\nNope, see above :-)\n\n> We know it will segfault.\n\nMost likely, but if the compiler can determine the pointer is null, it\ncan do something else. In practice the range of \"something else\" is\nlimited, even if it's allowed by the standard, I don't think any\ncompiler will compile undefined behaviour to 'rm -rf /' (except perhaps\nin Intercal).\n\n> - Making a reference from a pointer which could potentially be null\n> (not that /is/ null) could be optimised out by the compiler.\n\nIt's not that it's optimized out, it's an undefined behaviour.\n\n> > Also note that it is possible to construct a ControlList with a\n> > nullptr infoMap\n> > \n> > ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> > \t: validator_(validator), idmap_(&idmap), infoMap_(nullptr)\n> \n> Ok, that's very bad.\n> \n> If we can call\n> \n> ControlList(nullptr, nullptr);\n> \n> on that constructor (ControlIdMap &idmap) - Then, I believe the compiler\n> can do anything it likes. Including not construct (or allocate memory?)\n> at all.\n> \n> > {\n> > }\n> > \n> > so it's legal to have it as nullptr, and ASSERT might be quite harsh.\n> \n> If we can choose to pass in a nullptr here, then it's important that\n> it's not taken as a reference.\n> \n> > Actually some parts of our code (serializers in particular) have to\n> > constantly deal with the if (!list->infoMap()) case.\n> > \n> > I think the fragile part comes from the fact a ControlList might be\n> > only optionally associated with a ControlInfoMap and that's not been made\n> > a stricter requirement.\n> > \n> > I think we should rather try to impose that, as that would allow to\n> > implement controls auto-validation against their limits and if any\n> > piece of code cretes a ControlList, it should have access to the\n> > controls limits.\n> \n> Ok - this is a slightly separate thing, so just to make sure I'm clear,\n> I am opposed to anything where we pass a pointer and store it as a\n> reference.\n> \n> But\n> - I'm ok updating to only use pointers as pointers or\n> - I'm ok updating the code to only pass references into references ;-)\n> \n> > This is what it might look like (on top of this series) making it a\n> > requirement to contruct a ControlList with a ControlInfoMap and\n> > removing the possibility to contruct it with a ControlIdMap:\n> > \n> > https://paste.debian.net/1210212/\n\nDoesn't look that bad, but we may need to dynamically allocated the\nControlInfoMap stored in IPAIPU3 and IPARkISP1 to drop the default\nconstructor for that class.\n\n> > In brief:\n> > - Request now creates controls_ and metadata_ with the\n> > controls::controls id map\n> > - Use camera_->controls() as the request has access to it.\n> > I see no drawbacks, and the validator_ does exactly this to validate\n> > controls (we can make a ControlList auto-validate if we impose to\n> > be constructed with a ControlInfoMap)\n\nIt's better than no drawbacks I think, it restricts the controls that\ncan be stored in the control list to what the camera supports, so it\nseems to be an improvement. There's a small voice in my head that warns\nme of a possible overdesign here though, but so far it looks good.\n\n> > - IPAs creates control lists with controls::controls id map\n> > IPA should be in charge of initializaing/updating some of the camera\n> > controls. They have access to the ControlInfoMap of controls they\n> > handle, and they can use it to construct ControlList. It might be\n> > challanging to keep the map in sync between the PH and the IPA\n> > \n> > - IPU3 is ok-ish: it initializes ControlsInfoMap of camera controls\n> > in init(), it does not yet update limits if configure() but has\n> > all it needs to do so\n> > \n> > - RaspberryPi is as usual top-class: it already receives the camera controls\n> > in configure(). We might want to move the initialization of some\n> > controls to the IPA in future, but that shouldn't make a\n> > difference\n\nThe ControlInfoMap for the RaspberryPi IPA is statically defined in a\nheader file, which is actually not good, it should be fixed to create it\ndynamically at runtime instead.\n\nI'm starting to wonder if the need to dynamically change min/max/def\nvalues in the ControlInfoMap at configure time will not require much\nmore work than I initially envisioned, especially if we decide to allow\nconfigure() to remove/add supported controls. I think we should minimize\nthe cleanups to ControlInfoMap in this series, to avoid doing work that\nmay get thrown away (due to a full refactoring) later.\n\n> > - RkISP1 needs a lot of love. It does not handle camera controls at\n> > all atm, but it's trivial to provide them at configure() time as\n> > RPi does.\n> > \n> > - Properties:\n> > ../src/libcamera/camera_sensor.cpp:58:11: error: no matching function for call to ‘libcamera::ControlList::ControlList(const ControlIdMap&)’\n> > 58 | properties_(properties::properties)\n> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > \n> > Properties are expressed as ControlList but there is a non-enforced\n> > rule that they are not mutable, or better it is only enforced\n> > towards application by returning properties as const & from the\n> > Camera class).\n> > \n> > For properties it is fair to contruct them with an id map as\n> > they are read only so creating a ControlInfoMap to associate it to\n> > a non-modifiable control list doesn't make much sense.\n> > \n> > I wonder if properties should not be made a read-only sub-class of\n> > ControlList and only allow them to be initialized by ControlIdMap.\n> > \n> > A possible challange would be that serializing them would require\n> > more effort, but for now, there doesn't seem a need to serialize\n> > properties lists between IPA and PH.\n> > \n> > All in all, I think it's worth a try, but I might have missed some\n> > reasons why I shouldn't go there!\n> > \n> >>> \tfor (uint32_t ctrl : optionalControls) {\n> >>> \t\tif (!controls.count(ctrl))\n> >>> \t\t\tLOG(CameraSensor, Debug)\n> >>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >>> index d4377c024079..7eef041a16ee 100644\n> >>> --- a/src/libcamera/control_serializer.cpp\n> >>> +++ b/src/libcamera/control_serializer.cpp\n> >>> @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> >>> \tfor (const auto &ctrl : infoMap)\n> >>> \t\tvaluesSize += binarySize(ctrl.second);\n> >>>\n> >>> -\tconst ControlIdMap *idmap = &infoMap.idmap();\n> >>> +\tconst ControlIdMap *idmap = infoMap.idMap();\n> >>> \tenum ipa_controls_id_map_type idMapType;\n> >>> \tif (idmap == &controls::controls)\n> >>> \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> >>> @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>> \t\t}\n> >>>\n> >>> \t\tconst ControlInfoMap *infoMap = iter->first;\n> >>> -\t\tidMap = &infoMap->idmap();\n> >>> +\t\tidMap = infoMap->idMap();\n> >>> \t} else {\n> >>> \t\tswitch (hdr->id_map_type) {\n> >>> \t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>> index 96625edb1380..9b9bad212db3 100644\n> >>> --- a/src/libcamera/controls.cpp\n> >>> +++ b/src/libcamera/controls.cpp\n> >>> @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const\n> >>> }\n> >>>\n> >>> /**\n> >>> - * \\fn const ControlIdMap &ControlInfoMap::idmap() const\n> >>> + * \\fn const ControlIdMap *ControlInfoMap::idMap() const\n> >>> * \\brief Retrieve the ControlId map\n> >>> *\n> >>> * Constructing ControlList instances for V4L2 controls requires a ControlIdMap\n> >>> @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> >>> * \\param[in] validator The validator (may be null)\n> >>> */\n> >>> ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)\n> >>> -\t: validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)\n> >>> +\t: validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)\n> >>> {\n> >>> }\n> >>>\n> >>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> >>> index 90ce7e0b5b52..763021ef09bb 100644\n> >>> --- a/src/libcamera/delayed_controls.cpp\n> >>> +++ b/src/libcamera/delayed_controls.cpp\n> >>> @@ -130,7 +130,7 @@ void DelayedControls::reset()\n> >>> \t/* Seed the control queue with the controls reported by the device. */\n> >>> \tvalues_.clear();\n> >>> \tfor (const auto &ctrl : controls) {\n> >>> -\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> >>> +\t\tconst ControlId *id = device_->controls().idMap()->at(ctrl.first);\n> >>> \t\t/*\n> >>> \t\t * Do not mark this control value as updated, it does not need\n> >>> \t\t * to be written to to device on startup.\n> >>> @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)\n> >>> \t}\n> >>>\n> >>> \t/* Update with new controls. */\n> >>> -\tconst ControlIdMap &idmap = device_->controls().idmap();\n> >>> +\tconst ControlIdMap &idmap = *device_->controls().idMap();\n> >>\n> >> Another location of taking a reference from a pointer.\n> >> I think this is likely to flag up the UBSAN, even if it compiles cleanly.\n> >>\n> >>> \tfor (const auto &control : controls) {\n> >>> \t\tconst auto &it = idmap.find(control.first);\n> >>> \t\tif (it == idmap.end()) {\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 58C79BDC71\n\tfor ;\n\tFri, 3 Sep 2021 12:38:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 066C86916A;\n\tFri, 3 Sep 2021 14:38:47 +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 A993E69167\n\tfor ;\n\tFri, 3 Sep 2021 14:38:45 +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 25865BBE;\n\tFri, 3 Sep 2021 14:38:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"s/JOvZxN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630672725;\n\tbh=OWVIQu9JN2vOuhg9JqTwP1iOm8giIMQSJ3JIXcq86ks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s/JOvZxNuepPXkH0g2wyX1WMF2H0d+1j7hURVQpY8kejxA35M6h388aDVMjpWzhji\n\trBsZz51eAHBQKxoMU58DVRZIt01gTMhsXSVuULLNMPeg644En3gXtXJQww9ZOtH9uc\n\tC9c6MLSqmpCfOskqkseTJ+b3Ay8GeCkDk3/uPagc=","Date":"Fri, 3 Sep 2021 15:38:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210901143800.119118-1-jacopo@jmondi.org>\n\t<20210901143800.119118-6-jacopo@jmondi.org>\n\t<5d6cf09b-28a4-2b6f-c26d-33c4a3ac032a@ideasonboard.com>\n\t<20210903102429.wsnkulnb23k6h7kz@uno.localdomain>\n\t<4ff40a94-a22e-c313-c9aa-959c55476d8e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4ff40a94-a22e-c313-c9aa-959c55476d8e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize\n\tidMap() function","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\" "}}]