[{"id":23570,"web_url":"https://patchwork.libcamera.org/comment/23570/","msgid":"<165606802330.1149771.2821643337790848162@Monstersaurus>","date":"2022-06-24T10:53:43","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n> At the moment the Python bindings only handle the requestCompleted\n> events. But we have others, bufferCompleted and disconnec from the\n\ndisconnec/disconnect\n\n> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> \n> This makes all those events available to the users.\n\n\\o/\n\n\n> The get_ready_requests() method is now deprecated, but available to keep\n> backward compatibility.\n\nAha, once again - answering my questions in the previous patch ;-)\nMaybe I need to read patch series backwards ... hehe.\n\n \n> The new event handling works as follows:\n> \n> The user sets callbacks to the CameraManager or Cameras (e.g.\n> Camera.buffer_completed). When the event_fd informs of an event, the\n> user must call CameraManager.dispatch_events() which gets the queued\n> events and calls the relevant callbacks for each queued event.\n> \n> Additionally there is CameraManager.discard_events() if the user does\n> not want to process the events but wants to clear the event queue (e.g.\n> after stopping the cameras or when exiting the app).\n\nHrm... in C++ lifetimes, the end of stop() means everything internally\nis released, so I'm still weary that if we change that it could be\nproblematic (i.e. if we have to 'rely' on an application doing clean\nup).\n\n \n> I'm not very happy with this patch. It works fine, but there's a lot of\n> repetition of almost-the-same code. Perhaps some template magics might\n> reduce the repetition, but I also fear that it can easily make the code\n> more difficult to read.\n> \n> TODO: Documentation.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--\n> src/py/libcamera/py_camera_manager.h | 41 +++-\n> src/py/libcamera/py_main.cpp | 87 +++++++-\n> 3 files changed, 397 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> index ba45f713..bbadb9ee 100644\n> --- a/src/py/libcamera/py_camera_manager.cpp\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -15,6 +15,37 @@ namespace py = pybind11;\n> \n> using namespace libcamera;\n> \n> +struct CameraEvent {\n> + enum class EventType {\n> + Undefined = 0,\n> + CameraAdded,\n> + CameraRemoved,\n> + Disconnect,\n> + RequestCompleted,\n> + BufferCompleted,\n> + };\n> +\n> + CameraEvent(EventType type)\n> + : type(type)\n> + {\n> + }\n> +\n> + EventType type;\n> +\n> + std::shared_ptr cam;\n> +\n> + union {\n> + struct {\n> + Request *req;\n> + FrameBuffer *fb;\n> + } buf_completed;\n> +\n> + struct {\n> + Request *req;\n> + } req_completed;\n> + };\n> +};\n> +\n> PyCameraManager::PyCameraManager()\n> {\n> int fd = eventfd(0, 0);\n> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()\n> return l;\n> }\n> \n> +/* DEPRECATED */\n> std::vector PyCameraManager::getReadyRequests(bool nonBlocking)\n> {\n> if (!nonBlocking || hasEvents())\n> readFd();\n> \n> - std::vector v;\n> - getRequests(v);\n> + std::vector v;\n> + getEvents(v);\n> \n> std::vector ret;\n> \n> - for (Request *req : v) {\n> - py::object o = py::cast(req);\n> - /* Decrease the ref increased in Camera.queue_request() */\n> - o.dec_ref();\n> - ret.push_back(o);\n> + for (const auto &ev : v) {\n> + switch (ev.type) {\n> + case CameraEvent::EventType::RequestCompleted: {\n> + Request *req = ev.req_completed.req;\n> + py::object o = py::cast(req);\n> + /* Decrease the ref increased in Camera.queue_request() */\n> + o.dec_ref();\n> + ret.push_back(o);\n> + }\n> + default:\n> + /* ignore */\n\nDoes this mean that events will get lost if the deprecated call gets\nused? (Ok, so it's deprecated, but will it cause 'bugs' ?)\n\n\n> + break;\n> + }\n> }\n> \n> return ret;\n> }\n> \n> /* Note: Called from another thread */\n> -void PyCameraManager::handleRequestCompleted(Request *req)\n> +void PyCameraManager::handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb)\n> +{\n> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);\n> + ev.cam = cam;\n> + ev.buf_completed.req = req;\n> + ev.buf_completed.fb = fb;\n> +\n> + pushEvent(ev);\n> + writeFd();\n\nShould writeFd be part of pushEvent? then there should be one entry on\nthe poll Fd for every event right? And forces them to be kept in sync...\n\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> +{\n> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);\n> + ev.cam = cam;\n> + ev.req_completed.req = req;\n> +\n> + pushEvent(ev);\n> + writeFd();\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> {\n> - pushRequest(req);\n> + CameraEvent ev(CameraEvent::EventType::Disconnect);\n> + ev.cam = cam;\n> +\n> + pushEvent(ev);\n> writeFd();\n> }\n> \n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> +{\n> + CameraEvent ev(CameraEvent::EventType::CameraAdded);\n> + ev.cam = cam;\n> +\n> + pushEvent(ev);\n> + writeFd();\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> +{\n> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);\n> + ev.cam = cam;\n> +\n> + pushEvent(ev);\n> + writeFd();\n> +}\n\nIs that what you mean about code duplication? I think it's fine.\n\nThese could be templated out to specify the event type I guess ... but\nthat might be more pain. Especially when the request completed has\ndifferent parameters anyway.\n\n> +\n> +void PyCameraManager::dispatchEvents(bool nonBlocking)\n> +{\n> + if (!nonBlocking || hasEvents())\n> + readFd();\n> +\n> + std::vector v;\n> + getEvents(v);\n> +\n> + LOG(Python, Debug) << \"Dispatch \" << v.size() << \" events\";\n> +\n> + for (const auto &ev : v) {\n> + switch (ev.type) {\n> + case CameraEvent::EventType::CameraAdded: {\n> + std::shared_ptr cam = ev.cam;\n> +\n> + if (cameraAddedHandler_)\n> + cameraAddedHandler_(cam);\n> +\n> + break;\n> + }\n> + case CameraEvent::EventType::CameraRemoved: {\n> + std::shared_ptr cam = ev.cam;\n> +\n> + if (cameraRemovedHandler_)\n> + cameraRemovedHandler_(cam);\n> +\n> + break;\n> + }\n> + case CameraEvent::EventType::BufferCompleted: {\n> + std::shared_ptr cam = ev.cam;\n> +\n> + auto cb = getBufferCompleted(cam.get());\n> +\n> + if (cb)\n> + cb(cam, ev.buf_completed.req, ev.buf_completed.fb);\n> +\n> + break;\n> + }\n> + case CameraEvent::EventType::RequestCompleted: {\n> + std::shared_ptr cam = ev.cam;\n> +\n> + auto cb = getRequestCompleted(cam.get());\n> +\n> + if (cb)\n> + cb(cam, ev.req_completed.req);\n> +\n> + /* Decrease the ref increased in Camera.queue_request() */\n> + py::object o = py::cast(ev.req_completed.req);\n> + o.dec_ref();\n> +\n> + break;\n> + }\n> + case CameraEvent::EventType::Disconnect: {\n> + std::shared_ptr cam = ev.cam;\n> +\n> + auto cb = getDisconnected(cam.get());\n> +\n> + if (cb)\n> + cb(cam);\n> +\n> + break;\n> + }\n> + default:\n> + assert(false);\n\nCan this be a python throw to generate a backtrace? Or a LOG(Python,\nFatal)? \n\nA plain assert isn't very informative.\n\n> + }\n> + }\n> +}\n> +\n> +void PyCameraManager::discardEvents()\n> +{\n> + if (hasEvents())\n> + readFd();\n> +\n> + std::vector v;\n> + getEvents(v);\n> +\n> + LOG(Python, Debug) << \"Discard \" << v.size() << \" events\";\n> +\n> + for (const auto &ev : v) {\n> + if (ev.type != CameraEvent::EventType::RequestCompleted)\n> + continue;\n> +\n> + std::shared_ptr cam = ev.cam;\n> +\n> + /* Decrease the ref increased in Camera.queue_request() */\n> + py::object o = py::cast(ev.req_completed.req);\n> + o.dec_ref();\n> + }\n> +}\n> +\n> +std::function)> PyCameraManager::getCameraAdded() const\n> +{\n> + return cameraAddedHandler_;\n> +}\n> +\n> +void PyCameraManager::setCameraAdded(std::function)> fun)\n> +{\n> + if (cameraAddedHandler_)\n> + cameraAdded.disconnect();\n> +\n> + cameraAddedHandler_ = fun;\n> +\n> + if (fun)\n\nReally trivially, but I'd call fun fn or func. Otherwise I read this as\n\"If you're having fun... connect the signal\" ;-)\n\n\n> + cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n> +}\n> +\n> +std::function)> PyCameraManager::getCameraRemoved() const\n> +{\n> + return cameraRemovedHandler_;\n> +}\n> +\n> +void PyCameraManager::setCameraRemoved(std::function)> fun)\n> +{\n> + if (cameraRemovedHandler_)\n> + cameraRemoved.disconnect();\n> +\n> + cameraRemovedHandler_ = fun;\n> +\n> + if (fun)\n> + cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n> +}\n> +\n> +std::function, Request *)> PyCameraManager::getRequestCompleted(Camera *cam)\n> +{\n> + if (auto it = cameraRequestCompletedHandlers_.find(cam);\n> + it != cameraRequestCompletedHandlers_.end())\n> + return it->second;\n> +\n> + return nullptr;\n> +}\n> +\n> +void PyCameraManager::setRequestCompleted(Camera *cam, std::function, Request *)> fun)\n> +{\n> + if (fun)\n> + cameraRequestCompletedHandlers_[cam] = fun;\n> + else\n> + cameraRequestCompletedHandlers_.erase(cam);\n> +}\n> +\n> +std::function, Request *, FrameBuffer *)> PyCameraManager::getBufferCompleted(Camera *cam)\n> +{\n> + if (auto it = cameraBufferCompletedHandlers_.find(cam);\n> + it != cameraBufferCompletedHandlers_.end())\n> + return it->second;\n> +\n> + return nullptr;\n> +}\n> +\n> +void PyCameraManager::setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun)\n> +{\n> + if (fun)\n> + cameraBufferCompletedHandlers_[cam] = fun;\n> + else\n> + cameraBufferCompletedHandlers_.erase(cam);\n> +}\n> +\n> +std::function)> PyCameraManager::getDisconnected(Camera *cam)\n> +{\n> + if (auto it = cameraDisconnectHandlers_.find(cam);\n> + it != cameraDisconnectHandlers_.end())\n> + return it->second;\n> +\n> + return nullptr;\n> +}\n> +\n> +void PyCameraManager::setDisconnected(Camera *cam, std::function)> fun)\n> +{\n> + if (fun)\n> + cameraDisconnectHandlers_[cam] = fun;\n> + else\n> + cameraDisconnectHandlers_.erase(cam);\n> +}\n> +\n\nYes, I see, lots more boilerplate code duplication. Not a problem in\nit's own right, and if it's templateable then maybe that can be helpful.\n\n> void PyCameraManager::writeFd()\n> {\n> uint64_t v = 1;\n> @@ -104,16 +363,18 @@ void PyCameraManager::readFd()\n> throw std::system_error(errno, std::generic_category());\n> }\n> \n> -void PyCameraManager::pushRequest(Request *req)\n> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n> {\n> - std::lock_guard guard(reqlist_mutex_);\n> - reqList_.push_back(req);\n> + std::lock_guard guard(reqlistMutex_);\n> + cameraEvents_.push_back(ev);\n> +\n> + LOG(Python, Debug) << \"Queued events: \" << cameraEvents_.size();\n> }\n> \n> -void PyCameraManager::getRequests(std::vector &v)\n> +void PyCameraManager::getEvents(std::vector &v)\n> {\n> - std::lock_guard guard(reqlist_mutex_);\n> - swap(v, reqList_);\n> + std::lock_guard guard(reqlistMutex_);\n\nIs this now supposed to be an eventListMutex_?\n\n\n> + swap(v, cameraEvents_);\n> }\n> \n> bool PyCameraManager::hasEvents()\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> index 2396d236..fd28291b 100644\n> --- a/src/py/libcamera/py_camera_manager.h\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -13,6 +13,8 @@\n> \n> using namespace libcamera;\n> \n> +struct CameraEvent;\n> +\n> class PyCameraManager : public CameraManager\n> {\n> public:\n> @@ -27,15 +29,46 @@ public:\n> \n> void handleRequestCompleted(Request *req);\n> \n> + void handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n> + void handleRequestCompleted(std::shared_ptr cam, Request *req);\n> + void handleDisconnected(std::shared_ptr cam);\n> + void handleCameraAdded(std::shared_ptr cam);\n> + void handleCameraRemoved(std::shared_ptr cam);\n\nIn libcamera, we don't prefix event handlers with 'handle'. i.e.\nhandleCameraRemoved would just be cameraRemoved.\n\nDo you prefer it to make it distinct? or is it possible to align with\nthe existing styles?\n\n\n> +\n> + void dispatchEvents(bool nonBlocking = false);\n> + void discardEvents();\n> +\n> + std::function)> getCameraAdded() const;\n> + void setCameraAdded(std::function)> fun);\n> +\n> + std::function)> getCameraRemoved() const;\n> + void setCameraRemoved(std::function)> fun);\n> +\n> + std::function, Request *)> getRequestCompleted(Camera *cam);\n> + void setRequestCompleted(Camera *cam, std::function, Request *)> fun);\n> +\n> + std::function, Request *, FrameBuffer *)> getBufferCompleted(Camera *cam);\n> + void setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun);\n> +\n> + std::function)> getDisconnected(Camera *cam);\n> + void setDisconnected(Camera *cam, std::function)> fun);\n\nso many getters and setters. Can the underlying events be more directly\naccessibly or do they have to have the setters?\n\n> +\n> private:\n> int eventFd_ = -1;\n> - std::mutex reqlist_mutex_;\n> + std::mutex reqlistMutex_;\n> std::vector reqList_;\n> + std::vector cameraEvents_;\n> +\n> + std::function)> cameraAddedHandler_;\n> + std::function)> cameraRemovedHandler_;\n> +\n> + std::map, Request *, FrameBuffer *)>> cameraBufferCompletedHandlers_;\n> + std::map, Request *)>> cameraRequestCompletedHandlers_;\n> + std::map)>> cameraDisconnectHandlers_;\n> \n> void writeFd();\n> void readFd();\n> - void pushRequest(Request *req);\n> - void getRequests(std::vector &v);\n> -\n> + void pushEvent(const CameraEvent &ev);\n> + void getEvents(std::vector &v);\n> bool hasEvents();\n> };\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index ee4ecb9b..b6fd9a9d 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -103,8 +103,20 @@ PYBIND11_MODULE(_libcamera, m)\n> .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n> \n> .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> + /* DEPRECATED */\n> .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests,\n> - py::arg(\"nonblocking\") = false);\n> + py::arg(\"nonblocking\") = false)\n> + .def(\"dispatch_events\", &PyCameraManager::dispatchEvents,\n> + py::arg(\"nonblocking\") = false)\n\nWhy are there newlines between camera_added and camera_removed, but not\nbetween get_ready_requests, dispatch_events, and discard_events? Is it\ngrouping events? or should these be separated by newlines too?\n\n\n> + .def(\"discard_events\", &PyCameraManager::discardEvents)\n> +\n> + .def_property(\"camera_added\",\n> + &PyCameraManager::getCameraAdded,\n> + &PyCameraManager::setCameraAdded)\n> +\n> + .def_property(\"camera_removed\",\n> + &PyCameraManager::getCameraRemoved,\n> + &PyCameraManager::setCameraRemoved);\n\nFor these chained code styles, if the ; was on a new line by\nitself, it wouldn't have to modify the previous property to add a new\none?\n\nIs that a good thing or worse code style for you ?\n\n \n> pyCamera\n> .def_property_readonly(\"id\", &Camera::id)\n> @@ -117,9 +129,13 @@ PYBIND11_MODULE(_libcamera, m)\n> auto cm = gCameraManager.lock();\n> assert(cm);\n> \n> - self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> + /*\n> + * Note: We always subscribe requestComplete, as the bindings\n> + * use requestComplete event to decrement the Request refcount-\n> + */\n\nInteresting, so there are multiple subscribers to the event? Is it still\nguaranteed to be referenced while the user code has the request? (I\nassume so, and that this is managing the internal refcnts while the\nobject is inside libcamera).\n\n\nIf the ordering is required, then this layer could subscribe to the\nevent, and the be responsible for calling the event on the user code and\ncleaning up after it completes.\n\nBut if it already is fine, then it's a nice example of how multiple\nfunctions can run on the event handler.\n\n> \n> - cm->handleRequestCompleted(req);\n> + self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n> + cm->handleRequestCompleted(camera, req);\n> });\n> \n> ControlList controlList(self.controls());\n> @@ -148,6 +164,71 @@ PYBIND11_MODULE(_libcamera, m)\n> return 0;\n> })\n> \n> + .def_property(\"request_completed\",\n> + [](Camera &self) {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + return cm->getRequestCompleted(&self);\n> + },\n> + [](Camera &self, std::function, Request *)> f) {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + cm->setRequestCompleted(&self, f);\n> +\n> + /*\n> + * Note: We do not subscribe requestComplete here, as we\n> + * do that in the start() method.\n> + */\n> + })\n\nOk - so this shows why they all need distince getters and setters ...\n\n\n> +\n> + .def_property(\"buffer_completed\",\n> + [](Camera &self) -> std::function, Request *, FrameBuffer *)> {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + return cm->getBufferCompleted(&self);\n> + },\n> + [](Camera &self, std::function, Request *, FrameBuffer *)> f) {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + cm->setBufferCompleted(&self, f);\n> +\n> + self.bufferCompleted.disconnect();\n\nCan a comment explain why we're disconnecting this bufferCompleted? It's\nnot obvious to me why it's there ... Is it an internal bufferCompleted\nhandler for some specificness that I haven't seen yet?\n\n\n> +\n> + if (!f)\n> + return;\n> +\n> + self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n> + cm->handleBufferCompleted(camera, req, fb);\n> + });\n> + })\n> +\n> + .def_property(\"disconnected\",\n> + [](Camera &self) -> std::function)> {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + return cm->getDisconnected(&self);\n> + },\n> + [](Camera &self, std::function)> f) {\n> + auto cm = gCameraManager.lock();\n> + assert(cm);\n> +\n> + cm->setDisconnected(&self, f);\n> +\n> + self.disconnected.disconnect();\n\nSame here. Are there internal events just to be able to run by default?\n\n> +\n> + if (!f)\n> + return;\n\nShould the self.disconnected be reconnected if f was null?\n\n> +\n> + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n> + cm->handleDisconnected(camera);\n> + });\n> + })\n> +\n> .def(\"__str__\", [](Camera &self) {\n> return \"\";\n> })\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 48097BE173\n\tfor ;\n\tFri, 24 Jun 2022 10:53:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DF7465635;\n\tFri, 24 Jun 2022 12:53:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C3A9600EC\n\tfor ;\n\tFri, 24 Jun 2022 12:53:46 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72864DD;\n\tFri, 24 Jun 2022 12:53:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656068026;\n\tbh=SWI9GTlMnQuPNvMhGQ8zNoiJOeDOYYp9ukI0VpPglQc=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=0EMnBfHAyYMAwtjrSYSKNLILAa7ZzkBZQZrukMcsozfs0n8ZQD7Hd+hY27qBpONlg\n\t+PYCIKE//Qz5nM80SQUw45P54S5vargwk1KUVDf+bafhIFE/SKdmgYu1Ht/ti0Fhw/\n\trufDuXDUB2mfn930470FcdztnGVgLk0NXKgmYlrrlLzzI3HY0UDWWInKvPU77sEsXz\n\tx6pF4OYM44j9X1A/suE1Ay8HTqels5j/JFC+QPEt8IT+he9CV+67lSa3kqhAnSdliG\n\t+6OQyjmcwNcvqbOmfKrRRM56GI+nPvRs20ER1ROJ2yd+fDaDHalnL4RJyHoG2+tmoa\n\tzkmID9Ex/d9pA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656068025;\n\tbh=SWI9GTlMnQuPNvMhGQ8zNoiJOeDOYYp9ukI0VpPglQc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZULkVIrWZys11tD1mvypj0pVKn7CCvT0sH6btoyXloafhxKRb3JxHTGU694SkrtWx\n\tJf+O1pJv25Lp51AmX5zrGam3wQdZZFZqRxBueIUcYrbhfhc5O1yBbh5xCMas+P3VzW\n\t1lQFuhvwHRlz7dlbD3RFmX8pf1EJkImfY3xUBqRg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZULkVIrW\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>","To":"David Plowman ,\n\tJacopo Mondi ,\n\tLaurent Pinchart ,\n\tTomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 24 Jun 2022 11:53:43 +0100","Message-ID":"<165606802330.1149771.2821643337790848162@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23580,"web_url":"https://patchwork.libcamera.org/comment/23580/","msgid":"","date":"2022-06-24T14:44:21","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n> > At the moment the Python bindings only handle the requestCompleted\n> > events. But we have others, bufferCompleted and disconnec from the\n> \n> disconnec/disconnect\n> \n> > Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> > \n> > This makes all those events available to the users.\n> \n> \\o/\n> \n> > The get_ready_requests() method is now deprecated, but available to keep\n> > backward compatibility.\n\nI'd rather drop it. We're nowhere close to guaranteeing any API in the\nPython bindings.\n\n> Aha, once again - answering my questions in the previous patch ;-)\n> Maybe I need to read patch series backwards ... hehe.\n> \n> > The new event handling works as follows:\n> > \n> > The user sets callbacks to the CameraManager or Cameras (e.g.\n> > Camera.buffer_completed). When the event_fd informs of an event, the\n> > user must call CameraManager.dispatch_events() which gets the queued\n> > events and calls the relevant callbacks for each queued event.\n> > \n> > Additionally there is CameraManager.discard_events() if the user does\n> > not want to process the events but wants to clear the event queue (e.g.\n> > after stopping the cameras or when exiting the app).\n\nWhat is this for ? If the user doesn't set any handler for a specific\nevent type, I would expect the corresponding events to be discarded.\nAnything else will be passed to the event handlers. If it's about\nensuring that the event queue is cleared after stop() if the use doesn't\ncall dispatch_events(), I'd rather clear the queue at stop() time, and\npossible even call dispatch_events() in stop().\n\n> Hrm... in C++ lifetimes, the end of stop() means everything internally\n> is released, so I'm still weary that if we change that it could be\n> problematic (i.e. if we have to 'rely' on an application doing clean\n> up).\n> \n> > I'm not very happy with this patch. It works fine, but there's a lot of\n> > repetition of almost-the-same code. Perhaps some template magics might\n> > reduce the repetition, but I also fear that it can easily make the code\n> > more difficult to read.\n> > \n> > TODO: Documentation.\n> > \n> > Signed-off-by: Tomi Valkeinen \n> > ---\n> > src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--\n> > src/py/libcamera/py_camera_manager.h | 41 +++-\n> > src/py/libcamera/py_main.cpp | 87 +++++++-\n> > 3 files changed, 397 insertions(+), 22 deletions(-)\n> > \n> > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> > index ba45f713..bbadb9ee 100644\n> > --- a/src/py/libcamera/py_camera_manager.cpp\n> > +++ b/src/py/libcamera/py_camera_manager.cpp\n> > @@ -15,6 +15,37 @@ namespace py = pybind11;\n> > \n> > using namespace libcamera;\n> > \n> > +struct CameraEvent {\n\nI'd make this a class.\n\n> > + enum class EventType {\n\nYou can s/EventType/Type/ as you'll need to write CameraEvent::Type\nanyway.\n\n> > + Undefined = 0,\n> > + CameraAdded,\n> > + CameraRemoved,\n> > + Disconnect,\n> > + RequestCompleted,\n> > + BufferCompleted,\n> > + };\n> > +\n> > + CameraEvent(EventType type)\n> > + : type(type)\n\nNo variable shadowing warning ? Ah, we disable them. It would still be\ngood to avoid any possible issue here, and use type_ for the member.\n\n> > + {\n> > + }\n> > +\n> > + EventType type;\n> > +\n> > + std::shared_ptr cam;\n> > +\n> > + union {\n> > + struct {\n> > + Request *req;\n> > + FrameBuffer *fb;\n> > + } buf_completed;\n> > +\n> > + struct {\n> > + Request *req;\n> > + } req_completed;\n> > + };\n\nAs this stores pointers only, I think you could simplify it to just\n\n\tRequest *request_;\n\tFrameBuffer *fb_;\n\nand initialize the two pointers to nullptr in the constructor, to more\neasily catch bugs.\n\n> > +};\n> > +\n> > PyCameraManager::PyCameraManager()\n> > {\n> > int fd = eventfd(0, 0);\n> > @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()\n> > return l;\n> > }\n> > \n> > +/* DEPRECATED */\n\nLet's drop it :-)\n\n> > std::vector PyCameraManager::getReadyRequests(bool nonBlocking)\n> > {\n> > if (!nonBlocking || hasEvents())\n> > readFd();\n> > \n> > - std::vector v;\n> > - getRequests(v);\n> > + std::vector v;\n> > + getEvents(v);\n> > \n> > std::vector ret;\n> > \n> > - for (Request *req : v) {\n> > - py::object o = py::cast(req);\n> > - /* Decrease the ref increased in Camera.queue_request() */\n> > - o.dec_ref();\n> > - ret.push_back(o);\n> > + for (const auto &ev : v) {\n> > + switch (ev.type) {\n> > + case CameraEvent::EventType::RequestCompleted: {\n> > + Request *req = ev.req_completed.req;\n> > + py::object o = py::cast(req);\n> > + /* Decrease the ref increased in Camera.queue_request() */\n> > + o.dec_ref();\n> > + ret.push_back(o);\n> > + }\n> > + default:\n> > + /* ignore */\n> \n> Does this mean that events will get lost if the deprecated call gets\n> used? (Ok, so it's deprecated, but will it cause 'bugs' ?)\n> \n> \n> > + break;\n> > + }\n> > }\n> > \n> > return ret;\n> > }\n> > \n> > /* Note: Called from another thread */\n> > -void PyCameraManager::handleRequestCompleted(Request *req)\n> > +void PyCameraManager::handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb)\n> > +{\n> > + CameraEvent ev(CameraEvent::EventType::BufferCompleted);\n> > + ev.cam = cam;\n> > + ev.buf_completed.req = req;\n> > + ev.buf_completed.fb = fb;\n> > +\n> > + pushEvent(ev);\n> > + writeFd();\n> \n> Should writeFd be part of pushEvent? then there should be one entry on\n> the poll Fd for every event right? And forces them to be kept in sync...\n\nSounds like a good idea.\n\n> > +}\n> > +\n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> > +{\n> > + CameraEvent ev(CameraEvent::EventType::RequestCompleted);\n> > + ev.cam = cam;\n> > + ev.req_completed.req = req;\n> > +\n> > + pushEvent(ev);\n> > + writeFd();\n> > +}\n> > +\n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> > {\n> > - pushRequest(req);\n> > + CameraEvent ev(CameraEvent::EventType::Disconnect);\n> > + ev.cam = cam;\n> > +\n> > + pushEvent(ev);\n> > writeFd();\n> > }\n> > \n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> > +{\n> > + CameraEvent ev(CameraEvent::EventType::CameraAdded);\n> > + ev.cam = cam;\n> > +\n> > + pushEvent(ev);\n> > + writeFd();\n> > +}\n> > +\n> > +/* Note: Called from another thread */\n> > +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> > +{\n> > + CameraEvent ev(CameraEvent::EventType::CameraRemoved);\n> > + ev.cam = cam;\n> > +\n> > + pushEvent(ev);\n> > + writeFd();\n> > +}\n> \n> Is that what you mean about code duplication? I think it's fine.\n> \n> These could be templated out to specify the event type I guess ... but\n> that might be more pain. Especially when the request completed has\n> different parameters anyway.\n\nYes, generalizing this is probably not really worth it.\n\n> > +\n> > +void PyCameraManager::dispatchEvents(bool nonBlocking)\n\nCan we make this unconditionally non-blocking ?\n\n> > +{\n> > + if (!nonBlocking || hasEvents())\n> > + readFd();\n> > +\n> > + std::vector v;\n\ns/v/events/\n\n> > + getEvents(v);\n\n\tstd::vector events = getEvents();\n\n> > +\n> > + LOG(Python, Debug) << \"Dispatch \" << v.size() << \" events\";\n> > +\n> > + for (const auto &ev : v) {\n\ns/ev/event/\n\n> > + switch (ev.type) {\n> > + case CameraEvent::EventType::CameraAdded: {\n> > + std::shared_ptr cam = ev.cam;\n\nAs there's always a camera in all events, you could move it before the\nswitch.\n\n> > +\n> > + if (cameraAddedHandler_)\n> > + cameraAddedHandler_(cam);\n> > +\n> > + break;\n> > + }\n> > + case CameraEvent::EventType::CameraRemoved: {\n> > + std::shared_ptr cam = ev.cam;\n> > +\n> > + if (cameraRemovedHandler_)\n> > + cameraRemovedHandler_(cam);\n> > +\n> > + break;\n> > + }\n> > + case CameraEvent::EventType::BufferCompleted: {\n> > + std::shared_ptr cam = ev.cam;\n> > +\n> > + auto cb = getBufferCompleted(cam.get());\n> > +\n\nExtra blank line. Same below.\n\n> > + if (cb)\n> > + cb(cam, ev.buf_completed.req, ev.buf_completed.fb);\n> > +\n> > + break;\n> > + }\n> > + case CameraEvent::EventType::RequestCompleted: {\n> > + std::shared_ptr cam = ev.cam;\n> > +\n> > + auto cb = getRequestCompleted(cam.get());\n> > +\n> > + if (cb)\n> > + cb(cam, ev.req_completed.req);\n> > +\n> > + /* Decrease the ref increased in Camera.queue_request() */\n> > + py::object o = py::cast(ev.req_completed.req);\n> > + o.dec_ref();\n> > +\n> > + break;\n> > + }\n> > + case CameraEvent::EventType::Disconnect: {\n> > + std::shared_ptr cam = ev.cam;\n> > +\n> > + auto cb = getDisconnected(cam.get());\n> > +\n> > + if (cb)\n> > + cb(cam);\n> > +\n> > + break;\n> > + }\n> > + default:\n> > + assert(false);\n> \n> Can this be a python throw to generate a backtrace? Or a LOG(Python,\n> Fatal)? \n> \n> A plain assert isn't very informative.\n> \n> > + }\n> > + }\n> > +}\n> > +\n> > +void PyCameraManager::discardEvents()\n> > +{\n> > + if (hasEvents())\n> > + readFd();\n> > +\n> > + std::vector v;\n> > + getEvents(v);\n> > +\n> > + LOG(Python, Debug) << \"Discard \" << v.size() << \" events\";\n> > +\n> > + for (const auto &ev : v) {\n> > + if (ev.type != CameraEvent::EventType::RequestCompleted)\n> > + continue;\n> > +\n> > + std::shared_ptr cam = ev.cam;\n> > +\n> > + /* Decrease the ref increased in Camera.queue_request() */\n> > + py::object o = py::cast(ev.req_completed.req);\n> > + o.dec_ref();\n> > + }\n> > +}\n> > +\n> > +std::function)> PyCameraManager::getCameraAdded() const\n> > +{\n> > + return cameraAddedHandler_;\n> > +}\n> > +\n> > +void PyCameraManager::setCameraAdded(std::function)> fun)\n> > +{\n> > + if (cameraAddedHandler_)\n> > + cameraAdded.disconnect();\n> > +\n> > + cameraAddedHandler_ = fun;\n> > +\n> > + if (fun)\n> \n> Really trivially, but I'd call fun fn or func. Otherwise I read this as\n> \"If you're having fun... connect the signal\" ;-)\n\nI'd go for func, we use that already in other places.\n\n> > + cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n> > +}\n> > +\n> > +std::function)> PyCameraManager::getCameraRemoved() const\n> > +{\n> > + return cameraRemovedHandler_;\n> > +}\n> > +\n> > +void PyCameraManager::setCameraRemoved(std::function)> fun)\n> > +{\n> > + if (cameraRemovedHandler_)\n> > + cameraRemoved.disconnect();\n> > +\n> > + cameraRemovedHandler_ = fun;\n> > +\n> > + if (fun)\n> > + cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n> > +}\n> > +\n> > +std::function, Request *)> PyCameraManager::getRequestCompleted(Camera *cam)\n> > +{\n> > + if (auto it = cameraRequestCompletedHandlers_.find(cam);\n> > + it != cameraRequestCompletedHandlers_.end())\n> > + return it->second;\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > +void PyCameraManager::setRequestCompleted(Camera *cam, std::function, Request *)> fun)\n> > +{\n> > + if (fun)\n> > + cameraRequestCompletedHandlers_[cam] = fun;\n> > + else\n> > + cameraRequestCompletedHandlers_.erase(cam);\n> > +}\n> > +\n> > +std::function, Request *, FrameBuffer *)> PyCameraManager::getBufferCompleted(Camera *cam)\n> > +{\n> > + if (auto it = cameraBufferCompletedHandlers_.find(cam);\n> > + it != cameraBufferCompletedHandlers_.end())\n> > + return it->second;\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > +void PyCameraManager::setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun)\n> > +{\n> > + if (fun)\n> > + cameraBufferCompletedHandlers_[cam] = fun;\n> > + else\n> > + cameraBufferCompletedHandlers_.erase(cam);\n> > +}\n> > +\n> > +std::function)> PyCameraManager::getDisconnected(Camera *cam)\n> > +{\n> > + if (auto it = cameraDisconnectHandlers_.find(cam);\n> > + it != cameraDisconnectHandlers_.end())\n> > + return it->second;\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > +void PyCameraManager::setDisconnected(Camera *cam, std::function)> fun)\n> > +{\n> > + if (fun)\n> > + cameraDisconnectHandlers_[cam] = fun;\n> > + else\n> > + cameraDisconnectHandlers_.erase(cam);\n> > +}\n> > +\n> \n> Yes, I see, lots more boilerplate code duplication. Not a problem in\n> it's own right, and if it's templateable then maybe that can be helpful.\n> \n> > void PyCameraManager::writeFd()\n> > {\n> > uint64_t v = 1;\n> > @@ -104,16 +363,18 @@ void PyCameraManager::readFd()\n> > throw std::system_error(errno, std::generic_category());\n> > }\n> > \n> > -void PyCameraManager::pushRequest(Request *req)\n> > +void PyCameraManager::pushEvent(const CameraEvent &ev)\n> > {\n> > - std::lock_guard guard(reqlist_mutex_);\n> > - reqList_.push_back(req);\n> > + std::lock_guard guard(reqlistMutex_);\n> > + cameraEvents_.push_back(ev);\n> > +\n> > + LOG(Python, Debug) << \"Queued events: \" << cameraEvents_.size();\n> > }\n> > \n> > -void PyCameraManager::getRequests(std::vector &v)\n> > +void PyCameraManager::getEvents(std::vector &v)\n> > {\n> > - std::lock_guard guard(reqlist_mutex_);\n> > - swap(v, reqList_);\n> > + std::lock_guard guard(reqlistMutex_);\n> \n> Is this now supposed to be an eventListMutex_?\n> \n> > + swap(v, cameraEvents_);\n> > }\n> > \n> > bool PyCameraManager::hasEvents()\n> > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> > index 2396d236..fd28291b 100644\n> > --- a/src/py/libcamera/py_camera_manager.h\n> > +++ b/src/py/libcamera/py_camera_manager.h\n> > @@ -13,6 +13,8 @@\n> > \n> > using namespace libcamera;\n> > \n> > +struct CameraEvent;\n> > +\n> > class PyCameraManager : public CameraManager\n> > {\n> > public:\n> > @@ -27,15 +29,46 @@ public:\n> > \n> > void handleRequestCompleted(Request *req);\n> > \n> > + void handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n> > + void handleRequestCompleted(std::shared_ptr cam, Request *req);\n> > + void handleDisconnected(std::shared_ptr cam);\n> > + void handleCameraAdded(std::shared_ptr cam);\n> > + void handleCameraRemoved(std::shared_ptr cam);\n> \n> In libcamera, we don't prefix event handlers with 'handle'. i.e.\n> handleCameraRemoved would just be cameraRemoved.\n> \n> Do you prefer it to make it distinct? or is it possible to align with\n> the existing styles?\n> \n> > +\n> > + void dispatchEvents(bool nonBlocking = false);\n> > + void discardEvents();\n> > +\n> > + std::function)> getCameraAdded() const;\n> > + void setCameraAdded(std::function)> fun);\n> > +\n> > + std::function)> getCameraRemoved() const;\n> > + void setCameraRemoved(std::function)> fun);\n> > +\n> > + std::function, Request *)> getRequestCompleted(Camera *cam);\n> > + void setRequestCompleted(Camera *cam, std::function, Request *)> fun);\n> > +\n> > + std::function, Request *, FrameBuffer *)> getBufferCompleted(Camera *cam);\n> > + void setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun);\n> > +\n> > + std::function)> getDisconnected(Camera *cam);\n> > + void setDisconnected(Camera *cam, std::function)> fun);\n> \n> so many getters and setters. Can the underlying events be more directly\n> accessibly or do they have to have the setters?\n\nIt would be nice to clean this up a bit indeed if possible. Nothing\nurgent though.\n\n> > +\n> > private:\n> > int eventFd_ = -1;\n> > - std::mutex reqlist_mutex_;\n> > + std::mutex reqlistMutex_;\n> > std::vector reqList_;\n> > + std::vector cameraEvents_;\n> > +\n> > + std::function)> cameraAddedHandler_;\n> > + std::function)> cameraRemovedHandler_;\n> > +\n> > + std::map, Request *, FrameBuffer *)>> cameraBufferCompletedHandlers_;\n> > + std::map, Request *)>> cameraRequestCompletedHandlers_;\n> > + std::map)>> cameraDisconnectHandlers_;\n> > \n> > void writeFd();\n> > void readFd();\n> > - void pushRequest(Request *req);\n> > - void getRequests(std::vector &v);\n> > -\n> > + void pushEvent(const CameraEvent &ev);\n> > + void getEvents(std::vector &v);\n> > bool hasEvents();\n> > };\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index ee4ecb9b..b6fd9a9d 100644\n> > --- a/src/py/libcamera/py_main.cpp\n> > +++ b/src/py/libcamera/py_main.cpp\n> > @@ -103,8 +103,20 @@ PYBIND11_MODULE(_libcamera, m)\n> > .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n> > \n> > .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> > + /* DEPRECATED */\n> > .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests,\n> > - py::arg(\"nonblocking\") = false);\n> > + py::arg(\"nonblocking\") = false)\n> > + .def(\"dispatch_events\", &PyCameraManager::dispatchEvents,\n> > + py::arg(\"nonblocking\") = false)\n> \n> Why are there newlines between camera_added and camera_removed, but not\n> between get_ready_requests, dispatch_events, and discard_events? Is it\n> grouping events? or should these be separated by newlines too?\n> \n> > + .def(\"discard_events\", &PyCameraManager::discardEvents)\n> > +\n> > + .def_property(\"camera_added\",\n> > + &PyCameraManager::getCameraAdded,\n> > + &PyCameraManager::setCameraAdded)\n> > +\n> > + .def_property(\"camera_removed\",\n> > + &PyCameraManager::getCameraRemoved,\n> > + &PyCameraManager::setCameraRemoved);\n> \n> For these chained code styles, if the ; was on a new line by\n> itself, it wouldn't have to modify the previous property to add a new\n> one?\n> \n> Is that a good thing or worse code style for you ?\n> \n> > pyCamera\n> > .def_property_readonly(\"id\", &Camera::id)\n> > @@ -117,9 +129,13 @@ PYBIND11_MODULE(_libcamera, m)\n> > auto cm = gCameraManager.lock();\n> > assert(cm);\n> > \n> > - self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n> > + /*\n> > + * Note: We always subscribe requestComplete, as the bindings\n> > + * use requestComplete event to decrement the Request refcount-\n> > + */\n> \n> Interesting, so there are multiple subscribers to the event? Is it still\n> guaranteed to be referenced while the user code has the request? (I\n> assume so, and that this is managing the internal refcnts while the\n> object is inside libcamera).\n> \n> \n> If the ordering is required, then this layer could subscribe to the\n> event, and the be responsible for calling the event on the user code and\n> cleaning up after it completes.\n> \n> But if it already is fine, then it's a nice example of how multiple\n> functions can run on the event handler.\n> \n> > \n> > - cm->handleRequestCompleted(req);\n> > + self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n> > + cm->handleRequestCompleted(camera, req);\n> > });\n> > \n> > ControlList controlList(self.controls());\n> > @@ -148,6 +164,71 @@ PYBIND11_MODULE(_libcamera, m)\n> > return 0;\n> > })\n> > \n> > + .def_property(\"request_completed\",\n> > + [](Camera &self) {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + return cm->getRequestCompleted(&self);\n> > + },\n> > + [](Camera &self, std::function, Request *)> f) {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + cm->setRequestCompleted(&self, f);\n> > +\n> > + /*\n> > + * Note: We do not subscribe requestComplete here, as we\n> > + * do that in the start() method.\n> > + */\n> > + })\n> \n> Ok - so this shows why they all need distince getters and setters ...\n> \n> > +\n> > + .def_property(\"buffer_completed\",\n> > + [](Camera &self) -> std::function, Request *, FrameBuffer *)> {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + return cm->getBufferCompleted(&self);\n> > + },\n> > + [](Camera &self, std::function, Request *, FrameBuffer *)> f) {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + cm->setBufferCompleted(&self, f);\n> > +\n> > + self.bufferCompleted.disconnect();\n> \n> Can a comment explain why we're disconnecting this bufferCompleted? It's\n> not obvious to me why it's there ... Is it an internal bufferCompleted\n> handler for some specificness that I haven't seen yet?\n> \n> \n> > +\n> > + if (!f)\n> > + return;\n> > +\n> > + self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n> > + cm->handleBufferCompleted(camera, req, fb);\n> > + });\n> > + })\n> > +\n> > + .def_property(\"disconnected\",\n> > + [](Camera &self) -> std::function)> {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + return cm->getDisconnected(&self);\n> > + },\n> > + [](Camera &self, std::function)> f) {\n> > + auto cm = gCameraManager.lock();\n> > + assert(cm);\n> > +\n> > + cm->setDisconnected(&self, f);\n> > +\n> > + self.disconnected.disconnect();\n> \n> Same here. Are there internal events just to be able to run by default?\n> \n> > +\n> > + if (!f)\n> > + return;\n> \n> Should the self.disconnected be reconnected if f was null?\n> \n> > +\n> > + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n> > + cm->handleDisconnected(camera);\n> > + });\n> > + })\n> > +\n> > .def(\"__str__\", [](Camera &self) {\n> > return \"\";\n> > })","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C08B2BE173\n\tfor ;\n\tFri, 24 Jun 2022 14:44:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D2F665638;\n\tFri, 24 Jun 2022 16:44:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9729460412\n\tfor ;\n\tFri, 24 Jun 2022 16:44:38 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C522847C;\n\tFri, 24 Jun 2022 16:44:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656081880;\n\tbh=/KMdN78ej5xCGt8pyM/fniuCfySljbiKKiANYOehLH8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=m2y61iYZrNBdfm3Qr2cljvyQmI+yCLy48N6Pqaopw8A3nbI+iJhb/WxjURK2CgHZa\n\thS6GXKlM8QwfecLu+py5vr7KORSyBYmeHCg8bZyO3TaYtygAnGP6idxsoIFBvMWP2i\n\t9TB4Bqr/s1OoP6GDDDWjNdlx467s2LnYEXDf8oRe5VafhlSLeZ+CbM3rB5WX7nwD6T\n\txKMGh6jnRymhUnj5711Vvv9DdBEMiqLOy3MMWZ+1r5PyyEsrGvzERrZZyTq/OBep/S\n\tujZI7R0WIiUqVNj+oX7seE7wAGf0mRZcGMBGfPraWSA2WDy6X9/7pvp040Zah0x0Yc\n\toUzZgbjm0Pq2A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656081878;\n\tbh=/KMdN78ej5xCGt8pyM/fniuCfySljbiKKiANYOehLH8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E67tznPcHyGdPyS5c/xMlJPayd2uaJOeuGU224J07eZcJim0LvZaE6lRUjPvxx0JA\n\tyGlh4oEcUiatHhNRidKk8qPBZ3r91HD/t1TS7NNQPf+oDizenW6WdScz+3i8FATVR5\n\tVEFO5G48iB0vIXuJ8ZfJDks04AHPCSLDrKaDWR+A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E67tznPc\"; dkim-atps=neutral","Date":"Fri, 24 Jun 2022 17:44:21 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>\n\t<165606802330.1149771.2821643337790848162@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165606802330.1149771.2821643337790848162@Monstersaurus>","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23600,"web_url":"https://patchwork.libcamera.org/comment/23600/","msgid":"","date":"2022-06-27T10:26:48","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 24/06/2022 13:53, Kieran Bingham wrote:\n> Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n>> At the moment the Python bindings only handle the requestCompleted\n>> events. But we have others, bufferCompleted and disconnec from the\n> \n> disconnec/disconnect\n> \n>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n>>\n>> This makes all those events available to the users.\n> \n> \\o/\n> \n> \n>> The get_ready_requests() method is now deprecated, but available to keep\n>> backward compatibility.\n> \n> Aha, once again - answering my questions in the previous patch ;-)\n> Maybe I need to read patch series backwards ... hehe.\n> \n> \n>> The new event handling works as follows:\n>>\n>> The user sets callbacks to the CameraManager or Cameras (e.g.\n>> Camera.buffer_completed). When the event_fd informs of an event, the\n>> user must call CameraManager.dispatch_events() which gets the queued\n>> events and calls the relevant callbacks for each queued event.\n>>\n>> Additionally there is CameraManager.discard_events() if the user does\n>> not want to process the events but wants to clear the event queue (e.g.\n>> after stopping the cameras or when exiting the app).\n> \n> Hrm... in C++ lifetimes, the end of stop() means everything internally\n> is released, so I'm still weary that if we change that it could be\n> problematic (i.e. if we have to 'rely' on an application doing clean\n> up).\n> \n> \n>> I'm not very happy with this patch. It works fine, but there's a lot of\n>> repetition of almost-the-same code. Perhaps some template magics might\n>> reduce the repetition, but I also fear that it can easily make the code\n>> more difficult to read.\n>>\n>> TODO: Documentation.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--\n>> src/py/libcamera/py_camera_manager.h | 41 +++-\n>> src/py/libcamera/py_main.cpp | 87 +++++++-\n>> 3 files changed, 397 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>> index ba45f713..bbadb9ee 100644\n>> --- a/src/py/libcamera/py_camera_manager.cpp\n>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>> @@ -15,6 +15,37 @@ namespace py = pybind11;\n>> \n>> using namespace libcamera;\n>> \n>> +struct CameraEvent {\n>> + enum class EventType {\n>> + Undefined = 0,\n>> + CameraAdded,\n>> + CameraRemoved,\n>> + Disconnect,\n>> + RequestCompleted,\n>> + BufferCompleted,\n>> + };\n>> +\n>> + CameraEvent(EventType type)\n>> + : type(type)\n>> + {\n>> + }\n>> +\n>> + EventType type;\n>> +\n>> + std::shared_ptr cam;\n>> +\n>> + union {\n>> + struct {\n>> + Request *req;\n>> + FrameBuffer *fb;\n>> + } buf_completed;\n>> +\n>> + struct {\n>> + Request *req;\n>> + } req_completed;\n>> + };\n>> +};\n>> +\n>> PyCameraManager::PyCameraManager()\n>> {\n>> int fd = eventfd(0, 0);\n>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()\n>> return l;\n>> }\n>> \n>> +/* DEPRECATED */\n>> std::vector PyCameraManager::getReadyRequests(bool nonBlocking)\n>> {\n>> if (!nonBlocking || hasEvents())\n>> readFd();\n>> \n>> - std::vector v;\n>> - getRequests(v);\n>> + std::vector v;\n>> + getEvents(v);\n>> \n>> std::vector ret;\n>> \n>> - for (Request *req : v) {\n>> - py::object o = py::cast(req);\n>> - /* Decrease the ref increased in Camera.queue_request() */\n>> - o.dec_ref();\n>> - ret.push_back(o);\n>> + for (const auto &ev : v) {\n>> + switch (ev.type) {\n>> + case CameraEvent::EventType::RequestCompleted: {\n>> + Request *req = ev.req_completed.req;\n>> + py::object o = py::cast(req);\n>> + /* Decrease the ref increased in Camera.queue_request() */\n>> + o.dec_ref();\n>> + ret.push_back(o);\n>> + }\n>> + default:\n>> + /* ignore */\n> \n> Does this mean that events will get lost if the deprecated call gets\n> used? (Ok, so it's deprecated, but will it cause 'bugs' ?)\n\nLost in what way? They are lost in the sense that the app won't see \nthem, but that's to be expected as the \"legacy\" app is not aware of such \na feature.\n\n> \n>> + break;\n>> + }\n>> }\n>> \n>> return ret;\n>> }\n>> \n>> /* Note: Called from another thread */\n>> -void PyCameraManager::handleRequestCompleted(Request *req)\n>> +void PyCameraManager::handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb)\n>> +{\n>> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);\n>> + ev.cam = cam;\n>> + ev.buf_completed.req = req;\n>> + ev.buf_completed.fb = fb;\n>> +\n>> + pushEvent(ev);\n>> + writeFd();\n> \n> Should writeFd be part of pushEvent? then there should be one entry on\n> the poll Fd for every event right? And forces them to be kept in sync...\n\nMaybe, but the same way as writeFd() and pushEvent() are a pair, the \nreadFd() and getEvents() is a pair. But for the latter pair, readFd() is \nnot always called, so readFd() is separate. Thus I wanted to keep \nwriteFd() separate too.\n\nSo, they could be merged. Let's see how this evolves...\n\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n>> +{\n>> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);\n>> + ev.cam = cam;\n>> + ev.req_completed.req = req;\n>> +\n>> + pushEvent(ev);\n>> + writeFd();\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n>> {\n>> - pushRequest(req);\n>> + CameraEvent ev(CameraEvent::EventType::Disconnect);\n>> + ev.cam = cam;\n>> +\n>> + pushEvent(ev);\n>> writeFd();\n>> }\n>> \n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n>> +{\n>> + CameraEvent ev(CameraEvent::EventType::CameraAdded);\n>> + ev.cam = cam;\n>> +\n>> + pushEvent(ev);\n>> + writeFd();\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n>> +{\n>> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);\n>> + ev.cam = cam;\n>> +\n>> + pushEvent(ev);\n>> + writeFd();\n>> +}\n> \n> Is that what you mean about code duplication? I think it's fine.\n> \n> These could be templated out to specify the event type I guess ... but\n> that might be more pain. Especially when the request completed has\n> different parameters anyway.\n> \n>> +\n>> +void PyCameraManager::dispatchEvents(bool nonBlocking)\n>> +{\n>> + if (!nonBlocking || hasEvents())\n>> + readFd();\n>> +\n>> + std::vector v;\n>> + getEvents(v);\n>> +\n>> + LOG(Python, Debug) << \"Dispatch \" << v.size() << \" events\";\n>> +\n>> + for (const auto &ev : v) {\n>> + switch (ev.type) {\n>> + case CameraEvent::EventType::CameraAdded: {\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + if (cameraAddedHandler_)\n>> + cameraAddedHandler_(cam);\n>> +\n>> + break;\n>> + }\n>> + case CameraEvent::EventType::CameraRemoved: {\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + if (cameraRemovedHandler_)\n>> + cameraRemovedHandler_(cam);\n>> +\n>> + break;\n>> + }\n>> + case CameraEvent::EventType::BufferCompleted: {\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + auto cb = getBufferCompleted(cam.get());\n>> +\n>> + if (cb)\n>> + cb(cam, ev.buf_completed.req, ev.buf_completed.fb);\n>> +\n>> + break;\n>> + }\n>> + case CameraEvent::EventType::RequestCompleted: {\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + auto cb = getRequestCompleted(cam.get());\n>> +\n>> + if (cb)\n>> + cb(cam, ev.req_completed.req);\n>> +\n>> + /* Decrease the ref increased in Camera.queue_request() */\n>> + py::object o = py::cast(ev.req_completed.req);\n>> + o.dec_ref();\n>> +\n>> + break;\n>> + }\n>> + case CameraEvent::EventType::Disconnect: {\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + auto cb = getDisconnected(cam.get());\n>> +\n>> + if (cb)\n>> + cb(cam);\n>> +\n>> + break;\n>> + }\n>> + default:\n>> + assert(false);\n> \n> Can this be a python throw to generate a backtrace? Or a LOG(Python,\n> Fatal)?\n> \n> A plain assert isn't very informative.\n\nI can change this (and I think there were a few more) asserts to log \nfatal. I'm not sure if throwing an exception is good if the error \nindicates to some kind of internal bug.\n\n>> + }\n>> + }\n>> +}\n>> +\n>> +void PyCameraManager::discardEvents()\n>> +{\n>> + if (hasEvents())\n>> + readFd();\n>> +\n>> + std::vector v;\n>> + getEvents(v);\n>> +\n>> + LOG(Python, Debug) << \"Discard \" << v.size() << \" events\";\n>> +\n>> + for (const auto &ev : v) {\n>> + if (ev.type != CameraEvent::EventType::RequestCompleted)\n>> + continue;\n>> +\n>> + std::shared_ptr cam = ev.cam;\n>> +\n>> + /* Decrease the ref increased in Camera.queue_request() */\n>> + py::object o = py::cast(ev.req_completed.req);\n>> + o.dec_ref();\n>> + }\n>> +}\n>> +\n>> +std::function)> PyCameraManager::getCameraAdded() const\n>> +{\n>> + return cameraAddedHandler_;\n>> +}\n>> +\n>> +void PyCameraManager::setCameraAdded(std::function)> fun)\n>> +{\n>> + if (cameraAddedHandler_)\n>> + cameraAdded.disconnect();\n>> +\n>> + cameraAddedHandler_ = fun;\n>> +\n>> + if (fun)\n> \n> Really trivially, but I'd call fun fn or func. Otherwise I read this as\n> \"If you're having fun... connect the signal\" ;-)\n\nSigh... Ok... =)\n\n> \n>> + cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n>> +}\n>> +\n>> +std::function)> PyCameraManager::getCameraRemoved() const\n>> +{\n>> + return cameraRemovedHandler_;\n>> +}\n>> +\n>> +void PyCameraManager::setCameraRemoved(std::function)> fun)\n>> +{\n>> + if (cameraRemovedHandler_)\n>> + cameraRemoved.disconnect();\n>> +\n>> + cameraRemovedHandler_ = fun;\n>> +\n>> + if (fun)\n>> + cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n>> +}\n>> +\n>> +std::function, Request *)> PyCameraManager::getRequestCompleted(Camera *cam)\n>> +{\n>> + if (auto it = cameraRequestCompletedHandlers_.find(cam);\n>> + it != cameraRequestCompletedHandlers_.end())\n>> + return it->second;\n>> +\n>> + return nullptr;\n>> +}\n>> +\n>> +void PyCameraManager::setRequestCompleted(Camera *cam, std::function, Request *)> fun)\n>> +{\n>> + if (fun)\n>> + cameraRequestCompletedHandlers_[cam] = fun;\n>> + else\n>> + cameraRequestCompletedHandlers_.erase(cam);\n>> +}\n>> +\n>> +std::function, Request *, FrameBuffer *)> PyCameraManager::getBufferCompleted(Camera *cam)\n>> +{\n>> + if (auto it = cameraBufferCompletedHandlers_.find(cam);\n>> + it != cameraBufferCompletedHandlers_.end())\n>> + return it->second;\n>> +\n>> + return nullptr;\n>> +}\n>> +\n>> +void PyCameraManager::setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun)\n>> +{\n>> + if (fun)\n>> + cameraBufferCompletedHandlers_[cam] = fun;\n>> + else\n>> + cameraBufferCompletedHandlers_.erase(cam);\n>> +}\n>> +\n>> +std::function)> PyCameraManager::getDisconnected(Camera *cam)\n>> +{\n>> + if (auto it = cameraDisconnectHandlers_.find(cam);\n>> + it != cameraDisconnectHandlers_.end())\n>> + return it->second;\n>> +\n>> + return nullptr;\n>> +}\n>> +\n>> +void PyCameraManager::setDisconnected(Camera *cam, std::function)> fun)\n>> +{\n>> + if (fun)\n>> + cameraDisconnectHandlers_[cam] = fun;\n>> + else\n>> + cameraDisconnectHandlers_.erase(cam);\n>> +}\n>> +\n> \n> Yes, I see, lots more boilerplate code duplication. Not a problem in\n> it's own right, and if it's templateable then maybe that can be helpful.\n\nRight. The callback functions are different, but these are so similar \nthat something must be possible. But I fear templates will make the code \nmore difficult to read and maintain.\n\n>> void PyCameraManager::writeFd()\n>> {\n>> uint64_t v = 1;\n>> @@ -104,16 +363,18 @@ void PyCameraManager::readFd()\n>> throw std::system_error(errno, std::generic_category());\n>> }\n>> \n>> -void PyCameraManager::pushRequest(Request *req)\n>> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n>> {\n>> - std::lock_guard guard(reqlist_mutex_);\n>> - reqList_.push_back(req);\n>> + std::lock_guard guard(reqlistMutex_);\n>> + cameraEvents_.push_back(ev);\n>> +\n>> + LOG(Python, Debug) << \"Queued events: \" << cameraEvents_.size();\n>> }\n>> \n>> -void PyCameraManager::getRequests(std::vector &v)\n>> +void PyCameraManager::getEvents(std::vector &v)\n>> {\n>> - std::lock_guard guard(reqlist_mutex_);\n>> - swap(v, reqList_);\n>> + std::lock_guard guard(reqlistMutex_);\n> \n> Is this now supposed to be an eventListMutex_?\n\nYes. Or rather, cameraEventsMutex_\n\n> \n>> + swap(v, cameraEvents_);\n>> }\n>> \n>> bool PyCameraManager::hasEvents()\n>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n>> index 2396d236..fd28291b 100644\n>> --- a/src/py/libcamera/py_camera_manager.h\n>> +++ b/src/py/libcamera/py_camera_manager.h\n>> @@ -13,6 +13,8 @@\n>> \n>> using namespace libcamera;\n>> \n>> +struct CameraEvent;\n>> +\n>> class PyCameraManager : public CameraManager\n>> {\n>> public:\n>> @@ -27,15 +29,46 @@ public:\n>> \n>> void handleRequestCompleted(Request *req);\n>> \n>> + void handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n>> + void handleRequestCompleted(std::shared_ptr cam, Request *req);\n>> + void handleDisconnected(std::shared_ptr cam);\n>> + void handleCameraAdded(std::shared_ptr cam);\n>> + void handleCameraRemoved(std::shared_ptr cam);\n> \n> In libcamera, we don't prefix event handlers with 'handle'. i.e.\n> handleCameraRemoved would just be cameraRemoved.\n> \n> Do you prefer it to make it distinct? or is it possible to align with\n> the existing styles?\n\nWe inherit CameraManager, which already has cameraRemoved... And even if \nit didn't, isn't \"cameraRemoved\" a bad name for a function that handles \nan event? Shouldn't functions be about doing something?\n\nIf it's just for signals and their handlers, isn't it bad there too? A \nsignal would be \"cameraRemoved\" and the handler would be \n\"cameraRemoved\", right?\n\n>> +\n>> + void dispatchEvents(bool nonBlocking = false);\n>> + void discardEvents();\n>> +\n>> + std::function)> getCameraAdded() const;\n>> + void setCameraAdded(std::function)> fun);\n>> +\n>> + std::function)> getCameraRemoved() const;\n>> + void setCameraRemoved(std::function)> fun);\n>> +\n>> + std::function, Request *)> getRequestCompleted(Camera *cam);\n>> + void setRequestCompleted(Camera *cam, std::function, Request *)> fun);\n>> +\n>> + std::function, Request *, FrameBuffer *)> getBufferCompleted(Camera *cam);\n>> + void setBufferCompleted(Camera *cam, std::function, Request *, FrameBuffer *)> fun);\n>> +\n>> + std::function)> getDisconnected(Camera *cam);\n>> + void setDisconnected(Camera *cam, std::function)> fun);\n> \n> so many getters and setters. Can the underlying events be more directly\n> accessibly or do they have to have the setters?\n\nI felt it makes the main bindings, in py_main.cpp cleaner if we don't \nexpose data structures from the PyCameraManager and force the \npy_main.cpp to do the work with them.\n\n>> +\n>> private:\n>> int eventFd_ = -1;\n>> - std::mutex reqlist_mutex_;\n>> + std::mutex reqlistMutex_;\n>> std::vector reqList_;\n>> + std::vector cameraEvents_;\n>> +\n>> + std::function)> cameraAddedHandler_;\n>> + std::function)> cameraRemovedHandler_;\n>> +\n>> + std::map, Request *, FrameBuffer *)>> cameraBufferCompletedHandlers_;\n>> + std::map, Request *)>> cameraRequestCompletedHandlers_;\n>> + std::map)>> cameraDisconnectHandlers_;\n>> \n>> void writeFd();\n>> void readFd();\n>> - void pushRequest(Request *req);\n>> - void getRequests(std::vector &v);\n>> -\n>> + void pushEvent(const CameraEvent &ev);\n>> + void getEvents(std::vector &v);\n>> bool hasEvents();\n>> };\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index ee4ecb9b..b6fd9a9d 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -103,8 +103,20 @@ PYBIND11_MODULE(_libcamera, m)\n>> .def_property_readonly(\"cameras\", &PyCameraManager::getCameras)\n>> \n>> .def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n>> + /* DEPRECATED */\n>> .def(\"get_ready_requests\", &PyCameraManager::getReadyRequests,\n>> - py::arg(\"nonblocking\") = false);\n>> + py::arg(\"nonblocking\") = false)\n>> + .def(\"dispatch_events\", &PyCameraManager::dispatchEvents,\n>> + py::arg(\"nonblocking\") = false)\n> \n> Why are there newlines between camera_added and camera_removed, but not\n> between get_ready_requests, dispatch_events, and discard_events? Is it\n> grouping events? or should these be separated by newlines too?\n\nNo particular reason. I often try to group things, or separate things, \nbut it's not always quite consistent (if it even could be).\n\n> \n>> + .def(\"discard_events\", &PyCameraManager::discardEvents)\n>> +\n>> + .def_property(\"camera_added\",\n>> + &PyCameraManager::getCameraAdded,\n>> + &PyCameraManager::setCameraAdded)\n>> +\n>> + .def_property(\"camera_removed\",\n>> + &PyCameraManager::getCameraRemoved,\n>> + &PyCameraManager::setCameraRemoved);\n> \n> For these chained code styles, if the ; was on a new line by\n> itself, it wouldn't have to modify the previous property to add a new\n> one?\n> \n> Is that a good thing or worse code style for you ?\n\nThat's what I had originally, but I dropped it either because of a \nreview comment, or because clang-format very much wants to move the ; to \nthe end of the line. Or maybe a style checker warned about it.\n\n> \n>> pyCamera\n>> .def_property_readonly(\"id\", &Camera::id)\n>> @@ -117,9 +129,13 @@ PYBIND11_MODULE(_libcamera, m)\n>> auto cm = gCameraManager.lock();\n>> assert(cm);\n>> \n>> - self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {\n>> + /*\n>> + * Note: We always subscribe requestComplete, as the bindings\n>> + * use requestComplete event to decrement the Request refcount-\n>> + */\n> \n> Interesting, so there are multiple subscribers to the event? Is it still\n\nJust a single subscriber. The comment above describes why this one, \nrequestComplete, is different than the other events. For the other \nevents the bindings are just passing them through. But the bindings \nrequire requestComplete, so it's always implicitly subscribed.\n\nOr maybe you mean something else, but if so, I didn't get your point =).\n\n> guaranteed to be referenced while the user code has the request? (I\n> assume so, and that this is managing the internal refcnts while the\n> object is inside libcamera).\n> \n> \n> If the ordering is required, then this layer could subscribe to the\n> event, and the be responsible for calling the event on the user code and\n> cleaning up after it completes.\n> \n> But if it already is fine, then it's a nice example of how multiple\n> functions can run on the event handler.\n> \n>> \n>> - cm->handleRequestCompleted(req);\n>> + self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n>> + cm->handleRequestCompleted(camera, req);\n>> });\n>> \n>> ControlList controlList(self.controls());\n>> @@ -148,6 +164,71 @@ PYBIND11_MODULE(_libcamera, m)\n>> return 0;\n>> })\n>> \n>> + .def_property(\"request_completed\",\n>> + [](Camera &self) {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + return cm->getRequestCompleted(&self);\n>> + },\n>> + [](Camera &self, std::function, Request *)> f) {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + cm->setRequestCompleted(&self, f);\n>> +\n>> + /*\n>> + * Note: We do not subscribe requestComplete here, as we\n>> + * do that in the start() method.\n>> + */\n>> + })\n> \n> Ok - so this shows why they all need distince getters and setters ...\n> \n> \n>> +\n>> + .def_property(\"buffer_completed\",\n>> + [](Camera &self) -> std::function, Request *, FrameBuffer *)> {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + return cm->getBufferCompleted(&self);\n>> + },\n>> + [](Camera &self, std::function, Request *, FrameBuffer *)> f) {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + cm->setBufferCompleted(&self, f);\n>> +\n>> + self.bufferCompleted.disconnect();\n> \n> Can a comment explain why we're disconnecting this bufferCompleted? It's\n> not obvious to me why it's there ... Is it an internal bufferCompleted\n> handler for some specificness that I haven't seen yet?\n\nWhen the user sets the callback function, like\n\ncam.buffer_completed = foobar\n\nwe store the callback (cm->setBufferCompleted()), we then disconnect any \npreviously connected handler, and then connect the new handler (unless !f).\n\nSo the disconnect is just disconnecting (possibly) previously set handler.\n\nHowever, that signal handler lambda below is always the same (we don't \npass the 'f' from the user), so we don't actually need to reconnect if \nalready connected. So this could be optimized a bit by\n- disconnecting only if we have connected earlier and !f\n- connecting only if not connected earlier and f\n\nBut it may not be worth it, especially as we don't have the \"have we \nalready connected?\" available here.\n\n>> +\n>> + if (!f)\n>> + return;\n>> +\n>> + self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n>> + cm->handleBufferCompleted(camera, req, fb);\n>> + });\n>> + })\n>> +\n>> + .def_property(\"disconnected\",\n>> + [](Camera &self) -> std::function)> {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + return cm->getDisconnected(&self);\n>> + },\n>> + [](Camera &self, std::function)> f) {\n>> + auto cm = gCameraManager.lock();\n>> + assert(cm);\n>> +\n>> + cm->setDisconnected(&self, f);\n>> +\n>> + self.disconnected.disconnect();\n> \n> Same here. Are there internal events just to be able to run by default?\n\nI don't think I understand the question. What do you mean with internal \nevents? The requestCompleted is the only one that has special handling.\n\n>> +\n>> + if (!f)\n>> + return;\n> \n> Should the self.disconnected be reconnected if f was null?\n\nHmm, no... If the f is null, we don't want to see any disconnect events.\n\nMaybe the confusion comes from the layered signal handling.\n\nThe bindings connect the C++ signals to its internal handlers. These \nhandlers store the events. Later the bindings use stored Python callback \nfunctions to deliver the events.\n\nSo e.g.\n\ncm->setDisconnected(&self, f);\n\nabove stores the Python callback, it doesn't deal with the C++ \nsignals/handling in any way.\n\n>> +\n>> + self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n>> + cm->handleDisconnected(camera);\n>> + });\n>> + })\n>> +\n>> .def(\"__str__\", [](Camera &self) {\n>> return \"\";\n>> })\n>> -- \n>> 2.34.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AEB1EBE173\n\tfor ;\n\tMon, 27 Jun 2022 10:26:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5546665635;\n\tMon, 27 Jun 2022 12:26:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0167C6059B\n\tfor ;\n\tMon, 27 Jun 2022 12:26:52 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DC591C82;\n\tMon, 27 Jun 2022 12:26:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656325613;\n\tbh=of8RhEyoAcgtMy0U9d2jlV8zNjbEE8rCCfxUZrZtZ/o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=eQzKCqrwDQFw4kPl4xdlcGZZyNOKUuiuXlIQyOUVxtqZxI3j6Wr6E8SsXMYQz6qiD\n\tIElumf7NWMErvJjXPP39eLhdPt8vuRC7wcGtEcyNoJTHSSPNUy9XfhO0zQsHy0hfvB\n\tG6cTRBNdD9uInoXpRi7ilZwwCM9E2eeK3cYuIzphoCga/zugZ61UG9532NgwnnOb/y\n\tw3ujMfDfxQa6ZlwEqO+NKTHwX1Cf//5JfIMzIeMd2iC3TqTSYHMtyUinsLoEKsZUDh\n\t/kRYmQPoWPPe4qlbZUa5U9P98W1PP4qXkrLRw2l7/0af8KSG93NewfLGFLpwJLZSZv\n\tglhggP61hULrw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656325611;\n\tbh=of8RhEyoAcgtMy0U9d2jlV8zNjbEE8rCCfxUZrZtZ/o=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Cq3TnhNe9MfIXceSFpl0i7z0rHs7A1nyzJKo/pmjIzaZHbr5DdaAj424IIsduAvGa\n\tem5zeAnWS91STt2gLOXxU6U9b93SKuijhm4S+9rHWDiH6T5qM+TKaXoM4nI1rZAhoJ\n\tViYT/farbFVqB2kwjhiXyxxCPuGIeOqvMVu4jAJ0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Cq3TnhNe\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 27 Jun 2022 13:26:48 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tDavid Plowman ,\n\tJacopo Mondi ,\n\tLaurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>\n\t<165606802330.1149771.2821643337790848162@Monstersaurus>","In-Reply-To":"<165606802330.1149771.2821643337790848162@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23610,"web_url":"https://patchwork.libcamera.org/comment/23610/","msgid":"<0b94abea-1633-45d5-8b44-a9ff75a62d17@ideasonboard.com>","date":"2022-06-27T14:40:13","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 24/06/2022 17:44, Laurent Pinchart wrote:\n> On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote:\n>> Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n>>> At the moment the Python bindings only handle the requestCompleted\n>>> events. But we have others, bufferCompleted and disconnec from the\n>>\n>> disconnec/disconnect\n>>\n>>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n>>>\n>>> This makes all those events available to the users.\n>>\n>> \\o/\n>>\n>>> The get_ready_requests() method is now deprecated, but available to keep\n>>> backward compatibility.\n> \n> I'd rather drop it. We're nowhere close to guaranteeing any API in the\n> Python bindings.\n\nIt's nice to keep the existing py scripts working. I can drop it \neventually, but as it's so trivial (at least so far) to keep it, I'd \nrather have it.\n\n>> Aha, once again - answering my questions in the previous patch ;-)\n>> Maybe I need to read patch series backwards ... hehe.\n>> \n>>> The new event handling works as follows:\n>>>\n>>> The user sets callbacks to the CameraManager or Cameras (e.g.\n>>> Camera.buffer_completed). When the event_fd informs of an event, the\n>>> user must call CameraManager.dispatch_events() which gets the queued\n>>> events and calls the relevant callbacks for each queued event.\n>>>\n>>> Additionally there is CameraManager.discard_events() if the user does\n>>> not want to process the events but wants to clear the event queue (e.g.\n>>> after stopping the cameras or when exiting the app).\n> \n> What is this for ? If the user doesn't set any handler for a specific\n> event type, I would expect the corresponding events to be discarded.\n> Anything else will be passed to the event handlers. If it's about\n> ensuring that the event queue is cleared after stop() if the use doesn't\n> call dispatch_events(), I'd rather clear the queue at stop() time, and\n> possible even call dispatch_events() in stop().\n\nIt's mainly for exit time. If there are unhandled events in the event \nqueue, CameraManager and Cameras are never freed. As the app is exiting \nthere should be no other downsides to this but memory leak checkers \npossibly complaining.\n\nThe user could alternatively call dispatch_event(), but then he either \nmust be able to process the dispatched events or unsubscribe the \ncallbacks first.\n\n>> Hrm... in C++ lifetimes, the end of stop() means everything internally\n>> is released, so I'm still weary that if we change that it could be\n>> problematic (i.e. if we have to 'rely' on an application doing clean\n>> up).\n>> \n>>> I'm not very happy with this patch. It works fine, but there's a lot of\n>>> repetition of almost-the-same code. Perhaps some template magics might\n>>> reduce the repetition, but I also fear that it can easily make the code\n>>> more difficult to read.\n>>>\n>>> TODO: Documentation.\n>>>\n>>> Signed-off-by: Tomi Valkeinen \n>>> ---\n>>> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--\n>>> src/py/libcamera/py_camera_manager.h | 41 +++-\n>>> src/py/libcamera/py_main.cpp | 87 +++++++-\n>>> 3 files changed, 397 insertions(+), 22 deletions(-)\n>>>\n>>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>>> index ba45f713..bbadb9ee 100644\n>>> --- a/src/py/libcamera/py_camera_manager.cpp\n>>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>>> @@ -15,6 +15,37 @@ namespace py = pybind11;\n>>> \n>>> using namespace libcamera;\n>>> \n>>> +struct CameraEvent {\n> \n> I'd make this a class.\n\nWhy?\n\n>>> + enum class EventType {\n> \n> You can s/EventType/Type/ as you'll need to write CameraEvent::Type\n> anyway.\n\nThat's true.\n\n>>> + Undefined = 0,\n>>> + CameraAdded,\n>>> + CameraRemoved,\n>>> + Disconnect,\n>>> + RequestCompleted,\n>>> + BufferCompleted,\n>>> + };\n>>> +\n>>> + CameraEvent(EventType type)\n>>> + : type(type)\n> \n> No variable shadowing warning ? Ah, we disable them. It would still be\n> good to avoid any possible issue here, and use type_ for the member.\n\nOk.\n\n>>> + {\n>>> + }\n>>> +\n>>> + EventType type;\n>>> +\n>>> + std::shared_ptr cam;\n>>> +\n>>> + union {\n>>> + struct {\n>>> + Request *req;\n>>> + FrameBuffer *fb;\n>>> + } buf_completed;\n>>> +\n>>> + struct {\n>>> + Request *req;\n>>> + } req_completed;\n>>> + };\n> \n> As this stores pointers only, I think you could simplify it to just\n> \n> \tRequest *request_;\n> \tFrameBuffer *fb_;\n> \n> and initialize the two pointers to nullptr in the constructor, to more\n> easily catch bugs.\n\nYes, I think that's fine. I also thought about std::variant, but it felt \nlike going a bit too far.\n\nThis could backfire at some point, though, if we get new events with \ndata that doesn't quite match the above.\n\n>>> +};\n>>> +\n>>> PyCameraManager::PyCameraManager()\n>>> {\n>>> int fd = eventfd(0, 0);\n>>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()\n>>> return l;\n>>> }\n>>> \n>>> +/* DEPRECATED */\n> \n> Let's drop it :-)\n> \n>>> std::vector PyCameraManager::getReadyRequests(bool nonBlocking)\n>>> {\n>>> if (!nonBlocking || hasEvents())\n>>> readFd();\n>>> \n>>> - std::vector v;\n>>> - getRequests(v);\n>>> + std::vector v;\n>>> + getEvents(v);\n>>> \n>>> std::vector ret;\n>>> \n>>> - for (Request *req : v) {\n>>> - py::object o = py::cast(req);\n>>> - /* Decrease the ref increased in Camera.queue_request() */\n>>> - o.dec_ref();\n>>> - ret.push_back(o);\n>>> + for (const auto &ev : v) {\n>>> + switch (ev.type) {\n>>> + case CameraEvent::EventType::RequestCompleted: {\n>>> + Request *req = ev.req_completed.req;\n>>> + py::object o = py::cast(req);\n>>> + /* Decrease the ref increased in Camera.queue_request() */\n>>> + o.dec_ref();\n>>> + ret.push_back(o);\n>>> + }\n>>> + default:\n>>> + /* ignore */\n>>\n>> Does this mean that events will get lost if the deprecated call gets\n>> used? (Ok, so it's deprecated, but will it cause 'bugs' ?)\n>>\n>>\n>>> + break;\n>>> + }\n>>> }\n>>> \n>>> return ret;\n>>> }\n>>> \n>>> /* Note: Called from another thread */\n>>> -void PyCameraManager::handleRequestCompleted(Request *req)\n>>> +void PyCameraManager::handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb)\n>>> +{\n>>> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);\n>>> + ev.cam = cam;\n>>> + ev.buf_completed.req = req;\n>>> + ev.buf_completed.fb = fb;\n>>> +\n>>> + pushEvent(ev);\n>>> + writeFd();\n>>\n>> Should writeFd be part of pushEvent? then there should be one entry on\n>> the poll Fd for every event right? And forces them to be kept in sync...\n> \n> Sounds like a good idea.\n> \n>>> +}\n>>> +\n>>> +/* Note: Called from another thread */\n>>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n>>> +{\n>>> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);\n>>> + ev.cam = cam;\n>>> + ev.req_completed.req = req;\n>>> +\n>>> + pushEvent(ev);\n>>> + writeFd();\n>>> +}\n>>> +\n>>> +/* Note: Called from another thread */\n>>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n>>> {\n>>> - pushRequest(req);\n>>> + CameraEvent ev(CameraEvent::EventType::Disconnect);\n>>> + ev.cam = cam;\n>>> +\n>>> + pushEvent(ev);\n>>> writeFd();\n>>> }\n>>> \n>>> +/* Note: Called from another thread */\n>>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n>>> +{\n>>> + CameraEvent ev(CameraEvent::EventType::CameraAdded);\n>>> + ev.cam = cam;\n>>> +\n>>> + pushEvent(ev);\n>>> + writeFd();\n>>> +}\n>>> +\n>>> +/* Note: Called from another thread */\n>>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n>>> +{\n>>> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);\n>>> + ev.cam = cam;\n>>> +\n>>> + pushEvent(ev);\n>>> + writeFd();\n>>> +}\n>>\n>> Is that what you mean about code duplication? I think it's fine.\n>>\n>> These could be templated out to specify the event type I guess ... but\n>> that might be more pain. Especially when the request completed has\n>> different parameters anyway.\n> \n> Yes, generalizing this is probably not really worth it.\n> \n>>> +\n>>> +void PyCameraManager::dispatchEvents(bool nonBlocking)\n> \n> Can we make this unconditionally non-blocking ?\n> \n>>> +{\n>>> + if (!nonBlocking || hasEvents())\n>>> + readFd();\n>>> +\n>>> + std::vector v;\n> \n> s/v/events/\n> \n>>> + getEvents(v);\n> \n> \tstd::vector events = getEvents();\n> \n>>> +\n>>> + LOG(Python, Debug) << \"Dispatch \" << v.size() << \" events\";\n>>> +\n>>> + for (const auto &ev : v) {\n> \n> s/ev/event/\n\nThis style of using long variable names and yet insisting on short lines \nis sometimes a bit annoying =).\n\n>>> + switch (ev.type) {\n>>> + case CameraEvent::EventType::CameraAdded: {\n>>> + std::shared_ptr cam = ev.cam;\n> \n> As there's always a camera in all events, you could move it before the\n> switch.\n\nTrue. Originally I had the camera separately for each event.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4BC34BE173\n\tfor ;\n\tMon, 27 Jun 2022 14:40:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CD0665635;\n\tMon, 27 Jun 2022 16:40:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE3AC6059B\n\tfor ;\n\tMon, 27 Jun 2022 16:40:16 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 053891C82;\n\tMon, 27 Jun 2022 16:40:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656340818;\n\tbh=9cWT5AVQIQM+FUXFZFJf1odYUc9zt3YJAGKUXb17yUA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Tb7ndW/vEWy6T7s1DTh4Q2P28PjJzXoHz0F/bwbYwYNmegRquoE/6NX21IX1zmKiU\n\t+T5vY4kihmU2apzBHAVUWmhsQo8CZyyAjs2l+UKbOZEwnOfxDeVdzCGq7u/1fpol4i\n\t7E1pdvnysL6JsE6dO7JGfvocvE+1RCM2rNowvgODwV4q0hNvAi8vXYtCMfNHGpLlW7\n\tLSaej1+LMMLK++o/k8CG7nD/KSjrENRalMRWlgrdfWxy/TTXpyvLu2MLLVKtIicP+B\n\tZYjcikFFGJul70k8fFZRPLxdD9sS2LNEntNysQZD7gK4s6uHXqsUXVayw6TnpJXCAb\n\tMBTvzeIxR2yig==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656340816;\n\tbh=9cWT5AVQIQM+FUXFZFJf1odYUc9zt3YJAGKUXb17yUA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=dZPNXDNBewFHH+E46jSz9OgPNFROMERxrdkJWpaAvy/pwvAWOTRNkEB1ywsnPF9wK\n\tSxTjuQEwyIMos+cZBe9cE3V0USrYbUoIj7jv+E5bzXZzal0IZ5olZdaa1LtzndZqa+\n\t1ez0Y17tTA/3w+p61yUlTqbh3LDfW/I9UnwW2Ctc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dZPNXDNB\"; dkim-atps=neutral","Message-ID":"<0b94abea-1633-45d5-8b44-a9ff75a62d17@ideasonboard.com>","Date":"Mon, 27 Jun 2022 17:40:13 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>\n\t<165606802330.1149771.2821643337790848162@Monstersaurus>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23673,"web_url":"https://patchwork.libcamera.org/comment/23673/","msgid":"","date":"2022-06-30T08:28:22","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, Jun 27, 2022 at 05:40:13PM +0300, Tomi Valkeinen wrote:\n> On 24/06/2022 17:44, Laurent Pinchart wrote:\n> > On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote:\n> >> Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n> >>> At the moment the Python bindings only handle the requestCompleted\n> >>> events. But we have others, bufferCompleted and disconnec from the\n> >>\n> >> disconnec/disconnect\n> >>\n> >>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> >>>\n> >>> This makes all those events available to the users.\n> >>\n> >> \\o/\n> >>\n> >>> The get_ready_requests() method is now deprecated, but available to keep\n> >>> backward compatibility.\n> > \n> > I'd rather drop it. We're nowhere close to guaranteeing any API in the\n> > Python bindings.\n> \n> It's nice to keep the existing py scripts working. I can drop it \n> eventually, but as it's so trivial (at least so far) to keep it, I'd \n> rather have it.\n\nI'm OK with that if it can help with the transition to the new API, but\nI'd like to drop legacy code sooner than later. How long do you expect\nto keep this ?\n\n> >> Aha, once again - answering my questions in the previous patch ;-)\n> >> Maybe I need to read patch series backwards ... hehe.\n> >> \n> >>> The new event handling works as follows:\n> >>>\n> >>> The user sets callbacks to the CameraManager or Cameras (e.g.\n> >>> Camera.buffer_completed). When the event_fd informs of an event, the\n> >>> user must call CameraManager.dispatch_events() which gets the queued\n> >>> events and calls the relevant callbacks for each queued event.\n> >>>\n> >>> Additionally there is CameraManager.discard_events() if the user does\n> >>> not want to process the events but wants to clear the event queue (e.g.\n> >>> after stopping the cameras or when exiting the app).\n> > \n> > What is this for ? If the user doesn't set any handler for a specific\n> > event type, I would expect the corresponding events to be discarded.\n> > Anything else will be passed to the event handlers. If it's about\n> > ensuring that the event queue is cleared after stop() if the use doesn't\n> > call dispatch_events(), I'd rather clear the queue at stop() time, and\n> > possible even call dispatch_events() in stop().\n> \n> It's mainly for exit time. If there are unhandled events in the event \n> queue, CameraManager and Cameras are never freed. As the app is exiting \n> there should be no other downsides to this but memory leak checkers \n> possibly complaining.\n> \n> The user could alternatively call dispatch_event(), but then he either \n> must be able to process the dispatched events or unsubscribe the \n> callbacks first.\n> \n> >> Hrm... in C++ lifetimes, the end of stop() means everything internally\n> >> is released, so I'm still weary that if we change that it could be\n> >> problematic (i.e. if we have to 'rely' on an application doing clean\n> >> up).\n> >> \n> >>> I'm not very happy with this patch. It works fine, but there's a lot of\n> >>> repetition of almost-the-same code. Perhaps some template magics might\n> >>> reduce the repetition, but I also fear that it can easily make the code\n> >>> more difficult to read.\n> >>>\n> >>> TODO: Documentation.\n> >>>\n> >>> Signed-off-by: Tomi Valkeinen \n> >>> ---\n> >>> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--\n> >>> src/py/libcamera/py_camera_manager.h | 41 +++-\n> >>> src/py/libcamera/py_main.cpp | 87 +++++++-\n> >>> 3 files changed, 397 insertions(+), 22 deletions(-)\n> >>>\n> >>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> >>> index ba45f713..bbadb9ee 100644\n> >>> --- a/src/py/libcamera/py_camera_manager.cpp\n> >>> +++ b/src/py/libcamera/py_camera_manager.cpp\n> >>> @@ -15,6 +15,37 @@ namespace py = pybind11;\n> >>> \n> >>> using namespace libcamera;\n> >>> \n> >>> +struct CameraEvent {\n> > \n> > I'd make this a class.\n> \n> Why?\n\nI'm not sure, I suppose it's the constructor and nested type that makes\nit different enough from a C struct, but there's nothing in C++ that\nstates a struct shouldn't be used.\n\n> >>> + enum class EventType {\n> > \n> > You can s/EventType/Type/ as you'll need to write CameraEvent::Type\n> > anyway.\n> \n> That's true.\n> \n> >>> + Undefined = 0,\n> >>> + CameraAdded,\n> >>> + CameraRemoved,\n> >>> + Disconnect,\n> >>> + RequestCompleted,\n> >>> + BufferCompleted,\n> >>> + };\n> >>> +\n> >>> + CameraEvent(EventType type)\n> >>> + : type(type)\n> > \n> > No variable shadowing warning ? Ah, we disable them. It would still be\n> > good to avoid any possible issue here, and use type_ for the member.\n> \n> Ok.\n> \n> >>> + {\n> >>> + }\n> >>> +\n> >>> + EventType type;\n> >>> +\n> >>> + std::shared_ptr cam;\n> >>> +\n> >>> + union {\n> >>> + struct {\n> >>> + Request *req;\n> >>> + FrameBuffer *fb;\n> >>> + } buf_completed;\n> >>> +\n> >>> + struct {\n> >>> + Request *req;\n> >>> + } req_completed;\n> >>> + };\n> > \n> > As this stores pointers only, I think you could simplify it to just\n> > \n> > \tRequest *request_;\n> > \tFrameBuffer *fb_;\n> > \n> > and initialize the two pointers to nullptr in the constructor, to more\n> > easily catch bugs.\n> \n> Yes, I think that's fine. I also thought about std::variant, but it felt \n> like going a bit too far.\n> \n> This could backfire at some point, though, if we get new events with \n> data that doesn't quite match the above.\n\nThat's true, but we can fix it later in that case.\n\n> >>> +};\n> >>> +\n> >>> PyCameraManager::PyCameraManager()\n> >>> {\n> >>> int fd = eventfd(0, 0);\n> >>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()\n> >>> return l;\n> >>> }\n> >>> \n> >>> +/* DEPRECATED */\n> > \n> > Let's drop it :-)\n> > \n> >>> std::vector PyCameraManager::getReadyRequests(bool nonBlocking)\n> >>> {\n> >>> if (!nonBlocking || hasEvents())\n> >>> readFd();\n> >>> \n> >>> - std::vector v;\n> >>> - getRequests(v);\n> >>> + std::vector v;\n> >>> + getEvents(v);\n> >>> \n> >>> std::vector ret;\n> >>> \n> >>> - for (Request *req : v) {\n> >>> - py::object o = py::cast(req);\n> >>> - /* Decrease the ref increased in Camera.queue_request() */\n> >>> - o.dec_ref();\n> >>> - ret.push_back(o);\n> >>> + for (const auto &ev : v) {\n> >>> + switch (ev.type) {\n> >>> + case CameraEvent::EventType::RequestCompleted: {\n> >>> + Request *req = ev.req_completed.req;\n> >>> + py::object o = py::cast(req);\n> >>> + /* Decrease the ref increased in Camera.queue_request() */\n> >>> + o.dec_ref();\n> >>> + ret.push_back(o);\n> >>> + }\n> >>> + default:\n> >>> + /* ignore */\n> >>\n> >> Does this mean that events will get lost if the deprecated call gets\n> >> used? (Ok, so it's deprecated, but will it cause 'bugs' ?)\n> >>\n> >>\n> >>> + break;\n> >>> + }\n> >>> }\n> >>> \n> >>> return ret;\n> >>> }\n> >>> \n> >>> /* Note: Called from another thread */\n> >>> -void PyCameraManager::handleRequestCompleted(Request *req)\n> >>> +void PyCameraManager::handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb)\n> >>> +{\n> >>> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);\n> >>> + ev.cam = cam;\n> >>> + ev.buf_completed.req = req;\n> >>> + ev.buf_completed.fb = fb;\n> >>> +\n> >>> + pushEvent(ev);\n> >>> + writeFd();\n> >>\n> >> Should writeFd be part of pushEvent? then there should be one entry on\n> >> the poll Fd for every event right? And forces them to be kept in sync...\n> > \n> > Sounds like a good idea.\n> > \n> >>> +}\n> >>> +\n> >>> +/* Note: Called from another thread */\n> >>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> >>> +{\n> >>> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);\n> >>> + ev.cam = cam;\n> >>> + ev.req_completed.req = req;\n> >>> +\n> >>> + pushEvent(ev);\n> >>> + writeFd();\n> >>> +}\n> >>> +\n> >>> +/* Note: Called from another thread */\n> >>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> >>> {\n> >>> - pushRequest(req);\n> >>> + CameraEvent ev(CameraEvent::EventType::Disconnect);\n> >>> + ev.cam = cam;\n> >>> +\n> >>> + pushEvent(ev);\n> >>> writeFd();\n> >>> }\n> >>> \n> >>> +/* Note: Called from another thread */\n> >>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> >>> +{\n> >>> + CameraEvent ev(CameraEvent::EventType::CameraAdded);\n> >>> + ev.cam = cam;\n> >>> +\n> >>> + pushEvent(ev);\n> >>> + writeFd();\n> >>> +}\n> >>> +\n> >>> +/* Note: Called from another thread */\n> >>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> >>> +{\n> >>> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);\n> >>> + ev.cam = cam;\n> >>> +\n> >>> + pushEvent(ev);\n> >>> + writeFd();\n> >>> +}\n> >>\n> >> Is that what you mean about code duplication? I think it's fine.\n> >>\n> >> These could be templated out to specify the event type I guess ... but\n> >> that might be more pain. Especially when the request completed has\n> >> different parameters anyway.\n> > \n> > Yes, generalizing this is probably not really worth it.\n> > \n> >>> +\n> >>> +void PyCameraManager::dispatchEvents(bool nonBlocking)\n> > \n> > Can we make this unconditionally non-blocking ?\n> > \n> >>> +{\n> >>> + if (!nonBlocking || hasEvents())\n> >>> + readFd();\n> >>> +\n> >>> + std::vector v;\n> > \n> > s/v/events/\n> > \n> >>> + getEvents(v);\n> > \n> > \tstd::vector events = getEvents();\n> > \n> >>> +\n> >>> + LOG(Python, Debug) << \"Dispatch \" << v.size() << \" events\";\n> >>> +\n> >>> + for (const auto &ev : v) {\n> > \n> > s/ev/event/\n> \n> This style of using long variable names and yet insisting on short lines \n> is sometimes a bit annoying =).\n\nConflicting requirements ? :-) I agree, and shortening can make sense,\nbut I'd do so only when it helps.\n\n> >>> + switch (ev.type) {\n> >>> + case CameraEvent::EventType::CameraAdded: {\n> >>> + std::shared_ptr cam = ev.cam;\n> > \n> > As there's always a camera in all events, you could move it before the\n> > switch.\n> \n> True. Originally I had the camera separately for each event.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6ADD7BE173\n\tfor ;\n\tThu, 30 Jun 2022 08:28:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BEF665651;\n\tThu, 30 Jun 2022 10:28:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B09106564C\n\tfor ;\n\tThu, 30 Jun 2022 10:28:42 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CDD0B59D;\n\tThu, 30 Jun 2022 10:28:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656577724;\n\tbh=ZWp/YVpaGtAOdjt6jmA4ogoODoKS+XAwZcQd47CffYQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=LTpoi1GKFv3IyQs1wNyI7OAVSQVEpR9jBtVv5gy0zTQDnZPkzo38DPaXUP7m8GPHP\n\tTsQfxxHk7noogRcNFDlFtEuC3eODmxfyZycOgBy/rYUR+Fx0LuAfvY75qZBwNpqZAJ\n\tXF7DFeDGSd3/fgPEuXUoHNjauk8xr/3I6JLWXcrdMvCLO1Au1KpgiqEY/LRoB94VY3\n\tcizrZAfWwbt4cHS8qCLnftsFYq7nV+Zs0P7lpeDs1J83v7L9Cs03Gzz+l/99y1V9X7\n\tLQ5fhIj7gIoIkNeAUq6hivmdU2sDhR4ZI74oCcB+NiJNtsrlx9BUSKmDCJm2lGrI7s\n\t8GCdafaQb3qbg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656577722;\n\tbh=ZWp/YVpaGtAOdjt6jmA4ogoODoKS+XAwZcQd47CffYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YBMwP++kDOlUiqlbaDKpSbDc13UkQxBco6WZeVHGyeXCPbfOW9c+lRjEuSWLN1K/e\n\tHZxkhbBxWI/U9o3fb6o4N4KeMVHxIf2LHN/OVB326ePErtSKBl/ynDqyhBDX/zISCC\n\tlbjM+AAiKfVg/CQk/XDbU+DWa3I9sI4WBtIEK7xE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YBMwP++k\"; dkim-atps=neutral","Date":"Thu, 30 Jun 2022 11:28:22 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>\n\t<165606802330.1149771.2821643337790848162@Monstersaurus>\n\t\n\t<0b94abea-1633-45d5-8b44-a9ff75a62d17@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<0b94abea-1633-45d5-8b44-a9ff75a62d17@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23675,"web_url":"https://patchwork.libcamera.org/comment/23675/","msgid":"<410b0556-e2f4-8633-21ee-e60024b45ef7@ideasonboard.com>","date":"2022-06-30T08:42:13","subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/06/2022 11:28, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Mon, Jun 27, 2022 at 05:40:13PM +0300, Tomi Valkeinen wrote:\n>> On 24/06/2022 17:44, Laurent Pinchart wrote:\n>>> On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote:\n>>>> Quoting Tomi Valkeinen (2022-06-23 15:47:35)\n>>>>> At the moment the Python bindings only handle the requestCompleted\n>>>>> events. But we have others, bufferCompleted and disconnec from the\n>>>>\n>>>> disconnec/disconnect\n>>>>\n>>>>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n>>>>>\n>>>>> This makes all those events available to the users.\n>>>>\n>>>> \\o/\n>>>>\n>>>>> The get_ready_requests() method is now deprecated, but available to keep\n>>>>> backward compatibility.\n>>>\n>>> I'd rather drop it. We're nowhere close to guaranteeing any API in the\n>>> Python bindings.\n>>\n>> It's nice to keep the existing py scripts working. I can drop it\n>> eventually, but as it's so trivial (at least so far) to keep it, I'd\n>> rather have it.\n> \n> I'm OK with that if it can help with the transition to the new API, but\n> I'd like to drop legacy code sooner than later. How long do you expect\n> to keep this ?\n\nI'll convert the examples etc. in libcamera repository (note that this \nseries so far only converts cam.py), which will be trivial, but \npicamera2 may be more difficult. So I'd say David can decide. Probably \nwe can drop it quite soon after picamera2 uses it.\n\nHowever, as I'm still not sure if the new event handling API is good \nenough, and shouldn't be merged, the get_ready_requests is not \ndeprecated yet.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C415BE173\n\tfor ;\n\tThu, 30 Jun 2022 08:42:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 242BF65651;\n\tThu, 30 Jun 2022 10:42:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E8E56564C\n\tfor ;\n\tThu, 30 Jun 2022 10:42:16 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A16A445F;\n\tThu, 30 Jun 2022 10:42:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656578538;\n\tbh=WNvs3o6pbo5fepJ+BqzK2BSAm4BCijbcm/OVGRiifJU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iSB0D5BObOqLGAtFBiDLxeyHvDqhsUC8QRPmklT7VlQtFBRP3yQLl3p0yaYRV1+PE\n\tmee4YqOgmRGS7NeBVltTlgvedKfZNfl00+rMnx7OLkCweG8S36rdb3aCA4OV+gQ9yx\n\t2UT9l9Xuam53bLZ9g6sqzw/kkIFe8VOmL4Op0dUV+6v7t/+hJ7Tbg4K3dUdI6Ya2jA\n\t2IT5J3FOSX7CTrgPH3GV9eIM1f8tf/PvmypVbD/qguAT/gqAn3bVL9jDjVX5NYymrH\n\tk/z0JQmKucjD83VtiIPrMjzZQSNyamXMTKfjgPU7jenOz25qG2SJuXzLFOrdNO2ljD\n\t7wK5bICw15lYQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656578535;\n\tbh=WNvs3o6pbo5fepJ+BqzK2BSAm4BCijbcm/OVGRiifJU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=IxwlL8y9hN4lHyXJgSX1S6dxnl6Y09qyKNpQKHKQHB7Vy+nBqQPl5eZTsxWlvglb8\n\t5x8wNWwiRiJ9NK6T0LACiep3tY3SkNtN4Yuormv9xBx025wavIe6210R0w1aZ7lKSZ\n\tGSG3Gtq9yOWEyyN1r2qwxpqtP+OIvolErQvXwXlo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"IxwlL8y9\"; dkim-atps=neutral","Message-ID":"<410b0556-e2f4-8633-21ee-e60024b45ef7@ideasonboard.com>","Date":"Thu, 30 Jun 2022 11:42:13 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220623144736.78537-1-tomi.valkeinen@ideasonboard.com>\n\t<20220623144736.78537-7-tomi.valkeinen@ideasonboard.com>\n\t<165606802330.1149771.2821643337790848162@Monstersaurus>\n\t\n\t<0b94abea-1633-45d5-8b44-a9ff75a62d17@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC v1 6/7] py: New event handling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]