[libcamera-devel] [PATCH v3 11/17] py: New event handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 19 00:00:26 CEST 2022
Hi Tomi,
Thank you for the patch.
On Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:
> At the moment the Python bindings only handle the requestCompleted
> events. But we have others, bufferCompleted and disconnect from the
> Camera, and cameraAdded and cameraRemoved from the CameraManager.
And we will likely have error events in the not too distant future.
> This patch makes all those events available to the users.
>
> The get_ready_requests() method is now deprecated, but available to keep
> backward compatibility.
Who are the users of get_ready_requests() that you are concern about,
and when could it be removed ? Other changes in this series break the
API (such as switching to exceptions), so all users will need to be
updated, I'd rather remove get_ready_requests() at the same time.
> The new event handling works like get_ready_requests(), but instead of
> returning only Requests from requestCompleted events, it returns all
> event types using a new Event class.
>
> Additionally camera.stop() has been changed to return events for that
> camera. This serves two purposes: first, it removes the requestCompleted
> events from the event queue, thus preventing the old events being
> returned when the camera is started again, and second, it allows the
> user to process those events if it so wants.
>
> All other event types are always collected and returned, except
> bufferCompleted which needs to be enabled by setting the
> CameraManager.buffer_completed_active to True. This is to reduce the
> overhead a bit. It is not clear if this is a necessary optimization or
> not.
As mentioned in the reply to the cover letter, we could have a more
generic subscription mechanism, but I also think it may not make much
sense to make subscription optional for all the other events, perhaps
with the exception of a future metadata completion event.
If only the buffer and metadata completion events are optional, should
we have a per-camera subscription ?
> TODO: Documentation.
This should be captured in the code. Or documentation could be written
:-)
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
> src/py/libcamera/py_camera_manager.h | 64 ++++++++-
> src/py/libcamera/py_main.cpp | 45 +++++-
> 3 files changed, 276 insertions(+), 24 deletions(-)
>
> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> index 3dd8668e..d1d63690 100644
> --- a/src/py/libcamera/py_camera_manager.cpp
> +++ b/src/py/libcamera/py_camera_manager.cpp
> @@ -5,6 +5,7 @@
>
> #include "py_camera_manager.h"
>
> +#include <algorithm>
> #include <errno.h>
> #include <sys/eventfd.h>
> #include <system_error>
> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()
> if (ret)
> throw std::system_error(-ret, std::generic_category(),
> "Failed to start CameraManager");
> +
> + cameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);
> + cameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);
> }
>
> PyCameraManager::~PyCameraManager()
> {
> LOG(Python, Debug) << "~PyCameraManager()";
> +
> + cameraManager_->cameraAdded.disconnect();
> + cameraManager_->cameraRemoved.disconnect();
> }
>
> py::list PyCameraManager::cameras()
> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
> return l;
> }
>
> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> +{
> + PyCameraEvent pyevent;
> +
> + pyevent.type_ = event.type_;
> +
> + /*
> + * We need to set a keep-alive here so that the camera keeps the
> + * camera manager alive.
> + */
> + py::object py_cm = py::cast(this);
> + py::object py_cam = py::cast(event.camera_);
> + py::detail::keep_alive_impl(py_cam, py_cm);
> +
> + pyevent.camera_ = py_cam;
> +
> + switch (event.type_) {
> + case CameraEventType::CameraAdded:
> + case CameraEventType::CameraRemoved:
> + case CameraEventType::Disconnect:
> + /* No additional parameters to add */
> + break;
> +
> + case CameraEventType::BufferCompleted: {
> + pyevent.request_ = py::cast(event.request_);
> + pyevent.fb_ = py::cast(event.fb_);
> + break;
> + }
No need for curly braces. But a blank line would be nice.
> + case CameraEventType::RequestCompleted: {
> + pyevent.request_ = py::cast(event.request_);
> +
> + /* Decrease the ref increased in Camera.queue_request() */
> + pyevent.request_.dec_ref();
> +
> + break;
> + }
> + default:
> + ASSERT(false);
What if we dropped the undefined event type ? You wouldn't need this
then. It's best to write code that makes errors impossible.
I think I would also move all of this, except the py::cast(this) call,
to a PyCameraEvent constructor that takes the CameraEvent and camera
manager py::object.
> + }
> +
> + return pyevent;
> +}
> +
> +/* DEPRECATED */
> std::vector<py::object> PyCameraManager::getReadyRequests()
> {
> int ret = readFd();
> @@ -70,21 +121,125 @@ std::vector<py::object> PyCameraManager::getReadyRequests()
>
> std::vector<py::object> py_reqs;
>
> - for (Request *request : getCompletedRequests()) {
> - py::object o = py::cast(request);
> - /* Decrease the ref increased in Camera.queue_request() */
> - o.dec_ref();
> - py_reqs.push_back(o);
> + for (const auto &ev : getEvents()) {
> + if (ev.type_ != CameraEventType::RequestCompleted)
> + continue;
Ouch, this will silently drop all other types of events. I'd really like
to drop getReadyRequests().
> +
> + PyCameraEvent pyev = convertEvent(ev);
> + py_reqs.push_back(pyev.request_);
> }
>
> return py_reqs;
> }
>
> +std::vector<PyCameraEvent> PyCameraManager::getPyEvents()
> +{
> + int ret = readFd();
> +
> + if (ret == EAGAIN) {
> + LOG(Python, Debug) << "No events";
> + return {};
> + }
> +
> + if (ret != 0)
> + throw std::system_error(ret, std::generic_category());
> +
> + std::vector<CameraEvent> events = getEvents();
> +
> + LOG(Python, Debug) << "Get " << events.size() << " events";
s/Get/Got/
> +
> + std::vector<PyCameraEvent> pyevents(events.size());
You'll end up creating events.size() default-constructed elements here,
only to overwrite them just after. I think calling .reserve() and using
a std::back_inserter in the std::transform loop below would be more
efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).
> +
> + std::transform(events.begin(), events.end(), pyevents.begin(),
> + [this](const CameraEvent &ev) {
> + return convertEvent(ev);
> + });
> +
> + return pyevents;
> +}
> +
> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr<Camera> &camera)
> +{
> + return event.camera_ == camera &&
> + (event.type_ == CameraEventType::RequestCompleted ||
> + event.type_ == CameraEventType::BufferCompleted ||
> + event.type_ == CameraEventType::Disconnect);
I don't think you should include disconnect events here. This would
result in stop() removing disconnect events from the queue, while I
think they need to be handled through the normal event handling
mechanism.
> +}
> +
> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> +{
> + MutexLocker guard(eventsMutex_);
> +
> + std::vector<PyCameraEvent> pyevents;
> +
> + /* Get events related to the given camera */
> +
> + for (const auto &event : events_) {
> + if (!isCameraSpecificEvent(event, camera))
> + continue;
> +
> + PyCameraEvent pyev = convertEvent(event);
> + pyevents.push_back(pyev);
> + }
> +
> + /* Drop the events from the events_ list */
> +
> + events_.erase(std::remove_if(events_.begin(), events_.end(),
> + [&camera](const CameraEvent &ev) {
> + return isCameraSpecificEvent(ev, camera);
> + }),
> + events_.end());
I'd like to minmize the time spent holding the lock. You can move the
events specific to this camera to a different vector without converting
them (and merge the two loops if possible), and the convert the events
without holding the lock.
Also, dropping elements in the middle of a vector is fairly inefficient.
I think you should use a list instead of a vector for events_, which
will allow you to efficiently move elements here with splice().
> +
> + LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "
s/Get/Got/ ?
> + << events_.size() << " unhandled events left";
> +
> + return pyevents;
> +}
> +
> /* Note: Called from another thread */
> -void PyCameraManager::handleRequestCompleted(Request *req)
> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
> {
> - pushRequest(req);
> - writeFd();
> + if (!bufferCompletedEventActive_)
> + return;
> +
> + CameraEvent ev(CameraEventType::BufferCompleted, cam);
> + ev.request_ = req;
> + ev.fb_ = fb;
> +
> + pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> +{
> + CameraEvent ev(CameraEventType::RequestCompleted, cam);
> + ev.request_ = req;
> +
> + pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
> +{
> + CameraEvent ev(CameraEventType::Disconnect, cam);
> +
> + pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
> +{
> + CameraEvent ev(CameraEventType::CameraAdded, cam);
> +
> + pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
> +{
> + CameraEvent ev(CameraEventType::CameraRemoved, cam);
> +
> + pushEvent(ev);
> }
>
> void PyCameraManager::writeFd()
> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
> return 0;
> }
>
> -void PyCameraManager::pushRequest(Request *req)
> +void PyCameraManager::pushEvent(const CameraEvent &ev)
> {
> - MutexLocker guard(completedRequestsMutex_);
> - completedRequests_.push_back(req);
> + MutexLocker guard(eventsMutex_);
> + events_.push_back(ev);
> +
> + writeFd();
This can be done without holding the lock.
> +
> + LOG(Python, Debug) << "Queued events: " << events_.size();
I'm tempted to drop this, it will make the log very chatty. Have you
found it useful to debug applications ?
> }
>
> -std::vector<Request *> PyCameraManager::getCompletedRequests()
> +std::vector<CameraEvent> PyCameraManager::getEvents()
> {
> - std::vector<Request *> v;
> - MutexLocker guard(completedRequestsMutex_);
> - swap(v, completedRequests_);
> + std::vector<CameraEvent> v;
> +
> + MutexLocker guard(eventsMutex_);
> + swap(v, events_);
> +
> return v;
> }
> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> index 3525057d..b313ba9b 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -13,6 +13,47 @@
>
> using namespace libcamera;
>
> +enum class CameraEventType {
> + Undefined = 0,
Unless we drop this (which would be my preference), maybe Invalid
instead of Undefined ? People don't like undefined behaviours :-D
> + CameraAdded,
> + CameraRemoved,
> + Disconnect,
> + RequestCompleted,
> + BufferCompleted,
> +};
> +
> +/*
> + * This event struct is used internally to queue the events we receive from
> + * other threads.
> + */
> +struct CameraEvent {
> + CameraEvent(CameraEventType type, std::shared_ptr<Camera> camera)
> + : type_(type), camera_(camera)
> + {
> + }
> +
> + CameraEventType type_ = CameraEventType::Undefined;
> +
> + std::shared_ptr<Camera> camera_;
> +
> + Request *request_ = nullptr;
> + FrameBuffer *fb_ = nullptr;
> +};
> +
> +/*
> + * This event struct is passed to Python. We need to use pybind11::object here
> + * instead of a C++ pointer so that we keep a ref to the Request, and a
> + * keep-alive from the camera to the camera manager.
> + */
> +struct PyCameraEvent {
> + CameraEventType type_ = CameraEventType::Undefined;
> +
> + pybind11::object camera_;
> +
> + pybind11::object request_;
> + pybind11::object fb_;
> +};
> +
> class PyCameraManager
> {
> public:
> @@ -26,20 +67,29 @@ public:
>
> int eventFd() const { return eventFd_.get(); }
>
> - std::vector<pybind11::object> getReadyRequests();
> + std::vector<pybind11::object> getReadyRequests(); /* DEPRECATED */
> + std::vector<PyCameraEvent> getPyEvents();
> + std::vector<PyCameraEvent> getPyCameraEvents(std::shared_ptr<Camera> camera);
>
> - void handleRequestCompleted(Request *req);
> + void handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb);
> + void handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req);
> + void handleDisconnected(std::shared_ptr<Camera> cam);
> + void handleCameraAdded(std::shared_ptr<Camera> cam);
> + void handleCameraRemoved(std::shared_ptr<Camera> cam);
>
> + bool bufferCompletedEventActive_ = false;
Missing blank line.
> private:
> std::unique_ptr<CameraManager> cameraManager_;
>
> UniqueFD eventFd_;
> - libcamera::Mutex completedRequestsMutex_;
> - std::vector<Request *> completedRequests_
> - LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);
> + libcamera::Mutex eventsMutex_;
> + std::vector<CameraEvent> events_
> + LIBCAMERA_TSA_GUARDED_BY(eventsMutex_);
>
> void writeFd();
> int readFd();
> - void pushRequest(Request *req);
> - std::vector<Request *> getCompletedRequests();
> + void pushEvent(const CameraEvent &ev);
> + std::vector<CameraEvent> getEvents();
> +
> + PyCameraEvent convertEvent(const CameraEvent &event);
> };
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 6cd99919..b4f756d7 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -58,6 +58,7 @@ PYBIND11_MODULE(_libcamera, m)
> * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
> */
>
> + auto pyEvent = py::class_<PyCameraEvent>(m, "Event");
> auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager");
> auto pyCamera = py::class_<Camera>(m, "Camera");
> auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)
> m.def("log_set_level", &logSetLevel);
>
> /* Classes */
> +
> + py::enum_<CameraEventType>(pyEvent, "Type")
> + .value("Undefined", CameraEventType::Undefined)
> + .value("CameraAdded", CameraEventType::CameraAdded)
> + .value("CameraRemoved", CameraEventType::CameraRemoved)
> + .value("Disconnect", CameraEventType::Disconnect)
> + .value("RequestCompleted", CameraEventType::RequestCompleted)
> + .value("BufferCompleted", CameraEventType::BufferCompleted);
> +
> + pyEvent
> + .def_readonly("type", &PyCameraEvent::type_)
> + .def_readonly("camera", &PyCameraEvent::camera_)
> + .def_readonly("request", &PyCameraEvent::request_)
> + .def_readonly("fb", &PyCameraEvent::fb_);
> +
> pyCameraManager
> .def_static("singleton", []() {
> std::shared_ptr<PyCameraManager> cm = gCameraManager.lock();
> @@ -107,7 +123,13 @@ PYBIND11_MODULE(_libcamera, m)
> .def_property_readonly("cameras", &PyCameraManager::cameras)
>
> .def_property_readonly("event_fd", &PyCameraManager::eventFd)
> - .def("get_ready_requests", &PyCameraManager::getReadyRequests);
> +
> + /* DEPRECATED */
Is there any way to generate a warning at runtime when this gets used ?
If we drop it (depending on the outcome of the discussion in the commit
message) that will be even easier :-)
> + .def("get_ready_requests", &PyCameraManager::getReadyRequests)
> +
> + .def("get_events", &PyCameraManager::getPyEvents)
> +
> + .def_readwrite("buffer_completed_active", &PyCameraManager::bufferCompletedEventActive_);
>
> pyCamera
> .def_property_readonly("id", &Camera::id)
> @@ -131,7 +153,17 @@ PYBIND11_MODULE(_libcamera, m)
> auto cm = gCameraManager.lock();
> ASSERT(cm);
>
> - self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
> + self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {
> + cm->handleRequestCompleted(camera, req);
> + });
> +
> + self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {
> + cm->handleBufferCompleted(camera, req, fb);
> + });
> +
> + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {
> + cm->handleDisconnected(camera);
> + });
>
> ControlList controlList(self.controls());
>
> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)
> int ret = self.stop();
>
> self.requestCompleted.disconnect();
> + self.bufferCompleted.disconnect();
> + self.disconnected.disconnect();
> +
> + auto cm = gCameraManager.lock();
> + ASSERT(cm);
> +
> + auto l = cm->getPyCameraEvents(self.shared_from_this());
Please use a more descriptive variable name.
>
> /* \todo Should we just ignore the error? */
> if (ret)
> throw std::system_error(-ret, std::generic_category(),
> "Failed to start camera");
> +
> + return l;
> })
>
> .def("__str__", [](Camera &self) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list