[{"id":29884,"web_url":"https://patchwork.libcamera.org/comment/29884/","msgid":"","date":"2024-06-13T07:30:55","subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jun 13, 2024 at 04:39:36AM +0300, Laurent Pinchart wrote:\n> Implement a specialization of the YamlObject::Getter structure to\n> support deserializing ipa::Vector objects from YAML data.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/libipa/vector.cpp | 17 +++++++++++++++++\n> src/ipa/libipa/vector.h | 25 +++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+)\n> \n> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> index 5de4ae48b419..4e987d82fa70 100644\n> --- a/src/ipa/libipa/vector.cpp\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -148,6 +148,23 @@ namespace ipa {\n> * \\return True if the two vectors are not equal, false otherwise\n> */\n> \n> +#ifndef __DOXYGEN__\n> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)\n> +{\n> +\tif (!obj.isList())\n> +\t\treturn false;\n> +\n> +\tif (obj.size() != size) {\n> +\t\tLOG(Vector, Error)\n> +\t\t\t<< \"Wrong number of values in YAML vector: expected \"\n> +\t\t\t<< size << \", got \" << obj.size();\n> +\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> } /* namespace ipa */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> index 7c444363d4bb..4b2fe581ecc2 100644\n> --- a/src/ipa/libipa/vector.h\n> +++ b/src/ipa/libipa/vector.h\n> @@ -180,6 +180,10 @@ bool operator!=(const Vector &lhs, const Vector &rhs)\n> \treturn !(lhs == rhs);\n> }\n> \n> +#ifndef __DOXYGEN__\n> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);\n> +#endif /* __DOXYGEN__ */\n> +\n> } /* namespace ipa */\n> \n> #ifndef __DOXYGEN__\n> @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector &v)\n> \n> \treturn out;\n> }\n> +\n> +template\n> +struct YamlObject::Getter> {\n> +\tstd::optional> get(const YamlObject &obj) const\n> +\t{\n> +\t\tif (!ipa::vectorValidateYaml(obj, Rows))\n> +\t\t\treturn std::nullopt;\n> +\n> +\t\tipa::Vector vector;\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const YamlObject &entry : obj.asList()) {\n> +\t\t\tconst auto value = entry.get();\n> +\t\t\tif (!value)\n> +\t\t\t\treturn std::nullopt;\n> +\t\t\tvector[i++] = *value;\n> +\t\t}\n> +\n> +\t\treturn vector;\n> +\t}\n> +};\n> #endif /* __DOXYGEN__ */\n> \n> } /* namespace libcamera */","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 C5749BD87C\n\tfor ;\n\tThu, 13 Jun 2024 07:31:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 105D46548C;\n\tThu, 13 Jun 2024 09:31:04 +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 031646548B\n\tfor ;\n\tThu, 13 Jun 2024 09:31:01 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C10DD4CF;\n\tThu, 13 Jun 2024 09:30:46 +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=\"RgvuMMYo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718263848;\n\tbh=XzDcfx3aEBsBkYWh9Avk1IOYx3PKOvZ6qlLmhKFJDkk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RgvuMMYo04qS5gZMb4+K8qUIKMG7gXGSfqUTOS9MKbNH5uTn6LoRxwzib6k7pNxXG\n\tjKQlvf5dVDp5rqXkDL/mTnkS2ju+RwcX6e8dxX/JHAX7Dnj9P9BG6UOge0fUTdfjvt\n\tu9wnopXUPDfpZGvEwMDoJ2cWIAkMhU6azhBXj934=","Date":"Thu, 13 Jun 2024 16:30:55 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tNaushir Patuck ","Subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","Message-ID":"","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240613013944.23344-4-laurent.pinchart@ideasonboard.com>","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":29894,"web_url":"https://patchwork.libcamera.org/comment/29894/","msgid":"<171826644330.2248009.6633859831051482881@ping.linuxembedded.co.uk>","date":"2024-06-13T08:14:03","subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-13 02:39:36)\n> Implement a specialization of the YamlObject::Getter structure to\n> support deserializing ipa::Vector objects from YAML data.\n\nDiving to the how / why the preceeding patches are used before those\nonese themselves :D\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/libipa/vector.cpp | 17 +++++++++++++++++\n> src/ipa/libipa/vector.h | 25 +++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+)\n> \n> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> index 5de4ae48b419..4e987d82fa70 100644\n> --- a/src/ipa/libipa/vector.cpp\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -148,6 +148,23 @@ namespace ipa {\n> * \\return True if the two vectors are not equal, false otherwise\n> */\n> \n> +#ifndef __DOXYGEN__\n> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)\n> +{\n> + if (!obj.isList())\n> + return false;\n> +\n> + if (obj.size() != size) {\n> + LOG(Vector, Error)\n> + << \"Wrong number of values in YAML vector: expected \"\n> + << size << \", got \" << obj.size();\n> + return false;\n> + }\n> +\n> + return true;\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> } /* namespace ipa */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> index 7c444363d4bb..4b2fe581ecc2 100644\n> --- a/src/ipa/libipa/vector.h\n> +++ b/src/ipa/libipa/vector.h\n> @@ -180,6 +180,10 @@ bool operator!=(const Vector &lhs, const Vector &rhs)\n> return !(lhs == rhs);\n> }\n> \n> +#ifndef __DOXYGEN__\n> +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);\n> +#endif /* __DOXYGEN__ */\n> +\n> } /* namespace ipa */\n> \n> #ifndef __DOXYGEN__\n> @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector &v)\n> \n> return out;\n> }\n> +\n> +template\n> +struct YamlObject::Getter> {\n> + std::optional> get(const YamlObject &obj) const\n\nOk - so the design is to keep the gets that mix the two types\n(YamlObject, Vector) outside of both of those classes, by implementing a\nGetter which yaml_parser.h std::optional get() will be able to map to\nthrough it's \"return Getter{}.get(*this);\"\n\nSounds good to me if it's working.\n\n> + {\n> + if (!ipa::vectorValidateYaml(obj, Rows))\n> + return std::nullopt;\n> +\n> + ipa::Vector vector;\n> +\n> + unsigned int i = 0;\n> + for (const YamlObject &entry : obj.asList()) {\n> + const auto value = entry.get();\n> + if (!value)\n> + return std::nullopt;\n\nShould we be LOGging an error here? This /shouldn't/ happen - I think it\ncan only happen if someone puts the wrong type into the Yaml object? But\nthat would be easier to identify/debug with a print... which should be\nfairly low cost to add in here right?\n\nOf course there's no error prints on the base type getters either - but\nas this is a specialisation - maybe it's more 'custom' and could warrant\nmore verbosity?\n\nAnyway, either way:\n\n\nReviewed-by: Kieran Bingham \n\n> + vector[i++] = *value;\n> + }\n> +\n> + return vector;\n> + }\n> +};\n> #endif /* __DOXYGEN__ */\n> \n> } /* namespace libcamera */\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 E1483BD87C\n\tfor ;\n\tThu, 13 Jun 2024 08:14:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A05E65495;\n\tThu, 13 Jun 2024 10:14:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99EEE6548C\n\tfor ;\n\tThu, 13 Jun 2024 10:14:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0341BEB;\n\tThu, 13 Jun 2024 10:13:51 +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=\"W+ThMPy8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718266431;\n\tbh=obtkllckIY1A2bSrTEaVhwgJ9TBxEwngkkjG2VC45Wc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=W+ThMPy8rwtuSzYaHV4nf7qhtkMpQuSl26tW5lhGIQfYyEIhjypO+83NLrE51qkno\n\tLy6ra1LwvNz9iuo0jk2bTuoR/L6XIWB2WTwYivsjwftxACzi/drUi2ZHxclR5E3h5n\n\tsr44q2Ay1XfKahtr3MslRUdIoK+GPsaAgJOg2pF4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240613013944.23344-4-laurent.pinchart@ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tDavid Plowman ,\n\tNaushir Patuck ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 13 Jun 2024 09:14:03 +0100","Message-ID":"<171826644330.2248009.6633859831051482881@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29903,"web_url":"https://patchwork.libcamera.org/comment/29903/","msgid":"<20240613101920.GA6019@pendragon.ideasonboard.com>","date":"2024-06-13T10:19:20","subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jun 13, 2024 at 09:14:03AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-13 02:39:36)\n> > Implement a specialization of the YamlObject::Getter structure to\n> > support deserializing ipa::Vector objects from YAML data.\n> \n> Diving to the how / why the preceeding patches are used before those\n> onese themselves :D\n\nGood idea :-) That's the most important part.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/libipa/vector.cpp | 17 +++++++++++++++++\n> > src/ipa/libipa/vector.h | 25 +++++++++++++++++++++++++\n> > 2 files changed, 42 insertions(+)\n> > \n> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> > index 5de4ae48b419..4e987d82fa70 100644\n> > --- a/src/ipa/libipa/vector.cpp\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -148,6 +148,23 @@ namespace ipa {\n> > * \\return True if the two vectors are not equal, false otherwise\n> > */\n> > \n> > +#ifndef __DOXYGEN__\n> > +bool vectorValidateYaml(const YamlObject &obj, unsigned int size)\n> > +{\n> > + if (!obj.isList())\n> > + return false;\n> > +\n> > + if (obj.size() != size) {\n> > + LOG(Vector, Error)\n> > + << \"Wrong number of values in YAML vector: expected \"\n> > + << size << \", got \" << obj.size();\n> > + return false;\n> > + }\n> > +\n> > + return true;\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > } /* namespace ipa */\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> > index 7c444363d4bb..4b2fe581ecc2 100644\n> > --- a/src/ipa/libipa/vector.h\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -180,6 +180,10 @@ bool operator!=(const Vector &lhs, const Vector &rhs)\n> > return !(lhs == rhs);\n> > }\n> > \n> > +#ifndef __DOXYGEN__\n> > +bool vectorValidateYaml(const YamlObject &obj, unsigned int size);\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > } /* namespace ipa */\n> > \n> > #ifndef __DOXYGEN__\n> > @@ -195,6 +199,27 @@ std::ostream &operator<<(std::ostream &out, const ipa::Vector &v)\n> > \n> > return out;\n> > }\n> > +\n> > +template\n> > +struct YamlObject::Getter> {\n> > + std::optional> get(const YamlObject &obj) const\n> \n> Ok - so the design is to keep the gets that mix the two types\n> (YamlObject, Vector) outside of both of those classes, by implementing a\n> Getter which yaml_parser.h std::optional get() will be able to map to\n> through it's \"return Getter{}.get(*this);\"\n\nYes that's the idea. If the type you want to deserialize is a type\ntemplate, you need to do it as in this patch, with a specialization of\nthe Getter structure. If the type is not a type template, then you can\ndo as in patch 09/11, skipping the structure and just write\n\ntemplate<>\nstd::optional\nYamlObject::Getter::get(const YamlObject &obj) const\n{\n\t...\n}\n\nIt's the same mechanism in both cases, with a small syntax shortcut for\nnon-template types.\n\nFor the users, nothing changes, it's YamlObject::get() in all cases.\n\n> Sounds good to me if it's working.\n\nAs far as I can tell it is :-)\n\n> > + {\n> > + if (!ipa::vectorValidateYaml(obj, Rows))\n> > + return std::nullopt;\n> > +\n> > + ipa::Vector vector;\n> > +\n> > + unsigned int i = 0;\n> > + for (const YamlObject &entry : obj.asList()) {\n> > + const auto value = entry.get();\n> > + if (!value)\n> > + return std::nullopt;\n> \n> Should we be LOGging an error here? This /shouldn't/ happen - I think it\n> can only happen if someone puts the wrong type into the Yaml object? But\n> that would be easier to identify/debug with a print... which should be\n> fairly low cost to add in here right?\n> \n> Of course there's no error prints on the base type getters either - but\n> as this is a specialisation - maybe it's more 'custom' and could warrant\n> more verbosity?\n\nI've avoided a print here, as this function is defined inline, so I\nwanted to avoid code bloat. Any message we would print here would only\nbe able to tell that there was an issue reading *a* vector. I think a\nmessage in the caller would be better, to indicate what failed.\n\n> Anyway, either way:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > + vector[i++] = *value;\n> > + }\n> > +\n> > + return vector;\n> > + }\n> > +};\n> > #endif /* __DOXYGEN__ */\n> > \n> > } /* namespace libcamera */","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 BF63FBD87C\n\tfor ;\n\tThu, 13 Jun 2024 10:19:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 037B8634D3;\n\tThu, 13 Jun 2024 12:19:43 +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 9EE0C634D3\n\tfor ;\n\tThu, 13 Jun 2024 12:19:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54E8A4CC;\n\tThu, 13 Jun 2024 12:19:27 +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=\"I11AEs6o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718273967;\n\tbh=ZnK0sNv61RzLQLP5swM/w0emMGMceqhYNzvawAYcJYU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I11AEs6oEFLmM92oaHtVtyuk1Uvd32LKT+PIrApDo+zUpRyfszQ6g+7YsQQu6qH2J\n\tgJqRPXX3U5H829XTOd4fc3JsMWelYIE7sbN7/1D1M+UxLJSp9zWZAr2pZUpyPpMILe\n\t350KY20Dq3YrBwO0RbAz/Bg9eB/0vkeHSZfQL6I8=","Date":"Thu, 13 Jun 2024 13:19:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tDavid Plowman ,\n\tNaushir Patuck ","Subject":"Re: [PATCH 03/11] ipa: libipa: vector: Specialize YamlObject getter","Message-ID":"<20240613101920.GA6019@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-4-laurent.pinchart@ideasonboard.com>\n\t<171826644330.2248009.6633859831051482881@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171826644330.2248009.6633859831051482881@ping.linuxembedded.co.uk>","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\" "}}]