[{"id":23567,"web_url":"https://patchwork.libcamera.org/comment/23567/","msgid":"<165606511140.1149771.16715214725620660926@Monstersaurus>","date":"2022-06-24T10:05:11","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Tomi Valkeinen (2022-06-23 15:47:33)\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> Note that attaching to requestCompleted signal is a bit funny, as\n> apparently the only way to use a lambda with a signal is to provide an\n> object instance pointer as the first parameter, even if there's really\n> no instance.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/meson.build | 1 +\n> src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++\n> src/py/libcamera/py_camera_manager.h | 39 ++++++++\n> src/py/libcamera/py_main.cpp | 120 ++++++-------------------\n> 4 files changed, 181 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..c9e5a99c\n> --- /dev/null\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -0,0 +1,115 @@\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> +\n> +#include \"py_main.h\"\n> +\n> +namespace py = pybind11;\n> +\n> +using namespace libcamera;\n> +\n> +PyCameraManager::PyCameraManager()\n> +{\n> + int fd = eventfd(0, 0);\n> + if (fd == -1)\n> + throw std::system_error(errno, std::generic_category(),\n> + \"Failed to create eventfd\");\n> +\n> + eventFd_ = fd;\n> +\n> + int ret = start();\n> + if (ret) {\n> + close(fd);\n> + eventFd_ = -1;\n> + throw std::system_error(-ret, std::generic_category(),\n> + \"Failed to start CameraManager\");\n> + }\n> +}\n> +\n> +PyCameraManager::~PyCameraManager()\n> +{\n> + if (eventFd_ != -1) {\n> + close(eventFd_);\n> + eventFd_ = -1;\n> + }\n> +}\n> +\n> +py::list PyCameraManager::getCameras()\n> +{\n> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> + py::list l;\n> +\n> + for (auto &c : cameras()) {\n> + py::object py_cm = py::cast(this);\n> + py::object py_cam = py::cast(c);\n> + py::detail::keep_alive_impl(py_cam, py_cm);\n> + l.append(py_cam);\n> + }\n> +\n> + return l;\n> +}\n> +\n> +std::vector PyCameraManager::getReadyRequests()\n> +{\n> + readFd();\n\nIs this blocking? Does it need any kind of time out of cancellation\nsupport? (What happens if a ctrl-c happens while blocked here etc..)\n\n> +\n> + std::vector v;\n> + getRequests(v);\n> +\n> + std::vector ret;\n> +\n> + for (Request *req : v) {\n> + py::object o = py::cast(req);\n> + /* Decrease the ref increased in Camera.queue_request() */\n> + o.dec_ref();\n> + ret.push_back(o);\n> + }\n> +\n> + return ret;\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleRequestCompleted(Request *req)\n> +{\n> + pushRequest(req);\n> + writeFd();\n> +}\n> +\n> +void PyCameraManager::writeFd()\n> +{\n> + uint64_t v = 1;\n> +\n> + size_t s = write(eventFd_, &v, 8);\n> + /*\n> + * We should never fail, and have no simple means to manage the error,\n> + * so let's use LOG(Python, Fatal).\n> + */\n\nTrivial spacing is wrong here on the last line.\n\n\n> + if (s != 8)\n> + LOG(Python, Fatal) << \"Unable to write to eventfd\";\n\nHrm ... should this tie in to throwing a python error to generate a\npython backtrace or such too ?\n\nIt certainly shouldn't happen though indeed so ... it probably doesn't\nmatter too much now.\n\n\n> +}\n> +\n> +void PyCameraManager::readFd()\n> +{\n> + uint8_t buf[8];\n> +\n> + if (read(eventFd_, buf, 8) != 8)\n> + throw std::system_error(errno, std::generic_category());\n> +}\n> +\n> +void PyCameraManager::pushRequest(Request *req)\n> +{\n> + std::lock_guard guard(reqlist_mutex_);\n\nAha, I was going to ask about locking up in\nPyCameraManager::handleRequestCompleted but now I see it's handled ...\n\n\n> + reqList_.push_back(req);\n> +}\n> +\n> +void PyCameraManager::getRequests(std::vector &v)\n> +{\n> + std::lock_guard guard(reqlist_mutex_);\n> + swap(v, reqList_);\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..b0b971ad\n> --- /dev/null\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -0,0 +1,39 @@\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 : public CameraManager\n> +{\n> +public:\n> + PyCameraManager();\n> + virtual ~PyCameraManager();\n> +\n> + pybind11::list getCameras();\n> +\n> + int eventFd() const { return eventFd_; }\n> +\n> + std::vector getReadyRequests();\n> +\n> + void handleRequestCompleted(Request *req);\n> +\n> +private:\n> + int eventFd_ = -1;\n> + std::mutex reqlist_mutex_;\n> + std::vector reqList_;\n> +\n> + void writeFd();\n> + void readFd();\n> + void pushRequest(Request *req);\n> + void getRequests(std::vector &v);\n> +};\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index 5a423ece..23018288 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> @@ -29,27 +27,11 @@ using namespace libcamera;\n> \n> LOG_DEFINE_CATEGORY(Python)\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> - {\n> - std::lock_guard guard(gReqlistMutex);\n> - gReqList.push_back(req);\n> - }\n> -\n> - uint64_t v = 1;\n> - size_t s = write(gEventfd, &v, 8);\n> - /*\n> - * We should never fail, and have no simple means to manage the error,\n> - * so let's use LOG(Python, Fatal).\n> - */\n> - if (s != 8)\n> - LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> -}\n> +/*\n> + * XXX Note: global C++ destructors can be ran on this before the py module is\n\nXXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight\nfor review, I think it should stay in the code.\n\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> @@ -72,7 +54,7 @@ PYBIND11_MODULE(_libcamera, m)\n> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> */\n> \n> - auto pyCameraManager = py::class_(m, \"CameraManager\");\n> + auto pyCameraManager = py::class_(m, \"CameraManager\");\n> auto pyCamera = py::class_(m, \"Camera\");\n> auto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> auto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> @@ -106,78 +88,22 @@ PYBIND11_MODULE(_libcamera, m)\n> /* Classes */\n> pyCameraManager\n> .def_static(\"singleton\", []() {\n> - std::shared_ptr cm = gCameraManager.lock();\n> - if (cm)\n> - return cm;\n> -\n> - int fd = eventfd(0, 0);\n> - if (fd == -1)\n> - throw std::system_error(errno, std::generic_category(),\n> - \"Failed to create eventfd\");\n> -\n> - cm = std::shared_ptr(new CameraManager, [](auto p) {\n> - close(gEventfd);\n> - gEventfd = -1;\n> - delete p;\n> - });\n> -\n> - gEventfd = fd;\n> - gCameraManager = cm;\n> -\n> - int ret = cm->start();\n> - if (ret)\n> - throw std::system_error(-ret, std::generic_category(),\n> - \"Failed to start CameraManager\");\n> -\n> - return cm;\n> - })\n> -\n> - .def_property_readonly(\"version\", &CameraManager::version)\n> -\n> - .def_property_readonly(\"event_fd\", [](CameraManager &) {\n> - return gEventfd;\n> - })\n> + std::shared_ptr cm = gCameraManager.lock();\n> \n> - .def(\"get_ready_requests\", [](CameraManager &) {\n> - uint8_t buf[8];\n> -\n> - if (read(gEventfd, buf, 8) != 8)\n> - throw std::system_error(errno, std::generic_category());\n> -\n> - std::vector v;\n> -\n> - {\n> - std::lock_guard guard(gReqlistMutex);\n> - swap(v, gReqList);\n> + if (!cm) {\n> + cm = std::make_shared();\n> + gCameraManager = cm;\n> }\n> \n> - std::vector ret;\n> -\n> - for (Request *req : v) {\n> - py::object o = py::cast(req);\n> - /* Decrease the ref increased in Camera.queue_request() */\n> - o.dec_ref();\n> - ret.push_back(o);\n> - }\n> -\n> - return ret;\n> + return cm;\n> })\n> \n> - .def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> + .def_property_readonly(\"version\", &PyCameraManager::version)\n> + .def(\"get\", py::overload_cast(&PyCameraManager::get), py::keep_alive<0, 1>())\n> + .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n> \n> - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> - .def_property_readonly(\"cameras\", [](CameraManager &self) {\n> - py::list l;\n> -\n> - for (auto &c : self.cameras()) {\n> - py::object py_cm = py::cast(self);\n> - py::object py_cam = py::cast(c);\n> - py::detail::keep_alive_impl(py_cam, py_cm);\n> - l.append(py_cam);\n> - }\n> -\n> - return l;\n> - });\n> + .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> + .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> \n> pyCamera\n> .def_property_readonly(\"id\", &Camera::id)\n> @@ -187,7 +113,13 @@ PYBIND11_MODULE(_libcamera, m)\n> const std::unordered_map &controls) {\n> /* \\todo What happens if someone calls start() multiple times? */\n> \n> - self.requestCompleted.connect(handleRequestCompleted);\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> +\n> + cm->handleRequestCompleted(req);\n> + });\n> \n> ControlList controlList(self.controls());\n> \n> @@ -198,7 +130,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \n> int ret = self.start(&controlList);\n> if (ret) {\n> - self.requestCompleted.disconnect(handleRequestCompleted);\n> + self.requestCompleted.disconnect();\n> return ret;\n> }\n> \n> @@ -210,7 +142,7 @@ PYBIND11_MODULE(_libcamera, m)\n> if (ret)\n> return ret;\n> \n> - self.requestCompleted.disconnect(handleRequestCompleted);\n> + self.requestCompleted.disconnect();\n> \n> return 0;\n> })\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 6ED3EBD808\n\tfor ;\n\tFri, 24 Jun 2022 10:05:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0D4E65635;\n\tFri, 24 Jun 2022 12:05:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 012B2600EC\n\tfor ;\n\tFri, 24 Jun 2022 12:05:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65AB3DD;\n\tFri, 24 Jun 2022 12:05:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656065115;\n\tbh=L+/9AwfDGqMojM7oa+R+QAPN6VOI/HnSzIOrw47Luc0=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=KoSvM14dmjGilOzTCTcdnM6oKD5zy1Pqj1Y/Ms3d19dolx8OheQZrYl1R29r+wOdj\n\tiAaTyMecmHggm2SbsBOu3jmIyyD205C0mfA9nkPPbrwwYEz5dxgyn8zuGZYcdY920c\n\tOFyWsraThtO057XKFaw4/w6eys0E068/hd5JCOBnpfQtT7UnjOSaK20vDJa7TUz0uw\n\tiZ0FZrOUAf8zJFDRCCkw5KC7dPGQtijVjUeVLp1r9AQjn5PSD1wBesEvZYgUEqTU8A\n\tWgbXPp7yQYZ7dAd4rX7YepREdeZ15QxpuNyG5lxqvN0L0TT69Dju7Hbpym0pVTruD9\n\tKXe4DgrB+q1gA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656065113;\n\tbh=L+/9AwfDGqMojM7oa+R+QAPN6VOI/HnSzIOrw47Luc0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rSMxK2fUOuhiehzw6oGZfZD7N/+dGX4fW6r0WmsclWg5Ks/r+MK23Qd4M+DxcVFMh\n\tJEMJSMxTNfvHI99WE/Oh1QmFTjaKhJxl6Hjd7vWEKCikXUVNzEFAo1QIHYHWOnjCuT\n\tQNKgSK3vWPDtbZfkpdfus1h2k+SvNKnr9ffpTrnU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rSMxK2fU\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>","To":"David Plowman ,\n\tJacopo Mondi ,\n\tLaurent Pinchart ,\n\tTomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 24 Jun 2022 11:05:11 +0100","Message-ID":"<165606511140.1149771.16715214725620660926@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23577,"web_url":"https://patchwork.libcamera.org/comment/23577/","msgid":"","date":"2022-06-24T13:47:54","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jun 24, 2022 at 11:05:11AM +0100, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2022-06-23 15:47:33)\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> > Note that attaching to requestCompleted signal is a bit funny, as\n> > apparently the only way to use a lambda with a signal is to provide an\n> > object instance pointer as the first parameter, even if there's really\n> > no instance.\n> > \n> > Signed-off-by: Tomi Valkeinen \n> > ---\n> > src/py/libcamera/meson.build | 1 +\n> > src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++\n> > src/py/libcamera/py_camera_manager.h | 39 ++++++++\n> > src/py/libcamera/py_main.cpp | 120 ++++++-------------------\n> > 4 files changed, 181 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..c9e5a99c\n> > --- /dev/null\n> > +++ b/src/py/libcamera/py_camera_manager.cpp\n> > @@ -0,0 +1,115 @@\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> > +\n> > +#include \"py_main.h\"\n> > +\n> > +namespace py = pybind11;\n> > +\n> > +using namespace libcamera;\n> > +\n> > +PyCameraManager::PyCameraManager()\n> > +{\n> > + int fd = eventfd(0, 0);\n\nIn a separate patch I'd set at least the EFD_CLOEXEC flag.\n\n> > + if (fd == -1)\n> > + throw std::system_error(errno, std::generic_category(),\n\n#include \n#include \n\n> > + \"Failed to create eventfd\");\n> > +\n> > + eventFd_ = fd;\n> > +\n> > + int ret = start();\n> > + if (ret) {\n> > + close(fd);\n> > + eventFd_ = -1;\n\nYou could use the UniqueFD class for eventFd_, this could when then\ndisappear. Same for the destructor.\n\n> > + throw std::system_error(-ret, std::generic_category(),\n> > + \"Failed to start CameraManager\");\n> > + }\n> > +}\n> > +\n> > +PyCameraManager::~PyCameraManager()\n> > +{\n> > + if (eventFd_ != -1) {\n> > + close(eventFd_);\n> > + eventFd_ = -1;\n> > + }\n> > +}\n> > +\n> > +py::list PyCameraManager::getCameras()\n\ns/getCameras()/cameras()/ ?\n\n> > +{\n> > + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n\nLine wrap.\n\n> > + py::list l;\n> > +\n> > + for (auto &c : cameras()) {\n\nWe usually try not to shorten names too much. s/c/camera/ would read\nnicely.\n\n> > + py::object py_cm = py::cast(this);\n> > + py::object py_cam = py::cast(c);\n> > + py::detail::keep_alive_impl(py_cam, py_cm);\n> > + l.append(py_cam);\n> > + }\n> > +\n> > + return l;\n> > +}\n> > +\n> > +std::vector PyCameraManager::getReadyRequests()\n> > +{\n> > + readFd();\n> \n> Is this blocking? Does it need any kind of time out of cancellation\n> support? (What happens if a ctrl-c happens while blocked here etc..)\n\nI expect this to be rework in the next patches of the series, but maybe\nsome of these concerns could be addressed already.\n\n> > +\n> > + std::vector v;\n\ns/v/requests/\n\n> > + getRequests(v);\n\nYou can make getRequests() return the vector instead of passing it as a\nreference, and then even do\n\n\tfor (Request *req : getRequests()) {\n\n> > +\n> > + std::vector ret;\n> > +\n> > + for (Request *req : v) {\n\ns/req/request/\n\n> > + py::object o = py::cast(req);\n> > + /* Decrease the ref increased in Camera.queue_request() */\n> > + o.dec_ref();\n> > + ret.push_back(o);\n> > + }\n> > +\n> > + return ret;\n> > +}\n> > +\n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleRequestCompleted(Request *req)\n> > +{\n> > + pushRequest(req);\n> > + writeFd();\n> > +}\n> > +\n> > +void PyCameraManager::writeFd()\n> > +{\n> > + uint64_t v = 1;\n> > +\n> > + size_t s = write(eventFd_, &v, 8);\n> > + /*\n> > + * We should never fail, and have no simple means to manage the error,\n> > + * so let's use LOG(Python, Fatal).\n> > + */\n> \n> Trivial spacing is wrong here on the last line.\n> \n> > + if (s != 8)\n> > + LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> \n> Hrm ... should this tie in to throwing a python error to generate a\n> python backtrace or such too ?\n\nWe can't, as this isn't run from the Python thread.\n\n> It certainly shouldn't happen though indeed so ... it probably doesn't\n> matter too much now.\n> \n> > +}\n> > +\n> > +void PyCameraManager::readFd()\n> > +{\n> > + uint8_t buf[8];\n> > +\n> > + if (read(eventFd_, buf, 8) != 8)\n> > + throw std::system_error(errno, std::generic_category());\n> > +}\n> > +\n> > +void PyCameraManager::pushRequest(Request *req)\n> > +{\n> > + std::lock_guard guard(reqlist_mutex_);\n\nCould you use the MutexLocker class ? That gives us access to\nthread-safety annotations.\n\n> Aha, I was going to ask about locking up in\n> PyCameraManager::handleRequestCompleted but now I see it's handled ...\n> \n> > + reqList_.push_back(req);\n> > +}\n> > +\n> > +void PyCameraManager::getRequests(std::vector &v)\n> > +{\n> > + std::lock_guard guard(reqlist_mutex_);\n> > + swap(v, reqList_);\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..b0b971ad\n> > --- /dev/null\n> > +++ b/src/py/libcamera/py_camera_manager.h\n> > @@ -0,0 +1,39 @@\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 : public CameraManager\n\nCameraManager isn't meant to be inherited from (I'm surprised it's not\nmarked as final). You can keep this for now, but we should soon switch\nto encapsulating the CameraManager instead.\n\n> > +{\n> > +public:\n> > + PyCameraManager();\n> > + virtual ~PyCameraManager();\n> > +\n> > + pybind11::list getCameras();\n> > +\n> > + int eventFd() const { return eventFd_; }\n> > +\n> > + std::vector getReadyRequests();\n> > +\n> > + void handleRequestCompleted(Request *req);\n> > +\n> > +private:\n> > + int eventFd_ = -1;\n> > + std::mutex reqlist_mutex_;\n\nMutex instead of std::mutex, and s/reqlist_mutex_/requestsListMutex_/\n(or similar). You can also call it just lock_, and use the\nLIBCAMERA_TSA_ macros to annotate the fields it protects, that's even\nmore explicit.\n\n> > + std::vector reqList_;\n\ncompletedRequests_\n\n> > +\n> > + void writeFd();\n> > + void readFd();\n> > + void pushRequest(Request *req);\n> > + void getRequests(std::vector &v);\n\ngetCompletedRequests()\n\n> > +};\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index 5a423ece..23018288 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> > @@ -29,27 +27,11 @@ using namespace libcamera;\n> > \n> > LOG_DEFINE_CATEGORY(Python)\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> > - {\n> > - std::lock_guard guard(gReqlistMutex);\n> > - gReqList.push_back(req);\n> > - }\n> > -\n> > - uint64_t v = 1;\n> > - size_t s = write(gEventfd, &v, 8);\n> > - /*\n> > - * We should never fail, and have no simple means to manage the error,\n> > - * so let's use LOG(Python, Fatal).\n> > - */\n> > - if (s != 8)\n> > - LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> > -}\n> > +/*\n> > + * XXX Note: global C++ destructors can be ran on this before the py module is\n> \n> XXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight\n> for review, I think it should stay in the code.\n> \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> > @@ -72,7 +54,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> > */\n> > \n> > - auto pyCameraManager = py::class_(m, \"CameraManager\");\n> > + auto pyCameraManager = py::class_(m, \"CameraManager\");\n> > auto pyCamera = py::class_(m, \"Camera\");\n> > auto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> > auto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> > @@ -106,78 +88,22 @@ PYBIND11_MODULE(_libcamera, m)\n> > /* Classes */\n> > pyCameraManager\n> > .def_static(\"singleton\", []() {\n> > - std::shared_ptr cm = gCameraManager.lock();\n> > - if (cm)\n> > - return cm;\n> > -\n> > - int fd = eventfd(0, 0);\n> > - if (fd == -1)\n> > - throw std::system_error(errno, std::generic_category(),\n> > - \"Failed to create eventfd\");\n> > -\n> > - cm = std::shared_ptr(new CameraManager, [](auto p) {\n> > - close(gEventfd);\n> > - gEventfd = -1;\n> > - delete p;\n> > - });\n> > -\n> > - gEventfd = fd;\n> > - gCameraManager = cm;\n> > -\n> > - int ret = cm->start();\n> > - if (ret)\n> > - throw std::system_error(-ret, std::generic_category(),\n> > - \"Failed to start CameraManager\");\n> > -\n> > - return cm;\n> > - })\n> > -\n> > - .def_property_readonly(\"version\", &CameraManager::version)\n> > -\n> > - .def_property_readonly(\"event_fd\", [](CameraManager &) {\n> > - return gEventfd;\n> > - })\n> > + std::shared_ptr cm = gCameraManager.lock();\n> > \n> > - .def(\"get_ready_requests\", [](CameraManager &) {\n> > - uint8_t buf[8];\n> > -\n> > - if (read(gEventfd, buf, 8) != 8)\n> > - throw std::system_error(errno, std::generic_category());\n> > -\n> > - std::vector v;\n> > -\n> > - {\n> > - std::lock_guard guard(gReqlistMutex);\n> > - swap(v, gReqList);\n> > + if (!cm) {\n> > + cm = std::make_shared();\n> > + gCameraManager = cm;\n> > }\n> > \n> > - std::vector ret;\n> > -\n> > - for (Request *req : v) {\n> > - py::object o = py::cast(req);\n> > - /* Decrease the ref increased in Camera.queue_request() */\n> > - o.dec_ref();\n> > - ret.push_back(o);\n> > - }\n> > -\n> > - return ret;\n> > + return cm;\n> > })\n> > \n> > - .def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> > + .def_property_readonly(\"version\", &PyCameraManager::version)\n> > + .def(\"get\", py::overload_cast(&PyCameraManager::get), py::keep_alive<0, 1>())\n> > + .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n> > \n> > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > - .def_property_readonly(\"cameras\", [](CameraManager &self) {\n> > - py::list l;\n> > -\n> > - for (auto &c : self.cameras()) {\n> > - py::object py_cm = py::cast(self);\n> > - py::object py_cam = py::cast(c);\n> > - py::detail::keep_alive_impl(py_cam, py_cm);\n> > - l.append(py_cam);\n> > - }\n> > -\n> > - return l;\n> > - });\n> > + .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> > + .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> > \n> > pyCamera\n> > .def_property_readonly(\"id\", &Camera::id)\n> > @@ -187,7 +113,13 @@ PYBIND11_MODULE(_libcamera, m)\n> > const std::unordered_map &controls) {\n> > /* \\todo What happens if someone calls start() multiple times? */\n> > \n> > - self.requestCompleted.connect(handleRequestCompleted);\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> > +\n\nExtra blank line.\n\n> > + cm->handleRequestCompleted(req);\n> > + });\n\nSo the following didn't work ?\n\n\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n\n? I can investigate.\n\n> > \n> > ControlList controlList(self.controls());\n> > \n> > @@ -198,7 +130,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > \n> > int ret = self.start(&controlList);\n> > if (ret) {\n> > - self.requestCompleted.disconnect(handleRequestCompleted);\n> > + self.requestCompleted.disconnect();\n> > return ret;\n> > }\n> > \n> > @@ -210,7 +142,7 @@ PYBIND11_MODULE(_libcamera, m)\n> > if (ret)\n> > return ret;\n> > \n> > - self.requestCompleted.disconnect(handleRequestCompleted);\n> > + self.requestCompleted.disconnect();\n> > \n> > return 0;\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 DEA77BD808\n\tfor ;\n\tFri, 24 Jun 2022 13:48:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F68A65638;\n\tFri, 24 Jun 2022 15:48:12 +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 C4F6360412\n\tfor ;\n\tFri, 24 Jun 2022 15:48:10 +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 E5F4F47C;\n\tFri, 24 Jun 2022 15:48:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656078492;\n\tbh=lOOePXELrCxd4aRgKusAB+MlHi/4UGsUnFluPV+LVy8=;\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=pK+OecbxlZ24Z4oJBga/sbmypRlYWhQrYhZfTPAe6cvuPvW0KT1rbgJ1TMMWciW2W\n\ttq2JcXtYe8PC28FETSzMKy1riQQyZXaRPh4jOTSnShHuMgBx7e0XkG+TMPSxDMz7aV\n\t2P3vVtQFI7IXkDVd4xhq6EkI1ou7dfhKRBa5zAxV1Zp2+wCoFnuHPk107uWF1M3A3h\n\t5dkfyhxhjAUHOPyV1qGvvD9XTM5IikpyPVbx61Z23Jjvo8ZU/pA+xbqrKTYUbpiu28\n\tQzXIlu+9Q+5W5Wzk2JYAzlHgRRD40SgFQ8WoXHdGEerACDNtpLtZLmqsky5YVkjF9F\n\tCqp5ZQJhWzCeA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656078490;\n\tbh=lOOePXELrCxd4aRgKusAB+MlHi/4UGsUnFluPV+LVy8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vCGGCj3YPhDtThGZM+XSFVJQ0mvbYbT6605Wl/9pc3zmo6zSTyiuYWoP09vDHdSfI\n\tx+XoEqSj8IgF7FJHoQDrRro5m4BCUebVAWJM4wCSIlFVD37a0bOW//CTM6lOiwgm0u\n\t5vCMY8WIbvaegD/HTpzLDEEf91di7gM3g7i5x1gw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vCGGCj3Y\"; dkim-atps=neutral","Date":"Fri, 24 Jun 2022 16:47:54 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165606511140.1149771.16715214725620660926@Monstersaurus>","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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":23589,"web_url":"https://patchwork.libcamera.org/comment/23589/","msgid":"","date":"2022-06-27T07:22:16","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 24/06/2022 13:05, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2022-06-23 15:47:33)\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>> Note that attaching to requestCompleted signal is a bit funny, as\n>> apparently the only way to use a lambda with a signal is to provide an\n>> object instance pointer as the first parameter, even if there's really\n>> no instance.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/meson.build | 1 +\n>> src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++\n>> src/py/libcamera/py_camera_manager.h | 39 ++++++++\n>> src/py/libcamera/py_main.cpp | 120 ++++++-------------------\n>> 4 files changed, 181 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..c9e5a99c\n>> --- /dev/null\n>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>> @@ -0,0 +1,115 @@\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>> +\n>> +#include \"py_main.h\"\n>> +\n>> +namespace py = pybind11;\n>> +\n>> +using namespace libcamera;\n>> +\n>> +PyCameraManager::PyCameraManager()\n>> +{\n>> + int fd = eventfd(0, 0);\n>> + if (fd == -1)\n>> + throw std::system_error(errno, std::generic_category(),\n>> + \"Failed to create eventfd\");\n>> +\n>> + eventFd_ = fd;\n>> +\n>> + int ret = start();\n>> + if (ret) {\n>> + close(fd);\n>> + eventFd_ = -1;\n>> + throw std::system_error(-ret, std::generic_category(),\n>> + \"Failed to start CameraManager\");\n>> + }\n>> +}\n>> +\n>> +PyCameraManager::~PyCameraManager()\n>> +{\n>> + if (eventFd_ != -1) {\n>> + close(eventFd_);\n>> + eventFd_ = -1;\n>> + }\n>> +}\n>> +\n>> +py::list PyCameraManager::getCameras()\n>> +{\n>> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n>> + py::list l;\n>> +\n>> + for (auto &c : cameras()) {\n>> + py::object py_cm = py::cast(this);\n>> + py::object py_cam = py::cast(c);\n>> + py::detail::keep_alive_impl(py_cam, py_cm);\n>> + l.append(py_cam);\n>> + }\n>> +\n>> + return l;\n>> +}\n>> +\n>> +std::vector PyCameraManager::getReadyRequests()\n>> +{\n>> + readFd();\n> \n> Is this blocking? Does it need any kind of time out of cancellation\n> support? (What happens if a ctrl-c happens while blocked here etc..)\n\nYes. No. \"RuntimeError: Interrupted system call\".\n\n>> +\n>> + std::vector v;\n>> + getRequests(v);\n>> +\n>> + std::vector ret;\n>> +\n>> + for (Request *req : v) {\n>> + py::object o = py::cast(req);\n>> + /* Decrease the ref increased in Camera.queue_request() */\n>> + o.dec_ref();\n>> + ret.push_back(o);\n>> + }\n>> +\n>> + return ret;\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleRequestCompleted(Request *req)\n>> +{\n>> + pushRequest(req);\n>> + writeFd();\n>> +}\n>> +\n>> +void PyCameraManager::writeFd()\n>> +{\n>> + uint64_t v = 1;\n>> +\n>> + size_t s = write(eventFd_, &v, 8);\n>> + /*\n>> + * We should never fail, and have no simple means to manage the error,\n>> + * so let's use LOG(Python, Fatal).\n>> + */\n> \n> Trivial spacing is wrong here on the last line.\n\nHmm interesting, it was indented with spaces, but no tool complained \nabout it.\n\n> \n>> + if (s != 8)\n>> + LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> \n> Hrm ... should this tie in to throwing a python error to generate a\n> python backtrace or such too ?\n> \n> It certainly shouldn't happen though indeed so ... it probably doesn't\n> matter too much now.\n\nThis is called in a callback from libcamera, in non-Python thread. We \ncould possibly set some flag, and then react to that flag when the \nPython thread handles the events. But... This should really never \nhappen, so I think that's not needed.\n\n>> +}\n>> +\n>> +void PyCameraManager::readFd()\n>> +{\n>> + uint8_t buf[8];\n>> +\n>> + if (read(eventFd_, buf, 8) != 8)\n>> + throw std::system_error(errno, std::generic_category());\n>> +}\n>> +\n>> +void PyCameraManager::pushRequest(Request *req)\n>> +{\n>> + std::lock_guard guard(reqlist_mutex_);\n> \n> Aha, I was going to ask about locking up in\n> PyCameraManager::handleRequestCompleted but now I see it's handled ...\n> \n> \n>> + reqList_.push_back(req);\n>> +}\n>> +\n>> +void PyCameraManager::getRequests(std::vector &v)\n>> +{\n>> + std::lock_guard guard(reqlist_mutex_);\n>> + swap(v, reqList_);\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..b0b971ad\n>> --- /dev/null\n>> +++ b/src/py/libcamera/py_camera_manager.h\n>> @@ -0,0 +1,39 @@\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 : public CameraManager\n>> +{\n>> +public:\n>> + PyCameraManager();\n>> + virtual ~PyCameraManager();\n>> +\n>> + pybind11::list getCameras();\n>> +\n>> + int eventFd() const { return eventFd_; }\n>> +\n>> + std::vector getReadyRequests();\n>> +\n>> + void handleRequestCompleted(Request *req);\n>> +\n>> +private:\n>> + int eventFd_ = -1;\n>> + std::mutex reqlist_mutex_;\n>> + std::vector reqList_;\n>> +\n>> + void writeFd();\n>> + void readFd();\n>> + void pushRequest(Request *req);\n>> + void getRequests(std::vector &v);\n>> +};\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index 5a423ece..23018288 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>> @@ -29,27 +27,11 @@ using namespace libcamera;\n>> \n>> LOG_DEFINE_CATEGORY(Python)\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>> - {\n>> - std::lock_guard guard(gReqlistMutex);\n>> - gReqList.push_back(req);\n>> - }\n>> -\n>> - uint64_t v = 1;\n>> - size_t s = write(gEventfd, &v, 8);\n>> - /*\n>> - * We should never fail, and have no simple means to manage the error,\n>> - * so let's use LOG(Python, Fatal).\n>> - */\n>> - if (s != 8)\n>> - LOG(Python, Fatal) << \"Unable to write to eventfd\";\n>> -}\n>> +/*\n>> + * XXX Note: global C++ destructors can be ran on this before the py module is\n> \n> XXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight\n> for review, I think it should stay in the code.\n\nAh, I forgot to remove the XXX. It's not really a todo or even a \nwarning. Just a reminder if hitting odd problems with object lifetimes. \nI think I got those fixed with this series, but it doesn't hurt to keep \nthe comment so that I remember the next time =).\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 09F84BD808\n\tfor ;\n\tMon, 27 Jun 2022 07:22:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C42A65637;\n\tMon, 27 Jun 2022 09:22:21 +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 5122D60412\n\tfor ;\n\tMon, 27 Jun 2022 09:22:20 +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 7F15A47C;\n\tMon, 27 Jun 2022 09:22:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656314541;\n\tbh=GLaoqRoF/0g8JLm3YEq8+z0kpqvEfQ1K1JTGiUsnT78=;\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=VvdAumxv2xfifrmBthEiacL0exe/u3sApEwdQFB6L85M7FSEnUiaeWWV/ausaTtmj\n\tJbfg+22D4kc17MmqFIbRWX4+WKzu4Zne8ekixj+zeMd4kJalGyK22ZuSyKrh7kuAyk\n\ttstU4v5Tlzk7CRUAJ0sTP664qWmzsLGNdKN5/uVX5A1AabL1Kesb2T2OhoDw52qLXn\n\teAolcCpWe4z+5Re6pHh5qD7wRZjlPwiAsU6IwwqH/hlcxwaPs7YM4WMfQhACtDKlB7\n\tjPCts1ifA3liVSZp0GBHjp6FbaWDqv3hidCyC2yzYPwdzXmUorEe6G5R/7bhIAfxGr\n\ti+tE/o9ys6H6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656314539;\n\tbh=GLaoqRoF/0g8JLm3YEq8+z0kpqvEfQ1K1JTGiUsnT78=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=vhAT7chK4hQJlSJ2Vdy7ukvgo2PIElML7oqIvjL/2sB7hLZOXRWXBB3MeE/5zmcrX\n\tUo5DFh5naB2I83nbyM0/3GqLnYNOW5LEwo9nr9QlOQuNzK596DTQCHz5sGZUry7/iQ\n\tRzLxoTe1nwazwUG5p94mkqczzoHLKgt0tO9ijGGY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vhAT7chK\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 27 Jun 2022 10:22:16 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tDavid Plowman ,\n\tJacopo Mondi ,\n\tLaurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>","In-Reply-To":"<165606511140.1149771.16715214725620660926@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23593,"web_url":"https://patchwork.libcamera.org/comment/23593/","msgid":"","date":"2022-06-27T08:52:17","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 24/06/2022 16:47, Laurent Pinchart wrote:\n> On Fri, Jun 24, 2022 at 11:05:11AM +0100, Kieran Bingham wrote:\n>> Quoting Tomi Valkeinen (2022-06-23 15:47:33)\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>>> Note that attaching to requestCompleted signal is a bit funny, as\n>>> apparently the only way to use a lambda with a signal is to provide an\n>>> object instance pointer as the first parameter, even if there's really\n>>> no instance.\n>>>\n>>> Signed-off-by: Tomi Valkeinen \n>>> ---\n>>> src/py/libcamera/meson.build | 1 +\n>>> src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++\n>>> src/py/libcamera/py_camera_manager.h | 39 ++++++++\n>>> src/py/libcamera/py_main.cpp | 120 ++++++-------------------\n>>> 4 files changed, 181 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..c9e5a99c\n>>> --- /dev/null\n>>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>>> @@ -0,0 +1,115 @@\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>>> +\n>>> +#include \"py_main.h\"\n>>> +\n>>> +namespace py = pybind11;\n>>> +\n>>> +using namespace libcamera;\n>>> +\n>>> +PyCameraManager::PyCameraManager()\n>>> +{\n>>> + int fd = eventfd(0, 0);\n> \n> In a separate patch I'd set at least the EFD_CLOEXEC flag.\n\nOk.\n\n>>> + if (fd == -1)\n>>> + throw std::system_error(errno, std::generic_category(),\n> \n> #include \n> #include \n\nOk.\n\n>>> + \"Failed to create eventfd\");\n>>> +\n>>> + eventFd_ = fd;\n>>> +\n>>> + int ret = start();\n>>> + if (ret) {\n>>> + close(fd);\n>>> + eventFd_ = -1;\n> \n> You could use the UniqueFD class for eventFd_, this could when then\n> disappear. Same for the destructor.\n\nYep. I'll do that in a separate patch, as here I'm just trying to move \nthe code.\n\n>>> + throw std::system_error(-ret, std::generic_category(),\n>>> + \"Failed to start CameraManager\");\n>>> + }\n>>> +}\n>>> +\n>>> +PyCameraManager::~PyCameraManager()\n>>> +{\n>>> + if (eventFd_ != -1) {\n>>> + close(eventFd_);\n>>> + eventFd_ = -1;\n>>> + }\n>>> +}\n>>> +\n>>> +py::list PyCameraManager::getCameras()\n> \n> s/getCameras()/cameras()/ ?\n\nCameraManager already has cameras()...\n\n>>> +{\n>>> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> \n> Line wrap.\n\nOk.\n\n>>> + py::list l;\n>>> +\n>>> + for (auto &c : cameras()) {\n> \n> We usually try not to shorten names too much. s/c/camera/ would read\n> nicely.\n\nOk.\n\n>>> + py::object py_cm = py::cast(this);\n>>> + py::object py_cam = py::cast(c);\n>>> + py::detail::keep_alive_impl(py_cam, py_cm);\n>>> + l.append(py_cam);\n>>> + }\n>>> +\n>>> + return l;\n>>> +}\n>>> +\n>>> +std::vector PyCameraManager::getReadyRequests()\n>>> +{\n>>> + readFd();\n>>\n>> Is this blocking? Does it need any kind of time out of cancellation\n>> support? (What happens if a ctrl-c happens while blocked here etc..)\n> \n> I expect this to be rework in the next patches of the series, but maybe\n> some of these concerns could be addressed already.\n\nI take it the concern was about ctrl-c, and that works?\n\n>>> +\n>>> + std::vector v;\n> \n> s/v/requests/\n\nOk.\n\n>>> + getRequests(v);\n> \n> You can make getRequests() return the vector instead of passing it as a\n> reference, and then even do\n> \n> \tfor (Request *req : getRequests()) {\n\nOk. I usually keep away from those as I don't seem to ever exactly know \nwhen copies are made and when not...\n\n>>> +\n>>> + std::vector ret;\n>>> +\n>>> + for (Request *req : v) {\n> \n> s/req/request/\n\nOk.\n\n>>> + py::object o = py::cast(req);\n>>> + /* Decrease the ref increased in Camera.queue_request() */\n>>> + o.dec_ref();\n>>> + ret.push_back(o);\n>>> + }\n>>> +\n>>> + return ret;\n>>> +}\n>>> +\n>>> +/* Note: Called from another thread */\n>>> +void PyCameraManager::handleRequestCompleted(Request *req)\n>>> +{\n>>> + pushRequest(req);\n>>> + writeFd();\n>>> +}\n>>> +\n>>> +void PyCameraManager::writeFd()\n>>> +{\n>>> + uint64_t v = 1;\n>>> +\n>>> + size_t s = write(eventFd_, &v, 8);\n>>> + /*\n>>> + * We should never fail, and have no simple means to manage the error,\n>>> + * so let's use LOG(Python, Fatal).\n>>> + */\n>>\n>> Trivial spacing is wrong here on the last line.\n>>\n>>> + if (s != 8)\n>>> + LOG(Python, Fatal) << \"Unable to write to eventfd\";\n>>\n>> Hrm ... should this tie in to throwing a python error to generate a\n>> python backtrace or such too ?\n> \n> We can't, as this isn't run from the Python thread.\n> \n>> It certainly shouldn't happen though indeed so ... it probably doesn't\n>> matter too much now.\n>>\n>>> +}\n>>> +\n>>> +void PyCameraManager::readFd()\n>>> +{\n>>> + uint8_t buf[8];\n>>> +\n>>> + if (read(eventFd_, buf, 8) != 8)\n>>> + throw std::system_error(errno, std::generic_category());\n>>> +}\n>>> +\n>>> +void PyCameraManager::pushRequest(Request *req)\n>>> +{\n>>> + std::lock_guard guard(reqlist_mutex_);\n> \n> Could you use the MutexLocker class ? That gives us access to\n> thread-safety annotations.\n\nOk. I'll look at that in a separate patch.\n\n>> Aha, I was going to ask about locking up in\n>> PyCameraManager::handleRequestCompleted but now I see it's handled ...\n>>\n>>> + reqList_.push_back(req);\n>>> +}\n>>> +\n>>> +void PyCameraManager::getRequests(std::vector &v)\n>>> +{\n>>> + std::lock_guard guard(reqlist_mutex_);\n>>> + swap(v, reqList_);\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..b0b971ad\n>>> --- /dev/null\n>>> +++ b/src/py/libcamera/py_camera_manager.h\n>>> @@ -0,0 +1,39 @@\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 : public CameraManager\n> \n> CameraManager isn't meant to be inherited from (I'm surprised it's not\n> marked as final). You can keep this for now, but we should soon switch\n> to encapsulating the CameraManager instead.\n\nHmm that may be problematic. At the moment if there's a CameraManager \npointer, the pybind11 bindings can easily convert that instance to a \nPython instance. But if we encapsulate it, that's no longer the case, as \nyou need something extra to associate the CameraManager pointer to the \nencapsulating class.\n\nI tried to do that with Cameras at some point (trying to sort out the \ndestructor issue), but I didn't get it working. Maybe I'll try again \nwith CameraManager this time.\n\n>>> +{\n>>> +public:\n>>> + PyCameraManager();\n>>> + virtual ~PyCameraManager();\n>>> +\n>>> + pybind11::list getCameras();\n>>> +\n>>> + int eventFd() const { return eventFd_; }\n>>> +\n>>> + std::vector getReadyRequests();\n>>> +\n>>> + void handleRequestCompleted(Request *req);\n>>> +\n>>> +private:\n>>> + int eventFd_ = -1;\n>>> + std::mutex reqlist_mutex_;\n> \n> Mutex instead of std::mutex, and s/reqlist_mutex_/requestsListMutex_/\n> (or similar). You can also call it just lock_, and use the\n> LIBCAMERA_TSA_ macros to annotate the fields it protects, that's even\n> more explicit.\n\nOk. I'll look at that in a separate patch.\n\n>>> + std::vector reqList_;\n> \n> completedRequests_\n\nOk.\n\n>>> +\n>>> + void writeFd();\n>>> + void readFd();\n>>> + void pushRequest(Request *req);\n>>> + void getRequests(std::vector &v);\n> \n> getCompletedRequests()\n\nOk.\n\n>>> +};\n>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>>> index 5a423ece..23018288 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>>> @@ -29,27 +27,11 @@ using namespace libcamera;\n>>> \n>>> LOG_DEFINE_CATEGORY(Python)\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>>> - {\n>>> - std::lock_guard guard(gReqlistMutex);\n>>> - gReqList.push_back(req);\n>>> - }\n>>> -\n>>> - uint64_t v = 1;\n>>> - size_t s = write(gEventfd, &v, 8);\n>>> - /*\n>>> - * We should never fail, and have no simple means to manage the error,\n>>> - * so let's use LOG(Python, Fatal).\n>>> - */\n>>> - if (s != 8)\n>>> - LOG(Python, Fatal) << \"Unable to write to eventfd\";\n>>> -}\n>>> +/*\n>>> + * XXX Note: global C++ destructors can be ran on this before the py module is\n>>\n>> XXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight\n>> for review, I think it should stay in the code.\n>>\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>>> @@ -72,7 +54,7 @@ PYBIND11_MODULE(_libcamera, m)\n>>> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n>>> */\n>>> \n>>> - auto pyCameraManager = py::class_(m, \"CameraManager\");\n>>> + auto pyCameraManager = py::class_(m, \"CameraManager\");\n>>> auto pyCamera = py::class_(m, \"Camera\");\n>>> auto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n>>> auto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n>>> @@ -106,78 +88,22 @@ PYBIND11_MODULE(_libcamera, m)\n>>> /* Classes */\n>>> pyCameraManager\n>>> .def_static(\"singleton\", []() {\n>>> - std::shared_ptr cm = gCameraManager.lock();\n>>> - if (cm)\n>>> - return cm;\n>>> -\n>>> - int fd = eventfd(0, 0);\n>>> - if (fd == -1)\n>>> - throw std::system_error(errno, std::generic_category(),\n>>> - \"Failed to create eventfd\");\n>>> -\n>>> - cm = std::shared_ptr(new CameraManager, [](auto p) {\n>>> - close(gEventfd);\n>>> - gEventfd = -1;\n>>> - delete p;\n>>> - });\n>>> -\n>>> - gEventfd = fd;\n>>> - gCameraManager = cm;\n>>> -\n>>> - int ret = cm->start();\n>>> - if (ret)\n>>> - throw std::system_error(-ret, std::generic_category(),\n>>> - \"Failed to start CameraManager\");\n>>> -\n>>> - return cm;\n>>> - })\n>>> -\n>>> - .def_property_readonly(\"version\", &CameraManager::version)\n>>> -\n>>> - .def_property_readonly(\"event_fd\", [](CameraManager &) {\n>>> - return gEventfd;\n>>> - })\n>>> + std::shared_ptr cm = gCameraManager.lock();\n>>> \n>>> - .def(\"get_ready_requests\", [](CameraManager &) {\n>>> - uint8_t buf[8];\n>>> -\n>>> - if (read(gEventfd, buf, 8) != 8)\n>>> - throw std::system_error(errno, std::generic_category());\n>>> -\n>>> - std::vector v;\n>>> -\n>>> - {\n>>> - std::lock_guard guard(gReqlistMutex);\n>>> - swap(v, gReqList);\n>>> + if (!cm) {\n>>> + cm = std::make_shared();\n>>> + gCameraManager = cm;\n>>> }\n>>> \n>>> - std::vector ret;\n>>> -\n>>> - for (Request *req : v) {\n>>> - py::object o = py::cast(req);\n>>> - /* Decrease the ref increased in Camera.queue_request() */\n>>> - o.dec_ref();\n>>> - ret.push_back(o);\n>>> - }\n>>> -\n>>> - return ret;\n>>> + return cm;\n>>> })\n>>> \n>>> - .def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n>>> + .def_property_readonly(\"version\", &PyCameraManager::version)\n>>> + .def(\"get\", py::overload_cast(&PyCameraManager::get), py::keep_alive<0, 1>())\n>>> + .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n>>> \n>>> - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n>>> - .def_property_readonly(\"cameras\", [](CameraManager &self) {\n>>> - py::list l;\n>>> -\n>>> - for (auto &c : self.cameras()) {\n>>> - py::object py_cm = py::cast(self);\n>>> - py::object py_cam = py::cast(c);\n>>> - py::detail::keep_alive_impl(py_cam, py_cm);\n>>> - l.append(py_cam);\n>>> - }\n>>> -\n>>> - return l;\n>>> - });\n>>> + .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n>>> + .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n>>> \n>>> pyCamera\n>>> .def_property_readonly(\"id\", &Camera::id)\n>>> @@ -187,7 +113,13 @@ PYBIND11_MODULE(_libcamera, m)\n>>> const std::unordered_map &controls) {\n>>> /* \\todo What happens if someone calls start() multiple times? */\n>>> \n>>> - self.requestCompleted.connect(handleRequestCompleted);\n>>> + auto cm = gCameraManager.lock();\n>>> + assert(cm);\n>>> +\n>>> + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n>>> +\n> \n> Extra blank line.\n\nYep.\n\n>>> + cm->handleRequestCompleted(req);\n>>> + });\n> \n> So the following didn't work ?\n> \n> \t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> \n> ? I can investigate.\n\nI didn't try that. I don't think what you have above is the same. Note \nthat in my version I store a shared_ptr to make sure it \nstays alive.\n\nThe issue I had (which I asked about in the chat) was in the \"New event \nhandling\", when connecting the cameraAdded signal.\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 89E0EBE173\n\tfor ;\n\tMon, 27 Jun 2022 08:52:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE90B65637;\n\tMon, 27 Jun 2022 10:52: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 4FA0D60412\n\tfor ;\n\tMon, 27 Jun 2022 10:52: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 5B63547C;\n\tMon, 27 Jun 2022 10:52:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656319942;\n\tbh=Mh6PbU7mmDWPGq02FlkDpXY0RdIf3AAm8Q0lTWoUvpk=;\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=19C2yLYd+T9+w2L0g04X0UKCgWCV5/eGKylJJRbwZm/XCgAQ4Xg6pTJWuA5KaU7rw\n\tb0MbCb2wY3RDtcuKAywfDgrBqpnxSLHRTLf0LqLzGfhi/h3NBdns1ODkxMUFUCcyr7\n\trGfBjiUD8ONfwTbzbwx9bx8fQU46aQTuFR5SdPnHlr0WTsGocjZFZBsVGRS7d8BRgw\n\tlCNN22zo/SVVUTaEVg7UVvAywopizMHL3Oo8fbQKcvrAywy3Je+VleUaaiujJ6Awyz\n\tZk9N4MZTjxk6DdDbRy70qATKhbFJd9Yxok5E5mgp0iP/g8Ga1FAEJNV0QgDiMsLCUs\n\tnxXfWTgeLwStg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656319940;\n\tbh=Mh6PbU7mmDWPGq02FlkDpXY0RdIf3AAm8Q0lTWoUvpk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=NLh3leTiZrRtGVAEuEG4Meo1HpPZ7O9J9m6ZlEr6NAg8LoWei49MqMQzJNdPOuHz4\n\tAT6qfYkH4CaZTVDnCcUKDxdWbTG2sd7z+wYlICJNiuX6HQN4kBPu0uHZ04BEv52jMm\n\t96E1IySfp9rzolMzmpcrTBVx7bfOHlzZFDEh0v2M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NLh3leTi\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 27 Jun 2022 11:52:17 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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":23599,"web_url":"https://patchwork.libcamera.org/comment/23599/","msgid":"","date":"2022-06-27T10:25:17","subject":"Re: [libcamera-devel] [RFC v1 4/7] 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 Mon, Jun 27, 2022 at 11:52:17AM +0300, Tomi Valkeinen wrote:\n> On 24/06/2022 16:47, Laurent Pinchart wrote:\n> > On Fri, Jun 24, 2022 at 11:05:11AM +0100, Kieran Bingham wrote:\n> >> Quoting Tomi Valkeinen (2022-06-23 15:47:33)\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> >>> Note that attaching to requestCompleted signal is a bit funny, as\n> >>> apparently the only way to use a lambda with a signal is to provide an\n> >>> object instance pointer as the first parameter, even if there's really\n> >>> no instance.\n> >>>\n> >>> Signed-off-by: Tomi Valkeinen \n> >>> ---\n> >>> src/py/libcamera/meson.build | 1 +\n> >>> src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++\n> >>> src/py/libcamera/py_camera_manager.h | 39 ++++++++\n> >>> src/py/libcamera/py_main.cpp | 120 ++++++-------------------\n> >>> 4 files changed, 181 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..c9e5a99c\n> >>> --- /dev/null\n> >>> +++ b/src/py/libcamera/py_camera_manager.cpp\n> >>> @@ -0,0 +1,115 @@\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> >>> +\n> >>> +#include \"py_main.h\"\n> >>> +\n> >>> +namespace py = pybind11;\n> >>> +\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +PyCameraManager::PyCameraManager()\n> >>> +{\n> >>> + int fd = eventfd(0, 0);\n> > \n> > In a separate patch I'd set at least the EFD_CLOEXEC flag.\n> \n> Ok.\n> \n> >>> + if (fd == -1)\n> >>> + throw std::system_error(errno, std::generic_category(),\n> > \n> > #include \n> > #include \n> \n> Ok.\n> \n> >>> + \"Failed to create eventfd\");\n> >>> +\n> >>> + eventFd_ = fd;\n> >>> +\n> >>> + int ret = start();\n> >>> + if (ret) {\n> >>> + close(fd);\n> >>> + eventFd_ = -1;\n> > \n> > You could use the UniqueFD class for eventFd_, this could when then\n> > disappear. Same for the destructor.\n> \n> Yep. I'll do that in a separate patch, as here I'm just trying to move \n> the code.\n> \n> >>> + throw std::system_error(-ret, std::generic_category(),\n> >>> + \"Failed to start CameraManager\");\n> >>> + }\n> >>> +}\n> >>> +\n> >>> +PyCameraManager::~PyCameraManager()\n> >>> +{\n> >>> + if (eventFd_ != -1) {\n> >>> + close(eventFd_);\n> >>> + eventFd_ = -1;\n> >>> + }\n> >>> +}\n> >>> +\n> >>> +py::list PyCameraManager::getCameras()\n> > \n> > s/getCameras()/cameras()/ ?\n> \n> CameraManager already has cameras()...\n> \n> >>> +{\n> >>> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> > \n> > Line wrap.\n> \n> Ok.\n> \n> >>> + py::list l;\n> >>> +\n> >>> + for (auto &c : cameras()) {\n> > \n> > We usually try not to shorten names too much. s/c/camera/ would read\n> > nicely.\n> \n> Ok.\n> \n> >>> + py::object py_cm = py::cast(this);\n> >>> + py::object py_cam = py::cast(c);\n> >>> + py::detail::keep_alive_impl(py_cam, py_cm);\n> >>> + l.append(py_cam);\n> >>> + }\n> >>> +\n> >>> + return l;\n> >>> +}\n> >>> +\n> >>> +std::vector PyCameraManager::getReadyRequests()\n> >>> +{\n> >>> + readFd();\n> >>\n> >> Is this blocking? Does it need any kind of time out of cancellation\n> >> support? (What happens if a ctrl-c happens while blocked here etc..)\n> > \n> > I expect this to be rework in the next patches of the series, but maybe\n> > some of these concerns could be addressed already.\n> \n> I take it the concern was about ctrl-c, and that works?\n\nAny of them, as reasonably feasible :-)\n\n> >>> +\n> >>> + std::vector v;\n> > \n> > s/v/requests/\n> \n> Ok.\n> \n> >>> + getRequests(v);\n> > \n> > You can make getRequests() return the vector instead of passing it as a\n> > reference, and then even do\n> > \n> > \tfor (Request *req : getRequests()) {\n> \n> Ok. I usually keep away from those as I don't seem to ever exactly know \n> when copies are made and when not...\n\nWith copy elision the vector returned by getRequest() will not be copied.\n\n> >>> +\n> >>> + std::vector ret;\n> >>> +\n> >>> + for (Request *req : v) {\n> > \n> > s/req/request/\n> \n> Ok.\n> \n> >>> + py::object o = py::cast(req);\n> >>> + /* Decrease the ref increased in Camera.queue_request() */\n> >>> + o.dec_ref();\n> >>> + ret.push_back(o);\n> >>> + }\n> >>> +\n> >>> + return ret;\n> >>> +}\n> >>> +\n> >>> +/* Note: Called from another thread */\n> >>> +void PyCameraManager::handleRequestCompleted(Request *req)\n> >>> +{\n> >>> + pushRequest(req);\n> >>> + writeFd();\n> >>> +}\n> >>> +\n> >>> +void PyCameraManager::writeFd()\n> >>> +{\n> >>> + uint64_t v = 1;\n> >>> +\n> >>> + size_t s = write(eventFd_, &v, 8);\n> >>> + /*\n> >>> + * We should never fail, and have no simple means to manage the error,\n> >>> + * so let's use LOG(Python, Fatal).\n> >>> + */\n> >>\n> >> Trivial spacing is wrong here on the last line.\n> >>\n> >>> + if (s != 8)\n> >>> + LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> >>\n> >> Hrm ... should this tie in to throwing a python error to generate a\n> >> python backtrace or such too ?\n> > \n> > We can't, as this isn't run from the Python thread.\n> > \n> >> It certainly shouldn't happen though indeed so ... it probably doesn't\n> >> matter too much now.\n> >>\n> >>> +}\n> >>> +\n> >>> +void PyCameraManager::readFd()\n> >>> +{\n> >>> + uint8_t buf[8];\n> >>> +\n> >>> + if (read(eventFd_, buf, 8) != 8)\n> >>> + throw std::system_error(errno, std::generic_category());\n> >>> +}\n> >>> +\n> >>> +void PyCameraManager::pushRequest(Request *req)\n> >>> +{\n> >>> + std::lock_guard guard(reqlist_mutex_);\n> > \n> > Could you use the MutexLocker class ? That gives us access to\n> > thread-safety annotations.\n> \n> Ok. I'll look at that in a separate patch.\n> \n> >> Aha, I was going to ask about locking up in\n> >> PyCameraManager::handleRequestCompleted but now I see it's handled ...\n> >>\n> >>> + reqList_.push_back(req);\n> >>> +}\n> >>> +\n> >>> +void PyCameraManager::getRequests(std::vector &v)\n> >>> +{\n> >>> + std::lock_guard guard(reqlist_mutex_);\n> >>> + swap(v, reqList_);\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..b0b971ad\n> >>> --- /dev/null\n> >>> +++ b/src/py/libcamera/py_camera_manager.h\n> >>> @@ -0,0 +1,39 @@\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 : public CameraManager\n> > \n> > CameraManager isn't meant to be inherited from (I'm surprised it's not\n> > marked as final). You can keep this for now, but we should soon switch\n> > to encapsulating the CameraManager instead.\n> \n> Hmm that may be problematic. At the moment if there's a CameraManager \n> pointer, the pybind11 bindings can easily convert that instance to a \n> Python instance. But if we encapsulate it, that's no longer the case, as \n> you need something extra to associate the CameraManager pointer to the \n> encapsulating class.\n> \n> I tried to do that with Cameras at some point (trying to sort out the \n> destructor issue), but I didn't get it working. Maybe I'll try again \n> with CameraManager this time.\n\nGiven that both the CameraManager and the PyCameraManager can only be\ninstantiated once, the Python code could unconditionally use the\nPyCameraManager, without ever using the CameraManager pointer directly.\nDo we even have any event that passes the CameraManager as an argument ?\nFor Camera instances we do, so that's indeed more problematic.\n\n> >>> +{\n> >>> +public:\n> >>> + PyCameraManager();\n> >>> + virtual ~PyCameraManager();\n> >>> +\n> >>> + pybind11::list getCameras();\n> >>> +\n> >>> + int eventFd() const { return eventFd_; }\n> >>> +\n> >>> + std::vector getReadyRequests();\n> >>> +\n> >>> + void handleRequestCompleted(Request *req);\n> >>> +\n> >>> +private:\n> >>> + int eventFd_ = -1;\n> >>> + std::mutex reqlist_mutex_;\n> > \n> > Mutex instead of std::mutex, and s/reqlist_mutex_/requestsListMutex_/\n> > (or similar). You can also call it just lock_, and use the\n> > LIBCAMERA_TSA_ macros to annotate the fields it protects, that's even\n> > more explicit.\n> \n> Ok. I'll look at that in a separate patch.\n> \n> >>> + std::vector reqList_;\n> > \n> > completedRequests_\n> \n> Ok.\n> \n> >>> +\n> >>> + void writeFd();\n> >>> + void readFd();\n> >>> + void pushRequest(Request *req);\n> >>> + void getRequests(std::vector &v);\n> > \n> > getCompletedRequests()\n> \n> Ok.\n> \n> >>> +};\n> >>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> >>> index 5a423ece..23018288 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> >>> @@ -29,27 +27,11 @@ using namespace libcamera;\n> >>> \n> >>> LOG_DEFINE_CATEGORY(Python)\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> >>> - {\n> >>> - std::lock_guard guard(gReqlistMutex);\n> >>> - gReqList.push_back(req);\n> >>> - }\n> >>> -\n> >>> - uint64_t v = 1;\n> >>> - size_t s = write(gEventfd, &v, 8);\n> >>> - /*\n> >>> - * We should never fail, and have no simple means to manage the error,\n> >>> - * so let's use LOG(Python, Fatal).\n> >>> - */\n> >>> - if (s != 8)\n> >>> - LOG(Python, Fatal) << \"Unable to write to eventfd\";\n> >>> -}\n> >>> +/*\n> >>> + * XXX Note: global C++ destructors can be ran on this before the py module is\n> >>\n> >> XXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight\n> >> for review, I think it should stay in the code.\n> >>\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> >>> @@ -72,7 +54,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >>> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> >>> */\n> >>> \n> >>> - auto pyCameraManager = py::class_(m, \"CameraManager\");\n> >>> + auto pyCameraManager = py::class_(m, \"CameraManager\");\n> >>> auto pyCamera = py::class_(m, \"Camera\");\n> >>> auto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> >>> auto pyCameraConfigurationStatus = py::enum_(pyCameraConfiguration, \"Status\");\n> >>> @@ -106,78 +88,22 @@ PYBIND11_MODULE(_libcamera, m)\n> >>> /* Classes */\n> >>> pyCameraManager\n> >>> .def_static(\"singleton\", []() {\n> >>> - std::shared_ptr cm = gCameraManager.lock();\n> >>> - if (cm)\n> >>> - return cm;\n> >>> -\n> >>> - int fd = eventfd(0, 0);\n> >>> - if (fd == -1)\n> >>> - throw std::system_error(errno, std::generic_category(),\n> >>> - \"Failed to create eventfd\");\n> >>> -\n> >>> - cm = std::shared_ptr(new CameraManager, [](auto p) {\n> >>> - close(gEventfd);\n> >>> - gEventfd = -1;\n> >>> - delete p;\n> >>> - });\n> >>> -\n> >>> - gEventfd = fd;\n> >>> - gCameraManager = cm;\n> >>> -\n> >>> - int ret = cm->start();\n> >>> - if (ret)\n> >>> - throw std::system_error(-ret, std::generic_category(),\n> >>> - \"Failed to start CameraManager\");\n> >>> -\n> >>> - return cm;\n> >>> - })\n> >>> -\n> >>> - .def_property_readonly(\"version\", &CameraManager::version)\n> >>> -\n> >>> - .def_property_readonly(\"event_fd\", [](CameraManager &) {\n> >>> - return gEventfd;\n> >>> - })\n> >>> + std::shared_ptr cm = gCameraManager.lock();\n> >>> \n> >>> - .def(\"get_ready_requests\", [](CameraManager &) {\n> >>> - uint8_t buf[8];\n> >>> -\n> >>> - if (read(gEventfd, buf, 8) != 8)\n> >>> - throw std::system_error(errno, std::generic_category());\n> >>> -\n> >>> - std::vector v;\n> >>> -\n> >>> - {\n> >>> - std::lock_guard guard(gReqlistMutex);\n> >>> - swap(v, gReqList);\n> >>> + if (!cm) {\n> >>> + cm = std::make_shared();\n> >>> + gCameraManager = cm;\n> >>> }\n> >>> \n> >>> - std::vector ret;\n> >>> -\n> >>> - for (Request *req : v) {\n> >>> - py::object o = py::cast(req);\n> >>> - /* Decrease the ref increased in Camera.queue_request() */\n> >>> - o.dec_ref();\n> >>> - ret.push_back(o);\n> >>> - }\n> >>> -\n> >>> - return ret;\n> >>> + return cm;\n> >>> })\n> >>> \n> >>> - .def(\"get\", py::overload_cast(&CameraManager::get), py::keep_alive<0, 1>())\n> >>> + .def_property_readonly(\"version\", &PyCameraManager::version)\n> >>> + .def(\"get\", py::overload_cast(&PyCameraManager::get), py::keep_alive<0, 1>())\n> >>> + .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n> >>> \n> >>> - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */\n> >>> - .def_property_readonly(\"cameras\", [](CameraManager &self) {\n> >>> - py::list l;\n> >>> -\n> >>> - for (auto &c : self.cameras()) {\n> >>> - py::object py_cm = py::cast(self);\n> >>> - py::object py_cam = py::cast(c);\n> >>> - py::detail::keep_alive_impl(py_cam, py_cm);\n> >>> - l.append(py_cam);\n> >>> - }\n> >>> -\n> >>> - return l;\n> >>> - });\n> >>> + .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> >>> + .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> >>> \n> >>> pyCamera\n> >>> .def_property_readonly(\"id\", &Camera::id)\n> >>> @@ -187,7 +113,13 @@ PYBIND11_MODULE(_libcamera, m)\n> >>> const std::unordered_map &controls) {\n> >>> /* \\todo What happens if someone calls start() multiple times? */\n> >>> \n> >>> - self.requestCompleted.connect(handleRequestCompleted);\n> >>> + auto cm = gCameraManager.lock();\n> >>> + assert(cm);\n> >>> +\n> >>> + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> >>> +\n> > \n> > Extra blank line.\n> \n> Yep.\n> \n> >>> + cm->handleRequestCompleted(req);\n> >>> + });\n> > \n> > So the following didn't work ?\n> > \n> > \t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> > \n> > ? I can investigate.\n> \n> I didn't try that. I don't think what you have above is the same. Note \n> that in my version I store a shared_ptr to make sure it \n> stays alive.\n\nIs there a way we could avoid that ?\n\n> The issue I had (which I asked about in the chat) was in the \"New event \n> handling\", when connecting the cameraAdded signal.\n\nI recall you had trouble receiving the cameraAdded signal even when\ntesting with cam, due to udev support being disabled. Have you retested\nafter fixing that ?","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 9949DBE173\n\tfor ;\n\tMon, 27 Jun 2022 10:25:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 006F065637;\n\tMon, 27 Jun 2022 12:25:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A11896059B\n\tfor ;\n\tMon, 27 Jun 2022 12:25:36 +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 BD3B91C82;\n\tMon, 27 Jun 2022 12:25:35 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656325538;\n\tbh=Bv0zyJAHf9ZMULbhzpDAyzFuW7yJQw9FKTo2A6IJCwk=;\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=IEepnQ3CKkEe36x98yjzq66nxBiVDg32e/XRVzM1D0M+rzRGBb41OgAnBRr8QjpXH\n\tLYvb2QzHJ5PHIjHOqY8EJFcy1wfjirCN+XoJALr374DloKtuECYH0+s4rRNY4xv8x0\n\tDaz3cF5MUW8nSBwtk5g6de/Qg2WlADd2qekZWAcJTj7nrsaWuGrZ9fLDZomjOQapBz\n\ths8BJn/5QGaS1c4lUPm8otvxW/vOVfxWU0EQI2Hm0tBg6Dw+6nWBDwjWVK/UB+XDCj\n\th2klzWM8kTvatBkBwZFYAWtJ02KDQ+cZBg76hNYnZ3yzqYJl4d3I2/VoihlpNimBR7\n\tpV06bahuO54BQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656325536;\n\tbh=Bv0zyJAHf9ZMULbhzpDAyzFuW7yJQw9FKTo2A6IJCwk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nJEuSm/qJ+lSAYryq/6DI6dn1sh8yoH1Ii9S/oEFTuB9TBTMmGQDesYvqSViiFwQP\n\tBX6r8DIzHsdI9DtJmskv2t7VIotcrKe8RVnqVM5dOsSGB/xp4wDZG08I2h1V2klDu9\n\tvP6xz33upKOqtLbsXSe2VoqeSYVuLK+PumQj78Dw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nJEuSm/q\"; dkim-atps=neutral","Date":"Mon, 27 Jun 2022 13:25:17 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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":23601,"web_url":"https://patchwork.libcamera.org/comment/23601/","msgid":"<8515e241-f0ab-eeb2-616b-56b6fb2edf96@ideasonboard.com>","date":"2022-06-27T10:44:22","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 27/06/2022 13:25, Laurent Pinchart wrote:\n>>>>> + getRequests(v);\n>>>\n>>> You can make getRequests() return the vector instead of passing it as a\n>>> reference, and then even do\n>>>\n>>> \tfor (Request *req : getRequests()) {\n>>\n>> Ok. I usually keep away from those as I don't seem to ever exactly know\n>> when copies are made and when not...\n> \n> With copy elision the vector returned by getRequest() will not be copied.\n\nRight. But when is copy elision done? Reading \nhttps://en.cppreference.com/w/cpp/language/copy_elision doesn't make it \nclear either, as it's a lot of rules and changes in each C++ version.\n\nI'm sure the answer is there, my point is just that when I write code, \nit's often not obvious when a copy will be made, or will possibly be \nmade, or will never be made. So I go with the safe way.\n\n>>>>> +class PyCameraManager : public CameraManager\n>>>\n>>> CameraManager isn't meant to be inherited from (I'm surprised it's not\n>>> marked as final). You can keep this for now, but we should soon switch\n>>> to encapsulating the CameraManager instead.\n>>\n>> Hmm that may be problematic. At the moment if there's a CameraManager\n>> pointer, the pybind11 bindings can easily convert that instance to a\n>> Python instance. But if we encapsulate it, that's no longer the case, as\n>> you need something extra to associate the CameraManager pointer to the\n>> encapsulating class.\n>>\n>> I tried to do that with Cameras at some point (trying to sort out the\n>> destructor issue), but I didn't get it working. Maybe I'll try again\n>> with CameraManager this time.\n> \n> Given that both the CameraManager and the PyCameraManager can only be\n> instantiated once, the Python code could unconditionally use the\n> PyCameraManager, without ever using the CameraManager pointer directly.\n> Do we even have any event that passes the CameraManager as an argument ?\n> For Camera instances we do, so that's indeed more problematic.\n\nAfaik it should be possible with Cameras too, but, as I mentioned, I \nnever got it working. I'll try again, CameraManager should be easier as \nyou say.\n\n>>>>> + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n>>>>> +\n>>>\n>>> Extra blank line.\n>>\n>> Yep.\n>>\n>>>>> + cm->handleRequestCompleted(req);\n>>>>> + });\n>>>\n>>> So the following didn't work ?\n>>>\n>>> \t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n>>>\n>>> ? I can investigate.\n>>\n>> I didn't try that. I don't think what you have above is the same. Note\n>> that in my version I store a shared_ptr to make sure it\n>> stays alive.\n> \n> Is there a way we could avoid that ?\n\nProbably. The life-time questions are difficult, especially with Python \nbindings. But I think we can never get requestCompleted signal if the \ncamera is not alive, and if the camera is alive, the camera manager must \nbe alive, so we should be able to just use the CameraManager pointer.\n\n>> The issue I had (which I asked about in the chat) was in the \"New event\n>> handling\", when connecting the cameraAdded signal.\n> \n> I recall you had trouble receiving the cameraAdded signal even when\n> testing with cam, due to udev support being disabled. Have you retested\n> after fixing that ?\n\nYes. And as I said, the signal works fine when I use a plain function. \nBut, for whatever reason, I was not able to get connecting with a lambda \nor an object method working.\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 CBB23BD808\n\tfor ;\n\tMon, 27 Jun 2022 10:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E8B965635;\n\tMon, 27 Jun 2022 12:44:28 +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 1EC1A6059B\n\tfor ;\n\tMon, 27 Jun 2022 12:44:26 +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 616531C82;\n\tMon, 27 Jun 2022 12:44:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656326668;\n\tbh=TImRJ2lW+jz5BlLKbyPyl4KfCh9zLu6CkNLrkU4aW5s=;\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=SBaZVwAFgJOZWZ/von5X6FdfahEqfVwGrMDNwUzWQ+ezPNmejaFq64m0uLaYgV4ga\n\t6fv2mMFTmaw6N8XUUuj3tCs9zrxN1TnTKy7kTcr6Fy9VikEVOUhSzMphekKrJVVKBv\n\taePF7x8if72XtE6pGyhcItqTpl3yahONh52Nww8FTpr03vLqSEJKdKVS7+2PiuGIN3\n\tzvaTxu0Nkr2EsYHn2e3ZUCCdhz5Y5Pv/K4ZAKFh+qCUMp+6vtfslbw8l1TFQ4r0elP\n\tvWijgOUu5ig8aN5IML4R+DOcCU3vhh2wzPJ/z5IW+70chio1rb5/rIrxMZEGKckjdc\n\tARBkAvndOfGZQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656326665;\n\tbh=TImRJ2lW+jz5BlLKbyPyl4KfCh9zLu6CkNLrkU4aW5s=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VYtPgQSfzk4Ck046MacGiYTxbMYzxcuSYfQa92GlvoCvuxtUrn67cyzwlCSe0+QrI\n\tyyvxH5ac3fjRjc2Fhk9d1+XI3aZotEIRsGNIkJ+X8CO/WVaA+R8miLVWKD0HD1t00P\n\tL9XWXGsqc9Tp/f375AsRHpAeb7rw/olDbOU8ayek="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VYtPgQSf\"; dkim-atps=neutral","Message-ID":"<8515e241-f0ab-eeb2-616b-56b6fb2edf96@ideasonboard.com>","Date":"Mon, 27 Jun 2022 13:44:22 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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":23607,"web_url":"https://patchwork.libcamera.org/comment/23607/","msgid":"","date":"2022-06-27T12:20:26","subject":"Re: [libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 27, 2022 at 01:44:22PM +0300, Tomi Valkeinen wrote:\n> On 27/06/2022 13:25, Laurent Pinchart wrote:\n> >>>>> + getRequests(v);\n> >>>\n> >>> You can make getRequests() return the vector instead of passing it as a\n> >>> reference, and then even do\n> >>>\n> >>> \tfor (Request *req : getRequests()) {\n> >>\n> >> Ok. I usually keep away from those as I don't seem to ever exactly know\n> >> when copies are made and when not...\n> > \n> > With copy elision the vector returned by getRequest() will not be copied.\n> \n> Right. But when is copy elision done? Reading \n> https://en.cppreference.com/w/cpp/language/copy_elision doesn't make it \n> clear either, as it's a lot of rules and changes in each C++ version.\n\nI agree it's a bit hard to follow :-) There's copy elision, and return\nvalue optimization (RVO), and their behaviour change based on the C++\nversion. In practice, C++17 introduced mandatory RVO, you should have a\nguarantee that the above code will use copy elision and avoid\nunnecessary copies.\n\n> I'm sure the answer is there, my point is just that when I write code, \n> it's often not obvious when a copy will be made, or will possibly be \n> made, or will never be made. So I go with the safe way.\n> \n> >>>>> +class PyCameraManager : public CameraManager\n> >>>\n> >>> CameraManager isn't meant to be inherited from (I'm surprised it's not\n> >>> marked as final). You can keep this for now, but we should soon switch\n> >>> to encapsulating the CameraManager instead.\n> >>\n> >> Hmm that may be problematic. At the moment if there's a CameraManager\n> >> pointer, the pybind11 bindings can easily convert that instance to a\n> >> Python instance. But if we encapsulate it, that's no longer the case, as\n> >> you need something extra to associate the CameraManager pointer to the\n> >> encapsulating class.\n> >>\n> >> I tried to do that with Cameras at some point (trying to sort out the\n> >> destructor issue), but I didn't get it working. Maybe I'll try again\n> >> with CameraManager this time.\n> > \n> > Given that both the CameraManager and the PyCameraManager can only be\n> > instantiated once, the Python code could unconditionally use the\n> > PyCameraManager, without ever using the CameraManager pointer directly.\n> > Do we even have any event that passes the CameraManager as an argument ?\n> > For Camera instances we do, so that's indeed more problematic.\n> \n> Afaik it should be possible with Cameras too, but, as I mentioned, I \n> never got it working. I'll try again, CameraManager should be easier as \n> you say.\n> \n> >>>>> + self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> >>>>> +\n> >>>\n> >>> Extra blank line.\n> >>\n> >> Yep.\n> >>\n> >>>>> + cm->handleRequestCompleted(req);\n> >>>>> + });\n> >>>\n> >>> So the following didn't work ?\n> >>>\n> >>> \t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> >>>\n> >>> ? I can investigate.\n> >>\n> >> I didn't try that. I don't think what you have above is the same. Note\n> >> that in my version I store a shared_ptr to make sure it\n> >> stays alive.\n> > \n> > Is there a way we could avoid that ?\n> \n> Probably. The life-time questions are difficult, especially with Python \n> bindings. But I think we can never get requestCompleted signal if the \n> camera is not alive, and if the camera is alive, the camera manager must \n> be alive, so we should be able to just use the CameraManager pointer.\n> \n> >> The issue I had (which I asked about in the chat) was in the \"New event\n> >> handling\", when connecting the cameraAdded signal.\n> > \n> > I recall you had trouble receiving the cameraAdded signal even when\n> > testing with cam, due to udev support being disabled. Have you retested\n> > after fixing that ?\n> \n> Yes. And as I said, the signal works fine when I use a plain function. \n> But, for whatever reason, I was not able to get connecting with a lambda \n> or an object method working.","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 3CC6CBD808\n\tfor ;\n\tMon, 27 Jun 2022 12:20:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B339E65635;\n\tMon, 27 Jun 2022 14:20:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE4BA6059B\n\tfor ;\n\tMon, 27 Jun 2022 14:20:45 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D150E1C82;\n\tMon, 27 Jun 2022 14:20:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656332446;\n\tbh=CpP6s4Vah+7UzNSEV18Ckg2X5a3IhvKG7ssCXzOuOSA=;\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=oPGCXcqOWG1xS2obnbnShPdnFW9/kxaNmO4KdLbEuO6znDs/NMtg9E5Cmwq3VnvqC\n\tqX5d8+JTLHOOh3sYksD9I0aDhJlb7VyX6Ll/EJBKKJtUqVJToOc7fwTRKAI5Kbakla\n\tcXT/2b0wen9EWSD9T6xY9+QsaXuVNtfmxDfA6DwxTOP8tEExCnriG/v1H+kyIhrgZa\n\tnVb3CWKzcqtgKaRpSjwZC7DaZTh0+1wT90PsUG0eyw1Pi/Wu+DL4UlrhyXC2PtN7oQ\n\tKuBCVWb/MYfjDAvC6FbTvBRLemBGFOSv2LZggPvZtZ84FZJ/tVPVbAiBWr5DHRUArp\n\tV4BrReJfzdMpg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656332445;\n\tbh=CpP6s4Vah+7UzNSEV18Ckg2X5a3IhvKG7ssCXzOuOSA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GZ6mO8OytBJ/orumFrUZQCTk8ip2s0LYupAfORYuC4iKOY45fhlF3kgNZ8wSnl4Ag\n\tpcRaKFnh0DQM3DObM/kwr0t0sLZk4WdrBiZi8kXzngqqEhpbDGv4uyeolBsQkdT48P\n\tBINt0+b60bwWNrjiyxa3MV6tc4JWpbUjXT6NB4hU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GZ6mO8Oy\"; dkim-atps=neutral","Date":"Mon, 27 Jun 2022 15:20:26 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-5-tomi.valkeinen@ideasonboard.com>\n\t<165606511140.1149771.16715214725620660926@Monstersaurus>\n\t\n\t\n\t\n\t<8515e241-f0ab-eeb2-616b-56b6fb2edf96@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8515e241-f0ab-eeb2-616b-56b6fb2edf96@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC v1 4/7] 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\" "}}]