[{"id":24694,"web_url":"https://patchwork.libcamera.org/comment/24694/","msgid":"","date":"2022-08-18T22:00:26","subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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\nThank you for the patch.\n\nOn Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:\n> At the moment the Python bindings only handle the requestCompleted\n> events. But we have others, bufferCompleted and disconnect from the\n> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n\nAnd we will likely have error events in the not too distant future.\n\n> This patch makes all those events available to the users.\n> \n> The get_ready_requests() method is now deprecated, but available to keep\n> backward compatibility.\n\nWho are the users of get_ready_requests() that you are concern about,\nand when could it be removed ? Other changes in this series break the\nAPI (such as switching to exceptions), so all users will need to be\nupdated, I'd rather remove get_ready_requests() at the same time.\n\n> The new event handling works like get_ready_requests(), but instead of\n> returning only Requests from requestCompleted events, it returns all\n> event types using a new Event class.\n> \n> Additionally camera.stop() has been changed to return events for that\n> camera. This serves two purposes: first, it removes the requestCompleted\n> events from the event queue, thus preventing the old events being\n> returned when the camera is started again, and second, it allows the\n> user to process those events if it so wants.\n> \n> All other event types are always collected and returned, except\n> bufferCompleted which needs to be enabled by setting the\n> CameraManager.buffer_completed_active to True. This is to reduce the\n> overhead a bit. It is not clear if this is a necessary optimization or\n> not.\n\nAs mentioned in the reply to the cover letter, we could have a more\ngeneric subscription mechanism, but I also think it may not make much\nsense to make subscription optional for all the other events, perhaps\nwith the exception of a future metadata completion event.\n\nIf only the buffer and metadata completion events are optional, should\nwe have a per-camera subscription ?\n\n> TODO: Documentation.\n\nThis should be captured in the code. Or documentation could be written\n:-)\n\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--\n> src/py/libcamera/py_camera_manager.h | 64 ++++++++-\n> src/py/libcamera/py_main.cpp | 45 +++++-\n> 3 files changed, 276 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> index 3dd8668e..d1d63690 100644\n> --- a/src/py/libcamera/py_camera_manager.cpp\n> +++ b/src/py/libcamera/py_camera_manager.cpp\n> @@ -5,6 +5,7 @@\n> \n> #include \"py_camera_manager.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()\n> \tif (ret)\n> \t\tthrow std::system_error(-ret, std::generic_category(),\n> \t\t\t\t\t\"Failed to start CameraManager\");\n> +\n> +\tcameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n> +\tcameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n> }\n> \n> PyCameraManager::~PyCameraManager()\n> {\n> \tLOG(Python, Debug) << \"~PyCameraManager()\";\n> +\n> +\tcameraManager_->cameraAdded.disconnect();\n> +\tcameraManager_->cameraRemoved.disconnect();\n> }\n> \n> py::list PyCameraManager::cameras()\n> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()\n> \treturn l;\n> }\n> \n> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)\n> +{\n> +\tPyCameraEvent pyevent;\n> +\n> +\tpyevent.type_ = event.type_;\n> +\n> +\t/*\n> +\t * We need to set a keep-alive here so that the camera keeps the\n> +\t * camera manager alive.\n> +\t */\n> +\tpy::object py_cm = py::cast(this);\n> +\tpy::object py_cam = py::cast(event.camera_);\n> +\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> +\n> +\tpyevent.camera_ = py_cam;\n> +\n> +\tswitch (event.type_) {\n> +\tcase CameraEventType::CameraAdded:\n> +\tcase CameraEventType::CameraRemoved:\n> +\tcase CameraEventType::Disconnect:\n> +\t\t/* No additional parameters to add */\n> +\t\tbreak;\n> +\n> +\tcase CameraEventType::BufferCompleted: {\n> +\t\tpyevent.request_ = py::cast(event.request_);\n> +\t\tpyevent.fb_ = py::cast(event.fb_);\n> +\t\tbreak;\n> +\t}\n\nNo need for curly braces. But a blank line would be nice.\n\n> +\tcase CameraEventType::RequestCompleted: {\n> +\t\tpyevent.request_ = py::cast(event.request_);\n> +\n> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> +\t\tpyevent.request_.dec_ref();\n> +\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tASSERT(false);\n\nWhat if we dropped the undefined event type ? You wouldn't need this\nthen. It's best to write code that makes errors impossible.\n\nI think I would also move all of this, except the py::cast(this) call,\nto a PyCameraEvent constructor that takes the CameraEvent and camera\nmanager py::object.\n\n> +\t}\n> +\n> +\treturn pyevent;\n> +}\n> +\n> +/* DEPRECATED */\n> std::vector PyCameraManager::getReadyRequests()\n> {\n> \tint ret = readFd();\n> @@ -70,21 +121,125 @@ std::vector PyCameraManager::getReadyRequests()\n> \n> \tstd::vector py_reqs;\n> \n> -\tfor (Request *request : getCompletedRequests()) {\n> -\t\tpy::object o = py::cast(request);\n> -\t\t/* Decrease the ref increased in Camera.queue_request() */\n> -\t\to.dec_ref();\n> -\t\tpy_reqs.push_back(o);\n> +\tfor (const auto &ev : getEvents()) {\n> +\t\tif (ev.type_ != CameraEventType::RequestCompleted)\n> +\t\t\tcontinue;\n\nOuch, this will silently drop all other types of events. I'd really like\nto drop getReadyRequests().\n\n> +\n> +\t\tPyCameraEvent pyev = convertEvent(ev);\n> +\t\tpy_reqs.push_back(pyev.request_);\n> \t}\n> \n> \treturn py_reqs;\n> }\n> \n> +std::vector PyCameraManager::getPyEvents()\n> +{\n> +\tint ret = readFd();\n> +\n> +\tif (ret == EAGAIN) {\n> +\t\tLOG(Python, Debug) << \"No events\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tif (ret != 0)\n> +\t\tthrow std::system_error(ret, std::generic_category());\n> +\n> +\tstd::vector events = getEvents();\n> +\n> +\tLOG(Python, Debug) << \"Get \" << events.size() << \" events\";\n\ns/Get/Got/\n\n> +\n> +\tstd::vector pyevents(events.size());\n\nYou'll end up creating events.size() default-constructed elements here,\nonly to overwrite them just after. I think calling .reserve() and using\na std::back_inserter in the std::transform loop below would be more\nefficient (see https://en.cppreference.com/w/cpp/algorithm/transform).\n\n> +\n> +\tstd::transform(events.begin(), events.end(), pyevents.begin(),\n> +\t\t [this](const CameraEvent &ev) {\n> +\t\t\t return convertEvent(ev);\n> +\t\t });\n> +\n> +\treturn pyevents;\n> +}\n> +\n> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr &camera)\n> +{\n> +\treturn event.camera_ == camera &&\n> +\t (event.type_ == CameraEventType::RequestCompleted ||\n> +\t\tevent.type_ == CameraEventType::BufferCompleted ||\n> +\t\tevent.type_ == CameraEventType::Disconnect);\n\nI don't think you should include disconnect events here. This would\nresult in stop() removing disconnect events from the queue, while I\nthink they need to be handled through the normal event handling\nmechanism.\n\n> +}\n> +\n> +std::vector PyCameraManager::getPyCameraEvents(std::shared_ptr camera)\n> +{\n> +\tMutexLocker guard(eventsMutex_);\n> +\n> +\tstd::vector pyevents;\n> +\n> +\t/* Get events related to the given camera */\n> +\n> +\tfor (const auto &event : events_) {\n> +\t\tif (!isCameraSpecificEvent(event, camera))\n> +\t\t\tcontinue;\n> +\n> +\t\tPyCameraEvent pyev = convertEvent(event);\n> +\t\tpyevents.push_back(pyev);\n> +\t}\n> +\n> +\t/* Drop the events from the events_ list */\n> +\n> +\tevents_.erase(std::remove_if(events_.begin(), events_.end(),\n> +\t\t[&camera](const CameraEvent &ev) {\n> +\t\t\treturn isCameraSpecificEvent(ev, camera);\n> +\t\t}),\n> +\t\tevents_.end());\n\nI'd like to minmize the time spent holding the lock. You can move the\nevents specific to this camera to a different vector without converting\nthem (and merge the two loops if possible), and the convert the events\nwithout holding the lock.\n\nAlso, dropping elements in the middle of a vector is fairly inefficient.\nI think you should use a list instead of a vector for events_, which\nwill allow you to efficiently move elements here with splice().\n\n> +\n> +\tLOG(Python, Debug) << \"Get \" << pyevents.size() << \" camera events, \"\n\ns/Get/Got/ ?\n\n> +\t\t\t << events_.size() << \" unhandled events left\";\n> +\n> +\treturn pyevents;\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> -\tpushRequest(req);\n> -\twriteFd();\n> +\tif (!bufferCompletedEventActive_)\n> +\t\treturn;\n> +\n> +\tCameraEvent ev(CameraEventType::BufferCompleted, cam);\n> +\tev.request_ = req;\n> +\tev.fb_ = fb;\n> +\n> +\tpushEvent(ev);\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> +{\n> +\tCameraEvent ev(CameraEventType::RequestCompleted, cam);\n> +\tev.request_ = req;\n> +\n> +\tpushEvent(ev);\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> +{\n> +\tCameraEvent ev(CameraEventType::Disconnect, cam);\n> +\n> +\tpushEvent(ev);\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> +{\n> +\tCameraEvent ev(CameraEventType::CameraAdded, cam);\n> +\n> +\tpushEvent(ev);\n> +}\n> +\n> +/* Note: Called from another thread */\n> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> +{\n> +\tCameraEvent ev(CameraEventType::CameraRemoved, cam);\n> +\n> +\tpushEvent(ev);\n> }\n> \n> void PyCameraManager::writeFd()\n> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()\n> \treturn 0;\n> }\n> \n> -void PyCameraManager::pushRequest(Request *req)\n> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n> {\n> -\tMutexLocker guard(completedRequestsMutex_);\n> -\tcompletedRequests_.push_back(req);\n> +\tMutexLocker guard(eventsMutex_);\n> +\tevents_.push_back(ev);\n> +\n> +\twriteFd();\n\nThis can be done without holding the lock.\n\n> +\n> +\tLOG(Python, Debug) << \"Queued events: \" << events_.size();\n\nI'm tempted to drop this, it will make the log very chatty. Have you\nfound it useful to debug applications ?\n\n> }\n> \n> -std::vector PyCameraManager::getCompletedRequests()\n> +std::vector PyCameraManager::getEvents()\n> {\n> -\tstd::vector v;\n> -\tMutexLocker guard(completedRequestsMutex_);\n> -\tswap(v, completedRequests_);\n> +\tstd::vector v;\n> +\n> +\tMutexLocker guard(eventsMutex_);\n> +\tswap(v, events_);\n> +\n> \treturn v;\n> }\n> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> index 3525057d..b313ba9b 100644\n> --- a/src/py/libcamera/py_camera_manager.h\n> +++ b/src/py/libcamera/py_camera_manager.h\n> @@ -13,6 +13,47 @@\n> \n> using namespace libcamera;\n> \n> +enum class CameraEventType {\n> +\tUndefined = 0,\n\nUnless we drop this (which would be my preference), maybe Invalid\ninstead of Undefined ? People don't like undefined behaviours :-D\n\n> +\tCameraAdded,\n> +\tCameraRemoved,\n> +\tDisconnect,\n> +\tRequestCompleted,\n> +\tBufferCompleted,\n> +};\n> +\n> +/*\n> + * This event struct is used internally to queue the events we receive from\n> + * other threads.\n> + */\n> +struct CameraEvent {\n> +\tCameraEvent(CameraEventType type, std::shared_ptr camera)\n> +\t\t: type_(type), camera_(camera)\n> +\t{\n> +\t}\n> +\n> +\tCameraEventType type_ = CameraEventType::Undefined;\n> +\n> +\tstd::shared_ptr camera_;\n> +\n> +\tRequest *request_ = nullptr;\n> +\tFrameBuffer *fb_ = nullptr;\n> +};\n> +\n> +/*\n> + * This event struct is passed to Python. We need to use pybind11::object here\n> + * instead of a C++ pointer so that we keep a ref to the Request, and a\n> + * keep-alive from the camera to the camera manager.\n> + */\n> +struct PyCameraEvent {\n> +\tCameraEventType type_ = CameraEventType::Undefined;\n> +\n> +\tpybind11::object camera_;\n> +\n> +\tpybind11::object request_;\n> +\tpybind11::object fb_;\n> +};\n> +\n> class PyCameraManager\n> {\n> public:\n> @@ -26,20 +67,29 @@ public:\n> \n> \tint eventFd() const { return eventFd_.get(); }\n> \n> -\tstd::vector getReadyRequests();\n> +\tstd::vector getReadyRequests(); /* DEPRECATED */\n> +\tstd::vector getPyEvents();\n> +\tstd::vector getPyCameraEvents(std::shared_ptr camera);\n> \n> -\tvoid handleRequestCompleted(Request *req);\n> +\tvoid handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n> +\tvoid handleRequestCompleted(std::shared_ptr cam, Request *req);\n> +\tvoid handleDisconnected(std::shared_ptr cam);\n> +\tvoid handleCameraAdded(std::shared_ptr cam);\n> +\tvoid handleCameraRemoved(std::shared_ptr cam);\n> \n> +\tbool bufferCompletedEventActive_ = false;\n\nMissing blank line.\n\n> private:\n> \tstd::unique_ptr cameraManager_;\n> \n> \tUniqueFD eventFd_;\n> -\tlibcamera::Mutex completedRequestsMutex_;\n> -\tstd::vector completedRequests_\n> -\t\tLIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);\n> +\tlibcamera::Mutex eventsMutex_;\n> +\tstd::vector events_\n> +\t\tLIBCAMERA_TSA_GUARDED_BY(eventsMutex_);\n> \n> \tvoid writeFd();\n> \tint readFd();\n> -\tvoid pushRequest(Request *req);\n> -\tstd::vector getCompletedRequests();\n> +\tvoid pushEvent(const CameraEvent &ev);\n> +\tstd::vector getEvents();\n> +\n> +\tPyCameraEvent convertEvent(const CameraEvent &event);\n> };\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index 6cd99919..b4f756d7 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -58,6 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> \t */\n> \n> +\tauto pyEvent = py::class_(m, \"Event\");\n> \tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> \tauto pyCamera = py::class_(m, \"Camera\");\n> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)\n> \tm.def(\"log_set_level\", &logSetLevel);\n> \n> \t/* Classes */\n> +\n> +\tpy::enum_(pyEvent, \"Type\")\n> +\t\t.value(\"Undefined\", CameraEventType::Undefined)\n> +\t\t.value(\"CameraAdded\", CameraEventType::CameraAdded)\n> +\t\t.value(\"CameraRemoved\", CameraEventType::CameraRemoved)\n> +\t\t.value(\"Disconnect\", CameraEventType::Disconnect)\n> +\t\t.value(\"RequestCompleted\", CameraEventType::RequestCompleted)\n> +\t\t.value(\"BufferCompleted\", CameraEventType::BufferCompleted);\n> +\n> +\tpyEvent\n> +\t\t.def_readonly(\"type\", &PyCameraEvent::type_)\n> +\t\t.def_readonly(\"camera\", &PyCameraEvent::camera_)\n> +\t\t.def_readonly(\"request\", &PyCameraEvent::request_)\n> +\t\t.def_readonly(\"fb\", &PyCameraEvent::fb_);\n> +\n> \tpyCameraManager\n> \t\t.def_static(\"singleton\", []() {\n> \t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> @@ -107,7 +123,13 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> \n> \t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> -\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> +\n> +\t\t/* DEPRECATED */\n\nIs there any way to generate a warning at runtime when this gets used ?\nIf we drop it (depending on the outcome of the discussion in the commit\nmessage) that will be even easier :-)\n\n> +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests)\n> +\n> +\t\t.def(\"get_events\", &PyCameraManager::getPyEvents)\n> +\n> +\t\t.def_readwrite(\"buffer_completed_active\", &PyCameraManager::bufferCompletedEventActive_);\n> \n> \tpyCamera\n> \t\t.def_property_readonly(\"id\", &Camera::id)\n> @@ -131,7 +153,17 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\tauto cm = gCameraManager.lock();\n> \t\t\tASSERT(cm);\n> \n> -\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> +\t\t\tself.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n> +\t\t\t\tcm->handleRequestCompleted(camera, req);\n> +\t\t\t});\n> +\n> +\t\t\tself.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n> +\t\t\t\tcm->handleBufferCompleted(camera, req, fb);\n> +\t\t\t});\n> +\n> +\t\t\tself.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n> +\t\t\t\tcm->handleDisconnected(camera);\n> +\t\t\t});\n> \n> \t\t\tControlList controlList(self.controls());\n> \n> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\tint ret = self.stop();\n> \n> \t\t\tself.requestCompleted.disconnect();\n> +\t\t\tself.bufferCompleted.disconnect();\n> +\t\t\tself.disconnected.disconnect();\n> +\n> +\t\t\tauto cm = gCameraManager.lock();\n> +\t\t\tASSERT(cm);\n> +\n> +\t\t\tauto l = cm->getPyCameraEvents(self.shared_from_this());\n\nPlease use a more descriptive variable name.\n\n> \n> \t\t\t/* \\todo Should we just ignore the error? */\n> \t\t\tif (ret)\n> \t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> \t\t\t\t\t\t\t\"Failed to start camera\");\n> +\n> +\t\t\treturn l;\n> \t\t})\n> \n> \t\t.def(\"__str__\", [](Camera &self) {","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 F2004BE173\n\tfor ;\n\tThu, 18 Aug 2022 22:00:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CB9961FC0;\n\tFri, 19 Aug 2022 00:00:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1AD9E61FA8\n\tfor ;\n\tFri, 19 Aug 2022 00:00:30 +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 489F5895;\n\tFri, 19 Aug 2022 00:00:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660860032;\n\tbh=/BRB+ex+e9pGBgiyhdcRG3Imq+3D5NMQ/qihYRhNaiE=;\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=3zncli0MOhJBUx72pi/LRtr8WXYHQG9WZcJmx3apsCh3dtV3ApUIB/XU8BBUQfS8I\n\tjHrXzrKpSO/UaVon84om2ZE2godMxnzZElKSPIze9YxGyGi/2RWQley60a1UuXhBZL\n\t4FEf5gVBbDi0Z1Q9E4eSz0z3F6r1Gb74A9BkApFFaQGlQVor5/UmTZ/6Gh4W4aWwPI\n\tuHQerkSfHid4VsgCpsYwBKPsFt8x7tLXAa8mFUJYcNNBlGucaGX9PRCut5crJ7JB6g\n\tC4eGsdLFNUdM9JuP7bc9hgYa2qmSFFUbg0Vu9ljec0112gC4yuwLYDG7ztUS+dkIKZ\n\tT4MhylViyQADA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660860029;\n\tbh=/BRB+ex+e9pGBgiyhdcRG3Imq+3D5NMQ/qihYRhNaiE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cogBF/xFQhAGpEGWkmRqXxD992z/PT7Bjdpxo0JGxEPBipmncCikx2Nv//MIorJJz\n\tgkPQOxngaFMonBqssAYSkSuDUY8IRsLTpQmY7XbPFoSGyRHsJoCP+tJCOg2NLzDeLn\n\tK6blek1tSWyLLWO3eUIwr1St7cfSicVRoOJYYfQ0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cogBF/xF\"; dkim-atps=neutral","Date":"Fri, 19 Aug 2022 01:00:26 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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":24796,"web_url":"https://patchwork.libcamera.org/comment/24796/","msgid":"<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>","date":"2022-08-26T13:24:46","subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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 19/08/2022 01:00, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:\n>> At the moment the Python bindings only handle the requestCompleted\n>> events. But we have others, bufferCompleted and disconnect from the\n>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> \n> And we will likely have error events in the not too distant future.\n> \n>> This patch makes all those events available to the users.\n>>\n>> The get_ready_requests() method is now deprecated, but available to keep\n>> backward compatibility.\n> \n> Who are the users of get_ready_requests() that you are concern about,\n> and when could it be removed ? Other changes in this series break the\n> API (such as switching to exceptions), so all users will need to be\n> updated, I'd rather remove get_ready_requests() at the same time.\n\nI'm fine with dropping get_ready_requests(). It is just so trivial to \nsupport it that I kept it, which also helped converting the examples, as \nI didn't have to convert everything in one commit.\n\n>> The new event handling works like get_ready_requests(), but instead of\n>> returning only Requests from requestCompleted events, it returns all\n>> event types using a new Event class.\n>>\n>> Additionally camera.stop() has been changed to return events for that\n>> camera. This serves two purposes: first, it removes the requestCompleted\n>> events from the event queue, thus preventing the old events being\n>> returned when the camera is started again, and second, it allows the\n>> user to process those events if it so wants.\n>>\n>> All other event types are always collected and returned, except\n>> bufferCompleted which needs to be enabled by setting the\n>> CameraManager.buffer_completed_active to True. This is to reduce the\n>> overhead a bit. It is not clear if this is a necessary optimization or\n>> not.\n> \n> As mentioned in the reply to the cover letter, we could have a more\n> generic subscription mechanism, but I also think it may not make much\n> sense to make subscription optional for all the other events, perhaps\n> with the exception of a future metadata completion event.\n> \n> If only the buffer and metadata completion events are optional, should\n> we have a per-camera subscription ?\n\nThat's one option. I did try multiple different approaches, and \neverything seems to have their drawbacks. When I get some time I'll \nagain try a few different things to refresh my memory.\n\n>> TODO: Documentation.\n> \n> This should be captured in the code. Or documentation could be written\n> :-)\n\nI'll write it before merging, when we're happy with the model. I don't \nwant to write it until I'm happy with the code =).\n\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--\n>> src/py/libcamera/py_camera_manager.h | 64 ++++++++-\n>> src/py/libcamera/py_main.cpp | 45 +++++-\n>> 3 files changed, 276 insertions(+), 24 deletions(-)\n>>\n>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>> index 3dd8668e..d1d63690 100644\n>> --- a/src/py/libcamera/py_camera_manager.cpp\n>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>> @@ -5,6 +5,7 @@\n>> \n>> #include \"py_camera_manager.h\"\n>> \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()\n>> \tif (ret)\n>> \t\tthrow std::system_error(-ret, std::generic_category(),\n>> \t\t\t\t\t\"Failed to start CameraManager\");\n>> +\n>> +\tcameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n>> +\tcameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n>> }\n>> \n>> PyCameraManager::~PyCameraManager()\n>> {\n>> \tLOG(Python, Debug) << \"~PyCameraManager()\";\n>> +\n>> +\tcameraManager_->cameraAdded.disconnect();\n>> +\tcameraManager_->cameraRemoved.disconnect();\n>> }\n>> \n>> py::list PyCameraManager::cameras()\n>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()\n>> \treturn l;\n>> }\n>> \n>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)\n>> +{\n>> +\tPyCameraEvent pyevent;\n>> +\n>> +\tpyevent.type_ = event.type_;\n>> +\n>> +\t/*\n>> +\t * We need to set a keep-alive here so that the camera keeps the\n>> +\t * camera manager alive.\n>> +\t */\n>> +\tpy::object py_cm = py::cast(this);\n>> +\tpy::object py_cam = py::cast(event.camera_);\n>> +\tpy::detail::keep_alive_impl(py_cam, py_cm);\n>> +\n>> +\tpyevent.camera_ = py_cam;\n>> +\n>> +\tswitch (event.type_) {\n>> +\tcase CameraEventType::CameraAdded:\n>> +\tcase CameraEventType::CameraRemoved:\n>> +\tcase CameraEventType::Disconnect:\n>> +\t\t/* No additional parameters to add */\n>> +\t\tbreak;\n>> +\n>> +\tcase CameraEventType::BufferCompleted: {\n>> +\t\tpyevent.request_ = py::cast(event.request_);\n>> +\t\tpyevent.fb_ = py::cast(event.fb_);\n>> +\t\tbreak;\n>> +\t}\n> \n> No need for curly braces. But a blank line would be nice.\n\nYep.\n\n>> +\tcase CameraEventType::RequestCompleted: {\n>> +\t\tpyevent.request_ = py::cast(event.request_);\n>> +\n>> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n>> +\t\tpyevent.request_.dec_ref();\n>> +\n>> +\t\tbreak;\n>> +\t}\n>> +\tdefault:\n>> +\t\tASSERT(false);\n> \n> What if we dropped the undefined event type ? You wouldn't need this\n> then. It's best to write code that makes errors impossible.\n\nThat wouldn't make errors impossible, it would just hide possible bugs \n=). When something goes bad, event.type_ can contains an illegal value. \nI think it's good to catch it here.\n\n> I think I would also move all of this, except the py::cast(this) call,\n> to a PyCameraEvent constructor that takes the CameraEvent and camera\n> manager py::object.\n\nWe also do the ref count magics for RequestCompleted. I'll think about \nit, but I probably like it better if we keep the PyCameraEvent as a \ndummy container.\n\n>> +\t}\n>> +\n>> +\treturn pyevent;\n>> +}\n>> +\n>> +/* DEPRECATED */\n>> std::vector PyCameraManager::getReadyRequests()\n>> {\n>> \tint ret = readFd();\n>> @@ -70,21 +121,125 @@ std::vector PyCameraManager::getReadyRequests()\n>> \n>> \tstd::vector py_reqs;\n>> \n>> -\tfor (Request *request : getCompletedRequests()) {\n>> -\t\tpy::object o = py::cast(request);\n>> -\t\t/* Decrease the ref increased in Camera.queue_request() */\n>> -\t\to.dec_ref();\n>> -\t\tpy_reqs.push_back(o);\n>> +\tfor (const auto &ev : getEvents()) {\n>> +\t\tif (ev.type_ != CameraEventType::RequestCompleted)\n>> +\t\t\tcontinue;\n> \n> Ouch, this will silently drop all other types of events. I'd really like\n> to drop getReadyRequests().\n\nThat's exactly as it was before.\n\n>> +\n>> +\t\tPyCameraEvent pyev = convertEvent(ev);\n>> +\t\tpy_reqs.push_back(pyev.request_);\n>> \t}\n>> \n>> \treturn py_reqs;\n>> }\n>> \n>> +std::vector PyCameraManager::getPyEvents()\n>> +{\n>> +\tint ret = readFd();\n>> +\n>> +\tif (ret == EAGAIN) {\n>> +\t\tLOG(Python, Debug) << \"No events\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\tif (ret != 0)\n>> +\t\tthrow std::system_error(ret, std::generic_category());\n>> +\n>> +\tstd::vector events = getEvents();\n>> +\n>> +\tLOG(Python, Debug) << \"Get \" << events.size() << \" events\";\n> \n> s/Get/Got/\n\nOk.\n\n>> +\n>> +\tstd::vector pyevents(events.size());\n> \n> You'll end up creating events.size() default-constructed elements here,\n> only to overwrite them just after. I think calling .reserve() and using\n> a std::back_inserter in the std::transform loop below would be more\n> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).\n\nTrue. And changing this allows me to add a more specific constructor for \nPyCameraEvent.\n\n>> +\n>> +\tstd::transform(events.begin(), events.end(), pyevents.begin(),\n>> +\t\t [this](const CameraEvent &ev) {\n>> +\t\t\t return convertEvent(ev);\n>> +\t\t });\n>> +\n>> +\treturn pyevents;\n>> +}\n>> +\n>> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr &camera)\n>> +{\n>> +\treturn event.camera_ == camera &&\n>> +\t (event.type_ == CameraEventType::RequestCompleted ||\n>> +\t\tevent.type_ == CameraEventType::BufferCompleted ||\n>> +\t\tevent.type_ == CameraEventType::Disconnect);\n> \n> I don't think you should include disconnect events here. This would\n> result in stop() removing disconnect events from the queue, while I\n> think they need to be handled through the normal event handling\n> mechanism.\n\nCan you elaborate? The point of this is to drop all the events related \nto the camera. Also note that there's the CameraRemoved event from \ncamera manager that is not dropped.\n\n>> +}\n>> +\n>> +std::vector PyCameraManager::getPyCameraEvents(std::shared_ptr camera)\n>> +{\n>> +\tMutexLocker guard(eventsMutex_);\n>> +\n>> +\tstd::vector pyevents;\n>> +\n>> +\t/* Get events related to the given camera */\n>> +\n>> +\tfor (const auto &event : events_) {\n>> +\t\tif (!isCameraSpecificEvent(event, camera))\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tPyCameraEvent pyev = convertEvent(event);\n>> +\t\tpyevents.push_back(pyev);\n>> +\t}\n>> +\n>> +\t/* Drop the events from the events_ list */\n>> +\n>> +\tevents_.erase(std::remove_if(events_.begin(), events_.end(),\n>> +\t\t[&camera](const CameraEvent &ev) {\n>> +\t\t\treturn isCameraSpecificEvent(ev, camera);\n>> +\t\t}),\n>> +\t\tevents_.end());\n> \n> I'd like to minmize the time spent holding the lock. You can move the\n> events specific to this camera to a different vector without converting\n> them (and merge the two loops if possible), and the convert the events\n> without holding the lock.\n> \n> Also, dropping elements in the middle of a vector is fairly inefficient.\n> I think you should use a list instead of a vector for events_, which\n> will allow you to efficiently move elements here with splice().\n\nI'll have a look, but these sound a bit like pointless optimizations. \nThis is called only when a camera is stopped, and we're talking about a \nfew events.\n\nTaking the conversion part outside the lock sounds feasible.\n\nIf we change the events from a vector to a list to speed up the code \nhere, where it doesn't matter, are we making the code slower in other \nparts where it matters?\n\n>> +\n>> +\tLOG(Python, Debug) << \"Get \" << pyevents.size() << \" camera events, \"\n> \n> s/Get/Got/ ?\n\nOk.\n\n>> +\t\t\t << events_.size() << \" unhandled events left\";\n>> +\n>> +\treturn pyevents;\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>> -\tpushRequest(req);\n>> -\twriteFd();\n>> +\tif (!bufferCompletedEventActive_)\n>> +\t\treturn;\n>> +\n>> +\tCameraEvent ev(CameraEventType::BufferCompleted, cam);\n>> +\tev.request_ = req;\n>> +\tev.fb_ = fb;\n>> +\n>> +\tpushEvent(ev);\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n>> +{\n>> +\tCameraEvent ev(CameraEventType::RequestCompleted, cam);\n>> +\tev.request_ = req;\n>> +\n>> +\tpushEvent(ev);\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n>> +{\n>> +\tCameraEvent ev(CameraEventType::Disconnect, cam);\n>> +\n>> +\tpushEvent(ev);\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n>> +{\n>> +\tCameraEvent ev(CameraEventType::CameraAdded, cam);\n>> +\n>> +\tpushEvent(ev);\n>> +}\n>> +\n>> +/* Note: Called from another thread */\n>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n>> +{\n>> +\tCameraEvent ev(CameraEventType::CameraRemoved, cam);\n>> +\n>> +\tpushEvent(ev);\n>> }\n>> \n>> void PyCameraManager::writeFd()\n>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()\n>> \treturn 0;\n>> }\n>> \n>> -void PyCameraManager::pushRequest(Request *req)\n>> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n>> {\n>> -\tMutexLocker guard(completedRequestsMutex_);\n>> -\tcompletedRequests_.push_back(req);\n>> +\tMutexLocker guard(eventsMutex_);\n>> +\tevents_.push_back(ev);\n>> +\n>> +\twriteFd();\n> \n> This can be done without holding the lock.\n\nThe writeFd()? Are you sure? We discussed this, I think, but... Well, \nI'd just rather be safe than sorry =).\n\n>> +\n>> +\tLOG(Python, Debug) << \"Queued events: \" << events_.size();\n> \n> I'm tempted to drop this, it will make the log very chatty. Have you\n> found it useful to debug applications ?\n\nFor development, yes. It's very easy to have very obscure issues with \nthe Python bindings. I think we can reduce the debug prints when things \nsettle down with the bindings.\n\n>> }\n>> \n>> -std::vector PyCameraManager::getCompletedRequests()\n>> +std::vector PyCameraManager::getEvents()\n>> {\n>> -\tstd::vector v;\n>> -\tMutexLocker guard(completedRequestsMutex_);\n>> -\tswap(v, completedRequests_);\n>> +\tstd::vector v;\n>> +\n>> +\tMutexLocker guard(eventsMutex_);\n>> +\tswap(v, events_);\n>> +\n>> \treturn v;\n>> }\n>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n>> index 3525057d..b313ba9b 100644\n>> --- a/src/py/libcamera/py_camera_manager.h\n>> +++ b/src/py/libcamera/py_camera_manager.h\n>> @@ -13,6 +13,47 @@\n>> \n>> using namespace libcamera;\n>> \n>> +enum class CameraEventType {\n>> +\tUndefined = 0,\n> \n> Unless we drop this (which would be my preference), maybe Invalid\n> instead of Undefined ? People don't like undefined behaviours :-D\n\nAnd they like invalid behavior? =)\n\nI'll see how I like it if I drop the Undefined value. Usually I like \nthose, it's good to be able to initialize things to 0.\n\n>> +\tCameraAdded,\n>> +\tCameraRemoved,\n>> +\tDisconnect,\n>> +\tRequestCompleted,\n>> +\tBufferCompleted,\n>> +};\n>> +\n>> +/*\n>> + * This event struct is used internally to queue the events we receive from\n>> + * other threads.\n>> + */\n>> +struct CameraEvent {\n>> +\tCameraEvent(CameraEventType type, std::shared_ptr camera)\n>> +\t\t: type_(type), camera_(camera)\n>> +\t{\n>> +\t}\n>> +\n>> +\tCameraEventType type_ = CameraEventType::Undefined;\n>> +\n>> +\tstd::shared_ptr camera_;\n>> +\n>> +\tRequest *request_ = nullptr;\n>> +\tFrameBuffer *fb_ = nullptr;\n>> +};\n>> +\n>> +/*\n>> + * This event struct is passed to Python. We need to use pybind11::object here\n>> + * instead of a C++ pointer so that we keep a ref to the Request, and a\n>> + * keep-alive from the camera to the camera manager.\n>> + */\n>> +struct PyCameraEvent {\n>> +\tCameraEventType type_ = CameraEventType::Undefined;\n>> +\n>> +\tpybind11::object camera_;\n>> +\n>> +\tpybind11::object request_;\n>> +\tpybind11::object fb_;\n>> +};\n>> +\n>> class PyCameraManager\n>> {\n>> public:\n>> @@ -26,20 +67,29 @@ public:\n>> \n>> \tint eventFd() const { return eventFd_.get(); }\n>> \n>> -\tstd::vector getReadyRequests();\n>> +\tstd::vector getReadyRequests(); /* DEPRECATED */\n>> +\tstd::vector getPyEvents();\n>> +\tstd::vector getPyCameraEvents(std::shared_ptr camera);\n>> \n>> -\tvoid handleRequestCompleted(Request *req);\n>> +\tvoid handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n>> +\tvoid handleRequestCompleted(std::shared_ptr cam, Request *req);\n>> +\tvoid handleDisconnected(std::shared_ptr cam);\n>> +\tvoid handleCameraAdded(std::shared_ptr cam);\n>> +\tvoid handleCameraRemoved(std::shared_ptr cam);\n>> \n>> +\tbool bufferCompletedEventActive_ = false;\n> \n> Missing blank line.\n\nYep.\n\n>> private:\n>> \tstd::unique_ptr cameraManager_;\n>> \n>> \tUniqueFD eventFd_;\n>> -\tlibcamera::Mutex completedRequestsMutex_;\n>> -\tstd::vector completedRequests_\n>> -\t\tLIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);\n>> +\tlibcamera::Mutex eventsMutex_;\n>> +\tstd::vector events_\n>> +\t\tLIBCAMERA_TSA_GUARDED_BY(eventsMutex_);\n>> \n>> \tvoid writeFd();\n>> \tint readFd();\n>> -\tvoid pushRequest(Request *req);\n>> -\tstd::vector getCompletedRequests();\n>> +\tvoid pushEvent(const CameraEvent &ev);\n>> +\tstd::vector getEvents();\n>> +\n>> +\tPyCameraEvent convertEvent(const CameraEvent &event);\n>> };\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index 6cd99919..b4f756d7 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -58,6 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n>> \t */\n>> \n>> +\tauto pyEvent = py::class_(m, \"Event\");\n>> \tauto pyCameraManager = py::class_(m, \"CameraManager\");\n>> \tauto pyCamera = py::class_(m, \"Camera\");\n>> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n>> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)\n>> \tm.def(\"log_set_level\", &logSetLevel);\n>> \n>> \t/* Classes */\n>> +\n>> +\tpy::enum_(pyEvent, \"Type\")\n>> +\t\t.value(\"Undefined\", CameraEventType::Undefined)\n>> +\t\t.value(\"CameraAdded\", CameraEventType::CameraAdded)\n>> +\t\t.value(\"CameraRemoved\", CameraEventType::CameraRemoved)\n>> +\t\t.value(\"Disconnect\", CameraEventType::Disconnect)\n>> +\t\t.value(\"RequestCompleted\", CameraEventType::RequestCompleted)\n>> +\t\t.value(\"BufferCompleted\", CameraEventType::BufferCompleted);\n>> +\n>> +\tpyEvent\n>> +\t\t.def_readonly(\"type\", &PyCameraEvent::type_)\n>> +\t\t.def_readonly(\"camera\", &PyCameraEvent::camera_)\n>> +\t\t.def_readonly(\"request\", &PyCameraEvent::request_)\n>> +\t\t.def_readonly(\"fb\", &PyCameraEvent::fb_);\n>> +\n>> \tpyCameraManager\n>> \t\t.def_static(\"singleton\", []() {\n>> \t\t\tstd::shared_ptr cm = gCameraManager.lock();\n>> @@ -107,7 +123,13 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n>> \n>> \t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n>> -\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n>> +\n>> +\t\t/* DEPRECATED */\n> \n> Is there any way to generate a warning at runtime when this gets used ?\n\nThe normal libcamera logging facilities, I guess? Well, we can also use \nthe standard python side logging, but I'm not sure how easy that is.\n\nIn any case, I'm not planning to keep it here forever =).\n\n> If we drop it (depending on the outcome of the discussion in the commit\n> message) that will be even easier :-)\n> \n>> +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests)\n>> +\n>> +\t\t.def(\"get_events\", &PyCameraManager::getPyEvents)\n>> +\n>> +\t\t.def_readwrite(\"buffer_completed_active\", &PyCameraManager::bufferCompletedEventActive_);\n>> \n>> \tpyCamera\n>> \t\t.def_property_readonly(\"id\", &Camera::id)\n>> @@ -131,7 +153,17 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t\tauto cm = gCameraManager.lock();\n>> \t\t\tASSERT(cm);\n>> \n>> -\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n>> +\t\t\tself.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n>> +\t\t\t\tcm->handleRequestCompleted(camera, req);\n>> +\t\t\t});\n>> +\n>> +\t\t\tself.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n>> +\t\t\t\tcm->handleBufferCompleted(camera, req, fb);\n>> +\t\t\t});\n>> +\n>> +\t\t\tself.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n>> +\t\t\t\tcm->handleDisconnected(camera);\n>> +\t\t\t});\n>> \n>> \t\t\tControlList controlList(self.controls());\n>> \n>> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t\tint ret = self.stop();\n>> \n>> \t\t\tself.requestCompleted.disconnect();\n>> +\t\t\tself.bufferCompleted.disconnect();\n>> +\t\t\tself.disconnected.disconnect();\n>> +\n>> +\t\t\tauto cm = gCameraManager.lock();\n>> +\t\t\tASSERT(cm);\n>> +\n>> +\t\t\tauto l = cm->getPyCameraEvents(self.shared_from_this());\n> \n> Please use a more descriptive variable name.\n\nOk.\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 6683CC3272\n\tfor ;\n\tFri, 26 Aug 2022 13:24:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D39A61FC0;\n\tFri, 26 Aug 2022 15:24:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F09F961FA2\n\tfor ;\n\tFri, 26 Aug 2022 15:24:50 +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 32ED22B3;\n\tFri, 26 Aug 2022 15:24:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661520292;\n\tbh=2+i9ncFBYyyRQj+v7STKE4VG+MCYaRnHrTLlYVsjH5U=;\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=0u6WJWtbrd2uaK+cf3Uv7Vli3TgGdLp78zY8ulnLXsexrS6UJpDYI1RbfOMmEVWkv\n\t5mF6ZCGVTmk7Bv1p5hTWfeR+6tgr7p12EA7cty/KIuJWMkAS4IgoigLWlx4SFnIbrA\n\tQS5Skl8sXiAluGtbfN85snE4feBtNG8H6zDvvrgrHPoKzOxOYHooxJhD/prC5IImBd\n\txIYR7Mmm92MbOKPBfCfiBtblVEjcoUnr74LhU7Vksu+BGNeEynYAAev60CRH6aofrQ\n\tF404a8zhqXlRJCRIe5Dgw/AcBi3Cu3SamyyoVSecDJPb32x9lk8ojc3AwFd0e5PLqm\n\tjVAM9+DgKcA6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661520290;\n\tbh=2+i9ncFBYyyRQj+v7STKE4VG+MCYaRnHrTLlYVsjH5U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=jfO90I7lGuU4kEmeEqT3e6qZ2XZyJ3S/d2B8ItJOkqj7Pud0TMAa0DetveN1sIbmH\n\t55Lv1Sego3kaaXSJduGi4o/BYsZEF0Qcy1Fh/FtXhLLsKcyS8+lq6ro/pU8fkkQXaL\n\tWXQtbWrwjAkFxKO6VZzYqc8whh+HbvufVCMoJHfM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jfO90I7l\"; dkim-atps=neutral","Message-ID":"<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>","Date":"Fri, 26 Aug 2022 16:24:46 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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":24798,"web_url":"https://patchwork.libcamera.org/comment/24798/","msgid":"","date":"2022-08-26T14:29:51","subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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 Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:\n> On 19/08/2022 01:00, Laurent Pinchart wrote:\n> > On Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:\n> >> At the moment the Python bindings only handle the requestCompleted\n> >> events. But we have others, bufferCompleted and disconnect from the\n> >> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> > \n> > And we will likely have error events in the not too distant future.\n> > \n> >> This patch makes all those events available to the users.\n> >>\n> >> The get_ready_requests() method is now deprecated, but available to keep\n> >> backward compatibility.\n> > \n> > Who are the users of get_ready_requests() that you are concern about,\n> > and when could it be removed ? Other changes in this series break the\n> > API (such as switching to exceptions), so all users will need to be\n> > updated, I'd rather remove get_ready_requests() at the same time.\n> \n> I'm fine with dropping get_ready_requests(). It is just so trivial to \n> support it that I kept it, which also helped converting the examples, as \n> I didn't have to convert everything in one commit.\n> \n> >> The new event handling works like get_ready_requests(), but instead of\n> >> returning only Requests from requestCompleted events, it returns all\n> >> event types using a new Event class.\n> >>\n> >> Additionally camera.stop() has been changed to return events for that\n> >> camera. This serves two purposes: first, it removes the requestCompleted\n> >> events from the event queue, thus preventing the old events being\n> >> returned when the camera is started again, and second, it allows the\n> >> user to process those events if it so wants.\n> >>\n> >> All other event types are always collected and returned, except\n> >> bufferCompleted which needs to be enabled by setting the\n> >> CameraManager.buffer_completed_active to True. This is to reduce the\n> >> overhead a bit. It is not clear if this is a necessary optimization or\n> >> not.\n> > \n> > As mentioned in the reply to the cover letter, we could have a more\n> > generic subscription mechanism, but I also think it may not make much\n> > sense to make subscription optional for all the other events, perhaps\n> > with the exception of a future metadata completion event.\n> > \n> > If only the buffer and metadata completion events are optional, should\n> > we have a per-camera subscription ?\n> \n> That's one option. I did try multiple different approaches, and \n> everything seems to have their drawbacks. When I get some time I'll \n> again try a few different things to refresh my memory.\n> \n> >> TODO: Documentation.\n> > \n> > This should be captured in the code. Or documentation could be written\n> > :-)\n> \n> I'll write it before merging, when we're happy with the model. I don't \n> want to write it until I'm happy with the code =).\n> \n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--\n> >> src/py/libcamera/py_camera_manager.h | 64 ++++++++-\n> >> src/py/libcamera/py_main.cpp | 45 +++++-\n> >> 3 files changed, 276 insertions(+), 24 deletions(-)\n> >>\n> >> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> >> index 3dd8668e..d1d63690 100644\n> >> --- a/src/py/libcamera/py_camera_manager.cpp\n> >> +++ b/src/py/libcamera/py_camera_manager.cpp\n> >> @@ -5,6 +5,7 @@\n> >> \n> >> #include \"py_camera_manager.h\"\n> >> \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()\n> >> \tif (ret)\n> >> \t\tthrow std::system_error(-ret, std::generic_category(),\n> >> \t\t\t\t\t\"Failed to start CameraManager\");\n> >> +\n> >> +\tcameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n> >> +\tcameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n> >> }\n> >> \n> >> PyCameraManager::~PyCameraManager()\n> >> {\n> >> \tLOG(Python, Debug) << \"~PyCameraManager()\";\n> >> +\n> >> +\tcameraManager_->cameraAdded.disconnect();\n> >> +\tcameraManager_->cameraRemoved.disconnect();\n> >> }\n> >> \n> >> py::list PyCameraManager::cameras()\n> >> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()\n> >> \treturn l;\n> >> }\n> >> \n> >> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)\n> >> +{\n> >> +\tPyCameraEvent pyevent;\n> >> +\n> >> +\tpyevent.type_ = event.type_;\n> >> +\n> >> +\t/*\n> >> +\t * We need to set a keep-alive here so that the camera keeps the\n> >> +\t * camera manager alive.\n> >> +\t */\n> >> +\tpy::object py_cm = py::cast(this);\n> >> +\tpy::object py_cam = py::cast(event.camera_);\n> >> +\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> >> +\n> >> +\tpyevent.camera_ = py_cam;\n> >> +\n> >> +\tswitch (event.type_) {\n> >> +\tcase CameraEventType::CameraAdded:\n> >> +\tcase CameraEventType::CameraRemoved:\n> >> +\tcase CameraEventType::Disconnect:\n> >> +\t\t/* No additional parameters to add */\n> >> +\t\tbreak;\n> >> +\n> >> +\tcase CameraEventType::BufferCompleted: {\n> >> +\t\tpyevent.request_ = py::cast(event.request_);\n> >> +\t\tpyevent.fb_ = py::cast(event.fb_);\n> >> +\t\tbreak;\n> >> +\t}\n> > \n> > No need for curly braces. But a blank line would be nice.\n> \n> Yep.\n> \n> >> +\tcase CameraEventType::RequestCompleted: {\n> >> +\t\tpyevent.request_ = py::cast(event.request_);\n> >> +\n> >> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> >> +\t\tpyevent.request_.dec_ref();\n> >> +\n> >> +\t\tbreak;\n> >> +\t}\n> >> +\tdefault:\n> >> +\t\tASSERT(false);\n> > \n> > What if we dropped the undefined event type ? You wouldn't need this\n> > then. It's best to write code that makes errors impossible.\n> \n> That wouldn't make errors impossible, it would just hide possible bugs \n> =). When something goes bad, event.type_ can contains an illegal value. \n> I think it's good to catch it here.\n\nCameraEventType is a scoped enum, so the compiler will not allow you\nsetting an invalid value. It will also enforce that all valid values\nhave a case in a switch statement. Of course if we have a buffer\noverflow somewhere that happens to overwrite a CameraEventType variable\nwith an invalid value we won't catch that, but I don't think the assert\nhere is a good answer to that kind of problem.\n\n> > I think I would also move all of this, except the py::cast(this) call,\n> > to a PyCameraEvent constructor that takes the CameraEvent and camera\n> > manager py::object.\n> \n> We also do the ref count magics for RequestCompleted. I'll think about \n> it, but I probably like it better if we keep the PyCameraEvent as a \n> dummy container.\n> \n> >> +\t}\n> >> +\n> >> +\treturn pyevent;\n> >> +}\n> >> +\n> >> +/* DEPRECATED */\n> >> std::vector PyCameraManager::getReadyRequests()\n> >> {\n> >> \tint ret = readFd();\n> >> @@ -70,21 +121,125 @@ std::vector PyCameraManager::getReadyRequests()\n> >> \n> >> \tstd::vector py_reqs;\n> >> \n> >> -\tfor (Request *request : getCompletedRequests()) {\n> >> -\t\tpy::object o = py::cast(request);\n> >> -\t\t/* Decrease the ref increased in Camera.queue_request() */\n> >> -\t\to.dec_ref();\n> >> -\t\tpy_reqs.push_back(o);\n> >> +\tfor (const auto &ev : getEvents()) {\n> >> +\t\tif (ev.type_ != CameraEventType::RequestCompleted)\n> >> +\t\t\tcontinue;\n> > \n> > Ouch, this will silently drop all other types of events. I'd really like\n> > to drop getReadyRequests().\n> \n> That's exactly as it was before.\n\nYes, but it doesn't make it nice :-)\n\n> >> +\n> >> +\t\tPyCameraEvent pyev = convertEvent(ev);\n> >> +\t\tpy_reqs.push_back(pyev.request_);\n> >> \t}\n> >> \n> >> \treturn py_reqs;\n> >> }\n> >> \n> >> +std::vector PyCameraManager::getPyEvents()\n> >> +{\n> >> +\tint ret = readFd();\n> >> +\n> >> +\tif (ret == EAGAIN) {\n> >> +\t\tLOG(Python, Debug) << \"No events\";\n> >> +\t\treturn {};\n> >> +\t}\n> >> +\n> >> +\tif (ret != 0)\n> >> +\t\tthrow std::system_error(ret, std::generic_category());\n> >> +\n> >> +\tstd::vector events = getEvents();\n> >> +\n> >> +\tLOG(Python, Debug) << \"Get \" << events.size() << \" events\";\n> > \n> > s/Get/Got/\n> \n> Ok.\n> \n> >> +\n> >> +\tstd::vector pyevents(events.size());\n> > \n> > You'll end up creating events.size() default-constructed elements here,\n> > only to overwrite them just after. I think calling .reserve() and using\n> > a std::back_inserter in the std::transform loop below would be more\n> > efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).\n> \n> True. And changing this allows me to add a more specific constructor for \n> PyCameraEvent.\n> \n> >> +\n> >> +\tstd::transform(events.begin(), events.end(), pyevents.begin(),\n> >> +\t\t [this](const CameraEvent &ev) {\n> >> +\t\t\t return convertEvent(ev);\n> >> +\t\t });\n> >> +\n> >> +\treturn pyevents;\n> >> +}\n> >> +\n> >> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr &camera)\n> >> +{\n> >> +\treturn event.camera_ == camera &&\n> >> +\t (event.type_ == CameraEventType::RequestCompleted ||\n> >> +\t\tevent.type_ == CameraEventType::BufferCompleted ||\n> >> +\t\tevent.type_ == CameraEventType::Disconnect);\n> > \n> > I don't think you should include disconnect events here. This would\n> > result in stop() removing disconnect events from the queue, while I\n> > think they need to be handled through the normal event handling\n> > mechanism.\n> \n> Can you elaborate? The point of this is to drop all the events related \n> to the camera. Also note that there's the CameraRemoved event from \n> camera manager that is not dropped.\n\nDropping the request completion and buffer completion events is fine.\nThe camera disconnection event should stay I think. Those are unrelated\nto a start/stop cycle. If an application handles disconnection events\noriginating from a camera, it will be very confusing (and very hard to\ndebug) if those events are randomly dropped because they happen to\narrive right at stop() time. Behaviour that depends on race conditions\nshould be minimized if not avoided completely.\n\n> >> +}\n> >> +\n> >> +std::vector PyCameraManager::getPyCameraEvents(std::shared_ptr camera)\n> >> +{\n> >> +\tMutexLocker guard(eventsMutex_);\n> >> +\n> >> +\tstd::vector pyevents;\n> >> +\n> >> +\t/* Get events related to the given camera */\n> >> +\n> >> +\tfor (const auto &event : events_) {\n> >> +\t\tif (!isCameraSpecificEvent(event, camera))\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tPyCameraEvent pyev = convertEvent(event);\n> >> +\t\tpyevents.push_back(pyev);\n> >> +\t}\n> >> +\n> >> +\t/* Drop the events from the events_ list */\n> >> +\n> >> +\tevents_.erase(std::remove_if(events_.begin(), events_.end(),\n> >> +\t\t[&camera](const CameraEvent &ev) {\n> >> +\t\t\treturn isCameraSpecificEvent(ev, camera);\n> >> +\t\t}),\n> >> +\t\tevents_.end());\n> > \n> > I'd like to minmize the time spent holding the lock. You can move the\n> > events specific to this camera to a different vector without converting\n> > them (and merge the two loops if possible), and the convert the events\n> > without holding the lock.\n> > \n> > Also, dropping elements in the middle of a vector is fairly inefficient.\n> > I think you should use a list instead of a vector for events_, which\n> > will allow you to efficiently move elements here with splice().\n> \n> I'll have a look, but these sound a bit like pointless optimizations. \n> This is called only when a camera is stopped, and we're talking about a \n> few events.\n> \n> Taking the conversion part outside the lock sounds feasible.\n> \n> If we change the events from a vector to a list to speed up the code \n> here, where it doesn't matter, are we making the code slower in other \n> parts where it matters?\n\nI don't think so (could be wrong of course).\n\n> >> +\n> >> +\tLOG(Python, Debug) << \"Get \" << pyevents.size() << \" camera events, \"\n> > \n> > s/Get/Got/ ?\n> \n> Ok.\n> \n> >> +\t\t\t << events_.size() << \" unhandled events left\";\n> >> +\n> >> +\treturn pyevents;\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> >> -\tpushRequest(req);\n> >> -\twriteFd();\n> >> +\tif (!bufferCompletedEventActive_)\n> >> +\t\treturn;\n> >> +\n> >> +\tCameraEvent ev(CameraEventType::BufferCompleted, cam);\n> >> +\tev.request_ = req;\n> >> +\tev.fb_ = fb;\n> >> +\n> >> +\tpushEvent(ev);\n> >> +}\n> >> +\n> >> +/* Note: Called from another thread */\n> >> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> >> +{\n> >> +\tCameraEvent ev(CameraEventType::RequestCompleted, cam);\n> >> +\tev.request_ = req;\n> >> +\n> >> +\tpushEvent(ev);\n> >> +}\n> >> +\n> >> +/* Note: Called from another thread */\n> >> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> >> +{\n> >> +\tCameraEvent ev(CameraEventType::Disconnect, cam);\n> >> +\n> >> +\tpushEvent(ev);\n> >> +}\n> >> +\n> >> +/* Note: Called from another thread */\n> >> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> >> +{\n> >> +\tCameraEvent ev(CameraEventType::CameraAdded, cam);\n> >> +\n> >> +\tpushEvent(ev);\n> >> +}\n> >> +\n> >> +/* Note: Called from another thread */\n> >> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> >> +{\n> >> +\tCameraEvent ev(CameraEventType::CameraRemoved, cam);\n> >> +\n> >> +\tpushEvent(ev);\n> >> }\n> >> \n> >> void PyCameraManager::writeFd()\n> >> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()\n> >> \treturn 0;\n> >> }\n> >> \n> >> -void PyCameraManager::pushRequest(Request *req)\n> >> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n> >> {\n> >> -\tMutexLocker guard(completedRequestsMutex_);\n> >> -\tcompletedRequests_.push_back(req);\n> >> +\tMutexLocker guard(eventsMutex_);\n> >> +\tevents_.push_back(ev);\n> >> +\n> >> +\twriteFd();\n> > \n> > This can be done without holding the lock.\n> \n> The writeFd()? Are you sure? We discussed this, I think, but... Well, \n> I'd just rather be safe than sorry =).\n\nwriteFd() only accesses eventFd_, and calls ::write() (plus logging a\nfatal message in case of error). The eventsMutex_ lock doesn't cover\nanything that would make eventFd_ invalid as far as I can tell, so\nthere's only the write() remaining. The corresponding read() is called\nwithout the lock held. Do you recall a reason why write would need to be\nprotected by the lock ?\n\n> >> +\n> >> +\tLOG(Python, Debug) << \"Queued events: \" << events_.size();\n> > \n> > I'm tempted to drop this, it will make the log very chatty. Have you\n> > found it useful to debug applications ?\n> \n> For development, yes. It's very easy to have very obscure issues with \n> the Python bindings. I think we can reduce the debug prints when things \n> settle down with the bindings.\n\nDo you mean for development of the bindings, or for development of\napplications (once the bindings are working fine) ?\n\n> >> }\n> >> \n> >> -std::vector PyCameraManager::getCompletedRequests()\n> >> +std::vector PyCameraManager::getEvents()\n> >> {\n> >> -\tstd::vector v;\n> >> -\tMutexLocker guard(completedRequestsMutex_);\n> >> -\tswap(v, completedRequests_);\n> >> +\tstd::vector v;\n> >> +\n> >> +\tMutexLocker guard(eventsMutex_);\n> >> +\tswap(v, events_);\n> >> +\n> >> \treturn v;\n> >> }\n> >> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h\n> >> index 3525057d..b313ba9b 100644\n> >> --- a/src/py/libcamera/py_camera_manager.h\n> >> +++ b/src/py/libcamera/py_camera_manager.h\n> >> @@ -13,6 +13,47 @@\n> >> \n> >> using namespace libcamera;\n> >> \n> >> +enum class CameraEventType {\n> >> +\tUndefined = 0,\n> > \n> > Unless we drop this (which would be my preference), maybe Invalid\n> > instead of Undefined ? People don't like undefined behaviours :-D\n> \n> And they like invalid behavior? =)\n> \n> I'll see how I like it if I drop the Undefined value. Usually I like \n> those, it's good to be able to initialize things to 0.\n\nI don't think I agree. I'd rather enforce that all variables of\nCameraEventType have a valid value to remove the possibility of bugs\nwhere a variable would be initialized to Undefined and mistakenly left\nat that value.\n\n> >> +\tCameraAdded,\n> >> +\tCameraRemoved,\n> >> +\tDisconnect,\n> >> +\tRequestCompleted,\n> >> +\tBufferCompleted,\n> >> +};\n> >> +\n> >> +/*\n> >> + * This event struct is used internally to queue the events we receive from\n> >> + * other threads.\n> >> + */\n> >> +struct CameraEvent {\n> >> +\tCameraEvent(CameraEventType type, std::shared_ptr camera)\n> >> +\t\t: type_(type), camera_(camera)\n> >> +\t{\n> >> +\t}\n> >> +\n> >> +\tCameraEventType type_ = CameraEventType::Undefined;\n> >> +\n> >> +\tstd::shared_ptr camera_;\n> >> +\n> >> +\tRequest *request_ = nullptr;\n> >> +\tFrameBuffer *fb_ = nullptr;\n> >> +};\n> >> +\n> >> +/*\n> >> + * This event struct is passed to Python. We need to use pybind11::object here\n> >> + * instead of a C++ pointer so that we keep a ref to the Request, and a\n> >> + * keep-alive from the camera to the camera manager.\n> >> + */\n> >> +struct PyCameraEvent {\n> >> +\tCameraEventType type_ = CameraEventType::Undefined;\n> >> +\n> >> +\tpybind11::object camera_;\n> >> +\n> >> +\tpybind11::object request_;\n> >> +\tpybind11::object fb_;\n> >> +};\n> >> +\n> >> class PyCameraManager\n> >> {\n> >> public:\n> >> @@ -26,20 +67,29 @@ public:\n> >> \n> >> \tint eventFd() const { return eventFd_.get(); }\n> >> \n> >> -\tstd::vector getReadyRequests();\n> >> +\tstd::vector getReadyRequests(); /* DEPRECATED */\n> >> +\tstd::vector getPyEvents();\n> >> +\tstd::vector getPyCameraEvents(std::shared_ptr camera);\n> >> \n> >> -\tvoid handleRequestCompleted(Request *req);\n> >> +\tvoid handleBufferCompleted(std::shared_ptr cam, Request *req, FrameBuffer *fb);\n> >> +\tvoid handleRequestCompleted(std::shared_ptr cam, Request *req);\n> >> +\tvoid handleDisconnected(std::shared_ptr cam);\n> >> +\tvoid handleCameraAdded(std::shared_ptr cam);\n> >> +\tvoid handleCameraRemoved(std::shared_ptr cam);\n> >> \n> >> +\tbool bufferCompletedEventActive_ = false;\n> > \n> > Missing blank line.\n> \n> Yep.\n> \n> >> private:\n> >> \tstd::unique_ptr cameraManager_;\n> >> \n> >> \tUniqueFD eventFd_;\n> >> -\tlibcamera::Mutex completedRequestsMutex_;\n> >> -\tstd::vector completedRequests_\n> >> -\t\tLIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);\n> >> +\tlibcamera::Mutex eventsMutex_;\n> >> +\tstd::vector events_\n> >> +\t\tLIBCAMERA_TSA_GUARDED_BY(eventsMutex_);\n> >> \n> >> \tvoid writeFd();\n> >> \tint readFd();\n> >> -\tvoid pushRequest(Request *req);\n> >> -\tstd::vector getCompletedRequests();\n> >> +\tvoid pushEvent(const CameraEvent &ev);\n> >> +\tstd::vector getEvents();\n> >> +\n> >> +\tPyCameraEvent convertEvent(const CameraEvent &event);\n> >> };\n> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> >> index 6cd99919..b4f756d7 100644\n> >> --- a/src/py/libcamera/py_main.cpp\n> >> +++ b/src/py/libcamera/py_main.cpp\n> >> @@ -58,6 +58,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings\n> >> \t */\n> >> \n> >> +\tauto pyEvent = py::class_(m, \"Event\");\n> >> \tauto pyCameraManager = py::class_(m, \"CameraManager\");\n> >> \tauto pyCamera = py::class_(m, \"Camera\");\n> >> \tauto pyCameraConfiguration = py::class_(m, \"CameraConfiguration\");\n> >> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \tm.def(\"log_set_level\", &logSetLevel);\n> >> \n> >> \t/* Classes */\n> >> +\n> >> +\tpy::enum_(pyEvent, \"Type\")\n> >> +\t\t.value(\"Undefined\", CameraEventType::Undefined)\n> >> +\t\t.value(\"CameraAdded\", CameraEventType::CameraAdded)\n> >> +\t\t.value(\"CameraRemoved\", CameraEventType::CameraRemoved)\n> >> +\t\t.value(\"Disconnect\", CameraEventType::Disconnect)\n> >> +\t\t.value(\"RequestCompleted\", CameraEventType::RequestCompleted)\n> >> +\t\t.value(\"BufferCompleted\", CameraEventType::BufferCompleted);\n> >> +\n> >> +\tpyEvent\n> >> +\t\t.def_readonly(\"type\", &PyCameraEvent::type_)\n> >> +\t\t.def_readonly(\"camera\", &PyCameraEvent::camera_)\n> >> +\t\t.def_readonly(\"request\", &PyCameraEvent::request_)\n> >> +\t\t.def_readonly(\"fb\", &PyCameraEvent::fb_);\n> >> +\n> >> \tpyCameraManager\n> >> \t\t.def_static(\"singleton\", []() {\n> >> \t\t\tstd::shared_ptr cm = gCameraManager.lock();\n> >> @@ -107,7 +123,13 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t.def_property_readonly(\"cameras\", &PyCameraManager::cameras)\n> >> \n> >> \t\t.def_property_readonly(\"event_fd\", &PyCameraManager::eventFd)\n> >> -\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests);\n> >> +\n> >> +\t\t/* DEPRECATED */\n> > \n> > Is there any way to generate a warning at runtime when this gets used ?\n> \n> The normal libcamera logging facilities, I guess? Well, we can also use \n> the standard python side logging, but I'm not sure how easy that is.\n> \n> In any case, I'm not planning to keep it here forever =).\n> \n> > If we drop it (depending on the outcome of the discussion in the commit\n> > message) that will be even easier :-)\n> > \n> >> +\t\t.def(\"get_ready_requests\", &PyCameraManager::getReadyRequests)\n> >> +\n> >> +\t\t.def(\"get_events\", &PyCameraManager::getPyEvents)\n> >> +\n> >> +\t\t.def_readwrite(\"buffer_completed_active\", &PyCameraManager::bufferCompletedEventActive_);\n> >> \n> >> \tpyCamera\n> >> \t\t.def_property_readonly(\"id\", &Camera::id)\n> >> @@ -131,7 +153,17 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t\tauto cm = gCameraManager.lock();\n> >> \t\t\tASSERT(cm);\n> >> \n> >> -\t\t\tself.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);\n> >> +\t\t\tself.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {\n> >> +\t\t\t\tcm->handleRequestCompleted(camera, req);\n> >> +\t\t\t});\n> >> +\n> >> +\t\t\tself.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {\n> >> +\t\t\t\tcm->handleBufferCompleted(camera, req, fb);\n> >> +\t\t\t});\n> >> +\n> >> +\t\t\tself.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {\n> >> +\t\t\t\tcm->handleDisconnected(camera);\n> >> +\t\t\t});\n> >> \n> >> \t\t\tControlList controlList(self.controls());\n> >> \n> >> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t\tint ret = self.stop();\n> >> \n> >> \t\t\tself.requestCompleted.disconnect();\n> >> +\t\t\tself.bufferCompleted.disconnect();\n> >> +\t\t\tself.disconnected.disconnect();\n> >> +\n> >> +\t\t\tauto cm = gCameraManager.lock();\n> >> +\t\t\tASSERT(cm);\n> >> +\n> >> +\t\t\tauto l = cm->getPyCameraEvents(self.shared_from_this());\n> > \n> > Please use a more descriptive variable name.\n> \n> Ok.","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 92106C3272\n\tfor ;\n\tFri, 26 Aug 2022 14:30:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0570961FC0;\n\tFri, 26 Aug 2022 16:30:00 +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 4536B61FA2\n\tfor ;\n\tFri, 26 Aug 2022 16:29:59 +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 6D0662B3;\n\tFri, 26 Aug 2022 16:29:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661524200;\n\tbh=m+jHbDxJFPXx4vBv2Fane0p1YkkcKoxAhnN5ZJPETQI=;\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=ZOYhA+BkasypmoY1PtUIaQqjQ77nOMuQxAl1VwuwrTIOSYB5YnNbFh39EQCN2xUp3\n\tDeuPVfUQsqVsdj6lVTLHlmvtF1wLTIeOA5MOsaxhsmuOY4B2CdjtU96ve02CPA8DM6\n\tfEgUaVKfL/ijfU9Aoz64DlNa4A03vPycrm/a14OlOaTTq4LqAp/8pTKgenzFLxl7N7\n\tB4ZWmi0QHCVj29MqyBh+20Z/lJmnG+4IWh1jaysYcgKhqD7E9crZQ2H9Yp/682ndXW\n\tv79m93kfDluUTY+15p/BGbxNl2fNw1OdAtG5mF1r9iNP+g0ARPHGHDWOaPhdFWVg3e\n\tj180vpPXhNVSA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661524198;\n\tbh=m+jHbDxJFPXx4vBv2Fane0p1YkkcKoxAhnN5ZJPETQI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fWTmMu8KOfAkVEnKcBi6FjKyDgMBwqyNIVE5S/zjchzhkzhIy/2iTs0leWeEqFHM9\n\tV5kKa2zWyvL4RSB7Qhz+jhECtspwtZ1vhF6b4KQX7J/bG7Mwdu/NbCIbvmsmB0lLqE\n\tNk65HxxH4NaM21uwvssiVbJhnRwjtZrskJBsccUo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fWTmMu8K\"; dkim-atps=neutral","Date":"Fri, 26 Aug 2022 17:29:51 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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":24813,"web_url":"https://patchwork.libcamera.org/comment/24813/","msgid":"<35d2f035-dd3d-a82d-f2b1-b429b2826716@ideasonboard.com>","date":"2022-08-29T10:19:54","subject":"Re: [libcamera-devel] [PATCH v3 11/17] py: New event handling","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"Hi,\n\nOn 26/08/2022 17:29, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:\n>> On 19/08/2022 01:00, Laurent Pinchart wrote:\n>>> On Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:\n>>>> At the moment the Python bindings only handle the requestCompleted\n>>>> events. But we have others, bufferCompleted and disconnect from the\n>>>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n>>>\n>>> And we will likely have error events in the not too distant future.\n>>>\n>>>> This patch makes all those events available to the users.\n>>>>\n>>>> The get_ready_requests() method is now deprecated, but available to keep\n>>>> backward compatibility.\n>>>\n>>> Who are the users of get_ready_requests() that you are concern about,\n>>> and when could it be removed ? Other changes in this series break the\n>>> API (such as switching to exceptions), so all users will need to be\n>>> updated, I'd rather remove get_ready_requests() at the same time.\n>>\n>> I'm fine with dropping get_ready_requests(). It is just so trivial to\n>> support it that I kept it, which also helped converting the examples, as\n>> I didn't have to convert everything in one commit.\n>>\n>>>> The new event handling works like get_ready_requests(), but instead of\n>>>> returning only Requests from requestCompleted events, it returns all\n>>>> event types using a new Event class.\n>>>>\n>>>> Additionally camera.stop() has been changed to return events for that\n>>>> camera. This serves two purposes: first, it removes the requestCompleted\n>>>> events from the event queue, thus preventing the old events being\n>>>> returned when the camera is started again, and second, it allows the\n>>>> user to process those events if it so wants.\n>>>>\n>>>> All other event types are always collected and returned, except\n>>>> bufferCompleted which needs to be enabled by setting the\n>>>> CameraManager.buffer_completed_active to True. This is to reduce the\n>>>> overhead a bit. It is not clear if this is a necessary optimization or\n>>>> not.\n>>>\n>>> As mentioned in the reply to the cover letter, we could have a more\n>>> generic subscription mechanism, but I also think it may not make much\n>>> sense to make subscription optional for all the other events, perhaps\n>>> with the exception of a future metadata completion event.\n>>>\n>>> If only the buffer and metadata completion events are optional, should\n>>> we have a per-camera subscription ?\n>>\n>> That's one option. I did try multiple different approaches, and\n>> everything seems to have their drawbacks. When I get some time I'll\n>> again try a few different things to refresh my memory.\n>>\n>>>> TODO: Documentation.\n>>>\n>>> This should be captured in the code. Or documentation could be written\n>>> :-)\n>>\n>> I'll write it before merging, when we're happy with the model. I don't\n>> want to write it until I'm happy with the code =).\n>>\n>>>> Signed-off-by: Tomi Valkeinen \n>>>> ---\n>>>> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--\n>>>> src/py/libcamera/py_camera_manager.h | 64 ++++++++-\n>>>> src/py/libcamera/py_main.cpp | 45 +++++-\n>>>> 3 files changed, 276 insertions(+), 24 deletions(-)\n>>>>\n>>>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n>>>> index 3dd8668e..d1d63690 100644\n>>>> --- a/src/py/libcamera/py_camera_manager.cpp\n>>>> +++ b/src/py/libcamera/py_camera_manager.cpp\n>>>> @@ -5,6 +5,7 @@\n>>>> \n>>>> #include \"py_camera_manager.h\"\n>>>> \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> #include \n>>>> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()\n>>>> \tif (ret)\n>>>> \t\tthrow std::system_error(-ret, std::generic_category(),\n>>>> \t\t\t\t\t\"Failed to start CameraManager\");\n>>>> +\n>>>> +\tcameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n>>>> +\tcameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n>>>> }\n>>>> \n>>>> PyCameraManager::~PyCameraManager()\n>>>> {\n>>>> \tLOG(Python, Debug) << \"~PyCameraManager()\";\n>>>> +\n>>>> +\tcameraManager_->cameraAdded.disconnect();\n>>>> +\tcameraManager_->cameraRemoved.disconnect();\n>>>> }\n>>>> \n>>>> py::list PyCameraManager::cameras()\n>>>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()\n>>>> \treturn l;\n>>>> }\n>>>> \n>>>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)\n>>>> +{\n>>>> +\tPyCameraEvent pyevent;\n>>>> +\n>>>> +\tpyevent.type_ = event.type_;\n>>>> +\n>>>> +\t/*\n>>>> +\t * We need to set a keep-alive here so that the camera keeps the\n>>>> +\t * camera manager alive.\n>>>> +\t */\n>>>> +\tpy::object py_cm = py::cast(this);\n>>>> +\tpy::object py_cam = py::cast(event.camera_);\n>>>> +\tpy::detail::keep_alive_impl(py_cam, py_cm);\n>>>> +\n>>>> +\tpyevent.camera_ = py_cam;\n>>>> +\n>>>> +\tswitch (event.type_) {\n>>>> +\tcase CameraEventType::CameraAdded:\n>>>> +\tcase CameraEventType::CameraRemoved:\n>>>> +\tcase CameraEventType::Disconnect:\n>>>> +\t\t/* No additional parameters to add */\n>>>> +\t\tbreak;\n>>>> +\n>>>> +\tcase CameraEventType::BufferCompleted: {\n>>>> +\t\tpyevent.request_ = py::cast(event.request_);\n>>>> +\t\tpyevent.fb_ = py::cast(event.fb_);\n>>>> +\t\tbreak;\n>>>> +\t}\n>>>\n>>> No need for curly braces. But a blank line would be nice.\n>>\n>> Yep.\n>>\n>>>> +\tcase CameraEventType::RequestCompleted: {\n>>>> +\t\tpyevent.request_ = py::cast(event.request_);\n>>>> +\n>>>> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n>>>> +\t\tpyevent.request_.dec_ref();\n>>>> +\n>>>> +\t\tbreak;\n>>>> +\t}\n>>>> +\tdefault:\n>>>> +\t\tASSERT(false);\n>>>\n>>> What if we dropped the undefined event type ? You wouldn't need this\n>>> then. It's best to write code that makes errors impossible.\n>>\n>> That wouldn't make errors impossible, it would just hide possible bugs\n>> =). When something goes bad, event.type_ can contains an illegal value.\n>> I think it's good to catch it here.\n> \n> CameraEventType is a scoped enum, so the compiler will not allow you\n> setting an invalid value. It will also enforce that all valid values\n> have a case in a switch statement. Of course if we have a buffer\n> overflow somewhere that happens to overwrite a CameraEventType variable\n> with an invalid value we won't catch that, but I don't think the assert\n> here is a good answer to that kind of problem.\n\nI think it's fine to drop the Undefined value, although we do expose the \nenum to Python, which makes things more complex to consider. However, I \ncan't right now think of a case where Python would need Undefined value, \nand it can always use None to represent undefined.\n\n>>> I think I would also move all of this, except the py::cast(this) call,\n>>> to a PyCameraEvent constructor that takes the CameraEvent and camera\n>>> manager py::object.\n>>\n>> We also do the ref count magics for RequestCompleted. I'll think about\n>> it, but I probably like it better if we keep the PyCameraEvent as a\n>> dummy container.\n>>\n>>>> +\t}\n>>>> +\n>>>> +\treturn pyevent;\n>>>> +}\n>>>> +\n>>>> +/* DEPRECATED */\n>>>> std::vector PyCameraManager::getReadyRequests()\n>>>> {\n>>>> \tint ret = readFd();\n>>>> @@ -70,21 +121,125 @@ std::vector PyCameraManager::getReadyRequests()\n>>>> \n>>>> \tstd::vector py_reqs;\n>>>> \n>>>> -\tfor (Request *request : getCompletedRequests()) {\n>>>> -\t\tpy::object o = py::cast(request);\n>>>> -\t\t/* Decrease the ref increased in Camera.queue_request() */\n>>>> -\t\to.dec_ref();\n>>>> -\t\tpy_reqs.push_back(o);\n>>>> +\tfor (const auto &ev : getEvents()) {\n>>>> +\t\tif (ev.type_ != CameraEventType::RequestCompleted)\n>>>> +\t\t\tcontinue;\n>>>\n>>> Ouch, this will silently drop all other types of events. I'd really like\n>>> to drop getReadyRequests().\n>>\n>> That's exactly as it was before.\n> \n> Yes, but it doesn't make it nice :-)\n> \n>>>> +\n>>>> +\t\tPyCameraEvent pyev = convertEvent(ev);\n>>>> +\t\tpy_reqs.push_back(pyev.request_);\n>>>> \t}\n>>>> \n>>>> \treturn py_reqs;\n>>>> }\n>>>> \n>>>> +std::vector PyCameraManager::getPyEvents()\n>>>> +{\n>>>> +\tint ret = readFd();\n>>>> +\n>>>> +\tif (ret == EAGAIN) {\n>>>> +\t\tLOG(Python, Debug) << \"No events\";\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\tif (ret != 0)\n>>>> +\t\tthrow std::system_error(ret, std::generic_category());\n>>>> +\n>>>> +\tstd::vector events = getEvents();\n>>>> +\n>>>> +\tLOG(Python, Debug) << \"Get \" << events.size() << \" events\";\n>>>\n>>> s/Get/Got/\n>>\n>> Ok.\n>>\n>>>> +\n>>>> +\tstd::vector pyevents(events.size());\n>>>\n>>> You'll end up creating events.size() default-constructed elements here,\n>>> only to overwrite them just after. I think calling .reserve() and using\n>>> a std::back_inserter in the std::transform loop below would be more\n>>> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).\n>>\n>> True. And changing this allows me to add a more specific constructor for\n>> PyCameraEvent.\n>>\n>>>> +\n>>>> +\tstd::transform(events.begin(), events.end(), pyevents.begin(),\n>>>> +\t\t [this](const CameraEvent &ev) {\n>>>> +\t\t\t return convertEvent(ev);\n>>>> +\t\t });\n>>>> +\n>>>> +\treturn pyevents;\n>>>> +}\n>>>> +\n>>>> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr &camera)\n>>>> +{\n>>>> +\treturn event.camera_ == camera &&\n>>>> +\t (event.type_ == CameraEventType::RequestCompleted ||\n>>>> +\t\tevent.type_ == CameraEventType::BufferCompleted ||\n>>>> +\t\tevent.type_ == CameraEventType::Disconnect);\n>>>\n>>> I don't think you should include disconnect events here. This would\n>>> result in stop() removing disconnect events from the queue, while I\n>>> think they need to be handled through the normal event handling\n>>> mechanism.\n>>\n>> Can you elaborate? The point of this is to drop all the events related\n>> to the camera. Also note that there's the CameraRemoved event from\n>> camera manager that is not dropped.\n> \n> Dropping the request completion and buffer completion events is fine.\n> The camera disconnection event should stay I think. Those are unrelated\n> to a start/stop cycle. If an application handles disconnection events\n> originating from a camera, it will be very confusing (and very hard to\n> debug) if those events are randomly dropped because they happen to\n> arrive right at stop() time. Behaviour that depends on race conditions\n> should be minimized if not avoided completely.\n\nBut we're not dropping the events. We are returning them from stop() and \nthe app can handle them.\n\nThe idea here is that we subscribe the events at start(), unsubscribe at \nstop(), and after stop() there will be no events originating from the \ncamera.\n\nI can't really say if not handling Disconnect here would be a real \nproblem, but the current behavior feels logical and clear to me.\n\n>>>> +}\n>>>> +\n>>>> +std::vector PyCameraManager::getPyCameraEvents(std::shared_ptr camera)\n>>>> +{\n>>>> +\tMutexLocker guard(eventsMutex_);\n>>>> +\n>>>> +\tstd::vector pyevents;\n>>>> +\n>>>> +\t/* Get events related to the given camera */\n>>>> +\n>>>> +\tfor (const auto &event : events_) {\n>>>> +\t\tif (!isCameraSpecificEvent(event, camera))\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\tPyCameraEvent pyev = convertEvent(event);\n>>>> +\t\tpyevents.push_back(pyev);\n>>>> +\t}\n>>>> +\n>>>> +\t/* Drop the events from the events_ list */\n>>>> +\n>>>> +\tevents_.erase(std::remove_if(events_.begin(), events_.end(),\n>>>> +\t\t[&camera](const CameraEvent &ev) {\n>>>> +\t\t\treturn isCameraSpecificEvent(ev, camera);\n>>>> +\t\t}),\n>>>> +\t\tevents_.end());\n>>>\n>>> I'd like to minmize the time spent holding the lock. You can move the\n>>> events specific to this camera to a different vector without converting\n>>> them (and merge the two loops if possible), and the convert the events\n>>> without holding the lock.\n>>>\n>>> Also, dropping elements in the middle of a vector is fairly inefficient.\n>>> I think you should use a list instead of a vector for events_, which\n>>> will allow you to efficiently move elements here with splice().\n>>\n>> I'll have a look, but these sound a bit like pointless optimizations.\n>> This is called only when a camera is stopped, and we're talking about a\n>> few events.\n>>\n>> Taking the conversion part outside the lock sounds feasible.\n>>\n>> If we change the events from a vector to a list to speed up the code\n>> here, where it doesn't matter, are we making the code slower in other\n>> parts where it matters?\n> \n> I don't think so (could be wrong of course).\n\n99.9% of the time we're just appending to the events vector when we're \ndoing a camera capture. With list, that would mean a heap alloc for \nevery event, wouldn't it?\n\nThis would (probably) look much nicer with a list, and the code here \ndoes bug me a bit =). But I don't think it's worth using a list \nconsidering the worse performance in the main use case.\n\nHowever, I can move the conversion to Python events to be outside the \nlock. That might be somewhat costly operation.\n\nAlso, I believe it would be possible to do the filtering and erasing \nwith a single loop, instead of first going through the events vector twice.\n\n>>>> +\n>>>> +\tLOG(Python, Debug) << \"Get \" << pyevents.size() << \" camera events, \"\n>>>\n>>> s/Get/Got/ ?\n>>\n>> Ok.\n>>\n>>>> +\t\t\t << events_.size() << \" unhandled events left\";\n>>>> +\n>>>> +\treturn pyevents;\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>>>> -\tpushRequest(req);\n>>>> -\twriteFd();\n>>>> +\tif (!bufferCompletedEventActive_)\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tCameraEvent ev(CameraEventType::BufferCompleted, cam);\n>>>> +\tev.request_ = req;\n>>>> +\tev.fb_ = fb;\n>>>> +\n>>>> +\tpushEvent(ev);\n>>>> +}\n>>>> +\n>>>> +/* Note: Called from another thread */\n>>>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n>>>> +{\n>>>> +\tCameraEvent ev(CameraEventType::RequestCompleted, cam);\n>>>> +\tev.request_ = req;\n>>>> +\n>>>> +\tpushEvent(ev);\n>>>> +}\n>>>> +\n>>>> +/* Note: Called from another thread */\n>>>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n>>>> +{\n>>>> +\tCameraEvent ev(CameraEventType::Disconnect, cam);\n>>>> +\n>>>> +\tpushEvent(ev);\n>>>> +}\n>>>> +\n>>>> +/* Note: Called from another thread */\n>>>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n>>>> +{\n>>>> +\tCameraEvent ev(CameraEventType::CameraAdded, cam);\n>>>> +\n>>>> +\tpushEvent(ev);\n>>>> +}\n>>>> +\n>>>> +/* Note: Called from another thread */\n>>>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n>>>> +{\n>>>> +\tCameraEvent ev(CameraEventType::CameraRemoved, cam);\n>>>> +\n>>>> +\tpushEvent(ev);\n>>>> }\n>>>> \n>>>> void PyCameraManager::writeFd()\n>>>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> -void PyCameraManager::pushRequest(Request *req)\n>>>> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n>>>> {\n>>>> -\tMutexLocker guard(completedRequestsMutex_);\n>>>> -\tcompletedRequests_.push_back(req);\n>>>> +\tMutexLocker guard(eventsMutex_);\n>>>> +\tevents_.push_back(ev);\n>>>> +\n>>>> +\twriteFd();\n>>>\n>>> This can be done without holding the lock.\n>>\n>> The writeFd()? Are you sure? We discussed this, I think, but... Well,\n>> I'd just rather be safe than sorry =).\n> \n> writeFd() only accesses eventFd_, and calls ::write() (plus logging a\n> fatal message in case of error). The eventsMutex_ lock doesn't cover\n> anything that would make eventFd_ invalid as far as I can tell, so\n> there's only the write() remaining. The corresponding read() is called\n> without the lock held. Do you recall a reason why write would need to be\n> protected by the lock ?\n\nI think you're right. I can't think of a reason to keep the lock during \nwriteFd().\n\n>>>> +\n>>>> +\tLOG(Python, Debug) << \"Queued events: \" << events_.size();\n>>>\n>>> I'm tempted to drop this, it will make the log very chatty. Have you\n>>> found it useful to debug applications ?\n>>\n>> For development, yes. It's very easy to have very obscure issues with\n>> the Python bindings. I think we can reduce the debug prints when things\n>> settle down with the bindings.\n> \n> Do you mean for development of the bindings, or for development of\n> applications (once the bindings are working fine) ?\n\nDevelopment of the bindings.\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 862B3C0DA4\n\tfor ;\n\tMon, 29 Aug 2022 10:19:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA56861FC0;\n\tMon, 29 Aug 2022 12:19:58 +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 C48F561FBB\n\tfor ;\n\tMon, 29 Aug 2022 12:19:57 +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 AF6198BD;\n\tMon, 29 Aug 2022 12:19:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661768398;\n\tbh=clJl4cib9UmeVyi7puS68j6/qK5GAfZ/l555qrZkocE=;\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=MzuL17vG2iL2EHSxM4pPpbWg20qAq9gNqqzGCbe6sOHeLzYuCOZAdIFzIcpMoEy6A\n\tGCrLakO1EPU2UJcWc/euI634541G5YAN3J4ZaXmHBFOJO1yTiAlyLLffgqt/UlJ+v/\n\t9X87Chgh93EZOcuJntbX842skeKeVl+jyQBdxTygZXcknHm94lWMs7so6N5plow8ln\n\tV6wguCRJmF4cAGTGuepzo/TJLQlfG/534B6ZdPTW80QadQo4Eo8cAwXj02IxG3HkvE\n\tTlypah1RP15l9DVwcBwPP3eiGhpzD+WTZA5k5udolWg4S3rEFM6ZoFE+bc8d2Bi+hK\n\tMTsVPpBKEU7KQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661768397;\n\tbh=clJl4cib9UmeVyi7puS68j6/qK5GAfZ/l555qrZkocE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=kh5eH+K7C1TknfzN7EXqByENCFZd9HLqyvNmw51ZCS6P9yXlja5qzTdmcAuTvDbJI\n\tLMWlvS8fjtb8gI0Yie24g6+74m0Gj1LYAYPwp/iz5WOSkSB4lpOaK+A918YNznIM7X\n\tBaVLN93ULPVCJf/C3CVAhFzkk9YorANp1y+uN3qk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kh5eH+K7\"; dkim-atps=neutral","Message-ID":"<35d2f035-dd3d-a82d-f2b1-b429b2826716@ideasonboard.com>","Date":"Mon, 29 Aug 2022 13:19:54 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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":24844,"web_url":"https://patchwork.libcamera.org/comment/24844/","msgid":"","date":"2022-08-30T19:27:07","subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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, Aug 29, 2022 at 01:19:54PM +0300, Tomi Valkeinen wrote:\n> On 26/08/2022 17:29, Laurent Pinchart wrote:\n> > On Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:\n> >> On 19/08/2022 01:00, Laurent Pinchart wrote:\n> >>> On Fri, Jul 01, 2022 at 11:45:15AM +0300, Tomi Valkeinen wrote:\n> >>>> At the moment the Python bindings only handle the requestCompleted\n> >>>> events. But we have others, bufferCompleted and disconnect from the\n> >>>> Camera, and cameraAdded and cameraRemoved from the CameraManager.\n> >>>\n> >>> And we will likely have error events in the not too distant future.\n> >>>\n> >>>> This patch makes all those events available to the users.\n> >>>>\n> >>>> The get_ready_requests() method is now deprecated, but available to keep\n> >>>> backward compatibility.\n> >>>\n> >>> Who are the users of get_ready_requests() that you are concern about,\n> >>> and when could it be removed ? Other changes in this series break the\n> >>> API (such as switching to exceptions), so all users will need to be\n> >>> updated, I'd rather remove get_ready_requests() at the same time.\n> >>\n> >> I'm fine with dropping get_ready_requests(). It is just so trivial to\n> >> support it that I kept it, which also helped converting the examples, as\n> >> I didn't have to convert everything in one commit.\n> >>\n> >>>> The new event handling works like get_ready_requests(), but instead of\n> >>>> returning only Requests from requestCompleted events, it returns all\n> >>>> event types using a new Event class.\n> >>>>\n> >>>> Additionally camera.stop() has been changed to return events for that\n> >>>> camera. This serves two purposes: first, it removes the requestCompleted\n> >>>> events from the event queue, thus preventing the old events being\n> >>>> returned when the camera is started again, and second, it allows the\n> >>>> user to process those events if it so wants.\n> >>>>\n> >>>> All other event types are always collected and returned, except\n> >>>> bufferCompleted which needs to be enabled by setting the\n> >>>> CameraManager.buffer_completed_active to True. This is to reduce the\n> >>>> overhead a bit. It is not clear if this is a necessary optimization or\n> >>>> not.\n> >>>\n> >>> As mentioned in the reply to the cover letter, we could have a more\n> >>> generic subscription mechanism, but I also think it may not make much\n> >>> sense to make subscription optional for all the other events, perhaps\n> >>> with the exception of a future metadata completion event.\n> >>>\n> >>> If only the buffer and metadata completion events are optional, should\n> >>> we have a per-camera subscription ?\n> >>\n> >> That's one option. I did try multiple different approaches, and\n> >> everything seems to have their drawbacks. When I get some time I'll\n> >> again try a few different things to refresh my memory.\n> >>\n> >>>> TODO: Documentation.\n> >>>\n> >>> This should be captured in the code. Or documentation could be written\n> >>> :-)\n> >>\n> >> I'll write it before merging, when we're happy with the model. I don't\n> >> want to write it until I'm happy with the code =).\n> >>\n> >>>> Signed-off-by: Tomi Valkeinen \n> >>>> ---\n> >>>> src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--\n> >>>> src/py/libcamera/py_camera_manager.h | 64 ++++++++-\n> >>>> src/py/libcamera/py_main.cpp | 45 +++++-\n> >>>> 3 files changed, 276 insertions(+), 24 deletions(-)\n> >>>>\n> >>>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp\n> >>>> index 3dd8668e..d1d63690 100644\n> >>>> --- a/src/py/libcamera/py_camera_manager.cpp\n> >>>> +++ b/src/py/libcamera/py_camera_manager.cpp\n> >>>> @@ -5,6 +5,7 @@\n> >>>> \n> >>>> #include \"py_camera_manager.h\"\n> >>>> \n> >>>> +#include \n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> @@ -33,11 +34,17 @@ PyCameraManager::PyCameraManager()\n> >>>> \tif (ret)\n> >>>> \t\tthrow std::system_error(-ret, std::generic_category(),\n> >>>> \t\t\t\t\t\"Failed to start CameraManager\");\n> >>>> +\n> >>>> +\tcameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);\n> >>>> +\tcameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);\n> >>>> }\n> >>>> \n> >>>> PyCameraManager::~PyCameraManager()\n> >>>> {\n> >>>> \tLOG(Python, Debug) << \"~PyCameraManager()\";\n> >>>> +\n> >>>> +\tcameraManager_->cameraAdded.disconnect();\n> >>>> +\tcameraManager_->cameraRemoved.disconnect();\n> >>>> }\n> >>>> \n> >>>> py::list PyCameraManager::cameras()\n> >>>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()\n> >>>> \treturn l;\n> >>>> }\n> >>>> \n> >>>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)\n> >>>> +{\n> >>>> +\tPyCameraEvent pyevent;\n> >>>> +\n> >>>> +\tpyevent.type_ = event.type_;\n> >>>> +\n> >>>> +\t/*\n> >>>> +\t * We need to set a keep-alive here so that the camera keeps the\n> >>>> +\t * camera manager alive.\n> >>>> +\t */\n> >>>> +\tpy::object py_cm = py::cast(this);\n> >>>> +\tpy::object py_cam = py::cast(event.camera_);\n> >>>> +\tpy::detail::keep_alive_impl(py_cam, py_cm);\n> >>>> +\n> >>>> +\tpyevent.camera_ = py_cam;\n> >>>> +\n> >>>> +\tswitch (event.type_) {\n> >>>> +\tcase CameraEventType::CameraAdded:\n> >>>> +\tcase CameraEventType::CameraRemoved:\n> >>>> +\tcase CameraEventType::Disconnect:\n> >>>> +\t\t/* No additional parameters to add */\n> >>>> +\t\tbreak;\n> >>>> +\n> >>>> +\tcase CameraEventType::BufferCompleted: {\n> >>>> +\t\tpyevent.request_ = py::cast(event.request_);\n> >>>> +\t\tpyevent.fb_ = py::cast(event.fb_);\n> >>>> +\t\tbreak;\n> >>>> +\t}\n> >>>\n> >>> No need for curly braces. But a blank line would be nice.\n> >>\n> >> Yep.\n> >>\n> >>>> +\tcase CameraEventType::RequestCompleted: {\n> >>>> +\t\tpyevent.request_ = py::cast(event.request_);\n> >>>> +\n> >>>> +\t\t/* Decrease the ref increased in Camera.queue_request() */\n> >>>> +\t\tpyevent.request_.dec_ref();\n> >>>> +\n> >>>> +\t\tbreak;\n> >>>> +\t}\n> >>>> +\tdefault:\n> >>>> +\t\tASSERT(false);\n> >>>\n> >>> What if we dropped the undefined event type ? You wouldn't need this\n> >>> then. It's best to write code that makes errors impossible.\n> >>\n> >> That wouldn't make errors impossible, it would just hide possible bugs\n> >> =). When something goes bad, event.type_ can contains an illegal value.\n> >> I think it's good to catch it here.\n> > \n> > CameraEventType is a scoped enum, so the compiler will not allow you\n> > setting an invalid value. It will also enforce that all valid values\n> > have a case in a switch statement. Of course if we have a buffer\n> > overflow somewhere that happens to overwrite a CameraEventType variable\n> > with an invalid value we won't catch that, but I don't think the assert\n> > here is a good answer to that kind of problem.\n> \n> I think it's fine to drop the Undefined value, although we do expose the \n> enum to Python, which makes things more complex to consider. However, I \n> can't right now think of a case where Python would need Undefined value, \n> and it can always use None to represent undefined.\n> \n> >>> I think I would also move all of this, except the py::cast(this) call,\n> >>> to a PyCameraEvent constructor that takes the CameraEvent and camera\n> >>> manager py::object.\n> >>\n> >> We also do the ref count magics for RequestCompleted. I'll think about\n> >> it, but I probably like it better if we keep the PyCameraEvent as a\n> >> dummy container.\n> >>\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn pyevent;\n> >>>> +}\n> >>>> +\n> >>>> +/* DEPRECATED */\n> >>>> std::vector PyCameraManager::getReadyRequests()\n> >>>> {\n> >>>> \tint ret = readFd();\n> >>>> @@ -70,21 +121,125 @@ std::vector PyCameraManager::getReadyRequests()\n> >>>> \n> >>>> \tstd::vector py_reqs;\n> >>>> \n> >>>> -\tfor (Request *request : getCompletedRequests()) {\n> >>>> -\t\tpy::object o = py::cast(request);\n> >>>> -\t\t/* Decrease the ref increased in Camera.queue_request() */\n> >>>> -\t\to.dec_ref();\n> >>>> -\t\tpy_reqs.push_back(o);\n> >>>> +\tfor (const auto &ev : getEvents()) {\n> >>>> +\t\tif (ev.type_ != CameraEventType::RequestCompleted)\n> >>>> +\t\t\tcontinue;\n> >>>\n> >>> Ouch, this will silently drop all other types of events. I'd really like\n> >>> to drop getReadyRequests().\n> >>\n> >> That's exactly as it was before.\n> > \n> > Yes, but it doesn't make it nice :-)\n> > \n> >>>> +\n> >>>> +\t\tPyCameraEvent pyev = convertEvent(ev);\n> >>>> +\t\tpy_reqs.push_back(pyev.request_);\n> >>>> \t}\n> >>>> \n> >>>> \treturn py_reqs;\n> >>>> }\n> >>>> \n> >>>> +std::vector PyCameraManager::getPyEvents()\n> >>>> +{\n> >>>> +\tint ret = readFd();\n> >>>> +\n> >>>> +\tif (ret == EAGAIN) {\n> >>>> +\t\tLOG(Python, Debug) << \"No events\";\n> >>>> +\t\treturn {};\n> >>>> +\t}\n> >>>> +\n> >>>> +\tif (ret != 0)\n> >>>> +\t\tthrow std::system_error(ret, std::generic_category());\n> >>>> +\n> >>>> +\tstd::vector events = getEvents();\n> >>>> +\n> >>>> +\tLOG(Python, Debug) << \"Get \" << events.size() << \" events\";\n> >>>\n> >>> s/Get/Got/\n> >>\n> >> Ok.\n> >>\n> >>>> +\n> >>>> +\tstd::vector pyevents(events.size());\n> >>>\n> >>> You'll end up creating events.size() default-constructed elements here,\n> >>> only to overwrite them just after. I think calling .reserve() and using\n> >>> a std::back_inserter in the std::transform loop below would be more\n> >>> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).\n> >>\n> >> True. And changing this allows me to add a more specific constructor for\n> >> PyCameraEvent.\n> >>\n> >>>> +\n> >>>> +\tstd::transform(events.begin(), events.end(), pyevents.begin(),\n> >>>> +\t\t [this](const CameraEvent &ev) {\n> >>>> +\t\t\t return convertEvent(ev);\n> >>>> +\t\t });\n> >>>> +\n> >>>> +\treturn pyevents;\n> >>>> +}\n> >>>> +\n> >>>> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr &camera)\n> >>>> +{\n> >>>> +\treturn event.camera_ == camera &&\n> >>>> +\t (event.type_ == CameraEventType::RequestCompleted ||\n> >>>> +\t\tevent.type_ == CameraEventType::BufferCompleted ||\n> >>>> +\t\tevent.type_ == CameraEventType::Disconnect);\n> >>>\n> >>> I don't think you should include disconnect events here. This would\n> >>> result in stop() removing disconnect events from the queue, while I\n> >>> think they need to be handled through the normal event handling\n> >>> mechanism.\n> >>\n> >> Can you elaborate? The point of this is to drop all the events related\n> >> to the camera. Also note that there's the CameraRemoved event from\n> >> camera manager that is not dropped.\n> > \n> > Dropping the request completion and buffer completion events is fine.\n> > The camera disconnection event should stay I think. Those are unrelated\n> > to a start/stop cycle. If an application handles disconnection events\n> > originating from a camera, it will be very confusing (and very hard to\n> > debug) if those events are randomly dropped because they happen to\n> > arrive right at stop() time. Behaviour that depends on race conditions\n> > should be minimized if not avoided completely.\n> \n> But we're not dropping the events. We are returning them from stop() and \n> the app can handle them.\n\nThat's right, I forgot about that for a moment.\n\n> The idea here is that we subscribe the events at start(), unsubscribe at \n> stop(), and after stop() there will be no events originating from the \n> camera.\n\nstart() and stop() are meant to start and stop video capture. The\ndisconnect event can occur while the camera is stopped. If you don't\nwant to handle the disconnect event when the camera is stopped, then it\nshouldn't be handled at all and not be exposed to Python applications.\n\n> I can't really say if not handling Disconnect here would be a real \n> problem, but the current behavior feels logical and clear to me.\n> \n> >>>> +}\n> >>>> +\n> >>>> +std::vector PyCameraManager::getPyCameraEvents(std::shared_ptr camera)\n> >>>> +{\n> >>>> +\tMutexLocker guard(eventsMutex_);\n> >>>> +\n> >>>> +\tstd::vector pyevents;\n> >>>> +\n> >>>> +\t/* Get events related to the given camera */\n> >>>> +\n> >>>> +\tfor (const auto &event : events_) {\n> >>>> +\t\tif (!isCameraSpecificEvent(event, camera))\n> >>>> +\t\t\tcontinue;\n> >>>> +\n> >>>> +\t\tPyCameraEvent pyev = convertEvent(event);\n> >>>> +\t\tpyevents.push_back(pyev);\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Drop the events from the events_ list */\n> >>>> +\n> >>>> +\tevents_.erase(std::remove_if(events_.begin(), events_.end(),\n> >>>> +\t\t[&camera](const CameraEvent &ev) {\n> >>>> +\t\t\treturn isCameraSpecificEvent(ev, camera);\n> >>>> +\t\t}),\n> >>>> +\t\tevents_.end());\n> >>>\n> >>> I'd like to minmize the time spent holding the lock. You can move the\n> >>> events specific to this camera to a different vector without converting\n> >>> them (and merge the two loops if possible), and the convert the events\n> >>> without holding the lock.\n> >>>\n> >>> Also, dropping elements in the middle of a vector is fairly inefficient.\n> >>> I think you should use a list instead of a vector for events_, which\n> >>> will allow you to efficiently move elements here with splice().\n> >>\n> >> I'll have a look, but these sound a bit like pointless optimizations.\n> >> This is called only when a camera is stopped, and we're talking about a\n> >> few events.\n> >>\n> >> Taking the conversion part outside the lock sounds feasible.\n> >>\n> >> If we change the events from a vector to a list to speed up the code\n> >> here, where it doesn't matter, are we making the code slower in other\n> >> parts where it matters?\n> > \n> > I don't think so (could be wrong of course).\n> \n> 99.9% of the time we're just appending to the events vector when we're \n> doing a camera capture. With list, that would mean a heap alloc for \n> every event, wouldn't it?\n\nBut appending to a vector may mean reallocating the whole vector if the\nsize is too small :-) Implementations don't shrink the storage size\nthough, so after some time the storage should get big enough, but once\nin a while you risk full reallocation.\n\n> This would (probably) look much nicer with a list, and the code here \n> does bug me a bit =). But I don't think it's worth using a list \n> considering the worse performance in the main use case.\n> \n> However, I can move the conversion to Python events to be outside the \n> lock. That might be somewhat costly operation.\n\nI'd like that if possible, yes.\n\n> Also, I believe it would be possible to do the filtering and erasing \n> with a single loop, instead of first going through the events vector twice.\n> \n> >>>> +\n> >>>> +\tLOG(Python, Debug) << \"Get \" << pyevents.size() << \" camera events, \"\n> >>>\n> >>> s/Get/Got/ ?\n> >>\n> >> Ok.\n> >>\n> >>>> +\t\t\t << events_.size() << \" unhandled events left\";\n> >>>> +\n> >>>> +\treturn pyevents;\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> >>>> -\tpushRequest(req);\n> >>>> -\twriteFd();\n> >>>> +\tif (!bufferCompletedEventActive_)\n> >>>> +\t\treturn;\n> >>>> +\n> >>>> +\tCameraEvent ev(CameraEventType::BufferCompleted, cam);\n> >>>> +\tev.request_ = req;\n> >>>> +\tev.fb_ = fb;\n> >>>> +\n> >>>> +\tpushEvent(ev);\n> >>>> +}\n> >>>> +\n> >>>> +/* Note: Called from another thread */\n> >>>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr cam, Request *req)\n> >>>> +{\n> >>>> +\tCameraEvent ev(CameraEventType::RequestCompleted, cam);\n> >>>> +\tev.request_ = req;\n> >>>> +\n> >>>> +\tpushEvent(ev);\n> >>>> +}\n> >>>> +\n> >>>> +/* Note: Called from another thread */\n> >>>> +void PyCameraManager::handleDisconnected(std::shared_ptr cam)\n> >>>> +{\n> >>>> +\tCameraEvent ev(CameraEventType::Disconnect, cam);\n> >>>> +\n> >>>> +\tpushEvent(ev);\n> >>>> +}\n> >>>> +\n> >>>> +/* Note: Called from another thread */\n> >>>> +void PyCameraManager::handleCameraAdded(std::shared_ptr cam)\n> >>>> +{\n> >>>> +\tCameraEvent ev(CameraEventType::CameraAdded, cam);\n> >>>> +\n> >>>> +\tpushEvent(ev);\n> >>>> +}\n> >>>> +\n> >>>> +/* Note: Called from another thread */\n> >>>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr cam)\n> >>>> +{\n> >>>> +\tCameraEvent ev(CameraEventType::CameraRemoved, cam);\n> >>>> +\n> >>>> +\tpushEvent(ev);\n> >>>> }\n> >>>> \n> >>>> void PyCameraManager::writeFd()\n> >>>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()\n> >>>> \treturn 0;\n> >>>> }\n> >>>> \n> >>>> -void PyCameraManager::pushRequest(Request *req)\n> >>>> +void PyCameraManager::pushEvent(const CameraEvent &ev)\n> >>>> {\n> >>>> -\tMutexLocker guard(completedRequestsMutex_);\n> >>>> -\tcompletedRequests_.push_back(req);\n> >>>> +\tMutexLocker guard(eventsMutex_);\n> >>>> +\tevents_.push_back(ev);\n> >>>> +\n> >>>> +\twriteFd();\n> >>>\n> >>> This can be done without holding the lock.\n> >>\n> >> The writeFd()? Are you sure? We discussed this, I think, but... Well,\n> >> I'd just rather be safe than sorry =).\n> > \n> > writeFd() only accesses eventFd_, and calls ::write() (plus logging a\n> > fatal message in case of error). The eventsMutex_ lock doesn't cover\n> > anything that would make eventFd_ invalid as far as I can tell, so\n> > there's only the write() remaining. The corresponding read() is called\n> > without the lock held. Do you recall a reason why write would need to be\n> > protected by the lock ?\n> \n> I think you're right. I can't think of a reason to keep the lock during \n> writeFd().\n> \n> >>>> +\n> >>>> +\tLOG(Python, Debug) << \"Queued events: \" << events_.size();\n> >>>\n> >>> I'm tempted to drop this, it will make the log very chatty. Have you\n> >>> found it useful to debug applications ?\n> >>\n> >> For development, yes. It's very easy to have very obscure issues with\n> >> the Python bindings. I think we can reduce the debug prints when things\n> >> settle down with the bindings.\n> > \n> > Do you mean for development of the bindings, or for development of\n> > applications (once the bindings are working fine) ?\n> \n> Development of the bindings.","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 95127C3272\n\tfor ;\n\tTue, 30 Aug 2022 19:27:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10D7061FBD;\n\tTue, 30 Aug 2022 21:27:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1024961F9C\n\tfor ;\n\tTue, 30 Aug 2022 21:27:19 +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 3E76625B;\n\tTue, 30 Aug 2022 21:27:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661887641;\n\tbh=4iby78erYSekjd+iEfOFbe9lg0Df9ZSLW1felY7TQtQ=;\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=gP3W+qVk+5W2NIkfqXX0V8Yq8SNuB1okuxQHiD4hkZhXvRa9tgN76/29XjB31YV/A\n\tkGDXjwV4+0w/QK4M6u7jeWLi1tOr/x/kNPcuijJE5nJbJxHREQY+0e3jiHGOFbHPGy\n\tYwfQzlo6Z8XafyS1cu6gCkiGRCycAWB4iyYMI5W8RuppXDXZuPPBlvrewTbReH+nbs\n\tT93Ky2lN5RYZCNykwUpJIkUF4ddCKSe4G7wWx5w69b6ljgM8VufRB83JTBXxJuH7YH\n\tpzSVEuN10l/562sWJ0+WkIumFu/A6l77/dKoZxpVVVQvp/Iy169wBJnRvYyJ6a+o/n\n\tSuJRUJxm786hw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661887638;\n\tbh=4iby78erYSekjd+iEfOFbe9lg0Df9ZSLW1felY7TQtQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xzx8/ApqJMrXty+8fNlDqV80vHKgydzy6HomrCS03T5jy6JxI9n1S1dEpRrG5yD+M\n\tkdVfoYN1n//ldBmsBP41Q69Pe8iKZqwWskhUPq1H1PoDWhlvlDZ1Nz4A0yoid2O44I\n\tbEzLYcbnyshWN0V/0so/l15zwNJ6iF59ciQR6xSM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Xzx8/Apq\"; dkim-atps=neutral","Date":"Tue, 30 Aug 2022 22:27:07 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t<059e7409-374d-b8f3-f10b-ff28f44628f2@ideasonboard.com>\n\t\n\t<35d2f035-dd3d-a82d-f2b1-b429b2826716@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<35d2f035-dd3d-a82d-f2b1-b429b2826716@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 11/17] 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\" "}}]