[libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 24 15:47:54 CEST 2022
On Fri, Jun 24, 2022 at 11:05:11AM +0100, Kieran Bingham wrote:
> 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);
In a separate patch I'd set at least the EFD_CLOEXEC flag.
> > + if (fd == -1)
> > + throw std::system_error(errno, std::generic_category(),
#include <errno.h>
#include <system_error>
> > + "Failed to create eventfd");
> > +
> > + eventFd_ = fd;
> > +
> > + int ret = start();
> > + if (ret) {
> > + close(fd);
> > + eventFd_ = -1;
You could use the UniqueFD class for eventFd_, this could when then
disappear. Same for the destructor.
> > + 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()
s/getCameras()/cameras()/ ?
> > +{
> > + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */
Line wrap.
> > + py::list l;
> > +
> > + for (auto &c : cameras()) {
We usually try not to shorten names too much. s/c/camera/ would read
nicely.
> > + 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..)
I expect this to be rework in the next patches of the series, but maybe
some of these concerns could be addressed already.
> > +
> > + std::vector<Request *> v;
s/v/requests/
> > + getRequests(v);
You can make getRequests() return the vector instead of passing it as a
reference, and then even do
for (Request *req : getRequests()) {
> > +
> > + std::vector<py::object> ret;
> > +
> > + for (Request *req : v) {
s/req/request/
> > + 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 ?
We can't, as this isn't run from the Python thread.
> 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_);
Could you use the MutexLocker class ? That gives us access to
thread-safety annotations.
> 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
CameraManager isn't meant to be inherited from (I'm surprised it's not
marked as final). You can keep this for now, but we should soon switch
to encapsulating the CameraManager instead.
> > +{
> > +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_;
Mutex instead of std::mutex, and s/reqlist_mutex_/requestsListMutex_/
(or similar). You can also call it just lock_, and use the
LIBCAMERA_TSA_ macros to annotate the fields it protects, that's even
more explicit.
> > + std::vector<Request *> reqList_;
completedRequests_
> > +
> > + void writeFd();
> > + void readFd();
> > + void pushRequest(Request *req);
> > + void getRequests(std::vector<Request *> &v);
getCompletedRequests()
> > +};
> > 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) {
> > +
Extra blank line.
> > + cm->handleRequestCompleted(req);
> > + });
So the following didn't work ?
self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
? I can investigate.
> >
> > 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;
> > })
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list