[libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Mon Jun 27 09:22:16 CEST 2022
On 24/06/2022 13:05, 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);
>> + 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..)
Yes. No. "RuntimeError: Interrupted system call".
>> +
>> + 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.
Hmm interesting, it was indented with spaces, but no tool complained
about it.
>
>> + 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.
This is called in a callback from libcamera, in non-Python thread. We
could possibly set some flag, and then react to that flag when the
Python thread handles the events. But... This should really never
happen, so I think that's not needed.
>> +}
>> +
>> +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.
Ah, I forgot to remove the XXX. It's not really a todo or even a
warning. Just a reminder if hitting odd problems with object lifetimes.
I think I got those fixed with this series, but it doesn't hurt to keep
the comment so that I remember the next time =).
Tomi
More information about the libcamera-devel
mailing list