[libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 18 17:00:17 CEST 2022
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 at 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 at 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 at 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
More information about the libcamera-devel
mailing list