[{"id":19846,"web_url":"https://patchwork.libcamera.org/comment/19846/","msgid":"","date":"2021-09-26T19:56:52","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: control_serializer:\n\tUse the right idmap","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 Fri, Sep 24, 2021 at 07:25:23PM +0200, Jacopo Mondi wrote:\n> When a ControlList is deserialized, the code searches for a valid\n> ControlInfoMap in the local cache and use its id map to initialize the\n> list. If no valid ControlInfoMap is found, as it's usually the case\n> for lists transporting libcamera controls and properties, the globally\n> defined controls::controls id map is used unconditionally.\n> \n> This breaks the deserialization of libcamera properties, for which a\n> wrong idmap is used at construction time.\n> \n> As the serialization header now transports an id_map_type field, store\n> the idmap type at serialization time, and re-use it at\n> deserialization time to identify the correct id map.\n> \n> Also make the validation stricter by imposing to list of V4L2 controls to\n> have an associated ControlInfoMap available, as there is no globally\n> defined idmap for such controls.\n> \n> To be able to retrieve the idmap associated with a ControlList, add an\n> accessor function to the ControlList class.\n> \n> It might be worth in future using a ControlInfoMap to initialize the\n> deserialized ControlList to implement controls validation against their\n> limit. As such validation is not implemented at the moment, maintain the\n> current behaviour and initialize the control list with an idmap.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n> Reviewed-by: Kieran Bingham \n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/control_serializer.cpp | 51 +++++++++++++++++++++++-----\n> src/libcamera/controls.cpp | 8 +++++\n> 3 files changed, 51 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index b3590fdb93ec..af851b4661c8 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -407,6 +407,7 @@ public:\n> \tvoid set(unsigned int id, const ControlValue &value);\n> \n> \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> +\tconst ControlIdMap *idMap() const { return idmap_; }\n> \n> private:\n> \tconst ControlValue *find(unsigned int id) const;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 27360526f9eb..d42840cddecb 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> \t\tinfoMapHandle = 0;\n> \t}\n> \n> +\tconst ControlIdMap *idmap = list.idMap();\n> +\tenum ipa_controls_id_map_type idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> +\n> \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> \tsize_t valuesSize = 0;\n> \tfor (const auto &ctrl : list)\n> @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> \thdr.entries = list.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.id_map_type = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -496,13 +506,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t}\n> \n> \t/*\n> -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> -\t * currently the case for ControlList related to libcamera controls),\n> -\t * use the global control::control idmap.\n> +\t * Retrieve the ControlIdMap associated with the ControlList.\n> +\t *\n> +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> +\t * a valid handle has been initialized at serialization time, or by\n> +\t * using the header's id_map_type field for lists that refer to the\n> +\t * globally defined libcamera controls and properties, for which no\n> +\t * ControlInfoMap is available.\n> \t */\n> -\tconst ControlInfoMap *infoMap;\n> +\tconst ControlIdMap *idMap;\n> \tif (hdr->handle) {\n> \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> @@ -514,12 +526,33 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tinfoMap = iter->first;\n> +\t\tconst ControlInfoMap *infoMap = iter->first;\n> +\t\tidMap = &infoMap->idmap();\n> +\t\tASSERT(idMap);\n\nA reference can't be null, that would be an undefined behaviour. The\ncompiler can skip the assert here. If you're concerned that\nControlInfoMap::idmap_ may be null when ControlInfoMap::idmap() is\ncalled, the assert should go to ControlInfoMap::idmap().\n\n> \t} else {\n> -\t\tinfoMap = nullptr;\n> +\t\tswitch (hdr->id_map_type) {\n> +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> +\t\t\tidMap = &controls::controls;\n> +\t\t\tbreak;\n> +\n> +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> +\t\t\tidMap = &properties::properties;\n> +\t\t\tbreak;\n> +\n> +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> +\t\t\tLOG(Serializer, Fatal)\n> +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> +\t\t\treturn {};\n> +\t\t}\n> \t}\n> \n> -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> +\t/*\n> +\t * \\todo When available, initialize the list with the ControlInfoMap\n> +\t * so that controls can be validated against their limits.\n> +\t * Currently no validation is performed, so it's fine relying on the\n> +\t * idmap only.\n> +\t */\n> +\tControlList ctrls(*idMap);\n> \n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_value_entry *entry =\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 55eec71ffe35..d23eff9e2728 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -1040,6 +1040,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> * associated ControlInfoMap, nullptr is returned in that case.\n> */\n> \n> +/**\n> + * \\fn ControlList::idMap()\n> + * \\brief Retrieve the ControlId map used to construct the ControlList\n> + * \\return The ControlId map used to construct the ControlList. ControlsList\n\ns/ControlsList/ControlList/\n\n> + * instances constructed with ControlList() have no associated idmap, nullptr is\n\ns/ControlList()/the default constructor/\n\nReviewed-by: Laurent Pinchart \n\n> + * returned in that case.\n> + */\n> +\n> const ControlValue *ControlList::find(unsigned int id) const\n> {\n> \tconst auto iter = controls_.find(id);","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 4B60BBDC71\n\tfor ;\n\tSun, 26 Sep 2021 19:57:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78C6C6918E;\n\tSun, 26 Sep 2021 21:57:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 823B96012D\n\tfor ;\n\tSun, 26 Sep 2021 21:56:58 +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 09E58EF;\n\tSun, 26 Sep 2021 21:56:57 +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=\"kF9FEZ/a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632686218;\n\tbh=TM5Uxga4zZLzbxiHAelEzXF9WfpM/VuzLIylGyRLayo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kF9FEZ/afsu1osbfR8UaHw2oEi1/D5S+x75HggFBlBLsOe1hb3h1q+hb2jQoQRNCc\n\txpdYMt7nwTSbDujeqPJKYGF+iLiZxLncjAFXCf1xdHhseJfX4ouza8QXdz5s2fEhK+\n\tlkkIlaHqEp2Uk4DRSLwnJht0Tae6Zff47JdGJp6U=","Date":"Sun, 26 Sep 2021 22:56:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210924172525.160482-1-jacopo@jmondi.org>\n\t<20210924172525.160482-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210924172525.160482-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: control_serializer:\n\tUse the right idmap","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":19881,"web_url":"https://patchwork.libcamera.org/comment/19881/","msgid":"<20210927123534.bxm6u26hdhz6mzpt@uno.localdomain>","date":"2021-09-27T12:35:34","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: control_serializer:\n\tUse the right idmap","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Sep 26, 2021 at 10:56:52PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 24, 2021 at 07:25:23PM +0200, Jacopo Mondi wrote:\n> > When a ControlList is deserialized, the code searches for a valid\n> > ControlInfoMap in the local cache and use its id map to initialize the\n> > list. If no valid ControlInfoMap is found, as it's usually the case\n> > for lists transporting libcamera controls and properties, the globally\n> > defined controls::controls id map is used unconditionally.\n> >\n> > This breaks the deserialization of libcamera properties, for which a\n> > wrong idmap is used at construction time.\n> >\n> > As the serialization header now transports an id_map_type field, store\n> > the idmap type at serialization time, and re-use it at\n> > deserialization time to identify the correct id map.\n> >\n> > Also make the validation stricter by imposing to list of V4L2 controls to\n> > have an associated ControlInfoMap available, as there is no globally\n> > defined idmap for such controls.\n> >\n> > To be able to retrieve the idmap associated with a ControlList, add an\n> > accessor function to the ControlList class.\n> >\n> > It might be worth in future using a ControlInfoMap to initialize the\n> > deserialized ControlList to implement controls validation against their\n> > limit. As such validation is not implemented at the moment, maintain the\n> > current behaviour and initialize the control list with an idmap.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Paul Elder \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > include/libcamera/controls.h | 1 +\n> > src/libcamera/control_serializer.cpp | 51 +++++++++++++++++++++++-----\n> > src/libcamera/controls.cpp | 8 +++++\n> > 3 files changed, 51 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index b3590fdb93ec..af851b4661c8 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -407,6 +407,7 @@ public:\n> > \tvoid set(unsigned int id, const ControlValue &value);\n> >\n> > \tconst ControlInfoMap *infoMap() const { return infoMap_; }\n> > +\tconst ControlIdMap *idMap() const { return idmap_; }\n> >\n> > private:\n> > \tconst ControlValue *find(unsigned int id) const;\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 27360526f9eb..d42840cddecb 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list,\n> > \t\tinfoMapHandle = 0;\n> > \t}\n> >\n> > +\tconst ControlIdMap *idmap = list.idMap();\n> > +\tenum ipa_controls_id_map_type idMapType;\n> > +\tif (idmap == &controls::controls)\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > +\telse if (idmap == &properties::properties)\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > +\telse\n> > +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > +\n> > \tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > \tsize_t valuesSize = 0;\n> > \tfor (const auto &ctrl : list)\n> > @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > \thdr.entries = list.size();\n> > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > +\thdr.id_map_type = idMapType;\n> >\n> > \tbuffer.write(&hdr);\n> >\n> > @@ -496,13 +506,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > \t}\n> >\n> > \t/*\n> > -\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > -\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > -\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > -\t * currently the case for ControlList related to libcamera controls),\n> > -\t * use the global control::control idmap.\n> > +\t * Retrieve the ControlIdMap associated with the ControlList.\n> > +\t *\n> > +\t * The idmap is either retrieved from the list's ControlInfoMap when\n> > +\t * a valid handle has been initialized at serialization time, or by\n> > +\t * using the header's id_map_type field for lists that refer to the\n> > +\t * globally defined libcamera controls and properties, for which no\n> > +\t * ControlInfoMap is available.\n> > \t */\n> > -\tconst ControlInfoMap *infoMap;\n> > +\tconst ControlIdMap *idMap;\n> > \tif (hdr->handle) {\n> > \t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > \t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > @@ -514,12 +526,33 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > \t\t\treturn {};\n> > \t\t}\n> >\n> > -\t\tinfoMap = iter->first;\n> > +\t\tconst ControlInfoMap *infoMap = iter->first;\n> > +\t\tidMap = &infoMap->idmap();\n> > +\t\tASSERT(idMap);\n>\n> A reference can't be null, that would be an undefined behaviour. The\n\nAh, right, we return a reference by dereferencing the idmap_ pointer\nunconditionally in ControlInfoMap\n\n\tconst ControlIdMap &idmap() const { return *idmap_; }\n\nIf idmap_ happens to be nullptr we'll have issues and the above ASSERT\nclearly can't help. I'll drop it, thanks for spotting!\n\n\n\n> compiler can skip the assert here. If you're concerned that\n> ControlInfoMap::idmap_ may be null when ControlInfoMap::idmap() is\n> called, the assert should go to ControlInfoMap::idmap().\n>\n> > \t} else {\n> > -\t\tinfoMap = nullptr;\n> > +\t\tswitch (hdr->id_map_type) {\n> > +\t\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> > +\t\t\tidMap = &controls::controls;\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> > +\t\t\tidMap = &properties::properties;\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase IPA_CONTROL_ID_MAP_V4L2:\n> > +\t\t\tLOG(Serializer, Fatal)\n> > +\t\t\t\t<< \"A list of V4L2 controls requires an ControlInfoMap\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > \t}\n> >\n> > -\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > +\t/*\n> > +\t * \\todo When available, initialize the list with the ControlInfoMap\n> > +\t * so that controls can be validated against their limits.\n> > +\t * Currently no validation is performed, so it's fine relying on the\n> > +\t * idmap only.\n> > +\t */\n> > +\tControlList ctrls(*idMap);\n> >\n> > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > \t\tconst struct ipa_control_value_entry *entry =\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 55eec71ffe35..d23eff9e2728 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -1040,6 +1040,14 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > * associated ControlInfoMap, nullptr is returned in that case.\n> > */\n> >\n> > +/**\n> > + * \\fn ControlList::idMap()\n> > + * \\brief Retrieve the ControlId map used to construct the ControlList\n> > + * \\return The ControlId map used to construct the ControlList. ControlsList\n>\n> s/ControlsList/ControlList/\n>\n> > + * instances constructed with ControlList() have no associated idmap, nullptr is\n>\n> s/ControlList()/the default constructor/\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + * returned in that case.\n> > + */\n> > +\n> > const ControlValue *ControlList::find(unsigned int id) const\n> > {\n> > \tconst auto iter = controls_.find(id);\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 7FAE6BDC71\n\tfor ;\n\tMon, 27 Sep 2021 12:34:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B00E69191;\n\tMon, 27 Sep 2021 14:34:49 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E63D6012C\n\tfor ;\n\tMon, 27 Sep 2021 14:34:48 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 9B68F1C000A;\n\tMon, 27 Sep 2021 12:34:47 +0000 (UTC)"],"Date":"Mon, 27 Sep 2021 14:35:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210927123534.bxm6u26hdhz6mzpt@uno.localdomain>","References":"<20210924172525.160482-1-jacopo@jmondi.org>\n\t<20210924172525.160482-4-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 3/5] libcamera: control_serializer:\n\tUse the right idmap","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\" "}}]