| Message ID | 20220701084521.31831-6-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote: > Wrap the CameraManager with a PyCameraManager class and move the related > code inside the new class. > > This helps understanding the life times of the used-to-be global > variables, gets rid of static handleRequestCompleted function, and > allows us to simplify the binding code as the more complex pieces are > inside the class. > > There should be no user visible functional changes. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/libcamera/meson.build | 1 + > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ > src/py/libcamera/py_camera_manager.h | 44 +++++++++ > src/py/libcamera/py_main.cpp | 117 +++++------------------ > 4 files changed, 193 insertions(+), 94 deletions(-) > create mode 100644 src/py/libcamera/py_camera_manager.cpp > create mode 100644 src/py/libcamera/py_camera_manager.h > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > index 04578bac..ad2a6858 100644 > --- a/src/py/libcamera/meson.build > +++ b/src/py/libcamera/meson.build > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') > pybind11_dep = pybind11_proj.get_variable('pybind11_dep') > > pycamera_sources = files([ > + 'py_camera_manager.cpp', > 'py_enums.cpp', > 'py_geometry.cpp', > 'py_helpers.cpp', > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > new file mode 100644 > index 00000000..ad47271d > --- /dev/null > +++ b/src/py/libcamera/py_camera_manager.cpp > @@ -0,0 +1,125 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + */ > + > +#include "py_camera_manager.h" > + > +#include <errno.h> > +#include <sys/eventfd.h> > +#include <system_error> > +#include <unistd.h> > + > +#include "py_main.h" > + > +namespace py = pybind11; > + > +using namespace libcamera; > + > +PyCameraManager::PyCameraManager() > +{ > + LOG(Python, Debug) << "PyCameraManager()"; Don't you need to LOG_DECLARE_CATEGORY() ? > + > + cameraManager_ = std::make_unique<CameraManager>(); > + > + int fd = eventfd(0, 0); > + if (fd == -1) > + throw std::system_error(errno, std::generic_category(), > + "Failed to create eventfd"); > + > + eventFd_ = fd; You can use UniqueFD() to avoid manually handling the eventfd file descriptor > + > + int ret = cameraManager_->start(); > + if (ret) { > + close(fd); > + eventFd_ = -1; > + throw std::system_error(-ret, std::generic_category(), > + "Failed to start CameraManager"); > + } > +} > + > +PyCameraManager::~PyCameraManager() > +{ > + LOG(Python, Debug) << "~PyCameraManager()"; > + > + if (eventFd_ != -1) { > + close(eventFd_); > + eventFd_ = -1; > + } > +} > + > +py::list PyCameraManager::cameras() > +{ > + /* > + * Create a list of Cameras, where each camera has a keep-alive to > + * CameraManager. > + */ > + py::list l; > + > + for (auto &camera : cameraManager_->cameras()) { > + py::object py_cm = py::cast(this); > + py::object py_cam = py::cast(camera); > + py::detail::keep_alive_impl(py_cam, py_cm); > + l.append(py_cam); > + } > + > + return l; I was about to suggest creating the list at creation time and cache it. But indeed cameras can be added or removed from the system > +} > + > +std::vector<py::object> PyCameraManager::getReadyRequests() > +{ > + readFd(); > + > + std::vector<py::object> ret; > + > + for (Request *request : getCompletedRequests()) { > + py::object o = py::cast(request); > + /* Decrease the ref increased in Camera.queue_request() */ > + o.dec_ref(); > + ret.push_back(o); > + } > + > + return ret; > +} > + > +/* Note: Called from another thread */ > +void PyCameraManager::handleRequestCompleted(Request *req) > +{ > + pushRequest(req); Is this function only used here ? Should it be inlined ? > + writeFd(); > +} > + > +void PyCameraManager::writeFd() > +{ > + uint64_t v = 1; > + > + size_t s = write(eventFd_, &v, 8); Any reason to write an 8 byte uint64 ? Just curious > + /* > + * We should never fail, and have no simple means to manage the error, > + * so let's log a fatal error. > + */ > + if (s != 8) > + LOG(Python, Fatal) << "Unable to write to eventfd"; > +} > + > +void PyCameraManager::readFd() > +{ > + uint8_t buf[8]; > + > + if (read(eventFd_, buf, 8) != 8) > + throw std::system_error(errno, std::generic_category()); > +} > + > +void PyCameraManager::pushRequest(Request *req) > +{ > + std::lock_guard guard(completedRequestsMutex_); > + completedRequests_.push_back(req); > +} > + > +std::vector<Request *> PyCameraManager::getCompletedRequests() > +{ > + std::vector<Request *> v; > + std::lock_guard guard(completedRequestsMutex_); > + swap(v, completedRequests_); > + return v; > +} > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > new file mode 100644 > index 00000000..9c15f814 > --- /dev/null > +++ b/src/py/libcamera/py_camera_manager.h > @@ -0,0 +1,44 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + */ > + > +#pragma once > + > +#include <mutex> > + > +#include <libcamera/libcamera.h> > + > +#include <pybind11/smart_holder.h> > + > +using namespace libcamera; > + > +class PyCameraManager > +{ > +public: > + PyCameraManager(); > + ~PyCameraManager(); > + > + pybind11::list cameras(); > + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } Do you need to include <memory> ? > + > + static const std::string &version() { return CameraManager::version(); } Do you need to include <string> Should this method be marked as const ? > + > + int eventFd() const { return eventFd_; } > + > + std::vector<pybind11::object> getReadyRequests(); Do you need to include vector ? > + > + void handleRequestCompleted(Request *req); > + > +private: > + std::unique_ptr<CameraManager> cameraManager_; > + > + int eventFd_ = -1; > + std::mutex completedRequestsMutex_; > + std::vector<Request *> completedRequests_; > + > + void writeFd(); > + void readFd(); > + void pushRequest(Request *req); > + std::vector<Request *> getCompletedRequests(); > +}; > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > index e652837f..e7d078b5 100644 > --- a/src/py/libcamera/py_main.cpp > +++ b/src/py/libcamera/py_main.cpp > @@ -7,10 +7,7 @@ > > #include "py_main.h" > > -#include <mutex> > #include <stdexcept> > -#include <sys/eventfd.h> > -#include <unistd.h> > > #include <libcamera/base/log.h> > > @@ -21,6 +18,7 @@ > #include <pybind11/stl.h> > #include <pybind11/stl_bind.h> > > +#include "py_camera_manager.h" > #include "py_helpers.h" > > namespace py = pybind11; > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) > > } > > -static std::weak_ptr<CameraManager> gCameraManager; > -static int gEventfd; > -static std::mutex gReqlistMutex; > -static std::vector<Request *> gReqList; > - > -static void handleRequestCompleted(Request *req) > -{ > - { > - std::lock_guard guard(gReqlistMutex); > - gReqList.push_back(req); > - } > - > - uint64_t v = 1; > - size_t s = write(gEventfd, &v, 8); > - /* > - * We should never fail, and have no simple means to manage the error, > - * so let's log a fatal error. > - */ > - if (s != 8) > - LOG(Python, Fatal) << "Unable to write to eventfd"; > -} > +/* > + * Note: global C++ destructors can be ran on this before the py module is > + * destructed. > + */ > +static std::weak_ptr<PyCameraManager> gCameraManager; > > void init_py_enums(py::module &m); > void init_py_controls_generated(py::module &m); > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) > * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings > */ > > - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); > + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); > auto pyCamera = py::class_<Camera>(m, "Camera"); > auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); > auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) > /* Classes */ > pyCameraManager > .def_static("singleton", []() { > - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); > - if (cm) > - return cm; > - > - int fd = eventfd(0, 0); > - if (fd == -1) > - throw std::system_error(errno, std::generic_category(), > - "Failed to create eventfd"); > - > - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { > - close(gEventfd); > - gEventfd = -1; > - delete p; > - }); > - > - gEventfd = fd; > - gCameraManager = cm; > - > - int ret = cm->start(); > - if (ret) > - throw std::system_error(-ret, std::generic_category(), > - "Failed to start CameraManager"); > - > - return cm; > - }) > - > - .def_property_readonly("version", &CameraManager::version) > + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); > > - .def_property_readonly("event_fd", [](CameraManager &) { > - return gEventfd; > - }) > - > - .def("get_ready_requests", [](CameraManager &) { > - uint8_t buf[8]; > - > - if (read(gEventfd, buf, 8) != 8) > - throw std::system_error(errno, std::generic_category()); > - > - std::vector<Request *> v; > - > - { > - std::lock_guard guard(gReqlistMutex); > - swap(v, gReqList); > + if (!cm) { > + cm = std::make_shared<PyCameraManager>(); > + gCameraManager = cm; What is the advantage of using a weak_ptr<> here ? Can't this be kept as a shared_ptr ? > } > > - std::vector<py::object> ret; > - > - for (Request *req : v) { > - py::object o = py::cast(req); > - /* Decrease the ref increased in Camera.queue_request() */ > - o.dec_ref(); > - ret.push_back(o); > - } > - > - return ret; > + return cm; > }) > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > - > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > - .def_property_readonly("cameras", [](CameraManager &self) { > - py::list l; > - > - for (auto &c : self.cameras()) { > - py::object py_cm = py::cast(self); > - py::object py_cam = py::cast(c); > - py::detail::keep_alive_impl(py_cam, py_cm); > - l.append(py_cam); > - } > + .def_property_readonly("version", &PyCameraManager::version) > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > - return l; > - }); > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) Is access to eventfs useful for applications ? > + .def("get_ready_requests", &PyCameraManager::getReadyRequests); > > pyCamera > .def_property_readonly("id", &Camera::id) > @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m) > const std::unordered_map<const ControlId *, py::object> &controls) { > /* \todo What happens if someone calls start() multiple times? */ > > - self.requestCompleted.connect(handleRequestCompleted); > + auto cm = gCameraManager.lock(); > + ASSERT(cm); > + > + self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted); > > ControlList controlList(self.controls()); > > @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m) > > int ret = self.start(&controlList); > if (ret) { > - self.requestCompleted.disconnect(handleRequestCompleted); > + self.requestCompleted.disconnect(); > return ret; > } > > @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m) > if (ret) > return ret; > > - self.requestCompleted.disconnect(handleRequestCompleted); > + self.requestCompleted.disconnect(); > > return 0; > }) > -- > 2.34.1 >
Hi, On 18/08/2022 17:22, Jacopo Mondi wrote: > Hi Tomi > > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote: >> Wrap the CameraManager with a PyCameraManager class and move the related >> code inside the new class. >> >> This helps understanding the life times of the used-to-be global >> variables, gets rid of static handleRequestCompleted function, and >> allows us to simplify the binding code as the more complex pieces are >> inside the class. >> >> There should be no user visible functional changes. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> src/py/libcamera/meson.build | 1 + >> src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ >> src/py/libcamera/py_camera_manager.h | 44 +++++++++ >> src/py/libcamera/py_main.cpp | 117 +++++------------------ >> 4 files changed, 193 insertions(+), 94 deletions(-) >> create mode 100644 src/py/libcamera/py_camera_manager.cpp >> create mode 100644 src/py/libcamera/py_camera_manager.h >> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build >> index 04578bac..ad2a6858 100644 >> --- a/src/py/libcamera/meson.build >> +++ b/src/py/libcamera/meson.build >> @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') >> pybind11_dep = pybind11_proj.get_variable('pybind11_dep') >> >> pycamera_sources = files([ >> + 'py_camera_manager.cpp', >> 'py_enums.cpp', >> 'py_geometry.cpp', >> 'py_helpers.cpp', >> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp >> new file mode 100644 >> index 00000000..ad47271d >> --- /dev/null >> +++ b/src/py/libcamera/py_camera_manager.cpp >> @@ -0,0 +1,125 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + */ >> + >> +#include "py_camera_manager.h" >> + >> +#include <errno.h> >> +#include <sys/eventfd.h> >> +#include <system_error> >> +#include <unistd.h> >> + >> +#include "py_main.h" >> + >> +namespace py = pybind11; >> + >> +using namespace libcamera; >> + >> +PyCameraManager::PyCameraManager() >> +{ >> + LOG(Python, Debug) << "PyCameraManager()"; > > Don't you need to LOG_DECLARE_CATEGORY() ? It's in py_main.h. > >> + >> + cameraManager_ = std::make_unique<CameraManager>(); >> + >> + int fd = eventfd(0, 0); >> + if (fd == -1) >> + throw std::system_error(errno, std::generic_category(), >> + "Failed to create eventfd"); >> + >> + eventFd_ = fd; > > You can use UniqueFD() to avoid manually handling the eventfd file > descriptor Yes, in the next patch. >> + >> + int ret = cameraManager_->start(); >> + if (ret) { >> + close(fd); >> + eventFd_ = -1; >> + throw std::system_error(-ret, std::generic_category(), >> + "Failed to start CameraManager"); >> + } >> +} >> + >> +PyCameraManager::~PyCameraManager() >> +{ >> + LOG(Python, Debug) << "~PyCameraManager()"; >> + >> + if (eventFd_ != -1) { >> + close(eventFd_); >> + eventFd_ = -1; >> + } >> +} >> + >> +py::list PyCameraManager::cameras() >> +{ >> + /* >> + * Create a list of Cameras, where each camera has a keep-alive to >> + * CameraManager. >> + */ >> + py::list l; >> + >> + for (auto &camera : cameraManager_->cameras()) { >> + py::object py_cm = py::cast(this); >> + py::object py_cam = py::cast(camera); >> + py::detail::keep_alive_impl(py_cam, py_cm); >> + l.append(py_cam); >> + } >> + >> + return l; > > I was about to suggest creating the list at creation time and cache > it. But indeed cameras can be added or removed from the system > >> +} >> + >> +std::vector<py::object> PyCameraManager::getReadyRequests() >> +{ >> + readFd(); >> + >> + std::vector<py::object> ret; >> + >> + for (Request *request : getCompletedRequests()) { >> + py::object o = py::cast(request); >> + /* Decrease the ref increased in Camera.queue_request() */ >> + o.dec_ref(); >> + ret.push_back(o); >> + } >> + >> + return ret; >> +} >> + >> +/* Note: Called from another thread */ >> +void PyCameraManager::handleRequestCompleted(Request *req) >> +{ >> + pushRequest(req); > > Is this function only used here ? > Should it be inlined ? pushRequest? I like it separately, as it uses the completedRequestsMutex_. It makes the code nice and clean as pushRequest and getCompletedRequests are small funcs and use completedRequestsMutex_. But these will be reworked in the new events support patches, so even if inlining would be nicer, I'd rather not do changes here that do not fix anything. >> + writeFd(); >> +} >> + >> +void PyCameraManager::writeFd() >> +{ >> + uint64_t v = 1; >> + >> + size_t s = write(eventFd_, &v, 8); > > Any reason to write an 8 byte uint64 ? > Just curious That's how eventfd works, it passes 64bit numbers. >> + /* >> + * We should never fail, and have no simple means to manage the error, >> + * so let's log a fatal error. >> + */ >> + if (s != 8) >> + LOG(Python, Fatal) << "Unable to write to eventfd"; >> +} >> + >> +void PyCameraManager::readFd() >> +{ >> + uint8_t buf[8]; >> + >> + if (read(eventFd_, buf, 8) != 8) >> + throw std::system_error(errno, std::generic_category()); >> +} >> + >> +void PyCameraManager::pushRequest(Request *req) >> +{ >> + std::lock_guard guard(completedRequestsMutex_); >> + completedRequests_.push_back(req); >> +} >> + >> +std::vector<Request *> PyCameraManager::getCompletedRequests() >> +{ >> + std::vector<Request *> v; >> + std::lock_guard guard(completedRequestsMutex_); >> + swap(v, completedRequests_); >> + return v; >> +} >> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h >> new file mode 100644 >> index 00000000..9c15f814 >> --- /dev/null >> +++ b/src/py/libcamera/py_camera_manager.h >> @@ -0,0 +1,44 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + */ >> + >> +#pragma once >> + >> +#include <mutex> >> + >> +#include <libcamera/libcamera.h> >> + >> +#include <pybind11/smart_holder.h> >> + >> +using namespace libcamera; >> + >> +class PyCameraManager >> +{ >> +public: >> + PyCameraManager(); >> + ~PyCameraManager(); >> + >> + pybind11::list cameras(); >> + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } > > Do you need to include <memory> ? I guess it's safer to include (the same goes for the other include suggestions below). >> + >> + static const std::string &version() { return CameraManager::version(); } > > Do you need to include <string> > > Should this method be marked as const ? It's a static method. >> + >> + int eventFd() const { return eventFd_; } >> + >> + std::vector<pybind11::object> getReadyRequests(); > > Do you need to include vector ? >> + >> + void handleRequestCompleted(Request *req); >> + >> +private: >> + std::unique_ptr<CameraManager> cameraManager_; >> + >> + int eventFd_ = -1; >> + std::mutex completedRequestsMutex_; >> + std::vector<Request *> completedRequests_; >> + >> + void writeFd(); >> + void readFd(); >> + void pushRequest(Request *req); >> + std::vector<Request *> getCompletedRequests(); >> +}; >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp >> index e652837f..e7d078b5 100644 >> --- a/src/py/libcamera/py_main.cpp >> +++ b/src/py/libcamera/py_main.cpp >> @@ -7,10 +7,7 @@ >> >> #include "py_main.h" >> >> -#include <mutex> >> #include <stdexcept> >> -#include <sys/eventfd.h> >> -#include <unistd.h> >> >> #include <libcamera/base/log.h> >> >> @@ -21,6 +18,7 @@ >> #include <pybind11/stl.h> >> #include <pybind11/stl_bind.h> >> >> +#include "py_camera_manager.h" >> #include "py_helpers.h" >> >> namespace py = pybind11; >> @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) >> >> } >> >> -static std::weak_ptr<CameraManager> gCameraManager; >> -static int gEventfd; >> -static std::mutex gReqlistMutex; >> -static std::vector<Request *> gReqList; >> - >> -static void handleRequestCompleted(Request *req) >> -{ >> - { >> - std::lock_guard guard(gReqlistMutex); >> - gReqList.push_back(req); >> - } >> - >> - uint64_t v = 1; >> - size_t s = write(gEventfd, &v, 8); >> - /* >> - * We should never fail, and have no simple means to manage the error, >> - * so let's log a fatal error. >> - */ >> - if (s != 8) >> - LOG(Python, Fatal) << "Unable to write to eventfd"; >> -} >> +/* >> + * Note: global C++ destructors can be ran on this before the py module is >> + * destructed. >> + */ >> +static std::weak_ptr<PyCameraManager> gCameraManager; >> >> void init_py_enums(py::module &m); >> void init_py_controls_generated(py::module &m); >> @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) >> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings >> */ >> >> - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); >> + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); >> auto pyCamera = py::class_<Camera>(m, "Camera"); >> auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); >> auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); >> @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) >> /* Classes */ >> pyCameraManager >> .def_static("singleton", []() { >> - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); >> - if (cm) >> - return cm; >> - >> - int fd = eventfd(0, 0); >> - if (fd == -1) >> - throw std::system_error(errno, std::generic_category(), >> - "Failed to create eventfd"); >> - >> - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { >> - close(gEventfd); >> - gEventfd = -1; >> - delete p; >> - }); >> - >> - gEventfd = fd; >> - gCameraManager = cm; >> - >> - int ret = cm->start(); >> - if (ret) >> - throw std::system_error(-ret, std::generic_category(), >> - "Failed to start CameraManager"); >> - >> - return cm; >> - }) >> - >> - .def_property_readonly("version", &CameraManager::version) >> + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); >> >> - .def_property_readonly("event_fd", [](CameraManager &) { >> - return gEventfd; >> - }) >> - >> - .def("get_ready_requests", [](CameraManager &) { >> - uint8_t buf[8]; >> - >> - if (read(gEventfd, buf, 8) != 8) >> - throw std::system_error(errno, std::generic_category()); >> - >> - std::vector<Request *> v; >> - >> - { >> - std::lock_guard guard(gReqlistMutex); >> - swap(v, gReqList); >> + if (!cm) { >> + cm = std::make_shared<PyCameraManager>(); >> + gCameraManager = cm; > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > as a shared_ptr ? shared_ptr keeps it alive. We don't want to keep it alive. It's the job of the application to keep it alive (as long as it uses it). >> } >> >> - std::vector<py::object> ret; >> - >> - for (Request *req : v) { >> - py::object o = py::cast(req); >> - /* Decrease the ref increased in Camera.queue_request() */ >> - o.dec_ref(); >> - ret.push_back(o); >> - } >> - >> - return ret; >> + return cm; >> }) >> >> - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) >> - >> - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ >> - .def_property_readonly("cameras", [](CameraManager &self) { >> - py::list l; >> - >> - for (auto &c : self.cameras()) { >> - py::object py_cm = py::cast(self); >> - py::object py_cam = py::cast(c); >> - py::detail::keep_alive_impl(py_cam, py_cm); >> - l.append(py_cam); >> - } >> + .def_property_readonly("version", &PyCameraManager::version) >> + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) >> + .def_property_readonly("cameras", &PyCameraManager::cameras) >> >> - return l; >> - }); >> + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > Is access to eventfs useful for applications ? Yes, that's how they wait for events. Tomi
Hi Tomi On Thu, Aug 18, 2022 at 05:39:18PM +0300, Tomi Valkeinen wrote: > Hi, > > On 18/08/2022 17:22, Jacopo Mondi wrote: > > Hi Tomi > > > > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote: > > > Wrap the CameraManager with a PyCameraManager class and move the related > > > code inside the new class. > > > > > > This helps understanding the life times of the used-to-be global > > > variables, gets rid of static handleRequestCompleted function, and > > > allows us to simplify the binding code as the more complex pieces are > > > inside the class. > > > > > > There should be no user visible functional changes. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > src/py/libcamera/meson.build | 1 + > > > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ > > > src/py/libcamera/py_camera_manager.h | 44 +++++++++ > > > src/py/libcamera/py_main.cpp | 117 +++++------------------ > > > 4 files changed, 193 insertions(+), 94 deletions(-) > > > create mode 100644 src/py/libcamera/py_camera_manager.cpp > > > create mode 100644 src/py/libcamera/py_camera_manager.h > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > index 04578bac..ad2a6858 100644 > > > --- a/src/py/libcamera/meson.build > > > +++ b/src/py/libcamera/meson.build > > > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') > > > pybind11_dep = pybind11_proj.get_variable('pybind11_dep') > > > > > > pycamera_sources = files([ > > > + 'py_camera_manager.cpp', > > > 'py_enums.cpp', > > > 'py_geometry.cpp', > > > 'py_helpers.cpp', > > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > > > new file mode 100644 > > > index 00000000..ad47271d > > > --- /dev/null > > > +++ b/src/py/libcamera/py_camera_manager.cpp > > > @@ -0,0 +1,125 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > + */ > > > + > > > +#include "py_camera_manager.h" > > > + > > > +#include <errno.h> > > > +#include <sys/eventfd.h> > > > +#include <system_error> > > > +#include <unistd.h> > > > + > > > +#include "py_main.h" > > > + > > > +namespace py = pybind11; > > > + > > > +using namespace libcamera; > > > + > > > +PyCameraManager::PyCameraManager() > > > +{ > > > + LOG(Python, Debug) << "PyCameraManager()"; > > > > Don't you need to LOG_DECLARE_CATEGORY() ? > > It's in py_main.h. > Ah, I overlooked the first patches as they were reviewed already. Well, that header only serves for that purpose. I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY() and spare that header. > > > > > + > > > + cameraManager_ = std::make_unique<CameraManager>(); > > > + > > > + int fd = eventfd(0, 0); > > > + if (fd == -1) > > > + throw std::system_error(errno, std::generic_category(), > > > + "Failed to create eventfd"); > > > + > > > + eventFd_ = fd; > > > > You can use UniqueFD() to avoid manually handling the eventfd file > > descriptor > > Yes, in the next patch. > > > > + > > > + int ret = cameraManager_->start(); > > > + if (ret) { > > > + close(fd); > > > + eventFd_ = -1; > > > + throw std::system_error(-ret, std::generic_category(), > > > + "Failed to start CameraManager"); > > > + } > > > +} > > > + > > > +PyCameraManager::~PyCameraManager() > > > +{ > > > + LOG(Python, Debug) << "~PyCameraManager()"; > > > + > > > + if (eventFd_ != -1) { > > > + close(eventFd_); > > > + eventFd_ = -1; > > > + } > > > +} > > > + > > > +py::list PyCameraManager::cameras() > > > +{ > > > + /* > > > + * Create a list of Cameras, where each camera has a keep-alive to > > > + * CameraManager. > > > + */ > > > + py::list l; > > > + > > > + for (auto &camera : cameraManager_->cameras()) { > > > + py::object py_cm = py::cast(this); > > > + py::object py_cam = py::cast(camera); > > > + py::detail::keep_alive_impl(py_cam, py_cm); > > > + l.append(py_cam); > > > + } > > > + > > > + return l; > > > > I was about to suggest creating the list at creation time and cache > > it. But indeed cameras can be added or removed from the system > > > > > +} > > > + > > > +std::vector<py::object> PyCameraManager::getReadyRequests() > > > +{ > > > + readFd(); > > > + > > > + std::vector<py::object> ret; > > > + > > > + for (Request *request : getCompletedRequests()) { > > > + py::object o = py::cast(request); > > > + /* Decrease the ref increased in Camera.queue_request() */ > > > + o.dec_ref(); > > > + ret.push_back(o); > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* Note: Called from another thread */ > > > +void PyCameraManager::handleRequestCompleted(Request *req) > > > +{ > > > + pushRequest(req); > > > > Is this function only used here ? > > Should it be inlined ? > > pushRequest? I like it separately, as it uses the completedRequestsMutex_. > It makes the code nice and clean as pushRequest and getCompletedRequests are > small funcs and use completedRequestsMutex_. > > But these will be reworked in the new events support patches, so even if > inlining would be nicer, I'd rather not do changes here that do not fix > anything. > ok.. > > > + writeFd(); > > > +} > > > + > > > +void PyCameraManager::writeFd() > > > +{ > > > + uint64_t v = 1; > > > + > > > + size_t s = write(eventFd_, &v, 8); > > > > Any reason to write an 8 byte uint64 ? > > Just curious > > That's how eventfd works, it passes 64bit numbers. > Ah sorry, missed that :) > > > + /* > > > + * We should never fail, and have no simple means to manage the error, > > > + * so let's log a fatal error. > > > + */ > > > + if (s != 8) > > > + LOG(Python, Fatal) << "Unable to write to eventfd"; > > > +} > > > + > > > +void PyCameraManager::readFd() > > > +{ > > > + uint8_t buf[8]; > > > + > > > + if (read(eventFd_, buf, 8) != 8) > > > + throw std::system_error(errno, std::generic_category()); > > > +} > > > + > > > +void PyCameraManager::pushRequest(Request *req) > > > +{ > > > + std::lock_guard guard(completedRequestsMutex_); > > > + completedRequests_.push_back(req); > > > +} > > > + > > > +std::vector<Request *> PyCameraManager::getCompletedRequests() > > > +{ > > > + std::vector<Request *> v; > > > + std::lock_guard guard(completedRequestsMutex_); > > > + swap(v, completedRequests_); > > > + return v; > > > +} > > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > > > new file mode 100644 > > > index 00000000..9c15f814 > > > --- /dev/null > > > +++ b/src/py/libcamera/py_camera_manager.h > > > @@ -0,0 +1,44 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <mutex> > > > + > > > +#include <libcamera/libcamera.h> > > > + > > > +#include <pybind11/smart_holder.h> > > > + > > > +using namespace libcamera; > > > + > > > +class PyCameraManager > > > +{ > > > +public: > > > + PyCameraManager(); > > > + ~PyCameraManager(); > > > + > > > + pybind11::list cameras(); > > > + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } > > > > Do you need to include <memory> ? > > I guess it's safer to include (the same goes for the other include > suggestions below). > > > > + > > > + static const std::string &version() { return CameraManager::version(); } > > > > Do you need to include <string> > > > > Should this method be marked as const ? > > It's a static method. > Right, missed that > > > + > > > + int eventFd() const { return eventFd_; } > > > + > > > + std::vector<pybind11::object> getReadyRequests(); > > > > Do you need to include vector ? > > > + > > > + void handleRequestCompleted(Request *req); > > > + > > > +private: > > > + std::unique_ptr<CameraManager> cameraManager_; > > > + > > > + int eventFd_ = -1; > > > + std::mutex completedRequestsMutex_; > > > + std::vector<Request *> completedRequests_; > > > + > > > + void writeFd(); > > > + void readFd(); > > > + void pushRequest(Request *req); > > > + std::vector<Request *> getCompletedRequests(); > > > +}; > > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > > > index e652837f..e7d078b5 100644 > > > --- a/src/py/libcamera/py_main.cpp > > > +++ b/src/py/libcamera/py_main.cpp > > > @@ -7,10 +7,7 @@ > > > > > > #include "py_main.h" > > > > > > -#include <mutex> > > > #include <stdexcept> > > > -#include <sys/eventfd.h> > > > -#include <unistd.h> > > > > > > #include <libcamera/base/log.h> > > > > > > @@ -21,6 +18,7 @@ > > > #include <pybind11/stl.h> > > > #include <pybind11/stl_bind.h> > > > > > > +#include "py_camera_manager.h" > > > #include "py_helpers.h" > > > > > > namespace py = pybind11; > > > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) > > > > > > } > > > > > > -static std::weak_ptr<CameraManager> gCameraManager; > > > -static int gEventfd; > > > -static std::mutex gReqlistMutex; > > > -static std::vector<Request *> gReqList; > > > - > > > -static void handleRequestCompleted(Request *req) > > > -{ > > > - { > > > - std::lock_guard guard(gReqlistMutex); > > > - gReqList.push_back(req); > > > - } > > > - > > > - uint64_t v = 1; > > > - size_t s = write(gEventfd, &v, 8); > > > - /* > > > - * We should never fail, and have no simple means to manage the error, > > > - * so let's log a fatal error. > > > - */ > > > - if (s != 8) > > > - LOG(Python, Fatal) << "Unable to write to eventfd"; > > > -} > > > +/* > > > + * Note: global C++ destructors can be ran on this before the py module is > > > + * destructed. > > > + */ > > > +static std::weak_ptr<PyCameraManager> gCameraManager; > > > > > > void init_py_enums(py::module &m); > > > void init_py_controls_generated(py::module &m); > > > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) > > > * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings > > > */ > > > > > > - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); > > > + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); > > > auto pyCamera = py::class_<Camera>(m, "Camera"); > > > auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); > > > auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); > > > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) > > > /* Classes */ > > > pyCameraManager > > > .def_static("singleton", []() { > > > - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); > > > - if (cm) > > > - return cm; > > > - > > > - int fd = eventfd(0, 0); > > > - if (fd == -1) > > > - throw std::system_error(errno, std::generic_category(), > > > - "Failed to create eventfd"); > > > - > > > - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { > > > - close(gEventfd); > > > - gEventfd = -1; > > > - delete p; > > > - }); > > > - > > > - gEventfd = fd; > > > - gCameraManager = cm; > > > - > > > - int ret = cm->start(); > > > - if (ret) > > > - throw std::system_error(-ret, std::generic_category(), > > > - "Failed to start CameraManager"); > > > - > > > - return cm; > > > - }) > > > - > > > - .def_property_readonly("version", &CameraManager::version) > > > + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); > > > > > > - .def_property_readonly("event_fd", [](CameraManager &) { > > > - return gEventfd; > > > - }) > > > - > > > - .def("get_ready_requests", [](CameraManager &) { > > > - uint8_t buf[8]; > > > - > > > - if (read(gEventfd, buf, 8) != 8) > > > - throw std::system_error(errno, std::generic_category()); > > > - > > > - std::vector<Request *> v; > > > - > > > - { > > > - std::lock_guard guard(gReqlistMutex); > > > - swap(v, gReqList); > > > + if (!cm) { > > > + cm = std::make_shared<PyCameraManager>(); > > > + gCameraManager = cm; > > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > > as a shared_ptr ? > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of Oh, I thought we wanted to keep it alive and share ownership with apps. I don't really understand how lifetime is handled in pybind, so I trust your judjment here > the application to keep it alive (as long as it uses it). > > > > } > > > > > > - std::vector<py::object> ret; > > > - > > > - for (Request *req : v) { > > > - py::object o = py::cast(req); > > > - /* Decrease the ref increased in Camera.queue_request() */ > > > - o.dec_ref(); > > > - ret.push_back(o); > > > - } > > > - > > > - return ret; > > > + return cm; > > > }) > > > > > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > > > - > > > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > > > - .def_property_readonly("cameras", [](CameraManager &self) { > > > - py::list l; > > > - > > > - for (auto &c : self.cameras()) { > > > - py::object py_cm = py::cast(self); > > > - py::object py_cam = py::cast(c); > > > - py::detail::keep_alive_impl(py_cam, py_cm); > > > - l.append(py_cam); > > > - } > > > + .def_property_readonly("version", &PyCameraManager::version) > > > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > > > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > > > > > - return l; > > > - }); > > > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > > > Is access to eventfs useful for applications ? > > Yes, that's how they wait for events. > Don't we maks them behind the getReadyRequests() and handleRequestCompleted() ? I see it will be reworked, so.. > Tomi
On 18/08/2022 18:00, Jacopo Mondi wrote: >>>> +PyCameraManager::PyCameraManager() >>>> +{ >>>> + LOG(Python, Debug) << "PyCameraManager()"; >>> >>> Don't you need to LOG_DECLARE_CATEGORY() ? >> >> It's in py_main.h. >> > > Ah, I overlooked the first patches as they were reviewed already. > Well, that header only serves for that purpose. > > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY() > and spare that header. Hmm, sorry can you clarify what you mean? >>> What is the advantage of using a weak_ptr<> here ? Can't this be kept >>> as a shared_ptr ? >> >> shared_ptr keeps it alive. We don't want to keep it alive. It's the job of > > Oh, I thought we wanted to keep it alive and share ownership with > apps. > > I don't really understand how lifetime is handled in pybind, so I > trust your judjment here Well, in short, the _bindings_ do not want to keep anything alive. It's the python side that keeps things alive (by having objects in variables/fields). But sometimes the bindings need to be able to access things in places where the thing is not trivially available (i.e. passed as a parameter from the python side). So we have to store the camera manager, but only as weak_ptr. >> the application to keep it alive (as long as it uses it). >> >>>> } >>>> >>>> - std::vector<py::object> ret; >>>> - >>>> - for (Request *req : v) { >>>> - py::object o = py::cast(req); >>>> - /* Decrease the ref increased in Camera.queue_request() */ >>>> - o.dec_ref(); >>>> - ret.push_back(o); >>>> - } >>>> - >>>> - return ret; >>>> + return cm; >>>> }) >>>> >>>> - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) >>>> - >>>> - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ >>>> - .def_property_readonly("cameras", [](CameraManager &self) { >>>> - py::list l; >>>> - >>>> - for (auto &c : self.cameras()) { >>>> - py::object py_cm = py::cast(self); >>>> - py::object py_cam = py::cast(c); >>>> - py::detail::keep_alive_impl(py_cam, py_cm); >>>> - l.append(py_cam); >>>> - } >>>> + .def_property_readonly("version", &PyCameraManager::version) >>>> + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) >>>> + .def_property_readonly("cameras", &PyCameraManager::cameras) >>>> >>>> - return l; >>>> - }); >>>> + .def_property_readonly("event_fd", &PyCameraManager::eventFd) >>> >>> Is access to eventfs useful for applications ? >> >> Yes, that's how they wait for events. >> > > Don't we maks them behind the getReadyRequests() and > handleRequestCompleted() ? There are two things, 1) using the fd for waiting for events, 2) reading and writing to the fd. The 1) has to be done by the app, the 2) is done with the helper functions here. Tomi
Hi Tomi, On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote: > On 18/08/2022 18:00, Jacopo Mondi wrote: > > > > > > +PyCameraManager::PyCameraManager() > > > > > +{ > > > > > + LOG(Python, Debug) << "PyCameraManager()"; > > > > > > > > Don't you need to LOG_DECLARE_CATEGORY() ? > > > > > > It's in py_main.h. > > > > > > > Ah, I overlooked the first patches as they were reviewed already. > > Well, that header only serves for that purpose. > > > > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY() > > and spare that header. > > Hmm, sorry can you clarify what you mean? > If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY(). I would have DEFINE_CATEGORY() in py_main.cpp and use LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in the other compilation units that use the log symbol (which if I'm not mistaken is py_camera_manager.cpp only). In this way you can get rid of py_main.h completely. > > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > > > > as a shared_ptr ? > > > > > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of > > > > Oh, I thought we wanted to keep it alive and share ownership with > > apps. > > > > I don't really understand how lifetime is handled in pybind, so I > > trust your judjment here > > Well, in short, the _bindings_ do not want to keep anything alive. It's the > python side that keeps things alive (by having objects in variables/fields). > But sometimes the bindings need to be able to access things in places where > the thing is not trivially available (i.e. passed as a parameter from the > python side). So we have to store the camera manager, but only as weak_ptr. > Thanks for the summary :) > > > the application to keep it alive (as long as it uses it). > > > > > > > > } > > > > > > > > > > - std::vector<py::object> ret; > > > > > - > > > > > - for (Request *req : v) { > > > > > - py::object o = py::cast(req); > > > > > - /* Decrease the ref increased in Camera.queue_request() */ > > > > > - o.dec_ref(); > > > > > - ret.push_back(o); > > > > > - } > > > > > - > > > > > - return ret; > > > > > + return cm; > > > > > }) > > > > > > > > > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > > > > > - > > > > > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > > > > > - .def_property_readonly("cameras", [](CameraManager &self) { > > > > > - py::list l; > > > > > - > > > > > - for (auto &c : self.cameras()) { > > > > > - py::object py_cm = py::cast(self); > > > > > - py::object py_cam = py::cast(c); > > > > > - py::detail::keep_alive_impl(py_cam, py_cm); > > > > > - l.append(py_cam); > > > > > - } > > > > > + .def_property_readonly("version", &PyCameraManager::version) > > > > > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > > > > > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > > > > > > > > > - return l; > > > > > - }); > > > > > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > > > > > > > Is access to eventfs useful for applications ? > > > > > > Yes, that's how they wait for events. > > > > > > > Don't we maks them behind the getReadyRequests() and > > handleRequestCompleted() ? > > There are two things, 1) using the fd for waiting for events, 2) reading and > writing to the fd. The 1) has to be done by the app, the 2) is done with the > helper functions here. > > Tomi
On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote: > On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote: > > On 18/08/2022 18:00, Jacopo Mondi wrote: > > > > > > > > +PyCameraManager::PyCameraManager() > > > > > > +{ > > > > > > + LOG(Python, Debug) << "PyCameraManager()"; > > > > > > > > > > Don't you need to LOG_DECLARE_CATEGORY() ? > > > > > > > > It's in py_main.h. > > > > > > > > > > Ah, I overlooked the first patches as they were reviewed already. > > > Well, that header only serves for that purpose. > > > > > > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY() > > > and spare that header. > > > > Hmm, sorry can you clarify what you mean? > > If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY(). > > I would have DEFINE_CATEGORY() in py_main.cpp and use > LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in > the other compilation units that use the log symbol (which if I'm not > mistaken is py_camera_manager.cpp only). In this way you can get rid > of py_main.h completely. Note that LOG_DECLARE_CATEGORY is similar to declaring a function prototype or external variable. Having it in a header file makes sense to me, but I would also not be against doing it manually in the .cpp files that need it, probably mostly because we do that already in a few places. > > > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > > > > > as a shared_ptr ? > > > > > > > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of > > > > > > Oh, I thought we wanted to keep it alive and share ownership with > > > apps. > > > > > > I don't really understand how lifetime is handled in pybind, so I > > > trust your judjment here > > > > Well, in short, the _bindings_ do not want to keep anything alive. It's the > > python side that keeps things alive (by having objects in variables/fields). > > But sometimes the bindings need to be able to access things in places where > > the thing is not trivially available (i.e. passed as a parameter from the > > python side). So we have to store the camera manager, but only as weak_ptr. > > Thanks for the summary :) > > > > > the application to keep it alive (as long as it uses it). > > > > > > > > > > } > > > > > > > > > > > > - std::vector<py::object> ret; > > > > > > - > > > > > > - for (Request *req : v) { > > > > > > - py::object o = py::cast(req); > > > > > > - /* Decrease the ref increased in Camera.queue_request() */ > > > > > > - o.dec_ref(); > > > > > > - ret.push_back(o); > > > > > > - } > > > > > > - > > > > > > - return ret; > > > > > > + return cm; > > > > > > }) > > > > > > > > > > > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > > > > > > - > > > > > > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > > > > > > - .def_property_readonly("cameras", [](CameraManager &self) { > > > > > > - py::list l; > > > > > > - > > > > > > - for (auto &c : self.cameras()) { > > > > > > - py::object py_cm = py::cast(self); > > > > > > - py::object py_cam = py::cast(c); > > > > > > - py::detail::keep_alive_impl(py_cam, py_cm); > > > > > > - l.append(py_cam); > > > > > > - } > > > > > > + .def_property_readonly("version", &PyCameraManager::version) > > > > > > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > > > > > > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > > > > > > > > > > > - return l; > > > > > > - }); > > > > > > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > > > > > > > > > Is access to eventfs useful for applications ? > > > > > > > > Yes, that's how they wait for events. > > > > > > > > > > Don't we maks them behind the getReadyRequests() and > > > handleRequestCompleted() ? > > > > There are two things, 1) using the fd for waiting for events, 2) reading and > > writing to the fd. The 1) has to be done by the app, the 2) is done with the > > helper functions here.
Hi Tomi On Thu, Aug 18, 2022 at 04:22:30PM +0200, Jacopo Mondi wrote: > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote: > > Wrap the CameraManager with a PyCameraManager class and move the related > > code inside the new class. > > > > This helps understanding the life times of the used-to-be global > > variables, gets rid of static handleRequestCompleted function, and > > allows us to simplify the binding code as the more complex pieces are > > inside the class. > > > > There should be no user visible functional changes. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> With Jacopo's comments addressed (except for the ones that got resolved in the replies to your patch), Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/py/libcamera/meson.build | 1 + > > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ > > src/py/libcamera/py_camera_manager.h | 44 +++++++++ > > src/py/libcamera/py_main.cpp | 117 +++++------------------ > > 4 files changed, 193 insertions(+), 94 deletions(-) > > create mode 100644 src/py/libcamera/py_camera_manager.cpp > > create mode 100644 src/py/libcamera/py_camera_manager.h > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > index 04578bac..ad2a6858 100644 > > --- a/src/py/libcamera/meson.build > > +++ b/src/py/libcamera/meson.build > > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') > > pybind11_dep = pybind11_proj.get_variable('pybind11_dep') > > > > pycamera_sources = files([ > > + 'py_camera_manager.cpp', > > 'py_enums.cpp', > > 'py_geometry.cpp', > > 'py_helpers.cpp', > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > > new file mode 100644 > > index 00000000..ad47271d > > --- /dev/null > > +++ b/src/py/libcamera/py_camera_manager.cpp > > @@ -0,0 +1,125 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > + */ > > + > > +#include "py_camera_manager.h" > > + > > +#include <errno.h> > > +#include <sys/eventfd.h> > > +#include <system_error> > > +#include <unistd.h> > > + > > +#include "py_main.h" > > + > > +namespace py = pybind11; > > + > > +using namespace libcamera; > > + > > +PyCameraManager::PyCameraManager() > > +{ > > + LOG(Python, Debug) << "PyCameraManager()"; > > Don't you need to LOG_DECLARE_CATEGORY() ? > > > + > > + cameraManager_ = std::make_unique<CameraManager>(); > > + > > + int fd = eventfd(0, 0); > > + if (fd == -1) > > + throw std::system_error(errno, std::generic_category(), > > + "Failed to create eventfd"); > > + > > + eventFd_ = fd; > > You can use UniqueFD() to avoid manually handling the eventfd file > descriptor > > > + > > + int ret = cameraManager_->start(); > > + if (ret) { > > + close(fd); > > + eventFd_ = -1; > > + throw std::system_error(-ret, std::generic_category(), > > + "Failed to start CameraManager"); > > + } > > +} > > + > > +PyCameraManager::~PyCameraManager() > > +{ > > + LOG(Python, Debug) << "~PyCameraManager()"; > > + > > + if (eventFd_ != -1) { > > + close(eventFd_); > > + eventFd_ = -1; > > + } > > +} > > + > > +py::list PyCameraManager::cameras() > > +{ > > + /* > > + * Create a list of Cameras, where each camera has a keep-alive to > > + * CameraManager. > > + */ > > + py::list l; > > + > > + for (auto &camera : cameraManager_->cameras()) { > > + py::object py_cm = py::cast(this); > > + py::object py_cam = py::cast(camera); > > + py::detail::keep_alive_impl(py_cam, py_cm); > > + l.append(py_cam); > > + } > > + > > + return l; > > I was about to suggest creating the list at creation time and cache > it. But indeed cameras can be added or removed from the system > > > +} > > + > > +std::vector<py::object> PyCameraManager::getReadyRequests() > > +{ > > + readFd(); > > + > > + std::vector<py::object> ret; > > + > > + for (Request *request : getCompletedRequests()) { > > + py::object o = py::cast(request); > > + /* Decrease the ref increased in Camera.queue_request() */ > > + o.dec_ref(); > > + ret.push_back(o); > > + } > > + > > + return ret; > > +} > > + > > +/* Note: Called from another thread */ > > +void PyCameraManager::handleRequestCompleted(Request *req) > > +{ > > + pushRequest(req); > > Is this function only used here ? > Should it be inlined ? > > > + writeFd(); > > +} > > + > > +void PyCameraManager::writeFd() > > +{ > > + uint64_t v = 1; > > + > > + size_t s = write(eventFd_, &v, 8); > > Any reason to write an 8 byte uint64 ? > Just curious > > > + /* > > + * We should never fail, and have no simple means to manage the error, > > + * so let's log a fatal error. > > + */ > > + if (s != 8) > > + LOG(Python, Fatal) << "Unable to write to eventfd"; > > +} > > + > > +void PyCameraManager::readFd() > > +{ > > + uint8_t buf[8]; > > + > > + if (read(eventFd_, buf, 8) != 8) > > + throw std::system_error(errno, std::generic_category()); > > +} > > + > > +void PyCameraManager::pushRequest(Request *req) > > +{ > > + std::lock_guard guard(completedRequestsMutex_); > > + completedRequests_.push_back(req); > > +} > > + > > +std::vector<Request *> PyCameraManager::getCompletedRequests() > > +{ > > + std::vector<Request *> v; > > + std::lock_guard guard(completedRequestsMutex_); > > + swap(v, completedRequests_); > > + return v; > > +} > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > > new file mode 100644 > > index 00000000..9c15f814 > > --- /dev/null > > +++ b/src/py/libcamera/py_camera_manager.h > > @@ -0,0 +1,44 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > + */ > > + > > +#pragma once > > + > > +#include <mutex> > > + > > +#include <libcamera/libcamera.h> > > + > > +#include <pybind11/smart_holder.h> > > + > > +using namespace libcamera; > > + > > +class PyCameraManager > > +{ > > +public: > > + PyCameraManager(); > > + ~PyCameraManager(); > > + > > + pybind11::list cameras(); > > + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } > > Do you need to include <memory> ? > > + > > + static const std::string &version() { return CameraManager::version(); } > > Do you need to include <string> > > Should this method be marked as const ? > > > + > > + int eventFd() const { return eventFd_; } > > + > > + std::vector<pybind11::object> getReadyRequests(); > > Do you need to include vector ? > > + > > + void handleRequestCompleted(Request *req); > > + > > +private: > > + std::unique_ptr<CameraManager> cameraManager_; > > + > > + int eventFd_ = -1; > > + std::mutex completedRequestsMutex_; > > + std::vector<Request *> completedRequests_; > > + > > + void writeFd(); > > + void readFd(); > > + void pushRequest(Request *req); > > + std::vector<Request *> getCompletedRequests(); > > +}; > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > > index e652837f..e7d078b5 100644 > > --- a/src/py/libcamera/py_main.cpp > > +++ b/src/py/libcamera/py_main.cpp > > @@ -7,10 +7,7 @@ > > > > #include "py_main.h" > > > > -#include <mutex> > > #include <stdexcept> > > -#include <sys/eventfd.h> > > -#include <unistd.h> > > > > #include <libcamera/base/log.h> > > > > @@ -21,6 +18,7 @@ > > #include <pybind11/stl.h> > > #include <pybind11/stl_bind.h> > > > > +#include "py_camera_manager.h" > > #include "py_helpers.h" > > > > namespace py = pybind11; > > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) > > > > } > > > > -static std::weak_ptr<CameraManager> gCameraManager; > > -static int gEventfd; > > -static std::mutex gReqlistMutex; > > -static std::vector<Request *> gReqList; > > - > > -static void handleRequestCompleted(Request *req) > > -{ > > - { > > - std::lock_guard guard(gReqlistMutex); > > - gReqList.push_back(req); > > - } > > - > > - uint64_t v = 1; > > - size_t s = write(gEventfd, &v, 8); > > - /* > > - * We should never fail, and have no simple means to manage the error, > > - * so let's log a fatal error. > > - */ > > - if (s != 8) > > - LOG(Python, Fatal) << "Unable to write to eventfd"; > > -} > > +/* > > + * Note: global C++ destructors can be ran on this before the py module is > > + * destructed. > > + */ > > +static std::weak_ptr<PyCameraManager> gCameraManager; > > > > void init_py_enums(py::module &m); > > void init_py_controls_generated(py::module &m); > > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) > > * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings > > */ > > > > - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); > > + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); > > auto pyCamera = py::class_<Camera>(m, "Camera"); > > auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); > > auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); > > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) > > /* Classes */ > > pyCameraManager > > .def_static("singleton", []() { > > - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); > > - if (cm) > > - return cm; > > - > > - int fd = eventfd(0, 0); > > - if (fd == -1) > > - throw std::system_error(errno, std::generic_category(), > > - "Failed to create eventfd"); > > - > > - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { > > - close(gEventfd); > > - gEventfd = -1; > > - delete p; > > - }); > > - > > - gEventfd = fd; > > - gCameraManager = cm; > > - > > - int ret = cm->start(); > > - if (ret) > > - throw std::system_error(-ret, std::generic_category(), > > - "Failed to start CameraManager"); > > - > > - return cm; > > - }) > > - > > - .def_property_readonly("version", &CameraManager::version) > > + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); > > > > - .def_property_readonly("event_fd", [](CameraManager &) { > > - return gEventfd; > > - }) > > - > > - .def("get_ready_requests", [](CameraManager &) { > > - uint8_t buf[8]; > > - > > - if (read(gEventfd, buf, 8) != 8) > > - throw std::system_error(errno, std::generic_category()); > > - > > - std::vector<Request *> v; > > - > > - { > > - std::lock_guard guard(gReqlistMutex); > > - swap(v, gReqList); > > + if (!cm) { > > + cm = std::make_shared<PyCameraManager>(); > > + gCameraManager = cm; > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > as a shared_ptr ? > > > } > > > > - std::vector<py::object> ret; > > - > > - for (Request *req : v) { > > - py::object o = py::cast(req); > > - /* Decrease the ref increased in Camera.queue_request() */ > > - o.dec_ref(); > > - ret.push_back(o); > > - } > > - > > - return ret; > > + return cm; > > }) > > > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > > - > > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > > - .def_property_readonly("cameras", [](CameraManager &self) { > > - py::list l; > > - > > - for (auto &c : self.cameras()) { > > - py::object py_cm = py::cast(self); > > - py::object py_cam = py::cast(c); > > - py::detail::keep_alive_impl(py_cam, py_cm); > > - l.append(py_cam); > > - } > > + .def_property_readonly("version", &PyCameraManager::version) > > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > > > - return l; > > - }); > > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > Is access to eventfs useful for applications ? > > > + .def("get_ready_requests", &PyCameraManager::getReadyRequests); > > > > pyCamera > > .def_property_readonly("id", &Camera::id) > > @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m) > > const std::unordered_map<const ControlId *, py::object> &controls) { > > /* \todo What happens if someone calls start() multiple times? */ > > > > - self.requestCompleted.connect(handleRequestCompleted); > > + auto cm = gCameraManager.lock(); > > + ASSERT(cm); > > + > > + self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted); > > > > ControlList controlList(self.controls()); > > > > @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m) > > > > int ret = self.start(&controlList); > > if (ret) { > > - self.requestCompleted.disconnect(handleRequestCompleted); > > + self.requestCompleted.disconnect(); > > return ret; > > } > > > > @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m) > > if (ret) > > return ret; > > > > - self.requestCompleted.disconnect(handleRequestCompleted); > > + self.requestCompleted.disconnect(); > > > > return 0; > > })
Hi Tomi On Thu, Aug 18, 2022 at 10:18:18PM +0300, Laurent Pinchart via libcamera-devel wrote: > On Thu, Aug 18, 2022 at 04:22:30PM +0200, Jacopo Mondi wrote: > > On Fri, Jul 01, 2022 at 11:45:09AM +0300, Tomi Valkeinen wrote: > > > Wrap the CameraManager with a PyCameraManager class and move the related > > > code inside the new class. > > > > > > This helps understanding the life times of the used-to-be global > > > variables, gets rid of static handleRequestCompleted function, and > > > allows us to simplify the binding code as the more complex pieces are > > > inside the class. > > > > > > There should be no user visible functional changes. > > > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > With Jacopo's comments addressed (except for the ones that got resolved > in the replies to your patch), Actually, one more comment, see below. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > src/py/libcamera/meson.build | 1 + > > > src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ > > > src/py/libcamera/py_camera_manager.h | 44 +++++++++ > > > src/py/libcamera/py_main.cpp | 117 +++++------------------ > > > 4 files changed, 193 insertions(+), 94 deletions(-) > > > create mode 100644 src/py/libcamera/py_camera_manager.cpp > > > create mode 100644 src/py/libcamera/py_camera_manager.h > > > > > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > > index 04578bac..ad2a6858 100644 > > > --- a/src/py/libcamera/meson.build > > > +++ b/src/py/libcamera/meson.build > > > @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') > > > pybind11_dep = pybind11_proj.get_variable('pybind11_dep') > > > > > > pycamera_sources = files([ > > > + 'py_camera_manager.cpp', > > > 'py_enums.cpp', > > > 'py_geometry.cpp', > > > 'py_helpers.cpp', > > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp > > > new file mode 100644 > > > index 00000000..ad47271d > > > --- /dev/null > > > +++ b/src/py/libcamera/py_camera_manager.cpp > > > @@ -0,0 +1,125 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > + */ > > > + > > > +#include "py_camera_manager.h" > > > + > > > +#include <errno.h> > > > +#include <sys/eventfd.h> > > > +#include <system_error> > > > +#include <unistd.h> > > > + > > > +#include "py_main.h" > > > + > > > +namespace py = pybind11; > > > + > > > +using namespace libcamera; > > > + > > > +PyCameraManager::PyCameraManager() > > > +{ > > > + LOG(Python, Debug) << "PyCameraManager()"; > > > > Don't you need to LOG_DECLARE_CATEGORY() ? > > > > > + > > > + cameraManager_ = std::make_unique<CameraManager>(); > > > + > > > + int fd = eventfd(0, 0); > > > + if (fd == -1) > > > + throw std::system_error(errno, std::generic_category(), > > > + "Failed to create eventfd"); > > > + > > > + eventFd_ = fd; > > > > You can use UniqueFD() to avoid manually handling the eventfd file > > descriptor > > > > > + > > > + int ret = cameraManager_->start(); > > > + if (ret) { > > > + close(fd); > > > + eventFd_ = -1; > > > + throw std::system_error(-ret, std::generic_category(), > > > + "Failed to start CameraManager"); > > > + } > > > +} > > > + > > > +PyCameraManager::~PyCameraManager() > > > +{ > > > + LOG(Python, Debug) << "~PyCameraManager()"; > > > + > > > + if (eventFd_ != -1) { > > > + close(eventFd_); > > > + eventFd_ = -1; > > > + } > > > +} > > > + > > > +py::list PyCameraManager::cameras() > > > +{ > > > + /* > > > + * Create a list of Cameras, where each camera has a keep-alive to > > > + * CameraManager. > > > + */ > > > + py::list l; > > > + > > > + for (auto &camera : cameraManager_->cameras()) { > > > + py::object py_cm = py::cast(this); > > > + py::object py_cam = py::cast(camera); > > > + py::detail::keep_alive_impl(py_cam, py_cm); > > > + l.append(py_cam); > > > + } > > > + > > > + return l; > > > > I was about to suggest creating the list at creation time and cache > > it. But indeed cameras can be added or removed from the system > > > > > +} > > > + > > > +std::vector<py::object> PyCameraManager::getReadyRequests() > > > +{ > > > + readFd(); > > > + > > > + std::vector<py::object> ret; > > > + > > > + for (Request *request : getCompletedRequests()) { > > > + py::object o = py::cast(request); > > > + /* Decrease the ref increased in Camera.queue_request() */ > > > + o.dec_ref(); > > > + ret.push_back(o); > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* Note: Called from another thread */ > > > +void PyCameraManager::handleRequestCompleted(Request *req) > > > +{ > > > + pushRequest(req); > > > > Is this function only used here ? > > Should it be inlined ? > > > > > + writeFd(); > > > +} > > > + > > > +void PyCameraManager::writeFd() > > > +{ > > > + uint64_t v = 1; > > > + > > > + size_t s = write(eventFd_, &v, 8); > > > > Any reason to write an 8 byte uint64 ? > > Just curious > > > > > + /* > > > + * We should never fail, and have no simple means to manage the error, > > > + * so let's log a fatal error. > > > + */ > > > + if (s != 8) > > > + LOG(Python, Fatal) << "Unable to write to eventfd"; > > > +} > > > + > > > +void PyCameraManager::readFd() > > > +{ > > > + uint8_t buf[8]; > > > + > > > + if (read(eventFd_, buf, 8) != 8) > > > + throw std::system_error(errno, std::generic_category()); > > > +} > > > + > > > +void PyCameraManager::pushRequest(Request *req) > > > +{ > > > + std::lock_guard guard(completedRequestsMutex_); > > > + completedRequests_.push_back(req); > > > +} > > > + > > > +std::vector<Request *> PyCameraManager::getCompletedRequests() > > > +{ > > > + std::vector<Request *> v; > > > + std::lock_guard guard(completedRequestsMutex_); > > > + swap(v, completedRequests_); > > > + return v; > > > +} > > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h > > > new file mode 100644 > > > index 00000000..9c15f814 > > > --- /dev/null > > > +++ b/src/py/libcamera/py_camera_manager.h > > > @@ -0,0 +1,44 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <mutex> > > > + > > > +#include <libcamera/libcamera.h> > > > + > > > +#include <pybind11/smart_holder.h> > > > + > > > +using namespace libcamera; > > > + > > > +class PyCameraManager > > > +{ > > > +public: > > > + PyCameraManager(); > > > + ~PyCameraManager(); > > > + > > > + pybind11::list cameras(); > > > + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } > > > > Do you need to include <memory> ? > > > + > > > + static const std::string &version() { return CameraManager::version(); } > > > > Do you need to include <string> > > > > Should this method be marked as const ? > > > > > + > > > + int eventFd() const { return eventFd_; } > > > + > > > + std::vector<pybind11::object> getReadyRequests(); > > > > Do you need to include vector ? > > > + > > > + void handleRequestCompleted(Request *req); > > > + > > > +private: > > > + std::unique_ptr<CameraManager> cameraManager_; > > > + > > > + int eventFd_ = -1; > > > + std::mutex completedRequestsMutex_; > > > + std::vector<Request *> completedRequests_; > > > + > > > + void writeFd(); > > > + void readFd(); > > > + void pushRequest(Request *req); > > > + std::vector<Request *> getCompletedRequests(); We usually place functions before variables. > > > +}; > > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > > > index e652837f..e7d078b5 100644 > > > --- a/src/py/libcamera/py_main.cpp > > > +++ b/src/py/libcamera/py_main.cpp > > > @@ -7,10 +7,7 @@ > > > > > > #include "py_main.h" > > > > > > -#include <mutex> > > > #include <stdexcept> > > > -#include <sys/eventfd.h> > > > -#include <unistd.h> > > > > > > #include <libcamera/base/log.h> > > > > > > @@ -21,6 +18,7 @@ > > > #include <pybind11/stl.h> > > > #include <pybind11/stl_bind.h> > > > > > > +#include "py_camera_manager.h" > > > #include "py_helpers.h" > > > > > > namespace py = pybind11; > > > @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) > > > > > > } > > > > > > -static std::weak_ptr<CameraManager> gCameraManager; > > > -static int gEventfd; > > > -static std::mutex gReqlistMutex; > > > -static std::vector<Request *> gReqList; > > > - > > > -static void handleRequestCompleted(Request *req) > > > -{ > > > - { > > > - std::lock_guard guard(gReqlistMutex); > > > - gReqList.push_back(req); > > > - } > > > - > > > - uint64_t v = 1; > > > - size_t s = write(gEventfd, &v, 8); > > > - /* > > > - * We should never fail, and have no simple means to manage the error, > > > - * so let's log a fatal error. > > > - */ > > > - if (s != 8) > > > - LOG(Python, Fatal) << "Unable to write to eventfd"; > > > -} > > > +/* > > > + * Note: global C++ destructors can be ran on this before the py module is > > > + * destructed. > > > + */ > > > +static std::weak_ptr<PyCameraManager> gCameraManager; > > > > > > void init_py_enums(py::module &m); > > > void init_py_controls_generated(py::module &m); > > > @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) > > > * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings > > > */ > > > > > > - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); > > > + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); > > > auto pyCamera = py::class_<Camera>(m, "Camera"); > > > auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); > > > auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); > > > @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) > > > /* Classes */ > > > pyCameraManager > > > .def_static("singleton", []() { > > > - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); > > > - if (cm) > > > - return cm; > > > - > > > - int fd = eventfd(0, 0); > > > - if (fd == -1) > > > - throw std::system_error(errno, std::generic_category(), > > > - "Failed to create eventfd"); > > > - > > > - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { > > > - close(gEventfd); > > > - gEventfd = -1; > > > - delete p; > > > - }); > > > - > > > - gEventfd = fd; > > > - gCameraManager = cm; > > > - > > > - int ret = cm->start(); > > > - if (ret) > > > - throw std::system_error(-ret, std::generic_category(), > > > - "Failed to start CameraManager"); > > > - > > > - return cm; > > > - }) > > > - > > > - .def_property_readonly("version", &CameraManager::version) > > > + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); > > > > > > - .def_property_readonly("event_fd", [](CameraManager &) { > > > - return gEventfd; > > > - }) > > > - > > > - .def("get_ready_requests", [](CameraManager &) { > > > - uint8_t buf[8]; > > > - > > > - if (read(gEventfd, buf, 8) != 8) > > > - throw std::system_error(errno, std::generic_category()); > > > - > > > - std::vector<Request *> v; > > > - > > > - { > > > - std::lock_guard guard(gReqlistMutex); > > > - swap(v, gReqList); > > > + if (!cm) { > > > + cm = std::make_shared<PyCameraManager>(); > > > + gCameraManager = cm; > > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept > > as a shared_ptr ? > > > > > } > > > > > > - std::vector<py::object> ret; > > > - > > > - for (Request *req : v) { > > > - py::object o = py::cast(req); > > > - /* Decrease the ref increased in Camera.queue_request() */ > > > - o.dec_ref(); > > > - ret.push_back(o); > > > - } > > > - > > > - return ret; > > > + return cm; > > > }) > > > > > > - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) > > > - > > > - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ > > > - .def_property_readonly("cameras", [](CameraManager &self) { > > > - py::list l; > > > - > > > - for (auto &c : self.cameras()) { > > > - py::object py_cm = py::cast(self); > > > - py::object py_cam = py::cast(c); > > > - py::detail::keep_alive_impl(py_cam, py_cm); > > > - l.append(py_cam); > > > - } > > > + .def_property_readonly("version", &PyCameraManager::version) > > > + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) > > > + .def_property_readonly("cameras", &PyCameraManager::cameras) > > > > > > - return l; > > > - }); > > > + .def_property_readonly("event_fd", &PyCameraManager::eventFd) > > > > Is access to eventfs useful for applications ? > > > > > + .def("get_ready_requests", &PyCameraManager::getReadyRequests); > > > > > > pyCamera > > > .def_property_readonly("id", &Camera::id) > > > @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m) > > > const std::unordered_map<const ControlId *, py::object> &controls) { > > > /* \todo What happens if someone calls start() multiple times? */ > > > > > > - self.requestCompleted.connect(handleRequestCompleted); > > > + auto cm = gCameraManager.lock(); > > > + ASSERT(cm); > > > + > > > + self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted); > > > > > > ControlList controlList(self.controls()); > > > > > > @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m) > > > > > > int ret = self.start(&controlList); > > > if (ret) { > > > - self.requestCompleted.disconnect(handleRequestCompleted); > > > + self.requestCompleted.disconnect(); > > > return ret; > > > } > > > > > > @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m) > > > if (ret) > > > return ret; > > > > > > - self.requestCompleted.disconnect(handleRequestCompleted); > > > + self.requestCompleted.disconnect(); > > > > > > return 0; > > > })
On 18/08/2022 22:06, Laurent Pinchart wrote: > On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote: >> On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote: >>> On 18/08/2022 18:00, Jacopo Mondi wrote: >>> >>>>>>> +PyCameraManager::PyCameraManager() >>>>>>> +{ >>>>>>> + LOG(Python, Debug) << "PyCameraManager()"; >>>>>> >>>>>> Don't you need to LOG_DECLARE_CATEGORY() ? >>>>> >>>>> It's in py_main.h. >>>>> >>>> >>>> Ah, I overlooked the first patches as they were reviewed already. >>>> Well, that header only serves for that purpose. >>>> >>>> I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY() >>>> and spare that header. >>> >>> Hmm, sorry can you clarify what you mean? >> >> If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY(). >> >> I would have DEFINE_CATEGORY() in py_main.cpp and use >> LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in >> the other compilation units that use the log symbol (which if I'm not >> mistaken is py_camera_manager.cpp only). In this way you can get rid >> of py_main.h completely. > > Note that LOG_DECLARE_CATEGORY is similar to declaring a function > prototype or external variable. Having it in a header file makes sense > to me, but I would also not be against doing it manually in the .cpp > files that need it, probably mostly because we do that already in a few > places. I think py_main.h is a logical place for LOG_DECLARE_CATEGORY, as the counterpart is in py_main.cpp. It's true that, at the moment, only single .cpp file uses LOG(), and there's nothing else in the py_main.h, but...feels like a bit needless optimization to try to get rid of the file. Tomi
diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build index 04578bac..ad2a6858 100644 --- a/src/py/libcamera/meson.build +++ b/src/py/libcamera/meson.build @@ -13,6 +13,7 @@ pybind11_proj = subproject('pybind11') pybind11_dep = pybind11_proj.get_variable('pybind11_dep') pycamera_sources = files([ + 'py_camera_manager.cpp', 'py_enums.cpp', 'py_geometry.cpp', 'py_helpers.cpp', diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp new file mode 100644 index 00000000..ad47271d --- /dev/null +++ b/src/py/libcamera/py_camera_manager.cpp @@ -0,0 +1,125 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> + */ + +#include "py_camera_manager.h" + +#include <errno.h> +#include <sys/eventfd.h> +#include <system_error> +#include <unistd.h> + +#include "py_main.h" + +namespace py = pybind11; + +using namespace libcamera; + +PyCameraManager::PyCameraManager() +{ + LOG(Python, Debug) << "PyCameraManager()"; + + cameraManager_ = std::make_unique<CameraManager>(); + + int fd = eventfd(0, 0); + if (fd == -1) + throw std::system_error(errno, std::generic_category(), + "Failed to create eventfd"); + + eventFd_ = fd; + + int ret = cameraManager_->start(); + if (ret) { + close(fd); + eventFd_ = -1; + throw std::system_error(-ret, std::generic_category(), + "Failed to start CameraManager"); + } +} + +PyCameraManager::~PyCameraManager() +{ + LOG(Python, Debug) << "~PyCameraManager()"; + + if (eventFd_ != -1) { + close(eventFd_); + eventFd_ = -1; + } +} + +py::list PyCameraManager::cameras() +{ + /* + * Create a list of Cameras, where each camera has a keep-alive to + * CameraManager. + */ + py::list l; + + for (auto &camera : cameraManager_->cameras()) { + py::object py_cm = py::cast(this); + py::object py_cam = py::cast(camera); + py::detail::keep_alive_impl(py_cam, py_cm); + l.append(py_cam); + } + + return l; +} + +std::vector<py::object> PyCameraManager::getReadyRequests() +{ + readFd(); + + std::vector<py::object> ret; + + for (Request *request : getCompletedRequests()) { + py::object o = py::cast(request); + /* Decrease the ref increased in Camera.queue_request() */ + o.dec_ref(); + ret.push_back(o); + } + + return ret; +} + +/* Note: Called from another thread */ +void PyCameraManager::handleRequestCompleted(Request *req) +{ + pushRequest(req); + writeFd(); +} + +void PyCameraManager::writeFd() +{ + uint64_t v = 1; + + size_t s = write(eventFd_, &v, 8); + /* + * We should never fail, and have no simple means to manage the error, + * so let's log a fatal error. + */ + if (s != 8) + LOG(Python, Fatal) << "Unable to write to eventfd"; +} + +void PyCameraManager::readFd() +{ + uint8_t buf[8]; + + if (read(eventFd_, buf, 8) != 8) + throw std::system_error(errno, std::generic_category()); +} + +void PyCameraManager::pushRequest(Request *req) +{ + std::lock_guard guard(completedRequestsMutex_); + completedRequests_.push_back(req); +} + +std::vector<Request *> PyCameraManager::getCompletedRequests() +{ + std::vector<Request *> v; + std::lock_guard guard(completedRequestsMutex_); + swap(v, completedRequests_); + return v; +} diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h new file mode 100644 index 00000000..9c15f814 --- /dev/null +++ b/src/py/libcamera/py_camera_manager.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> + */ + +#pragma once + +#include <mutex> + +#include <libcamera/libcamera.h> + +#include <pybind11/smart_holder.h> + +using namespace libcamera; + +class PyCameraManager +{ +public: + PyCameraManager(); + ~PyCameraManager(); + + pybind11::list cameras(); + std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); } + + static const std::string &version() { return CameraManager::version(); } + + int eventFd() const { return eventFd_; } + + std::vector<pybind11::object> getReadyRequests(); + + void handleRequestCompleted(Request *req); + +private: + std::unique_ptr<CameraManager> cameraManager_; + + int eventFd_ = -1; + std::mutex completedRequestsMutex_; + std::vector<Request *> completedRequests_; + + void writeFd(); + void readFd(); + void pushRequest(Request *req); + std::vector<Request *> getCompletedRequests(); +}; diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp index e652837f..e7d078b5 100644 --- a/src/py/libcamera/py_main.cpp +++ b/src/py/libcamera/py_main.cpp @@ -7,10 +7,7 @@ #include "py_main.h" -#include <mutex> #include <stdexcept> -#include <sys/eventfd.h> -#include <unistd.h> #include <libcamera/base/log.h> @@ -21,6 +18,7 @@ #include <pybind11/stl.h> #include <pybind11/stl_bind.h> +#include "py_camera_manager.h" #include "py_helpers.h" namespace py = pybind11; @@ -33,27 +31,11 @@ LOG_DEFINE_CATEGORY(Python) } -static std::weak_ptr<CameraManager> gCameraManager; -static int gEventfd; -static std::mutex gReqlistMutex; -static std::vector<Request *> gReqList; - -static void handleRequestCompleted(Request *req) -{ - { - std::lock_guard guard(gReqlistMutex); - gReqList.push_back(req); - } - - uint64_t v = 1; - size_t s = write(gEventfd, &v, 8); - /* - * We should never fail, and have no simple means to manage the error, - * so let's log a fatal error. - */ - if (s != 8) - LOG(Python, Fatal) << "Unable to write to eventfd"; -} +/* + * Note: global C++ destructors can be ran on this before the py module is + * destructed. + */ +static std::weak_ptr<PyCameraManager> gCameraManager; void init_py_enums(py::module &m); void init_py_controls_generated(py::module &m); @@ -76,7 +58,7 @@ PYBIND11_MODULE(_libcamera, m) * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings */ - auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager"); + auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager"); auto pyCamera = py::class_<Camera>(m, "Camera"); auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration"); auto pyCameraConfigurationStatus = py::enum_<CameraConfiguration::Status>(pyCameraConfiguration, "Status"); @@ -110,78 +92,22 @@ PYBIND11_MODULE(_libcamera, m) /* Classes */ pyCameraManager .def_static("singleton", []() { - std::shared_ptr<CameraManager> cm = gCameraManager.lock(); - if (cm) - return cm; - - int fd = eventfd(0, 0); - if (fd == -1) - throw std::system_error(errno, std::generic_category(), - "Failed to create eventfd"); - - cm = std::shared_ptr<CameraManager>(new CameraManager, [](auto p) { - close(gEventfd); - gEventfd = -1; - delete p; - }); - - gEventfd = fd; - gCameraManager = cm; - - int ret = cm->start(); - if (ret) - throw std::system_error(-ret, std::generic_category(), - "Failed to start CameraManager"); - - return cm; - }) - - .def_property_readonly("version", &CameraManager::version) + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock(); - .def_property_readonly("event_fd", [](CameraManager &) { - return gEventfd; - }) - - .def("get_ready_requests", [](CameraManager &) { - uint8_t buf[8]; - - if (read(gEventfd, buf, 8) != 8) - throw std::system_error(errno, std::generic_category()); - - std::vector<Request *> v; - - { - std::lock_guard guard(gReqlistMutex); - swap(v, gReqList); + if (!cm) { + cm = std::make_shared<PyCameraManager>(); + gCameraManager = cm; } - std::vector<py::object> ret; - - for (Request *req : v) { - py::object o = py::cast(req); - /* Decrease the ref increased in Camera.queue_request() */ - o.dec_ref(); - ret.push_back(o); - } - - return ret; + return cm; }) - .def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>()) - - /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */ - .def_property_readonly("cameras", [](CameraManager &self) { - py::list l; - - for (auto &c : self.cameras()) { - py::object py_cm = py::cast(self); - py::object py_cam = py::cast(c); - py::detail::keep_alive_impl(py_cam, py_cm); - l.append(py_cam); - } + .def_property_readonly("version", &PyCameraManager::version) + .def("get", &PyCameraManager::get, py::keep_alive<0, 1>()) + .def_property_readonly("cameras", &PyCameraManager::cameras) - return l; - }); + .def_property_readonly("event_fd", &PyCameraManager::eventFd) + .def("get_ready_requests", &PyCameraManager::getReadyRequests); pyCamera .def_property_readonly("id", &Camera::id) @@ -191,7 +117,10 @@ PYBIND11_MODULE(_libcamera, m) const std::unordered_map<const ControlId *, py::object> &controls) { /* \todo What happens if someone calls start() multiple times? */ - self.requestCompleted.connect(handleRequestCompleted); + auto cm = gCameraManager.lock(); + ASSERT(cm); + + self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted); ControlList controlList(self.controls()); @@ -202,7 +131,7 @@ PYBIND11_MODULE(_libcamera, m) int ret = self.start(&controlList); if (ret) { - self.requestCompleted.disconnect(handleRequestCompleted); + self.requestCompleted.disconnect(); return ret; } @@ -214,7 +143,7 @@ PYBIND11_MODULE(_libcamera, m) if (ret) return ret; - self.requestCompleted.disconnect(handleRequestCompleted); + self.requestCompleted.disconnect(); return 0; })
Wrap the CameraManager with a PyCameraManager class and move the related code inside the new class. This helps understanding the life times of the used-to-be global variables, gets rid of static handleRequestCompleted function, and allows us to simplify the binding code as the more complex pieces are inside the class. There should be no user visible functional changes. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/libcamera/meson.build | 1 + src/py/libcamera/py_camera_manager.cpp | 125 +++++++++++++++++++++++++ src/py/libcamera/py_camera_manager.h | 44 +++++++++ src/py/libcamera/py_main.cpp | 117 +++++------------------ 4 files changed, 193 insertions(+), 94 deletions(-) create mode 100644 src/py/libcamera/py_camera_manager.cpp create mode 100644 src/py/libcamera/py_camera_manager.h