[libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jun 24 12:05:11 CEST 2022
Quoting Tomi Valkeinen (2022-06-23 15:47:33)
> 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.
>
> Note that attaching to requestCompleted signal is a bit funny, as
> apparently the only way to use a lambda with a signal is to provide an
> object instance pointer as the first parameter, even if there's really
> no instance.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
> src/py/libcamera/meson.build | 1 +
> src/py/libcamera/py_camera_manager.cpp | 115 ++++++++++++++++++++++++
> src/py/libcamera/py_camera_manager.h | 39 ++++++++
> src/py/libcamera/py_main.cpp | 120 ++++++-------------------
> 4 files changed, 181 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..c9e5a99c
> --- /dev/null
> +++ b/src/py/libcamera/py_camera_manager.cpp
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> + */
> +
> +#include "py_camera_manager.h"
> +
> +#include <sys/eventfd.h>
> +#include <unistd.h>
> +
> +#include "py_main.h"
> +
> +namespace py = pybind11;
> +
> +using namespace libcamera;
> +
> +PyCameraManager::PyCameraManager()
> +{
> + int fd = eventfd(0, 0);
> + if (fd == -1)
> + throw std::system_error(errno, std::generic_category(),
> + "Failed to create eventfd");
> +
> + eventFd_ = fd;
> +
> + int ret = start();
> + if (ret) {
> + close(fd);
> + eventFd_ = -1;
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to start CameraManager");
> + }
> +}
> +
> +PyCameraManager::~PyCameraManager()
> +{
> + if (eventFd_ != -1) {
> + close(eventFd_);
> + eventFd_ = -1;
> + }
> +}
> +
> +py::list PyCameraManager::getCameras()
> +{
> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */
> + py::list l;
> +
> + for (auto &c : cameras()) {
> + py::object py_cm = py::cast(this);
> + py::object py_cam = py::cast(c);
> + py::detail::keep_alive_impl(py_cam, py_cm);
> + l.append(py_cam);
> + }
> +
> + return l;
> +}
> +
> +std::vector<py::object> PyCameraManager::getReadyRequests()
> +{
> + readFd();
Is this blocking? Does it need any kind of time out of cancellation
support? (What happens if a ctrl-c happens while blocked here etc..)
> +
> + std::vector<Request *> v;
> + getRequests(v);
> +
> + 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;
> +}
> +
> +/* 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 use LOG(Python, Fatal).
> + */
Trivial spacing is wrong here on the last line.
> + if (s != 8)
> + LOG(Python, Fatal) << "Unable to write to eventfd";
Hrm ... should this tie in to throwing a python error to generate a
python backtrace or such too ?
It certainly shouldn't happen though indeed so ... it probably doesn't
matter too much now.
> +}
> +
> +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(reqlist_mutex_);
Aha, I was going to ask about locking up in
PyCameraManager::handleRequestCompleted but now I see it's handled ...
> + reqList_.push_back(req);
> +}
> +
> +void PyCameraManager::getRequests(std::vector<Request *> &v)
> +{
> + std::lock_guard guard(reqlist_mutex_);
> + swap(v, reqList_);
> +}
> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> new file mode 100644
> index 00000000..b0b971ad
> --- /dev/null
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> + */
> +
> +#pragma once
> +
> +#include <mutex>
> +
> +#include <libcamera/libcamera.h>
> +
> +#include <pybind11/smart_holder.h>
> +
> +using namespace libcamera;
> +
> +class PyCameraManager : public CameraManager
> +{
> +public:
> + PyCameraManager();
> + virtual ~PyCameraManager();
> +
> + pybind11::list getCameras();
> +
> + int eventFd() const { return eventFd_; }
> +
> + std::vector<pybind11::object> getReadyRequests();
> +
> + void handleRequestCompleted(Request *req);
> +
> +private:
> + int eventFd_ = -1;
> + std::mutex reqlist_mutex_;
> + std::vector<Request *> reqList_;
> +
> + void writeFd();
> + void readFd();
> + void pushRequest(Request *req);
> + void getRequests(std::vector<Request *> &v);
> +};
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 5a423ece..23018288 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;
> @@ -29,27 +27,11 @@ using namespace libcamera;
>
> 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 use LOG(Python, Fatal).
> - */
> - if (s != 8)
> - LOG(Python, Fatal) << "Unable to write to eventfd";
> -}
> +/*
> + * XXX Note: global C++ destructors can be ran on this before the py module is
XXX or Todo ... or Warning (WARNING!)? Even if this is just a highglight
for review, I think it should stay in the code.
> + * destructed.
> + */
> +static std::weak_ptr<PyCameraManager> gCameraManager;
>
> void init_py_enums(py::module &m);
> void init_py_controls_generated(py::module &m);
> @@ -72,7 +54,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");
> @@ -106,78 +88,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)
> -
> - .def_property_readonly("event_fd", [](CameraManager &) {
> - return gEventfd;
> - })
> + std::shared_ptr<PyCameraManager> cm = gCameraManager.lock();
>
> - .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>())
> + .def_property_readonly("version", &PyCameraManager::version)
> + .def("get", py::overload_cast<const std::string &>(&PyCameraManager::get), py::keep_alive<0, 1>())
> + .def_property_readonly("cameras", &PyCameraManager::getCameras)
>
> - /* 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);
> - }
> -
> - return l;
> - });
> + .def_property_readonly("event_fd", &PyCameraManager::eventFd)
> + .def("get_ready_requests", &PyCameraManager::getReadyRequests);
>
> pyCamera
> .def_property_readonly("id", &Camera::id)
> @@ -187,7 +113,13 @@ 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(&self, [cm=std::move(cm)](Request *req) {
> +
> + cm->handleRequestCompleted(req);
> + });
>
> ControlList controlList(self.controls());
>
> @@ -198,7 +130,7 @@ PYBIND11_MODULE(_libcamera, m)
>
> int ret = self.start(&controlList);
> if (ret) {
> - self.requestCompleted.disconnect(handleRequestCompleted);
> + self.requestCompleted.disconnect();
> return ret;
> }
>
> @@ -210,7 +142,7 @@ PYBIND11_MODULE(_libcamera, m)
> if (ret)
> return ret;
>
> - self.requestCompleted.disconnect(handleRequestCompleted);
> + self.requestCompleted.disconnect();
>
> return 0;
> })
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list