| Message ID | 20220623144736.78537-7-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote: > 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. I'd rather drop it. We're nowhere close to guaranteeing any API in the Python bindings. > 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). What is this for ? If the user doesn't set any handler for a specific event type, I would expect the corresponding events to be discarded. Anything else will be passed to the event handlers. If it's about ensuring that the event queue is cleared after stop() if the use doesn't call dispatch_events(), I'd rather clear the queue at stop() time, and possible even call dispatch_events() in stop(). > 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@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 { I'd make this a class. > > + enum class EventType { You can s/EventType/Type/ as you'll need to write CameraEvent::Type anyway. > > + Undefined = 0, > > + CameraAdded, > > + CameraRemoved, > > + Disconnect, > > + RequestCompleted, > > + BufferCompleted, > > + }; > > + > > + CameraEvent(EventType type) > > + : type(type) No variable shadowing warning ? Ah, we disable them. It would still be good to avoid any possible issue here, and use type_ for the member. > > + { > > + } > > + > > + EventType type; > > + > > + std::shared_ptr<Camera> cam; > > + > > + union { > > + struct { > > + Request *req; > > + FrameBuffer *fb; > > + } buf_completed; > > + > > + struct { > > + Request *req; > > + } req_completed; > > + }; As this stores pointers only, I think you could simplify it to just Request *request_; FrameBuffer *fb_; and initialize the two pointers to nullptr in the constructor, to more easily catch bugs. > > +}; > > + > > PyCameraManager::PyCameraManager() > > { > > int fd = eventfd(0, 0); > > @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras() > > return l; > > } > > > > +/* DEPRECATED */ Let's drop it :-) > > 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... Sounds like a good idea. > > +} > > + > > +/* 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. Yes, generalizing this is probably not really worth it. > > + > > +void PyCameraManager::dispatchEvents(bool nonBlocking) Can we make this unconditionally non-blocking ? > > +{ > > + if (!nonBlocking || hasEvents()) > > + readFd(); > > + > > + std::vector<CameraEvent> v; s/v/events/ > > + getEvents(v); std::vector<CameraEvent> events = getEvents(); > > + > > + LOG(Python, Debug) << "Dispatch " << v.size() << " events"; > > + > > + for (const auto &ev : v) { s/ev/event/ > > + switch (ev.type) { > > + case CameraEvent::EventType::CameraAdded: { > > + std::shared_ptr<Camera> cam = ev.cam; As there's always a camera in all events, you could move it before the switch. > > + > > + 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()); > > + Extra blank line. Same below. > > + 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" ;-) I'd go for func, we use that already in other places. > > + 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? It would be nice to clean this up a bit indeed if possible. Nothing urgent though. > > + > > 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() + "'>"; > > })
On 24/06/2022 13:53, Kieran Bingham wrote: > 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@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' ?) Lost in what way? They are lost in the sense that the app won't see them, but that's to be expected as the "legacy" app is not aware of such a feature. > >> + 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... Maybe, but the same way as writeFd() and pushEvent() are a pair, the readFd() and getEvents() is a pair. But for the latter pair, readFd() is not always called, so readFd() is separate. Thus I wanted to keep writeFd() separate too. So, they could be merged. Let's see how this evolves... >> +} >> + >> +/* 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. I can change this (and I think there were a few more) asserts to log fatal. I'm not sure if throwing an exception is good if the error indicates to some kind of internal bug. >> + } >> + } >> +} >> + >> +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" ;-) Sigh... Ok... =) > >> + 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. Right. The callback functions are different, but these are so similar that something must be possible. But I fear templates will make the code more difficult to read and maintain. >> 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_? Yes. Or rather, cameraEventsMutex_ > >> + 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? We inherit CameraManager, which already has cameraRemoved... And even if it didn't, isn't "cameraRemoved" a bad name for a function that handles an event? Shouldn't functions be about doing something? If it's just for signals and their handlers, isn't it bad there too? A signal would be "cameraRemoved" and the handler would be "cameraRemoved", right? >> + >> + 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? I felt it makes the main bindings, in py_main.cpp cleaner if we don't expose data structures from the PyCameraManager and force the py_main.cpp to do the work with them. >> + >> 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? No particular reason. I often try to group things, or separate things, but it's not always quite consistent (if it even could be). > >> + .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 ? That's what I had originally, but I dropped it either because of a review comment, or because clang-format very much wants to move the ; to the end of the line. Or maybe a style checker warned about it. > >> 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 Just a single subscriber. The comment above describes why this one, requestComplete, is different than the other events. For the other events the bindings are just passing them through. But the bindings require requestComplete, so it's always implicitly subscribed. Or maybe you mean something else, but if so, I didn't get your point =). > 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? When the user sets the callback function, like cam.buffer_completed = foobar we store the callback (cm->setBufferCompleted()), we then disconnect any previously connected handler, and then connect the new handler (unless !f). So the disconnect is just disconnecting (possibly) previously set handler. However, that signal handler lambda below is always the same (we don't pass the 'f' from the user), so we don't actually need to reconnect if already connected. So this could be optimized a bit by - disconnecting only if we have connected earlier and !f - connecting only if not connected earlier and f But it may not be worth it, especially as we don't have the "have we already connected?" available here. >> + >> + 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? I don't think I understand the question. What do you mean with internal events? The requestCompleted is the only one that has special handling. >> + >> + if (!f) >> + return; > > Should the self.disconnected be reconnected if f was null? Hmm, no... If the f is null, we don't want to see any disconnect events. Maybe the confusion comes from the layered signal handling. The bindings connect the C++ signals to its internal handlers. These handlers store the events. Later the bindings use stored Python callback functions to deliver the events. So e.g. cm->setDisconnected(&self, f); above stores the Python callback, it doesn't deal with the C++ signals/handling in any way. >> + >> + 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 >>
On 24/06/2022 17:44, Laurent Pinchart wrote: > On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote: >> 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. > > I'd rather drop it. We're nowhere close to guaranteeing any API in the > Python bindings. It's nice to keep the existing py scripts working. I can drop it eventually, but as it's so trivial (at least so far) to keep it, I'd rather have it. >> 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). > > What is this for ? If the user doesn't set any handler for a specific > event type, I would expect the corresponding events to be discarded. > Anything else will be passed to the event handlers. If it's about > ensuring that the event queue is cleared after stop() if the use doesn't > call dispatch_events(), I'd rather clear the queue at stop() time, and > possible even call dispatch_events() in stop(). It's mainly for exit time. If there are unhandled events in the event queue, CameraManager and Cameras are never freed. As the app is exiting there should be no other downsides to this but memory leak checkers possibly complaining. The user could alternatively call dispatch_event(), but then he either must be able to process the dispatched events or unsubscribe the callbacks first. >> 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@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 { > > I'd make this a class. Why? >>> + enum class EventType { > > You can s/EventType/Type/ as you'll need to write CameraEvent::Type > anyway. That's true. >>> + Undefined = 0, >>> + CameraAdded, >>> + CameraRemoved, >>> + Disconnect, >>> + RequestCompleted, >>> + BufferCompleted, >>> + }; >>> + >>> + CameraEvent(EventType type) >>> + : type(type) > > No variable shadowing warning ? Ah, we disable them. It would still be > good to avoid any possible issue here, and use type_ for the member. Ok. >>> + { >>> + } >>> + >>> + EventType type; >>> + >>> + std::shared_ptr<Camera> cam; >>> + >>> + union { >>> + struct { >>> + Request *req; >>> + FrameBuffer *fb; >>> + } buf_completed; >>> + >>> + struct { >>> + Request *req; >>> + } req_completed; >>> + }; > > As this stores pointers only, I think you could simplify it to just > > Request *request_; > FrameBuffer *fb_; > > and initialize the two pointers to nullptr in the constructor, to more > easily catch bugs. Yes, I think that's fine. I also thought about std::variant, but it felt like going a bit too far. This could backfire at some point, though, if we get new events with data that doesn't quite match the above. >>> +}; >>> + >>> PyCameraManager::PyCameraManager() >>> { >>> int fd = eventfd(0, 0); >>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras() >>> return l; >>> } >>> >>> +/* DEPRECATED */ > > Let's drop it :-) > >>> 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... > > Sounds like a good idea. > >>> +} >>> + >>> +/* 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. > > Yes, generalizing this is probably not really worth it. > >>> + >>> +void PyCameraManager::dispatchEvents(bool nonBlocking) > > Can we make this unconditionally non-blocking ? > >>> +{ >>> + if (!nonBlocking || hasEvents()) >>> + readFd(); >>> + >>> + std::vector<CameraEvent> v; > > s/v/events/ > >>> + getEvents(v); > > std::vector<CameraEvent> events = getEvents(); > >>> + >>> + LOG(Python, Debug) << "Dispatch " << v.size() << " events"; >>> + >>> + for (const auto &ev : v) { > > s/ev/event/ This style of using long variable names and yet insisting on short lines is sometimes a bit annoying =). >>> + switch (ev.type) { >>> + case CameraEvent::EventType::CameraAdded: { >>> + std::shared_ptr<Camera> cam = ev.cam; > > As there's always a camera in all events, you could move it before the > switch. True. Originally I had the camera separately for each event. Tomi
Hi Tomi, On Mon, Jun 27, 2022 at 05:40:13PM +0300, Tomi Valkeinen wrote: > On 24/06/2022 17:44, Laurent Pinchart wrote: > > On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote: > >> 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. > > > > I'd rather drop it. We're nowhere close to guaranteeing any API in the > > Python bindings. > > It's nice to keep the existing py scripts working. I can drop it > eventually, but as it's so trivial (at least so far) to keep it, I'd > rather have it. I'm OK with that if it can help with the transition to the new API, but I'd like to drop legacy code sooner than later. How long do you expect to keep this ? > >> 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). > > > > What is this for ? If the user doesn't set any handler for a specific > > event type, I would expect the corresponding events to be discarded. > > Anything else will be passed to the event handlers. If it's about > > ensuring that the event queue is cleared after stop() if the use doesn't > > call dispatch_events(), I'd rather clear the queue at stop() time, and > > possible even call dispatch_events() in stop(). > > It's mainly for exit time. If there are unhandled events in the event > queue, CameraManager and Cameras are never freed. As the app is exiting > there should be no other downsides to this but memory leak checkers > possibly complaining. > > The user could alternatively call dispatch_event(), but then he either > must be able to process the dispatched events or unsubscribe the > callbacks first. > > >> 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@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 { > > > > I'd make this a class. > > Why? I'm not sure, I suppose it's the constructor and nested type that makes it different enough from a C struct, but there's nothing in C++ that states a struct shouldn't be used. > >>> + enum class EventType { > > > > You can s/EventType/Type/ as you'll need to write CameraEvent::Type > > anyway. > > That's true. > > >>> + Undefined = 0, > >>> + CameraAdded, > >>> + CameraRemoved, > >>> + Disconnect, > >>> + RequestCompleted, > >>> + BufferCompleted, > >>> + }; > >>> + > >>> + CameraEvent(EventType type) > >>> + : type(type) > > > > No variable shadowing warning ? Ah, we disable them. It would still be > > good to avoid any possible issue here, and use type_ for the member. > > Ok. > > >>> + { > >>> + } > >>> + > >>> + EventType type; > >>> + > >>> + std::shared_ptr<Camera> cam; > >>> + > >>> + union { > >>> + struct { > >>> + Request *req; > >>> + FrameBuffer *fb; > >>> + } buf_completed; > >>> + > >>> + struct { > >>> + Request *req; > >>> + } req_completed; > >>> + }; > > > > As this stores pointers only, I think you could simplify it to just > > > > Request *request_; > > FrameBuffer *fb_; > > > > and initialize the two pointers to nullptr in the constructor, to more > > easily catch bugs. > > Yes, I think that's fine. I also thought about std::variant, but it felt > like going a bit too far. > > This could backfire at some point, though, if we get new events with > data that doesn't quite match the above. That's true, but we can fix it later in that case. > >>> +}; > >>> + > >>> PyCameraManager::PyCameraManager() > >>> { > >>> int fd = eventfd(0, 0); > >>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras() > >>> return l; > >>> } > >>> > >>> +/* DEPRECATED */ > > > > Let's drop it :-) > > > >>> 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... > > > > Sounds like a good idea. > > > >>> +} > >>> + > >>> +/* 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. > > > > Yes, generalizing this is probably not really worth it. > > > >>> + > >>> +void PyCameraManager::dispatchEvents(bool nonBlocking) > > > > Can we make this unconditionally non-blocking ? > > > >>> +{ > >>> + if (!nonBlocking || hasEvents()) > >>> + readFd(); > >>> + > >>> + std::vector<CameraEvent> v; > > > > s/v/events/ > > > >>> + getEvents(v); > > > > std::vector<CameraEvent> events = getEvents(); > > > >>> + > >>> + LOG(Python, Debug) << "Dispatch " << v.size() << " events"; > >>> + > >>> + for (const auto &ev : v) { > > > > s/ev/event/ > > This style of using long variable names and yet insisting on short lines > is sometimes a bit annoying =). Conflicting requirements ? :-) I agree, and shortening can make sense, but I'd do so only when it helps. > >>> + switch (ev.type) { > >>> + case CameraEvent::EventType::CameraAdded: { > >>> + std::shared_ptr<Camera> cam = ev.cam; > > > > As there's always a camera in all events, you could move it before the > > switch. > > True. Originally I had the camera separately for each event.
On 30/06/2022 11:28, Laurent Pinchart wrote: > Hi Tomi, > > On Mon, Jun 27, 2022 at 05:40:13PM +0300, Tomi Valkeinen wrote: >> On 24/06/2022 17:44, Laurent Pinchart wrote: >>> On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote: >>>> 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. >>> >>> I'd rather drop it. We're nowhere close to guaranteeing any API in the >>> Python bindings. >> >> It's nice to keep the existing py scripts working. I can drop it >> eventually, but as it's so trivial (at least so far) to keep it, I'd >> rather have it. > > I'm OK with that if it can help with the transition to the new API, but > I'd like to drop legacy code sooner than later. How long do you expect > to keep this ? I'll convert the examples etc. in libcamera repository (note that this series so far only converts cam.py), which will be trivial, but picamera2 may be more difficult. So I'd say David can decide. Probably we can drop it quite soon after picamera2 uses it. However, as I'm still not sure if the new event handling API is good enough, and shouldn't be merged, the get_ready_requests is not deprecated yet. Tomi
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 */ + 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(); +} + +/* 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(); +} + +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); + } + } +} + +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) + 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); +} + 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_); + 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); + + 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); + 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) + .def("discard_events", &PyCameraManager::discardEvents) + + .def_property("camera_added", + &PyCameraManager::getCameraAdded, + &PyCameraManager::setCameraAdded) + + .def_property("camera_removed", + &PyCameraManager::getCameraRemoved, + &PyCameraManager::setCameraRemoved); 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- + */ - 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. + */ + }) + + .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(); + + 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(); + + if (!f) + return; + + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() { + cm->handleDisconnected(camera); + }); + }) + .def("__str__", [](Camera &self) { return "<libcamera.Camera '" + self.id() + "'>"; })
At the moment the Python bindings only handle the requestCompleted events. But we have others, bufferCompleted and disconnec from the Camera, and cameraAdded and cameraRemoved from the CameraManager. This makes all those events available to the users. The get_ready_requests() method is now deprecated, but available to keep backward compatibility. 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). 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@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(-)