[libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Mon Jun 27 10:52:17 CEST 2022
On 24/06/2022 16:47, Laurent Pinchart wrote:
> 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.
Ok.
>>> + if (fd == -1)
>>> + throw std::system_error(errno, std::generic_category(),
>
> #include <errno.h>
> #include <system_error>
Ok.
>>> + "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.
Yep. I'll do that in a separate patch, as here I'm just trying to move
the code.
>>> + 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()/ ?
CameraManager already has cameras()...
>>> +{
>>> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */
>
> Line wrap.
Ok.
>>> + py::list l;
>>> +
>>> + for (auto &c : cameras()) {
>
> We usually try not to shorten names too much. s/c/camera/ would read
> nicely.
Ok.
>>> + 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.
I take it the concern was about ctrl-c, and that works?
>>> +
>>> + std::vector<Request *> v;
>
> s/v/requests/
Ok.
>>> + getRequests(v);
>
> You can make getRequests() return the vector instead of passing it as a
> reference, and then even do
>
> for (Request *req : getRequests()) {
Ok. I usually keep away from those as I don't seem to ever exactly know
when copies are made and when not...
>>> +
>>> + std::vector<py::object> ret;
>>> +
>>> + for (Request *req : v) {
>
> s/req/request/
Ok.
>>> + 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.
Ok. I'll look at that in a separate patch.
>> 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.
Hmm that may be problematic. At the moment if there's a CameraManager
pointer, the pybind11 bindings can easily convert that instance to a
Python instance. But if we encapsulate it, that's no longer the case, as
you need something extra to associate the CameraManager pointer to the
encapsulating class.
I tried to do that with Cameras at some point (trying to sort out the
destructor issue), but I didn't get it working. Maybe I'll try again
with CameraManager this time.
>>> +{
>>> +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.
Ok. I'll look at that in a separate patch.
>>> + std::vector<Request *> reqList_;
>
> completedRequests_
Ok.
>>> +
>>> + void writeFd();
>>> + void readFd();
>>> + void pushRequest(Request *req);
>>> + void getRequests(std::vector<Request *> &v);
>
> getCompletedRequests()
Ok.
>>> +};
>>> 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.
Yep.
>>> + cm->handleRequestCompleted(req);
>>> + });
>
> So the following didn't work ?
>
> self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
>
> ? I can investigate.
I didn't try that. I don't think what you have above is the same. Note
that in my version I store a shared_ptr<CameraManager> to make sure it
stays alive.
The issue I had (which I asked about in the chat) was in the "New event
handling", when connecting the cameraAdded signal.
Tomi
More information about the libcamera-devel
mailing list