[{"id":22974,"web_url":"https://patchwork.libcamera.org/comment/22974/","msgid":"","date":"2022-05-17T08:37:49","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:\n> Add libcamera's geometry classes to the Python bindings.\n> \n> Note that this commit only adds the classes, but they are not used\n> anywhere yet.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 1 +\n> src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++\n> src/py/libcamera/pymain.cpp | 2 +\n> 3 files changed, 107 insertions(+)\n> create mode 100644 src/py/libcamera/pygeometry.cpp\n> \n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index af8ba6a5..12e006e7 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> \n> pycamera_sources = files([\n> 'pyenums.cpp',\n> + 'pygeometry.cpp',\n> 'pymain.cpp',\n> ])\n> \n> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp\n> new file mode 100644\n> index 00000000..2cd1432e\n> --- /dev/null\n> +++ b/src/py/libcamera/pygeometry.cpp\n> @@ -0,0 +1,104 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Tomi Valkeinen \n> + *\n> + * Python bindings - Geometry classes\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace py = pybind11;\n> +\n> +using namespace libcamera;\n> +\n> +void init_py_geometry(py::module &m)\n> +{\n> +\tpy::class_(m, \"Point\")\n> +\t\t.def(py::init<>())\n> +\t\t.def(py::init())\n> +\t\t.def_readwrite(\"x\", &Point::x)\n> +\t\t.def_readwrite(\"y\", &Point::y)\n> +\t\t.def(py::self == py::self)\n> +\t\t.def(-py::self)\n> +\t\t.def(\"__str__\", &Point::toString)\n> +\t\t.def(\"__repr__\", [](const Point &self) {\n> +\t\t\treturn \"libcamera.Point(\" + std::to_string(self.x) + \", \" + std::to_string(self.y) + \")\";\n> +\t\t});\n> +\n> +\tpy::class_(m, \"Size\")\n> +\t\t.def(py::init<>())\n> +\t\t.def(py::init())\n> +\t\t.def_readwrite(\"width\", &Size::width)\n> +\t\t.def_readwrite(\"height\", &Size::height)\n> +\t\t.def_property_readonly(\"is_null\", &Size::isNull)\n> +\t\t.def(py::self == py::self)\n> +\t\t.def(py::self * float())\n> +\t\t.def(py::self / float())\n> +\t\t.def(py::self *= float())\n> +\t\t.def(py::self /= float())\n> +\t\t.def(\"__str__\", &Size::toString)\n> +\t\t.def(\"__repr__\", [](const Size &self) {\n> +\t\t\treturn \"libcamera.Size(\" + std::to_string(self.width) + \", \" + std::to_string(self.height) + \")\";\n> +\t\t});\n> +\n> +\tpy::class_(m, \"SizeRange\")\n> +\t\t.def(py::init<>())\n> +\t\t.def(py::init())\n> +\t\t.def(py::init())\n> +\t\t.def(py::init())\n> +\t\t.def(py::init<>([](const std::array &s) {\n> +\t\t\treturn SizeRange(Size(std::get<0>(s), std::get<1>(s)));\n> +\t\t}))\n> +\t\t.def(py::init<>([](const std::array &min, const std::array &max) {\n> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)));\n> +\t\t}))\n> +\t\t.def(py::init<>([](const std::array &min, const std::array &max,\n> +\t\t\t\t unsigned int hStep, unsigned int vStep) {\n> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)),\n> +\t\t\t\t\t hStep, vStep);\n> +\t\t}))\n> +\t\t.def_readwrite(\"min\", &SizeRange::min)\n> +\t\t.def_readwrite(\"max\", &SizeRange::max)\n> +\t\t.def_readwrite(\"hStep\", &SizeRange::hStep)\n> +\t\t.def_readwrite(\"vStep\", &SizeRange::vStep)\n> +\t\t.def(py::self == py::self)\n> +\t\t.def(\"__str__\", &SizeRange::toString)\n> +\t\t.def(\"__repr__\", [](const SizeRange &self) {\n> +\t\t\treturn py::str(\"libcamera.SizeRange(({}, {}), ({}, {}), {}, {})\")\n> +\t\t\t\t.format(self.min.width, self.min.height,\n> +\t\t\t\t\tself.max.width, self.max.height,\n> +\t\t\t\t\tself.hStep, self.vStep);\n> +\t\t});\n> +\n> +\tpy::class_(m, \"Rectangle\")\n> +\t\t.def(py::init<>())\n> +\t\t.def(py::init())\n> +\t\t.def(py::init<>([](int x, int y, const std::array &s) {\n> +\t\t\treturn Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> +\t\t}))\n\nI'm puzzled a bit by this constructor. Where do you use it, and could\nthen next constructor be used instead ? If it's meant to cover a common\nuse case, should the C++ API also implement this ?\n\n> +\t\t.def(py::init())\n> +\t\t.def(py::init())\n> +\t\t.def_readwrite(\"x\", &Rectangle::x)\n> +\t\t.def_readwrite(\"y\", &Rectangle::y)\n> +\t\t.def_readwrite(\"width\", &Rectangle::width)\n> +\t\t.def_readwrite(\"height\", &Rectangle::height)\n> +\t\t.def_property_readonly(\"is_null\", &Rectangle::isNull)\n> +\t\t.def_property_readonly(\"center\", &Rectangle::center)\n> +\t\t.def_property_readonly(\"size\", &Rectangle::size)\n> +\t\t.def_property_readonly(\"topLeft\", &Rectangle::topLeft)\n> +\t\t.def(py::self == py::self)\n> +\t\t.def(\"__str__\", &Rectangle::toString)\n> +\t\t.def(\"__repr__\", [](const Rectangle &self) {\n> +\t\t\treturn py::str(\"libcamera.Rectangle({}, {}, {}, {})\")\n> +\t\t\t\t.format(self.x, self.y, self.width, self.height);\n> +\t\t});\n\nCould you add todo comments to remind that the other members of the\ngeometry classes should be implemented too ?\n\n> +}\n> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n> index cc2ddee5..1dd70d9c 100644\n> --- a/src/py/libcamera/pymain.cpp\n> +++ b/src/py/libcamera/pymain.cpp\n> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)\n> \n> void init_pyenums(py::module &m);\n> void init_pyenums_generated(py::module &m);\n> +void init_py_geometry(py::module &m);\n> \n> PYBIND11_MODULE(_libcamera, m)\n> {\n> \tinit_pyenums(m);\n> \tinit_pyenums_generated(m);\n> +\tinit_py_geometry(m);\n\nMy OCD kicks in :-)\n\n> \n> \t/* Forward declarations */","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 0A0A2C3256\n\tfor ;\n\tTue, 17 May 2022 08:37:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6471365656;\n\tTue, 17 May 2022 10:37:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C45C360420\n\tfor ;\n\tTue, 17 May 2022 10:37:56 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0AD2F48F;\n\tTue, 17 May 2022 10:37:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652776678;\n\tbh=Hw6hEBS5d0BUVf/PDVbcXT6XAsVZTYFseRb7GTjaR9o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QFX8D8FqC8UuqxDdQJ6ZFWyjBYHZy0gZLJPwGxJeHi2v8CtQQdHzL9asfP4oTeCY7\n\tdli24YSRogzbjtjtuyJb7hDY3wuZmZxlBU5oRP7FDuyEgW5XFUaL3uVkOv2koS7vTN\n\tAuN+QDj/FeVqgtgaDtR88siRwf6Kpy2e8aeleEAL0WrWWCPV4RRb7gq80hXfGKJw5G\n\t5Nho0FMxVUN7abngkdAn7lYKlVA+Pciuse11U/wmWIMwtRDVexcd6lUAhJXVGYQCfi\n\tGKxjGmEl3KMph/xK5WmKR2ptg6Qkyve/TrEBbA/02A7GpjLs5u8lE71pInt8lKmoH8\n\tjh9HfwKuDXkPQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652776676;\n\tbh=Hw6hEBS5d0BUVf/PDVbcXT6XAsVZTYFseRb7GTjaR9o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HxuFesWmZU7fJQq7HWPlpt7ALIg0mqCvD05HveSKj5aMEGyogKL1kpy5r24D2Gv3R\n\t5OoI8R94T9HtOEPuTE/hSnJjjqL66B/4rhpEn3YuNVklLD+NVLraH5pxXpFh7xc8fS\n\t+aAh49zqsaP7y3g2H8FYVy/60igafKkODohrRpW4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HxuFesWm\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 11:37:49 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22983,"web_url":"https://patchwork.libcamera.org/comment/22983/","msgid":"","date":"2022-05-17T09:02:13","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 17/05/2022 11:37, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:\n>> Add libcamera's geometry classes to the Python bindings.\n>>\n>> Note that this commit only adds the classes, but they are not used\n>> anywhere yet.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/meson.build | 1 +\n>> src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++\n>> src/py/libcamera/pymain.cpp | 2 +\n>> 3 files changed, 107 insertions(+)\n>> create mode 100644 src/py/libcamera/pygeometry.cpp\n>>\n>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n>> index af8ba6a5..12e006e7 100644\n>> --- a/src/py/libcamera/meson.build\n>> +++ b/src/py/libcamera/meson.build\n>> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>> \n>> pycamera_sources = files([\n>> 'pyenums.cpp',\n>> + 'pygeometry.cpp',\n>> 'pymain.cpp',\n>> ])\n>> \n>> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp\n>> new file mode 100644\n>> index 00000000..2cd1432e\n>> --- /dev/null\n>> +++ b/src/py/libcamera/pygeometry.cpp\n>> @@ -0,0 +1,104 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2022, Tomi Valkeinen \n>> + *\n>> + * Python bindings - Geometry classes\n>> + */\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +namespace py = pybind11;\n>> +\n>> +using namespace libcamera;\n>> +\n>> +void init_py_geometry(py::module &m)\n>> +{\n>> +\tpy::class_(m, \"Point\")\n>> +\t\t.def(py::init<>())\n>> +\t\t.def(py::init())\n>> +\t\t.def_readwrite(\"x\", &Point::x)\n>> +\t\t.def_readwrite(\"y\", &Point::y)\n>> +\t\t.def(py::self == py::self)\n>> +\t\t.def(-py::self)\n>> +\t\t.def(\"__str__\", &Point::toString)\n>> +\t\t.def(\"__repr__\", [](const Point &self) {\n>> +\t\t\treturn \"libcamera.Point(\" + std::to_string(self.x) + \", \" + std::to_string(self.y) + \")\";\n>> +\t\t});\n>> +\n>> +\tpy::class_(m, \"Size\")\n>> +\t\t.def(py::init<>())\n>> +\t\t.def(py::init())\n>> +\t\t.def_readwrite(\"width\", &Size::width)\n>> +\t\t.def_readwrite(\"height\", &Size::height)\n>> +\t\t.def_property_readonly(\"is_null\", &Size::isNull)\n>> +\t\t.def(py::self == py::self)\n>> +\t\t.def(py::self * float())\n>> +\t\t.def(py::self / float())\n>> +\t\t.def(py::self *= float())\n>> +\t\t.def(py::self /= float())\n>> +\t\t.def(\"__str__\", &Size::toString)\n>> +\t\t.def(\"__repr__\", [](const Size &self) {\n>> +\t\t\treturn \"libcamera.Size(\" + std::to_string(self.width) + \", \" + std::to_string(self.height) + \")\";\n>> +\t\t});\n>> +\n>> +\tpy::class_(m, \"SizeRange\")\n>> +\t\t.def(py::init<>())\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init<>([](const std::array &s) {\n>> +\t\t\treturn SizeRange(Size(std::get<0>(s), std::get<1>(s)));\n>> +\t\t}))\n>> +\t\t.def(py::init<>([](const std::array &min, const std::array &max) {\n>> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n>> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)));\n>> +\t\t}))\n>> +\t\t.def(py::init<>([](const std::array &min, const std::array &max,\n>> +\t\t\t\t unsigned int hStep, unsigned int vStep) {\n>> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n>> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)),\n>> +\t\t\t\t\t hStep, vStep);\n>> +\t\t}))\n>> +\t\t.def_readwrite(\"min\", &SizeRange::min)\n>> +\t\t.def_readwrite(\"max\", &SizeRange::max)\n>> +\t\t.def_readwrite(\"hStep\", &SizeRange::hStep)\n>> +\t\t.def_readwrite(\"vStep\", &SizeRange::vStep)\n>> +\t\t.def(py::self == py::self)\n>> +\t\t.def(\"__str__\", &SizeRange::toString)\n>> +\t\t.def(\"__repr__\", [](const SizeRange &self) {\n>> +\t\t\treturn py::str(\"libcamera.SizeRange(({}, {}), ({}, {}), {}, {})\")\n>> +\t\t\t\t.format(self.min.width, self.min.height,\n>> +\t\t\t\t\tself.max.width, self.max.height,\n>> +\t\t\t\t\tself.hStep, self.vStep);\n>> +\t\t});\n>> +\n>> +\tpy::class_(m, \"Rectangle\")\n>> +\t\t.def(py::init<>())\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init<>([](int x, int y, const std::array &s) {\n>> +\t\t\treturn Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n>> +\t\t}))\n> \n> I'm puzzled a bit by this constructor. Where do you use it, and could\n> then next constructor be used instead ? If it's meant to cover a common\n> use case, should the C++ API also implement this ?\n\nIt allows:\n\nlibcam.Rectangle(1, 2, (3, 4))\n\nIt's more obvious with SizeRange:\n\nlibcam.SizeRange((1, 2), (3, 4), 5, 6)\n\ninstead of\n\nlibcam.SizeRange(libcam.Size(1, 2), libcam.Size(3, 4), 5, 6)\n\nI really like tuples in python. I'd also like to do unpacking, like:\n\nw, h = libcam.Size(1, 2)\n\nI think I figured out how to do that, but after sending the series.\n\nThese kind of features are perhaps something we need to do a decision \non: include them, or keep the API as bare-bones as possible.\n\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init())\n>> +\t\t.def_readwrite(\"x\", &Rectangle::x)\n>> +\t\t.def_readwrite(\"y\", &Rectangle::y)\n>> +\t\t.def_readwrite(\"width\", &Rectangle::width)\n>> +\t\t.def_readwrite(\"height\", &Rectangle::height)\n>> +\t\t.def_property_readonly(\"is_null\", &Rectangle::isNull)\n>> +\t\t.def_property_readonly(\"center\", &Rectangle::center)\n>> +\t\t.def_property_readonly(\"size\", &Rectangle::size)\n>> +\t\t.def_property_readonly(\"topLeft\", &Rectangle::topLeft)\n>> +\t\t.def(py::self == py::self)\n>> +\t\t.def(\"__str__\", &Rectangle::toString)\n>> +\t\t.def(\"__repr__\", [](const Rectangle &self) {\n>> +\t\t\treturn py::str(\"libcamera.Rectangle({}, {}, {}, {})\")\n>> +\t\t\t\t.format(self.x, self.y, self.width, self.height);\n>> +\t\t});\n> \n> Could you add todo comments to remind that the other members of the\n> geometry classes should be implemented too ?\n\nYes.\n\n>> +}\n>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n>> index cc2ddee5..1dd70d9c 100644\n>> --- a/src/py/libcamera/pymain.cpp\n>> +++ b/src/py/libcamera/pymain.cpp\n>> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)\n>> \n>> void init_pyenums(py::module &m);\n>> void init_pyenums_generated(py::module &m);\n>> +void init_py_geometry(py::module &m);\n>> \n>> PYBIND11_MODULE(_libcamera, m)\n>> {\n>> \tinit_pyenums(m);\n>> \tinit_pyenums_generated(m);\n>> +\tinit_py_geometry(m);\n> \n> My OCD kicks in :-)\n\nNow that I look at it... Yes...\n\n Tomi","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 117C1C0F2A\n\tfor ;\n\tTue, 17 May 2022 09:02:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7405165656;\n\tTue, 17 May 2022 11:02: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 4D98560420\n\tfor ;\n\tTue, 17 May 2022 11:02:16 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C64E2A5;\n\tTue, 17 May 2022 11:02:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652778138;\n\tbh=MdopVwNtPAvJjREashf9t/aV2AuFF6ngf4hnDNM/Xmc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Q2/xUKOj++BdKozthJXBq7PCMk7FBYfkWn7jXx60RPEnh1Px+RNqbznNsxsrKanqo\n\thFLm9Y7bxCY0E5pti0p23z8wUD57xt3pHpzFuaPy27/i5cSHDFrM1JSTerx+h4cCiZ\n\t7+bLJYzVWwJsEFCIuplVD9wVGmfPAzf+Mc0UdEjaGpNdehFD48lUhfeLXG1x16MkLn\n\tvUdHfjDQ4ZaxxEKC4CAFklq1Q6FO2Chw20AaGgWtVY79UguGVxsIGQpz3kPi3igf5S\n\tOY0Q4y8/W0MHujotS2Y5RiyF80FOLxllxtWjJeO77V6gFY/W39LfEw76LJOt0ZH0eQ\n\tECL+/rH1Imj9A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652778135;\n\tbh=MdopVwNtPAvJjREashf9t/aV2AuFF6ngf4hnDNM/Xmc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=mWs6MUbSyaI9BsTtNBU/3TH11FmP/AWjPW12duix2PFeHBReET2epYq9n2xkRoud9\n\t79PHNDhMICNlTmNq+0mWdydiIaNsjwuh7nIS7Is3mMNmfYoQniTj2Trca0/j5S5JBS\n\tN4cd0fwgxmNhHNSCuxEPdLTXEzwgtaMZ3jQ866pI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mWs6MUbS\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 17 May 2022 12:02:13 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22985,"web_url":"https://patchwork.libcamera.org/comment/22985/","msgid":"","date":"2022-05-17T09:48:13","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Tue, May 17, 2022 at 12:02:13PM +0300, Tomi Valkeinen wrote:\n> On 17/05/2022 11:37, Laurent Pinchart wrote:\n> > On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:\n> >> Add libcamera's geometry classes to the Python bindings.\n> >>\n> >> Note that this commit only adds the classes, but they are not used\n> >> anywhere yet.\n> >>\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/libcamera/meson.build | 1 +\n> >> src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++\n> >> src/py/libcamera/pymain.cpp | 2 +\n> >> 3 files changed, 107 insertions(+)\n> >> create mode 100644 src/py/libcamera/pygeometry.cpp\n> >>\n> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> >> index af8ba6a5..12e006e7 100644\n> >> --- a/src/py/libcamera/meson.build\n> >> +++ b/src/py/libcamera/meson.build\n> >> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> >> \n> >> pycamera_sources = files([\n> >> 'pyenums.cpp',\n> >> + 'pygeometry.cpp',\n> >> 'pymain.cpp',\n> >> ])\n> >> \n> >> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp\n> >> new file mode 100644\n> >> index 00000000..2cd1432e\n> >> --- /dev/null\n> >> +++ b/src/py/libcamera/pygeometry.cpp\n> >> @@ -0,0 +1,104 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2022, Tomi Valkeinen \n> >> + *\n> >> + * Python bindings - Geometry classes\n> >> + */\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +namespace py = pybind11;\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +void init_py_geometry(py::module &m)\n> >> +{\n> >> +\tpy::class_(m, \"Point\")\n> >> +\t\t.def(py::init<>())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def_readwrite(\"x\", &Point::x)\n> >> +\t\t.def_readwrite(\"y\", &Point::y)\n> >> +\t\t.def(py::self == py::self)\n> >> +\t\t.def(-py::self)\n> >> +\t\t.def(\"__str__\", &Point::toString)\n> >> +\t\t.def(\"__repr__\", [](const Point &self) {\n> >> +\t\t\treturn \"libcamera.Point(\" + std::to_string(self.x) + \", \" + std::to_string(self.y) + \")\";\n> >> +\t\t});\n> >> +\n> >> +\tpy::class_(m, \"Size\")\n> >> +\t\t.def(py::init<>())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def_readwrite(\"width\", &Size::width)\n> >> +\t\t.def_readwrite(\"height\", &Size::height)\n> >> +\t\t.def_property_readonly(\"is_null\", &Size::isNull)\n> >> +\t\t.def(py::self == py::self)\n> >> +\t\t.def(py::self * float())\n> >> +\t\t.def(py::self / float())\n> >> +\t\t.def(py::self *= float())\n> >> +\t\t.def(py::self /= float())\n> >> +\t\t.def(\"__str__\", &Size::toString)\n> >> +\t\t.def(\"__repr__\", [](const Size &self) {\n> >> +\t\t\treturn \"libcamera.Size(\" + std::to_string(self.width) + \", \" + std::to_string(self.height) + \")\";\n> >> +\t\t});\n> >> +\n> >> +\tpy::class_(m, \"SizeRange\")\n> >> +\t\t.def(py::init<>())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init<>([](const std::array &s) {\n> >> +\t\t\treturn SizeRange(Size(std::get<0>(s), std::get<1>(s)));\n> >> +\t\t}))\n> >> +\t\t.def(py::init<>([](const std::array &min, const std::array &max) {\n> >> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n> >> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)));\n> >> +\t\t}))\n> >> +\t\t.def(py::init<>([](const std::array &min, const std::array &max,\n> >> +\t\t\t\t unsigned int hStep, unsigned int vStep) {\n> >> +\t\t\treturn SizeRange(Size(std::get<0>(min), std::get<1>(min)),\n> >> +\t\t\t\t\t Size(std::get<0>(max), std::get<1>(max)),\n> >> +\t\t\t\t\t hStep, vStep);\n> >> +\t\t}))\n> >> +\t\t.def_readwrite(\"min\", &SizeRange::min)\n> >> +\t\t.def_readwrite(\"max\", &SizeRange::max)\n> >> +\t\t.def_readwrite(\"hStep\", &SizeRange::hStep)\n> >> +\t\t.def_readwrite(\"vStep\", &SizeRange::vStep)\n> >> +\t\t.def(py::self == py::self)\n> >> +\t\t.def(\"__str__\", &SizeRange::toString)\n> >> +\t\t.def(\"__repr__\", [](const SizeRange &self) {\n> >> +\t\t\treturn py::str(\"libcamera.SizeRange(({}, {}), ({}, {}), {}, {})\")\n> >> +\t\t\t\t.format(self.min.width, self.min.height,\n> >> +\t\t\t\t\tself.max.width, self.max.height,\n> >> +\t\t\t\t\tself.hStep, self.vStep);\n> >> +\t\t});\n> >> +\n> >> +\tpy::class_(m, \"Rectangle\")\n> >> +\t\t.def(py::init<>())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init<>([](int x, int y, const std::array &s) {\n> >> +\t\t\treturn Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> >> +\t\t}))\n> > \n> > I'm puzzled a bit by this constructor. Where do you use it, and could\n> > then next constructor be used instead ? If it's meant to cover a common\n> > use case, should the C++ API also implement this ?\n> \n> It allows:\n> \n> libcam.Rectangle(1, 2, (3, 4))\n\nAnd then someone will request being able to write\n\n\tlibcam.Rectangle((1, 2), (3, 4))\n\nand possibly even\n\n\tlibcam.Rectangle((1, 2), 3, 4)\n\n:-)\n\nIn C++ it's a bit different thanks to implicit constructors and\ninitializer lists, so I agree that we may want to expose a similar\nfeature manually to make the code more readable. I'm just concerned\nabout the possible large number of combinations.\n\nIs there a Python coding style rule (or rather API design rule) about\nthis ?\n\n> It's more obvious with SizeRange:\n> \n> libcam.SizeRange((1, 2), (3, 4), 5, 6)\n> \n> instead of\n> \n> libcam.SizeRange(libcam.Size(1, 2), libcam.Size(3, 4), 5, 6)\n> \n> I really like tuples in python. I'd also like to do unpacking, like:\n> \n> w, h = libcam.Size(1, 2)\n> \n> I think I figured out how to do that, but after sending the series.\n> \n> These kind of features are perhaps something we need to do a decision \n> on: include them, or keep the API as bare-bones as possible.\n\nGood question. I personally prefer using s.width and s.height instead of\nunpacking and using w and h in most cases, but that's probably a matter\nof personal preference.\n\nI'm not against such \"small\" differences between the C++ and Python\nAPIs, if it makes the API better. \"better\" of course needs to be defined\nhere, my go-to reference is usually \"The Little Manual of API Design\" by\nJasmin Blanchette (https://www.cs.vu.nl/~jbe248/api-design.pdf). A good\nAPI should, in my opinion, have two important characteristics:\n\n- Easy to learn and memorize: guessing how to achieve a task should give\n the right answer. Consistency of the API is important here, but\n exceptions are totally fine when they make the API easier to guess.\n\n- Readability: an API piece (e.g. the name of a function on an\n object) should do what you expect it to do when reading it.\n\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init())\n> >> +\t\t.def_readwrite(\"x\", &Rectangle::x)\n> >> +\t\t.def_readwrite(\"y\", &Rectangle::y)\n> >> +\t\t.def_readwrite(\"width\", &Rectangle::width)\n> >> +\t\t.def_readwrite(\"height\", &Rectangle::height)\n> >> +\t\t.def_property_readonly(\"is_null\", &Rectangle::isNull)\n> >> +\t\t.def_property_readonly(\"center\", &Rectangle::center)\n> >> +\t\t.def_property_readonly(\"size\", &Rectangle::size)\n> >> +\t\t.def_property_readonly(\"topLeft\", &Rectangle::topLeft)\n> >> +\t\t.def(py::self == py::self)\n> >> +\t\t.def(\"__str__\", &Rectangle::toString)\n> >> +\t\t.def(\"__repr__\", [](const Rectangle &self) {\n> >> +\t\t\treturn py::str(\"libcamera.Rectangle({}, {}, {}, {})\")\n> >> +\t\t\t\t.format(self.x, self.y, self.width, self.height);\n> >> +\t\t});\n> > \n> > Could you add todo comments to remind that the other members of the\n> > geometry classes should be implemented too ?\n> \n> Yes.\n> \n> >> +}\n> >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n> >> index cc2ddee5..1dd70d9c 100644\n> >> --- a/src/py/libcamera/pymain.cpp\n> >> +++ b/src/py/libcamera/pymain.cpp\n> >> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)\n> >> \n> >> void init_pyenums(py::module &m);\n> >> void init_pyenums_generated(py::module &m);\n> >> +void init_py_geometry(py::module &m);\n> >> \n> >> PYBIND11_MODULE(_libcamera, m)\n> >> {\n> >> \tinit_pyenums(m);\n> >> \tinit_pyenums_generated(m);\n> >> +\tinit_py_geometry(m);\n> > \n> > My OCD kicks in :-)\n> \n> Now that I look at it... Yes...","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 6A507C3256\n\tfor ;\n\tTue, 17 May 2022 09:48:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C50C165656;\n\tTue, 17 May 2022 11:48:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03A4860420\n\tfor ;\n\tTue, 17 May 2022 11:48:20 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 480B4484;\n\tTue, 17 May 2022 11:48:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652780902;\n\tbh=IKEt5Yy4wdfoNY+p3F8lbVhnZfEk1BRAGsUcot4zHBA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gPteS1OjDDQASQAF5ZaAsvCZPGBbJgmBukeK1l9guI50EiKty7n1rDat8URBC5Y3E\n\tMAT1dk9H+2S8Mqg0CAq75DOMyUKBEoJH/A6/Uvgqn9b1zL/uwjY7gnKKNfhdktvOmf\n\tXiAQ6LFqlBzsk9DG/L24F4rlKQuSKkxEv8GQhYiRYLD+NQpdrumTSJa1YOjkgMf+OS\n\tk3f7fF3nvQrlP7nHWwWOxEibhnA2p3KPLM6Tyc5DoN+TYugRr3dyKKfp8A/EfUkUhP\n\t+jw7xnOWks8uwPQ8oFhUCVySEw4HbS7ktuyXYorpx+o/pF3fsC0hWHDTC2H2SjDdUf\n\tI1ZQdKCcMNUyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652780900;\n\tbh=IKEt5Yy4wdfoNY+p3F8lbVhnZfEk1BRAGsUcot4zHBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dL5HvvGHg7yUKvJ4WflzwlviSrbcBoLx1qu30yYgqChztPm2gHvRKPstCJXFR0/r6\n\tx6xCyfADnLV0lCHZ2KKQ4vaJuejHOFOC65CRf+TPiF+y/l9yWHH9czmQ2bzD72XnZK\n\t7nTW08kp26Dse8ovyzyAGKihIuRv3Cbu7wqmxPJw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dL5HvvGH\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 12:48:13 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22986,"web_url":"https://patchwork.libcamera.org/comment/22986/","msgid":"","date":"2022-05-17T12:56:53","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 17/05/2022 12:48, Laurent Pinchart wrote:\n\n>>>> +\tpy::class_(m, \"Rectangle\")\n>>>> +\t\t.def(py::init<>())\n>>>> +\t\t.def(py::init())\n>>>> +\t\t.def(py::init<>([](int x, int y, const std::array &s) {\n>>>> +\t\t\treturn Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n>>>> +\t\t}))\n>>>\n>>> I'm puzzled a bit by this constructor. Where do you use it, and could\n>>> then next constructor be used instead ? If it's meant to cover a common\n>>> use case, should the C++ API also implement this ?\n>>\n>> It allows:\n>>\n>> libcam.Rectangle(1, 2, (3, 4))\n> \n> And then someone will request being able to write\n> \n> \tlibcam.Rectangle((1, 2), (3, 4))\n> \n> and possibly even\n> \n> \tlibcam.Rectangle((1, 2), 3, 4)\n> \n> :-)\n\nYep. As a user, I would expect/hope all these would work:\n\nRectangle(Point(1, 2), Size(3, 4))\nRectangle((1, 2), (3, 4))\nRectangle([1, 2], [3, 4])\nRectangle(size=(3,4)) # pos is (0, 0)\nRectangle(1, 2, 3, 4) # not sure about this, it's a bit confusing\n\nManaging Point, Size, tuples and lists in the above example is solved by \nexpecting an object that gives us two ints, instead of expecting \nsomething specific. If both Point and Size support __iter__, and tuples \nand lists already do, then:\n\nx,y = pos\nw,h = size\n\nwill work for all those cases.\n\nAnd looking at the transforming methods, e.g.:\n\nrect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))\n\nis a bit annoying to use, compared to:\n\nrect.scale_by((1, 2), (3,4)).translate_by((5,6))\n\nTo fix that, I think we would have to overwrite all the methods we want \nto support such conversions. Which does not sound nice.\n\nI did some testing in the __init__.py, and I think we can (re-)implement \nanything we need there. E.g.:\n\ndef __Size_iter(self):\n yield from (self.width, self.height)\n\nSize.__iter__ = __Size_iter\n\n\nand\n\n\ndef __Rectangle_init(self, pos=(0, 0), size=(0, 0)):\n x, y = pos\n w, h = size\n Rectangle.__old_init(self, x, y, w, h)\n\nRectangle.__old_init = Rectangle.__init__\nRectangle.__init__ = __Rectangle_init\n\n> In C++ it's a bit different thanks to implicit constructors and\n> initializer lists, so I agree that we may want to expose a similar\n> feature manually to make the code more readable. I'm just concerned\n> about the possible large number of combinations.\n> \n> Is there a Python coding style rule (or rather API design rule) about\n> this ?\n\nI'm not aware of such. In my experience python classes/functions often \ntake a wide range of different inputs, and they do what you expect them \nto do. I don't know if that's recommended or is it just just something \nthat the authors have implemented because they liked it.\n\n Tomi","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 5DA42C0F2A\n\tfor ;\n\tTue, 17 May 2022 12:57:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55D366041E;\n\tTue, 17 May 2022 14:57:00 +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 F07E56041D\n\tfor ;\n\tTue, 17 May 2022 14:56:57 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32ACF4A8;\n\tTue, 17 May 2022 14:56:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652792220;\n\tbh=vWNN50x41lnqv8ezhxHDqgXvQGi80kMz3F93qe4dgWY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HqrqDsU0uaKnYu2w9BPifli/HU2fFNfeZFXjgBpjJrn/YYKaY4capCPKTfV5IFSso\n\tAjvd+N9tLyySHSHQorX354xTqxbWVIsEALmYNVGjSFVFF3vea6pc7mWHzfpNCN5vaz\n\tDJ/hoZqCIAgd9baeuiGF41j1IUd87EG40eRpm6SGhIhG0LQmZEOuzAjsYWIL7rQui0\n\t6Bxsrtq5kl2FizgKx0uNTwL7a3k3Z9RPcwDfBrqGbSgh8BL50mOUaOit1c1v6f+ka4\n\tFD8SUwwEnZ9vislXdbJi95ecM8e+wVMF7iT7qmWPcep2bkPNN88rGwQqvDza9BBwwC\n\tJLFBeV7sqs9Jg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652792217;\n\tbh=vWNN50x41lnqv8ezhxHDqgXvQGi80kMz3F93qe4dgWY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=JV5jDlp3NQB3Rjgcw8Duk8nkfFb8zjSJSbR1vy2947Kmxm9ed6vERC5Hia9Ba9nzl\n\tDR6temdIcz88bVD2zDUkF/W2haa5Pm783E1fivC4RL0alXf3tqmIo/sNqveqlCLzXf\n\t48hhTcd1O9P91g9xzlgnXsmQxC54RxKtbDgg/ENg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JV5jDlp3\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 17 May 2022 15:56:53 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22987,"web_url":"https://patchwork.libcamera.org/comment/22987/","msgid":"","date":"2022-05-17T13:27:25","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nI see there's been an interesting discussion going on. I'm just going\nto toss in my tuppence worth without coming to any firm conclusions,\nso I apologise in advance!\n\n* Most importantly libcamera's Python bindings need to expose the\nbasic libcamera C++ API. I hope that's uncontroversial!\n\n* Generally I'm not convinced it's worth worrying too much about how\n\"friendly\" the Python API is. I'm not saying we should deliberately\nmake it difficult, but I think the libcamera API is too intimidating\nfor casual camera users (\"help! I just want to capture a picture!\") or\neven those developing Python applications who would probably\nappreciate something higher level (\"I just want clicking on this\nbutton to start the camera!\").\n\n* So I wouldn't do lots of work or add lots of features to try and\nachieve that, though making it Pythonic and easy-to-use wherever we\ncan is of course still desirable.\n\n* I think there's maybe an argument for a \"friendly\" and slightly\nhigher level (?) libcamera Python API on top of the basic one (a bit\nlike Picamera2 is for us). But maybe there should be a distinction?\nNot sure.\n\n* I'm not sure what I think of providing all the geometry classes. On\none hand there's no harm in it, on the other... wouldn't most Python\nprogrammers prefer to deal with tuples?\n\nSorry if I'm being annoying!\n\nDavid\n\nOn Tue, 17 May 2022 at 13:56, Tomi Valkeinen\n wrote:\n>\n> On 17/05/2022 12:48, Laurent Pinchart wrote:\n>\n> >>>> + py::class_(m, \"Rectangle\")\n> >>>> + .def(py::init<>())\n> >>>> + .def(py::init())\n> >>>> + .def(py::init<>([](int x, int y, const std::array &s) {\n> >>>> + return Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> >>>> + }))\n> >>>\n> >>> I'm puzzled a bit by this constructor. Where do you use it, and could\n> >>> then next constructor be used instead ? If it's meant to cover a common\n> >>> use case, should the C++ API also implement this ?\n> >>\n> >> It allows:\n> >>\n> >> libcam.Rectangle(1, 2, (3, 4))\n> >\n> > And then someone will request being able to write\n> >\n> > libcam.Rectangle((1, 2), (3, 4))\n> >\n> > and possibly even\n> >\n> > libcam.Rectangle((1, 2), 3, 4)\n> >\n> > :-)\n>\n> Yep. As a user, I would expect/hope all these would work:\n>\n> Rectangle(Point(1, 2), Size(3, 4))\n> Rectangle((1, 2), (3, 4))\n> Rectangle([1, 2], [3, 4])\n> Rectangle(size=(3,4)) # pos is (0, 0)\n> Rectangle(1, 2, 3, 4) # not sure about this, it's a bit confusing\n>\n> Managing Point, Size, tuples and lists in the above example is solved by\n> expecting an object that gives us two ints, instead of expecting\n> something specific. If both Point and Size support __iter__, and tuples\n> and lists already do, then:\n>\n> x,y = pos\n> w,h = size\n>\n> will work for all those cases.\n>\n> And looking at the transforming methods, e.g.:\n>\n> rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))\n>\n> is a bit annoying to use, compared to:\n>\n> rect.scale_by((1, 2), (3,4)).translate_by((5,6))\n>\n> To fix that, I think we would have to overwrite all the methods we want\n> to support such conversions. Which does not sound nice.\n>\n> I did some testing in the __init__.py, and I think we can (re-)implement\n> anything we need there. E.g.:\n>\n> def __Size_iter(self):\n> yield from (self.width, self.height)\n>\n> Size.__iter__ = __Size_iter\n>\n>\n> and\n>\n>\n> def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):\n> x, y = pos\n> w, h = size\n> Rectangle.__old_init(self, x, y, w, h)\n>\n> Rectangle.__old_init = Rectangle.__init__\n> Rectangle.__init__ = __Rectangle_init\n>\n> > In C++ it's a bit different thanks to implicit constructors and\n> > initializer lists, so I agree that we may want to expose a similar\n> > feature manually to make the code more readable. I'm just concerned\n> > about the possible large number of combinations.\n> >\n> > Is there a Python coding style rule (or rather API design rule) about\n> > this ?\n>\n> I'm not aware of such. In my experience python classes/functions often\n> take a wide range of different inputs, and they do what you expect them\n> to do. I don't know if that's recommended or is it just just something\n> that the authors have implemented because they liked it.\n>\n> Tomi","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 F3C3BC3256\n\tfor ;\n\tTue, 17 May 2022 13:27:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FDFC65656;\n\tTue, 17 May 2022 15:27:39 +0200 (CEST)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BAEB6041D\n\tfor ;\n\tTue, 17 May 2022 15:27:37 +0200 (CEST)","by mail-wr1-x42e.google.com with SMTP id k30so12073790wrd.5\n\tfor ;\n\tTue, 17 May 2022 06:27:37 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652794059;\n\tbh=9LuYZR8wckXOjrEMtpsr96q7kETY8MtTdFHQAYGEy84=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=H1LoK44mGaOr93WHOw6ZiYTx1jaS8/EtC+WdkCD+u7Ga5cfU5LXXt7znavbPkcKyz\n\tzPzG+PBLIyaQdRIS5WqKE9j8vB/uB0EsWII/TZfL51pi6TlrREy/0tJv1BMfX4m3cr\n\ts+Zn+KcchCHqE9HoAN3zVKIujf9eErpvi9WDP2qhpdUxZKkudZ3KZAF8bEfa+fji79\n\tp9Sn4P+ND8PeYigs6XerZxZkV8ZseN5L1sT5hK/Cas2tPTxUozuQm8FWta+3dlLTj9\n\tQsQv+7uUnoqKeZ2wbiF9ngSOCA+2mFY6AF2XvldZznn/14p0u3VSOu2mlwI5O9LaLv\n\tXAguFYEpmplEw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=KFOvfW1dcFT6MaLAlNkvP0rVoJ3VG5QADRLenca5ijw=;\n\tb=OBwFVuf2F3Zra2RPNyMl8f4AV2TsTT8V/leTHTtvNyQjuickZ5HVGTUQo+7WTnblsi\n\tKRiJkJBMydbW5hfbcY2iZm17WsFy4hGjRgLqNd1CX5v2+N9S2nwPf4+kNc2CoU+2dLoh\n\tSwvORP83YLC7oRHSxYiBq0hj4rGX33kxxyjCySK2ts3dolDBEbP+d/e9QUYZt7dDVnyd\n\tMlzaa2L9F/Q75LjqzLG6GcJYH20w7EKzbeBDk8qXEm6RYzZlNBznnDFoAwYBIUnCsGQD\n\t8YvwluGmW2RufhqRkY+n5Uk5o5AYB7W8rFf/yaqa3tyjlZtV3GfGXbPRShiVF5xeBsEh\n\tGQ0g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"OBwFVuf2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=KFOvfW1dcFT6MaLAlNkvP0rVoJ3VG5QADRLenca5ijw=;\n\tb=NZVgIyG6P44n5w4A8XZhNA03K3JqTV8P36LK1JR3D9qzIF48DW+wndEB3N/NcBEKBg\n\tCFbrUnp6p6/RWWVK/QeLzaXfZ7GfNuvntQJsRlq6GlZ5BDdQGrdwskZZz2pAibpZpDAY\n\t7hH0YOTQgpkQInr3IvL4IjQnRHuryd4Gt2iKoJpAMVt9bqd66mv7N7WrAJsw3o739Cdr\n\tJnoNte0RlP82nHGlGGgb3+aUbRL5mvg6nmnpiBIs/kuMPHk2gFj81V29BLSoHomjrL3P\n\tbJQx6aqu42XR5UavSriOG2OAnjJjgIVxWrcXDlPK+R7RY9wIFNtFZCoduVTuqQQRDFSo\n\tUJIw==","X-Gm-Message-State":"AOAM531fUugQ5F5aaUM0LSGH9sultMYq7To3JoGO996J3/0jQ1Px/exp\n\tsbLkjGap2XEAZOiQDoGw3jtI3PHz8sseRRlQbDb18/6p89U1Kg==","X-Google-Smtp-Source":"ABdhPJwVOd+7E0jTS8umsxGJU/p166Bu89as1jOb5NI6ADyT50OYSyzZCfXyW4qD0KzIUpNssYLHvo8nSaKyWuLJqj4=","X-Received":"by 2002:a5d:6504:0:b0:20d:dbb:5003 with SMTP id\n\tx4-20020a5d6504000000b0020d0dbb5003mr6699238wru.595.1652794056783;\n\tTue, 17 May 2022 06:27:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Tue, 17 May 2022 14:27:25 +0100","Message-ID":"","To":"Tomi Valkeinen ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22988,"web_url":"https://patchwork.libcamera.org/comment/22988/","msgid":"","date":"2022-05-17T13:56:41","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:\n> Hi everyone\n> \n> I see there's been an interesting discussion going on. I'm just going\n> to toss in my tuppence worth without coming to any firm conclusions,\n> so I apologise in advance!\n> \n> * Most importantly libcamera's Python bindings need to expose the\n> basic libcamera C++ API. I hope that's uncontroversial!\n> \n> * Generally I'm not convinced it's worth worrying too much about how\n> \"friendly\" the Python API is. I'm not saying we should deliberately\n> make it difficult, but I think the libcamera API is too intimidating\n> for casual camera users (\"help! I just want to capture a picture!\") or\n> even those developing Python applications who would probably\n> appreciate something higher level (\"I just want clicking on this\n> button to start the camera!\").\n> \n> * So I wouldn't do lots of work or add lots of features to try and\n> achieve that, though making it Pythonic and easy-to-use wherever we\n> can is of course still desirable.\n\nThat seems reasonable, we can start by staying close to the C++ API with\na minimum amount of \"pythonic\" rework of the API, and then improve on\ntop later.\n\n> * I think there's maybe an argument for a \"friendly\" and slightly\n> higher level (?) libcamera Python API on top of the basic one (a bit\n> like Picamera2 is for us). But maybe there should be a distinction?\n> Not sure.\n> \n> * I'm not sure what I think of providing all the geometry classes. On\n> one hand there's no harm in it, on the other... wouldn't most Python\n> programmers prefer to deal with tuples?\n\nI can't tell about \"most Python programmers\", but I find\n\n\tfoo(Size(100, 100), Size(500, 500))\n\nmore readable than\n\n\tfoo((100, 100), (500, 500))\n\nas the latter doesn't clearly say if we're dealing with points or sizes\n(or something else). I don't know if that's relevant, but\nhttps://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html\ndoesn't provide a constructor that takes two lists or tuples.\n\n> Sorry if I'm being annoying!\n\nYou're not :-)\n\n> On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:\n> > On 17/05/2022 12:48, Laurent Pinchart wrote:\n> >\n> > >>>> + py::class_(m, \"Rectangle\")\n> > >>>> + .def(py::init<>())\n> > >>>> + .def(py::init())\n> > >>>> + .def(py::init<>([](int x, int y, const std::array &s) {\n> > >>>> + return Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> > >>>> + }))\n> > >>>\n> > >>> I'm puzzled a bit by this constructor. Where do you use it, and could\n> > >>> then next constructor be used instead ? If it's meant to cover a common\n> > >>> use case, should the C++ API also implement this ?\n> > >>\n> > >> It allows:\n> > >>\n> > >> libcam.Rectangle(1, 2, (3, 4))\n> > >\n> > > And then someone will request being able to write\n> > >\n> > > libcam.Rectangle((1, 2), (3, 4))\n> > >\n> > > and possibly even\n> > >\n> > > libcam.Rectangle((1, 2), 3, 4)\n> > >\n> > > :-)\n> >\n> > Yep. As a user, I would expect/hope all these would work:\n> >\n> > Rectangle(Point(1, 2), Size(3, 4))\n> > Rectangle((1, 2), (3, 4))\n> > Rectangle([1, 2], [3, 4])\n> > Rectangle(size=(3,4)) # pos is (0, 0)\n> > Rectangle(1, 2, 3, 4) # not sure about this, it's a bit confusing\n> >\n> > Managing Point, Size, tuples and lists in the above example is solved by\n> > expecting an object that gives us two ints, instead of expecting\n> > something specific. If both Point and Size support __iter__, and tuples\n> > and lists already do, then:\n> >\n> > x,y = pos\n> > w,h = size\n> >\n> > will work for all those cases.\n> >\n> > And looking at the transforming methods, e.g.:\n> >\n> > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))\n> >\n> > is a bit annoying to use, compared to:\n> >\n> > rect.scale_by((1, 2), (3,4)).translate_by((5,6))\n> >\n> > To fix that, I think we would have to overwrite all the methods we want\n> > to support such conversions. Which does not sound nice.\n> >\n> > I did some testing in the __init__.py, and I think we can (re-)implement\n> > anything we need there. E.g.:\n> >\n> > def __Size_iter(self):\n> > yield from (self.width, self.height)\n> >\n> > Size.__iter__ = __Size_iter\n> >\n> >\n> > and\n> >\n> >\n> > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):\n> > x, y = pos\n> > w, h = size\n> > Rectangle.__old_init(self, x, y, w, h)\n> >\n> > Rectangle.__old_init = Rectangle.__init__\n> > Rectangle.__init__ = __Rectangle_init\n> >\n> > > In C++ it's a bit different thanks to implicit constructors and\n> > > initializer lists, so I agree that we may want to expose a similar\n> > > feature manually to make the code more readable. I'm just concerned\n> > > about the possible large number of combinations.\n> > >\n> > > Is there a Python coding style rule (or rather API design rule) about\n> > > this ?\n> >\n> > I'm not aware of such. In my experience python classes/functions often\n> > take a wide range of different inputs, and they do what you expect them\n> > to do. I don't know if that's recommended or is it just just something\n> > that the authors have implemented because they liked it.","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 9112EC0F2A\n\tfor ;\n\tTue, 17 May 2022 13:56:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE0CB65656;\n\tTue, 17 May 2022 15:56:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A61B6041D\n\tfor ;\n\tTue, 17 May 2022 15:56:49 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 859B54A8;\n\tTue, 17 May 2022 15:56:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652795811;\n\tbh=y02sBEhJPUjeySjYdVWywtqahgftzoXrAHxrEEJfDQY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=u17sIuuNCqdKb1IMV7mKP5L/q27ep8uHkvk1R7saHhFfVgt2+KL8uW/A5+hG3rYbW\n\tsrnlfzG2nFwixUbxjRcUB4q+GJX7PjDI9wYLXgc6j2NTdWzsH8VGn+YyPdl2xa9Xa8\n\tshdIs/AOG3WRFf7NP3sAIKDemgRxsHff97INXztgr99XR5mS8E2wOT+vh2w48lzM6f\n\tw4ReVRX+QVtJJdI2i272Lj6zkQ5GHWmi4kNTeiJrBkJ1HuwrJAFmrhkGhDi0vfkPlS\n\tGtK3aLkfMcMkKHXBhq1UqDOe6ea7D/Cxsql5CET001U9h/AjNwL0aYDjs0tgZX3ZnZ\n\tAUoLS1r58EW/w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652795809;\n\tbh=y02sBEhJPUjeySjYdVWywtqahgftzoXrAHxrEEJfDQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=worndwxXc6d5Ro8bs+UZtfUZBSAghpk82eS8nyTWbTQiqByqeDccjjurH/LXaDqQq\n\t3egIeRbJXwKNmP30bNcmWWdT5//b8aDn5NQyd/ENpaaxQt/dDhMjiv1OWmCGw1tTs4\n\tESWKDBqxXoqDwR387vB3okQbz1A3XgnlGl4JgRgw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"worndwxX\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 16:56:41 +0300","To":"David Plowman ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22989,"web_url":"https://patchwork.libcamera.org/comment/22989/","msgid":"<206670ab-2b14-3794-1ecb-53f5a777ffa6@ideasonboard.com>","date":"2022-05-17T14:17:46","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 17/05/2022 16:56, Laurent Pinchart wrote:\n> Hi David,\n> \n> On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:\n>> Hi everyone\n>>\n>> I see there's been an interesting discussion going on. I'm just going\n>> to toss in my tuppence worth without coming to any firm conclusions,\n>> so I apologise in advance!\n>>\n>> * Most importantly libcamera's Python bindings need to expose the\n>> basic libcamera C++ API. I hope that's uncontroversial!\n>>\n>> * Generally I'm not convinced it's worth worrying too much about how\n>> \"friendly\" the Python API is. I'm not saying we should deliberately\n>> make it difficult, but I think the libcamera API is too intimidating\n>> for casual camera users (\"help! I just want to capture a picture!\") or\n>> even those developing Python applications who would probably\n>> appreciate something higher level (\"I just want clicking on this\n>> button to start the camera!\").\n>>\n>> * So I wouldn't do lots of work or add lots of features to try and\n>> achieve that, though making it Pythonic and easy-to-use wherever we\n>> can is of course still desirable.\n> \n> That seems reasonable, we can start by staying close to the C++ API with\n> a minimum amount of \"pythonic\" rework of the API, and then improve on\n> top later.\n> \n>> * I think there's maybe an argument for a \"friendly\" and slightly\n>> higher level (?) libcamera Python API on top of the basic one (a bit\n>> like Picamera2 is for us). But maybe there should be a distinction?\n>> Not sure.\n>>\n>> * I'm not sure what I think of providing all the geometry classes. On\n>> one hand there's no harm in it, on the other... wouldn't most Python\n>> programmers prefer to deal with tuples?\n\nThe geometry classes do offer quite many transformation functions. \nHaving the classes also makes the bindings simpler, as we don't need to \nconvert between tuples and the proper geometry class in multiple places \n(see the \"py: use geometry classes\").\n\n> I can't tell about \"most Python programmers\", but I find\n> \n> \tfoo(Size(100, 100), Size(500, 500))\n> \n> more readable than\n> \n> \tfoo((100, 100), (500, 500))\n\nFor \"foo\", I agree, as I have no clue what it does ;).\n\nFor Rect, I agree also, as you can well have Rect(Point, Point) and \nRect(Point, Size).\n\nI was a bit surprised to find Rectangle(int, int, uint, uint) \nconstructor on the C++ side, as it's not obvious if the latter uints are \nfor a Point or Size. It also doesn't have Rectangle(Point, Size) at all.\n\nIn any case, I've dropped the \"friendly\" code from the series for now.\n\n> as the latter doesn't clearly say if we're dealing with points or sizes\n> (or something else). I don't know if that's relevant, but\n> https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html\n> doesn't provide a constructor that takes two lists or tuples.\n\nI don't think Qt is very pythonic.\n\n Tomi","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 B1AE4C3256\n\tfor ;\n\tTue, 17 May 2022 14:17:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6FEF6041D;\n\tTue, 17 May 2022 16:17:51 +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 D3BDD6041D\n\tfor ;\n\tTue, 17 May 2022 16:17:49 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F08454A8;\n\tTue, 17 May 2022 16:17:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652797071;\n\tbh=5pOCmjC0kU8p1GivChYS0csKvZ3m0ViLkScwMLjvPEY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3Yz0c/+DuLoo9kTQa2CVLz2FnjMFRgcGEq8bAcwhnxBY1dWyzIG1n+rStcsHon4gZ\n\t4gKEjBuJmHoQr+YbT8TrtHa9/fMKIm0QyEikc/u+L9rwjaDQPvg5+/0QUuG3CUQkBp\n\t3/E3gZ1/ffGcbyrXVPsquD0gvj5LcsEPDHpVTIIEAouVJOhErkVHF3oJRQCW5wDgTv\n\tqlN1voWj+MWPnyolsql9jbzhTO1XGwaD52qJEO4ul7Dq2DC4xCM7YSx0Hccl+oZtYR\n\trN1u377fpg5D9w4BCO/W4eOOHUqMMMDs9VprHvVd6ffZn0MVLFo0RrA99shIuMnonj\n\tKI8kJZ7hJ+4xw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652797069;\n\tbh=5pOCmjC0kU8p1GivChYS0csKvZ3m0ViLkScwMLjvPEY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=kyMQT9LcoUEpfdOf7dqgHm1zSZym97DWigChUTc/fq+4jC2o4F0uLZglJX2lb7q+i\n\tP0R0Ew3iAeHaTwB5k5p792ryLmVbp7tAzq90qaIy0ljIyRYtCk2NYwuqsn+zeqRlRL\n\tksqCgmDHa2D5sltD8yz69WS9m/cp3UAtC63rIR2k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kyMQT9Lc\"; dkim-atps=neutral","Message-ID":"<206670ab-2b14-3794-1ecb-53f5a777ffa6@ideasonboard.com>","Date":"Tue, 17 May 2022 17:17:46 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tDavid Plowman ","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22991,"web_url":"https://patchwork.libcamera.org/comment/22991/","msgid":"","date":"2022-05-17T14:22:27","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Tue, May 17, 2022 at 05:17:46PM +0300, Tomi Valkeinen wrote:\n> On 17/05/2022 16:56, Laurent Pinchart wrote:\n> > On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:\n> >> Hi everyone\n> >>\n> >> I see there's been an interesting discussion going on. I'm just going\n> >> to toss in my tuppence worth without coming to any firm conclusions,\n> >> so I apologise in advance!\n> >>\n> >> * Most importantly libcamera's Python bindings need to expose the\n> >> basic libcamera C++ API. I hope that's uncontroversial!\n> >>\n> >> * Generally I'm not convinced it's worth worrying too much about how\n> >> \"friendly\" the Python API is. I'm not saying we should deliberately\n> >> make it difficult, but I think the libcamera API is too intimidating\n> >> for casual camera users (\"help! I just want to capture a picture!\") or\n> >> even those developing Python applications who would probably\n> >> appreciate something higher level (\"I just want clicking on this\n> >> button to start the camera!\").\n> >>\n> >> * So I wouldn't do lots of work or add lots of features to try and\n> >> achieve that, though making it Pythonic and easy-to-use wherever we\n> >> can is of course still desirable.\n> > \n> > That seems reasonable, we can start by staying close to the C++ API with\n> > a minimum amount of \"pythonic\" rework of the API, and then improve on\n> > top later.\n> > \n> >> * I think there's maybe an argument for a \"friendly\" and slightly\n> >> higher level (?) libcamera Python API on top of the basic one (a bit\n> >> like Picamera2 is for us). But maybe there should be a distinction?\n> >> Not sure.\n> >>\n> >> * I'm not sure what I think of providing all the geometry classes. On\n> >> one hand there's no harm in it, on the other... wouldn't most Python\n> >> programmers prefer to deal with tuples?\n> \n> The geometry classes do offer quite many transformation functions. \n> Having the classes also makes the bindings simpler, as we don't need to \n> convert between tuples and the proper geometry class in multiple places \n> (see the \"py: use geometry classes\").\n> \n> > I can't tell about \"most Python programmers\", but I find\n> > \n> > \tfoo(Size(100, 100), Size(500, 500))\n> > \n> > more readable than\n> > \n> > \tfoo((100, 100), (500, 500))\n> \n> For \"foo\", I agree, as I have no clue what it does ;).\n> \n> For Rect, I agree also, as you can well have Rect(Point, Point) and \n> Rect(Point, Size).\n> \n> I was a bit surprised to find Rectangle(int, int, uint, uint) \n> constructor on the C++ side, as it's not obvious if the latter uints are \n> for a Point or Size. It also doesn't have Rectangle(Point, Size) at all.\n\nYou're right about that. At least the latter should be fixed, and\npossibly the former too.\n\n> In any case, I've dropped the \"friendly\" code from the series for now.\n> \n> > as the latter doesn't clearly say if we're dealing with points or sizes\n> > (or something else). I don't know if that's relevant, but\n> > https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html\n> > doesn't provide a constructor that takes two lists or tuples.\n> \n> I don't think Qt is very pythonic.\n\nThat's a fair point, and maybe why my APIs are more \"Qt-ic\" than\npythonic :-)","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 18923C0F2A\n\tfor ;\n\tTue, 17 May 2022 14:22:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BFAB6041E;\n\tTue, 17 May 2022 16:22:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED7896041D\n\tfor ;\n\tTue, 17 May 2022 16:22:34 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 767634A8;\n\tTue, 17 May 2022 16:22:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652797356;\n\tbh=UII4ZvNwE4GoJyvLufhPJE6Qc8NpSy7IS5GrEJw+tUM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Z/hvYK19tltXoWDanMZ63UpHSMs0zkd9H6ZnLumxpwNDnhmXu23QaF/2SSPzydHLP\n\t4FmXqnoUGZ4XP2qfsgY2vPYPP+uqCr6iN6HWNiop6J7aDIYX3f0lWDCjbyJWoOQXM3\n\tjRJUu+Jf3kB8ZGmU/JGuuPQy9+JAcFS94qcx0letlMeN6yuxGO1qaxTy4SWFyzMD/g\n\t37fyIQJgSP8w2Un5zZvnkhv4T7AE/ud4abZmAwY1JpOaTIvHqFloQp/gT+kvkrHb9o\n\tXFQWYUsR+sGmBqd4brfZ2fRz0F8RelBBswaCiSA+TgL9GYwsKzvzmv2z3quZ5Jvjgq\n\tr44duEiGzmQwA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652797354;\n\tbh=UII4ZvNwE4GoJyvLufhPJE6Qc8NpSy7IS5GrEJw+tUM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MIhxDU4Lh/xUUiuVoTCFxJzYfVBflUuLFHr/G4S5XGN5QeCTYfmcXI/H/FSvJFXy5\n\tgZcP1yFp8LjC0OiDYUkuqyUWQ7XfMvQcIw+r93Ipcu1vXYHLG/xB7ihYH/EBa7CbI/\n\tffvHHs8LzTCSo6j1q/vnP34e3K00C2ydov69vXVo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MIhxDU4L\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 17:22:27 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<206670ab-2b14-3794-1ecb-53f5a777ffa6@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<206670ab-2b14-3794-1ecb-53f5a777ffa6@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22993,"web_url":"https://patchwork.libcamera.org/comment/22993/","msgid":"","date":"2022-05-17T15:21:18","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent, everyone\n\nOn Tue, 17 May 2022 at 14:56, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:\n> > Hi everyone\n> >\n> > I see there's been an interesting discussion going on. I'm just going\n> > to toss in my tuppence worth without coming to any firm conclusions,\n> > so I apologise in advance!\n> >\n> > * Most importantly libcamera's Python bindings need to expose the\n> > basic libcamera C++ API. I hope that's uncontroversial!\n> >\n> > * Generally I'm not convinced it's worth worrying too much about how\n> > \"friendly\" the Python API is. I'm not saying we should deliberately\n> > make it difficult, but I think the libcamera API is too intimidating\n> > for casual camera users (\"help! I just want to capture a picture!\") or\n> > even those developing Python applications who would probably\n> > appreciate something higher level (\"I just want clicking on this\n> > button to start the camera!\").\n> >\n> > * So I wouldn't do lots of work or add lots of features to try and\n> > achieve that, though making it Pythonic and easy-to-use wherever we\n> > can is of course still desirable.\n>\n> That seems reasonable, we can start by staying close to the C++ API with\n> a minimum amount of \"pythonic\" rework of the API, and then improve on\n> top later.\n>\n> > * I think there's maybe an argument for a \"friendly\" and slightly\n> > higher level (?) libcamera Python API on top of the basic one (a bit\n> > like Picamera2 is for us). But maybe there should be a distinction?\n> > Not sure.\n> >\n> > * I'm not sure what I think of providing all the geometry classes. On\n> > one hand there's no harm in it, on the other... wouldn't most Python\n> > programmers prefer to deal with tuples?\n>\n> I can't tell about \"most Python programmers\", but I find\n>\n> foo(Size(100, 100), Size(500, 500))\n>\n> more readable than\n>\n> foo((100, 100), (500, 500))\n\nThat's true, but from my vast experience of \"a few months\" writing\nnot-completely-trivial Python I think I might quite like:\n\nfoo(offset=(100, 100), size=(500, 500))\n\nI really like keyword args as they reduce silly mistakes and are\nself-documenting. Of course there's also \"foo(offset=Point(100, 100),\nsize=Size(500, 500))\", that works too.\n\nIn my (again, vast!) experience I find that Python often seems to use\nlots of lists/tuples and dicts where in the C/C++ world we'd have\nstructs, so it can sometimes feel a bit unstructured to me. But I'm\nnot sure what I prefer...\n\nDavid\n\n>\n> as the latter doesn't clearly say if we're dealing with points or sizes\n> (or something else). I don't know if that's relevant, but\n> https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html\n> doesn't provide a constructor that takes two lists or tuples.\n>\n> > Sorry if I'm being annoying!\n>\n> You're not :-)\n>\n> > On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:\n> > > On 17/05/2022 12:48, Laurent Pinchart wrote:\n> > >\n> > > >>>> + py::class_(m, \"Rectangle\")\n> > > >>>> + .def(py::init<>())\n> > > >>>> + .def(py::init())\n> > > >>>> + .def(py::init<>([](int x, int y, const std::array &s) {\n> > > >>>> + return Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> > > >>>> + }))\n> > > >>>\n> > > >>> I'm puzzled a bit by this constructor. Where do you use it, and could\n> > > >>> then next constructor be used instead ? If it's meant to cover a common\n> > > >>> use case, should the C++ API also implement this ?\n> > > >>\n> > > >> It allows:\n> > > >>\n> > > >> libcam.Rectangle(1, 2, (3, 4))\n> > > >\n> > > > And then someone will request being able to write\n> > > >\n> > > > libcam.Rectangle((1, 2), (3, 4))\n> > > >\n> > > > and possibly even\n> > > >\n> > > > libcam.Rectangle((1, 2), 3, 4)\n> > > >\n> > > > :-)\n> > >\n> > > Yep. As a user, I would expect/hope all these would work:\n> > >\n> > > Rectangle(Point(1, 2), Size(3, 4))\n> > > Rectangle((1, 2), (3, 4))\n> > > Rectangle([1, 2], [3, 4])\n> > > Rectangle(size=(3,4)) # pos is (0, 0)\n> > > Rectangle(1, 2, 3, 4) # not sure about this, it's a bit confusing\n> > >\n> > > Managing Point, Size, tuples and lists in the above example is solved by\n> > > expecting an object that gives us two ints, instead of expecting\n> > > something specific. If both Point and Size support __iter__, and tuples\n> > > and lists already do, then:\n> > >\n> > > x,y = pos\n> > > w,h = size\n> > >\n> > > will work for all those cases.\n> > >\n> > > And looking at the transforming methods, e.g.:\n> > >\n> > > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))\n> > >\n> > > is a bit annoying to use, compared to:\n> > >\n> > > rect.scale_by((1, 2), (3,4)).translate_by((5,6))\n> > >\n> > > To fix that, I think we would have to overwrite all the methods we want\n> > > to support such conversions. Which does not sound nice.\n> > >\n> > > I did some testing in the __init__.py, and I think we can (re-)implement\n> > > anything we need there. E.g.:\n> > >\n> > > def __Size_iter(self):\n> > > yield from (self.width, self.height)\n> > >\n> > > Size.__iter__ = __Size_iter\n> > >\n> > >\n> > > and\n> > >\n> > >\n> > > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):\n> > > x, y = pos\n> > > w, h = size\n> > > Rectangle.__old_init(self, x, y, w, h)\n> > >\n> > > Rectangle.__old_init = Rectangle.__init__\n> > > Rectangle.__init__ = __Rectangle_init\n> > >\n> > > > In C++ it's a bit different thanks to implicit constructors and\n> > > > initializer lists, so I agree that we may want to expose a similar\n> > > > feature manually to make the code more readable. I'm just concerned\n> > > > about the possible large number of combinations.\n> > > >\n> > > > Is there a Python coding style rule (or rather API design rule) about\n> > > > this ?\n> > >\n> > > I'm not aware of such. In my experience python classes/functions often\n> > > take a wide range of different inputs, and they do what you expect them\n> > > to do. I don't know if that's recommended or is it just just something\n> > > that the authors have implemented because they liked it.\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 4EC6DC0F2A\n\tfor ;\n\tTue, 17 May 2022 15:21:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DCBF6041E;\n\tTue, 17 May 2022 17:21:31 +0200 (CEST)","from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com\n\t[IPv6:2a00:1450:4864:20::42b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90A096041D\n\tfor ;\n\tTue, 17 May 2022 17:21:29 +0200 (CEST)","by mail-wr1-x42b.google.com with SMTP id a5so21441673wrp.7\n\tfor ;\n\tTue, 17 May 2022 08:21:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652800891;\n\tbh=J7TqEeFBRTWoocqIvJ0Zvb8skxBvE0bEFrIOn/IDoEU=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=TXALJdufgKaZwGafJlANmToSkv3ksuIP/4Qq1iJPaDsTfb8ZwqWsqO9I/oMyCN7jO\n\tc/UtDvNkImeQdxmm2JTvorPzOuQ+QY5n5ku8iP3CCYM0RkOKeD6iXrNK+bsH2av6gI\n\tHUXGnukvzZunjt6pjNQfG1ZQvFryuO93rZvg82to4uVqQwfcaA4IGf4bNSIB+Mbe09\n\ta6tBdn1jG1LuizHpaYto0IUFaA+ZoefGiM0+TqS6YhG1uH0MQMr2q9hoSmMTN2GwIa\n\tBRLhnsgXBhb47JoLRE4DOfK88sJ3mCAXblOA5XQWFByeUHm+xtThT1s4itSo4Y9DK+\n\tI9lip3/s9FVOQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ZWPFOdJpYjld7kWrB0vRySWlUalw3D1u41tuXmoyAiA=;\n\tb=EUy2T0+TGGQzcK+8o0qzcsNRTM5tjN+16kkcGz+pkuNaGiU3WxLZFbh54kmvHHKx7A\n\tKM4HF5WwE60XmNqvm3FfPMFr7Hppxh6FjuuJYh7qK3mHYbNsueZgWggR/3Yd977jo4si\n\tuh3a5GK720yBcYJC4LN4d19eghMJ1qCcjjUUmLfQofzj4I3GIlIA3gjRzi/c05EsP2Li\n\tQSs2BX8WetmJQXhbqqKb78hfJdO5cvGWsYsVKVAIRi2WLxPfqgyGd6lQBt7fNB597P8x\n\tboiCNcql2/hamWnnmYAdfk0eXZaOcpfdiBsmjtMMjGfvh4udSJe2lswF7AW0K7zdvQi0\n\tHMsw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EUy2T0+T\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ZWPFOdJpYjld7kWrB0vRySWlUalw3D1u41tuXmoyAiA=;\n\tb=49VXZVUY5WZGwuuJ2+MuhHJ0BMsqZkbn0CAO/QQzeonShWP3wrwKD/wPCxiQRnMSMX\n\tQqxcFp5aTysgN0Kn76EdsaCto/Wz2uwx6WPy3wQG4r14Yl7zO0H4K33mlcy6V4T/M7jd\n\t+ZgKewjTwP4otFdeujdEXDtzL3QhLr95GX0eqFmuHXg3h6ODIFU1STrdCqpvb+Uf+nga\n\tH6fHqat4nuThTR4El2PRiyWGrSTG9AwLMQ1FqeEF0p5P2Pb08WRyoDYH4JG9bMBIvmNg\n\tnGCuhVVXecH5FkRF4dC8ynU89h+Ekvo5yvGIDb8+yP8bakk7lB8XPOJz3Vmw1I+qtJ0t\n\tvnhg==","X-Gm-Message-State":"AOAM531KCx/eQJ1fx+QzI/qvw3NpnjsTJnTuo+bhlFllQCHmUcURPjC0\n\t+rjFOG30rlsKoEZ6coYztn8fpiR1Prn8ZG00SkOjEg==","X-Google-Smtp-Source":"ABdhPJzWPv+qdG8/Q3vbD/5PZWucrUyKysAEl9sxHr6xDjKs7fI+BxhEgZIRVi4Pi1UN0mePcvsjkOuLG8oX6bFsi+g=","X-Received":"by 2002:a5d:6489:0:b0:20c:6b7d:1bab with SMTP id\n\to9-20020a5d6489000000b0020c6b7d1babmr18906474wri.449.1652800889089;\n\tTue, 17 May 2022 08:21:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Tue, 17 May 2022 16:21:18 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23003,"web_url":"https://patchwork.libcamera.org/comment/23003/","msgid":"","date":"2022-05-17T16:24:09","subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, May 17, 2022 at 04:21:18PM +0100, David Plowman wrote:\n> On Tue, 17 May 2022 at 14:56, Laurent Pinchart wrote:\n> > On Tue, May 17, 2022 at 02:27:25PM +0100, David Plowman wrote:\n> > > Hi everyone\n> > >\n> > > I see there's been an interesting discussion going on. I'm just going\n> > > to toss in my tuppence worth without coming to any firm conclusions,\n> > > so I apologise in advance!\n> > >\n> > > * Most importantly libcamera's Python bindings need to expose the\n> > > basic libcamera C++ API. I hope that's uncontroversial!\n> > >\n> > > * Generally I'm not convinced it's worth worrying too much about how\n> > > \"friendly\" the Python API is. I'm not saying we should deliberately\n> > > make it difficult, but I think the libcamera API is too intimidating\n> > > for casual camera users (\"help! I just want to capture a picture!\") or\n> > > even those developing Python applications who would probably\n> > > appreciate something higher level (\"I just want clicking on this\n> > > button to start the camera!\").\n> > >\n> > > * So I wouldn't do lots of work or add lots of features to try and\n> > > achieve that, though making it Pythonic and easy-to-use wherever we\n> > > can is of course still desirable.\n> >\n> > That seems reasonable, we can start by staying close to the C++ API with\n> > a minimum amount of \"pythonic\" rework of the API, and then improve on\n> > top later.\n> >\n> > > * I think there's maybe an argument for a \"friendly\" and slightly\n> > > higher level (?) libcamera Python API on top of the basic one (a bit\n> > > like Picamera2 is for us). But maybe there should be a distinction?\n> > > Not sure.\n> > >\n> > > * I'm not sure what I think of providing all the geometry classes. On\n> > > one hand there's no harm in it, on the other... wouldn't most Python\n> > > programmers prefer to deal with tuples?\n> >\n> > I can't tell about \"most Python programmers\", but I find\n> >\n> > foo(Size(100, 100), Size(500, 500))\n> >\n> > more readable than\n> >\n> > foo((100, 100), (500, 500))\n> \n> That's true, but from my vast experience of \"a few months\" writing\n> not-completely-trivial Python I think I might quite like:\n> \n> foo(offset=(100, 100), size=(500, 500))\n\nI agree with you, that's readable, and it's the style we should adopt in\nour own code. It doesn't prevent people from shooting themselves in the\nfoot by writing\n\n\tfoo((100, 100), (500, 500))\n\nbut maybe it's not our role to do so. I would however like to prevent\n\n\tfoo(Point(100, 100), Point(100, 100))\n\nfrom being silently ignored, which I think would be the case if the\nbindings only took tuple arguments and relied on geometry classes\nimplementing __iter__(). I don't know if there's an easy way to handle\nthis though.\n\n> I really like keyword args as they reduce silly mistakes and are\n> self-documenting. Of course there's also \"foo(offset=Point(100, 100),\n> size=Size(500, 500))\", that works too.\n> \n> In my (again, vast!) experience I find that Python often seems to use\n> lots of lists/tuples and dicts where in the C/C++ world we'd have\n> structs, so it can sometimes feel a bit unstructured to me. But I'm\n> not sure what I prefer...\n\nI wonder if that coud be caused by the fact that creating structures\nwith named fields is more difficult in Python than in C/C++.\n\n> > as the latter doesn't clearly say if we're dealing with points or sizes\n> > (or something else). I don't know if that's relevant, but\n> > https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtcore/qrect.html\n> > doesn't provide a constructor that takes two lists or tuples.\n> >\n> > > Sorry if I'm being annoying!\n> >\n> > You're not :-)\n> >\n> > > On Tue, 17 May 2022 at 13:56, Tomi Valkeinen wrote:\n> > > > On 17/05/2022 12:48, Laurent Pinchart wrote:\n> > > >\n> > > > >>>> + py::class_(m, \"Rectangle\")\n> > > > >>>> + .def(py::init<>())\n> > > > >>>> + .def(py::init())\n> > > > >>>> + .def(py::init<>([](int x, int y, const std::array &s) {\n> > > > >>>> + return Rectangle(x, y, std::get<0>(s), std::get<1>(s));\n> > > > >>>> + }))\n> > > > >>>\n> > > > >>> I'm puzzled a bit by this constructor. Where do you use it, and could\n> > > > >>> then next constructor be used instead ? If it's meant to cover a common\n> > > > >>> use case, should the C++ API also implement this ?\n> > > > >>\n> > > > >> It allows:\n> > > > >>\n> > > > >> libcam.Rectangle(1, 2, (3, 4))\n> > > > >\n> > > > > And then someone will request being able to write\n> > > > >\n> > > > > libcam.Rectangle((1, 2), (3, 4))\n> > > > >\n> > > > > and possibly even\n> > > > >\n> > > > > libcam.Rectangle((1, 2), 3, 4)\n> > > > >\n> > > > > :-)\n> > > >\n> > > > Yep. As a user, I would expect/hope all these would work:\n> > > >\n> > > > Rectangle(Point(1, 2), Size(3, 4))\n> > > > Rectangle((1, 2), (3, 4))\n> > > > Rectangle([1, 2], [3, 4])\n> > > > Rectangle(size=(3,4)) # pos is (0, 0)\n> > > > Rectangle(1, 2, 3, 4) # not sure about this, it's a bit confusing\n> > > >\n> > > > Managing Point, Size, tuples and lists in the above example is solved by\n> > > > expecting an object that gives us two ints, instead of expecting\n> > > > something specific. If both Point and Size support __iter__, and tuples\n> > > > and lists already do, then:\n> > > >\n> > > > x,y = pos\n> > > > w,h = size\n> > > >\n> > > > will work for all those cases.\n> > > >\n> > > > And looking at the transforming methods, e.g.:\n> > > >\n> > > > rect.scale_by(Size(1, 2), Size(3,4)).translate_by(Point(5,6))\n> > > >\n> > > > is a bit annoying to use, compared to:\n> > > >\n> > > > rect.scale_by((1, 2), (3,4)).translate_by((5,6))\n> > > >\n> > > > To fix that, I think we would have to overwrite all the methods we want\n> > > > to support such conversions. Which does not sound nice.\n> > > >\n> > > > I did some testing in the __init__.py, and I think we can (re-)implement\n> > > > anything we need there. E.g.:\n> > > >\n> > > > def __Size_iter(self):\n> > > > yield from (self.width, self.height)\n> > > >\n> > > > Size.__iter__ = __Size_iter\n> > > >\n> > > >\n> > > > and\n> > > >\n> > > >\n> > > > def __Rectangle_init(self, pos=(0, 0), size=(0, 0)):\n> > > > x, y = pos\n> > > > w, h = size\n> > > > Rectangle.__old_init(self, x, y, w, h)\n> > > >\n> > > > Rectangle.__old_init = Rectangle.__init__\n> > > > Rectangle.__init__ = __Rectangle_init\n> > > >\n> > > > > In C++ it's a bit different thanks to implicit constructors and\n> > > > > initializer lists, so I agree that we may want to expose a similar\n> > > > > feature manually to make the code more readable. I'm just concerned\n> > > > > about the possible large number of combinations.\n> > > > >\n> > > > > Is there a Python coding style rule (or rather API design rule) about\n> > > > > this ?\n> > > >\n> > > > I'm not aware of such. In my experience python classes/functions often\n> > > > take a wide range of different inputs, and they do what you expect them\n> > > > to do. I don't know if that's recommended or is it just just something\n> > > > that the authors have implemented because they liked it.","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 A4317C0F2A\n\tfor ;\n\tTue, 17 May 2022 16:24:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C5A86041E;\n\tTue, 17 May 2022 18:24:19 +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 556E26041D\n\tfor ;\n\tTue, 17 May 2022 18:24:17 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B34F348F;\n\tTue, 17 May 2022 18:24:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652804659;\n\tbh=4+U5KSwhmNvgAbe5Tz/A42KxSJ9jSC41XZollI5u7i4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=RW/MMdpW/IWtLVXZICYaBVyAlDH1VVDzgeCkqDUzcodNzwqlyPfGfS0LkywSyfSY5\n\toh1GlR1IUMBBPSk1VF9UxyQBGTvdTGUqlJl3qMEoNPXECjPUFhdEGP/K+d+v2fw+tg\n\thCZ8L9UR9U/squ+m/LqXyOMUa6TAfZkhQF5kyjm98vGBuDDfiArgFEsQ9KG1+Tq9l1\n\tc0sGipxWR+RhvG7OnyXiq6xZERTijsN1CG2LH2iel7eRyWDZ2FjWWsV/38HRo7XKBR\n\tPryxncNEi9zVkE2DQzjdvSD8Rfp+wVgK6vzODjhg0YwcOB+U+tAYPScHZNrb+JKsAu\n\tRMuGQPsFIbOKg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652804657;\n\tbh=4+U5KSwhmNvgAbe5Tz/A42KxSJ9jSC41XZollI5u7i4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i2QzEr0tCksMoxIxRhyXOcMdEmoSDKcn0WuHxre1nlQHRLchQtsLTK9RMEMQrk60o\n\t2AJD2FBC9a8GIMHLQ+PTkznyi1hI8X7idXstzMg6/iq4IpHLt8F2etFWPOJCAHE8gX\n\tVTZTIZ3OaeRKrn/I3TCIR0L4fbVNBiMW7G7vJfno="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"i2QzEr0t\"; dkim-atps=neutral","Date":"Tue, 17 May 2022 19:24:09 +0300","To":"David Plowman ","Message-ID":"","References":"<20220516141022.96327-1-tomi.valkeinen@ideasonboard.com>\n\t<20220516141022.96327-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/14] py: add geometry classes","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]