[{"id":24647,"web_url":"https://patchwork.libcamera.org/comment/24647/","msgid":"<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","date":"2022-08-18T14:22:30","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomi\n\nOn Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote:\n> Wrap the CameraManager with a PyCameraManager class and move the related\n> code inside the new class.\n>\n> This helps understanding the life times of the used-to-be global\n> variables, gets rid of static handleRequestCompleted function, and\n> allows us to simplify the binding code as the more complex pieces are\n> inside the class.\n>\n> There should be no user visible functional changes.\n>\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 1 +\n> src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++\n> src/py/libcamera/py_camera_manager.h | 44 +++++++++\n> src/py/libcamera/py_main.cpp | 117 +++++------------------\n> 4 files changed, 193 insertions(+), 94 deletions(-)\n> create mode 100644 src/py/libcamera/py_camera_manager.cpp\n> create mode 100644 src/py/libcamera/py_camera_manager.h\n>\n> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> index 04578bac..ad2a6858 100644\n> --- a/src/py/libcamera/meson.build\n> +++ b/src/py/libcamera/meson.build\n> @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11')\n> pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>\n> pycamera_sources = files([\n> + 'py_camera_manager.cpp',\n> 'py_enums.cpp',\n> 'py_geometry.cpp',\n> 'py_helpers.cpp',\n> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> new file mode 100644\n> index 00000000..ad47271d\n> --- /dev/null\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -0,0 +1,125 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Tomi Valkeinen \n> + */\n> +\n> +#include \"py_camera_manager.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"py_main.h\"\n> +\n> +namespace py = pybind11;\n> +\n> +using namespace libcamera;\n> +\n> +PyCameraManager::PyCameraManager()\n> +{\n> +\tLOG(Python, Debug) << \"PyCameraManager()\";\n\nDon't you need to LOG_DECLARE_CATEGORY() ?\n\n> +\n> +\tcameraManager_ = std::make_unique();\n> +\n> +\tint fd = eventfd(0, 0);\n> +\tif (fd == -1)\n> +\t\tthrow std::system_error(errno, std::generic_category(),\n> +\t\t\t\t\t\"Failed to create eventfd\");\n> +\n> +\teventFd_ = fd;\n\nYou can use UniqueFD() to avoid manually handling the eventfd file\ndescriptor\n\n> +\n> +\tint ret = cameraManager_->start();\n> +\tif (ret) {\n> +\t\tclose(fd);\n> +\t\teventFd_ = -1;\n> +\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\"Failed to start CameraManager\");\n> +\t}\n> +}\n> +\n> +PyCameraManager::~PyCameraManager()\n> +{\n> +\tLOG(Python, Debug) << \"~PyCameraManager()\";\n> +\n> +\tif (eventFd_ != -1) {\n> +\t\tclose(eventFd_);\n> +\t\teventFd_ = -1;\n> +\t}\n> +}\n> +\n> +py::list PyCameraManager::cameras()\n> +{\n> +\t/*\n> +\t * Create a list of Cameras, where each camera has a keep-alive to\n> +\t * CameraManager.\n> +\t */\n> +\tpy::list l;\n> +\n> +\tfor (auto &camera : cameraManager_->cameras()) {\n> +\t\tpy::object py_cm = py::cast(this);\n> +\t\tpy::object py_cam = py::cast(camera);\n> +\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> +\t\tl.append(py_cam);\n> +\t}\n> +\n> +\treturn l;\n\nI was about to suggest creating the list at creation time and cache\nit. But indeed cameras can be added or removed from the system\n\n> +}\n> +\n> +std::vector PyCameraManager::getReadyRequests()\n> +{\n> +\treadFd();\n> +\n> +\tstd::vector ret;\n> +\n> +\tfor (Request *request : getCompletedRequests()) {\n> +\t\tpy::object o = py::cast(request);\n> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> +\t\to.dec_ref();\n> +\t\tret.push_back(o);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleRequestCompleted(Request *req)\n> +{\n> +\tpushRequest(req);\n\nIs this function only used here ?\nShould it be inlined ?\n\n> +\twriteFd();\n> +}\n> +\n> +void PyCameraManager::writeFd()\n> +{\n> +\tuint64_t v = 1;\n> +\n> +\tsize_t s = write(eventFd_, &v, 8);\n\nAny reason to write an 8 byte uint64 ?\nJust curious\n\n> +\t/*\n> +\t * We should never fail, and have no simple means to manage the error,\n> +\t * so let's log a fatal error.\n> +\t */\n> +\tif (s != 8)\n> +\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> +}\n> +\n> +void PyCameraManager::readFd()\n> +{\n> +\tuint8_t buf[8];\n> +\n> +\tif (read(eventFd_, buf, 8) != 8)\n> +\t\tthrow std::system_error(errno, std::generic_category());\n> +}\n> +\n> +void PyCameraManager::pushRequest(Request *req)\n> +{\n> +\tstd::lock_guard guard(completedRequestsMutex_);\n> +\tcompletedRequests_.push_back(req);\n> +}\n> +\n> +std::vector PyCameraManager::getCompletedRequests()\n> +{\n> +\tstd::vector v;\n> +\tstd::lock_guard guard(completedRequestsMutex_);\n> +\tswap(v, completedRequests_);\n> +\treturn v;\n> +}\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> new file mode 100644\n> index 00000000..9c15f814\n> --- /dev/null\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Tomi Valkeinen \n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +class PyCameraManager\n> +{\n> +public:\n> +\tPyCameraManager();\n> +\t~PyCameraManager();\n> +\n> +\tpybind11::list cameras();\n> +\tstd::shared_ptr get(const std::string &name) { return cameraManager_->get(name); }\n\nDo you need to include ?\n> +\n> +\tstatic const std::string &version() { return CameraManager::version(); }\n\nDo you need to include \n\nShould this method be marked as const ?\n\n> +\n> +\tint eventFd() const { return eventFd_; }\n> +\n> +\tstd::vector getReadyRequests();\n\nDo you need to include vector ?\n> +\n> +\tvoid handleRequestCompleted(Request *req);\n> +\n> +private:\n> +\tstd::unique_ptr cameraManager_;\n> +\n> +\tint eventFd_ = -1;\n> +\tstd::mutex completedRequestsMutex_;\n> +\tstd::vector completedRequests_;\n> +\n> +\tvoid writeFd();\n> +\tvoid readFd();\n> +\tvoid pushRequest(Request *req);\n> +\tstd::vector getCompletedRequests();\n> +};\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index e652837f..e7d078b5 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -7,10 +7,7 @@\n>\n> #include \"py_main.h\"\n>\n> -#include \n> #include \n> -#include \n> -#include \n>\n> #include \n>\n> @@ -21,6 +18,7 @@\n> #include \n> #include \n>\n> +#include \"py_camera_manager.h\"\n> #include \"py_helpers.h\"\n>\n> namespace py = pybind11;\n> @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python)\n>\n> }\n>\n> -static std::weak_ptr gCameraManager;\n> -static int gEventfd;\n> -static std::mutex gReqlistMutex;\n> -static std::vector gReqList;\n> -\n> -static void handleRequestCompleted(Request *req)\n> -{\n> -\t{\n> -\t\tstd::lock_guard guard(gReqlistMutex);\n> -\t\tgReqList.push_back(req);\n> -\t}\n> -\n> -\tuint64_t v = 1;\n> -\tsize_t s = write(gEventfd, &v, 8);\n> -\t/*\n> -\t * We should never fail, and have no simple means to manage the error,\n> -\t * so let's log a fatal error.\n> -\t */\n> -\tif (s != 8)\n> -\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> -}\n> +/*\n> + * Note: global C++ destructors can be ran on this before the py module is\n> + * destructed.\n> + */\n> +static std::weak_ptr gCameraManager;\n>\n> void init_py_enums(py::module &m);\n> void init_py_controls_generated(py::module &m);\n> @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> \t */\n>\n> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> +\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> \tauto pyCamera = py::class_(m, \"Camera\");\n> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m)\n> \t/* Classes */\n> \tpyCameraManager\n> \t\t.def_static(\"singleton\", []() {\n> -\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> -\t\t\tif (cm)\n> -\t\t\t\treturn cm;\n> -\n> -\t\t\tint fd = eventfd(0, 0);\n> -\t\t\tif (fd == -1)\n> -\t\t\t\tthrow std::system_error(errno, std::generic_category(),\n> -\t\t\t\t\t\t\t\"Failed to create eventfd\");\n> -\n> -\t\t\tcm = std::shared_ptr(new CameraManager, [](auto p) {\n> -\t\t\t\tclose(gEventfd);\n> -\t\t\t\tgEventfd = -1;\n> -\t\t\t\tdelete p;\n> -\t\t\t});\n> -\n> -\t\t\tgEventfd = fd;\n> -\t\t\tgCameraManager = cm;\n> -\n> -\t\t\tint ret = cm->start();\n> -\t\t\tif (ret)\n> -\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> -\t\t\t\t\t\t\t\"Failed to start CameraManager\");\n> -\n> -\t\t\treturn cm;\n> -\t\t})\n> -\n> -\t\t.def_property_readonly(\"version\", &CameraManager::version)\n> +\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n>\n> -\t\t.def_property_readonly(\"event_fd\", [](CameraManager &) {\n> -\t\t\treturn gEventfd;\n> -\t\t})\n> -\n> -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> -\t\t\tuint8_t buf[8];\n> -\n> -\t\t\tif (read(gEventfd, buf, 8) != 8)\n> -\t\t\t\tthrow std::system_error(errno, std::generic_category());\n> -\n> -\t\t\tstd::vector v;\n> -\n> -\t\t\t{\n> -\t\t\t\tstd::lock_guard guard(gReqlistMutex);\n> -\t\t\t\tswap(v, gReqList);\n> +\t\t\tif (!cm) {\n> +\t\t\t\tcm = std::make_shared();\n> +\t\t\t\tgCameraManager = cm;\n\nWhat is the advantage of using a weak_ptr<> here ? Can't this be kept\nas a shared_ptr ?\n\n> \t\t\t}\n>\n> -\t\t\tstd::vector ret;\n> -\n> -\t\t\tfor (Request *req : v) {\n> -\t\t\t\tpy::object o = py::cast(req);\n> -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> -\t\t\t\to.dec_ref();\n> -\t\t\t\tret.push_back(o);\n> -\t\t\t}\n> -\n> -\t\t\treturn ret;\n> +\t\t\treturn cm;\n> \t\t})\n>\n> -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> -\n> -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> -\t\t\tpy::list l;\n> -\n> -\t\t\tfor (auto &c : self.cameras()) {\n> -\t\t\t\tpy::object py_cm = py::cast(self);\n> -\t\t\t\tpy::object py_cam = py::cast(c);\n> -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> -\t\t\t\tl.append(py_cam);\n> -\t\t\t}\n> +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n>\n> -\t\t\treturn l;\n> -\t\t});\n> +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n\nIs access to eventfs useful for applications ?\n\n> +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n>\n> \tpyCamera\n> \t\t.def_property_readonly(\"id\", &Camera::id)\n> @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t const std::unordered_map &controls) {\n> \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n>\n> -\t\t\tself.requestCompleted.connect(handleRequestCompleted);\n> +\t\t\tauto cm = gCameraManager.lock();\n> +\t\t\tASSERT(cm);\n> +\n> +\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n>\n> \t\t\tControlList controlList(self.controls());\n>\n> @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m)\n>\n> \t\t\tint ret = self.start(&controlList);\n> \t\t\tif (ret) {\n> -\t\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> +\t\t\t\tself.requestCompleted.disconnect();\n> \t\t\t\treturn ret;\n> \t\t\t}\n>\n> @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\tif (ret)\n> \t\t\t\treturn ret;\n>\n> -\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> +\t\t\tself.requestCompleted.disconnect();\n>\n> \t\t\treturn 0;\n> \t\t})\n> --\n> 2.34.1\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 E7DC4C3272\n\tfor ;\n\tThu, 18 Aug 2022 14:22:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6EE2861FBC;\n\tThu, 18 Aug 2022 16:22:37 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D192D61FA7\n\tfor ;\n\tThu, 18 Aug 2022 16:22:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E9190200003;\n\tThu, 18 Aug 2022 14:22:32 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660832557;\n\tbh=CJuyigY/RyOlJzyrftQRnd/RitWVJM0Iqed8f3GjiH0=;\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=W8ew9CGskzg2tQBIaf84qwtdRiy0rYOQtRSJL+51Y4Zb7f1CeInWT2EIOLxetva7q\n\tvhV3IOrFVjcmQ3Rek4Sl0SfTeaTVkyrEzj5XSMm9G3l4Ck19rUs2l2X4uvwhbyznnP\n\t5bkl0XcEd2f8jh2IYT/10sUizoB4widtQMeXYeEGSgV6ee5exh8tXAvI9/LBIsBoAS\n\txtAUJHPENvCRx+SXM26kiYOkI8N2kE08+0NhnSNBaJ9AcNo3eabrPyJd3wXLlvH56Z\n\tF534mRZg6cx2i/0IAY6i0KcIVbnQUdq8iHSWGBIdqKBEO+g3Ty0l12O6XJ6hCk2xP5\n\t60cUqAnAbgWmg==","Date":"Thu, 18 Aug 2022 16:22:30 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24654,"web_url":"https://patchwork.libcamera.org/comment/24654/","msgid":"<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>","date":"2022-08-18T14:39:18","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"Hi,\n\nOn 18/08/2022 17:22, Jacopo Mondi wrote:\n> Hi Tomi\n> \n> On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote:\n>> Wrap the CameraManager with a PyCameraManager class and move the related\n>> code inside the new class.\n>>\n>> This helps understanding the life times of the used-to-be global\n>> variables, gets rid of static handleRequestCompleted function, and\n>> allows us to simplify the binding code as the more complex pieces are\n>> inside the class.\n>>\n>> There should be no user visible functional changes.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/meson.build | 1 +\n>> src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++\n>> src/py/libcamera/py_camera_manager.h | 44 +++++++++\n>> src/py/libcamera/py_main.cpp | 117 +++++------------------\n>> 4 files changed, 193 insertions(+), 94 deletions(-)\n>> create mode 100644 src/py/libcamera/py_camera_manager.cpp\n>> create mode 100644 src/py/libcamera/py_camera_manager.h\n>>\n>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n>> index 04578bac..ad2a6858 100644\n>> --- a/src/py/libcamera/meson.build\n>> +++ b/src/py/libcamera/meson.build\n>> @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11')\n>> pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n>>\n>> pycamera_sources = files([\n>> + 'py_camera_manager.cpp',\n>> 'py_enums.cpp',\n>> 'py_geometry.cpp',\n>> 'py_helpers.cpp',\n>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>> new file mode 100644\n>> index 00000000..ad47271d\n>> --- /dev/null\n>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>> @@ -0,0 +1,125 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2022, Tomi Valkeinen \n>> + */\n>> +\n>> +#include \"py_camera_manager.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"py_main.h\"\n>> +\n>> +namespace py = pybind11;\n>> +\n>> +using namespace libcamera;\n>> +\n>> +PyCameraManager::PyCameraManager()\n>> +{\n>> +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> \n> Don't you need to LOG_DECLARE_CATEGORY() ?\n\nIt's in py_main.h.\n\n> \n>> +\n>> +\tcameraManager_ = std::make_unique();\n>> +\n>> +\tint fd = eventfd(0, 0);\n>> +\tif (fd == -1)\n>> +\t\tthrow std::system_error(errno, std::generic_category(),\n>> +\t\t\t\t\t\"Failed to create eventfd\");\n>> +\n>> +\teventFd_ = fd;\n> \n> You can use UniqueFD() to avoid manually handling the eventfd file\n> descriptor\n\nYes, in the next patch.\n\n>> +\n>> +\tint ret = cameraManager_->start();\n>> +\tif (ret) {\n>> +\t\tclose(fd);\n>> +\t\teventFd_ = -1;\n>> +\t\tthrow std::system_error(-ret, std::generic_category(),\n>> +\t\t\t\t\t\"Failed to start CameraManager\");\n>> +\t}\n>> +}\n>> +\n>> +PyCameraManager::~PyCameraManager()\n>> +{\n>> +\tLOG(Python, Debug) << \"~PyCameraManager()\";\n>> +\n>> +\tif (eventFd_ != -1) {\n>> +\t\tclose(eventFd_);\n>> +\t\teventFd_ = -1;\n>> +\t}\n>> +}\n>> +\n>> +py::list PyCameraManager::cameras()\n>> +{\n>> +\t/*\n>> +\t * Create a list of Cameras, where each camera has a keep-alive to\n>> +\t * CameraManager.\n>> +\t */\n>> +\tpy::list l;\n>> +\n>> +\tfor (auto &camera : cameraManager_->cameras()) {\n>> +\t\tpy::object py_cm = py::cast(this);\n>> +\t\tpy::object py_cam = py::cast(camera);\n>> +\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n>> +\t\tl.append(py_cam);\n>> +\t}\n>> +\n>> +\treturn l;\n> \n> I was about to suggest creating the list at creation time and cache\n> it. But indeed cameras can be added or removed from the system\n> \n>> +}\n>> +\n>> +std::vector PyCameraManager::getReadyRequests()\n>> +{\n>> +\treadFd();\n>> +\n>> +\tstd::vector ret;\n>> +\n>> +\tfor (Request *request : getCompletedRequests()) {\n>> +\t\tpy::object o = py::cast(request);\n>> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n>> +\t\to.dec_ref();\n>> +\t\tret.push_back(o);\n>> +\t}\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleRequestCompleted(Request *req)\n>> +{\n>> +\tpushRequest(req);\n> \n> Is this function only used here ?\n> Should it be inlined ?\n\npushRequest? I like it separately, as it uses the \ncompletedRequestsMutex_. It makes the code nice and clean as pushRequest \nand getCompletedRequests are small funcs and use completedRequestsMutex_.\n\nBut these will be reworked in the new events support patches, so even if \ninlining would be nicer, I'd rather not do changes here that do not fix \nanything.\n\n>> +\twriteFd();\n>> +}\n>> +\n>> +void PyCameraManager::writeFd()\n>> +{\n>> +\tuint64_t v = 1;\n>> +\n>> +\tsize_t s = write(eventFd_, &v, 8);\n> \n> Any reason to write an 8 byte uint64 ?\n> Just curious\n\nThat's how eventfd works, it passes 64bit numbers.\n\n>> +\t/*\n>> +\t * We should never fail, and have no simple means to manage the error,\n>> +\t * so let's log a fatal error.\n>> +\t */\n>> +\tif (s != 8)\n>> +\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n>> +}\n>> +\n>> +void PyCameraManager::readFd()\n>> +{\n>> +\tuint8_t buf[8];\n>> +\n>> +\tif (read(eventFd_, buf, 8) != 8)\n>> +\t\tthrow std::system_error(errno, std::generic_category());\n>> +}\n>> +\n>> +void PyCameraManager::pushRequest(Request *req)\n>> +{\n>> +\tstd::lock_guard guard(completedRequestsMutex_);\n>> +\tcompletedRequests_.push_back(req);\n>> +}\n>> +\n>> +std::vector PyCameraManager::getCompletedRequests()\n>> +{\n>> +\tstd::vector v;\n>> +\tstd::lock_guard guard(completedRequestsMutex_);\n>> +\tswap(v, completedRequests_);\n>> +\treturn v;\n>> +}\n>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n>> new file mode 100644\n>> index 00000000..9c15f814\n>> --- /dev/null\n>> +++ b/src/py/libcamera/py_camera_manager.h\n>> @@ -0,0 +1,44 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2022, Tomi Valkeinen \n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +using namespace libcamera;\n>> +\n>> +class PyCameraManager\n>> +{\n>> +public:\n>> +\tPyCameraManager();\n>> +\t~PyCameraManager();\n>> +\n>> +\tpybind11::list cameras();\n>> +\tstd::shared_ptr get(const std::string &name) { return cameraManager_->get(name); }\n> \n> Do you need to include ?\n\nI guess it's safer to include (the same goes for the other include \nsuggestions below).\n\n>> +\n>> +\tstatic const std::string &version() { return CameraManager::version(); }\n> \n> Do you need to include \n> \n> Should this method be marked as const ?\n\nIt's a static method.\n\n>> +\n>> +\tint eventFd() const { return eventFd_; }\n>> +\n>> +\tstd::vector getReadyRequests();\n> \n> Do you need to include vector ?\n>> +\n>> +\tvoid handleRequestCompleted(Request *req);\n>> +\n>> +private:\n>> +\tstd::unique_ptr cameraManager_;\n>> +\n>> +\tint eventFd_ = -1;\n>> +\tstd::mutex completedRequestsMutex_;\n>> +\tstd::vector completedRequests_;\n>> +\n>> +\tvoid writeFd();\n>> +\tvoid readFd();\n>> +\tvoid pushRequest(Request *req);\n>> +\tstd::vector getCompletedRequests();\n>> +};\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index e652837f..e7d078b5 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -7,10 +7,7 @@\n>>\n>> #include \"py_main.h\"\n>>\n>> -#include \n>> #include \n>> -#include \n>> -#include \n>>\n>> #include \n>>\n>> @@ -21,6 +18,7 @@\n>> #include \n>> #include \n>>\n>> +#include \"py_camera_manager.h\"\n>> #include \"py_helpers.h\"\n>>\n>> namespace py = pybind11;\n>> @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python)\n>>\n>> }\n>>\n>> -static std::weak_ptr gCameraManager;\n>> -static int gEventfd;\n>> -static std::mutex gReqlistMutex;\n>> -static std::vector gReqList;\n>> -\n>> -static void handleRequestCompleted(Request *req)\n>> -{\n>> -\t{\n>> -\t\tstd::lock_guard guard(gReqlistMutex);\n>> -\t\tgReqList.push_back(req);\n>> -\t}\n>> -\n>> -\tuint64_t v = 1;\n>> -\tsize_t s = write(gEventfd, &v, 8);\n>> -\t/*\n>> -\t * We should never fail, and have no simple means to manage the error,\n>> -\t * so let's log a fatal error.\n>> -\t */\n>> -\tif (s != 8)\n>> -\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n>> -}\n>> +/*\n>> + * Note: global C++ destructors can be ran on this before the py module is\n>> + * destructed.\n>> + */\n>> +static std::weak_ptr gCameraManager;\n>>\n>> void init_py_enums(py::module &m);\n>> void init_py_controls_generated(py::module &m);\n>> @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n>> \t */\n>>\n>> -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n>> +\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n>> \tauto pyCamera = py::class_(m, \"Camera\");\n>> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n>> \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n>> @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t/* Classes */\n>> \tpyCameraManager\n>> \t\t.def_static(\"singleton\", []() {\n>> -\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n>> -\t\t\tif (cm)\n>> -\t\t\t\treturn cm;\n>> -\n>> -\t\t\tint fd = eventfd(0, 0);\n>> -\t\t\tif (fd == -1)\n>> -\t\t\t\tthrow std::system_error(errno, std::generic_category(),\n>> -\t\t\t\t\t\t\t\"Failed to create eventfd\");\n>> -\n>> -\t\t\tcm = std::shared_ptr(new CameraManager, [](auto p) {\n>> -\t\t\t\tclose(gEventfd);\n>> -\t\t\t\tgEventfd = -1;\n>> -\t\t\t\tdelete p;\n>> -\t\t\t});\n>> -\n>> -\t\t\tgEventfd = fd;\n>> -\t\t\tgCameraManager = cm;\n>> -\n>> -\t\t\tint ret = cm->start();\n>> -\t\t\tif (ret)\n>> -\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>> -\t\t\t\t\t\t\t\"Failed to start CameraManager\");\n>> -\n>> -\t\t\treturn cm;\n>> -\t\t})\n>> -\n>> -\t\t.def_property_readonly(\"version\", &CameraManager::version)\n>> +\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n>>\n>> -\t\t.def_property_readonly(\"event_fd\", [](CameraManager &) {\n>> -\t\t\treturn gEventfd;\n>> -\t\t})\n>> -\n>> -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n>> -\t\t\tuint8_t buf[8];\n>> -\n>> -\t\t\tif (read(gEventfd, buf, 8) != 8)\n>> -\t\t\t\tthrow std::system_error(errno, std::generic_category());\n>> -\n>> -\t\t\tstd::vector v;\n>> -\n>> -\t\t\t{\n>> -\t\t\t\tstd::lock_guard guard(gReqlistMutex);\n>> -\t\t\t\tswap(v, gReqList);\n>> +\t\t\tif (!cm) {\n>> +\t\t\t\tcm = std::make_shared();\n>> +\t\t\t\tgCameraManager = cm;\n> \n> What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> as a shared_ptr ?\n\nshared_ptr keeps it alive. We don't want to keep it alive. It's the job \nof the application to keep it alive (as long as it uses it).\n\n>> \t\t\t}\n>>\n>> -\t\t\tstd::vector ret;\n>> -\n>> -\t\t\tfor (Request *req : v) {\n>> -\t\t\t\tpy::object o = py::cast(req);\n>> -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n>> -\t\t\t\to.dec_ref();\n>> -\t\t\t\tret.push_back(o);\n>> -\t\t\t}\n>> -\n>> -\t\t\treturn ret;\n>> +\t\t\treturn cm;\n>> \t\t})\n>>\n>> -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n>> -\n>> -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n>> -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n>> -\t\t\tpy::list l;\n>> -\n>> -\t\t\tfor (auto &c : self.cameras()) {\n>> -\t\t\t\tpy::object py_cm = py::cast(self);\n>> -\t\t\t\tpy::object py_cam = py::cast(c);\n>> -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n>> -\t\t\t\tl.append(py_cam);\n>> -\t\t\t}\n>> +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n>> +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n>> +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n>>\n>> -\t\t\treturn l;\n>> -\t\t});\n>> +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> \n> Is access to eventfs useful for applications ?\n\nYes, that's how they wait for events.\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 3C01EBE173\n\tfor ;\n\tThu, 18 Aug 2022 14:39:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BBD961FC0;\n\tThu, 18 Aug 2022 16:39: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 3AEA861FA7\n\tfor ;\n\tThu, 18 Aug 2022 16:39:21 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D3438B;\n\tThu, 18 Aug 2022 16:39:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660833562;\n\tbh=rOhks6u1eP35BMijJO4aX2gnZEFu8ojl1lP/X/4Rt7E=;\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=ycuU5n9cQqiQ7VJbzjKgLb/BpzZ4VDCxB0aBKJr+0e5uB7HQDCmQi0EBPZz7Rn3es\n\tN0ftUNQU4NjxNEQP56mgRDg/pPq+bcH2tveHpobYA488Wf5GD4btPV/mskV7SIGBT8\n\tdvAzN9a74K3sAvsD03v/A3gxK5jrQWzID2nXMfRKFRH/CSPvzHXrMQExz17QKTHEms\n\to3LJJQyEd7H4IdCNDqQEPHasiDQRvbcIE2iKRsTGot39hbEjpL+ZQc0m49lT0jBxRH\n\tO/+Xb5GJVe0k8f/RhTUbl3Wd138akp+XQHqi0qqllzRAf6rqZIpofTaJcqn/qti128\n\t96/1BdQFdERDQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660833560;\n\tbh=rOhks6u1eP35BMijJO4aX2gnZEFu8ojl1lP/X/4Rt7E=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=JGV7IHLoYCm4mO5nW8MVeSvFUBeavUmh+FXFanvKOdI0GWqHU2+qRL8ajV0ggU6In\n\tkAZ9pva0qcJmn6sktFIWXjahGnCZDJFv+ORpeb5Dp6da5FAuVdGC0DQUGCLvlT7Eqd\n\tPl+LqtuLUohxgWHFJKM8sDuoEfMT+ojAvsHVWP2I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JGV7IHLo\"; dkim-atps=neutral","Message-ID":"<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>","Date":"Thu, 18 Aug 2022 17:39:18 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","In-Reply-To":"<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":24660,"web_url":"https://patchwork.libcamera.org/comment/24660/","msgid":"<20220818150017.4igyj6lyjrch7obh@uno.localdomain>","date":"2022-08-18T15:00:17","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomi\n\nOn Thu, Aug 18, 2022 at 05:39:18PM +0300, Tomi Valkeinen wrote:\n> Hi,\n>\n> On 18/08/2022 17:22, Jacopo Mondi wrote:\n> > Hi Tomi\n> >\n> > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote:\n> > > Wrap the CameraManager with a PyCameraManager class and move the related\n> > > code inside the new class.\n> > >\n> > > This helps understanding the life times of the used-to-be global\n> > > variables, gets rid of static handleRequestCompleted function, and\n> > > allows us to simplify the binding code as the more complex pieces are\n> > > inside the class.\n> > >\n> > > There should be no user visible functional changes.\n> > >\n> > > Signed-off-by: Tomi Valkeinen \n> > > ---\n> > > src/py/libcamera/meson.build | 1 +\n> > > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++\n> > > src/py/libcamera/py_camera_manager.h | 44 +++++++++\n> > > src/py/libcamera/py_main.cpp | 117 +++++------------------\n> > > 4 files changed, 193 insertions(+), 94 deletions(-)\n> > > create mode 100644 src/py/libcamera/py_camera_manager.cpp\n> > > create mode 100644 src/py/libcamera/py_camera_manager.h\n> > >\n> > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> > > index 04578bac..ad2a6858 100644\n> > > --- a/src/py/libcamera/meson.build\n> > > +++ b/src/py/libcamera/meson.build\n> > > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11')\n> > > pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> > >\n> > > pycamera_sources = files([\n> > > + 'py_camera_manager.cpp',\n> > > 'py_enums.cpp',\n> > > 'py_geometry.cpp',\n> > > 'py_helpers.cpp',\n> > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> > > new file mode 100644\n> > > index 00000000..ad47271d\n> > > --- /dev/null\n> > > +++ b/src/py/libcamera/py_camera_manager.cpp\n> > > @@ -0,0 +1,125 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Tomi Valkeinen \n> > > + */\n> > > +\n> > > +#include \"py_camera_manager.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"py_main.h\"\n> > > +\n> > > +namespace py = pybind11;\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +PyCameraManager::PyCameraManager()\n> > > +{\n> > > +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> >\n> > Don't you need to LOG_DECLARE_CATEGORY() ?\n>\n> It's in py_main.h.\n>\n\nAh, I overlooked the first patches as they were reviewed already.\nWell, that header only serves for that purpose.\n\nI would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()\nand spare that header.\n\n> >\n> > > +\n> > > +\tcameraManager_ = std::make_unique();\n> > > +\n> > > +\tint fd = eventfd(0, 0);\n> > > +\tif (fd == -1)\n> > > +\t\tthrow std::system_error(errno, std::generic_category(),\n> > > +\t\t\t\t\t\"Failed to create eventfd\");\n> > > +\n> > > +\teventFd_ = fd;\n> >\n> > You can use UniqueFD() to avoid manually handling the eventfd file\n> > descriptor\n>\n> Yes, in the next patch.\n>\n> > > +\n> > > +\tint ret = cameraManager_->start();\n> > > +\tif (ret) {\n> > > +\t\tclose(fd);\n> > > +\t\teventFd_ = -1;\n> > > +\t\tthrow std::system_error(-ret, std::generic_category(),\n> > > +\t\t\t\t\t\"Failed to start CameraManager\");\n> > > +\t}\n> > > +}\n> > > +\n> > > +PyCameraManager::~PyCameraManager()\n> > > +{\n> > > +\tLOG(Python, Debug) << \"~PyCameraManager()\";\n> > > +\n> > > +\tif (eventFd_ != -1) {\n> > > +\t\tclose(eventFd_);\n> > > +\t\teventFd_ = -1;\n> > > +\t}\n> > > +}\n> > > +\n> > > +py::list PyCameraManager::cameras()\n> > > +{\n> > > +\t/*\n> > > +\t * Create a list of Cameras, where each camera has a keep-alive to\n> > > +\t * CameraManager.\n> > > +\t */\n> > > +\tpy::list l;\n> > > +\n> > > +\tfor (auto &camera : cameraManager_->cameras()) {\n> > > +\t\tpy::object py_cm = py::cast(this);\n> > > +\t\tpy::object py_cam = py::cast(camera);\n> > > +\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > +\t\tl.append(py_cam);\n> > > +\t}\n> > > +\n> > > +\treturn l;\n> >\n> > I was about to suggest creating the list at creation time and cache\n> > it. But indeed cameras can be added or removed from the system\n> >\n> > > +}\n> > > +\n> > > +std::vector PyCameraManager::getReadyRequests()\n> > > +{\n> > > +\treadFd();\n> > > +\n> > > +\tstd::vector ret;\n> > > +\n> > > +\tfor (Request *request : getCompletedRequests()) {\n> > > +\t\tpy::object o = py::cast(request);\n> > > +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > +\t\to.dec_ref();\n> > > +\t\tret.push_back(o);\n> > > +\t}\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +/* Note: Called from another thread */\n> > > +void PyCameraManager::handleRequestCompleted(Request *req)\n> > > +{\n> > > +\tpushRequest(req);\n> >\n> > Is this function only used here ?\n> > Should it be inlined ?\n>\n> pushRequest? I like it separately, as it uses the completedRequestsMutex_.\n> It makes the code nice and clean as pushRequest and getCompletedRequests are\n> small funcs and use completedRequestsMutex_.\n>\n\n> But these will be reworked in the new events support patches, so even if\n> inlining would be nicer, I'd rather not do changes here that do not fix\n> anything.\n>\n\nok..\n\n> > > +\twriteFd();\n> > > +}\n> > > +\n> > > +void PyCameraManager::writeFd()\n> > > +{\n> > > +\tuint64_t v = 1;\n> > > +\n> > > +\tsize_t s = write(eventFd_, &v, 8);\n> >\n> > Any reason to write an 8 byte uint64 ?\n> > Just curious\n>\n> That's how eventfd works, it passes 64bit numbers.\n>\n\nAh sorry, missed that :)\n\n> > > +\t/*\n> > > +\t * We should never fail, and have no simple means to manage the error,\n> > > +\t * so let's log a fatal error.\n> > > +\t */\n> > > +\tif (s != 8)\n> > > +\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > > +}\n> > > +\n> > > +void PyCameraManager::readFd()\n> > > +{\n> > > +\tuint8_t buf[8];\n> > > +\n> > > +\tif (read(eventFd_, buf, 8) != 8)\n> > > +\t\tthrow std::system_error(errno, std::generic_category());\n> > > +}\n> > > +\n> > > +void PyCameraManager::pushRequest(Request *req)\n> > > +{\n> > > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > > +\tcompletedRequests_.push_back(req);\n> > > +}\n> > > +\n> > > +std::vector PyCameraManager::getCompletedRequests()\n> > > +{\n> > > +\tstd::vector v;\n> > > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > > +\tswap(v, completedRequests_);\n> > > +\treturn v;\n> > > +}\n> > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> > > new file mode 100644\n> > > index 00000000..9c15f814\n> > > --- /dev/null\n> > > +++ b/src/py/libcamera/py_camera_manager.h\n> > > @@ -0,0 +1,44 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Tomi Valkeinen \n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +class PyCameraManager\n> > > +{\n> > > +public:\n> > > +\tPyCameraManager();\n> > > +\t~PyCameraManager();\n> > > +\n> > > +\tpybind11::list cameras();\n> > > +\tstd::shared_ptr get(const std::string &name) { return cameraManager_->get(name); }\n> >\n> > Do you need to include ?\n>\n> I guess it's safer to include (the same goes for the other include\n> suggestions below).\n>\n> > > +\n> > > +\tstatic const std::string &version() { return CameraManager::version(); }\n> >\n> > Do you need to include \n> >\n> > Should this method be marked as const ?\n>\n> It's a static method.\n>\n\nRight, missed that\n\n> > > +\n> > > +\tint eventFd() const { return eventFd_; }\n> > > +\n> > > +\tstd::vector getReadyRequests();\n> >\n> > Do you need to include vector ?\n> > > +\n> > > +\tvoid handleRequestCompleted(Request *req);\n> > > +\n> > > +private:\n> > > +\tstd::unique_ptr cameraManager_;\n> > > +\n> > > +\tint eventFd_ = -1;\n> > > +\tstd::mutex completedRequestsMutex_;\n> > > +\tstd::vector completedRequests_;\n> > > +\n> > > +\tvoid writeFd();\n> > > +\tvoid readFd();\n> > > +\tvoid pushRequest(Request *req);\n> > > +\tstd::vector getCompletedRequests();\n> > > +};\n> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > > index e652837f..e7d078b5 100644\n> > > --- a/src/py/libcamera/py_main.cpp\n> > > +++ b/src/py/libcamera/py_main.cpp\n> > > @@ -7,10 +7,7 @@\n> > >\n> > > #include \"py_main.h\"\n> > >\n> > > -#include \n> > > #include \n> > > -#include \n> > > -#include \n> > >\n> > > #include \n> > >\n> > > @@ -21,6 +18,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \"py_camera_manager.h\"\n> > > #include \"py_helpers.h\"\n> > >\n> > > namespace py = pybind11;\n> > > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python)\n> > >\n> > > }\n> > >\n> > > -static std::weak_ptr gCameraManager;\n> > > -static int gEventfd;\n> > > -static std::mutex gReqlistMutex;\n> > > -static std::vector gReqList;\n> > > -\n> > > -static void handleRequestCompleted(Request *req)\n> > > -{\n> > > -\t{\n> > > -\t\tstd::lock_guard guard(gReqlistMutex);\n> > > -\t\tgReqList.push_back(req);\n> > > -\t}\n> > > -\n> > > -\tuint64_t v = 1;\n> > > -\tsize_t s = write(gEventfd, &v, 8);\n> > > -\t/*\n> > > -\t * We should never fail, and have no simple means to manage the error,\n> > > -\t * so let's log a fatal error.\n> > > -\t */\n> > > -\tif (s != 8)\n> > > -\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > > -}\n> > > +/*\n> > > + * Note: global C++ destructors can be ran on this before the py module is\n> > > + * destructed.\n> > > + */\n> > > +static std::weak_ptr gCameraManager;\n> > >\n> > > void init_py_enums(py::module &m);\n> > > void init_py_controls_generated(py::module &m);\n> > > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> > > \t */\n> > >\n> > > -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > > +\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > > \tauto pyCamera = py::class_(m, \"Camera\");\n> > > \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> > > \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> > > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t/* Classes */\n> > > \tpyCameraManager\n> > > \t\t.def_static(\"singleton\", []() {\n> > > -\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> > > -\t\t\tif (cm)\n> > > -\t\t\t\treturn cm;\n> > > -\n> > > -\t\t\tint fd = eventfd(0, 0);\n> > > -\t\t\tif (fd == -1)\n> > > -\t\t\t\tthrow std::system_error(errno, std::generic_category(),\n> > > -\t\t\t\t\t\t\t\"Failed to create eventfd\");\n> > > -\n> > > -\t\t\tcm = std::shared_ptr(new CameraManager, [](auto p) {\n> > > -\t\t\t\tclose(gEventfd);\n> > > -\t\t\t\tgEventfd = -1;\n> > > -\t\t\t\tdelete p;\n> > > -\t\t\t});\n> > > -\n> > > -\t\t\tgEventfd = fd;\n> > > -\t\t\tgCameraManager = cm;\n> > > -\n> > > -\t\t\tint ret = cm->start();\n> > > -\t\t\tif (ret)\n> > > -\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > > -\t\t\t\t\t\t\t\"Failed to start CameraManager\");\n> > > -\n> > > -\t\t\treturn cm;\n> > > -\t\t})\n> > > -\n> > > -\t\t.def_property_readonly(\"version\", &CameraManager::version)\n> > > +\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> > >\n> > > -\t\t.def_property_readonly(\"event_fd\", [](CameraManager &) {\n> > > -\t\t\treturn gEventfd;\n> > > -\t\t})\n> > > -\n> > > -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> > > -\t\t\tuint8_t buf[8];\n> > > -\n> > > -\t\t\tif (read(gEventfd, buf, 8) != 8)\n> > > -\t\t\t\tthrow std::system_error(errno, std::generic_category());\n> > > -\n> > > -\t\t\tstd::vector v;\n> > > -\n> > > -\t\t\t{\n> > > -\t\t\t\tstd::lock_guard guard(gReqlistMutex);\n> > > -\t\t\t\tswap(v, gReqList);\n> > > +\t\t\tif (!cm) {\n> > > +\t\t\t\tcm = std::make_shared();\n> > > +\t\t\t\tgCameraManager = cm;\n> >\n> > What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> > as a shared_ptr ?\n>\n> shared_ptr keeps it alive. We don't want to keep it alive. It's the job of\n\nOh, I thought we wanted to keep it alive and share ownership with\napps.\n\nI don't really understand how lifetime is handled in pybind, so I\ntrust your judjment here\n\n> the application to keep it alive (as long as it uses it).\n>\n> > > \t\t\t}\n> > >\n> > > -\t\t\tstd::vector ret;\n> > > -\n> > > -\t\t\tfor (Request *req : v) {\n> > > -\t\t\t\tpy::object o = py::cast(req);\n> > > -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > -\t\t\t\to.dec_ref();\n> > > -\t\t\t\tret.push_back(o);\n> > > -\t\t\t}\n> > > -\n> > > -\t\t\treturn ret;\n> > > +\t\t\treturn cm;\n> > > \t\t})\n> > >\n> > > -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > > -\n> > > -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > > -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > > -\t\t\tpy::list l;\n> > > -\n> > > -\t\t\tfor (auto &c : self.cameras()) {\n> > > -\t\t\t\tpy::object py_cm = py::cast(self);\n> > > -\t\t\t\tpy::object py_cam = py::cast(c);\n> > > -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > -\t\t\t\tl.append(py_cam);\n> > > -\t\t\t}\n> > > +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> > > +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> > > +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> > >\n> > > -\t\t\treturn l;\n> > > -\t\t});\n> > > +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> >\n> > Is access to eventfs useful for applications ?\n>\n> Yes, that's how they wait for events.\n>\n\nDon't we maks them behind the getReadyRequests() and\nhandleRequestCompleted() ?\n\nI see it will be reworked, so..\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 14B19BE173\n\tfor ;\n\tThu, 18 Aug 2022 15:00:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D8AF61FC0;\n\tThu, 18 Aug 2022 17:00:22 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7526861FA7\n\tfor ;\n\tThu, 18 Aug 2022 17:00:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 1854F20000B;\n\tThu, 18 Aug 2022 15:00:18 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660834822;\n\tbh=2HiJtH8ZGQbdoUyhNWMOoGC33nNrDuP1/YXyq1W/efM=;\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=MnUMxc81CCpxSjhQ99AHcxqvKRemCd88mkQvALkwvs4jwsEFxenJKVhMRydoxlkwq\n\tLP0nfiaChjNLi25HWUwBADLF8A1zQUnfzUFSD2WXhZwFaHWa7vMYwAWrpCqagkwK7H\n\tB1c2M7ZFuHe179EQAX36UfjVOMxMEtSy3ptpoZ+wmTkJGrmJ5+X5qPi4zC+iV6vsC0\n\tjFs6+WKOm+B3w2KmgFQX8VeB9m8tCqrAtf3qGGdPRr/Z2ArV51AeuUVU0z3HYOE6R+\n\tdol2LVEbEuA4+Qc39B2E90llyiXTYJ0SBo3SzM9mxZ+BPkh7rO9Fsk4Yl2hAUe69tD\n\t44b+0SU+m8R0w==","Date":"Thu, 18 Aug 2022 17:00:17 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818150017.4igyj6lyjrch7obh@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24667,"web_url":"https://patchwork.libcamera.org/comment/24667/","msgid":"<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>","date":"2022-08-18T15:14:41","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 18:00, Jacopo Mondi wrote:\n\n>>>> +PyCameraManager::PyCameraManager()\n>>>> +{\n>>>> +\tLOG(Python, Debug) << \"PyCameraManager()\";\n>>>\n>>> Don't you need to LOG_DECLARE_CATEGORY() ?\n>>\n>> It's in py_main.h.\n>>\n> \n> Ah, I overlooked the first patches as they were reviewed already.\n> Well, that header only serves for that purpose.\n> \n> I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()\n> and spare that header.\n\nHmm, sorry can you clarify what you mean?\n\n>>> What is the advantage of using a weak_ptr<> here ? Can't this be kept\n>>> as a shared_ptr ?\n>>\n>> shared_ptr keeps it alive. We don't want to keep it alive. It's the job of\n> \n> Oh, I thought we wanted to keep it alive and share ownership with\n> apps.\n> \n> I don't really understand how lifetime is handled in pybind, so I\n> trust your judjment here\n\nWell, in short, the _bindings_ do not want to keep anything alive. It's \nthe python side that keeps things alive (by having objects in \nvariables/fields). But sometimes the bindings need to be able to access \nthings in places where the thing is not trivially available (i.e. passed \nas a parameter from the python side). So we have to store the camera \nmanager, but only as weak_ptr.\n\n>> the application to keep it alive (as long as it uses it).\n>>\n>>>> \t\t\t}\n>>>>\n>>>> -\t\t\tstd::vector ret;\n>>>> -\n>>>> -\t\t\tfor (Request *req : v) {\n>>>> -\t\t\t\tpy::object o = py::cast(req);\n>>>> -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n>>>> -\t\t\t\to.dec_ref();\n>>>> -\t\t\t\tret.push_back(o);\n>>>> -\t\t\t}\n>>>> -\n>>>> -\t\t\treturn ret;\n>>>> +\t\t\treturn cm;\n>>>> \t\t})\n>>>>\n>>>> -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n>>>> -\n>>>> -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n>>>> -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n>>>> -\t\t\tpy::list l;\n>>>> -\n>>>> -\t\t\tfor (auto &c : self.cameras()) {\n>>>> -\t\t\t\tpy::object py_cm = py::cast(self);\n>>>> -\t\t\t\tpy::object py_cam = py::cast(c);\n>>>> -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n>>>> -\t\t\t\tl.append(py_cam);\n>>>> -\t\t\t}\n>>>> +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n>>>> +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n>>>> +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n>>>>\n>>>> -\t\t\treturn l;\n>>>> -\t\t});\n>>>> +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n>>>\n>>> Is access to eventfs useful for applications ?\n>>\n>> Yes, that's how they wait for events.\n>>\n> \n> Don't we maks them behind the getReadyRequests() and\n> handleRequestCompleted() ?\n\nThere are two things, 1) using the fd for waiting for events, 2) reading \nand writing to the fd. The 1) has to be done by the app, the 2) is done \nwith the helper functions here.\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 D07A6C3272\n\tfor ;\n\tThu, 18 Aug 2022 15:14:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92A5861FC0;\n\tThu, 18 Aug 2022 17:14:45 +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 42B8F61FA7\n\tfor ;\n\tThu, 18 Aug 2022 17:14:44 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C20338B;\n\tThu, 18 Aug 2022 17:14:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660835685;\n\tbh=alQd+LjNcIkD/g9OrSe/8q4P5VO+xp1qLYBd01KUeNg=;\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=JuRCf4N4lcEOK6GYE0ZcNx2n8BBLYzemCy/2ZO4TkdnHoIqN4eQiKJER3sy+kDNdI\n\ty/rXKxdodsl63jtPFM0BaaTRCCuZNOa/2xKFIs0cDeLxtPcap54fB6dcXfLHOf80ME\n\tJAnRiKSIMEkb+l5CU4H3Vhu1J7fYsUlXBUezQJZOfz/mASsfmje4WkPme/Rsu2ILlm\n\tKPDFIDx8wLRN4lPolk+Q6cf0imfxwOtUpNQSNoHTP/t9fDic0dB7/S1lYMIlcqI9gU\n\t5t6BW/awOy5/apOYx3FuiacJ+kMn5uDb44wIvr7ramaw6tGEbQuTcj38x5hmD3x0rf\n\tj7foJGlU0/7Mw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660835684;\n\tbh=alQd+LjNcIkD/g9OrSe/8q4P5VO+xp1qLYBd01KUeNg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Xs3Xdd0GNaJYKRkhV1y7i+y3zD3Y12TGvs+gWS9SD+qpqe4upRclRyyifXuB9zA/I\n\tNG9WCUtrxOAeqqnrLB1/dJNYX2l0S9QkJkLyXcXWlgBH3D1P36bn4klCLDpSCGWAKZ\n\tQj30unlpiL9lSqBUDelFfuyAOmW/Ddu29FsIxhEw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Xs3Xdd0G\"; dkim-atps=neutral","Message-ID":"<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>","Date":"Thu, 18 Aug 2022 18:14:41 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>\n\t<20220818150017.4igyj6lyjrch7obh@uno.localdomain>","In-Reply-To":"<20220818150017.4igyj6lyjrch7obh@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":24669,"web_url":"https://patchwork.libcamera.org/comment/24669/","msgid":"<20220818152620.e42xnshpavgt4vnw@uno.localdomain>","date":"2022-08-18T15:26:20","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomi,\n\nOn Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote:\n> On 18/08/2022 18:00, Jacopo Mondi wrote:\n>\n> > > > > +PyCameraManager::PyCameraManager()\n> > > > > +{\n> > > > > +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> > > >\n> > > > Don't you need to LOG_DECLARE_CATEGORY() ?\n> > >\n> > > It's in py_main.h.\n> > >\n> >\n> > Ah, I overlooked the first patches as they were reviewed already.\n> > Well, that header only serves for that purpose.\n> >\n> > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()\n> > and spare that header.\n>\n> Hmm, sorry can you clarify what you mean?\n>\n\nIf I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY().\n\nI would have DEFINE_CATEGORY() in py_main.cpp and use\nLOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in\nthe other compilation units that use the log symbol (which if I'm not\nmistaken is py_camera_manager.cpp only). In this way you can get rid\nof py_main.h completely.\n\n\n> > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> > > > as a shared_ptr ?\n> > >\n> > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of\n> >\n> > Oh, I thought we wanted to keep it alive and share ownership with\n> > apps.\n> >\n> > I don't really understand how lifetime is handled in pybind, so I\n> > trust your judjment here\n>\n> Well, in short, the _bindings_ do not want to keep anything alive. It's the\n> python side that keeps things alive (by having objects in variables/fields).\n> But sometimes the bindings need to be able to access things in places where\n> the thing is not trivially available (i.e. passed as a parameter from the\n> python side). So we have to store the camera manager, but only as weak_ptr.\n>\n\nThanks for the summary :)\n\n> > > the application to keep it alive (as long as it uses it).\n> > >\n> > > > > \t\t\t}\n> > > > >\n> > > > > -\t\t\tstd::vector ret;\n> > > > > -\n> > > > > -\t\t\tfor (Request *req : v) {\n> > > > > -\t\t\t\tpy::object o = py::cast(req);\n> > > > > -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > > > -\t\t\t\to.dec_ref();\n> > > > > -\t\t\t\tret.push_back(o);\n> > > > > -\t\t\t}\n> > > > > -\n> > > > > -\t\t\treturn ret;\n> > > > > +\t\t\treturn cm;\n> > > > > \t\t})\n> > > > >\n> > > > > -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > > > > -\n> > > > > -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > > > > -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > > > > -\t\t\tpy::list l;\n> > > > > -\n> > > > > -\t\t\tfor (auto &c : self.cameras()) {\n> > > > > -\t\t\t\tpy::object py_cm = py::cast(self);\n> > > > > -\t\t\t\tpy::object py_cam = py::cast(c);\n> > > > > -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > > > -\t\t\t\tl.append(py_cam);\n> > > > > -\t\t\t}\n> > > > > +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> > > > > +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> > > > > +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> > > > >\n> > > > > -\t\t\treturn l;\n> > > > > -\t\t});\n> > > > > +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> > > >\n> > > > Is access to eventfs useful for applications ?\n> > >\n> > > Yes, that's how they wait for events.\n> > >\n> >\n> > Don't we maks them behind the getReadyRequests() and\n> > handleRequestCompleted() ?\n>\n> There are two things, 1) using the fd for waiting for events, 2) reading and\n> writing to the fd. The 1) has to be done by the app, the 2) is done with the\n> helper functions here.\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 30E52C3272\n\tfor ;\n\tThu, 18 Aug 2022 15:26:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 785B861FC0;\n\tThu, 18 Aug 2022 17:26:26 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2083461FA7\n\tfor ;\n\tThu, 18 Aug 2022 17:26:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 74EF6100002;\n\tThu, 18 Aug 2022 15:26:22 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660836386;\n\tbh=dK1P3oShCBviiVP8ixc73stNj29XXq1EX0REYGo2GCU=;\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=UNvYHJ9Og+DeIxKqMfLsLuvyMIaNFVLp14zZBwRVUob4eDiafwH+dHu1P986RDfh0\n\tYqcw52nVByUnhrwsXIhiBIhGLwF+sZMjZ87rQlf7e67aQUyMmEQF5Xf8fQTf4RtjY0\n\tYYwHDxI3mPUubPmJaBxDx9EwCN0BTQwm82jypWxJYultNceVfwDHwz35oVp/UMmcR7\n\tNkkdsH34HeslT057ol3U3K/sUPbHAAbpFfMExjvNMon57PfN4nkGZSfBwLFqNZZEWe\n\t73i8DmXL4MwR5F1LuaDGJYksrdrcSB2kNfByhq1PJQArJriW1tJMqVmo5ySB6AJfJd\n\tvgNlo35dsGpyQ==","Date":"Thu, 18 Aug 2022 17:26:20 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818152620.e42xnshpavgt4vnw@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>\n\t<20220818150017.4igyj6lyjrch7obh@uno.localdomain>\n\t<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24680,"web_url":"https://patchwork.libcamera.org/comment/24680/","msgid":"","date":"2022-08-18T19:06:31","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote:\n> On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote:\n> > On 18/08/2022 18:00, Jacopo Mondi wrote:\n> >\n> > > > > > +PyCameraManager::PyCameraManager()\n> > > > > > +{\n> > > > > > +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> > > > >\n> > > > > Don't you need to LOG_DECLARE_CATEGORY() ?\n> > > >\n> > > > It's in py_main.h.\n> > > >\n> > >\n> > > Ah, I overlooked the first patches as they were reviewed already.\n> > > Well, that header only serves for that purpose.\n> > >\n> > > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()\n> > > and spare that header.\n> >\n> > Hmm, sorry can you clarify what you mean?\n> \n> If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY().\n> \n> I would have DEFINE_CATEGORY() in py_main.cpp and use\n> LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in\n> the other compilation units that use the log symbol (which if I'm not\n> mistaken is py_camera_manager.cpp only). In this way you can get rid\n> of py_main.h completely.\n\nNote that LOG_DECLARE_CATEGORY is similar to declaring a function\nprototype or external variable. Having it in a header file makes sense\nto me, but I would also not be against doing it manually in the .cpp\nfiles that need it, probably mostly because we do that already in a few\nplaces.\n\n> > > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> > > > > as a shared_ptr ?\n> > > >\n> > > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of\n> > >\n> > > Oh, I thought we wanted to keep it alive and share ownership with\n> > > apps.\n> > >\n> > > I don't really understand how lifetime is handled in pybind, so I\n> > > trust your judjment here\n> >\n> > Well, in short, the _bindings_ do not want to keep anything alive. It's the\n> > python side that keeps things alive (by having objects in variables/fields).\n> > But sometimes the bindings need to be able to access things in places where\n> > the thing is not trivially available (i.e. passed as a parameter from the\n> > python side). So we have to store the camera manager, but only as weak_ptr.\n> \n> Thanks for the summary :)\n> \n> > > > the application to keep it alive (as long as it uses it).\n> > > >\n> > > > > > \t\t\t}\n> > > > > >\n> > > > > > -\t\t\tstd::vector ret;\n> > > > > > -\n> > > > > > -\t\t\tfor (Request *req : v) {\n> > > > > > -\t\t\t\tpy::object o = py::cast(req);\n> > > > > > -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > > > > -\t\t\t\to.dec_ref();\n> > > > > > -\t\t\t\tret.push_back(o);\n> > > > > > -\t\t\t}\n> > > > > > -\n> > > > > > -\t\t\treturn ret;\n> > > > > > +\t\t\treturn cm;\n> > > > > > \t\t})\n> > > > > >\n> > > > > > -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > > > > > -\n> > > > > > -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > > > > > -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > > > > > -\t\t\tpy::list l;\n> > > > > > -\n> > > > > > -\t\t\tfor (auto &c : self.cameras()) {\n> > > > > > -\t\t\t\tpy::object py_cm = py::cast(self);\n> > > > > > -\t\t\t\tpy::object py_cam = py::cast(c);\n> > > > > > -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > > > > -\t\t\t\tl.append(py_cam);\n> > > > > > -\t\t\t}\n> > > > > > +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> > > > > > +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> > > > > > +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> > > > > >\n> > > > > > -\t\t\treturn l;\n> > > > > > -\t\t});\n> > > > > > +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> > > > >\n> > > > > Is access to eventfs useful for applications ?\n> > > >\n> > > > Yes, that's how they wait for events.\n> > > >\n> > >\n> > > Don't we maks them behind the getReadyRequests() and\n> > > handleRequestCompleted() ?\n> >\n> > There are two things, 1) using the fd for waiting for events, 2) reading and\n> > writing to the fd. The 1) has to be done by the app, the 2) is done with the\n> > helper functions here.","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 59B45C3272\n\tfor ;\n\tThu, 18 Aug 2022 19:06:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94C2C61FC0;\n\tThu, 18 Aug 2022 21:06: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 0114661FA7\n\tfor ;\n\tThu, 18 Aug 2022 21:06:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3F3688B;\n\tThu, 18 Aug 2022 21:06:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660849596;\n\tbh=W37Ljnx41sYsEjVP1upr22nB+Hh3s6YdQex5Q6mWW/c=;\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=kh6dTF6s+bHoJseVtGfVjdsLOv2HEn6wgj1ApNBoUXjS8By5ntYbRSXBnaeylGszb\n\t+eWR9gGoGN1G5G763HCuAAtoO+lGR2sTufHvz3P/+wRgtIxxj5vd2oB/2l3lt0gPwb\n\tl1gqGQ88y9c14ov444h4GcTGLyuCj5O8UX1BKfjvvZJTkr4AaubDlxXlw91txiVeBr\n\tbKb7tyR2g6d+atdGkB0oddUhdtjkPDUpUpmZZys9d/RAyomM90g7Q1xjMjiZjnOnke\n\tfFINcTWZYRL1rGMJQPT/kWSJjiYGda5H8r4fu+kvmXnxSGnrUUltQGUhUduq96rO26\n\tKHVLV12wAkp9g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660849594;\n\tbh=W37Ljnx41sYsEjVP1upr22nB+Hh3s6YdQex5Q6mWW/c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qANeqhhxdlUuOy3ANvyP179u20AChO4+7DEzy148otlMdgRSLW9gf/HPAn5rkzfSx\n\tqrlNgmglwQS3w7bl8fVLnrVJsCAw0CpPxCZPfB3k8Ru21X6V2DQCcy69HypPyOhNAz\n\tH3KVnntjB0Hby/zQH7obiWtUqkaXImTZHwElhhVM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qANeqhhx\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 22:06:31 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>\n\t<20220818150017.4igyj6lyjrch7obh@uno.localdomain>\n\t<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>\n\t<20220818152620.e42xnshpavgt4vnw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220818152620.e42xnshpavgt4vnw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":24681,"web_url":"https://patchwork.libcamera.org/comment/24681/","msgid":"","date":"2022-08-18T19:18:18","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi\n\nOn Thu, Aug 18, 2022 at 04:22:30PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote:\n> > Wrap the CameraManager with a PyCameraManager class and move the related\n> > code inside the new class.\n> >\n> > This helps understanding the life times of the used-to-be global\n> > variables, gets rid of static handleRequestCompleted function, and\n> > allows us to simplify the binding code as the more complex pieces are\n> > inside the class.\n> >\n> > There should be no user visible functional changes.\n> >\n> > Signed-off-by: Tomi Valkeinen \n\nWith Jacopo's comments addressed (except for the ones that got resolved\nin the replies to your patch),\n\nReviewed-by: Laurent Pinchart \n\n> > ---\n> > src/py/libcamera/meson.build | 1 +\n> > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++\n> > src/py/libcamera/py_camera_manager.h | 44 +++++++++\n> > src/py/libcamera/py_main.cpp | 117 +++++------------------\n> > 4 files changed, 193 insertions(+), 94 deletions(-)\n> > create mode 100644 src/py/libcamera/py_camera_manager.cpp\n> > create mode 100644 src/py/libcamera/py_camera_manager.h\n> >\n> > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> > index 04578bac..ad2a6858 100644\n> > --- a/src/py/libcamera/meson.build\n> > +++ b/src/py/libcamera/meson.build\n> > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11')\n> > pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> >\n> > pycamera_sources = files([\n> > + 'py_camera_manager.cpp',\n> > 'py_enums.cpp',\n> > 'py_geometry.cpp',\n> > 'py_helpers.cpp',\n> > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> > new file mode 100644\n> > index 00000000..ad47271d\n> > --- /dev/null\n> > +++ b/src/py/libcamera/py_camera_manager.cpp\n> > @@ -0,0 +1,125 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Tomi Valkeinen \n> > + */\n> > +\n> > +#include \"py_camera_manager.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"py_main.h\"\n> > +\n> > +namespace py = pybind11;\n> > +\n> > +using namespace libcamera;\n> > +\n> > +PyCameraManager::PyCameraManager()\n> > +{\n> > +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> \n> Don't you need to LOG_DECLARE_CATEGORY() ?\n> \n> > +\n> > +\tcameraManager_ = std::make_unique();\n> > +\n> > +\tint fd = eventfd(0, 0);\n> > +\tif (fd == -1)\n> > +\t\tthrow std::system_error(errno, std::generic_category(),\n> > +\t\t\t\t\t\"Failed to create eventfd\");\n> > +\n> > +\teventFd_ = fd;\n> \n> You can use UniqueFD() to avoid manually handling the eventfd file\n> descriptor\n> \n> > +\n> > +\tint ret = cameraManager_->start();\n> > +\tif (ret) {\n> > +\t\tclose(fd);\n> > +\t\teventFd_ = -1;\n> > +\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\"Failed to start CameraManager\");\n> > +\t}\n> > +}\n> > +\n> > +PyCameraManager::~PyCameraManager()\n> > +{\n> > +\tLOG(Python, Debug) << \"~PyCameraManager()\";\n> > +\n> > +\tif (eventFd_ != -1) {\n> > +\t\tclose(eventFd_);\n> > +\t\teventFd_ = -1;\n> > +\t}\n> > +}\n> > +\n> > +py::list PyCameraManager::cameras()\n> > +{\n> > +\t/*\n> > +\t * Create a list of Cameras, where each camera has a keep-alive to\n> > +\t * CameraManager.\n> > +\t */\n> > +\tpy::list l;\n> > +\n> > +\tfor (auto &camera : cameraManager_->cameras()) {\n> > +\t\tpy::object py_cm = py::cast(this);\n> > +\t\tpy::object py_cam = py::cast(camera);\n> > +\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > +\t\tl.append(py_cam);\n> > +\t}\n> > +\n> > +\treturn l;\n> \n> I was about to suggest creating the list at creation time and cache\n> it. But indeed cameras can be added or removed from the system\n> \n> > +}\n> > +\n> > +std::vector PyCameraManager::getReadyRequests()\n> > +{\n> > +\treadFd();\n> > +\n> > +\tstd::vector ret;\n> > +\n> > +\tfor (Request *request : getCompletedRequests()) {\n> > +\t\tpy::object o = py::cast(request);\n> > +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > +\t\to.dec_ref();\n> > +\t\tret.push_back(o);\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleRequestCompleted(Request *req)\n> > +{\n> > +\tpushRequest(req);\n> \n> Is this function only used here ?\n> Should it be inlined ?\n> \n> > +\twriteFd();\n> > +}\n> > +\n> > +void PyCameraManager::writeFd()\n> > +{\n> > +\tuint64_t v = 1;\n> > +\n> > +\tsize_t s = write(eventFd_, &v, 8);\n> \n> Any reason to write an 8 byte uint64 ?\n> Just curious\n> \n> > +\t/*\n> > +\t * We should never fail, and have no simple means to manage the error,\n> > +\t * so let's log a fatal error.\n> > +\t */\n> > +\tif (s != 8)\n> > +\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > +}\n> > +\n> > +void PyCameraManager::readFd()\n> > +{\n> > +\tuint8_t buf[8];\n> > +\n> > +\tif (read(eventFd_, buf, 8) != 8)\n> > +\t\tthrow std::system_error(errno, std::generic_category());\n> > +}\n> > +\n> > +void PyCameraManager::pushRequest(Request *req)\n> > +{\n> > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > +\tcompletedRequests_.push_back(req);\n> > +}\n> > +\n> > +std::vector PyCameraManager::getCompletedRequests()\n> > +{\n> > +\tstd::vector v;\n> > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > +\tswap(v, completedRequests_);\n> > +\treturn v;\n> > +}\n> > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> > new file mode 100644\n> > index 00000000..9c15f814\n> > --- /dev/null\n> > +++ b/src/py/libcamera/py_camera_manager.h\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Tomi Valkeinen \n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +class PyCameraManager\n> > +{\n> > +public:\n> > +\tPyCameraManager();\n> > +\t~PyCameraManager();\n> > +\n> > +\tpybind11::list cameras();\n> > +\tstd::shared_ptr get(const std::string &name) { return cameraManager_->get(name); }\n> \n> Do you need to include ?\n> > +\n> > +\tstatic const std::string &version() { return CameraManager::version(); }\n> \n> Do you need to include \n> \n> Should this method be marked as const ?\n> \n> > +\n> > +\tint eventFd() const { return eventFd_; }\n> > +\n> > +\tstd::vector getReadyRequests();\n> \n> Do you need to include vector ?\n> > +\n> > +\tvoid handleRequestCompleted(Request *req);\n> > +\n> > +private:\n> > +\tstd::unique_ptr cameraManager_;\n> > +\n> > +\tint eventFd_ = -1;\n> > +\tstd::mutex completedRequestsMutex_;\n> > +\tstd::vector completedRequests_;\n> > +\n> > +\tvoid writeFd();\n> > +\tvoid readFd();\n> > +\tvoid pushRequest(Request *req);\n> > +\tstd::vector getCompletedRequests();\n> > +};\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index e652837f..e7d078b5 100644\n> > --- a/src/py/libcamera/py_main.cpp\n> > +++ b/src/py/libcamera/py_main.cpp\n> > @@ -7,10 +7,7 @@\n> >\n> > #include \"py_main.h\"\n> >\n> > -#include \n> > #include \n> > -#include \n> > -#include \n> >\n> > #include \n> >\n> > @@ -21,6 +18,7 @@\n> > #include \n> > #include \n> >\n> > +#include \"py_camera_manager.h\"\n> > #include \"py_helpers.h\"\n> >\n> > namespace py = pybind11;\n> > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python)\n> >\n> > }\n> >\n> > -static std::weak_ptr gCameraManager;\n> > -static int gEventfd;\n> > -static std::mutex gReqlistMutex;\n> > -static std::vector gReqList;\n> > -\n> > -static void handleRequestCompleted(Request *req)\n> > -{\n> > -\t{\n> > -\t\tstd::lock_guard guard(gReqlistMutex);\n> > -\t\tgReqList.push_back(req);\n> > -\t}\n> > -\n> > -\tuint64_t v = 1;\n> > -\tsize_t s = write(gEventfd, &v, 8);\n> > -\t/*\n> > -\t * We should never fail, and have no simple means to manage the error,\n> > -\t * so let's log a fatal error.\n> > -\t */\n> > -\tif (s != 8)\n> > -\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > -}\n> > +/*\n> > + * Note: global C++ destructors can be ran on this before the py module is\n> > + * destructed.\n> > + */\n> > +static std::weak_ptr gCameraManager;\n> >\n> > void init_py_enums(py::module &m);\n> > void init_py_controls_generated(py::module &m);\n> > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> > \t */\n> >\n> > -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > +\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > \tauto pyCamera = py::class_(m, \"Camera\");\n> > \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> > \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t/* Classes */\n> > \tpyCameraManager\n> > \t\t.def_static(\"singleton\", []() {\n> > -\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> > -\t\t\tif (cm)\n> > -\t\t\t\treturn cm;\n> > -\n> > -\t\t\tint fd = eventfd(0, 0);\n> > -\t\t\tif (fd == -1)\n> > -\t\t\t\tthrow std::system_error(errno, std::generic_category(),\n> > -\t\t\t\t\t\t\t\"Failed to create eventfd\");\n> > -\n> > -\t\t\tcm = std::shared_ptr(new CameraManager, [](auto p) {\n> > -\t\t\t\tclose(gEventfd);\n> > -\t\t\t\tgEventfd = -1;\n> > -\t\t\t\tdelete p;\n> > -\t\t\t});\n> > -\n> > -\t\t\tgEventfd = fd;\n> > -\t\t\tgCameraManager = cm;\n> > -\n> > -\t\t\tint ret = cm->start();\n> > -\t\t\tif (ret)\n> > -\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > -\t\t\t\t\t\t\t\"Failed to start CameraManager\");\n> > -\n> > -\t\t\treturn cm;\n> > -\t\t})\n> > -\n> > -\t\t.def_property_readonly(\"version\", &CameraManager::version)\n> > +\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> >\n> > -\t\t.def_property_readonly(\"event_fd\", [](CameraManager &) {\n> > -\t\t\treturn gEventfd;\n> > -\t\t})\n> > -\n> > -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> > -\t\t\tuint8_t buf[8];\n> > -\n> > -\t\t\tif (read(gEventfd, buf, 8) != 8)\n> > -\t\t\t\tthrow std::system_error(errno, std::generic_category());\n> > -\n> > -\t\t\tstd::vector v;\n> > -\n> > -\t\t\t{\n> > -\t\t\t\tstd::lock_guard guard(gReqlistMutex);\n> > -\t\t\t\tswap(v, gReqList);\n> > +\t\t\tif (!cm) {\n> > +\t\t\t\tcm = std::make_shared();\n> > +\t\t\t\tgCameraManager = cm;\n> \n> What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> as a shared_ptr ?\n> \n> > \t\t\t}\n> >\n> > -\t\t\tstd::vector ret;\n> > -\n> > -\t\t\tfor (Request *req : v) {\n> > -\t\t\t\tpy::object o = py::cast(req);\n> > -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > -\t\t\t\to.dec_ref();\n> > -\t\t\t\tret.push_back(o);\n> > -\t\t\t}\n> > -\n> > -\t\t\treturn ret;\n> > +\t\t\treturn cm;\n> > \t\t})\n> >\n> > -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > -\n> > -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > -\t\t\tpy::list l;\n> > -\n> > -\t\t\tfor (auto &c : self.cameras()) {\n> > -\t\t\t\tpy::object py_cm = py::cast(self);\n> > -\t\t\t\tpy::object py_cam = py::cast(c);\n> > -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > -\t\t\t\tl.append(py_cam);\n> > -\t\t\t}\n> > +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> > +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> > +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> >\n> > -\t\t\treturn l;\n> > -\t\t});\n> > +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> \n> Is access to eventfs useful for applications ?\n> \n> > +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> >\n> > \tpyCamera\n> > \t\t.def_property_readonly(\"id\", &Camera::id)\n> > @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t\t const std::unordered_map &controls) {\n> > \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n> >\n> > -\t\t\tself.requestCompleted.connect(handleRequestCompleted);\n> > +\t\t\tauto cm = gCameraManager.lock();\n> > +\t\t\tASSERT(cm);\n> > +\n> > +\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> >\n> > \t\t\tControlList controlList(self.controls());\n> >\n> > @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >\n> > \t\t\tint ret = self.start(&controlList);\n> > \t\t\tif (ret) {\n> > -\t\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> > +\t\t\t\tself.requestCompleted.disconnect();\n> > \t\t\t\treturn ret;\n> > \t\t\t}\n> >\n> > @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t\t\tif (ret)\n> > \t\t\t\treturn ret;\n> >\n> > -\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> > +\t\t\tself.requestCompleted.disconnect();\n> >\n> > \t\t\treturn 0;\n> > \t\t})","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 A2F54BE173\n\tfor ;\n\tThu, 18 Aug 2022 19:18:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0864161FBC;\n\tThu, 18 Aug 2022 21:18:25 +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 B051F61FA7\n\tfor ;\n\tThu, 18 Aug 2022 21:18:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3C858B;\n\tThu, 18 Aug 2022 21:18:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660850305;\n\tbh=prYX0H4Con3M6vhPJYgktSovHN+OM0i3GQzOg482HqQ=;\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=TAsCREzQTTZZ4fSeQTmqJvVZS9YMtL1NmYQvIy8dnK365rrquBMTdYv/lTtshz0Bm\n\t2LwnuXPbvRUfOM8X105GL0hEeJYfg+y5lgnVfPxOmSuXB3Iew3T8E2YmIzPJLjbbc4\n\tAMBg6LMd33omGC1WarB+nJrWVoF/7iF4bOmPd210uoaGGecHq5uwh3dISIrCCHS5dU\n\tpDYNreF9k3yhIjTgovD0JfBM1HnJpsW7AGt626H7vIYHxcBWaDzlavzAf2nAI3waRY\n\tajWWQid/qhx8Y4bqtxHvSyIifCRYshTV+8WUq6SzKLfgS2JH5tdg+47XEn2gxz7Ubs\n\tcpsUaG4acw/qQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660850302;\n\tbh=prYX0H4Con3M6vhPJYgktSovHN+OM0i3GQzOg482HqQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m9mIkDpVX0Y+oJQG+XKcIXdrb3NlbH8NZUw0SuqERtvBXDwgGKwTUP/EjamDr39g5\n\tPuIadf7zaLXZuLSzejttMSbRpU9J7AigwyFMwdMEOzjS6p6BDqfGzWCfR5T6WUX7Az\n\t1WfY7BmryXxfBl444ZUcEag4IY5fclGTONG8exiU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"m9mIkDpV\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 22:18:18 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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":24682,"web_url":"https://patchwork.libcamera.org/comment/24682/","msgid":"","date":"2022-08-18T19:19:18","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi\n\nOn Thu, Aug 18, 2022 at 10:18:18PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Thu, Aug 18, 2022 at 04:22:30PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote:\n> > > Wrap the CameraManager with a PyCameraManager class and move the related\n> > > code inside the new class.\n> > >\n> > > This helps understanding the life times of the used-to-be global\n> > > variables, gets rid of static handleRequestCompleted function, and\n> > > allows us to simplify the binding code as the more complex pieces are\n> > > inside the class.\n> > >\n> > > There should be no user visible functional changes.\n> > >\n> > > Signed-off-by: Tomi Valkeinen \n> \n> With Jacopo's comments addressed (except for the ones that got resolved\n> in the replies to your patch),\n\nActually, one more comment, see below.\n\n> Reviewed-by: Laurent Pinchart \n> \n> > > ---\n> > > src/py/libcamera/meson.build | 1 +\n> > > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++\n> > > src/py/libcamera/py_camera_manager.h | 44 +++++++++\n> > > src/py/libcamera/py_main.cpp | 117 +++++------------------\n> > > 4 files changed, 193 insertions(+), 94 deletions(-)\n> > > create mode 100644 src/py/libcamera/py_camera_manager.cpp\n> > > create mode 100644 src/py/libcamera/py_camera_manager.h\n> > >\n> > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build\n> > > index 04578bac..ad2a6858 100644\n> > > --- a/src/py/libcamera/meson.build\n> > > +++ b/src/py/libcamera/meson.build\n> > > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11')\n> > > pybind11_dep = pybind11_proj.get_variable('pybind11_dep')\n> > >\n> > > pycamera_sources = files([\n> > > + 'py_camera_manager.cpp',\n> > > 'py_enums.cpp',\n> > > 'py_geometry.cpp',\n> > > 'py_helpers.cpp',\n> > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> > > new file mode 100644\n> > > index 00000000..ad47271d\n> > > --- /dev/null\n> > > +++ b/src/py/libcamera/py_camera_manager.cpp\n> > > @@ -0,0 +1,125 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Tomi Valkeinen \n> > > + */\n> > > +\n> > > +#include \"py_camera_manager.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"py_main.h\"\n> > > +\n> > > +namespace py = pybind11;\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +PyCameraManager::PyCameraManager()\n> > > +{\n> > > +\tLOG(Python, Debug) << \"PyCameraManager()\";\n> > \n> > Don't you need to LOG_DECLARE_CATEGORY() ?\n> > \n> > > +\n> > > +\tcameraManager_ = std::make_unique();\n> > > +\n> > > +\tint fd = eventfd(0, 0);\n> > > +\tif (fd == -1)\n> > > +\t\tthrow std::system_error(errno, std::generic_category(),\n> > > +\t\t\t\t\t\"Failed to create eventfd\");\n> > > +\n> > > +\teventFd_ = fd;\n> > \n> > You can use UniqueFD() to avoid manually handling the eventfd file\n> > descriptor\n> > \n> > > +\n> > > +\tint ret = cameraManager_->start();\n> > > +\tif (ret) {\n> > > +\t\tclose(fd);\n> > > +\t\teventFd_ = -1;\n> > > +\t\tthrow std::system_error(-ret, std::generic_category(),\n> > > +\t\t\t\t\t\"Failed to start CameraManager\");\n> > > +\t}\n> > > +}\n> > > +\n> > > +PyCameraManager::~PyCameraManager()\n> > > +{\n> > > +\tLOG(Python, Debug) << \"~PyCameraManager()\";\n> > > +\n> > > +\tif (eventFd_ != -1) {\n> > > +\t\tclose(eventFd_);\n> > > +\t\teventFd_ = -1;\n> > > +\t}\n> > > +}\n> > > +\n> > > +py::list PyCameraManager::cameras()\n> > > +{\n> > > +\t/*\n> > > +\t * Create a list of Cameras, where each camera has a keep-alive to\n> > > +\t * CameraManager.\n> > > +\t */\n> > > +\tpy::list l;\n> > > +\n> > > +\tfor (auto &camera : cameraManager_->cameras()) {\n> > > +\t\tpy::object py_cm = py::cast(this);\n> > > +\t\tpy::object py_cam = py::cast(camera);\n> > > +\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > +\t\tl.append(py_cam);\n> > > +\t}\n> > > +\n> > > +\treturn l;\n> > \n> > I was about to suggest creating the list at creation time and cache\n> > it. But indeed cameras can be added or removed from the system\n> > \n> > > +}\n> > > +\n> > > +std::vector PyCameraManager::getReadyRequests()\n> > > +{\n> > > +\treadFd();\n> > > +\n> > > +\tstd::vector ret;\n> > > +\n> > > +\tfor (Request *request : getCompletedRequests()) {\n> > > +\t\tpy::object o = py::cast(request);\n> > > +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > +\t\to.dec_ref();\n> > > +\t\tret.push_back(o);\n> > > +\t}\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +/* Note: Called from another thread */\n> > > +void PyCameraManager::handleRequestCompleted(Request *req)\n> > > +{\n> > > +\tpushRequest(req);\n> > \n> > Is this function only used here ?\n> > Should it be inlined ?\n> > \n> > > +\twriteFd();\n> > > +}\n> > > +\n> > > +void PyCameraManager::writeFd()\n> > > +{\n> > > +\tuint64_t v = 1;\n> > > +\n> > > +\tsize_t s = write(eventFd_, &v, 8);\n> > \n> > Any reason to write an 8 byte uint64 ?\n> > Just curious\n> > \n> > > +\t/*\n> > > +\t * We should never fail, and have no simple means to manage the error,\n> > > +\t * so let's log a fatal error.\n> > > +\t */\n> > > +\tif (s != 8)\n> > > +\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > > +}\n> > > +\n> > > +void PyCameraManager::readFd()\n> > > +{\n> > > +\tuint8_t buf[8];\n> > > +\n> > > +\tif (read(eventFd_, buf, 8) != 8)\n> > > +\t\tthrow std::system_error(errno, std::generic_category());\n> > > +}\n> > > +\n> > > +void PyCameraManager::pushRequest(Request *req)\n> > > +{\n> > > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > > +\tcompletedRequests_.push_back(req);\n> > > +}\n> > > +\n> > > +std::vector PyCameraManager::getCompletedRequests()\n> > > +{\n> > > +\tstd::vector v;\n> > > +\tstd::lock_guard guard(completedRequestsMutex_);\n> > > +\tswap(v, completedRequests_);\n> > > +\treturn v;\n> > > +}\n> > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> > > new file mode 100644\n> > > index 00000000..9c15f814\n> > > --- /dev/null\n> > > +++ b/src/py/libcamera/py_camera_manager.h\n> > > @@ -0,0 +1,44 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Tomi Valkeinen \n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +class PyCameraManager\n> > > +{\n> > > +public:\n> > > +\tPyCameraManager();\n> > > +\t~PyCameraManager();\n> > > +\n> > > +\tpybind11::list cameras();\n> > > +\tstd::shared_ptr get(const std::string &name) { return cameraManager_->get(name); }\n> > \n> > Do you need to include ?\n> > > +\n> > > +\tstatic const std::string &version() { return CameraManager::version(); }\n> > \n> > Do you need to include \n> > \n> > Should this method be marked as const ?\n> > \n> > > +\n> > > +\tint eventFd() const { return eventFd_; }\n> > > +\n> > > +\tstd::vector getReadyRequests();\n> > \n> > Do you need to include vector ?\n> > > +\n> > > +\tvoid handleRequestCompleted(Request *req);\n> > > +\n> > > +private:\n> > > +\tstd::unique_ptr cameraManager_;\n> > > +\n> > > +\tint eventFd_ = -1;\n> > > +\tstd::mutex completedRequestsMutex_;\n> > > +\tstd::vector completedRequests_;\n> > > +\n> > > +\tvoid writeFd();\n> > > +\tvoid readFd();\n> > > +\tvoid pushRequest(Request *req);\n> > > +\tstd::vector getCompletedRequests();\n\nWe usually place functions before variables.\n\n> > > +};\n> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > > index e652837f..e7d078b5 100644\n> > > --- a/src/py/libcamera/py_main.cpp\n> > > +++ b/src/py/libcamera/py_main.cpp\n> > > @@ -7,10 +7,7 @@\n> > >\n> > > #include \"py_main.h\"\n> > >\n> > > -#include \n> > > #include \n> > > -#include \n> > > -#include \n> > >\n> > > #include \n> > >\n> > > @@ -21,6 +18,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \"py_camera_manager.h\"\n> > > #include \"py_helpers.h\"\n> > >\n> > > namespace py = pybind11;\n> > > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python)\n> > >\n> > > }\n> > >\n> > > -static std::weak_ptr gCameraManager;\n> > > -static int gEventfd;\n> > > -static std::mutex gReqlistMutex;\n> > > -static std::vector gReqList;\n> > > -\n> > > -static void handleRequestCompleted(Request *req)\n> > > -{\n> > > -\t{\n> > > -\t\tstd::lock_guard guard(gReqlistMutex);\n> > > -\t\tgReqList.push_back(req);\n> > > -\t}\n> > > -\n> > > -\tuint64_t v = 1;\n> > > -\tsize_t s = write(gEventfd, &v, 8);\n> > > -\t/*\n> > > -\t * We should never fail, and have no simple means to manage the error,\n> > > -\t * so let's log a fatal error.\n> > > -\t */\n> > > -\tif (s != 8)\n> > > -\t\tLOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > > -}\n> > > +/*\n> > > + * Note: global C++ destructors can be ran on this before the py module is\n> > > + * destructed.\n> > > + */\n> > > +static std::weak_ptr gCameraManager;\n> > >\n> > > void init_py_enums(py::module &m);\n> > > void init_py_controls_generated(py::module &m);\n> > > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> > > \t */\n> > >\n> > > -\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > > +\tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> > > \tauto pyCamera = py::class_(m, \"Camera\");\n> > > \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> > > \tauto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> > > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t/* Classes */\n> > > \tpyCameraManager\n> > > \t\t.def_static(\"singleton\", []() {\n> > > -\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> > > -\t\t\tif (cm)\n> > > -\t\t\t\treturn cm;\n> > > -\n> > > -\t\t\tint fd = eventfd(0, 0);\n> > > -\t\t\tif (fd == -1)\n> > > -\t\t\t\tthrow std::system_error(errno, std::generic_category(),\n> > > -\t\t\t\t\t\t\t\"Failed to create eventfd\");\n> > > -\n> > > -\t\t\tcm = std::shared_ptr(new CameraManager, [](auto p) {\n> > > -\t\t\t\tclose(gEventfd);\n> > > -\t\t\t\tgEventfd = -1;\n> > > -\t\t\t\tdelete p;\n> > > -\t\t\t});\n> > > -\n> > > -\t\t\tgEventfd = fd;\n> > > -\t\t\tgCameraManager = cm;\n> > > -\n> > > -\t\t\tint ret = cm->start();\n> > > -\t\t\tif (ret)\n> > > -\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > > -\t\t\t\t\t\t\t\"Failed to start CameraManager\");\n> > > -\n> > > -\t\t\treturn cm;\n> > > -\t\t})\n> > > -\n> > > -\t\t.def_property_readonly(\"version\", &CameraManager::version)\n> > > +\t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> > >\n> > > -\t\t.def_property_readonly(\"event_fd\", [](CameraManager &) {\n> > > -\t\t\treturn gEventfd;\n> > > -\t\t})\n> > > -\n> > > -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> > > -\t\t\tuint8_t buf[8];\n> > > -\n> > > -\t\t\tif (read(gEventfd, buf, 8) != 8)\n> > > -\t\t\t\tthrow std::system_error(errno, std::generic_category());\n> > > -\n> > > -\t\t\tstd::vector v;\n> > > -\n> > > -\t\t\t{\n> > > -\t\t\t\tstd::lock_guard guard(gReqlistMutex);\n> > > -\t\t\t\tswap(v, gReqList);\n> > > +\t\t\tif (!cm) {\n> > > +\t\t\t\tcm = std::make_shared();\n> > > +\t\t\t\tgCameraManager = cm;\n> > \n> > What is the advantage of using a weak_ptr<> here ? Can't this be kept\n> > as a shared_ptr ?\n> > \n> > > \t\t\t}\n> > >\n> > > -\t\t\tstd::vector ret;\n> > > -\n> > > -\t\t\tfor (Request *req : v) {\n> > > -\t\t\t\tpy::object o = py::cast(req);\n> > > -\t\t\t\t/* Decrease the ref increased in Camera.queue_request() */\n> > > -\t\t\t\to.dec_ref();\n> > > -\t\t\t\tret.push_back(o);\n> > > -\t\t\t}\n> > > -\n> > > -\t\t\treturn ret;\n> > > +\t\t\treturn cm;\n> > > \t\t})\n> > >\n> > > -\t\t.def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > > -\n> > > -\t\t/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > > -\t\t.def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > > -\t\t\tpy::list l;\n> > > -\n> > > -\t\t\tfor (auto &c : self.cameras()) {\n> > > -\t\t\t\tpy::object py_cm = py::cast(self);\n> > > -\t\t\t\tpy::object py_cam = py::cast(c);\n> > > -\t\t\t\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> > > -\t\t\t\tl.append(py_cam);\n> > > -\t\t\t}\n> > > +\t\t.def_property_readonly(\"version\", &PyCameraManager::version)\n> > > +\t\t.def(\"get\", &PyCameraManager::get, py::keep_alive<0, 1>())\n> > > +\t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> > >\n> > > -\t\t\treturn l;\n> > > -\t\t});\n> > > +\t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> > \n> > Is access to eventfs useful for applications ?\n> > \n> > > +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> > >\n> > > \tpyCamera\n> > > \t\t.def_property_readonly(\"id\", &Camera::id)\n> > > @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t\t const std::unordered_map &controls) {\n> > > \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n> > >\n> > > -\t\t\tself.requestCompleted.connect(handleRequestCompleted);\n> > > +\t\t\tauto cm = gCameraManager.lock();\n> > > +\t\t\tASSERT(cm);\n> > > +\n> > > +\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> > >\n> > > \t\t\tControlList controlList(self.controls());\n> > >\n> > > @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > >\n> > > \t\t\tint ret = self.start(&controlList);\n> > > \t\t\tif (ret) {\n> > > -\t\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> > > +\t\t\t\tself.requestCompleted.disconnect();\n> > > \t\t\t\treturn ret;\n> > > \t\t\t}\n> > >\n> > > @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > > \t\t\tif (ret)\n> > > \t\t\t\treturn ret;\n> > >\n> > > -\t\t\tself.requestCompleted.disconnect(handleRequestCompleted);\n> > > +\t\t\tself.requestCompleted.disconnect();\n> > >\n> > > \t\t\treturn 0;\n> > > \t\t})","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 3042CBE173\n\tfor ;\n\tThu, 18 Aug 2022 19:19:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80FB461FC0;\n\tThu, 18 Aug 2022 21:19: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 A0F6161FA7\n\tfor ;\n\tThu, 18 Aug 2022 21:19:21 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41BF18B;\n\tThu, 18 Aug 2022 21:19:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660850362;\n\tbh=lhXEjIg021dHP8awdUvKTjp9xRznXhCTOi1QhRe/2nc=;\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:\n\tFrom;\n\tb=CI3c9Vd0ike7Bi0R6x/FcXQCXMPxMThwbnNLnWbafD4WrZVkfJvyBaE7lCTLqQoHi\n\tBK6HorfJUxpCDMUT1r2s5yTapVWMymcrBbRnMQw8jrGaPajCdyHtIa593U6HIv5xn1\n\tzeNFsPLb3k/LfuP9aFjt1RADD9Mswlp6yj9YkGBhx2U55X5lkklQWpKW1l/PXs29DD\n\tzLBqKa8/i8QL+SdwfnU7+F5rP/ZU1DlagrN+4n7Zwoqj5k2gjitmofHuxTaeVqA9GU\n\tvwwKR0oT6BzWWDSHFSTyH+fGD683lKKkzUbThAbHXIK+PFBPw7ELoix/MM+hYEOGeH\n\tNJq+IVGREhppA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660850361;\n\tbh=lhXEjIg021dHP8awdUvKTjp9xRznXhCTOi1QhRe/2nc=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=GPuwUNs+pHxCxuDxPeBuyjoR0MViteij4BhYhjUGHEZYYHkxSahu90tu8qYW8nkrb\n\tqrR0YXPJAUPKjVJU3l+6z0OM1tjjzVCEh03GHe/AQo7QZm3nXtcWJ5rFcM5p0UQd/R\n\txhkaQgAl2T4w5j4B+iL+eP52BU2N5klGrn416AUM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GPuwUNs+\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 22:19:18 +0300","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24703,"web_url":"https://patchwork.libcamera.org/comment/24703/","msgid":"<515802c5-363b-7717-0025-70ce76345c79@ideasonboard.com>","date":"2022-08-19T06:17:23","subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 22:06, Laurent Pinchart wrote:\n> On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote:\n>> On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote:\n>>> On 18/08/2022 18:00, Jacopo Mondi wrote:\n>>>\n>>>>>>> +PyCameraManager::PyCameraManager()\n>>>>>>> +{\n>>>>>>> +\tLOG(Python, Debug) << \"PyCameraManager()\";\n>>>>>>\n>>>>>> Don't you need to LOG_DECLARE_CATEGORY() ?\n>>>>>\n>>>>> It's in py_main.h.\n>>>>>\n>>>>\n>>>> Ah, I overlooked the first patches as they were reviewed already.\n>>>> Well, that header only serves for that purpose.\n>>>>\n>>>> I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()\n>>>> and spare that header.\n>>>\n>>> Hmm, sorry can you clarify what you mean?\n>>\n>> If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY().\n>>\n>> I would have DEFINE_CATEGORY() in py_main.cpp and use\n>> LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in\n>> the other compilation units that use the log symbol (which if I'm not\n>> mistaken is py_camera_manager.cpp only). In this way you can get rid\n>> of py_main.h completely.\n> \n> Note that LOG_DECLARE_CATEGORY is similar to declaring a function\n> prototype or external variable. Having it in a header file makes sense\n> to me, but I would also not be against doing it manually in the .cpp\n> files that need it, probably mostly because we do that already in a few\n> places.\n\nI think py_main.h is a logical place for LOG_DECLARE_CATEGORY, as the \ncounterpart is in py_main.cpp. It's true that, at the moment, only \nsingle .cpp file uses LOG(), and there's nothing else in the py_main.h, \nbut...feels like a bit needless optimization to try to get rid of the file.\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 C1438BE173\n\tfor ;\n\tFri, 19 Aug 2022 06:17:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40BD161FC0;\n\tFri, 19 Aug 2022 08:17:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D674603E2\n\tfor ;\n\tFri, 19 Aug 2022 08:17:27 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 375643F1;\n\tFri, 19 Aug 2022 08:17:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660889848;\n\tbh=PMXbCRy+6JdmRh3ltczFgawhiVyLdDe6gS/DzAZQN7o=;\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=CvKHX0HL//JdO9ur2zMdUBGm0ZgA0+r+HQ6/kZxYNoo5KP++E7jELp8q6PuWpvHM/\n\tQAmDMeLRmPoBqg/U7hO46ozVxwAxo2wqPmH9XlueoY8bUz5UToNdKRauFvKMxua+h2\n\tC070azcValoEINzDG+h7kMxZnfup/o4/DLtOx8ZOYSP467B27OdFEEjc3XtuV6RDdK\n\tyyFlJ5duIcrvOBtjSojFMTRt/zMuHqLs+2KIYhUC6/ckmrPUY0B/bQzuigLGaLUwNn\n\tx4t/20h2JyeaBzcgQinVezoYDWugquwShO2+YfyD9pUKA7IQnf4Qu5SRHRaBuQ9R8A\n\tOPpO9A8zJ1wkw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660889846;\n\tbh=PMXbCRy+6JdmRh3ltczFgawhiVyLdDe6gS/DzAZQN7o=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=IbBrpdSjwxvRWAPOYXRIuZDw02DqTbAaAa5xqjMfQ3XeskXihoV1MUV1KBbYl77R+\n\tJ37XGULua1lyI4dwZKzMAVYdRANOwRPxaDelzSECEOwo42sUE9PdPB5l98Xg0WK5XV\n\t8yqvUIuTWjS2UfewKD/dE3O10VpBWlyQasSKkDwk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IbBrpdSj\"; dkim-atps=neutral","Message-ID":"<515802c5-363b-7717-0025-70ce76345c79@ideasonboard.com>","Date":"Fri, 19 Aug 2022 09:17:23 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-6-tomi.valkeinen@ideasonboard.com>\n\t<20220818142230.jxh33uwe2ypsfqnp@uno.localdomain>\n\t<4961b9a1-4368-6ac4-83c8-8fbc44815657@ideasonboard.com>\n\t<20220818150017.4igyj6lyjrch7obh@uno.localdomain>\n\t<24179961-2973-7ffe-71d4-f18fe1f54201@ideasonboard.com>\n\t<20220818152620.e42xnshpavgt4vnw@uno.localdomain>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager","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\" "}}]