[libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Thu Aug 18 16:39:18 CEST 2022
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.
>
>> +
>> + 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 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.
>> +
>> + 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
More information about the libcamera-devel
mailing list