[libcamera-devel] [RFC v1 6/7] py: New event handling
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jun 24 12:53:43 CEST 2022
Quoting Tomi Valkeinen (2022-06-23 15:47:35)
> At the moment the Python bindings only handle the requestCompleted
> events. But we have others, bufferCompleted and disconnec from the
disconnec/disconnect
> Camera, and cameraAdded and cameraRemoved from the CameraManager.
>
> This makes all those events available to the users.
\o/
> The get_ready_requests() method is now deprecated, but available to keep
> backward compatibility.
Aha, once again - answering my questions in the previous patch ;-)
Maybe I need to read patch series backwards ... hehe.
> The new event handling works as follows:
>
> The user sets callbacks to the CameraManager or Cameras (e.g.
> Camera.buffer_completed). When the event_fd informs of an event, the
> user must call CameraManager.dispatch_events() which gets the queued
> events and calls the relevant callbacks for each queued event.
>
> Additionally there is CameraManager.discard_events() if the user does
> not want to process the events but wants to clear the event queue (e.g.
> after stopping the cameras or when exiting the app).
Hrm... in C++ lifetimes, the end of stop() means everything internally
is released, so I'm still weary that if we change that it could be
problematic (i.e. if we have to 'rely' on an application doing clean
up).
> I'm not very happy with this patch. It works fine, but there's a lot of
> repetition of almost-the-same code. Perhaps some template magics might
> reduce the repetition, but I also fear that it can easily make the code
> more difficult to read.
>
> TODO: Documentation.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--
> src/py/libcamera/py_camera_manager.h | 41 +++-
> src/py/libcamera/py_main.cpp | 87 +++++++-
> 3 files changed, 397 insertions(+), 22 deletions(-)
>
> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> index ba45f713..bbadb9ee 100644
> --- a/src/py/libcamera/py_camera_manager.cpp
> +++ b/src/py/libcamera/py_camera_manager.cpp
> @@ -15,6 +15,37 @@ namespace py = pybind11;
>
> using namespace libcamera;
>
> +struct CameraEvent {
> + enum class EventType {
> + Undefined = 0,
> + CameraAdded,
> + CameraRemoved,
> + Disconnect,
> + RequestCompleted,
> + BufferCompleted,
> + };
> +
> + CameraEvent(EventType type)
> + : type(type)
> + {
> + }
> +
> + EventType type;
> +
> + std::shared_ptr<Camera> cam;
> +
> + union {
> + struct {
> + Request *req;
> + FrameBuffer *fb;
> + } buf_completed;
> +
> + struct {
> + Request *req;
> + } req_completed;
> + };
> +};
> +
> PyCameraManager::PyCameraManager()
> {
> int fd = eventfd(0, 0);
> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()
> return l;
> }
>
> +/* DEPRECATED */
> std::vector<py::object> PyCameraManager::getReadyRequests(bool nonBlocking)
> {
> if (!nonBlocking || hasEvents())
> readFd();
>
> - std::vector<Request *> v;
> - getRequests(v);
> + std::vector<CameraEvent> v;
> + getEvents(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);
> + for (const auto &ev : v) {
> + switch (ev.type) {
> + case CameraEvent::EventType::RequestCompleted: {
> + Request *req = ev.req_completed.req;
> + py::object o = py::cast(req);
> + /* Decrease the ref increased in Camera.queue_request() */
> + o.dec_ref();
> + ret.push_back(o);
> + }
> + default:
> + /* ignore */
Does this mean that events will get lost if the deprecated call gets
used? (Ok, so it's deprecated, but will it cause 'bugs' ?)
> + break;
> + }
> }
>
> return ret;
> }
>
> /* Note: Called from another thread */
> -void PyCameraManager::handleRequestCompleted(Request *req)
> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
> +{
> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);
> + ev.cam = cam;
> + ev.buf_completed.req = req;
> + ev.buf_completed.fb = fb;
> +
> + pushEvent(ev);
> + writeFd();
Should writeFd be part of pushEvent? then there should be one entry on
the poll Fd for every event right? And forces them to be kept in sync...
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> +{
> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);
> + ev.cam = cam;
> + ev.req_completed.req = req;
> +
> + pushEvent(ev);
> + writeFd();
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
> {
> - pushRequest(req);
> + CameraEvent ev(CameraEvent::EventType::Disconnect);
> + ev.cam = cam;
> +
> + pushEvent(ev);
> writeFd();
> }
>
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
> +{
> + CameraEvent ev(CameraEvent::EventType::CameraAdded);
> + ev.cam = cam;
> +
> + pushEvent(ev);
> + writeFd();
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
> +{
> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);
> + ev.cam = cam;
> +
> + pushEvent(ev);
> + writeFd();
> +}
Is that what you mean about code duplication? I think it's fine.
These could be templated out to specify the event type I guess ... but
that might be more pain. Especially when the request completed has
different parameters anyway.
> +
> +void PyCameraManager::dispatchEvents(bool nonBlocking)
> +{
> + if (!nonBlocking || hasEvents())
> + readFd();
> +
> + std::vector<CameraEvent> v;
> + getEvents(v);
> +
> + LOG(Python, Debug) << "Dispatch " << v.size() << " events";
> +
> + for (const auto &ev : v) {
> + switch (ev.type) {
> + case CameraEvent::EventType::CameraAdded: {
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + if (cameraAddedHandler_)
> + cameraAddedHandler_(cam);
> +
> + break;
> + }
> + case CameraEvent::EventType::CameraRemoved: {
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + if (cameraRemovedHandler_)
> + cameraRemovedHandler_(cam);
> +
> + break;
> + }
> + case CameraEvent::EventType::BufferCompleted: {
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + auto cb = getBufferCompleted(cam.get());
> +
> + if (cb)
> + cb(cam, ev.buf_completed.req, ev.buf_completed.fb);
> +
> + break;
> + }
> + case CameraEvent::EventType::RequestCompleted: {
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + auto cb = getRequestCompleted(cam.get());
> +
> + if (cb)
> + cb(cam, ev.req_completed.req);
> +
> + /* Decrease the ref increased in Camera.queue_request() */
> + py::object o = py::cast(ev.req_completed.req);
> + o.dec_ref();
> +
> + break;
> + }
> + case CameraEvent::EventType::Disconnect: {
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + auto cb = getDisconnected(cam.get());
> +
> + if (cb)
> + cb(cam);
> +
> + break;
> + }
> + default:
> + assert(false);
Can this be a python throw to generate a backtrace? Or a LOG(Python,
Fatal)?
A plain assert isn't very informative.
> + }
> + }
> +}
> +
> +void PyCameraManager::discardEvents()
> +{
> + if (hasEvents())
> + readFd();
> +
> + std::vector<CameraEvent> v;
> + getEvents(v);
> +
> + LOG(Python, Debug) << "Discard " << v.size() << " events";
> +
> + for (const auto &ev : v) {
> + if (ev.type != CameraEvent::EventType::RequestCompleted)
> + continue;
> +
> + std::shared_ptr<Camera> cam = ev.cam;
> +
> + /* Decrease the ref increased in Camera.queue_request() */
> + py::object o = py::cast(ev.req_completed.req);
> + o.dec_ref();
> + }
> +}
> +
> +std::function<void(std::shared_ptr<Camera>)> PyCameraManager::getCameraAdded() const
> +{
> + return cameraAddedHandler_;
> +}
> +
> +void PyCameraManager::setCameraAdded(std::function<void(std::shared_ptr<Camera>)> fun)
> +{
> + if (cameraAddedHandler_)
> + cameraAdded.disconnect();
> +
> + cameraAddedHandler_ = fun;
> +
> + if (fun)
Really trivially, but I'd call fun fn or func. Otherwise I read this as
"If you're having fun... connect the signal" ;-)
> + cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);
> +}
> +
> +std::function<void(std::shared_ptr<Camera>)> PyCameraManager::getCameraRemoved() const
> +{
> + return cameraRemovedHandler_;
> +}
> +
> +void PyCameraManager::setCameraRemoved(std::function<void(std::shared_ptr<Camera>)> fun)
> +{
> + if (cameraRemovedHandler_)
> + cameraRemoved.disconnect();
> +
> + cameraRemovedHandler_ = fun;
> +
> + if (fun)
> + cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);
> +}
> +
> +std::function<void(std::shared_ptr<Camera>, Request *)> PyCameraManager::getRequestCompleted(Camera *cam)
> +{
> + if (auto it = cameraRequestCompletedHandlers_.find(cam);
> + it != cameraRequestCompletedHandlers_.end())
> + return it->second;
> +
> + return nullptr;
> +}
> +
> +void PyCameraManager::setRequestCompleted(Camera *cam, std::function<void(std::shared_ptr<Camera>, Request *)> fun)
> +{
> + if (fun)
> + cameraRequestCompletedHandlers_[cam] = fun;
> + else
> + cameraRequestCompletedHandlers_.erase(cam);
> +}
> +
> +std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> PyCameraManager::getBufferCompleted(Camera *cam)
> +{
> + if (auto it = cameraBufferCompletedHandlers_.find(cam);
> + it != cameraBufferCompletedHandlers_.end())
> + return it->second;
> +
> + return nullptr;
> +}
> +
> +void PyCameraManager::setBufferCompleted(Camera *cam, std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> fun)
> +{
> + if (fun)
> + cameraBufferCompletedHandlers_[cam] = fun;
> + else
> + cameraBufferCompletedHandlers_.erase(cam);
> +}
> +
> +std::function<void(std::shared_ptr<Camera>)> PyCameraManager::getDisconnected(Camera *cam)
> +{
> + if (auto it = cameraDisconnectHandlers_.find(cam);
> + it != cameraDisconnectHandlers_.end())
> + return it->second;
> +
> + return nullptr;
> +}
> +
> +void PyCameraManager::setDisconnected(Camera *cam, std::function<void(std::shared_ptr<Camera>)> fun)
> +{
> + if (fun)
> + cameraDisconnectHandlers_[cam] = fun;
> + else
> + cameraDisconnectHandlers_.erase(cam);
> +}
> +
Yes, I see, lots more boilerplate code duplication. Not a problem in
it's own right, and if it's templateable then maybe that can be helpful.
> void PyCameraManager::writeFd()
> {
> uint64_t v = 1;
> @@ -104,16 +363,18 @@ void PyCameraManager::readFd()
> throw std::system_error(errno, std::generic_category());
> }
>
> -void PyCameraManager::pushRequest(Request *req)
> +void PyCameraManager::pushEvent(const CameraEvent &ev)
> {
> - std::lock_guard guard(reqlist_mutex_);
> - reqList_.push_back(req);
> + std::lock_guard guard(reqlistMutex_);
> + cameraEvents_.push_back(ev);
> +
> + LOG(Python, Debug) << "Queued events: " << cameraEvents_.size();
> }
>
> -void PyCameraManager::getRequests(std::vector<Request *> &v)
> +void PyCameraManager::getEvents(std::vector<CameraEvent> &v)
> {
> - std::lock_guard guard(reqlist_mutex_);
> - swap(v, reqList_);
> + std::lock_guard guard(reqlistMutex_);
Is this now supposed to be an eventListMutex_?
> + swap(v, cameraEvents_);
> }
>
> bool PyCameraManager::hasEvents()
> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> index 2396d236..fd28291b 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -13,6 +13,8 @@
>
> using namespace libcamera;
>
> +struct CameraEvent;
> +
> class PyCameraManager : public CameraManager
> {
> public:
> @@ -27,15 +29,46 @@ public:
>
> 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);
In libcamera, we don't prefix event handlers with 'handle'. i.e.
handleCameraRemoved would just be cameraRemoved.
Do you prefer it to make it distinct? or is it possible to align with
the existing styles?
> +
> + void dispatchEvents(bool nonBlocking = false);
> + void discardEvents();
> +
> + std::function<void(std::shared_ptr<Camera>)> getCameraAdded() const;
> + void setCameraAdded(std::function<void(std::shared_ptr<Camera>)> fun);
> +
> + std::function<void(std::shared_ptr<Camera>)> getCameraRemoved() const;
> + void setCameraRemoved(std::function<void(std::shared_ptr<Camera>)> fun);
> +
> + std::function<void(std::shared_ptr<Camera>, Request *)> getRequestCompleted(Camera *cam);
> + void setRequestCompleted(Camera *cam, std::function<void(std::shared_ptr<Camera>, Request *)> fun);
> +
> + std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> getBufferCompleted(Camera *cam);
> + void setBufferCompleted(Camera *cam, std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> fun);
> +
> + std::function<void(std::shared_ptr<Camera>)> getDisconnected(Camera *cam);
> + void setDisconnected(Camera *cam, std::function<void(std::shared_ptr<Camera>)> fun);
so many getters and setters. Can the underlying events be more directly
accessibly or do they have to have the setters?
> +
> private:
> int eventFd_ = -1;
> - std::mutex reqlist_mutex_;
> + std::mutex reqlistMutex_;
> std::vector<Request *> reqList_;
> + std::vector<CameraEvent> cameraEvents_;
> +
> + std::function<void(std::shared_ptr<Camera>)> cameraAddedHandler_;
> + std::function<void(std::shared_ptr<Camera>)> cameraRemovedHandler_;
> +
> + std::map<Camera *, std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)>> cameraBufferCompletedHandlers_;
> + std::map<Camera *, std::function<void(std::shared_ptr<Camera>, Request *)>> cameraRequestCompletedHandlers_;
> + std::map<Camera *, std::function<void(std::shared_ptr<Camera>)>> cameraDisconnectHandlers_;
>
> void writeFd();
> void readFd();
> - void pushRequest(Request *req);
> - void getRequests(std::vector<Request *> &v);
> -
> + void pushEvent(const CameraEvent &ev);
> + void getEvents(std::vector<CameraEvent> &v);
> bool hasEvents();
> };
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index ee4ecb9b..b6fd9a9d 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -103,8 +103,20 @@ PYBIND11_MODULE(_libcamera, m)
> .def_property_readonly("cameras", &PyCameraManager::getCameras)
>
> .def_property_readonly("event_fd", &PyCameraManager::eventFd)
> + /* DEPRECATED */
> .def("get_ready_requests", &PyCameraManager::getReadyRequests,
> - py::arg("nonblocking") = false);
> + py::arg("nonblocking") = false)
> + .def("dispatch_events", &PyCameraManager::dispatchEvents,
> + py::arg("nonblocking") = false)
Why are there newlines between camera_added and camera_removed, but not
between get_ready_requests, dispatch_events, and discard_events? Is it
grouping events? or should these be separated by newlines too?
> + .def("discard_events", &PyCameraManager::discardEvents)
> +
> + .def_property("camera_added",
> + &PyCameraManager::getCameraAdded,
> + &PyCameraManager::setCameraAdded)
> +
> + .def_property("camera_removed",
> + &PyCameraManager::getCameraRemoved,
> + &PyCameraManager::setCameraRemoved);
For these chained code styles, if the ; was on a new line by
itself, it wouldn't have to modify the previous property to add a new
one?
Is that a good thing or worse code style for you ?
> pyCamera
> .def_property_readonly("id", &Camera::id)
> @@ -117,9 +129,13 @@ PYBIND11_MODULE(_libcamera, m)
> auto cm = gCameraManager.lock();
> assert(cm);
>
> - self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {
> + /*
> + * Note: We always subscribe requestComplete, as the bindings
> + * use requestComplete event to decrement the Request refcount-
> + */
Interesting, so there are multiple subscribers to the event? Is it still
guaranteed to be referenced while the user code has the request? (I
assume so, and that this is managing the internal refcnts while the
object is inside libcamera).
If the ordering is required, then this layer could subscribe to the
event, and the be responsible for calling the event on the user code and
cleaning up after it completes.
But if it already is fine, then it's a nice example of how multiple
functions can run on the event handler.
>
> - cm->handleRequestCompleted(req);
> + self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {
> + cm->handleRequestCompleted(camera, req);
> });
>
> ControlList controlList(self.controls());
> @@ -148,6 +164,71 @@ PYBIND11_MODULE(_libcamera, m)
> return 0;
> })
>
> + .def_property("request_completed",
> + [](Camera &self) {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + return cm->getRequestCompleted(&self);
> + },
> + [](Camera &self, std::function<void(std::shared_ptr<Camera>, Request *)> f) {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + cm->setRequestCompleted(&self, f);
> +
> + /*
> + * Note: We do not subscribe requestComplete here, as we
> + * do that in the start() method.
> + */
> + })
Ok - so this shows why they all need distince getters and setters ...
> +
> + .def_property("buffer_completed",
> + [](Camera &self) -> std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + return cm->getBufferCompleted(&self);
> + },
> + [](Camera &self, std::function<void(std::shared_ptr<Camera>, Request *, FrameBuffer *)> f) {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + cm->setBufferCompleted(&self, f);
> +
> + self.bufferCompleted.disconnect();
Can a comment explain why we're disconnecting this bufferCompleted? It's
not obvious to me why it's there ... Is it an internal bufferCompleted
handler for some specificness that I haven't seen yet?
> +
> + if (!f)
> + return;
> +
> + self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {
> + cm->handleBufferCompleted(camera, req, fb);
> + });
> + })
> +
> + .def_property("disconnected",
> + [](Camera &self) -> std::function<void(std::shared_ptr<Camera>)> {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + return cm->getDisconnected(&self);
> + },
> + [](Camera &self, std::function<void(std::shared_ptr<Camera>)> f) {
> + auto cm = gCameraManager.lock();
> + assert(cm);
> +
> + cm->setDisconnected(&self, f);
> +
> + self.disconnected.disconnect();
Same here. Are there internal events just to be able to run by default?
> +
> + if (!f)
> + return;
Should the self.disconnected be reconnected if f was null?
> +
> + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {
> + cm->handleDisconnected(camera);
> + });
> + })
> +
> .def("__str__", [](Camera &self) {
> return "<libcamera.Camera '" + self.id() + "'>";
> })
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list